[Identity] Simplify readyForEvent by removing unnecessary configuration shared state checks #789
Conversation
…on shared state checks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #789 +/- ##
============================================
- Coverage 75.67% 75.65% -0.01%
+ Complexity 2348 2345 -3
============================================
Files 219 219
Lines 10902 10897 -5
Branches 1414 1411 -3
============================================
- Hits 8249 8244 -5
+ Misses 2026 2024 -2
- Partials 627 629 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } else { | ||
| return true; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
The check for a valid Configuration is still needed for the block above (lines 213-232) for appendToUrl and getUrlVariables. The logic for that block relied on the Configuration state check if the block evaluated to true and did not return false due to the Analytics state check. If the current Configuration state is not set then appendToUrl/getUrlVariables won't necessarily fail, but they may not include the org ID in their responses.
There was a problem hiding this comment.
I can add an explicit configuration check but the behaviour now is same as before. The forceSyncIdentifiers call only succeeds if the SDK has a valid configuration that includes an org-id. Right now, we only check the shared state in the context of the specific event, and the call is assumed to always pass if forceSyncIdentifiers succeeds. In this particular case, we had a valid (but empty) configuration, which will allow the call to proceed, but the org-id was still missing.
I think we should reconsider the broader approach of how we validate states in readyForEvent.
There was a problem hiding this comment.
Okay, I see your point. Adding this check back for configuration is just checking the configuration wasn't cleared which isn't really possible, so it's not necessary. Sorry for the confusion, you can roll back your last commit if you want.
| } else { | ||
| return true; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Okay, I see your point. Adding this check back for configuration is just checking the configuration wasn't cleared which isn't really possible, so it's not necessary. Sorry for the confusion, you can roll back your last commit if you want.
3 participants
Currently
readyForEventin Identity extension checks whether non empty configuration shared state is present for each incoming event. This usually works fine because Core sets a valid cached Configuration when the app starts. However, we found an edge case: if the app launches for the very first time with no internet connection and no pre-bundled configuration, the SDK might start with an empty configuration.This leads to a scenario where Identity hangs, even after the network is restored and a valid configuration is later downloaded. This is because the
readyForEventcontinues to validate against the initially empty Configuration state as newer shared states having higher versions.One potential solution would be to simply verify whether the shared state is set, without checking its contents. However, this is also unnecessary, as
readyForEventis invoked for every event (including unrelated ones), resulting in unnecessary reading of configuration shared state.To resolve this, the code has been updated to eliminate the shared state checks for events unrelated to the Identity extension.
This was injected in iOS and later ported to Android.