Skip to content

Commit

Permalink
Update fetch to utilize latest custom signals (#6582)
Browse files Browse the repository at this point in the history 
When we update custom signals using setCustomSignals, the latest custom
signals are not retrieved in the subsequent fetch. Currently, we are
required to reload the app to fetch them.

Link to the bug: [Fetch uses stale custom signal
values](https://b.corp.google.com/issues/381353888)
  • Loading branch information
@tusharkhandelwal8
tusharkhandelwal8 authored Dec 10, 2024
1 parent 17cc491 commit 9fa2ac9
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 35 deletions.
3 changes: 1 addition & 2 deletions firebase-config/src/main/java/com/google/firebase/remoteconfig/RemoteConfigComponent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,7 @@ ConfigFetchHttpClient getFrcBackendApiClient(
apiKey,
namespace,
/* connectTimeoutInSeconds= */ sharedPrefsClient.getFetchTimeoutInSeconds(),
/* readTimeoutInSeconds= */ sharedPrefsClient.getFetchTimeoutInSeconds(),
/* customSignals= */ sharedPrefsClient.getCustomSignals());
/* readTimeoutInSeconds= */ sharedPrefsClient.getFetchTimeoutInSeconds());
}

@VisibleForTesting
Expand Down
3 changes: 2 additions & 1 deletion ...se-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigFetchHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,8 @@ private FetchResponse fetchFromBackend(
frcSharedPrefs.getLastFetchETag(),
customFetchHeaders,
getFirstOpenTime(),
currentTime);
currentTime,
frcSharedPrefs.getCustomSignals());

if (response.getFetchedConfigs() != null) {
// Set template version in metadata to be saved on disk.
Expand Down
17 changes: 10 additions & 7 deletions ...config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigFetchHttpClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ public class ConfigFetchHttpClient {
private final String apiKey;
private final String projectNumber;
private final String namespace;
Map<String, String> customSignalsMap;
private final long connectTimeoutInSeconds;
private final long readTimeoutInSeconds;

Expand All @@ -108,16 +107,14 @@ public ConfigFetchHttpClient(
String apiKey,
String namespace,
long connectTimeoutInSeconds,
long readTimeoutInSeconds,
Map<String, String> customSignalsMap) {
long readTimeoutInSeconds) {
this.context = context;
this.appId = appId;
this.apiKey = apiKey;
this.projectNumber = extractProjectNumberFromAppId(appId);
this.namespace = namespace;
this.connectTimeoutInSeconds = connectTimeoutInSeconds;
this.readTimeoutInSeconds = readTimeoutInSeconds;
this.customSignalsMap = customSignalsMap;
}

/** Used to verify that the timeout is being set correctly. */
Expand Down Expand Up @@ -187,7 +184,8 @@ FetchResponse fetch(
String lastFetchETag,
Map<String, String> customHeaders,
Long firstOpenTime,
Date currentTime)
Date currentTime,
Map<String, String> customSignalsMap)
throws FirebaseRemoteConfigException {
setUpUrlConnection(urlConnection, lastFetchETag, installationAuthToken, customHeaders);

Expand All @@ -196,7 +194,11 @@ FetchResponse fetch(
try {
byte[] requestBody =
createFetchRequestBody(
installationId, installationAuthToken, analyticsUserProperties, firstOpenTime)
installationId,
installationAuthToken,
analyticsUserProperties,
firstOpenTime,
customSignalsMap)
.toString()
.getBytes("utf-8");
setFetchRequestBody(urlConnection, requestBody);
Expand Down Expand Up @@ -307,7 +309,8 @@ private JSONObject createFetchRequestBody(
String installationId,
String installationAuthToken,
Map<String, String> analyticsUserProperties,
Long firstOpenTime)
Long firstOpenTime,
Map<String, String> customSignalsMap)
throws FirebaseRemoteConfigClientException {
Map<String, Object> requestBodyMap = new HashMap<>();

Expand Down
51 changes: 42 additions & 9 deletions ...onfig/src/test/java/com/google/firebase/remoteconfig/internal/ConfigFetchHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ public void fetch_firstFetch_includesInstallationAuthToken() throws Exception {
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any());
/* currentTime= */ any(),
/* customSignals= */ any());
}

@Test
Expand Down Expand Up @@ -401,7 +402,8 @@ public void fetch_gettingFetchCacheFails_doesNotThrowException() throws Exceptio

@Test
public void fetch_fetchBackendCallFails_taskThrowsException() throws Exception {
when(mockBackendFetchApiClient.fetch(any(), any(), any(), any(), any(), any(), any(), any()))
when(mockBackendFetchApiClient.fetch(
any(), any(), any(), any(), any(), any(), any(), any(), any()))
.thenThrow(
new FirebaseRemoteConfigClientException("Fetch failed due to an unexpected error."));

Expand Down Expand Up @@ -766,6 +768,30 @@ public void getInfo_hitsThrottleLimit_throttledStatus() throws Exception {
.isEqualTo(firstFetchedContainer.getFetchTime());
}

@Test
public void fetch_usesLatestCustomSignals() throws Exception {
Map<String, String> customSignals =
ImmutableMap.of(
"subscription", "premium",
"age", "20");
sharedPrefsClient.setCustomSignals(customSignals);
fetchCallToHttpClientUpdatesClockAndReturnsConfig(firstFetchedContainer);
fetchHandler.fetch();

verify(mockBackendFetchApiClient)
.fetch(
any(HttpURLConnection.class),
/* instanceId= */ any(),
/* instanceIdToken= */ any(),
/* analyticsUserProperties= */ any(),
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any(),
/* customSignals= */ eq(sharedPrefsClient.getCustomSignals()));
assertThat(sharedPrefsClient.getCustomSignals()).isEqualTo(customSignals);
}

private ConfigFetchHandler getNewFetchHandler(AnalyticsConnector analyticsConnector) {
ConfigFetchHandler fetchHandler =
spy(
Expand Down Expand Up @@ -811,7 +837,8 @@ private void setBackendResponseConfigsTo(ConfigContainer container) throws Excep
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any());
/* currentTime= */ any(),
/* customSignals= */ any());
}

private void setBackendResponseToNoChange(Date date) throws Exception {
Expand All @@ -823,7 +850,8 @@ private void setBackendResponseToNoChange(Date date) throws Exception {
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any()))
/* currentTime= */ any(),
/* customSignals= */ any()))
.thenReturn(FetchResponse.forBackendHasNoUpdates(date, firstFetchedContainer));
}

