Quick fix to bug with push token request equality operator #117

evan-masseau · 2024-01-17T22:17:39Z

Description

Fixes #116 by removing the lateinit property. Instead, keep body itself as the initial value, and use requestBody to append metadata at send time. This way the equality operator will work (and not crash) the same after decoding from json.

Check List

Are you changing anything with the public API? - NO
Are your changes backwards compatible with previous SDK Versions? - YES
Have you tested this change on real device? - YES
Have you added unit test coverage for your changes? - YES
Have you verified that your changes are compatible with all Android versions the SDK currently supports? - YES

Changelog / Code Overview

remove lateinit property initialBody
move metadata insertion to the requestBody method that is invoked only at send time.

Test Plan

Added a failing test to repro the initial issue: json decoded PushTokenApiRequest doesn't set initialBody and crashes when equality operator is invoked.
Fixed code to pass the repro test conditions, see above

Related Issues/Tickets

#116

ndurell · 2024-01-18T16:21:50Z

...analytics/src/test/java/com/klaviyo/analytics/networking/requests/PushTokenApiRequestTest.kt

+        // At this time, decoding from JSON does not preserve equality check for push token requests
+        // But at least it should not crash to compare them
+        val bRequestDecoded = KlaviyoApiRequestDecoder.fromJson(bRequest.toJson())
+        assertNotEquals(aRequest, bRequestDecoded)


What's different between these?

The cause of the bug was that we use a different constructor when decoding this class from JSON. So the first is just create 2 objects, test their equality. The second is encode and decode it, and re-test equality

So previously this test would have reproduced the crash?

Previously, testing equality failed on a decoded json object because the lateinit var was not initialized. With this patch, equality check should not crash, though expected behavior is that it will not be equal anymore, because the payload body has been mutated

Exactly, this was my failing-test repro.

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/requests/PushTokenApiRequest.kt

…it keeps the idea of having up to date metadata at send time.

evan-masseau · 2024-01-18T17:44:53Z

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/requests/PushTokenApiRequest.kt

    }

+    // Update body to include Device metadata whenever the body is retrieved (typically during sending) so the latest data is included
+    override val requestBody: String?


requestBody is invoked at send time to format the body JSON for the network request. but it is not invoked when json encoding. This way, decoding from json will expect the same initial body payload.

evan-masseau · 2024-01-18T17:45:28Z

sdk/core/src/main/java/com/klaviyo/core/config/KlaviyoConfig.kt

@@ -11,7 +11,7 @@ import com.klaviyo.core.Registry
 import com.klaviyo.core.networking.NetworkMonitor

 /**
- * Exception that is thrown when the the Klaviyo API token is missing from the config
+ * Exception that is thrown when the Klaviyo API token is missing from the config


can't help fixing grammar things when i see them

is there a linter that helps with this?

Good Q, grammar problems are highlighted in a shade of red in my IDE color theme, so it always catches my eye.

we should find a way to share some IDE configs

ndurell · 2024-01-18T18:04:56Z

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/requests/PushTokenApiRequest.kt

    override fun equals(other: Any?): Boolean {
        return when (other) {
-            is PushTokenApiRequest -> initialBody == other.initialBody
+            is PushTokenApiRequest -> body.toString() == other.body.toString()


Sorry if I missed it, what's the reasoning behind using toString() vs comparing body to other.body?

I didn't actually try that, let me see. My assumption was separate object instances won't appear equal

Ahh might compare references?

one difficult thing is order:

JSONObject("{'keyA': 'value', 'keyB': 1}").toString() == JSONObject("{'keyB': 1, 'keyA': 'value'}").toString() // false

but yeah, without to string, its a reference check:

JSONObject("{'keyA': 'value'}") == JSONObject("{'keyA': 'value'}") // false JSONObject("{'keyA': 'value'}").toString() == JSONObject("{'keyA': 'value'}").toString() // true

Thanks for the check kenny. Yeah I was surprised the order thing didn't cause my test to fail actually. Unless its just coincidentally passing. I had to add a json compare function to the unit tests for that same reason.

* master: Release 1.4.1 (#118) Replace lateinit property in PushTokenApiRequest (#117) # Conflicts: # README.md # versions.gradle

Quick fix to bug with push token request equality operator

d9fe039

evan-masseau requested a review from kennyklaviyo January 17, 2024 22:17

kennyklaviyo previously approved these changes Jan 17, 2024

View reviewed changes

evan-masseau added the patch-release For issues or code that should be included in a patch release per semantic versioning guidelines label Jan 18, 2024

evan-masseau marked this pull request as ready for review January 18, 2024 16:17

evan-masseau requested a review from a team as a code owner January 18, 2024 16:17

ndurell reviewed Jan 18, 2024

View reviewed changes

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/requests/PushTokenApiRequest.kt Outdated Show resolved Hide resolved

ndurell reviewed Jan 18, 2024

View reviewed changes

sdk/analytics/src/main/java/com/klaviyo/analytics/networking/requests/PushTokenApiRequest.kt Outdated Show resolved Hide resolved

Better fix that allows us to preserve equality operator, and besides …

b7c7944

…it keeps the idea of having up to date metadata at send time.

evan-masseau dismissed kennyklaviyo’s stale review via b7c7944 January 18, 2024 17:43

evan-masseau commented Jan 18, 2024

View reviewed changes

evan-masseau requested review from ndurell and kennyklaviyo January 18, 2024 17:58

ndurell reviewed Jan 18, 2024

View reviewed changes

kennyklaviyo approved these changes Jan 18, 2024

View reviewed changes

ndurell approved these changes Jan 18, 2024

View reviewed changes

evan-masseau merged commit 8c6298d into master Jan 18, 2024
2 checks passed

evan-masseau deleted the ecm/pushtoken-bug branch January 18, 2024 18:51

evan-masseau pushed a commit that referenced this pull request Jan 18, 2024

Merge branch 'master' into rel/2.0.0

528ae78

* master: Release 1.4.1 (#118) Replace lateinit property in PushTokenApiRequest (#117) # Conflicts: # README.md # versions.gradle

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quick fix to bug with push token request equality operator #117

Quick fix to bug with push token request equality operator #117

evan-masseau commented Jan 17, 2024 •

edited

Loading

ndurell Jan 18, 2024

evan-masseau Jan 18, 2024

ndurell Jan 18, 2024

evan-masseau Jan 18, 2024

evan-masseau Jan 18, 2024

evan-masseau Jan 18, 2024

ndurell Jan 18, 2024

evan-masseau Jan 18, 2024

kennyklaviyo Jan 18, 2024

evan-masseau Jan 18, 2024

kennyklaviyo Jan 18, 2024

ndurell Jan 18, 2024 •

edited

Loading

evan-masseau Jan 18, 2024

ndurell Jan 18, 2024

kennyklaviyo Jan 18, 2024 •

edited

Loading

kennyklaviyo Jan 18, 2024

evan-masseau Jan 18, 2024

Quick fix to bug with push token request equality operator #117

Quick fix to bug with push token request equality operator #117

Conversation

evan-masseau commented Jan 17, 2024 • edited Loading

Description

Check List

Changelog / Code Overview

Test Plan

Related Issues/Tickets

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndurell Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyklaviyo Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-masseau commented Jan 17, 2024 •

edited

Loading

ndurell Jan 18, 2024 •

edited

Loading

kennyklaviyo Jan 18, 2024 •

edited

Loading