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.

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

Handle being able to be minified by ProGuard #43

Merged
merged 7 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
7 changes: 7 additions & 0 deletions eppo/src/main/java/cloud/eppo/android/CacheLoadCallback.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package cloud.eppo.android;

public interface CacheLoadCallback {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

void onCacheLoadSuccess();

void onCacheLoadFail();
}
23 changes: 18 additions & 5 deletions eppo/src/main/java/cloud/eppo/android/ConfigurationRequestor.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.google.gson.JsonSyntaxException;

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

import cloud.eppo.android.dto.FlagConfig;

Expand All @@ -23,7 +24,19 @@ public ConfigurationRequestor(ConfigurationStore configurationStore, EppoHttpCli
}

public void load(InitializationCallback callback) {
boolean usedCache = configurationStore.loadFromCache(callback);
AtomicBoolean cachedUsed = new AtomicBoolean(false);
configurationStore.loadFromCache(new CacheLoadCallback() {
@Override
public void onCacheLoadSuccess() {
cachedUsed.set(true);
callback.onCompleted();
}

@Override
public void onCacheLoadFail() {
cachedUsed.set(false);
}
});

client.get("/api/randomized_assignment/v3/config", new RequestCallback() {
@Override
Expand All @@ -32,21 +45,21 @@ public void onSuccess(Reader response) {
configurationStore.setFlags(response);
} catch (JsonSyntaxException | JsonIOException e) {
Log.e(TAG, "Error loading configuration response", e);
if (callback != null && !usedCache) {
callback.onError("Unable to load configuration");
if (callback != null && !cachedUsed.get()) {
callback.onError("Unable to request configuration");
}
return;
}

if (callback != null && !usedCache) {
if (callback != null && !cachedUsed.get()) {
callback.onCompleted();
}
}

@Override
public void onFailure(String errorMessage) {
Log.e(TAG, errorMessage);
if (callback != null && !usedCache) {
if (callback != null && !cachedUsed.get()) {
callback.onError(errorMessage);
}
}
Expand Down
24 changes: 10 additions & 14 deletions eppo/src/main/java/cloud/eppo/android/ConfigurationStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@
import cloud.eppo.android.dto.FlagConfig;
import cloud.eppo.android.dto.RandomizationConfigResponse;
import cloud.eppo.android.dto.adapters.EppoValueAdapter;
import cloud.eppo.android.dto.deserializers.RandomizationConfigResponseDeserializer;
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())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

.registerTypeAdapter(EppoValue.class, new EppoValueAdapter())
.serializeNulls()
.create();
Expand All @@ -43,10 +45,11 @@ public ConfigurationStore(Application application) {
this.sharedPrefs = Utils.getSharedPrefs(application);
}

public boolean loadFromCache(InitializationCallback callback) {
public void loadFromCache(CacheLoadCallback callback) {
if (flags != null || !cacheFile.exists()) {
Log.d(TAG, "Not loading from cache ("+(flags == null ? "null flags" : "non-null flags")+")");
return false;
callback.onCacheLoadFail();
return;
}

AsyncTask.execute(() -> {
Expand All @@ -57,35 +60,28 @@ public boolean loadFromCache(InitializationCallback callback) {
RandomizationConfigResponse configResponse = gson.fromJson(reader, RandomizationConfigResponse.class);
reader.close();
if (configResponse == null || configResponse.getFlags() == null) {
// Invalid cached configuration, initialize as an empty map and delete file
throw new JsonSyntaxException("Configuration file missing flags");
}
flags = configResponse.getFlags();
updateConfigsInSharedPrefs();
}
Log.d(TAG, "Cache loaded successfully");
callback.onCacheLoadSuccess();
} catch (Exception e) {
Log.e(TAG, "Error loading from local cache", e);
cacheFile.delete();

if (callback != null) {
callback.onError("Unable to load config from cache");
}
}

if (callback != null) {
callback.onCompleted();
callback.onCacheLoadFail();
}
});
return true;
}

public void setFlags(Reader response) {
RandomizationConfigResponse config = gson.fromJson(response, RandomizationConfigResponse.class);
flags = config.getFlags();
if (flags == null) {
if (config == null || config.getFlags() == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Log.w(TAG, "Flags missing in configuration response");
flags = new ConcurrentHashMap<>();
} else {
flags = config.getFlags();
}

// update any existing flags already in shared prefs
Expand Down
11 changes: 8 additions & 3 deletions eppo/src/main/java/cloud/eppo/android/dto/Allocation.java
Original file line number Diff line number Diff line change
@@ -1,20 +1,25 @@
package cloud.eppo.android.dto;

import java.util.List;
import com.google.gson.annotations.SerializedName;

public class Allocation {
@SerializedName("percentExposure")
private float percentExposure;

@SerializedName("variations")
private List<Variation> variations;

public float getPercentExposure() {
return percentExposure;
}

public void setPercentExposure(float percentExposure) {
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 we are not using automagic, we need explicitly-defined setters for our DTOs

this.percentExposure = percentExposure;
}

public List<Variation> getVariations() {
return variations;
}

public void setVariations(List<Variation> variations) {
this.variations = variations;
}
}
27 changes: 20 additions & 7 deletions eppo/src/main/java/cloud/eppo/android/dto/FlagConfig.java
Original file line number Diff line number Diff line change
@@ -1,44 +1,57 @@
package cloud.eppo.android.dto;

import com.google.gson.annotations.SerializedName;

import java.util.List;
import java.util.Map;
import java.util.HashMap;

public class FlagConfig {
@SerializedName("subjectShards")
private int subjectShards;

@SerializedName("enabled")
private boolean enabled;

@SerializedName("typedOverrides")
private Map<String, String> typedOverrides = new HashMap<>();

@SerializedName("rules")
private List<TargetingRule> rules;

@SerializedName("allocations")
private Map<String, Allocation> allocations;

public int getSubjectShards() {
return subjectShards;
}

public void setSubjectShards(int subjectShards) {
this.subjectShards = subjectShards;
}

public boolean isEnabled() {
return enabled;
}

public void setEnabled(boolean enabled) {
this.enabled = enabled;
}

public Map<String, String> getTypedOverrides() {
return typedOverrides;
}

public void setTypedOverrides(Map<String, String> typedOverrides) {
this.typedOverrides = typedOverrides;
}

public Map<String, Allocation> getAllocations() {
return allocations;
}

public void setAllocations(Map<String, Allocation> allocations) {
this.allocations = allocations;
}

public List<TargetingRule> getRules() {
return rules;
}

public void setRules(List<TargetingRule> rules) {
this.rules = rules;
}
}
2 changes: 2 additions & 0 deletions eppo/src/main/java/cloud/eppo/android/dto/OperatorType.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ public enum OperatorType {
this.value = value;
}


// TODO: does this work with obfuscation?
public static OperatorType fromString(String value) {
for (OperatorType type : OperatorType.values()) {
if (type.value.equals(value)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package cloud.eppo.android.dto;

import java.util.concurrent.ConcurrentHashMap;
import com.google.gson.annotations.SerializedName;

public class RandomizationConfigResponse {
@SerializedName("flags")
private ConcurrentHashMap<String, FlagConfig> flags;

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

public void setFlags(ConcurrentHashMap<String, FlagConfig> flags) {

Choose a reason for hiding this comment

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

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

this.flags = flags;
}
}
8 changes: 4 additions & 4 deletions eppo/src/main/java/cloud/eppo/android/dto/ShardRange.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ public int getStart() {
return start;
}

public int getEnd() {
return end;
}

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

public int getEnd() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aiming for consistent get/set ordering in all our DTOs

return end;
}

public void setEnd(int end) {
this.end = end;
}
Expand Down
19 changes: 8 additions & 11 deletions eppo/src/main/java/cloud/eppo/android/dto/TargetingCondition.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,35 +3,32 @@
import com.google.gson.annotations.SerializedName;

public class TargetingCondition {
@SerializedName("operator")
private String operator;

@SerializedName("attribute")
private String attribute;

@SerializedName("value")
private EppoValue value;

public OperatorType getOperator() {
return OperatorType.fromString(operator);
}

public String getAttribute() {
return attribute;
}

public EppoValue getValue() {
return value;
}

public void setOperator(OperatorType operatorType) {
this.operator = operatorType.value;
}

public String getAttribute() {
return attribute;
}

public void setAttribute(String attribute) {
this.attribute = attribute;
}

public EppoValue getValue() {
return value;
}

public void setValue(EppoValue value) {
this.value = value;
}
Expand Down
6 changes: 6 additions & 0 deletions eppo/src/main/java/cloud/eppo/android/dto/TargetingRule.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,17 @@ public String getAllocationKey() {
return allocationKey;
}

public void setAllocationKey(String allocationKey) {
Copy link

Choose a reason for hiding this comment

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

Why not cleanup SerializedName here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Yes, we absolutely should! Good catch!

this.allocationKey = allocationKey;
}

public List<TargetingCondition> getConditions() {

Choose a reason for hiding this comment

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

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?

return conditions;
}

public void setConditions(List<TargetingCondition> conditions) {
this.conditions = conditions;
}


}
12 changes: 9 additions & 3 deletions eppo/src/main/java/cloud/eppo/android/dto/Variation.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,23 @@
import com.google.gson.annotations.SerializedName;

public class Variation {
@SerializedName("typedValue")
private EppoValue typedValue;

@SerializedName("shardRange")

private ShardRange shardRange;

public EppoValue getTypedValue() {
return typedValue;
}

public void setTypedValue(EppoValue typedValue) {
this.typedValue = typedValue;
}

public ShardRange getShardRange() {
return shardRange;
}

public void setShardRange(ShardRange shardRange) {
this.shardRange = shardRange;
}
}
Loading
Loading