fix(server): compare numeric string flag properties numerically

.changeset/khaki-falcons-wear.md

@@ -0,0 +1,5 @@
+---
+"posthog-server": patch
+---
+
+Fix numeric-string ordering in server-side local feature flag evaluation. `gt`, `gte`, `lt`, and `lte` now compare finite numbers rather than strings, which can change flag membership for numeric-string properties. Invalid property values no longer match through lexicographic comparison, and invalid numeric conditions remain eligible for remote fallback.

posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt

@@ -32,6 +32,8 @@ internal class FlagEvaluator(
     companion object {
         private const val LONG_SCALE = 0xFFFFFFFFFFFFFFF.toDouble()
         private val NONE_VALUES_ALLOWED_OPERATORS = setOf(PropertyOperator.EXACT, PropertyOperator.IS_NOT)
+        private val NUMERIC_OPERATORS = setOf(PropertyOperator.GT, PropertyOperator.GTE, PropertyOperator.LT, PropertyOperator.LTE)
+        private val REGEX_DECIMAL = Regex("""[+-]?([0-9]+(\.[0-9]*)?|\.[0-9]+)([eE][+-]?[0-9]+)?""")
         private val JSON = GsonBuilder().disableHtmlEscaping().serializeNulls().create()
         private val REGEX_RELATIVE_DATE = "^-?([0-9]+)([hdwmy])$".toRegex()
         private val REGEX_SEMVER =
@@ -145,6 +147,10 @@ internal class FlagEvaluator(
 
         val overrideValue = propertyValues[key]
 
+        if (propertyOperator in NUMERIC_OPERATORS) {
+            return compareValues(overrideValue, propertyValue, propertyOperator)
+        }
+
         // Handle null values (only allowed for certain operators)
         if (propertyOperator !in NONE_VALUES_ALLOWED_OPERATORS && overrideValue == null) {
             return false
@@ -204,13 +210,6 @@ internal class FlagEvaluator(
                     overrideValue.toString(),
                 )
 
-            PropertyOperator.GT, PropertyOperator.GTE, PropertyOperator.LT, PropertyOperator.LTE ->
-                compareValues(
-                    overrideValue,
-                    propertyValue,
-                    propertyOperator,
-                )
-
             PropertyOperator.IS_DATE_BEFORE, PropertyOperator.IS_DATE_AFTER ->
                 compareDates(
                     overrideValue,
@@ -401,35 +400,21 @@ internal class FlagEvaluator(
         propertyValue: Any?,
         propertyOperator: PropertyOperator,
     ): Boolean {
-        val numericValue = propertyValue?.toString()?.toDoubleOrNull()
-
-        return if (numericValue != null && overrideValue != null) {
-            when (overrideValue) {
-                is String ->
-                    compareStrings(
-                        overrideValue,
-                        propertyValue.toString(),
-                        propertyOperator,
-                    )
-
-                is Number ->
-                    compareNumbers(
-                        overrideValue.toDouble(),
-                        numericValue,
-                        propertyOperator,
-                    )
+        val numericValue =
+            parseFiniteNumber(propertyValue)
+                ?: throw InconclusiveMatchException("The number set on the flag is not a valid finite number")
+        val numericOverride = parseFiniteNumber(overrideValue) ?: return false
+        return compareNumbers(numericOverride, numericValue, propertyOperator)
+    }
 
-                else ->
-                    compareStrings(
-                        overrideValue.toString(),
-                        propertyValue.toString(),
-                        propertyOperator,
-                    )
+    private fun parseFiniteNumber(value: Any?): Double? {
+        val number =
+            when (value) {
+                is Number -> value.toDouble()
+                is String -> if (REGEX_DECIMAL.matches(value)) value.toDoubleOrNull() else null
+                else -> null
             }
-        } else {
-            // String comparison if numeric parsing fails
-            compareStrings(overrideValue.toString(), propertyValue.toString(), propertyOperator)
-        }
+        return number?.takeIf { it.isFinite() }
     }
 
     private fun compareNumbers(
@@ -446,20 +431,6 @@ internal class FlagEvaluator(
         }
     }
 
-    private fun compareStrings(
-        lhs: String,
-        rhs: String,
-        propertyOperator: PropertyOperator,
-    ): Boolean {
-        return when (propertyOperator) {
-            PropertyOperator.GT -> lhs > rhs
-            PropertyOperator.GTE -> lhs >= rhs
-            PropertyOperator.LT -> lhs < rhs
-            PropertyOperator.LTE -> lhs <= rhs
-            else -> false
-        }
-    }
-
     private fun compareDates(
         overrideValue: Any?,
         propertyValue: Any?,

posthog-server/src/test/java/com/posthog/server/PostHogEvaluateFlagsTest.kt

@@ -547,6 +547,68 @@ internal class PostHogEvaluateFlagsTest {
         }
     }
 
+    @Test
+    fun `numeric ordering evaluates locally and falls back only for invalid filters or missing properties`() {
+        fun definition(
+            key: String,
+            operator: String,
+            filter: String,
+        ): String =
+            """
+            {
+                "id": 1, "key": "$key", "name": "$key", "active": true,
+                "filters": { "groups": [{ "properties": [
+                    { "key": "$key", "type": "person", "operator": "$operator", "value": $filter }
+                ], "rollout_percentage": 100 }] }
+            }
+            """.trimIndent()
+
+        val definitions =
+            createLocalEvaluationResponseFrom(
+                definition("gt", "gt", "2"),
+                definition("lt", "lt", "30"),
+                definition("gte", "gte", "\"5\""),
+                definition("lte", "lte", "\"5\""),
+                definition("invalid-actual", "gt", "\"9\""),
+                definition("null-actual", "gt", "\"9\""),
+                definition("invalid-filter", "gt", "\"9px\""),
+                definition("missing", "gt", "\"9\""),
+            )
+        withLocalEvaluation(
+            definitions = definitions,
+            flagsResponse = { jsonResponse(createMultipleFlagsResponse("invalid-filter" to true, "missing" to true)) },
+        ) { postHog, dispatcher, _ ->
+            val properties =
+                mapOf("gt" to "10", "lt" to "100", "gte" to "10", "lte" to "10", "invalid-actual" to "10px", "null-actual" to null)
+            val localKeys = listOf("gt", "lt", "gte", "lte", "invalid-actual", "null-actual")
+            val local = postHog.evaluateFlags("local-user", personProperties = properties, flagKeys = localKeys)
+            assertEquals(localKeys.toSet(), local.keys.toSet())
+            assertTrue(local.isEnabled("gt"))
+            assertFalse(local.isEnabled("lt"))
+            assertTrue(local.isEnabled("gte"))
+            assertFalse(local.isEnabled("lte"))
+            assertFalse(local.isEnabled("invalid-actual"))
+            assertFalse(local.isEnabled("null-actual"))
+            assertEquals(0, dispatcher.flagsCalls.get())
+
+            val withFallback = postHog.evaluateFlags("fallback-user", personProperties = properties + ("invalid-filter" to null))
+            assertTrue(withFallback.isEnabled("gt"))
+            assertFalse(withFallback.isEnabled("invalid-actual"))
+            assertTrue(withFallback.isEnabled("invalid-filter"))
+            assertTrue(withFallback.isEnabled("missing"))
+            assertEquals(1, dispatcher.flagsCalls.get())
+
+            val localOnly =
+                postHog.evaluateFlags(
+                    "local-only-user",
+                    personProperties = properties + ("invalid-filter" to "10"),
+                    onlyEvaluateLocally = true,
+                )
+            assertEquals(localKeys.toSet(), localOnly.keys.toSet())
+            assertEquals(1, dispatcher.flagsCalls.get())
+        }
+    }
+
     /** One flag local evaluation always resolves, one it can never resolve without an `email`. */
     private fun conclusiveAndGatedDefinitions(): String =
         createLocalEvaluationResponseFrom(

posthog-server/src/test/java/com/posthog/server/internal/FlagEvaluatorTest.kt

@@ -699,6 +699,92 @@ internal class FlagEvaluatorTest {
         assertTrue(evaluator.matchProperty(property, properties))
     }
 
+    @Test
+    internal fun testNumericOrderingIssueCases() {
+        data class Row(val actual: String, val filter: Any, val operator: PropertyOperator, val expected: Boolean)
+
+        val rows =
+            listOf(
+                Row("10", 2, PropertyOperator.GT, true),
+                Row("100", 30, PropertyOperator.LT, false),
+                Row("10", "5", PropertyOperator.GTE, true),
+                Row("10", "5", PropertyOperator.LTE, false),
+            )
+        for (row in rows) {
+            val property = FlagProperty("value", row.filter, row.operator, PropertyType.PERSON, false, null)
+            assertEquals("$row", row.expected, evaluator.matchProperty(property, mapOf("value" to row.actual)))
+        }
+    }
+
+    @Test
+    internal fun testNumericOrderingUsesFiniteBinary64() {
+        val pairs =
+            listOf(
+                Triple("10", "2", 1),
+                Triple("100", "30", 1),
+                Triple("10", "5", 1),
+                Triple("5", "10", -1),
+                Triple("+0010.", "1e1", 0),
+                Triple("-.5", "-0.50", 0),
+                Triple("+1.25e+2", "124.9", 1),
+                Triple("-1.25E-2", "-.01", -1),
+                Triple("-0", "+0.0", 0),
+                Triple("1e-9999", "0", 0),
+                Triple("9007199254740993", "9007199254740992", 0),
+            )
+        for ((actual, filter, comparison) in pairs) {
+            for (actualValue in listOfNotNull(actual, actual.toDouble(), actual.toLongOrNull())) {
+                for (filterValue in listOfNotNull(filter, filter.toDouble(), filter.toLongOrNull())) {
+                    for (operator in listOf(PropertyOperator.GT, PropertyOperator.GTE, PropertyOperator.LT, PropertyOperator.LTE)) {
+                        val expected =
+                            when (operator) {
+                                PropertyOperator.GT -> comparison > 0
+                                PropertyOperator.GTE -> comparison >= 0
+                                PropertyOperator.LT -> comparison < 0
+                                else -> comparison <= 0
+                            }
+                        val property = FlagProperty("value", filterValue, operator, PropertyType.PERSON, false, null)
+                        assertEquals(
+                            "actual=$actualValue filter=$filterValue operator=$operator",
+                            expected,
+                            evaluator.matchProperty(property, mapOf("value" to actualValue)),
+                        )
+                    }
+                }
+            }
+        }
+    }
+
+    @Test
+    internal fun testNumericOrderingDistinguishesInvalidPropertyFromInvalidFilter() {
+        val invalidValues =
+            listOf(
+                "", " ", " 10", "10 ", "10\n", "10\r\n", "\t10", "10px", "1_0", "1,000", "١٠", "１０",
+                "+", ".", "1e", "0x10", "0x1.0p0", "10d", "NaN", "Infinity", "-Infinity", "1e309",
+                Double.NaN, Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY,
+                true, false, null, listOf(10), mapOf("value" to 10),
+            )
+        for (operator in listOf(PropertyOperator.GT, PropertyOperator.GTE, PropertyOperator.LT, PropertyOperator.LTE)) {
+            for (validFilter in listOf("9", 9)) {
+                val property = FlagProperty("value", validFilter, operator, PropertyType.PERSON, false, null)
+                for (invalid in invalidValues) {
+                    assertFalse("actual=$invalid operator=$operator", evaluator.matchProperty(property, mapOf("value" to invalid)))
+                }
+                kotlin.test.assertFailsWith<InconclusiveMatchException> {
+                    evaluator.matchProperty(property, emptyMap())
+                }
+            }
+            for (invalid in invalidValues) {
+                val property = FlagProperty("value", invalid, operator, PropertyType.PERSON, false, null)
+                for (actual in listOf(10, "10", null, "invalid")) {
+                    kotlin.test.assertFailsWith<InconclusiveMatchException>("filter=$invalid operator=$operator actual=$actual") {
+                        evaluator.matchProperty(property, mapOf("value" to actual))
+                    }
+                }
+            }
+        }
+    }
+
     @Test
     internal fun testMatchPropertyDateBefore() {
         val property =