fix(flags): compare numeric string properties numerically

.changeset/fix-numeric-string-ordering.md

@@ -0,0 +1,5 @@
+---
+"posthog-php": patch
+---
+
+Fix local feature flag `gt`, `gte`, `lt`, and `lte` comparisons to compare numeric strings numerically, which can change flag membership for numeric-string properties. Invalid property values no longer compare lexicographically; invalid numeric filters fall back to remote evaluation.

lib/FeatureFlag.php

@@ -89,22 +89,17 @@ public static function matchProperty($property, $propertyValues, $propertyMatchi
         }
 
         if (in_array($operator, ["gt", "gte", "lt", "lte"])) {
-            $parsedValue = null;
-
-            if (is_numeric($value)) {
-                $parsedValue = floatval($value);
+            $parsedValue = FeatureFlag::parseNumericValue($value);
+            if ($parsedValue === null) {
+                throw new InconclusiveMatchException("The number set on the flag is not a valid format");
             }
 
-            if (!is_null($parsedValue) && !is_null($overrideValue)) {
-                if (is_string($overrideValue)) {
-                    return FeatureFlag::compare($overrideValue, FeatureFlag::valueToString($value), $operator);
-                } else {
-                    return FeatureFlag::compare($overrideValue, $parsedValue, $operator, "numeric");
-                }
-            } else {
-                // Preserve the existing fallback for non-numeric comparison values.
-                return FeatureFlag::compare(strval($overrideValue), strval($value), $operator);
+            $parsedOverrideValue = FeatureFlag::parseNumericValue($overrideValue);
+            if ($parsedOverrideValue === null) {
+                return false;
             }
+
+            return FeatureFlag::compare($parsedOverrideValue, $parsedValue, $operator);
         }
 
         if (in_array($operator, ["is_date_before", "is_date_after"])) {
@@ -810,17 +805,24 @@ private static function floatToString($value)
         return $sign . substr($digits, 0, $position) . '.' . substr($digits, $position);
     }
 
-    private static function compare($lhs, $rhs, $operator, $type = "string")
+    private static function parseNumericValue($value): ?float
     {
-        // If type is string, we use strcmp to compare the two strings
-        // If type is numeric, we use <=> to compare the two numbers
-
-        if ($type == "string") {
-            $comparison = strcmp($lhs, $rhs);
-        } else {
-            $comparison = $lhs <=> $rhs;
+        if (is_string($value)) {
+            if (preg_match('/\A[+-]?([0-9]+(\.[0-9]*)?|\.[0-9]+)([eE][+-]?[0-9]+)?\z/', $value) !== 1) {
+                return null;
+            }
+        } elseif (!is_int($value) && !is_float($value)) {
+            return null;
         }
 
+        $parsedValue = (float) $value;
+        return is_finite($parsedValue) ? $parsedValue : null;
+    }
+
+    private static function compare(float $lhs, float $rhs, string $operator): bool
+    {
+        $comparison = $lhs <=> $rhs;
+
         if ($operator == "gt") {
             return $comparison > 0;
         } elseif ($operator == "gte") {

test/FeatureFlagEvaluationsTest.php

@@ -489,6 +489,72 @@ public function testLocalEvaluationSkipsRemoteFlagsRequestWhenAllResolved(): voi
         $this->assertSame(0, $this->flagsRequestCount());
     }
 
+    #[DataProvider('numericLocalEvaluationProvider')]
+    public function testNumericOrderingLocalEvaluationAndFallback(
+        string $operator,
+        mixed $filter,
+        array $personProperties,
+        bool|string $expected,
+        bool $fallback
+    ): void {
+        $this->makeClient(
+            personalApiKey: 'test-personal-key',
+            flagsEndpointResponse: [
+                'flags' => ['numeric-flag' => ['enabled' => true, 'variant' => 'remote-value']],
+            ],
+            localEvaluationResponse: [
+                'flags' => [[
+                    'id' => 1,
+                    'key' => 'numeric-flag',
+                    'active' => true,
+                    'filters' => ['groups' => [[
+                        'properties' => [[
+                            'key' => 'number',
+                            'type' => 'person',
+                            'operator' => $operator,
+                            'value' => $filter,
+                        ]],
+                        'rollout_percentage' => 100,
+                    ]]],
+                ]],
+                'group_type_mapping' => [],
+                'cohorts' => [],
+            ],
+        );
+
+        $snapshot = $this->client->evaluateFlags('user-1', personProperties: $personProperties);
+
+        $this->assertSame($expected, $snapshot->getFlag('numeric-flag'));
+        $this->assertSame($fallback ? 1 : 0, $this->flagsRequestCount());
+
+        if ($fallback) {
+            $localSnapshot = $this->client->evaluateFlags(
+                'user-1',
+                personProperties: $personProperties,
+                onlyEvaluateLocally: true
+            );
+            $this->assertSame([], $localSnapshot->getKeys());
+            $this->assertSame(1, $this->flagsRequestCount());
+        }
+    }
+
+    public static function numericLocalEvaluationProvider(): iterable
+    {
+        yield 'reported gt numeric filter' => ['gt', 2, ['number' => '10'], true, false];
+        yield 'reported lt numeric filter' => ['lt', 30, ['number' => '100'], false, false];
+        yield 'reported gte string filter' => ['gte', '5', ['number' => '10'], true, false];
+        yield 'reported lte string filter' => ['lte', '5', ['number' => '10'], false, false];
+        yield 'numeric actual and string filter' => ['gt', '2', ['number' => 10], true, false];
+        yield 'signed exponent' => ['lt', '-.5', ['number' => '-1e1'], true, false];
+        $invalidOperands = ['10px', ' 10 ', "10\n", '1_0', 'NaN', 'Infinity', '1e309', null, true, [], (object) []];
+        foreach ($invalidOperands as $index => $invalid) {
+            yield "invalid actual $index" => ['gt', '2', ['number' => $invalid], false, false];
+            yield "invalid filter $index" => ['gt', $invalid, ['number' => '10'], 'remote-value', true];
+        }
+        yield 'missing actual' => ['gt', '2', [], 'remote-value', true];
+        yield 'invalid filter and actual' => ['gt', 'bad', ['number' => null], 'remote-value', true];
+    }
+
     public function testLocalEvaluationResolvesPayloads(): void
     {
         // Local definitions carry `filters.payloads`, keyed by "true" for boolean flags and by

test/FeatureFlagLocalEvaluationTest.php

@@ -1028,32 +1028,121 @@ public function testMatchPropertyMathOperators(): void
         self::assertTrue(FeatureFlag::matchProperty($prop_e, [
             "key" => "29",
         ]));
-        # depending on the type of override, we adjust type comparison
-        self::assertTrue(FeatureFlag::matchProperty($prop_e, [
+        self::assertFalse(FeatureFlag::matchProperty($prop_e, [
             "key" => "100",
         ]));
         self::assertFalse(FeatureFlag::matchProperty($prop_e, [
             "key" => 100,
         ]));
 
-        $prop_f = [
-            "key" => "key",
-            "value" => "123aloha",
-            "operator" => "gt"
+    }
+
+    #[DataProvider('numericOrderingProvider')]
+    public function testMatchPropertyNumericOrdering($actual, $filter, string $operator, bool $expected): void
+    {
+        self::assertSame($expected, FeatureFlag::matchProperty(
+            ['key' => 'number', 'value' => $filter, 'operator' => $operator],
+            ['number' => $actual]
+        ));
+    }
+
+    public static function numericOrderingProvider(): iterable
+    {
+        $operators = ['gt', 'gte', 'lt', 'lte'];
+        foreach ([
+            [10, 2, [true, true, false, false]],
+            [100, 30, [true, true, false, false]],
+            [10, 5, [true, true, false, false]],
+            [5, 10, [false, false, true, true]],
+            [10, 10, [false, true, false, true]],
+            [-10, -2, [false, false, true, true]],
+            [0, 0, [false, true, false, true]],
+        ] as [$actual, $filter, $expected]) {
+            foreach ([$actual, (string) $actual] as $property) {
+                foreach ([$filter, (string) $filter] as $condition) {
+                    foreach ($operators as $index => $operator) {
+                        yield "$operator " . gettype($property) . " $property / " . gettype($condition) . " $condition"
+                            => [$property, $condition, $operator, $expected[$index]];
+                    }
+                }
+            }
+        }
+
+        foreach ([
+            ['+10', 10.0], ['-10', -10.0], ['10.', 10.0], ['+.5', 0.5],
+            ['-.5', -0.5], ['01.50', 1.5], ['1e2', 100.0], ['-2E+1', -20.0],
+            ['2.5e-1', 0.25], ['-0', 0.0], ['1e-9999', 0.0],
+            ['9007199254740993', 9007199254740992.0],
+            ['1.7976931348623157e308', PHP_FLOAT_MAX],
+        ] as [$string, $number]) {
+            foreach ([[$string, $number], [$number, $string], [$string, $string]] as [$actual, $filter]) {
+                foreach ($operators as $index => $operator) {
+                    yield "$operator decimal $string " . gettype($actual) . '/' . gettype($filter)
+                        => [$actual, $filter, $operator, [false, true, false, true][$index]];
+                }
+            }
+        }
+        yield 'integer operands round to binary64' => [9007199254740993, 9007199254740992, 'gt', false];
+    }
+
+    public static function invalidNumericOperandProvider(): iterable
+    {
+        $invalid = [
+            'empty' => '', 'partial' => '10px', 'padded' => ' 10 ',
+            'leading space' => ' 10', 'trailing space' => '10 ',
+            'newline' => "10\n", 'leading newline' => "\n10", 'tab' => "10\t",
+            'underscore' => '1_0', 'hex' => '0x10', 'comma' => '1,5',
+            'unicode digit' => '١٠', 'incomplete exponent' => '1e', 'dot' => '.',
+            'nan string' => 'NaN', 'infinity string' => 'Infinity', 'inf string' => '-inf', 'null string' => 'null',
+            'overflow string' => '1e309', 'negative overflow string' => '-1e309',
+            'nan' => NAN, 'infinity' => INF, 'negative infinity' => -INF,
+            'null' => null, 'true' => true, 'false' => false,
+            'array' => [], 'numeric array' => [10], 'object' => (object) ['number' => 10],
         ];
-        self::assertFalse(FeatureFlag::matchProperty($prop_f, [
-            "key" => "123",
-        ]));
-        self::assertFalse(FeatureFlag::matchProperty($prop_f, [
-            "key" => 122,
-        ]));
+        foreach (['gt', 'gte', 'lt', 'lte'] as $operator) {
+            foreach ($invalid as $name => $value) {
+                yield "$operator $name" => [$operator, $value];
+            }
+        }
+    }
 
-        # this turns into a string comparison
-        self::assertTrue(FeatureFlag::matchProperty($prop_f, [
-            "key" => 129,
-        ]));
+    #[DataProvider('invalidNumericOperandProvider')]
+    public function testMatchPropertyInvalidNumericActualDoesNotMatch(string $operator, $actual): void
+    {
+        foreach ([2, '2'] as $filter) {
+            self::assertFalse(FeatureFlag::matchProperty(
+                ['key' => 'number', 'value' => $filter, 'operator' => $operator],
+                ['number' => $actual]
+            ));
+        }
+    }
 
+    #[DataProvider('invalidNumericOperandProvider')]
+    public function testMatchPropertyInvalidNumericFilterIsInconclusive(string $operator, $filter): void
+    {
+        foreach ([10, '10', null] as $actual) {
+            try {
+                FeatureFlag::matchProperty(
+                    ['key' => 'number', 'value' => $filter, 'operator' => $operator],
+                    ['number' => $actual]
+                );
+                self::fail('Expected an invalid numeric filter to be inconclusive');
+            } catch (InconclusiveMatchException $exception) {
+                self::assertSame('The number set on the flag is not a valid format', $exception->getMessage());
+            }
+        }
+    }
 
+    public function testMatchPropertyMissingNumericActualIsInconclusive(): void
+    {
+        foreach (['gt', 'gte', 'lt', 'lte'] as $operator) {
+            try {
+                FeatureFlag::matchProperty(['key' => 'number', 'value' => '2', 'operator' => $operator], []);
+                self::fail('Expected a missing numeric property to be inconclusive');
+            } catch (InconclusiveMatchException $exception) {
+                self::assertSame("Can't match properties without a given property value", $exception->getMessage());
+            }
+        }
     }
 
     public function testMatchPropertyDateOperators(): void
@@ -1437,54 +1526,6 @@ public function testMatchPropertyWithNones(): void
             "key" => "null",
         ]));
 
-        $prop_e = [
-            "key" => "key",
-            "value" => "null",
-            "operator" => "gt"
-        ];
-        self::assertFalse(FeatureFlag::matchProperty($prop_e, [
-            "key" => null,
-        ]));
-        self::assertFalse(FeatureFlag::matchProperty($prop_e, [
-            "key" => "null",
-        ]));
-
-        $prop_f = [
-            "key" => "key",
-            "value" => "null",
-            "operator" => "lt"
-        ];
-        self::assertTrue(FeatureFlag::matchProperty($prop_f, [
-            "key" => null,
-        ]));
-        self::assertFalse(FeatureFlag::matchProperty($prop_f, [
-            "key" => "null",
-        ]));
-
-        $prop_g = [
-            "key" => "key",
-            "value" => "null",
-            "operator" => "gte"
-        ];
-        self::assertFalse(FeatureFlag::matchProperty($prop_g, [
-            "key" => null,
-        ]));
-        self::assertTrue(FeatureFlag::matchProperty($prop_g, [
-            "key" => "null",
-        ]));
-
-        $prop_h = [
-            "key" => "key",
-            "value" => "null",
-            "operator" => "lte"
-        ];
-        self::assertTrue(FeatureFlag::matchProperty($prop_h, [
-            "key" => null,
-        ]));
-        self::assertTrue(FeatureFlag::matchProperty($prop_h, [
-            "key" => "null",
-        ]));
-
     }
 
     public function testRelativeDateParsingInvalidInput()