Expand All @@ -838,7 +866,8 @@ private void fetchCallToBackendThrowsException(int httpErrorCode) throws Excepti
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any());
/* currentTime= */ any(),
/* customSignals= */ any());
}

/**
Expand Down Expand Up @@ -919,7 +948,8 @@ private void verifyBackendIsCalled() throws Exception {
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any());
/* currentTime= */ any(),
/* customSignals= */ any());
}

private void verifyBackendIsCalled(Map<String, String> userProperties, Long firstOpenTime)
Expand All @@ -933,7 +963,8 @@ private void verifyBackendIsCalled(Map<String, String> userProperties, Long firs
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ eq(firstOpenTime),
/* currentTime= */ any());
/* currentTime= */ any(),
/* customSignals= */ any());
}

private void verifyBackendIsNeverCalled() throws Exception {
Expand All @@ -946,7 +977,8 @@ private void verifyBackendIsNeverCalled() throws Exception {
/* lastFetchETag= */ any(),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any());
/* currentTime= */ any(),
/* customSignals= */ any());
}

private void verifyETags(@Nullable String requestETag, String responseETag) throws Exception {
Expand All @@ -959,7 +991,8 @@ private void verifyETags(@Nullable String requestETag, String responseETag) thro
/* lastFetchETag= */ eq(requestETag),
/* customHeaders= */ any(),
/* firstOpenTime= */ any(),
/* currentTime= */ any());
/* currentTime= */ any(),
/* customSignals= */ any());
assertThat(sharedPrefsClient.getLastFetchETag()).isEqualTo(responseETag);
}

Expand Down
28 changes: 12 additions & 16 deletions ...ig/src/test/java/com/google/firebase/remoteconfig/internal/ConfigFetchHttpClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import static com.google.firebase.remoteconfig.RemoteConfigConstants.RequestFieldKey.APP_ID;
import static com.google.firebase.remoteconfig.RemoteConfigConstants.RequestFieldKey.APP_VERSION;
import static com.google.firebase.remoteconfig.RemoteConfigConstants.RequestFieldKey.COUNTRY_CODE;
import static com.google.firebase.remoteconfig.RemoteConfigConstants.RequestFieldKey.CUSTOM_SIGNALS;
import static com.google.firebase.remoteconfig.RemoteConfigConstants.RequestFieldKey.FIRST_OPEN_TIME;
import static com.google.firebase.remoteconfig.RemoteConfigConstants.RequestFieldKey.INSTANCE_ID;
import static com.google.firebase.remoteconfig.RemoteConfigConstants.RequestFieldKey.INSTANCE_ID_TOKEN;
Expand Down Expand Up @@ -86,10 +85,6 @@ public class ConfigFetchHttpClientTest {
"etag-" + PROJECT_NUMBER + "-" + DEFAULT_NAMESPACE + "-fetch-%d";
private static final String FIRST_ETAG = String.format(ETAG_FORMAT, 1);
private static final String SECOND_ETAG = String.format(ETAG_FORMAT, 2);
private static final Map<String, String> SAMPLE_CUSTOM_SIGNALS =
ImmutableMap.of(
"subscription", "premium",
"age", "20");

private Context context;
private ConfigFetchHttpClient configFetchHttpClient;
Expand All @@ -110,8 +105,7 @@ public void setUp() throws Exception {
API_KEY,
DEFAULT_NAMESPACE,
/* connectTimeoutInSeconds= */ 10L,
/* readTimeoutInSeconds= */ 10L,
/* customSignals= */ SAMPLE_CUSTOM_SIGNALS);
/* readTimeoutInSeconds= */ 10L);

