From a51ff9bde14c5b5ad2db1b00373d0d371f0cc85f Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> Date: Wed, 18 Oct 2023 14:46:56 +0200 Subject: [PATCH] Cache registered properties (#51) --- .github/workflows/build.yml | 2 + .../workflows/gradle-wrapper-validation.yml | 2 + CHANGELOG.md | 2 + .../internal/PostHogSharedPreferences.kt | 50 +++++++++++++++++-- .../internal/PostHogSharedPreferencesTests.kt | 13 +++-- .../posthog/android/sample/MainActivity.kt | 1 + posthog/api/posthog.api | 3 ++ posthog/src/main/java/com/posthog/PostHog.kt | 47 +++++++++-------- .../main/java/com/posthog/PostHogInterface.kt | 7 ++- .../internal/PostHogMemoryPreferences.kt | 4 +- .../posthog/internal/PostHogPreferences.kt | 11 ++++ .../src/test/java/com/posthog/PostHogTest.kt | 32 ++++++++++++ 12 files changed, 143 insertions(+), 31 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 0b2b7cbf..3000c9cc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,6 +1,8 @@ name: 'Build & Test' on: push: + branches: + - main pull_request: jobs: diff --git a/.github/workflows/gradle-wrapper-validation.yml b/.github/workflows/gradle-wrapper-validation.yml index 0e72f2b5..21ca8f37 100644 --- a/.github/workflows/gradle-wrapper-validation.yml +++ b/.github/workflows/gradle-wrapper-validation.yml @@ -1,6 +1,8 @@ name: 'Validate Gradle Wrapper' on: push: + branches: + - main pull_request: jobs: diff --git a/CHANGELOG.md b/CHANGELOG.md index ed6e76ec..956c17ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## Next +- Registered keys are cached in the disk preferences ([#51](https://github.com/PostHog/posthog-android/pull/51)) + ## 3.0.0-alpha.8 - 2023-10-12 - SDK only sends the `$feature_flag_called` event once per flag ([#47](https://github.com/PostHog/posthog-android/pull/47)) diff --git a/posthog-android/src/main/java/com/posthog/android/internal/PostHogSharedPreferences.kt b/posthog-android/src/main/java/com/posthog/android/internal/PostHogSharedPreferences.kt index 5afdc227..56a6a753 100644 --- a/posthog-android/src/main/java/com/posthog/android/internal/PostHogSharedPreferences.kt +++ b/posthog-android/src/main/java/com/posthog/android/internal/PostHogSharedPreferences.kt @@ -5,7 +5,9 @@ import android.content.Context.MODE_PRIVATE import android.content.SharedPreferences import com.posthog.android.PostHogAndroidConfig import com.posthog.internal.PostHogPreferences +import com.posthog.internal.PostHogPreferences.Companion.ALL_INTERNAL_KEYS import com.posthog.internal.PostHogPreferences.Companion.GROUPS +import com.posthog.internal.PostHogPreferences.Companion.STRINGIFIED_KEYS /** * Reads and writes to the SDKs shared preferences @@ -29,10 +31,18 @@ internal class PostHogSharedPreferences( value = sharedPreferences.all[key] ?: defaultValue } + val stringifiedKeys = getStringifiedKeys() + return convertValue(key, value, stringifiedKeys) + } + + private fun convertValue(key: String, value: Any?, keys: Set): Any? { return when (value) { is String -> { // we only want to deserialize special keys - if (SPECIAL_KEYS.contains(key)) { + // or keys that were stringified. + if (SPECIAL_KEYS.contains(key) || + keys.contains(key) + ) { deserializeObject(value) } else { value @@ -108,10 +118,32 @@ internal class PostHogSharedPreferences( } } + private fun addToStringifiedKeys(key: String, editor: SharedPreferences.Editor) { + val stringifiedKeys = getStringifiedKeys() + key + editor.putStringSet(STRINGIFIED_KEYS, stringifiedKeys) + } + + private fun removeFromStringifiedKeys(key: String, editor: SharedPreferences.Editor) { + val keys = getStringifiedKeys().toMutableSet() + if (!keys.contains(key)) { + return + } + keys.remove(key) + editor.putStringSet(STRINGIFIED_KEYS, keys) + } + + private fun getStringifiedKeys(): Set { + return sharedPreferences.getStringSet(STRINGIFIED_KEYS, setOf()) ?: setOf() + } + private fun serializeObject(key: String, value: Any, editor: SharedPreferences.Editor) { try { config.serializer.serializeObject(value)?.let { editor.putString(key, it) + + addToStringifiedKeys(key, editor) + } ?: run { + config.logger.log("Value type: ${value.javaClass.name} and value: $value isn't valid.") } } catch (e: Throwable) { config.logger.log("Value type: ${value.javaClass.name} and value: $value isn't valid.") @@ -133,16 +165,28 @@ internal class PostHogSharedPreferences( val edit = sharedPreferences.edit() synchronized(lock) { edit.remove(key) + removeFromStringifiedKeys(key, edit) edit.apply() } } override fun getAll(): Map { - val preferences: Map + val allPreferences: Map synchronized(lock) { @Suppress("UNCHECKED_CAST") - preferences = sharedPreferences.all.toMap() as? Map ?: emptyMap() + allPreferences = sharedPreferences.all.toMap() as? Map ?: emptyMap() + } + val filteredPreferences = allPreferences.filterKeys { key -> + !ALL_INTERNAL_KEYS.contains(key) } + val preferences = mutableMapOf() + val stringifiedKeys = getStringifiedKeys() + for ((key, value) in filteredPreferences) { + convertValue(key, value, stringifiedKeys)?.let { + preferences[key] = it + } + } + return preferences } diff --git a/posthog-android/src/test/java/com/posthog/android/internal/PostHogSharedPreferencesTests.kt b/posthog-android/src/test/java/com/posthog/android/internal/PostHogSharedPreferencesTests.kt index 7475d8c2..901ac351 100644 --- a/posthog-android/src/test/java/com/posthog/android/internal/PostHogSharedPreferencesTests.kt +++ b/posthog-android/src/test/java/com/posthog/android/internal/PostHogSharedPreferencesTests.kt @@ -6,6 +6,7 @@ import com.posthog.android.FakeSharedPreferences import com.posthog.android.PostHogAndroidConfig import com.posthog.android.apiKey import com.posthog.internal.PostHogPreferences.Companion.GROUPS +import com.posthog.internal.PostHogPreferences.Companion.STRINGIFIED_KEYS import org.junit.runner.RunWith import org.mockito.kotlin.mock import kotlin.test.Test @@ -100,7 +101,8 @@ internal class PostHogSharedPreferencesTests { sut.setValue("key", Any()) - assertEquals("{}", sut.getValue("key")) + @Suppress("UNCHECKED_CAST") + assertEquals(emptyMap(), sut.getValue("key") as? Map) } @Test @@ -114,14 +116,17 @@ internal class PostHogSharedPreferencesTests { } @Test - fun `preferences fallback to stringified version if not special key`() { + fun `preferences fallback to stringified version if not special and not stringified key`() { val sut = getSut() val props = mapOf("key" to "value") - sut.setValue("key", props) + sut.setValue("myJson", props) + + // removing to make it testable + sut.remove(STRINGIFIED_KEYS) val json = """{"key":"value"}""" - assertEquals(json, sut.getValue("key")) + assertEquals(json, sut.getValue("myJson")) } @Test diff --git a/posthog-samples/posthog-android-sample/src/main/java/com/posthog/android/sample/MainActivity.kt b/posthog-samples/posthog-android-sample/src/main/java/com/posthog/android/sample/MainActivity.kt index dd536691..83163931 100644 --- a/posthog-samples/posthog-android-sample/src/main/java/com/posthog/android/sample/MainActivity.kt +++ b/posthog-samples/posthog-android-sample/src/main/java/com/posthog/android/sample/MainActivity.kt @@ -41,6 +41,7 @@ fun Greeting(name: String, modifier: Modifier = Modifier) { // PostHog.optOut() // PostHog.optIn() // PostHog.identify("my_distinct_id", properties = mapOf("my_property" to 1), userProperties = mapOf("name" to "hello")) + PostHog.register("test", mapOf("one" to "two")) PostHog.capture("testEvent", properties = mapOf("testProperty" to "testValue")) // PostHog.reloadFeatureFlagsRequest() // PostHog.isFeatureEnabled("sessionRecording") diff --git a/posthog/api/posthog.api b/posthog/api/posthog.api index 7f9d79c6..51f7d17b 100644 --- a/posthog/api/posthog.api +++ b/posthog/api/posthog.api @@ -210,6 +210,7 @@ public abstract interface class com/posthog/internal/PostHogPreferences { public static final field BUILD Ljava/lang/String; public static final field Companion Lcom/posthog/internal/PostHogPreferences$Companion; public static final field GROUPS Ljava/lang/String; + public static final field STRINGIFIED_KEYS Ljava/lang/String; public static final field VERSION Ljava/lang/String; public abstract fun clear (Ljava/util/List;)V public abstract fun getAll ()Ljava/util/Map; @@ -221,7 +222,9 @@ public abstract interface class com/posthog/internal/PostHogPreferences { public final class com/posthog/internal/PostHogPreferences$Companion { public static final field BUILD Ljava/lang/String; public static final field GROUPS Ljava/lang/String; + public static final field STRINGIFIED_KEYS Ljava/lang/String; public static final field VERSION Ljava/lang/String; + public final fun getALL_INTERNAL_KEYS ()Ljava/util/Set; } public final class com/posthog/internal/PostHogPreferences$DefaultImpls { diff --git a/posthog/src/main/java/com/posthog/PostHog.kt b/posthog/src/main/java/com/posthog/PostHog.kt index 758b29c3..36fa1d4e 100644 --- a/posthog/src/main/java/com/posthog/PostHog.kt +++ b/posthog/src/main/java/com/posthog/PostHog.kt @@ -4,6 +4,8 @@ import com.posthog.internal.PostHogApi import com.posthog.internal.PostHogCalendarDateProvider import com.posthog.internal.PostHogFeatureFlags import com.posthog.internal.PostHogMemoryPreferences +import com.posthog.internal.PostHogPreferences +import com.posthog.internal.PostHogPreferences.Companion.ALL_INTERNAL_KEYS import com.posthog.internal.PostHogPreferences.Companion.ANONYMOUS_ID import com.posthog.internal.PostHogPreferences.Companion.BUILD import com.posthog.internal.PostHogPreferences.Companion.DISTINCT_ID @@ -57,7 +59,7 @@ public class PostHog private constructor( config.logger.log("API Key: ${config.apiKey} already has a PostHog instance.") } - val cachePreferences = config.cachePreferences ?: PostHogMemoryPreferences() + val cachePreferences = config.cachePreferences ?: memoryPreferences config.cachePreferences = cachePreferences val dateProvider = PostHogCalendarDateProvider() val api = PostHogApi(config, dateProvider) @@ -65,7 +67,7 @@ public class PostHog private constructor( val featureFlags = PostHogFeatureFlags(config, api, featureFlagsExecutor) // no need to lock optOut here since the setup is locked already - val optOut = config.cachePreferences?.getValue( + val optOut = getPreferences().getValue( OPT_OUT, defaultValue = config.optOut, ) as? Boolean @@ -110,8 +112,12 @@ public class PostHog private constructor( } } + private fun getPreferences(): PostHogPreferences { + return config?.cachePreferences ?: memoryPreferences + } + private fun legacyPreferences(config: PostHogConfig, serializer: PostHogSerializer) { - val cachedPrefs = config.cachePreferences?.getValue(config.apiKey) as? String + val cachedPrefs = getPreferences().getValue(config.apiKey) as? String cachedPrefs?.let { try { serializer.deserialize?>(it.reader())?.let { props -> @@ -125,7 +131,7 @@ public class PostHog private constructor( this.distinctId = distId } - config.cachePreferences?.remove(config.apiKey) + getPreferences().remove(config.apiKey) } } catch (e: Throwable) { config.logger.log("Legacy cached prefs: $cachedPrefs failed to parse: $e.") @@ -164,7 +170,7 @@ public class PostHog private constructor( get() { var anonymousId: String? synchronized(anonymousLock) { - anonymousId = config?.cachePreferences?.getValue(ANONYMOUS_ID) as? String + anonymousId = getPreferences().getValue(ANONYMOUS_ID) as? String if (anonymousId == null) { anonymousId = UUID.randomUUID().toString() this.anonymousId = anonymousId ?: "" @@ -173,18 +179,18 @@ public class PostHog private constructor( return anonymousId ?: "" } set(value) { - config?.cachePreferences?.setValue(ANONYMOUS_ID, value) + getPreferences().setValue(ANONYMOUS_ID, value) } private var distinctId: String get() { - return config?.cachePreferences?.getValue( + return getPreferences().getValue( DISTINCT_ID, defaultValue = anonymousId, ) as? String ?: "" } set(value) { - config?.cachePreferences?.setValue(DISTINCT_ID, value) + getPreferences().setValue(DISTINCT_ID, value) } private fun buildProperties( @@ -197,7 +203,7 @@ public class PostHog private constructor( val props = mutableMapOf() if (appendSharedProps) { - val registeredPrefs = memoryPreferences.getAll() + val registeredPrefs = getPreferences().getAll() if (registeredPrefs.isNotEmpty()) { props.putAll(registeredPrefs) } @@ -287,7 +293,7 @@ public class PostHog private constructor( synchronized(lockOptOut) { config?.optOut = false - config?.cachePreferences?.setValue(OPT_OUT, false) + getPreferences().setValue(OPT_OUT, false) } } @@ -298,7 +304,7 @@ public class PostHog private constructor( synchronized(lockOptOut) { config?.optOut = true - config?.cachePreferences?.setValue(OPT_OUT, true) + getPreferences().setValue(OPT_OUT, true) } } @@ -393,8 +399,7 @@ public class PostHog private constructor( props["\$group_set"] = it } - // just defensive, if there's no cachePreferences, we fallback to in memory - val preferences = config?.cachePreferences ?: memoryPreferences + val preferences = getPreferences() @Suppress("UNCHECKED_CAST") val groups = preferences.getValue(GROUPS) as? Map @@ -429,11 +434,8 @@ public class PostHog private constructor( } private fun loadFeatureFlagsRequest(onFeatureFlags: PostHogOnFeatureFlags?) { - // just defensive, if there's no config.cachePreferences, we fallback to in memory - val preferences = config?.cachePreferences ?: memoryPreferences - @Suppress("UNCHECKED_CAST") - val groups = preferences.getValue(GROUPS) as? Map + val groups = getPreferences().getValue(GROUPS) as? Map featureFlags?.loadFeatureFlags(distinctId, anonymousId, groups, onFeatureFlags) } @@ -496,8 +498,7 @@ public class PostHog private constructor( // only remove properties, preserve BUILD and VERSION keys in order to to fix over-sending // of 'Application Installed' events and under-sending of 'Application Updated' events val except = listOf(VERSION, BUILD) - memoryPreferences.clear(except = except) - config?.cachePreferences?.clear(except = except) + getPreferences().clear(except = except) featureFlags?.clear() queue?.clear() featureFlagsCalled.clear() @@ -514,14 +515,18 @@ public class PostHog private constructor( if (!isEnabled()) { return } - memoryPreferences.setValue(key, value) + if (ALL_INTERNAL_KEYS.contains(key)) { + config?.logger?.log("Key: $key is reserved for internal use.") + return + } + getPreferences().setValue(key, value) } public override fun unregister(key: String) { if (!isEnabled()) { return } - memoryPreferences.remove(key) + getPreferences().remove(key) } override fun distinctId(): String { diff --git a/posthog/src/main/java/com/posthog/PostHogInterface.kt b/posthog/src/main/java/com/posthog/PostHogInterface.kt index 3691c90f..b5bd4c46 100644 --- a/posthog/src/main/java/com/posthog/PostHogInterface.kt +++ b/posthog/src/main/java/com/posthog/PostHogInterface.kt @@ -129,14 +129,17 @@ public interface PostHogInterface { public fun isOptOut(): Boolean /** - * Register a property to always be sent within this session + * Register a property to always be sent with all the following events until you call + * [unregister] with the same key + * PostHogPreferences.ALL_INTERNAL_KEYS are not allowed since they are internal and used by + * the SDK only. * @param key the Key * @param value the Value */ public fun register(key: String, value: Any) /** - * Unregisters the previously set property to be sent within this session + * Unregisters the previously set property to be sent with all the following events * @param key the Key */ public fun unregister(key: String) diff --git a/posthog/src/main/java/com/posthog/internal/PostHogMemoryPreferences.kt b/posthog/src/main/java/com/posthog/internal/PostHogMemoryPreferences.kt index 3db71faa..fb4e341c 100644 --- a/posthog/src/main/java/com/posthog/internal/PostHogMemoryPreferences.kt +++ b/posthog/src/main/java/com/posthog/internal/PostHogMemoryPreferences.kt @@ -47,6 +47,8 @@ public class PostHogMemoryPreferences : PostHogPreferences { synchronized(lock) { props = preferences.toMap() } - return props + return props.filterKeys { key -> + !PostHogPreferences.ALL_INTERNAL_KEYS.contains(key) + } } } diff --git a/posthog/src/main/java/com/posthog/internal/PostHogPreferences.kt b/posthog/src/main/java/com/posthog/internal/PostHogPreferences.kt index e1902ca4..5cb34b99 100644 --- a/posthog/src/main/java/com/posthog/internal/PostHogPreferences.kt +++ b/posthog/src/main/java/com/posthog/internal/PostHogPreferences.kt @@ -25,5 +25,16 @@ public interface PostHogPreferences { internal const val OPT_OUT = "opt-out" public const val VERSION: String = "version" public const val BUILD: String = "build" + public const val STRINGIFIED_KEYS: String = "stringifiedKeys" + + public val ALL_INTERNAL_KEYS: Set = setOf( + GROUPS, + ANONYMOUS_ID, + DISTINCT_ID, + OPT_OUT, + VERSION, + BUILD, + STRINGIFIED_KEYS, + ) } } diff --git a/posthog/src/test/java/com/posthog/PostHogTest.kt b/posthog/src/test/java/com/posthog/PostHogTest.kt index 6544f1d0..c1a32fe8 100644 --- a/posthog/src/test/java/com/posthog/PostHogTest.kt +++ b/posthog/src/test/java/com/posthog/PostHogTest.kt @@ -558,6 +558,38 @@ internal class PostHogTest { sut.close() } + @Test + fun `registers ignore internal keys`() { + val http = mockHttp() + val url = http.url("/") + + val sut = getSut(url.toString(), preloadFeatureFlags = false) + + sut.register("version", "123") + + sut.capture( + event, + distinctId, + props, + userProperties = userProps, + userPropertiesSetOnce = userPropsOnce, + groupProperties = groupProps, + ) + + queueExecutor.shutdownAndAwaitTermination() + + val request = http.takeRequest() + + val content = request.body.unGzip() + val batch = serializer.deserialize(content.reader()) + + val theEvent = batch.batch.first() + + assertNull(theEvent.properties!!["version"]) + + sut.close() + } + @Test fun `unregister removes property`() { val http = mockHttp()