Skip to content

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

Merged
merged 20 commits into from
Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion firebase-config/api.txt
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
// Signature format: 2.0
package com.google.firebase.remoteconfig {

@com.google.auto.value.AutoValue public abstract class ConfigUpdate {
ctor public ConfigUpdate();
method @NonNull public static com.google.firebase.remoteconfig.ConfigUpdate create(@NonNull java.util.Set<java.lang.String>);
method @NonNull public abstract java.util.Set<java.lang.String> getUpdatedParams();
}

public interface ConfigUpdateListener {
method public void onError(@NonNull com.google.firebase.remoteconfig.FirebaseRemoteConfigException);
method public void onEvent();
method public void onUpdate(@NonNull com.google.firebase.remoteconfig.ConfigUpdate);
}

public interface ConfigUpdateListenerRegistration {
Expand Down
3 changes: 3 additions & 0 deletions firebase-config/firebase-config.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@ dependencies {
implementation 'com.google.firebase:firebase-measurement-connector:18.0.0'
implementation 'com.google.android.gms:play-services-tasks:18.0.1'

compileOnly 'com.google.auto.value:auto-value-annotations:1.6.6'
compileOnly 'com.google.code.findbugs:jsr305:3.0.2'

annotationProcessor 'com.google.auto.value:auto-value:1.6.6'

javadocClasspath 'com.google.auto.value:auto-value-annotations:1.6.6'

testImplementation 'org.mockito:mockito-core:2.25.0'
Expand Down
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
Expand Up @@ -14,6 +14,7 @@

package com.google.firebase.remoteconfig;

import androidx.annotation.NonNull;
import javax.annotation.Nonnull;

/**
Expand All @@ -23,13 +24,16 @@
*/
public interface ConfigUpdateListener {
/**
* Callback for when a new config has been automatically fetched from the backend. Can be used to
* activate the new config.
* Callback for when a new config has been automatically fetched from the backend and has changed
* from the activated config.
*
* @param configUpdate A {@link ConfigUpdate} with information about the updated config, including
* the set of updated parameters.
*/
void onEvent();
void onUpdate(@NonNull ConfigUpdate configUpdate);

/**
* Call back for when an error occurs during Realtime.
* Callback for when an error occurs while listening for or fetching a config update.
*
* @param error
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,11 @@ protected RemoteConfigComponent(
boolean loadGetDefault) {
this.context = context;
this.executorService = executorService;
this.scheduledExecutorService = scheduledExecutorService;
this.firebaseApp = firebaseApp;
this.firebaseInstallations = firebaseInstallations;
this.firebaseAbt = firebaseAbt;
this.analyticsConnector = analyticsConnector;
this.scheduledExecutorService = scheduledExecutorService;

this.appId = firebaseApp.getOptions().getApplicationId();
GlobalBackgroundListener.ensureBackgroundListenerIsRegistered(context);
Expand Down Expand Up @@ -216,7 +216,13 @@ synchronized FirebaseRemoteConfig get(
fetchHandler,
getHandler,
metadataClient,
getRealtime(firebaseApp, firebaseInstallations, fetchHandler, context, namespace));
getRealtime(
firebaseApp,
firebaseInstallations,
fetchHandler,
activatedClient,
context,
namespace));
in.startLoadingConfigsFromDisk();
frcNamespaceInstances.put(namespace, in);
frcNamespaceInstancesStatic.put(namespace, in);
Expand Down Expand Up @@ -270,15 +276,16 @@ synchronized ConfigRealtimeHandler getRealtime(
FirebaseApp firebaseApp,
FirebaseInstallationsApi firebaseInstallations,
ConfigFetchHandler configFetchHandler,
ConfigCacheClient activatedCacheClient,
Context context,
String namespace) {
return new ConfigRealtimeHandler(
firebaseApp,
firebaseInstallations,
configFetchHandler,
activatedCacheClient,
context,
namespace,
executorService,
scheduledExecutorService);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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()) {
Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

.

It also seemed safer to check for null before getting the value later on in this method.

Copy link
Contributor

@qdpham13 qdpham13 Dec 7, 2022

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondered if we could remove this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.

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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
package com.google.firebase.remoteconfig.internal;

import java.util.Date;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import org.json.JSONArray;
import org.json.JSONException;
import org.json.JSONObject;
Expand Down Expand Up @@ -154,6 +157,73 @@ public boolean equals(Object o) {
return containerJson.toString().equals(that.toString());
}

/**
* @param other The other {@link ConfigContainer} against which to compute the diff
* @return The set of config keys that have changed between the this config and {@code other}
* @throws JSONException
*/
public Set<String> getChangedParams(ConfigContainer other) throws JSONException {
// Make a copy of the other config before modifying it
JSONObject otherConfig = ConfigContainer.copyOf(other.containerJson).getConfigs();

// Experiments aren't associated with params, so we can just compare arrays once
Boolean experimentsChanged = !this.getAbtExperiments().equals(other.getAbtExperiments());

Set<String> changed = new HashSet<>();
Iterator<String> keys = this.getConfigs().keys();
while (keys.hasNext()) {
String key = keys.next();

// If the ABT Experiments have changed, add all keys since we don't know which keys the ABT
// experiments apply to
if (experimentsChanged) {
changed.add(key);
continue;
}

// If the other config doesn't have the key
if (!other.getConfigs().has(key)) {
changed.add(key);
continue;
}

// If the other config has a different value for the key
if (!this.getConfigs().get(key).equals(other.getConfigs().get(key))) {
changed.add(key);
continue;
}

// If only one of the configs has PersonalizationMetadata for the key
if (this.getPersonalizationMetadata().has(key) && !other.getPersonalizationMetadata().has(key)
|| !this.getPersonalizationMetadata().has(key)
&& other.getPersonalizationMetadata().has(key)) {
changed.add(key);
continue;
}

// If the both configs have PersonalizationMetadata for the key, but the metadata has changed
if (this.getPersonalizationMetadata().has(key)
&& other.getPersonalizationMetadata().has(key)
&& !this.getPersonalizationMetadata()
.get(key)
.equals(other.getPersonalizationMetadata().get(key))) {
changed.add(key);
continue;
}

// Since the key is the same in both configs, remove it from otherConfig
otherConfig.remove(key);
}

// Add all the keys from other that are different
Iterator<String> remainingOtherKeys = otherConfig.keys();
while (remainingOtherKeys.hasNext()) {
changed.add(remainingOtherKeys.next());
}

return changed;
}

@Override
public int hashCode() {
return containerJson.hashCode();
Expand Down
Loading