hasChangeResponseBody =
new JSONObject()
Expand Down Expand Up @@ -244,8 +238,6 @@ public void fetch_setsAllElementsOfRequestBody_sendsRequestBodyToServer() throws
assertThat(requestBody.get(FIRST_OPEN_TIME)).isEqualTo(firstOpenTimeIsoString);
assertThat(requestBody.getJSONObject(ANALYTICS_USER_PROPERTIES).toString())
.isEqualTo(new JSONObject(customUserProperties).toString());
assertThat(requestBody.getJSONObject(CUSTOM_SIGNALS).toString())
.isEqualTo(new JSONObject(SAMPLE_CUSTOM_SIGNALS).toString());
}

@Test
Expand Down Expand Up @@ -324,8 +316,7 @@ public void fetch_setsTimeouts_urlConnectionHasTimeouts() throws Exception {
API_KEY,
DEFAULT_NAMESPACE,
/* connectTimeoutInSeconds= */ 15L,
/* readTimeoutInSeconds= */ 20L,
/* customSignals= */ SAMPLE_CUSTOM_SIGNALS);
/* readTimeoutInSeconds= */ 20L);
setServerResponseTo(noChangeResponseBody, SECOND_ETAG);

fetch(FIRST_ETAG);
Expand Down Expand Up @@ -353,7 +344,8 @@ private FetchResponse fetch(String eTag) throws Exception {
eTag,
/* customHeaders= */ ImmutableMap.of(),
/* firstOpenTime= */ null,
/* currentTime= */ new Date(mockClock.currentTimeMillis()));
/* currentTime= */ new Date(mockClock.currentTimeMillis()),
/* customSignals= */ ImmutableMap.of());
}

private FetchResponse fetch(String eTag, Map<String, String> userProperties, Long firstOpenTime)
Expand All @@ -366,7 +358,8 @@ private FetchResponse fetch(String eTag, Map<String, String> userProperties, Lon
eTag,
/* customHeaders= */ ImmutableMap.of(),
firstOpenTime,
new Date(mockClock.currentTimeMillis()));
new Date(mockClock.currentTimeMillis()),
/* customSignals= */ ImmutableMap.of());
}

private FetchResponse fetch(String eTag, Map<String, String> customHeaders) throws Exception {
Expand All @@ -378,7 +371,8 @@ private FetchResponse fetch(String eTag, Map<String, String> customHeaders) thro
eTag,
customHeaders,
/* firstOpenTime= */ null,
new Date(mockClock.currentTimeMillis()));
new Date(mockClock.currentTimeMillis()),
/* customSignals= */ ImmutableMap.of());
}

private FetchResponse fetchWithoutInstallationId() throws Exception {
Expand All @@ -390,7 +384,8 @@ private FetchResponse fetchWithoutInstallationId() throws Exception {
/* lastFetchETag= */ "bogus-etag",
/* customHeaders= */ ImmutableMap.of(),
/* firstOpenTime= */ null,
new Date(mockClock.currentTimeMillis()));
new Date(mockClock.currentTimeMillis()),
/* customSignals= */ ImmutableMap.of());
}

private FetchResponse fetchWithoutInstallationAuthToken() throws Exception {
Expand All @@ -402,7 +397,8 @@ private FetchResponse fetchWithoutInstallationAuthToken() throws Exception {
/* lastFetchETag= */ "bogus-etag",
/* customHeaders= */ ImmutableMap.of(),
/* firstOpenTime= */ null,
new Date(mockClock.currentTimeMillis()));
new Date(mockClock.currentTimeMillis()),
/* customSignals= */ ImmutableMap.of());
}

private void setServerResponseTo(JSONObject requestBody, String eTag) {
Expand Down

0 comments on commit 9fa2ac9

Please sign in to comment.