diff --git a/.changeset/fix-numeric-string-ordering.md b/.changeset/fix-numeric-string-ordering.md new file mode 100644 index 0000000..ffe6945 --- /dev/null +++ b/.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. diff --git a/lib/FeatureFlag.php b/lib/FeatureFlag.php index 18d1ea7..8f6072a 100644 --- a/lib/FeatureFlag.php +++ b/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") { diff --git a/test/FeatureFlagEvaluationsTest.php b/test/FeatureFlagEvaluationsTest.php index 62d28f0..8240d94 100644 --- a/test/FeatureFlagEvaluationsTest.php +++ b/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 diff --git a/test/FeatureFlagLocalEvaluationTest.php b/test/FeatureFlagLocalEvaluationTest.php index b9785a7..2b7b90e 100644 --- a/test/FeatureFlagLocalEvaluationTest.php +++ b/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()