launchdarkly / android-client-sdk

LaunchDarkly Client-side SDK for Android
Other
45 stars 23 forks source link

Each time the application is restarted, a new FlagStore is created for the same LDUser #149

Closed vorobyov-denis closed 1 year ago

vorobyov-denis commented 2 years ago

Not sure if this is a bug or a feature request

Is this a support request? No

Describe the bug Each time LDClient.init() is called, the current user is set via DefaultUserManager.setCurrentUser(user), inside which FlagStoreManager is switched to the specified LDUser, but to create a userKey, DefaultUserManager.sharedPrefs(user) is used, which internally converts LDUser to a JSON string using LDConfig.GSON and then converts it to a hash using HASHER. The problem is that GSON is not guaranteed to preserve the field order when converting an object to a JSON string. It turns out that the hash calculated from the JSON string obtained from the same object may differ, which is what happens

To reproduce Restart the app with the same LDUser multiple times. To improve a chance to reproduce, add an LDValueArray with multiple values to the LDUser custom field

Expected behavior I expect that the same userKey will be calculated for the same LDUser, and therefore the same FlagStore will be used by LDClient

SDK version Android LD SDK 3.1.2

Language version, developer tools Tested on API 23 and 30

Additional context Possible solutions:

louis-launchdarkly commented 2 years ago

Hello @vorobyov-denis, thank you for reaching out.

Just to check, were there any changes to the LDUser object when you test your change out? If there is none, this is something weird and we will need to dig deeper into this.

denis-bezrukov commented 2 years ago

@louis-launchdarkly it should be enough to setup privateFirstName/privateLastName/privateEmail in our case the diff was in that part:

"privateAttributeNames":["lastName","email","firstName"]}

vs

"privateAttributeNames":["lastName","firstName","email"]}

Along with gson issue it also makes sense to change HashSet to LinkedHashSet here , I believe not every HashSet/HashMap impl promises stable ordering

louis-launchdarkly commented 2 years ago

Hello @denis-bezrukov, thank you for explaining the detail of your situation, that is enough information for us to debug the issue. We will look into this to see is this a bug or an intended behavior and give you an update afterward.

Filed Internally as 142144.

vorobyov-denis commented 2 years ago

@louis-launchdarkly are there any updates on this? Can we somehow see the status of an internal ticket? Thanks

louis-launchdarkly commented 2 years ago

@vorobyov-denis, Sorry for the wait. You are correct that this is an issue. And because of the combinatorics nature of this, the more privateAttributeNames you have, it is more likely this will be a problem and increasing MaxCachedUser as a temporary workaround will be less effective. (But if you don't have too many privateAttributeNames, I recommend giving that a try for now to at least reduce the number of full refreshes).

https://github.com/launchdarkly/android-client-sdk/issues/150 is similarly related that because the Hashed key changed, the SDK is not using the cache. I am discussing with the squad to see do decide what solution we should go with. Unfortunately, I don't have an ETA for this yet, but I can give another update when the squad decided the approach to this.

vorobyov-denis commented 2 years ago

The issue is reproduced with one user, so I think that increasing MaxCachedUser will not affect it in any way. Thank you for keeping up to date! We will wait for updates on this issue.

gpeal commented 2 years ago

@louis-launchdarkly was this issue incorrectly referenced in the 3.1.6 release notes? This is mentioned next to an "event retry" bullet.

louis-launchdarkly commented 1 year ago

Hello @gpeal, sorry for catching this pretty late - I wonder is that our private/public mirror of the repo causing the reference in GitHub not to work correctly.

For anyone who is reading this issue - this was fixed in 4.0.0 as we using Contexts instead of Users and stop hashing the whole JSON, sorry to not make that clear.