-
Notifications
You must be signed in to change notification settings - Fork 626
Add updated params to the Realtime listener callback. #4339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3963ebe
1df7a80
79f304c
a216341
ad4d97f
ce9e012
460a635
d47625a
ee88e0f
47c4798
01262b4
3b9ed42
eb9c9fa
a6a4a19
d4944f5
ea97203
39cf168
8dd8f91
567ac9e
154da4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
// Copyright 2022 Google LLC | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package com.google.firebase.remoteconfig; | ||
|
||
import androidx.annotation.NonNull; | ||
import com.google.auto.value.AutoValue; | ||
import java.util.Set; | ||
|
||
/** | ||
* Information about the updated config passed to the {@code onUpdate} callback of {@link | ||
* ConfigUpdateListener}. | ||
*/ | ||
@AutoValue | ||
public abstract class ConfigUpdate { | ||
@NonNull | ||
public static ConfigUpdate create(@NonNull Set<String> updatedParams) { | ||
return new AutoValue_ConfigUpdate(updatedParams); | ||
} | ||
|
||
/** | ||
* Parameter keys whose values have been updated from the currently activated values. Includes | ||
* keys that are added, deleted, and whose value, value source, or metadata has changed. | ||
*/ | ||
@NonNull | ||
public abstract Set<String> getUpdatedParams(); | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -21,6 +21,7 @@ | |||
import androidx.annotation.VisibleForTesting; | ||||
import com.google.android.gms.tasks.Task; | ||||
import com.google.android.gms.tasks.Tasks; | ||||
import com.google.firebase.remoteconfig.ConfigUpdate; | ||||
import com.google.firebase.remoteconfig.ConfigUpdateListener; | ||||
import com.google.firebase.remoteconfig.FirebaseRemoteConfigClientException; | ||||
import com.google.firebase.remoteconfig.FirebaseRemoteConfigException; | ||||
|
@@ -30,6 +31,7 @@ | |||
import java.io.InputStream; | ||||
import java.io.InputStreamReader; | ||||
import java.net.HttpURLConnection; | ||||
import java.util.HashSet; | ||||
import java.util.Random; | ||||
import java.util.Set; | ||||
import java.util.concurrent.ScheduledExecutorService; | ||||
|
@@ -49,18 +51,21 @@ public class ConfigAutoFetch { | |||
private final HttpURLConnection httpURLConnection; | ||||
|
||||
private final ConfigFetchHandler configFetchHandler; | ||||
private final ConfigCacheClient activatedCache; | ||||
private final ConfigUpdateListener retryCallback; | ||||
private final ScheduledExecutorService scheduledExecutorService; | ||||
private final Random random; | ||||
|
||||
public ConfigAutoFetch( | ||||
HttpURLConnection httpURLConnection, | ||||
ConfigFetchHandler configFetchHandler, | ||||
ConfigCacheClient activatedCache, | ||||
Set<ConfigUpdateListener> eventListeners, | ||||
ConfigUpdateListener retryCallback, | ||||
ScheduledExecutorService scheduledExecutorService) { | ||||
this.httpURLConnection = httpURLConnection; | ||||
this.configFetchHandler = configFetchHandler; | ||||
this.activatedCache = activatedCache; | ||||
this.eventListeners = eventListeners; | ||||
this.retryCallback = retryCallback; | ||||
this.scheduledExecutorService = scheduledExecutorService; | ||||
|
@@ -73,9 +78,9 @@ private synchronized void propagateErrors(FirebaseRemoteConfigException exceptio | |||
} | ||||
} | ||||
|
||||
private synchronized void executeAllListenerCallbacks() { | ||||
private synchronized void executeAllListenerCallbacks(ConfigUpdate configUpdate) { | ||||
for (ConfigUpdateListener listener : eventListeners) { | ||||
listener.onEvent(); | ||||
listener.onUpdate(configUpdate); | ||||
} | ||||
} | ||||
|
||||
|
@@ -111,7 +116,8 @@ public void listenForNotifications() { | |||
} | ||||
} | ||||
|
||||
retryCallback.onEvent(); | ||||
// TODO: Factor ConfigUpdateListener out of internal retry logic. | ||||
retryCallback.onUpdate(ConfigUpdate.create(new HashSet<>())); | ||||
scheduledExecutorService.shutdownNow(); | ||||
try { | ||||
scheduledExecutorService.awaitTermination(3L, TimeUnit.SECONDS); | ||||
|
@@ -194,35 +200,78 @@ public void run() { | |||
} | ||||
|
||||
@VisibleForTesting | ||||
public synchronized void fetchLatestConfig(int remainingAttempts, long targetVersion) { | ||||
int currentAttempts = remainingAttempts - 1; | ||||
public synchronized Task<Void> fetchLatestConfig(int remainingAttempts, long targetVersion) { | ||||
int remainingAttemptsAfterFetch = remainingAttempts - 1; | ||||
int currentAttemptNumber = MAXIMUM_FETCH_ATTEMPTS - remainingAttemptsAfterFetch; | ||||
|
||||
// fetchAttemptNumber is calculated by subtracting current attempts from the max number of | ||||
// possible attempts. | ||||
Task<ConfigFetchHandler.FetchResponse> fetchTask = | ||||
configFetchHandler.fetchNowWithTypeAndAttemptNumber( | ||||
ConfigFetchHandler.FetchType.REALTIME, MAXIMUM_FETCH_ATTEMPTS - currentAttempts); | ||||
fetchTask.onSuccessTask( | ||||
(fetchResponse) -> { | ||||
long newTemplateVersion = 0; | ||||
if (fetchResponse.getFetchedConfigs() != null) { | ||||
newTemplateVersion = fetchResponse.getFetchedConfigs().getTemplateVersionNumber(); | ||||
} else if (fetchResponse.getStatus() | ||||
== ConfigFetchHandler.FetchResponse.Status.BACKEND_HAS_NO_UPDATES) { | ||||
newTemplateVersion = targetVersion; | ||||
} | ||||
ConfigFetchHandler.FetchType.REALTIME, currentAttemptNumber); | ||||
Task<ConfigContainer> activatedConfigsTask = activatedCache.get(); | ||||
|
||||
if (newTemplateVersion >= targetVersion) { | ||||
executeAllListenerCallbacks(); | ||||
} else { | ||||
Log.d( | ||||
TAG, | ||||
"Fetched template version is the same as SDK's current version." | ||||
+ " Retrying fetch."); | ||||
// Continue fetching until template version number if greater then current. | ||||
autoFetch(currentAttempts, targetVersion); | ||||
} | ||||
return Tasks.forResult(null); | ||||
}); | ||||
return Tasks.whenAllComplete(fetchTask, activatedConfigsTask) | ||||
.continueWithTask( | ||||
scheduledExecutorService, | ||||
(listOfUnusedCompletedTasks) -> { | ||||
if (!fetchTask.isSuccessful()) { | ||||
return Tasks.forException( | ||||
new FirebaseRemoteConfigClientException( | ||||
"Failed to auto-fetch config update.", fetchTask.getException())); | ||||
} | ||||
|
||||
if (!activatedConfigsTask.isSuccessful()) { | ||||
return Tasks.forException( | ||||
new FirebaseRemoteConfigClientException( | ||||
"Failed to get activated config for auto-fetch", | ||||
activatedConfigsTask.getException())); | ||||
} | ||||
|
||||
ConfigFetchHandler.FetchResponse fetchResponse = fetchTask.getResult(); | ||||
ConfigContainer activatedConfigs = activatedConfigsTask.getResult(); | ||||
|
||||
if (!fetchResponseIsUpToDate(fetchResponse, targetVersion)) { | ||||
Log.d( | ||||
TAG, | ||||
"Fetched template version is the same as SDK's current version." | ||||
+ " Retrying fetch."); | ||||
// Continue fetching until template version number is greater then current. | ||||
autoFetch(remainingAttemptsAfterFetch, targetVersion); | ||||
return Tasks.forResult(null); | ||||
} | ||||
|
||||
if (fetchResponse.getFetchedConfigs() == null) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looked like getFetchedConfigs() might return a value when the state is BACKEND_HAS_NO_UPDATES, since it's constructed with one here: Line 650 in 993673e
It also seemed safer to check for null before getting the value later on in this method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondered if we could remove this check? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point it likely isn't necessary. I'd keep it for now since |
||||
Log.d(TAG, "The fetch succeeded, but the backend had no updates."); | ||||
return Tasks.forResult(null); | ||||
} | ||||
|
||||
// Activate hasn't been called yet, so use an empty container for comparison. | ||||
// See ConfigCacheClient#get() for details on the async operation. | ||||
if (activatedConfigs == null) { | ||||
activatedConfigs = ConfigContainer.newBuilder().build(); | ||||
} | ||||
|
||||
Set<String> updatedParams = | ||||
activatedConfigs.getChangedParams(fetchResponse.getFetchedConfigs()); | ||||
if (updatedParams.isEmpty()) { | ||||
Log.d(TAG, "Config was fetched, but no params changed."); | ||||
return Tasks.forResult(null); | ||||
} | ||||
|
||||
ConfigUpdate configUpdate = ConfigUpdate.create(updatedParams); | ||||
executeAllListenerCallbacks(configUpdate); | ||||
return Tasks.forResult(null); | ||||
}); | ||||
} | ||||
|
||||
private static Boolean fetchResponseIsUpToDate( | ||||
ConfigFetchHandler.FetchResponse response, long lastKnownVersion) { | ||||
// If there's a config, make sure its version is >= the last known version. | ||||
if (response.getFetchedConfigs() != null) { | ||||
return response.getFetchedConfigs().getTemplateVersionNumber() >= lastKnownVersion; | ||||
} | ||||
|
||||
// If there isn't a config, return true if the backend had no update. | ||||
// Else, it returned an out of date config. | ||||
return response.getStatus() == ConfigFetchHandler.FetchResponse.Status.BACKEND_HAS_NO_UPDATES; | ||||
} | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to bring up this possible scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.