Exclude transient query parameter from identities request when false

[caselett]

Currently, get identities request contains transient query parameter only when set to true.

Previously, this bug would result in an unexpected flag state for the user identity.

• Implemented according to iOS library implementation.
• Covered with test.

[matthewelwell]

Hi @SergeiRR , thanks for the contribution. I have a feeling, however, that this is a duplicate of a PR that was merged earlier this week here?

[caselett]

Hi @matthewelwell, actually, this is another side of the issue.
PR you mentioned covered responce part, but my PR covers request part.

[matthewelwell]

Hi @matthewelwell, actually, this is another side of the issue. PR you mentioned covered responce part, but my PR covers request part.

I'm not sure I understand what you mean by 'response' part, but on inspection, I think that the PR I shared only covers the POST by modifying the request / json body, not the GET request.

@rolodato are you able to contribute to the conversation here?

[caselett]

@matthewelwell Anyway, we found this issue using 1.7.3 (the latest) version.
When transient exists (even it false) as query parameter server responds with default value.
This PR fixes the issue.

[matthewelwell]

Ok, thanks @SergeiRR - is there a way to catch this further upstream though? It seems like the queryInterceptor is having to correct something that's happened earlier in the URL building - couldn't we just prevent the query parameter being added earlier in the codepath?

[caselett]

@matthewelwell yes, we can use null value for transient query parameter, but we will face necessity to add check like:

if (transient) transient else null

to every function call (currently there are 2 calls that use transient parameter).

But this may cause misunderstandings in the future.

[caselett]

@matthewelwell Hi, any activity on this? This issue still affects us.

[rolodato]

I understand the correct fix would be to remove the call to GET /identities altogether, which does not have this problem, based on @matthewelwell's comment here: Flagsmith/flagsmith#5260 (comment).

However, we can't simply switch from GET to POST calls because of how the client implements caching. We cache API responses by adding a Cache-Control headers to responses, and letting OkHttp handle the caching:

flagsmith-kotlin-android-client/FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt

Line 115 in 0a64715

.let { if (cacheConfig.enableCache) it.addNetworkInterceptor(cacheControlInterceptor()) else it }

The problem is that OkHttp will not cache POST responses: https://github.com/square/okhttp/blob/2f78681a1f5ef5d9cc4f7bce48b9975f79c06978/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt#L231-L235

This means that we would need to implement our own caching separate from OkHttp.

As an interim solution, I changed this PR's approach to:

• Not include the transient parameter in requests by changing the signature of getIdentityFlagsAndTraits to accept an optional Boolean that defaults to null, which should be backwards compatible. This works because a null query string parameter is not serialised, whereas a false value gets serialised as transient=false
• Explicitly check that the parameter is missing, instead of expecting a generic AssertionError

[matthewelwell]

@rolodato I'm not sure that this is actually any improvement on @SergeiRR 's implementation since we still have the interceptor logic included which was what I was hoping that we could remove.

I do see, however, that we should include it, even with your change, in case any user explicitly sets transient: false.

I'm unsure then what benefit we get from including your change?

[rolodato]

It would have been an improvement, if I had remembered to actually remove the interceptor :) It's fixed now.

[matthewelwell]

@rolodato can you merge this into a native branch, other than main, so we can verify the test / build process and then let's get it merged and released.