Add updated params to the Realtime listener callback. #4339
Conversation
Coverage Report 1Affected Products
Test Logs |
|
The public api surface has changed for the subproject firebase-config: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Size Report 1Affected Products
Test Logs |
|
The public api surface has changed for the subproject firebase-config: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
|
The public api surface has changed for the subproject firebase-config: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Account for no params activated when realtime message is received.
Allows the executor to be injected for testing. This came up testing Add updated params to the Realtime listener callback. #4339. Prepares Realtime to integrate with Firebase Executors.
| "Failed to auto-fetch config update.", fetchTask.getException())); | ||
| } | ||
|
|
||
| if (!activatedConfigsTask.isSuccessful()) { |
There was a problem hiding this comment.
If the fetch is successful but getting the activated params isn't, do you think we should still execute the listener onUpdate callback but with all of the fetch params? B/c in this case the SDK would have the most up to date configs in it's fetched cache.
There was a problem hiding this comment.
I'd prefer to return an error here rather than catch it since we might obscure a problem and return incomplete results (ex. a deleted param wouldn't show up).
There was a problem hiding this comment.
Yeah returning an error makes sense here but I just wanted to bring up a potential edge case where the fetch is successful but getting the activated configs is not. In this case the new fetched config/template version will be saved on the device but the user's listener wouldn't be notified of it and so it would seem like the app never got the Config Update but it actually did. It would need to wait until the next event that incremented the template version before it gets another invalidation signal. Doesn't need to be resolved now, just wanted to bring up the possibility.
There was a problem hiding this comment.
Just wanted to bring up this possible scenario
There was a problem hiding this comment.
Yeah... that's a good call out. Since we'd still surface the error, it seems right to raise it rather than change the SDK behavior in this case. Devs can still decide if they want to manually fetch/activate when they get an error from realtime.
| return Tasks.forResult(null); | ||
| } | ||
|
|
||
| if (fetchResponse.getFetchedConfigs() == null) { |
There was a problem hiding this comment.
Instead of checking the configs for null could we check the response status for BACKEND_HAS_NO_UPDATES?
There was a problem hiding this comment.
It looked like getFetchedConfigs() might return a value when the state is BACKEND_HAS_NO_UPDATES, since it's constructed with one here:
It also seemed safer to check for null before getting the value later on in this method.
There was a problem hiding this comment.
I don't think there is a scenario anymore where the response returns a null fetchedConfigs, due to the change where we added the template version to the NO_CHANGE response. Plus in the fetchResponseisUpToDate method it checks the fetchedConfig for null and the only way to return true for when the fetchedConfig is null is if the status is BACKEND_HAS_NO_UPDATES. So maybe just remove this check altogether
There was a problem hiding this comment.
Wondered if we could remove this check?
There was a problem hiding this comment.
Good point it likely isn't necessary. I'd keep it for now since getFetchedConfigs() isn't annotated @Nonnull so it could be null and checking for null keeps with the assumptions that the rest of the SDK is written with right now. We might change that separately later.
| @@ -136,6 +139,10 @@ public long getTemplateVersionNumber() { | |||
| return templateVersionNumber; | |||
| } | |||
|
|
|||
| public ConfigContainer getCopy() throws JSONException { | |||
There was a problem hiding this comment.
Can we just directly use the getCopy method on line 94?
There was a problem hiding this comment.
We can, yeah. It seemed a bit better in terms of OO to not use another instance's private variables, but Java lets you and I can't find any strong reasoning against it. I updated this
| return Tasks.forResult(null); | ||
| } | ||
|
|
||
| if (fetchResponse.getFetchedConfigs() == null) { |
There was a problem hiding this comment.
Wondered if we could remove this check?
| "Failed to auto-fetch config update.", fetchTask.getException())); | ||
| } | ||
|
|
||
| if (!activatedConfigsTask.isSuccessful()) { |
There was a problem hiding this comment.
Just wanted to bring up this possible scenario
Allows the executor to be injected for testing. This came up testing Add updated params to the Realtime listener callback. #4339. Prepares Realtime to integrate with Firebase Executors.
Add the et updated config parameter keys as a param in the ConfigUpdateListener callback. Keys are added if their presence, value, or metadata has changed. The callback is only called when there is an update; the set should never be empty.
Add the
Set<String>of updated config parameter keys as a param in the ConfigUpdateListener callback. Keys are added if their presence, value, or metadata has changed. The callback is only called when there is an update; the set should never be empty.Internal: b/258843184