Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix session start data issue #253

Merged
merged 5 commits into from
Jan 28, 2025
Merged

Fix session start data issue #253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
730f751
Revert "Fallback to the defaultSettings if cdn cannot be reached (#231)"
didiergarcia Jan 16, 2025
b7fbef2
Update tests after reverting.
didiergarcia Jan 16, 2025
d1b7bc4
add a sane default Settings object similar to swift.
didiergarcia Jan 16, 2025
e018a4d
fix: use a sane default settings object in cases where we can't connect.
didiergarcia Jan 21, 2025
d8112d3
fix: set DestinationPlugins to enabled by default.
didiergarcia Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/com/segment/analytics/kotlin/core/Configuration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ data class Configuration(
var flushAt: Int = 20,
var flushInterval: Int = 30,
var flushPolicies: List<FlushPolicy> = emptyList<FlushPolicy>(),
var defaultSettings: Settings = Settings(),
var defaultSettings: Settings? = null,
var autoAddSegmentDestination: Boolean = true,
var apiHost: String = DEFAULT_API_HOST,
var cdnHost: String = DEFAULT_CDN_HOST,
Expand Down
44 changes: 25 additions & 19 deletions core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ suspend fun Analytics.checkSettings() {
val settingsObj: Settings? = fetchSettings(writeKey, cdnHost)

withContext(analyticsDispatcher) {

settingsObj?.let {
log("Dispatching update settings on ${Thread.currentThread().name}")
store.dispatch(System.UpdateSettingsAction(settingsObj), System::class)
Expand All @@ -108,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"
}
configuration.defaultSettings
}
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
}
38 changes: 31 additions & 7 deletions core/src/main/java/com/segment/analytics/kotlin/core/State.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +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) {
configuration.defaultSettings
val storedSettings = storage.read(Storage.Constants.Settings)
val defaultSettings = configuration.defaultSettings ?: Settings(
integrations = buildJsonObject {
put(
"Segment.io",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better to make this a constant in SegmentDestination and reference it here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add constructors in Settings so it's consistent with swift's implementation and that forces every settings created to have a segment destination

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree, there are too many differences :( from swift.

buildJsonObject {
put(
"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,
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/segment/analytics/kotlin/core/platform/Plugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catch! otherwise we might have issue when the user first opens the app offline and plugins are disabled by default.

abstract val key: String

override fun setup(analytics: Analytics) {
Expand Down
79 changes: 37 additions & 42 deletions core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -102,7 +104,7 @@ class SettingsTests {
// load settings again
mockHTTPClient()
analytics.checkSettings()
verify (exactly = 2) {
verify (exactly = 1) {
mockPlugin.update(any(), Plugin.UpdateType.Refresh)
}
}
Expand Down Expand Up @@ -230,102 +232,95 @@ 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("<!DOCTYPE html>")
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
fun `fetchSettings returns null when Settings string is null for known properties`() {
// 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")
Expand All @@ -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)
}
}
Loading