From 730f751246254cf9ccd7a13a011e5fa3545cb304 Mon Sep 17 00:00:00 2001 From: Didier Garcia Date: Thu, 16 Jan 2025 11:35:53 -0500 Subject: [PATCH 1/5] Revert "Fallback to the defaultSettings if cdn cannot be reached (#231)" This reverts commit d37d2d32cecb3a698d7f679057334cabc1ec8ca8. --- .../com/segment/analytics/kotlin/core/Settings.kt | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt b/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt index 197dafa4..62218dee 100644 --- a/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt +++ b/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt @@ -92,7 +92,16 @@ suspend fun Analytics.checkSettings() { val settingsObj: Settings? = fetchSettings(writeKey, cdnHost) withContext(analyticsDispatcher) { - settingsObj?.let { + + val systemState = store.currentState(System::class) + val defaultSettings = systemState?.settings + + + if (settingsObj == null) { + defaultSettings?.let { + update(defaultSettings) + } + } else { log("Dispatching update settings on ${Thread.currentThread().name}") store.dispatch(System.UpdateSettingsAction(settingsObj), System::class) update(settingsObj) @@ -125,5 +134,5 @@ internal fun Analytics.fetchSettings( it["writekey"] = writeKey it["message"] = "Error retrieving settings" } - configuration.defaultSettings + null } \ No newline at end of file From b7fbef296150353d7cef3f7eb5f098bff1a86599 Mon Sep 17 00:00:00 2001 From: Didier Garcia Date: Thu, 16 Jan 2025 11:37:21 -0500 Subject: [PATCH 2/5] Update tests after reverting. --- .../analytics/kotlin/core/SettingsTests.kt | 79 +++++++++---------- 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt b/core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt index 56a1bda8..d67d6963 100644 --- a/core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt +++ b/core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt @@ -88,7 +88,9 @@ class SettingsTests { // no settings available, should not be called analytics.add(mockPlugin) - + verify (exactly = 0){ + mockPlugin.update(any(), any()) + } // load settings mockHTTPClient() @@ -102,7 +104,7 @@ class SettingsTests { // load settings again mockHTTPClient() analytics.checkSettings() - verify (exactly = 2) { + verify (exactly = 1) { mockPlugin.update(any(), Plugin.UpdateType.Refresh) } } @@ -230,69 +232,67 @@ class SettingsTests { @Test fun `fetchSettings returns null when Settings string is invalid`() { - val emptySettings = analytics.fetchSettings("emptySettingsObject", "cdn-settings.segment.com/v1") // Null on invalid JSON mockHTTPClient("") var settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("hello") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("#! /bin/sh") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("true") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("[]") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("}{") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("{{{{}}}}") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null on invalid JSON mockHTTPClient("{null:\"bar\"}") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) } @Test fun `fetchSettings returns null when parameters are invalid`() { - val emptySettings = analytics.fetchSettings("emptySettingsObject", "cdn-settings.segment.com/v1") mockHTTPClient("{\"integrations\":{}, \"plan\":{}, \"edgeFunction\": {}, \"middlewareSettings\": {}}") // empty host var settings = analytics.fetchSettings("foo", "") - assertEquals(emptySettings, settings) + assertNull(settings) // not a host name settings = analytics.fetchSettings("foo", "http://blah") - assertEquals(emptySettings, settings) + assertNull(settings) // emoji settings = analytics.fetchSettings("foo", "😃") - assertEquals(emptySettings, settings) + assertNull(settings) } @Test @@ -300,32 +300,27 @@ class SettingsTests { // Null if integrations is null mockHTTPClient("{\"integrations\":null}") var settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertTrue(settings?.integrations?.isEmpty() ?: true, "Integrations should be empty") - assertTrue(settings?.plan?.isEmpty() ?: true, "Plan should be empty") - assertTrue(settings?.edgeFunction?.isEmpty() ?: true, "EdgeFunction should be empty") - assertTrue(settings?.middlewareSettings?.isEmpty() ?: true, "MiddlewareSettings should be empty") - assertTrue(settings?.metrics?.isEmpty() ?: true, "Metrics should be empty") - assertTrue(settings?.consentSettings?.isEmpty() ?: true, "ConsentSettings should be empty") - -// // Null if plan is null -// mockHTTPClient("{\"plan\":null}") -// settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") -// assertNull(settings) -// -// // Null if edgeFunction is null -// mockHTTPClient("{\"edgeFunction\":null}") -// settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") -// assertNull(settings) -// -// // Null if middlewareSettings is null -// mockHTTPClient("{\"middlewareSettings\":null}") -// settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") -// assertNull(settings) + assertNull(settings) + + // Null if plan is null + mockHTTPClient("{\"plan\":null}") + settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") + assertNull(settings) + + // Null if edgeFunction is null + mockHTTPClient("{\"edgeFunction\":null}") + settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") + assertNull(settings) + + // Null if middlewareSettings is null + mockHTTPClient("{\"middlewareSettings\":null}") + settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") + assertNull(settings) } @Test fun `known Settings property types must match json type`() { - val emptySettings = analytics.fetchSettings("emptySettingsObject", "cdn-settings.segment.com/v1") + // integrations must be a JSON object mockHTTPClient("{\"integrations\":{}}") var settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") @@ -334,21 +329,21 @@ class SettingsTests { // Null if integrations is a number mockHTTPClient("{\"integrations\":123}") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null if integrations is a string mockHTTPClient("{\"integrations\":\"foo\"}") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null if integrations is an array mockHTTPClient("{\"integrations\":[\"foo\"]}") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) // Null if integrations is an emoji (UTF-8 string) mockHTTPClient("{\"integrations\": 😃}") settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1") - assertEquals(emptySettings, settings) + assertNull(settings) } } \ No newline at end of file From d1b7bc4762721c6624a7d80eda5393d0e096838a Mon Sep 17 00:00:00 2001 From: Didier Garcia Date: Thu, 16 Jan 2025 11:38:05 -0500 Subject: [PATCH 3/5] add a sane default Settings object similar to swift. --- .../segment/analytics/kotlin/core/State.kt | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/State.kt b/core/src/main/java/com/segment/analytics/kotlin/core/State.kt index 21275a88..c5b8eafc 100644 --- a/core/src/main/java/com/segment/analytics/kotlin/core/State.kt +++ b/core/src/main/java/com/segment/analytics/kotlin/core/State.kt @@ -31,7 +31,30 @@ data class System( storage.read(Storage.Constants.Settings) ?: "" ) } catch (ignored: Exception) { - configuration.defaultSettings + + // This could be an empty Settings... + if (configuration.defaultSettings.integrations.contains("Segment.io")) { + configuration.defaultSettings + } else { + Settings( + integrations = buildJsonObject { + put( + "Segment.io", + buildJsonObject { + put( + "apiKey", + configuration.writeKey + ) + put("apiHost", Constants.DEFAULT_API_HOST) + }) + }, + plan = emptyJsonObject, + edgeFunction = emptyJsonObject, + middlewareSettings = emptyJsonObject + ) + } + + } return System( configuration = configuration, From e018a4d07e171be5470b75e196a8759e100e8d84 Mon Sep 17 00:00:00 2001 From: Didier Garcia Date: Tue, 21 Jan 2025 13:27:04 -0500 Subject: [PATCH 4/5] fix: use a sane default settings object in cases where we can't connect. --- .../analytics/kotlin/core/Configuration.kt | 2 +- .../segment/analytics/kotlin/core/Settings.kt | 53 +++++++++--------- .../segment/analytics/kotlin/core/State.kt | 55 ++++++++++--------- 3 files changed, 54 insertions(+), 56 deletions(-) diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/Configuration.kt b/core/src/main/java/com/segment/analytics/kotlin/core/Configuration.kt index a7ee8895..8ef1c8d2 100644 --- a/core/src/main/java/com/segment/analytics/kotlin/core/Configuration.kt +++ b/core/src/main/java/com/segment/analytics/kotlin/core/Configuration.kt @@ -33,7 +33,7 @@ data class Configuration( var flushAt: Int = 20, var flushInterval: Int = 30, var flushPolicies: List = emptyList(), - var defaultSettings: Settings = Settings(), + var defaultSettings: Settings? = null, var autoAddSegmentDestination: Boolean = true, var apiHost: String = DEFAULT_API_HOST, var cdnHost: String = DEFAULT_CDN_HOST, diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt b/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt index 62218dee..073b7cbf 100644 --- a/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt +++ b/core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt @@ -93,15 +93,7 @@ suspend fun Analytics.checkSettings() { withContext(analyticsDispatcher) { - val systemState = store.currentState(System::class) - val defaultSettings = systemState?.settings - - - if (settingsObj == null) { - defaultSettings?.let { - update(defaultSettings) - } - } else { + settingsObj?.let { log("Dispatching update settings on ${Thread.currentThread().name}") store.dispatch(System.UpdateSettingsAction(settingsObj), System::class) update(settingsObj) @@ -117,22 +109,27 @@ internal fun Analytics.fetchSettings( writeKey: String, cdnHost: String ): Settings? = try { - val connection = HTTPClient(writeKey, this.configuration.requestFactory).settings(cdnHost) - val settingsString = - connection.inputStream?.bufferedReader()?.use(BufferedReader::readText) ?: "" - log("Fetched Settings: $settingsString") - LenientJson.decodeFromString(settingsString) - } catch (ex: Exception) { - reportErrorWithMetrics( - this, - AnalyticsError.SettingsFail(AnalyticsError.NetworkUnknown(URL("https://$cdnHost/projects/$writeKey/settings"), ex)), - "Failed to fetch settings", - Telemetry.INVOKE_ERROR_METRIC, - ex.stackTraceToString() - ) { - it["error"] = ex.toString() - it["writekey"] = writeKey - it["message"] = "Error retrieving settings" - } - null - } \ No newline at end of file + val connection = HTTPClient(writeKey, this.configuration.requestFactory).settings(cdnHost) + val settingsString = + connection.inputStream?.bufferedReader()?.use(BufferedReader::readText) ?: "" + log("Fetched Settings: $settingsString") + LenientJson.decodeFromString(settingsString) +} catch (ex: Exception) { + reportErrorWithMetrics( + this, + AnalyticsError.SettingsFail( + AnalyticsError.NetworkUnknown( + URL("https://$cdnHost/projects/$writeKey/settings"), + ex + ) + ), + "Failed to fetch settings", + Telemetry.INVOKE_ERROR_METRIC, + ex.stackTraceToString() + ) { + it["error"] = ex.toString() + it["writekey"] = writeKey + it["message"] = "Error retrieving settings" + } + null +} \ No newline at end of file diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/State.kt b/core/src/main/java/com/segment/analytics/kotlin/core/State.kt index c5b8eafc..374c56c5 100644 --- a/core/src/main/java/com/segment/analytics/kotlin/core/State.kt +++ b/core/src/main/java/com/segment/analytics/kotlin/core/State.kt @@ -25,37 +25,38 @@ data class System( companion object { fun defaultState(configuration: Configuration, storage: Storage): System { - val settings = try { - Json.decodeFromString( - Settings.serializer(), - storage.read(Storage.Constants.Settings) ?: "" - ) - } catch (ignored: Exception) { - - // This could be an empty Settings... - if (configuration.defaultSettings.integrations.contains("Segment.io")) { - configuration.defaultSettings - } else { - Settings( - integrations = buildJsonObject { + val storedSettings = storage.read(Storage.Constants.Settings) + val defaultSettings = configuration.defaultSettings ?: Settings( + integrations = buildJsonObject { + put( + "Segment.io", + buildJsonObject { put( - "Segment.io", - buildJsonObject { - put( - "apiKey", - configuration.writeKey - ) - put("apiHost", Constants.DEFAULT_API_HOST) - }) - }, - plan = emptyJsonObject, - edgeFunction = emptyJsonObject, - middlewareSettings = emptyJsonObject + "apiKey", + configuration.writeKey + ) + put("apiHost", Constants.DEFAULT_API_HOST) + }) + }, + plan = emptyJsonObject, + edgeFunction = emptyJsonObject, + middlewareSettings = emptyJsonObject + ) + + // Use stored settings or fallback to default settings + val settings = if (storedSettings == null || storedSettings == "" || storedSettings == "{}") { + defaultSettings + } else { + try { + Json.decodeFromString( + Settings.serializer(), + storedSettings ) + } catch (ignored: Exception) { + defaultSettings } - - } + return System( configuration = configuration, settings = settings, From d8112d33602326093d112c7d669e31516fc3f017 Mon Sep 17 00:00:00 2001 From: Didier Garcia Date: Mon, 27 Jan 2025 12:07:20 -0500 Subject: [PATCH 5/5] fix: set DestinationPlugins to enabled by default. --- .../java/com/segment/analytics/kotlin/core/platform/Plugin.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/segment/analytics/kotlin/core/platform/Plugin.kt b/core/src/main/java/com/segment/analytics/kotlin/core/platform/Plugin.kt index 4fb337a2..0be7decb 100644 --- a/core/src/main/java/com/segment/analytics/kotlin/core/platform/Plugin.kt +++ b/core/src/main/java/com/segment/analytics/kotlin/core/platform/Plugin.kt @@ -89,7 +89,7 @@ abstract class DestinationPlugin : EventPlugin { override val type: Plugin.Type = Plugin.Type.Destination private val timeline: Timeline = Timeline() override lateinit var analytics: Analytics - internal var enabled = false + internal var enabled = true abstract val key: String override fun setup(analytics: Analytics) {