diff --git a/CHANGELOG.md b/CHANGELOG.md index c6b041b..317c56d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- [#77](https://github.com/itk-dev/devops_itksites/pull/77) + Fix SemverFilter: respect value2 with directional operators + - [#75](https://github.com/itk-dev/devops_itksites/pull/75) Add semver-aware filter on every admin version column, make version column semver sortable diff --git a/src/Form/Type/Admin/SemverFilter.php b/src/Form/Type/Admin/SemverFilter.php index b8f092d..2ffe120 100644 --- a/src/Form/Type/Admin/SemverFilter.php +++ b/src/Form/Type/Admin/SemverFilter.php @@ -39,9 +39,23 @@ public function apply(QueryBuilder $queryBuilder, FilterDataDto $filterDataDto, return; } - $isRange = SemverFilterType::COMPARISON_BETWEEN === $comparison - || SemverFilterType::COMPARISON_BETWEEN_EXCLUSIVE === $comparison; - if ($isRange && '' === $value2) { + // When the user fills the upper-bound field together with a directional + // operator (>, >=, <, <=), treat the filter as a range. The operator's + // inclusivity carries over (>= / <= → inclusive, > / < → exclusive), + // so the natural reading "from X to Y" works regardless of which side + // the user picked. Range operators (between / between_exclusive) + // require both values and behave the same way. = and != are exact + // matches, so value2 is ignored. + $isExplicitRange = in_array($comparison, [SemverFilterType::COMPARISON_BETWEEN, SemverFilterType::COMPARISON_BETWEEN_EXCLUSIVE], true); + $autoRange = '' !== $value2 && in_array($comparison, [ + SemverFilterType::COMPARISON_GT, + SemverFilterType::COMPARISON_GTE, + SemverFilterType::COMPARISON_LT, + SemverFilterType::COMPARISON_LTE, + ], true); + $isRange = $isExplicitRange || $autoRange; + + if ($isExplicitRange && '' === $value2) { return; } @@ -70,9 +84,19 @@ public function apply(QueryBuilder $queryBuilder, FilterDataDto $filterDataDto, // references its argument five times) and PDO would complain about // bound-variable count. if ($isRange) { - [$lowerOp, $upperOp] = SemverFilterType::COMPARISON_BETWEEN === $comparison - ? ['>=', '<='] - : ['>', '<']; + $inclusive = in_array($comparison, [ + SemverFilterType::COMPARISON_BETWEEN, + SemverFilterType::COMPARISON_GTE, + SemverFilterType::COMPARISON_LTE, + ], true); + [$lowerOp, $upperOp] = $inclusive ? ['>=', '<='] : ['>', '<']; + + // Sort the two values numerically so the user can enter them in any + // order — "< 11.3.0" with value2 = "10.0.0" still produces a sane + // range, not an unsatisfiable WHERE. + $a = self::toSemverNumeric($value); + $b = self::toSemverNumeric($value2); + [$min, $max] = $a <= $b ? [$a, $b] : [$b, $a]; $queryBuilder ->andWhere(sprintf( @@ -83,8 +107,8 @@ public function apply(QueryBuilder $queryBuilder, FilterDataDto $filterDataDto, $parameter, $upperOp, )) - ->setParameter($parameter.'_min', self::toSemverNumeric($value)) - ->setParameter($parameter.'_max', self::toSemverNumeric($value2)) + ->setParameter($parameter.'_min', $min) + ->setParameter($parameter.'_max', $max) ; return; diff --git a/src/Form/Type/Admin/SemverFilterType.php b/src/Form/Type/Admin/SemverFilterType.php index 939df16..4f6006c 100644 --- a/src/Form/Type/Admin/SemverFilterType.php +++ b/src/Form/Type/Admin/SemverFilterType.php @@ -46,7 +46,7 @@ public function buildForm(FormBuilderInterface $builder, array $options): void ]) ->add('value2', TextType::class, [ 'required' => false, - 'attr' => ['placeholder' => 'upper bound (when between)'], + 'attr' => ['placeholder' => 'upper bound (optional, makes it a range)'], ]) ; } diff --git a/tests/Form/Type/Admin/SemverFilterTest.php b/tests/Form/Type/Admin/SemverFilterTest.php index e95c721..57b1c80 100644 --- a/tests/Form/Type/Admin/SemverFilterTest.php +++ b/tests/Form/Type/Admin/SemverFilterTest.php @@ -102,6 +102,60 @@ public function testApplyShortCircuitsRangeOnInvalidUpperBound(): void self::assertStringContainsString('1 = 0', $qb->getDQL(), 'Invalid upper-bound input must produce a 0-row query'); } + /** + * Regression: `>= 11.0.0` with value2=11.2.0 must produce a range, + * not silently ignore the upper bound (which previously let 11.3.5 + * leak through). + */ + public function testApplyAutoPromotesGteWithValue2ToInclusiveRange(): void + { + $qb = $this->makeInstallationQueryBuilder(); + + $this->apply($qb, '>=', '11.0.0', '11.2.0'); + + $dql = $qb->getDQL(); + self::assertStringContainsString('SEMVER_NUMERIC(entity.frameworkVersion) >= :frameworkVersion_0_min', $dql); + self::assertStringContainsString('SEMVER_NUMERIC(entity.frameworkVersion) <= :frameworkVersion_0_max', $dql); + self::assertSame(11_000_000_000_000, $qb->getParameter('frameworkVersion_0_min')?->getValue()); + self::assertSame(11_000_200_000_000, $qb->getParameter('frameworkVersion_0_max')?->getValue()); + } + + public function testApplyAutoPromotesGtWithValue2ToExclusiveRange(): void + { + $qb = $this->makeInstallationQueryBuilder(); + + $this->apply($qb, '>', '11.0.0', '11.2.0'); + + $dql = $qb->getDQL(); + self::assertStringContainsString('SEMVER_NUMERIC(entity.frameworkVersion) > :frameworkVersion_0_min', $dql); + self::assertStringContainsString('SEMVER_NUMERIC(entity.frameworkVersion) < :frameworkVersion_0_max', $dql); + } + + /** + * Auto-promote sorts the two values so users can enter them in either + * order — "< 11.2.0" with value2 = "11.0.0" still means [11.0.0, 11.2.0]. + */ + public function testApplyAutoPromoteSortsValuesNumerically(): void + { + $qb = $this->makeInstallationQueryBuilder(); + + $this->apply($qb, '<', '11.2.0', '11.0.0'); + + self::assertSame(11_000_000_000_000, $qb->getParameter('frameworkVersion_0_min')?->getValue()); + self::assertSame(11_000_200_000_000, $qb->getParameter('frameworkVersion_0_max')?->getValue()); + } + + public function testApplyIgnoresValue2ForEqualsOperator(): void + { + $qb = $this->makeInstallationQueryBuilder(); + + $this->apply($qb, '=', '11.0.0', '11.2.0'); + + $dql = $qb->getDQL(); + self::assertStringNotContainsString('_min', $dql, '= operator with value2 must remain a single-value match'); + self::assertSame(11_000_000_000_000, $qb->getParameter('frameworkVersion_0')?->getValue()); + } + /** * @return iterable */