Merge pull request #77 from itk-dev/feature/semver-filter-upper-bound

fix: SemverFilter must respect value2 with directional operators

Author: turegjorup

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

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;

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)'],
             ])
         ;
     }

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<string, array{string, string, int}>
      */