[Fix] external_id is skipped and updates stop if something updates the User (such as addTag) shortly before login is called #2046
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
All operations must have at least a create or modify key, this is required so operations are not grouped together when they should not be. This fixes a bug where calling addTag() back-to-back with login() would cause the login to be dropped. This is because SetTagOperation.modifyComparisonKey was "" which matched LoginUserOperation.modifyComparisonKey as it was also "" and both these operations we returned by OperationRepo.getGroupableOperations. Both operations were given to UpdateUserOperationExecutor which it would skip the LoginUserOperation silently. This mistake originated from f1d204e which wasn't in the 5.0.0-beta4 release, but was introduced in 5.0.0. The intent of PR #1794 is still preserved in this commit, we just simply fixed the bug noted above.
jkasten2
changed the title
To ensure operations are not silently skipped each executor now checks if it can process all the operations. This is important since bugs can go unnoticed for a while and without this it's hard to debug too.
jkasten2
force-pushed
the
fix/operation-repo-missing-grouping-modify-key
branch
from
d0d2b84
to
20075f7
Compare
This test was missed when support for this was droped in commit f1d204e
This test had an extra update subscription operation. 1. This test shouldn't contain an update since it is testing a delete 2. An update and a delete could never be executed together as it's keys are different and therefore would never be grouped together.
We want to know if this happens as soon as possible, as it will almost always result in operations being combined when they should not.
jkasten2
changed the title
jkasten2
requested review from
nan-li,
emawby,
shepherd-l,
jennantilla and
brismithers
jkasten2
changed the title
jkasten2
changed the title
jkasten2
changed the title
Commit 7fe2010 fixed a bug with login operations being drop in some scenarios, however it doesn't fix those already in the bad state. This comment get us out of the bad state by generating a replacement LoginUserOperation. See the comments in the code for more details. This comment was manually tested, ensuring it gets us out of the bad state, and updates works afterwords.
nan-li
approved these changes
Apr 9, 2024
There was a problem hiding this comment.
I think the commit add operation checks to all executors points out some things we may want to address in a future PR, but this PR to address the issues it is fixing, I approve after testing on my end.
Merged
This was referenced Apr 10, 2024
This was referenced Apr 10, 2024
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Fixes login silently failing (and operations after gets stuck) when login is called within 5 seconds of a user property update (such as addTag).
Details
This fixes a bug where calling addTag() then login() would cause the login to be dropped.
The bug happen on all operations didn't have a create or modify key, this is required so operations are not grouped together when they should not be. Since we fixed batching in 5.1.7 to always wait 5 seconds this issue will be most common on 5.1.7, however all versions back to 5.0.0 are effected.
Motivation
It's critical that login works consistently.
Scope
Affects operation repo grouping and error handling of executors.
How this issue happened
This is because
SetTagOperation.modifyComparisonKeywas always""which matchedLoginUserOperation.modifyComparisonKeyas it was also""and both these operations were returned byOperationRepo.getGroupableOperations. Both operations were given toUpdateUserOperationExecutorwhich it would skip theLoginUserOperationsilently.Recovering from stalled state
Since the bug was dropping the login operation, it needs to be replaced to get those already in the bad state out. Commit bc91777 does this.
If the commit above was not done then even if the developer called OneSignal.login() again with the same
externalIdit would not correct the stalled User. Only calling logout() or login() with differentexternalIdwould have un-stalled theOperationRepo. And then only after logging back to that stalled User would it have recover all the unsent operations they may exist.Related PRs
This mistake originated from PR #1794, commit f1d204e, which was shipped in the 5.0.0 (wasn't in 5.0.0-beta4 yet). I preserved the intent of PR #1794, and fixed the mistake in commit 7fe2010.
Related Issues
Testing
Unit testing
Corrected some existing unit tests.
Manual testing
Android 14 Emulator.
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is