diff --git a/.changeset/android-v4-major.md b/.changeset/android-v4-major.md index 1b6846771..e50a3684b 100644 --- a/.changeset/android-v4-major.md +++ b/.changeset/android-v4-major.md @@ -8,3 +8,4 @@ - **Breaking:** remove the deprecated `evaluationEnvironments` config option; use `evaluationContexts` instead - **Breaking:** remove the deprecated `getFeatureFlagPayload()`; use `getFeatureFlagResult(key)?.payload` instead - **Breaking:** remove the deprecated `PostHogSessionReplayConfig.debouncerDelayMs`; use `throttleDelayMs` instead +- **Breaking:** return `null` instead of the raw string for a malformed, empty or whitespace-only feature flag payload from `getFeatureFlagResult(key)?.payload`; payloads must be valid JSON (a JSON `null` payload now also reads as `null` instead of the string `"null"`) diff --git a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt index 03ba9946a..af4d05fc0 100644 --- a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt +++ b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt @@ -1,5 +1,8 @@ package com.posthog.internal +import com.google.gson.stream.JsonReader +import com.google.gson.stream.JsonToken +import com.google.gson.stream.MalformedJsonException import com.posthog.FeatureFlagResult import com.posthog.PostHogConfig import com.posthog.PostHogInternal @@ -17,6 +20,7 @@ import com.posthog.internal.PostHogPreferences.Companion.PUSH import com.posthog.internal.PostHogPreferences.Companion.SESSION_REPLAY import com.posthog.internal.PostHogPreferences.Companion.SURVEYS import com.posthog.surveys.Survey +import java.io.StringReader import java.util.concurrent.ExecutorService import java.util.concurrent.atomic.AtomicBoolean @@ -1198,26 +1202,28 @@ public class PostHogRemoteConfig( } } - private fun normalizePayloads(featureFlagPayloads: Map?): Map { - val parsedPayloads = (featureFlagPayloads ?: mapOf()).toMutableMap() - - for (item in parsedPayloads) { - val value = item.value + private fun normalizePayloads(featureFlagPayloads: Map?): Map = + (featureFlagPayloads ?: mapOf()).mapValues { (key, value) -> + // payloads arrive as serialized JSON strings; other values are already decoded + if (value is String) decodePayload(key, value) else value + } - try { - // only try to parse if its a String, since the JSON values are stringified - if (value is String) { - // try to deserialize as Any? - config.serializer.deserializeString(value)?.let { - parsedPayloads[item.key] = it - } - } - } catch (ignored: Throwable) { - // if it fails, we keep the original value + // Strict JSON, so a malformed, empty or whitespace-only payload is no payload (null), never the raw string. + private fun decodePayload( + key: String, + serialized: String, + ): Any? = + try { + val reader = JsonReader(StringReader(serialized)) + val decoded = config.serializer.gson.getAdapter(Any::class.java).read(reader) + if (reader.peek() != JsonToken.END_DOCUMENT) { + throw MalformedJsonException("Unexpected data after the JSON value") } + decoded + } catch (e: Throwable) { + config.logger.log("Ignoring malformed payload for feature flag $key: $e") + null } - return parsedPayloads - } private fun normalizeFlagsResponse(flagsResponse: PostHogFlagsResponse): PostHogFlagsResponse { val flags = flagsResponse.flags diff --git a/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsTest.kt b/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsTest.kt index eec9f7337..a03b735b5 100644 --- a/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsTest.kt +++ b/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsTest.kt @@ -407,7 +407,7 @@ internal class PostHogFeatureFlagsTest { assertTrue(sut.getFeatureFlagPayload("theBoolean", defaultValue = null) as Boolean) assertNull(sut.getFeatureFlagPayload("theNull", defaultValue = null)) - assertEquals("[1, 2", sut.getFeatureFlagPayload("theBroken", defaultValue = null) as String) + assertNull(sut.getFeatureFlagPayload("theBroken", defaultValue = null)) } @Test @@ -465,7 +465,7 @@ internal class PostHogFeatureFlagsTest { """ { "featureFlags": {"flag1": true}, - "featureFlagPayloads": {"flag1": "payload1"} + "featureFlagPayloads": {"flag1": "\"payload1\""} } """.trimIndent(), ), @@ -515,7 +515,7 @@ internal class PostHogFeatureFlagsTest { """ { "featureFlags": {"flag1": true}, - "featureFlagPayloads": {"flag1": "payload1"} + "featureFlagPayloads": {"flag1": "\"payload1\""} } """.trimIndent(), ), diff --git a/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsV1Test.kt b/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsV1Test.kt index 7cb40c827..fd2fa4220 100644 --- a/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsV1Test.kt +++ b/posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsV1Test.kt @@ -270,7 +270,7 @@ internal class PostHogFeatureFlagsV1Test { assertTrue(sut.getFeatureFlagPayload("theBoolean", defaultValue = null) as Boolean) assertNull(sut.getFeatureFlagPayload("theNull", defaultValue = null)) - assertEquals("[1, 2", sut.getFeatureFlagPayload("theBroken", defaultValue = null) as String) + assertNull(sut.getFeatureFlagPayload("theBroken", defaultValue = null)) } @Test @@ -328,7 +328,7 @@ internal class PostHogFeatureFlagsV1Test { """ { "featureFlags": {"flag1": true}, - "featureFlagPayloads": {"flag1": "payload1"} + "featureFlagPayloads": {"flag1": "\"payload1\""} } """.trimIndent(), ), diff --git a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt index 5701d46e4..0d9249fc1 100644 --- a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt +++ b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt @@ -3084,4 +3084,105 @@ internal class PostHogRemoteConfigTest { sut.clear() http.shutdown() } + + private fun loadFlagsWithPayloads( + payloads: Map, + logs: MutableList = mutableListOf(), + ): PostHogRemoteConfig { + val serializedPayloads = payloads.entries.joinToString(",") { (key, value) -> "\"$key\":${jsonString(value)}" } + val flags = payloads.keys.joinToString(",") { "\"$it\":\"blue\"" } + val http = + mockHttp( + response = + MockResponse().setBody( + """{"errorsWhileComputingFlags":false,"featureFlags":{$flags,"beta-ui":true},""" + + """"featureFlagPayloads":{$serializedPayloads,"beta-ui":"{\"color\":\"green\"}"}}""", + ), + ) + val sut = getSut(host = http.url("/").toString()) + config!!.logger = + object : PostHogLogger { + override fun log(message: String) { + logs.add(message) + } + + override fun isEnabled(): Boolean = true + } + + val latch = CountDownLatch(1) + sut.loadFeatureFlags( + "my_identify", + anonymousId = "anonId", + emptyMap(), + onFeatureFlags = PostHogOnFeatureFlags { latch.countDown() }, + ) + assertTrue(latch.await(5, TimeUnit.SECONDS), "flags load should complete") + http.shutdown() + return sut + } + + private fun jsonString(value: String): String = PostHogSerializer(PostHogConfig(PROJECT_TOKEN)).gson.toJson(value) + + @Test + fun `malformed, empty or whitespace payloads are null and keep the flag and sibling payloads`() { + for (input in listOf("{broken", "", " ", "hello", "{\"a\":1} trailing")) { + preferences.clear() + val logs = mutableListOf() + val sut = loadFlagsWithPayloads(mapOf("checkout" to input), logs) + + assertNull(sut.getFeatureFlagPayload("checkout"), "payload for input '$input'") + val result = sut.getFeatureFlagResult("checkout") + assertNotNull(result) + assertTrue(result.enabled) + assertEquals("blue", result.variant) + assertNull(result.payload) + assertEquals(mapOf("checkout" to "blue", "beta-ui" to true), sut.getFeatureFlags()) + assertEquals(mapOf("color" to "green"), sut.getFeatureFlagPayload("beta-ui")) + assertTrue(logs.any { "checkout" in it && "malformed" in it }, "logs for input '$input': $logs") + + // the cache holds no raw string either, so a later launch reads no payload + assertNull(getSut(host = "http://127.0.0.1:1/").getFeatureFlagPayload("checkout")) + } + } + + @Test + fun `valid JSON payloads keep their decoded value`() { + val sut = + loadFlagsWithPayloads( + mapOf( + "string" to "\"hello\"", + "empty" to "\"\"", + "false" to "false", + "zero" to "0", + "null" to "null", + "object" to "{\"color\":\"green\"}", + "array" to "[1,false]", + ), + ) + + assertEquals("hello", sut.getFeatureFlagPayload("string")) + assertEquals("", sut.getFeatureFlagPayload("empty")) + assertEquals(false, sut.getFeatureFlagPayload("false")) + assertEquals(0, sut.getFeatureFlagPayload("zero")) + assertNull(sut.getFeatureFlagPayload("null")) + assertEquals(mapOf("color" to "green"), sut.getFeatureFlagPayload("object")) + assertEquals(listOf(1, false), sut.getFeatureFlagPayload("array")) + } + + @Test + fun `bootstrapped string payloads are not JSON-decoded again`() { + for (payload in listOf("hello", "123", "true")) { + val sut = + getSut( + host = "http://127.0.0.1:1/", + bootstrap = + PostHogBootstrapConfig( + featureFlags = mapOf("beta-ui" to "variant-a"), + featureFlagPayloads = mapOf("beta-ui" to payload), + ), + ) + + assertEquals(payload, sut.getFeatureFlagResult("beta-ui")?.payload) + } + } }