Skip to content

Check for missing metadata when constructing ConfigContainer. #2217

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.

Sign up for GitHub

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 1 commit into from
Dec 2, 2020
+41 −1
Merged

Check for missing metadata when constructing ConfigContainer. #2217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion firebase-config/src/main/java/com/google/firebase/remoteconfig/internal/ConfigContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,18 @@ private ConfigContainer(
* <p>The {@code containerJson} must not be modified.
*/
static ConfigContainer copyOf(JSONObject containerJson) throws JSONException {
// Personalization metadata may not have been written yet.
JSONObject personalizationMetadataJSON =
containerJson.optJSONObject(PERSONALIZATION_METADATA_KEY);
Copy link

@erikeldridge erikeldridge Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking out loud: getJSONObject throws if the given key isn't found, which would result in the new ConfigContainer not being created, as noted in #2186 (comment). Metadata depends on integrations with RC and may not be defined, ie it's not necessarily an error case, and may be a common case (eg for a project that never uses metadata). Using optJSONObject instead facilitates checking for null and assigning a default value rather than throwing.

if (personalizationMetadataJSON == null) {
personalizationMetadataJSON = new JSONObject();
}

return new ConfigContainer(
containerJson.getJSONObject(CONFIGS_KEY),
new Date(containerJson.getLong(FETCH_TIME_KEY)),
containerJson.getJSONArray(ABT_EXPERIMENTS_KEY),
containerJson.getJSONObject(PERSONALIZATION_METADATA_KEY));
personalizationMetadataJSON);
}

/**
Expand Down
33 changes: 33 additions & 0 deletions ...nfig/src/test/java/com/google/firebase/remoteconfig/internal/ConfigStorageClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@
import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
import java.util.Date;
import org.json.JSONObject;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -82,6 +85,36 @@ public void read_validContainer_returnsContainer() throws Exception {
assertThat(container).isEqualTo(configContainer);
}

@Test
public void read_validContainerWithPersonalization_returnsContainer() throws Exception {
ConfigContainer configWithPersonalization =
ConfigContainer.newBuilder(configContainer)
.withPersonalizationMetadata(
new JSONObject(ImmutableMap.of(Personalization.ARM_KEY, "arm_value")))
.build();
storageClient.write(configWithPersonalization);
Preconditions.checkArgument(getFileAsString().equals(configWithPersonalization.toString()));

ConfigContainer container = storageClient.read();
assertThat(container).isEqualTo(configWithPersonalization);
}

@Test
public void read_validContainerWithoutPersonalization_returnsContainer() throws Exception {
// Configs written by SDK versions <20.0.1 do not contain personalization metadata.
// Since the serialized configContainer contains personalization metadata, we manually remove it
// and write the config to disk directly to test.
JSONObject configJSON = new JSONObject(configContainer.toString());
configJSON.remove(ConfigContainer.PERSONALIZATION_METADATA_KEY);

try (FileOutputStream outputStream = context.openFileOutput(FILE_NAME, Context.MODE_PRIVATE)) {
outputStream.write(configJSON.toString().getBytes(StandardCharsets.UTF_8));
}

ConfigContainer container = storageClient.read();
assertThat(container).isEqualTo(configContainer);
}

@Test
public void read_emptyFile_returnsNull() throws Exception {
ConfigContainer container = storageClient.read();
Expand Down