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

aarsilv · 2024-05-23T21:42:50Z

Eppo Internal:
🎟️ ticket: FF-2159 - Android SDK Needs a custom serializer to handle code minification

If ProGuard is used, then Gson won't serialize flag configurations with known field names. To get around this, we should just write the JSON file exactly as we received it.

Verified fix checking the logs of our example app, which has its source minified using ProGuard.

Before:

Loading from cache
Error loading from local cache
java.lang.NullPointerException: Attempt to invoke virtual method 'int com.google.gson.JsonElement.getAsInt()' on a null object reference

After:

Loading from cache
Cache loaded successfully
Initialized from cache

aarsilv · 2024-05-23T21:44:14Z

eppo/src/androidTest/java/cloud/eppo/android/EppoClientTest.java

@@ -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;


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

aarsilv · 2024-05-23T21:44:33Z

eppo/src/androidTest/java/cloud/eppo/android/EppoClientTest.java


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


🧹 some cleanup suggested by Android Studio

aarsilv · 2024-05-23T21:45:07Z

eppo/src/androidTest/java/cloud/eppo/android/EppoClientTest.java

@@ -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


This is really what we are waiting for

aarsilv · 2024-05-23T21:45:29Z

eppo/src/main/java/cloud/eppo/android/ConfigurationRequestor.java

+        // 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


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

aarsilv · 2024-05-23T21:46:53Z

eppo/src/main/java/cloud/eppo/android/ConfigurationStore.java

+    public void setFlagsFromJsonString(String jsonString) {
+        RandomizationConfigResponse config = gson.fromJson(jsonString, RandomizationConfigResponse.class);


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.

aarsilv · 2024-05-23T21:47:26Z

eppo/src/main/java/cloud/eppo/android/ConfigurationStore.java

    }

-    private void writeConfigToFile(RandomizationConfigResponse config) {
+    public void asyncWriteToCache(String jsonString) {


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

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

yeah that's a good point; I'm keeping that pattern just separating the responsibilities of the underlying functions

aarsilv · 2024-05-23T21:49:35Z

eppo/src/main/java/cloud/eppo/android/EppoHttpClient.java

@@ -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());


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

aarsilv · 2024-05-23T21:50:04Z

eppo/src/test/java/cloud/eppo/android/EppoValueAdapterTest.java


    @Test
-    public void testDeserializingDouble() throws Exception {


🧹 These do not actually throw any (known) Exception

aarsilv · 2024-05-23T21:50:48Z

eppo/src/main/java/cloud/eppo/android/ConfigurationStore.java

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


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

aarsilv · 2024-05-23T22:16:41Z

eppo/src/main/java/cloud/eppo/android/dto/adapters/EppoValueAdapter.java

-                if (stringValue == "null") {
-                    return EppoValue.valueOf();
-                }


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

leoromanovsky · 2024-05-24T03:17:20Z

eppo/src/main/java/cloud/eppo/android/ConfigurationRequestor.java

+                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


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.

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

aarsilv added 2 commits May 23, 2024 15:40

write cache file as we get it

2629071

update comment

1d8f818

aarsilv commented May 23, 2024

View reviewed changes

aarsilv requested review from petzel and leoromanovsky May 23, 2024 21:54

aarsilv assigned leoromanovsky May 23, 2024

aarsilv marked this pull request as ready for review May 23, 2024 21:54

aarsilv requested a review from greghuels May 23, 2024 22:01

aarsilv commented May 23, 2024

View reviewed changes

petzel approved these changes May 24, 2024

View reviewed changes

leoromanovsky approved these changes May 24, 2024

View reviewed changes

bump version number

b9b073e

aarsilv assigned aarsilv and unassigned leoromanovsky May 24, 2024

aarsilv merged commit e7239ab into main May 24, 2024
1 check passed

aarsilv deleted the aaron/ff-2159/explicit-serialize branch May 24, 2024 04:25

sameerank self-requested a review May 28, 2024 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aarsilv commented May 23, 2024 •

edited

Loading

aarsilv May 23, 2024

aarsilv May 23, 2024

aarsilv May 23, 2024

aarsilv May 23, 2024

aarsilv May 23, 2024

aarsilv May 23, 2024

petzel May 24, 2024

aarsilv May 24, 2024

aarsilv May 23, 2024

aarsilv May 23, 2024

aarsilv May 23, 2024

aarsilv May 23, 2024

leoromanovsky May 24, 2024

aarsilv May 24, 2024

		public void setFlagsFromJsonString(String jsonString) {
		RandomizationConfigResponse config = gson.fromJson(jsonString, RandomizationConfigResponse.class);


		@Test
		public void testDeserializingDouble() throws Exception {

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

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

Conversation

aarsilv commented May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarsilv commented May 23, 2024 •

edited

Loading