Handle being able to be minified by ProGuard #43

aarsilv · 2024-04-09T16:04:12Z

Eppo Internal: 🎟️ Ticket: FF-1892 - Android SDK Handle loading flags when using ProGuard minification

Our existing JSON deserialization leveraged gson's automatic mapping of JSON fields to Java Object properties based on their names. However, this would fail when those property names changed due to minification, such as by ProGuard.

To get around this without the need to modify proguard-rules.pro (which could be at best burdonsome and at worst fragile), we explicitly define a deserializer and use that to parse the configuration response.

This was tested by building the example application with minification enabled. Before the changes, it didn't get assignments, but now it does! 🎉

aarsilv · 2024-04-09T16:05:41Z

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

@@ -0,0 +1,7 @@
+package cloud.eppo.android;
+
+public interface CacheLoadCallback {


Instead of having the Cache operation call the passed-through InitializationCallback, it now has its own callback. This way, if loading the configuration from the cache fails but getting the configuration from the RAC succeeds, the success callback will called (instead of the failure callback).

aarsilv · 2024-04-09T16:13:11Z

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

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


We now use a custom (vs. automagic) deserializer.

This way, upstream users of our SDK who also use ProGuard modification don't need to modify their proguard-rules.pro file. (Note: This is what LaunchDarkly requires)

It will also protect against accidentally breaking things if we add new classes that for whatever reason are not covered by the rules, gson is updated, etc.

aarsilv · 2024-04-09T16:14:09Z

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

    }

    public void setFlags(Reader response) {
        RandomizationConfigResponse config = gson.fromJson(response, RandomizationConfigResponse.class);
-        flags = config.getFlags();
-        if (flags == null) {
+        if (config == null || config.getFlags() == null) {


Re-arrange to handle the null config edge case (shouldn't happen, but being defensive)

aarsilv · 2024-04-09T16:23:54Z

eppo/src/main/java/cloud/eppo/android/dto/Allocation.java

    private List<Variation> variations;

    public float getPercentExposure() {
        return percentExposure;
    }

+    public void setPercentExposure(float percentExposure) {


Now that we are not using automagic, we need explicitly-defined setters for our DTOs

aarsilv · 2024-04-09T16:45:26Z

eppo/src/main/java/cloud/eppo/android/dto/ShardRange.java

    public void setStart(int start) {
        this.start = start;
    }

+    public int getEnd() {


Aiming for consistent get/set ordering in all our DTOs

aarsilv · 2024-04-09T16:49:43Z

.../main/java/cloud/eppo/android/dto/deserializers/RandomizationConfigResponseDeserializer.java

+ *  unreliable when ProGuard minification is in-use and not configured to protect
+ *  JSON-deserialization-related classes and annotations.
+ */
+public class RandomizationConfigResponseDeserializer implements JsonDeserializer<RandomizationConfigResponse> {


I went with RandomizationConfigResponseDeserializer instead of RandomizationConfigResponseAdapter (the pattern EppoValueAdapater followed) as it's implementing the JsonDeserializer interface so I felt it was more descriptive, even though its main use is in gson's .registerTypeAdapter() method.

aarsilv · 2024-04-09T16:55:36Z

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

@@ -1,4 +1,4 @@
-package cloud.eppo.android.dto.adapters;
+package cloud.eppo.android.dto.deserializers;


Figured EppoValueAdapater, which is a Deserializer, should be in the same folder/package as RandomizationConfigResponseDeserializer

aarsilv · 2024-04-09T16:56:23Z

.../main/java/cloud/eppo/android/dto/deserializers/RandomizationConfigResponseDeserializer.java

+        if (rootElement == null || !rootElement.isJsonObject()) {
+            Log.w(TAG, "no top-level JSON object");
+            return configResponse;
+        }
+
+        JsonObject rootObject = rootElement.getAsJsonObject();
+        JsonElement flagsElement = rootObject.get("flags");
+        if (flagsElement == null) {
+            Log.w(TAG, "no root-level flags property");
+            return configResponse;
+        }


Defensiveness against getting a response that is valid JSON, but doesn't have any flags (error/edge case)

Presumably the deserialization process will log appropriate errors for invalid JSON, like the Log.w(TAG, "Unknown operator \""+operatorKey+"\""); line that comes later

Yeah, these we want to handle especially, as it means something is up with the JSON being returned. However if we get a config but it's malformed, a JsonParseException will be thrown and caught upstream.

aarsilv · 2024-04-09T16:59:57Z

example/build.gradle

@@ -18,8 +18,9 @@ android {

    buildTypes {
        release {
-            minifyEnabled false
+            minifyEnabled true


To mirror what some customers have going on, we minify for our release build. (This let me reproduce the issue 🎉)

aarsilv · 2024-04-09T17:01:24Z

example/build.gradle

            proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'
+            signingConfig signingConfigs.debug


Added by Android Studio to allow the app to run in "release mode" on the emulator

aarsilv · 2024-04-09T17:22:48Z

example/src/main/java/cloud/eppo/androidexample/EppoApplication.java


 public class EppoApplication extends Application {
    private static final String TAG = EppoApplication.class.getSimpleName();
-    private static final String API_KEY = "REPLACE WITH YOUR API KEY";
+    private static final String API_KEY = BuildConfig.API_KEY; // Set in root-level local.properties


Pull the API key from .gitignoreed local.properties rather than having it here, where somebody could accidentally check one in.

aarsilv · 2024-04-09T17:23:38Z

example/build.gradle

+def localProperties = new Properties()
+def localPropertiesFile = rootProject.file('local.properties')
+if (localPropertiesFile.exists()) {
+    localProperties.load(new FileInputStream(localPropertiesFile))
+}


Leverage .gitignoreed local.properties for secure placing of secrets during local development and testing

aarsilv · 2024-04-09T17:24:23Z

example/build.gradle

@@ -14,12 +20,15 @@ android {
        versionName "1.0"

        testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
+
+        buildConfigField "String", "API_KEY", "\"" + (localProperties['cloud.eppo.apiKey'] ?: "need to set cloud.eppo.apiKey in local.properties") + "\""


make cloud.eppo.apiKey in local.properties available as API_KEY to the example application

aarsilv · 2024-04-09T18:09:55Z

.../main/java/cloud/eppo/android/dto/deserializers/RandomizationConfigResponseDeserializer.java

+    private final EppoValueAdapter eppoValueAdapter = new EppoValueAdapter();
+
+    @Override
+    public RandomizationConfigResponse deserialize(JsonElement rootElement, Type type, JsonDeserializationContext context) throws JsonParseException {


Example file to deserialize: https://us-central1-eppo-qa.cloudfunctions.net/serveGitHubRacTestFile/api/v3/randomized_assignment?sdkName=android

Beautified version:
test_rac.json

lisaah

🙏

lisaah · 2024-04-09T18:10:20Z

eppo/src/main/java/cloud/eppo/android/dto/TargetingRule.java

@@ -13,6 +13,10 @@ public String getAllocationKey() {
        return allocationKey;
    }

+    public void setAllocationKey(String allocationKey) {


Why not cleanup SerializedName here too?

🤦 Yes, we absolutely should! Good catch!

lisaah · 2024-04-09T18:12:03Z

.../main/java/cloud/eppo/android/dto/deserializers/RandomizationConfigResponseDeserializer.java

+ *  JSON-deserialization-related classes and annotations.
+ */
+public class RandomizationConfigResponseDeserializer implements JsonDeserializer<RandomizationConfigResponse> {
+    private static final String TAG = logTag(EppoClient.class);


copy pasta?

Suggested change

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

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

😱 Another good catch! THIS IS WHY WE DO PULL REQUESTS! 🙌

lisaah · 2024-04-09T18:14:27Z

.../main/java/cloud/eppo/android/dto/deserializers/RandomizationConfigResponseDeserializer.java

+        }
+
+        for (Map.Entry<String, JsonElement> typedOverridesEntry : jsonElement.getAsJsonObject().entrySet()) {
+            typedOverrides.put(typedOverridesEntry.getKey(), typedOverridesEntry.getValue().getAsString());


getValue() shouldn't ever be null here right?

It could be, as this is JSON, so it's certainly possible. However, if we have a typedOverrides, we're going to count on it being properly formatted. If not, this will throw a JsonParseException, which is what we want in this situation. Both places we parse are surrounded by a higher-level try-catch to handle that exception appropriately.

sameerank

Nice work!

sameerank · 2024-04-09T18:39:09Z

eppo/src/main/java/cloud/eppo/android/dto/TargetingRule.java

+    public void setAllocationKey(String allocationKey) {
+        this.allocationKey = allocationKey;
+    }
+
    public List<TargetingCondition> getConditions() {


Related to Lisa's comment -- get and set conditions methods were already in place here, but also with the @SerializedName("conditions") decorator. Was there a reason we needed both?

sameerank · 2024-04-09T18:48:00Z

.../main/java/cloud/eppo/android/dto/deserializers/RandomizationConfigResponseDeserializer.java

+        if (rootElement == null || !rootElement.isJsonObject()) {
+            Log.w(TAG, "no top-level JSON object");
+            return configResponse;
+        }
+
+        JsonObject rootObject = rootElement.getAsJsonObject();
+        JsonElement flagsElement = rootObject.get("flags");
+        if (flagsElement == null) {
+            Log.w(TAG, "no root-level flags property");
+            return configResponse;
+        }


Presumably the deserialization process will log appropriate errors for invalid JSON, like the Log.w(TAG, "Unknown operator \""+operatorKey+"\""); line that comes later

sameerank · 2024-04-09T18:49:57Z

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

+
+    @Test
+    public void testDeserialize() throws IOException {
+        File testRac = new File("src/androidTest/assets/rac-experiments-v3.json");


sameerank · 2024-04-09T18:54:01Z

eppo/src/main/java/cloud/eppo/android/dto/RandomizationConfigResponse.java

    private ConcurrentHashMap<String, FlagConfig> flags;

    public ConcurrentHashMap<String, FlagConfig> getFlags() {
        return flags;
    }
+
+    public void setFlags(ConcurrentHashMap<String, FlagConfig> flags) {


And this time, we don't need a getFlags method?

aarsilv added 2 commits April 8, 2024 22:45

tests passing

d98a4dd

clean up serialization code

02b721b

aarsilv commented Apr 9, 2024

View reviewed changes

consistent ordering for getters and setters

54227ed

aarsilv commented Apr 9, 2024

View reviewed changes

aarsilv added 2 commits April 9, 2024 10:50

changes from self-review of PR

774d935

pass in API key using root-level local.properties

0a4c254

aarsilv commented Apr 9, 2024

View reviewed changes

aarsilv marked this pull request as ready for review April 9, 2024 17:40

aarsilv requested review from petzel and sameerank April 9, 2024 17:40

aarsilv assigned sameerank Apr 9, 2024

aarsilv commented Apr 9, 2024

View reviewed changes

lisaah approved these changes Apr 9, 2024

View reviewed changes

sameerank approved these changes Apr 9, 2024

View reviewed changes

aarsilv added 2 commits April 9, 2024 12:57

feedback from PR

ddf65d7

cache images also based on gradle settings

2b7fa61

aarsilv assigned aarsilv and unassigned sameerank Apr 9, 2024

aarsilv merged commit 34c5c51 into main Apr 9, 2024
1 check passed

aarsilv deleted the aaron/ff-1892/handle-minification branch April 9, 2024 19:16

aarsilv mentioned this pull request Apr 9, 2024

Bump version to 1.0.6 #44

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle being able to be minified by ProGuard #43

Handle being able to be minified by ProGuard #43

aarsilv commented Apr 9, 2024 •

edited

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

sameerank Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

aarsilv Apr 9, 2024

lisaah left a comment

lisaah Apr 9, 2024

aarsilv Apr 9, 2024

lisaah Apr 9, 2024

aarsilv Apr 9, 2024

lisaah Apr 9, 2024

aarsilv Apr 9, 2024

sameerank left a comment

sameerank Apr 9, 2024

sameerank Apr 9, 2024

sameerank Apr 9, 2024

sameerank Apr 9, 2024

		@@ -0,0 +1,7 @@
		package cloud.eppo.android;

		public interface CacheLoadCallback {

		@@ -1,4 +1,4 @@
		package cloud.eppo.android.dto.adapters;
		package cloud.eppo.android.dto.deserializers;

		proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'
		signingConfig signingConfigs.debug

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

Handle being able to be minified by ProGuard #43

Handle being able to be minified by ProGuard #43

Conversation

aarsilv commented Apr 9, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisaah left a 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

sameerank left a 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 Apr 9, 2024 •

edited