Skip to content
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

Save cache file as received rather than serializing it again #51

Merged
merged 3 commits into from
May 24, 2024
Merged

Save cache file as received rather than serializing it again #51

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2629071
write cache file as we get it
aarsilv May 23, 2024
1d8f818
update comment
aarsilv May 23, 2024
b9b073e
bump version number
aarsilv May 24, 2024
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: 4 additions & 5 deletions eppo/src/androidTest/java/cloud/eppo/android/EppoClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.StringReader;
import java.lang.reflect.Type;
import java.lang.reflect.Field;
import java.util.List;
Expand All @@ -51,15 +50,15 @@
import cloud.eppo.android.dto.FlagConfig;
import cloud.eppo.android.dto.RandomizationConfigResponse;
import cloud.eppo.android.dto.SubjectAttributes;
import cloud.eppo.android.dto.deserializers.EppoValueAdapter;
import cloud.eppo.android.dto.adapters.EppoValueAdapter;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I understand more about Adapters, I renamed this package back to adapters (how it was earlier)


public class EppoClientTest {
private static final String TAG = logTag(EppoClient.class);
private static final String DUMMY_API_KEY = "mock-api-key";
private static final String DUMMY_OTHER_API_KEY = "another-mock-api-key";
private static final String TEST_HOST = "https://us-central1-eppo-qa.cloudfunctions.net/serveGitHubRacTestFile";
private static final String INVALID_HOST = "https://thisisabaddomainforthistest.com";
private Gson gson = new GsonBuilder()
private final Gson gson = new GsonBuilder()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧹 some cleanup suggested by Android Studio

.registerTypeAdapter(EppoValue.class, new EppoValueAdapter())
.registerTypeAdapter(AssignmentValueType.class, new AssignmentValueTypeAdapter(AssignmentValueType.STRING))
.create();
Expand Down Expand Up @@ -266,7 +265,7 @@ public void testCachedAssignments() {
// First initialize successfully
initClient(TEST_HOST, true, true, false, DUMMY_API_KEY); // ensure cache is populated

// wait for a bit since cache file is loaded asynchronously
// wait for a bit since cache file is written asynchronously
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really what we are waiting for

waitForPopulatedCache();

// Then reinitialize with a bad host so we know it's using the cached RAC built from the first initialization
Expand Down Expand Up @@ -371,7 +370,7 @@ public void testInvalidConfigJSON() {

doAnswer(invocation -> {
RequestCallback callback = invocation.getArgument(1);
callback.onSuccess(new StringReader("{}"));
callback.onSuccess("{}");
return null; // doAnswer doesn't require a return value
}).when(mockHttpClient).get(anyString(), any(RequestCallback.class));

Expand Down
42 changes: 32 additions & 10 deletions eppo/src/main/java/cloud/eppo/android/ConfigurationRequestor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,34 @@
import com.google.gson.JsonIOException;
import com.google.gson.JsonSyntaxException;

import java.io.Reader;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;

import cloud.eppo.android.dto.FlagConfig;

public class ConfigurationRequestor {
private static final String TAG = logTag(ConfigurationRequestor.class);

private EppoHttpClient client;
private ConfigurationStore configurationStore;
private final EppoHttpClient client;
private final ConfigurationStore configurationStore;

public ConfigurationRequestor(ConfigurationStore configurationStore, EppoHttpClient client) {
this.configurationStore = configurationStore;
this.client = client;
}

public void load(InitializationCallback callback) {
// We have two at-bats to load the configuration: loading from cache and fetching
// The below variables help them keep track of each other's progress
AtomicBoolean cacheLoadInProgress = new AtomicBoolean(true);
AtomicReference<String> fetchErrorMessage = new AtomicReference<>(null);
// We only want to fire the callback off once; so track whether or not we have yet
Comment on lines +27 to +31
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulling back comments and tracking used in the UFC branch as the are more robust than what we have here.

AtomicBoolean callbackCalled = new AtomicBoolean(false);
configurationStore.loadFromCache(new CacheLoadCallback() {
@Override
public void onCacheLoadSuccess() {
cacheLoadInProgress.set(false);
// If cache loaded successfully, fire off success callback if not yet done so by fetching
if (callback != null && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialized from cache");
callback.onCompleted();
Expand All @@ -36,27 +43,39 @@ public void onCacheLoadSuccess() {

@Override
public void onCacheLoadFail() {
// no-op; fall-back to Fetch
cacheLoadInProgress.set(false);
Log.d(TAG, "Did not initialize from cache");
// If cache loading failed, and fetching failed, fire off the failure callback if not yet done so
// Otherwise, if fetching has not failed yet, defer to it for firing off callbacks
Copy link
Member

Choose a reason for hiding this comment

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

What happens is about the same time the fetching exceptional handler checks cacheLoadInProgress, thinks it is in progress and returns? I guess this is covered right - we will eventually either get good data from the cache or empty data from the cache (if its empty). This is a good endpoint.

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 so if cacheLoadInProgress is checked right before it's set to false, then the fetcher won't fire off the callbacks but the cache load completion will because the final check for callback called flag is an atomic check and a set

if (callback != null && fetchErrorMessage.get() != null && callbackCalled.compareAndSet(false, true)) {
Log.e(TAG, "Failed to initialize from fetching or by cache");
callback.onError("Cache and fetch failed "+fetchErrorMessage.get());
}
}
});

Log.d(TAG, "Fetching configuration");
client.get("/api/randomized_assignment/v3/config", new RequestCallback() {
@Override
public void onSuccess(Reader response) {
public void onSuccess(String responseBody) {
try {
Log.d(TAG, "Processing fetch response");
configurationStore.setFlagsFromResponse(response);
configurationStore.setFlagsFromJsonString(responseBody);
Log.d(TAG, "Configuration fetch successful");
configurationStore.asyncWriteToCache(responseBody);
} catch (JsonSyntaxException | JsonIOException e) {
fetchErrorMessage.set(e.getMessage());
Log.e(TAG, "Error loading configuration response", e);
if (callback != null && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialization failure due to fetch response");
callback.onError("Unable to request configuration");
// If fetching failed, and cache loading failed, fire off the failure callback if not yet done so
// Otherwise, if cache has not finished yet, defer to it for firing off callbacks
if (callback != null && !cacheLoadInProgress.get() && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Failed to initialize from cache or by fetching");
callback.onError("Cache and fetch failed "+e.getMessage());
}
return;
}

// If fetching succeeded, fire off success callback if not yet done so from cache loading
if (callback != null && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialized from fetch");
callback.onCompleted();
Expand All @@ -65,8 +84,11 @@ public void onSuccess(Reader response) {

@Override
public void onFailure(String errorMessage) {
fetchErrorMessage.set(errorMessage);
Log.e(TAG, "Error fetching configuration: " + errorMessage);
if (callback != null && callbackCalled.compareAndSet(false, true)) {
// If fetching failed, and cache loading failed, fire off the failure callback if not yet done so
// Otherwise, if cache has not finished yet, defer to it for firing off callbacks
if (callback != null && !cacheLoadInProgress.get() && callbackCalled.compareAndSet(false, true)) {
Log.d(TAG, "Initialization failure due to fetch error");
callback.onError(errorMessage);
}
Expand Down
20 changes: 9 additions & 11 deletions eppo/src/main/java/cloud/eppo/android/ConfigurationStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,28 @@
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.IOException;
import java.io.Reader;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicBoolean;

import cloud.eppo.android.dto.EppoValue;
import cloud.eppo.android.dto.FlagConfig;
import cloud.eppo.android.dto.RandomizationConfigResponse;
import cloud.eppo.android.dto.deserializers.EppoValueAdapter;
import cloud.eppo.android.dto.deserializers.RandomizationConfigResponseDeserializer;
import cloud.eppo.android.dto.adapters.EppoValueAdapter;
import cloud.eppo.android.dto.adapters.RandomizationConfigResponseAdapter;
import cloud.eppo.android.util.Utils;

public class ConfigurationStore {

private static final String TAG = logTag(ConfigurationStore.class);

private final Gson gson = new GsonBuilder()
.registerTypeAdapter(RandomizationConfigResponse.class, new RandomizationConfigResponseDeserializer())
.registerTypeAdapter(RandomizationConfigResponse.class, new RandomizationConfigResponseAdapter())
.registerTypeAdapter(EppoValue.class, new EppoValueAdapter())
.serializeNulls()
.create();
private final ConfigCacheFile cacheFile;

private AtomicBoolean loadedFromFetchResponse = new AtomicBoolean(false);
private final AtomicBoolean loadedFromFetchResponse = new AtomicBoolean(false);
private ConcurrentHashMap<String, FlagConfig> flags;

public ConfigurationStore(Application application, String cacheFileNameSuffix) {
Expand Down Expand Up @@ -86,8 +85,8 @@ protected RandomizationConfigResponse readCacheFile() throws IOException {
return configResponse;
}

public void setFlagsFromResponse(Reader response) {
RandomizationConfigResponse config = gson.fromJson(response, RandomizationConfigResponse.class);
public void setFlagsFromJsonString(String jsonString) {
RandomizationConfigResponse config = gson.fromJson(jsonString, RandomizationConfigResponse.class);
Comment on lines +88 to +89
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now takes and passes to Gson a string instead of a reader.

This should be fine, provided our responses are relatively (<100mb) small, which they will be.

if (config == null || config.getFlags() == null) {
Log.w(TAG, "Flags missing in configuration response");
flags = new ConcurrentHashMap<>();
Expand All @@ -96,18 +95,17 @@ public void setFlagsFromResponse(Reader response) {
flags = config.getFlags();
Log.d(TAG, "Loaded " + flags.size() + " flags from configuration response");
}

writeConfigToFile(config);
}

private void writeConfigToFile(RandomizationConfigResponse config) {
public void asyncWriteToCache(String jsonString) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Promoted this to an upstream public function, as it seemed a weird side effect of a function that sets flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a weird side effect. This is essentially a write through cache, which is a very common pattern

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 point; I'm keeping that pattern just separating the responsibilities of the underlying functions

AsyncTask.execute(() -> {
try {
synchronized (cacheFile) {
BufferedWriter writer = cacheFile.getWriter();
gson.toJson(config, writer);
writer.write(jsonString);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👉 CORE FIX 👈
Write the response body instead of re-serializing

writer.close();
}
Log.d(TAG, "Updated cache file");
} catch (Exception e) {
Log.e(TAG, "Unable to cache config to file", e);
}
Expand Down
16 changes: 6 additions & 10 deletions eppo/src/main/java/cloud/eppo/android/EppoHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,13 @@

import android.util.Log;

import org.jetbrains.annotations.NotNull;

import java.io.IOException;
import java.io.Reader;
import java.net.HttpURLConnection;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.TimeUnit;

import okhttp3.Call;
import okhttp3.Callback;
import okhttp3.Dns;
import okhttp3.HttpUrl;
import okhttp3.OkHttpClient;
import okhttp3.Request;
Expand Down Expand Up @@ -60,7 +52,11 @@ public void get(String path, RequestCallback callback) {
public void onResponse(Call call, Response response) {
if (response.isSuccessful()) {
Log.d(TAG, "Fetch successful");
callback.onSuccess(response.body().charStream());
try {
callback.onSuccess(response.body().string());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using string() to load the entire response into a string. This should be fine as long as its relatively small.

} catch (IOException ex) {
callback.onFailure("Failed to read response from URL "+httpUrl);
}
} else {
switch (response.code()) {
case HttpURLConnection.HTTP_FORBIDDEN:
Expand All @@ -86,6 +82,6 @@ public void onFailure(Call call, IOException e) {
}

interface RequestCallback {
void onSuccess(Reader response);
void onSuccess(String responseBody);
void onFailure(String errorMessage);
}
5 changes: 1 addition & 4 deletions ...d/dto/deserializers/EppoValueAdapter.java → ...ndroid/dto/adapters/EppoValueAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package cloud.eppo.android.dto.deserializers;
package cloud.eppo.android.dto.adapters;

import static cloud.eppo.android.util.Utils.logTag;

Expand Down Expand Up @@ -45,9 +45,6 @@ public EppoValue deserialize(JsonElement json, Type typeOfT, JsonDeserialization

try {
String stringValue = json.getAsString();
if (stringValue == "null") {
return EppoValue.valueOf();
}
return EppoValue.valueOf(stringValue);
Comment on lines -48 to -50
Copy link
Contributor Author

Choose a reason for hiding this comment

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

String equality uses .equals() so this was never true and we don't even do this shenanigans in other SDKs / UFC so just axing it

} catch (Exception ignored) {
}
Expand Down
6 changes: 3 additions & 3 deletions ...domizationConfigResponseDeserializer.java → ...s/RandomizationConfigResponseAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package cloud.eppo.android.dto.deserializers;
package cloud.eppo.android.dto.adapters;

import static cloud.eppo.android.util.Utils.logTag;

Expand Down Expand Up @@ -32,8 +32,8 @@
* unreliable when ProGuard minification is in-use and not configured to protect
* JSON-deserialization-related classes and annotations.
*/
public class RandomizationConfigResponseDeserializer implements JsonDeserializer<RandomizationConfigResponse> {
private static final String TAG = logTag(RandomizationConfigResponseDeserializer.class);
public class RandomizationConfigResponseAdapter implements JsonDeserializer<RandomizationConfigResponse> {
private static final String TAG = logTag(RandomizationConfigResponseAdapter.class);

private final EppoValueAdapter eppoValueAdapter = new EppoValueAdapter();

Expand Down
18 changes: 9 additions & 9 deletions ...po/android/EppoValueDeserializerTest.java → ...ud/eppo/android/EppoValueAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,41 +9,41 @@
import org.junit.Test;

import cloud.eppo.android.dto.EppoValue;
import cloud.eppo.android.dto.deserializers.EppoValueAdapter;
import cloud.eppo.android.dto.adapters.EppoValueAdapter;

public class EppoValueDeserializerTest {
private EppoValueAdapter adapter = new EppoValueAdapter();
public class EppoValueAdapterTest {
private final EppoValueAdapter adapter = new EppoValueAdapter();

@Test
public void testDeserializingDouble() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧹 These do not actually throw any (known) Exception

public void testDeserializingDouble() {
JsonElement object = JsonParser.parseString("1");
EppoValue value = adapter.deserialize(object, EppoValue.class, null);
assertEquals(value.doubleValue(), 1, 0.001);
}

@Test
public void testDeserializingBoolean() throws Exception {
public void testDeserializingBoolean() {
JsonElement object = JsonParser.parseString("true");
EppoValue value = adapter.deserialize(object, EppoValue.class, null);
assertEquals(value.boolValue(), true);
assertTrue(value.boolValue());
}

@Test
public void testDeserializingString() throws Exception {
public void testDeserializingString() {
JsonElement object = JsonParser.parseString("\"true\"");
EppoValue value = adapter.deserialize(object, EppoValue.class, null);
assertEquals(value.stringValue(), "true");
}

@Test
public void testDeserializingArray() throws Exception {
public void testDeserializingArray() {
JsonElement object = JsonParser.parseString("[\"value1\", \"value2\"]");
EppoValue value = adapter.deserialize(object, EppoValue.class, null);
assertTrue(value.arrayValue().contains("value1"));
}

@Test
public void testDeserializingNull() throws Exception {
public void testDeserializingNull() {
JsonElement object = JsonParser.parseString("null");
EppoValue value = adapter.deserialize(object, EppoValue.class, null);
assertTrue(value.isNull());
Expand Down
8 changes: 4 additions & 4 deletions ...zationConfigResponseDeserializerTest.java → ...ndomizationConfigResponseAdapterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@
import cloud.eppo.android.dto.TargetingCondition;
import cloud.eppo.android.dto.TargetingRule;
import cloud.eppo.android.dto.Variation;
import cloud.eppo.android.dto.deserializers.EppoValueAdapter;
import cloud.eppo.android.dto.deserializers.RandomizationConfigResponseDeserializer;
import cloud.eppo.android.dto.adapters.EppoValueAdapter;
import cloud.eppo.android.dto.adapters.RandomizationConfigResponseAdapter;

public class RandomizationConfigResponseDeserializerTest {
public class RandomizationConfigResponseAdapterTest {

private final Gson gson = new GsonBuilder()
.registerTypeAdapter(RandomizationConfigResponse.class, new RandomizationConfigResponseDeserializer())
.registerTypeAdapter(RandomizationConfigResponse.class, new RandomizationConfigResponseAdapter())
.registerTypeAdapter(EppoValue.class, new EppoValueAdapter())
.serializeNulls()
.create();
Expand Down
Loading