diff --git a/.changeset/khaki-falcons-wear.md b/.changeset/khaki-falcons-wear.md new file mode 100644 index 000000000..857750284 --- /dev/null +++ b/.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. diff --git a/posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt b/posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt index a436c61b3..94d003c18 100644 --- a/posthog-server/src/main/java/com/posthog/server/internal/FlagEvaluator.kt +++ b/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?, diff --git a/posthog-server/src/test/java/com/posthog/server/PostHogEvaluateFlagsTest.kt b/posthog-server/src/test/java/com/posthog/server/PostHogEvaluateFlagsTest.kt index a04488502..65084276a 100644 --- a/posthog-server/src/test/java/com/posthog/server/PostHogEvaluateFlagsTest.kt +++ b/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( diff --git a/posthog-server/src/test/java/com/posthog/server/internal/FlagEvaluatorTest.kt b/posthog-server/src/test/java/com/posthog/server/internal/FlagEvaluatorTest.kt index 7267d8042..1c1fd2d47 100644 --- a/posthog-server/src/test/java/com/posthog/server/internal/FlagEvaluatorTest.kt +++ b/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 { + 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("filter=$invalid operator=$operator actual=$actual") { + evaluator.matchProperty(property, mapOf("value" to actual)) + } + } + } + } + } + @Test internal fun testMatchPropertyDateBefore() { val property =