From c1aa1ecee8ac9a42d07570fcdb2f546ba28aa0ce Mon Sep 17 00:00:00 2001 From: Abhishek Pandey <64667840+1abhishekpandey@users.noreply.github.com> Date: Sun, 30 Jun 2024 17:19:54 +0530 Subject: [PATCH] fix: remove userId from the configurationAndroid class (#448) * fix: remove userId from ConfigurationAndroid and ConfigurationAndroidBuilder UserId should not be part of the sdk configuration. It can only be modified by the identify event. * fix: remove the code to update the userId in configuration object * refactor: use `androidStorage` to fetch the userId * test: fix failing test case in FillDefaultsPluginTest Use real storage object so that userId could be fetched * refactor: directly set the userId in storage Analytics.setUserId extension function was required for two purpose: First is to store the userId in storage and second is to update the userId configuration. Now we no longer require the latter part we can simplify the logic. * chore: remove setting of userId from v2Cache As we are no longer updating the userId in configuration object, we don't need to basically fetch the userId from the storage and again store it in storage. * refactor: create an extension function for empty string and use that --- .../android/ConfigurationAndroid.kt | 10 ------ .../compat/ConfigurationAndroidBuilder.java | 6 ---- .../infrastructure/ReinstatePlugin.kt | 7 +--- .../internal/plugins/ExtractStatePlugin.kt | 9 +++-- .../internal/plugins/FillDefaultsPlugin.kt | 5 +-- .../android/utilities/AnalyticsUtil.kt | 19 ++--------- .../android/plugins/FillDefaultsPluginTest.kt | 34 +++++++++---------- 7 files changed, 28 insertions(+), 62 deletions(-) diff --git a/android/src/main/java/com/rudderstack/android/ConfigurationAndroid.kt b/android/src/main/java/com/rudderstack/android/ConfigurationAndroid.kt index 03f283ae..f619f246 100644 --- a/android/src/main/java/com/rudderstack/android/ConfigurationAndroid.kt +++ b/android/src/main/java/com/rudderstack/android/ConfigurationAndroid.kt @@ -33,7 +33,6 @@ interface ConfigurationAndroid : Configuration { * * @property application * @property anonymousId - * @property userId * @property trackLifecycleEvents * @property recordScreenViews * @property isPeriodicFlushEnabled @@ -62,7 +61,6 @@ interface ConfigurationAndroid : Configuration { */ val application: Application val anonymousId: String? - val userId: String? val trackLifecycleEvents: Boolean val recordScreenViews: Boolean val isPeriodicFlushEnabled: Boolean @@ -83,7 +81,6 @@ interface ConfigurationAndroid : Configuration { application: Application, jsonAdapter: JsonAdapter, anonymousId: String? = null, - userId: String? = null, options: RudderOption = RudderOption(), flushQueueSize: Int = Defaults.DEFAULT_FLUSH_QUEUE_SIZE, maxFlushInterval: Long = Defaults.DEFAULT_MAX_FLUSH_INTERVAL, @@ -112,7 +109,6 @@ interface ConfigurationAndroid : Configuration { application, jsonAdapter, anonymousId, - userId, options, flushQueueSize, maxFlushInterval, @@ -143,7 +139,6 @@ interface ConfigurationAndroid : Configuration { application: Application, jsonAdapter: JsonAdapter, anonymousId: String? = null, - userId: String? = null, options: RudderOption = RudderOption(), flushQueueSize: Int = Defaults.DEFAULT_FLUSH_QUEUE_SIZE, maxFlushInterval: Long = Defaults.DEFAULT_MAX_FLUSH_INTERVAL, @@ -175,7 +170,6 @@ interface ConfigurationAndroid : Configuration { ): ConfigurationAndroid = object : ConfigurationAndroid { override val application: Application = application override val anonymousId: String? = anonymousId - override val userId: String? = userId override val trackLifecycleEvents: Boolean = trackLifecycleEvents override val recordScreenViews: Boolean = recordScreenViews override val isPeriodicFlushEnabled: Boolean = isPeriodicFlushEnabled @@ -210,7 +204,6 @@ interface ConfigurationAndroid : Configuration { configuration: Configuration, application: Application, anonymousId: String = AndroidUtils.generateAnonymousId(Defaults.COLLECT_DEVICE_ID, application), - userId: String? = null, trackLifecycleEvents: Boolean = Defaults.TRACK_LIFECYCLE_EVENTS, recordScreenViews: Boolean = Defaults.RECORD_SCREEN_VIEWS, isPeriodicFlushEnabled: Boolean = Defaults.IS_PERIODIC_FLUSH_ENABLED, @@ -229,7 +222,6 @@ interface ConfigurationAndroid : Configuration { application, configuration.jsonAdapter, anonymousId, - userId, configuration.options, configuration.flushQueueSize, configuration.maxFlushInterval, @@ -302,7 +294,6 @@ interface ConfigurationAndroid : Configuration { networkExecutor: ExecutorService = this.networkExecutor, base64Generator: Base64Generator = this.base64Generator, anonymousId: String? = this.anonymousId, - userId: String? = this.userId, advertisingId: String? = this.advertisingId, autoCollectAdvertId: Boolean = this.autoCollectAdvertId, deviceToken: String? = this.deviceToken, @@ -313,7 +304,6 @@ interface ConfigurationAndroid : Configuration { application, jsonAdapter, anonymousId, - userId, options, flushQueueSize, maxFlushInterval, diff --git a/android/src/main/java/com/rudderstack/android/compat/ConfigurationAndroidBuilder.java b/android/src/main/java/com/rudderstack/android/compat/ConfigurationAndroidBuilder.java index 374ef856..4dd57eab 100644 --- a/android/src/main/java/com/rudderstack/android/compat/ConfigurationAndroidBuilder.java +++ b/android/src/main/java/com/rudderstack/android/compat/ConfigurationAndroidBuilder.java @@ -30,7 +30,6 @@ public class ConfigurationAndroidBuilder extends ConfigurationBuilder { private final Application application ; private String anonymousId; - private String userId = null; private Boolean trackLifecycleEvents = ConfigurationAndroid.Defaults.TRACK_LIFECYCLE_EVENTS; private Boolean recordScreenViews = ConfigurationAndroid.Defaults.RECORD_SCREEN_VIEWS; private Boolean isPeriodicFlushEnabled = ConfigurationAndroid.Defaults.IS_PERIODIC_FLUSH_ENABLED; @@ -54,10 +53,6 @@ public ConfigurationBuilder withAnonymousId(String anonymousId) { this.anonymousId = anonymousId; return this; } - public ConfigurationBuilder withUserId(String userId) { - this.userId = userId; - return this; - } public ConfigurationBuilder withTrackLifecycleEvents(Boolean trackLifecycleEvents) { this.trackLifecycleEvents = trackLifecycleEvents; return this; @@ -118,7 +113,6 @@ public ConfigurationAndroid build() { return ConfigurationAndroid.Companion.invoke(super.build(), application, anonymousId, - userId, trackLifecycleEvents, recordScreenViews, isPeriodicFlushEnabled, diff --git a/android/src/main/java/com/rudderstack/android/internal/infrastructure/ReinstatePlugin.kt b/android/src/main/java/com/rudderstack/android/internal/infrastructure/ReinstatePlugin.kt index 23abcaae..60017425 100644 --- a/android/src/main/java/com/rudderstack/android/internal/infrastructure/ReinstatePlugin.kt +++ b/android/src/main/java/com/rudderstack/android/internal/infrastructure/ReinstatePlugin.kt @@ -22,7 +22,6 @@ import com.rudderstack.android.utilities.currentConfigurationAndroid import com.rudderstack.android.utilities.initializeSessionManagement import com.rudderstack.android.utilities.processNewContext import com.rudderstack.android.utilities.setAnonymousId -import com.rudderstack.android.utilities.setUserId import com.rudderstack.core.Analytics import com.rudderstack.core.DataUploadService import com.rudderstack.core.InfrastructurePlugin @@ -84,7 +83,6 @@ internal class ReinstatePlugin : InfrastructurePlugin { } private fun reinstateV2FromCache() { - val userId = _analytics?.androidStorage?.userId val anonId = _analytics?.androidStorage?.anonymousId ?: _analytics?.currentConfigurationAndroid?.let { AndroidUtils.generateAnonymousId( @@ -96,9 +94,6 @@ internal class ReinstatePlugin : InfrastructurePlugin { context?.let { _analytics?.processNewContext(context) } - userId?.let { - _analytics?.setUserId(it) - } if (anonId != null) _analytics?.setAnonymousId(anonId) _analytics?.initializeSessionManagement( @@ -146,7 +141,7 @@ internal class ReinstatePlugin : InfrastructurePlugin { val traits = androidStorage.v1Traits val userId = traits?.get("userId") as? String ?: traits?.get("id") as? String if (userId.isNullOrEmpty() || !this.androidStorage.userId.isNullOrEmpty()) return - _analytics?.setUserId(userId) + androidStorage.setUserId(userId) } private fun Analytics.migrateAnonymousIdFromV1() { diff --git a/android/src/main/java/com/rudderstack/android/internal/plugins/ExtractStatePlugin.kt b/android/src/main/java/com/rudderstack/android/internal/plugins/ExtractStatePlugin.kt index b8eca094..a66def19 100644 --- a/android/src/main/java/com/rudderstack/android/internal/plugins/ExtractStatePlugin.kt +++ b/android/src/main/java/com/rudderstack/android/internal/plugins/ExtractStatePlugin.kt @@ -14,10 +14,11 @@ package com.rudderstack.android.internal.plugins +import com.rudderstack.android.utilities.androidStorage import com.rudderstack.android.utilities.contextState import com.rudderstack.android.utilities.currentConfigurationAndroid +import com.rudderstack.android.utilities.empty import com.rudderstack.android.utilities.processNewContext -import com.rudderstack.android.utilities.setUserId import com.rudderstack.core.Analytics import com.rudderstack.core.Plugin import com.rudderstack.core.optAdd @@ -65,9 +66,7 @@ internal class ExtractStatePlugin : Plugin { val newUserId = getUserId(message) _analytics?.rudderLogger?.debug(log = "New user id detected: $newUserId") - val prevId = _analytics?.currentConfigurationAndroid?.let { - it.userId ?: it.anonymousId - } ?: "" + val prevId = _analytics?.androidStorage?.userId ?: _analytics?.currentConfigurationAndroid?.anonymousId ?: String.empty() // in case of identify, the stored traits (if any) are replaced by the ones provided // if user id is different. else traits are added to it val msg = when (message) { @@ -95,7 +94,7 @@ internal class ExtractStatePlugin : Plugin { }?:message msg.also { newUserId?.let { id -> - _analytics?.setUserId(id) + _analytics?.androidStorage?.setUserId(id) } } return chain.proceed(msg) diff --git a/android/src/main/java/com/rudderstack/android/internal/plugins/FillDefaultsPlugin.kt b/android/src/main/java/com/rudderstack/android/internal/plugins/FillDefaultsPlugin.kt index fff694ba..09c2edc4 100644 --- a/android/src/main/java/com/rudderstack/android/internal/plugins/FillDefaultsPlugin.kt +++ b/android/src/main/java/com/rudderstack/android/internal/plugins/FillDefaultsPlugin.kt @@ -14,6 +14,7 @@ package com.rudderstack.android.internal.plugins +import com.rudderstack.android.utilities.androidStorage import com.rudderstack.android.utilities.contextState import com.rudderstack.android.utilities.currentConfigurationAndroid import com.rudderstack.core.Analytics @@ -51,7 +52,7 @@ internal class FillDefaultsPlugin : Plugin { @Throws(MissingPropertiesException::class) private inline fun T.withDefaults(): T { val anonId = this.anonymousId ?: _analytics?.currentConfigurationAndroid?.anonymousId - val userId = this.userId ?: _analytics?.currentConfigurationAndroid?.userId + val userId = this.userId ?: _analytics?.androidStorage?.userId if (anonId == null && userId == null) { val ex = MissingPropertiesException("Either Anonymous Id or User Id must be present"); _analytics?.currentConfigurationAndroid?.rudderLogger?.error( @@ -64,7 +65,7 @@ internal class FillDefaultsPlugin : Plugin { val newContext = // in case of alias we purposefully remove traits from context _analytics?.contextState?.value?.let { - if (this is AliasMessage && this.userId != _analytics?.currentConfigurationAndroid?.userId) it.updateWith( + if (this is AliasMessage && this.userId != _analytics?.androidStorage?.userId) it.updateWith( traits = mapOf() ) else it } selectiveReplace context.let { diff --git a/android/src/main/java/com/rudderstack/android/utilities/AnalyticsUtil.kt b/android/src/main/java/com/rudderstack/android/utilities/AnalyticsUtil.kt index dc7b8efc..3a36edbe 100644 --- a/android/src/main/java/com/rudderstack/android/utilities/AnalyticsUtil.kt +++ b/android/src/main/java/com/rudderstack/android/utilities/AnalyticsUtil.kt @@ -85,21 +85,6 @@ fun Analytics.setAnonymousId(anonymousId: String) { processNewContext(newContext) } -/** - * Setting the [ConfigurationAndroid.userId] explicitly. - * - * @param userId String to be used as userId - */ -fun Analytics.setUserId(userId: String) { - androidStorage.setUserId(userId) - applyConfiguration { - if (this is ConfigurationAndroid) copy( - userId = userId - ) - else this - } -} - private val infrastructurePlugins = arrayOf( ReinstatePlugin(), AnonymousIdHeaderPlugin(), @@ -157,4 +142,6 @@ private fun Analytics.attachSavedContextIfAvailable() { androidStorage.context?.let { processNewContext(it) } -} \ No newline at end of file +} + +fun String.Companion.empty(): String = "" diff --git a/android/src/test/java/com/rudderstack/android/plugins/FillDefaultsPluginTest.kt b/android/src/test/java/com/rudderstack/android/plugins/FillDefaultsPluginTest.kt index 5115e9fd..038c92b8 100644 --- a/android/src/test/java/com/rudderstack/android/plugins/FillDefaultsPluginTest.kt +++ b/android/src/test/java/com/rudderstack/android/plugins/FillDefaultsPluginTest.kt @@ -20,13 +20,16 @@ import com.rudderstack.android.ConfigurationAndroid import com.rudderstack.android.utils.TestExecutor import com.rudderstack.android.internal.plugins.FillDefaultsPlugin import com.rudderstack.android.internal.states.ContextState +import com.rudderstack.android.storage.AndroidStorage +import com.rudderstack.android.storage.AndroidStorageImpl import com.rudderstack.core.Analytics import com.rudderstack.core.RudderLogger import com.rudderstack.core.RudderUtils import com.rudderstack.core.holder.associateState import com.rudderstack.core.holder.retrieveState +import com.rudderstack.gsonrudderadapter.GsonAdapter import com.rudderstack.models.* -import com.vagabond.testcommon.Verification +import com.rudderstack.rudderjsonadapter.JsonAdapter import com.vagabond.testcommon.assertArgument import com.vagabond.testcommon.generateTestAnalytics import com.vagabond.testcommon.testPlugin @@ -36,7 +39,6 @@ import org.junit.After import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.mockito.kotlin.mock import org.robolectric.RobolectricTestRunner import org.robolectric.annotation.Config @@ -46,34 +48,36 @@ import org.robolectric.annotation.Config @Config(manifest = Config.NONE, sdk = [Build.VERSION_CODES.P]) class FillDefaultsPluginTest { - // private val commonContext = mapOf( -// "some_context1" to "some_value", -// "some_context2" to "some_value_2" -// ) private lateinit var analytics: Analytics lateinit var mockConfig: ConfigurationAndroid - - private val fillDefaultsPlugin = FillDefaultsPlugin( - ) + private val fillDefaultsPlugin = FillDefaultsPlugin() + private val jsonAdapter: JsonAdapter = GsonAdapter() + private lateinit var storage: AndroidStorage @Before fun setup() { + storage = AndroidStorageImpl( + getApplicationContext(), + false, + writeKey = "test_writeKey", + storageExecutor = TestExecutor(), + ) mockConfig = ConfigurationAndroid( application = getApplicationContext(), - mock(), + jsonAdapter, anonymousId = "anon_id", - userId = "user_id", shouldVerifySdk = false, analyticsExecutor = TestExecutor(), logLevel = RudderLogger.LogLevel.DEBUG, ) - analytics = generateTestAnalytics(mockConfig) + analytics = generateTestAnalytics(mockConfig, storage = storage) analytics.associateState(ContextState()) fillDefaultsPlugin.setup(analytics) fillDefaultsPlugin.updateConfiguration(mockConfig) } @After fun destroy() { + analytics.storage.clearStorage() analytics.shutdown() } @@ -104,21 +108,18 @@ class FillDefaultsPluginTest { ), customContextMap = null ) -// val chain = CentralPluginChain(message, listOf(fillDefaultsPlugin)) analytics.testPlugin(fillDefaultsPlugin) analytics.track(message) analytics.assertArgument { input, output -> //check for expected values assertThat(output?.anonymousId, allOf(notNullValue(), `is`("anon_id"))) - assertThat(output?.userId, allOf(notNullValue(), `is`("user_id"))) //message context to override assertThat( output?.context?.traits, allOf( notNullValue(), aMapWithSize(2), - hasEntry("age", 31), + hasEntry("age", 31.0), hasEntry("office", "Rudderstack"), -// hasEntry("name", "some_name"), ) ) assertThat( @@ -137,6 +138,5 @@ class FillDefaultsPluginTest { ) } - } }