no issue - fixes version compare algorithm

makinacorpus · Apr 24, 2024 · 1012b84 · 1012b84
1 parent e887af1
commit 1012b84
Show file tree

Hide file tree

Showing 4 changed files with 168 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 * [feature] ⭐️ Add `MakinaCorpus\QueryBuilder\BridgeFactory` for creating
   standalone connections.
+* [fix] Better version compare algorithm, with less erroneous edge cases.
 * [deprecation] ⚠️ Renamed `MakinaCorpus\QueryBuilder\Bridge\Doctrine\DoctrineQueryBuilder`
   to `MakinaCorpus\QueryBuilder\Bridge\Doctrine\DoctrineBridge`.
 * [deprecation] ⚠️ Renamed `MakinaCorpus\QueryBuilder\Bridge\Pdo\PdoQueryBuilder`

diff --git a/src/Vendor.php b/src/Vendor.php
@@ -5,6 +5,7 @@
 namespace MakinaCorpus\QueryBuilder;
 
 use MakinaCorpus\QueryBuilder\Error\QueryBuilderError;
+use MakinaCorpus\QueryBuilder\Version\Version;
 
 /**
  * RDMBS identification.
@@ -40,17 +41,7 @@ public static function versionNormalize(string $version): string
      */
     public static function versionCompare(string $userGiven, string $serverVersion, string $operator): bool
     {
-        $userGiven = self::versionNormalize($userGiven);
-        $serverVersion = self::versionNormalize($serverVersion);
-
-        return match ($operator) {
-            '<' => 0 > \version_compare($userGiven, $serverVersion),
-            '<=' => 0 >= \version_compare($userGiven, $serverVersion),
-            '=' => 0 === \version_compare($userGiven, $serverVersion),
-            '>=' => 0 <= \version_compare($userGiven, $serverVersion),
-            '>' => 0 < \version_compare($userGiven, $serverVersion),
-            default => throw new QueryBuilderError("Version comparison operator must be one of '<', '<=', '=', '>=', '>'"),
-        };
+        return (new Version($userGiven))->compare($serverVersion, $operator);
     }
 
     /**

diff --git a/src/Version/Version.php b/src/Version/Version.php
@@ -0,0 +1,79 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MakinaCorpus\QueryBuilder\Version;
+
+use MakinaCorpus\QueryBuilder\Error\QueryBuilderError;
+
+/**
+ * RDMBS identification.
+ *
+ * One could have used an enum here, but we want server identification
+ * string to be arbitrary in various places in the code, to allow this
+ * API to remain extensible.
+ */
+class Version
+{
+    private int $major = 0;
+    private int $minor = 0;
+    private int $patch = 0;
+    private int $precision = 3;
+
+    public function __construct(string $version)
+    {
+        $matches = [];
+        if (!\preg_match('@(\d+)(\.(\d+)|)(\.(\d+)|)@', $version, $matches)) {
+            throw new QueryBuilderError(\sprintf("Version '%s', is not in 'x.y.z' semantic format", $version));
+        }
+
+        $this->major = (int) $matches[1];
+
+        if (isset($matches[3]) && $matches[3] !== '') {
+            $this->minor = (int) $matches[3];
+            if (isset($matches[5]) && $matches[5] !== '') {
+                $this->patch = (int) $matches[5];
+                $this->precision = 3;
+            } else {
+                $this->precision = 2;
+            }
+        } else {
+            $this->precision = 1;
+        }
+    }
+
+    /**
+     * Versions cannot be simply
+     */
+    private function compareTo(string|Version $other): int
+    {
+        $other = \is_string($other) ? new Version($other) : $other;
+
+        $precision = \min($this->precision, $other->precision);
+
+        if (1 === $precision || $this->major !== $other->major) {
+            return $this->major - $other->major;
+        }
+        if (2 === $precision || $this->minor !== $other->minor) {
+            return $this->minor - $other->minor;
+        }
+        return $this->patch - $other->patch;
+    }
+
+    /**
+     * Is the given version OP this version?
+     */
+    public function compare(string|Version $other, string $operator = '='): bool
+    {
+        $compare = $this->compareTo($other);
+
+        return match ($operator) {
+            '<' => 0 < $compare,
+            '<=' => 0 <= $compare,
+            '=' => 0 === $compare,
+            '>=' => 0 >= $compare,
+            '>' => 0 > $compare,
+            default => throw new QueryBuilderError("Version comparison operator must be one of '<', '<=', '=', '>=', '>'"),
+        };
+    }
+}
diff --git a/tests/VendorTest.php b/tests/VendorTest.php
@@ -0,0 +1,86 @@
+<?php
+
+declare(strict_types=1);
+
+namespace MakinaCorpus\QueryBuilder\Tests;
+
+use MakinaCorpus\QueryBuilder\Vendor;
+use PHPUnit\Framework\TestCase;
+
+class VendorTest extends TestCase
+{
+    public function testVersionCompareLessThan(): void
+    {
+        self::assertFalse(Vendor::versionCompare('5.6', '5.7.44', '<'));
+        self::assertFalse(Vendor::versionCompare('5.7', '5.7.44', '<'));
+        self::assertTrue(Vendor::versionCompare('8.0', '5.7.44', '<'));
+
+        self::assertFalse(Vendor::versionCompare('5.6.0', '5.7.44', '<'));
+        self::assertFalse(Vendor::versionCompare('5.7.0', '5.7.44', '<'));
+        self::assertTrue(Vendor::versionCompare('8.0.0', '5.7.44', '<'));
+
+        self::assertFalse(Vendor::versionCompare('5.7.43', '5.7.44', '<'));
+        self::assertFalse(Vendor::versionCompare('5.7.44', '5.7.44', '<'));
+        self::assertTrue(Vendor::versionCompare('5.7.45', '5.7.44', '<'));
+    }
+
+    public function testVersionCompareLessOrEqualThan(): void
+    {
+        self::assertFalse(Vendor::versionCompare('5.6', '5.7.44', '<='));
+        self::assertTrue(Vendor::versionCompare('5.7', '5.7.44', '<='));
+        self::assertTrue(Vendor::versionCompare('8.0', '5.7.44', '<='));
+
+        self::assertFalse(Vendor::versionCompare('5.6.0', '5.7.44', '<='));
+        self::assertFalse(Vendor::versionCompare('5.7.0', '5.7.44', '<='));
+        self::assertTrue(Vendor::versionCompare('8.0.0', '5.7.44', '<='));
+
+        self::assertFalse(Vendor::versionCompare('5.7.43', '5.7.44', '<='));
+        self::assertTrue(Vendor::versionCompare('5.7.44', '5.7.44', '<='));
+        self::assertTrue(Vendor::versionCompare('5.7.45', '5.7.44', '<='));
+    }
+
+    public function testVersionCompareEqualto(): void
+    {
+        self::assertFalse(Vendor::versionCompare('5.6', '5.7.44', '='));
+        self::assertTrue(Vendor::versionCompare('5.7', '5.7.44', '='));
+        self::assertFalse(Vendor::versionCompare('8.0', '5.7.44', '='));
+
+        self::assertFalse(Vendor::versionCompare('5.6.0', '5.7.44', '='));
+        self::assertFalse(Vendor::versionCompare('5.7.0', '5.7.44', '='));
+        self::assertFalse(Vendor::versionCompare('8.0.0', '5.7.44', '='));
+
+        self::assertFalse(Vendor::versionCompare('5.7.43', '5.7.44', '='));
+        self::assertTrue(Vendor::versionCompare('5.7.44', '5.7.44', '='));
+        self::assertFalse(Vendor::versionCompare('5.7.45', '5.7.44', '='));
+    }
+
+    public function testVersionCompareGreaterOrEqualThan(): void
+    {
+        self::assertTrue(Vendor::versionCompare('5.6', '5.7.44', '>='));
+        self::assertTrue(Vendor::versionCompare('5.7', '5.7.44', '>='));
+        self::assertFalse(Vendor::versionCompare('8.0', '5.7.44', '>='));
+
+        self::assertTrue(Vendor::versionCompare('5.6.0', '5.7.44', '>='));
+        self::assertTrue(Vendor::versionCompare('5.7.0', '5.7.44', '>='));
+        self::assertFalse(Vendor::versionCompare('8.0.0', '5.7.44', '>='));
+
+        self::assertTrue(Vendor::versionCompare('5.7.43', '5.7.44', '>='));
+        self::assertTrue(Vendor::versionCompare('5.7.44', '5.7.44', '>='));
+        self::assertFalse(Vendor::versionCompare('5.7.45', '5.7.44', '>='));
+    }
+
+    public function testVersionCompareGreaterThan(): void
+    {
+        self::assertTrue(Vendor::versionCompare('5.6', '5.7.44', '>'));
+        self::assertFalse(Vendor::versionCompare('5.7', '5.7.44', '>'));
+        self::assertFalse(Vendor::versionCompare('8.0', '5.7.44', '>'));
+
+        self::assertTrue(Vendor::versionCompare('5.6.0', '5.7.44', '>'));
+        self::assertTrue(Vendor::versionCompare('5.7.0', '5.7.44', '>'));
+        self::assertFalse(Vendor::versionCompare('8.0.0', '5.7.44', '>'));
+
+        self::assertTrue(Vendor::versionCompare('5.7.43', '5.7.44', '>'));
+        self::assertFalse(Vendor::versionCompare('5.7.44', '5.7.44', '>'));
+        self::assertFalse(Vendor::versionCompare('5.7.45', '5.7.44', '>'));
+    }
+}