Skip to content

feat: refactor to allow for parsing of any single leaf or array of leafs #235

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 4 commits into from
Nov 16, 2018
Merged

feat: refactor to allow for parsing of any single leaf or array of leafs #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec08d34
refactor to allow for parsing of any single leaf or array of leafs
thomaszurkan-optimizely Nov 15, 2018
d77b42f
remove unused import
thomaszurkan-optimizely Nov 15, 2018
cc855a5
add parsing for audience.conditions leaf nodes
thomaszurkan-optimizely Nov 15, 2018
17e7b60
Merge branch 'master' into featParseLeafNode
thomaszurkan-optimizely Nov 16, 2018
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
11 changes: 9 additions & 2 deletions core-api/src/main/java/com/optimizely/ab/config/parser/AudienceGsonDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,15 @@ public Audience deserialize(JsonElement json, Type typeOfT, JsonDeserializationC
if (!typeOfT.toString().contains("TypedAudience")) {
conditionsElement = parser.parse(jsonObject.get("conditions").getAsString());
}
List<Object> rawObjectList = gson.fromJson(conditionsElement, List.class);
Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, rawObjectList);
Condition conditions = null;
if (conditionsElement.isJsonArray()) {
List<Object> rawObjectList = gson.fromJson(conditionsElement, List.class);
conditions = ConditionUtils.parseConditions(UserAttribute.class, rawObjectList);
}
else if (conditionsElement.isJsonObject()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: predominant convention in this repo is to have } else if (...) { on one line

Object object = gson.fromJson(conditionsElement,Object.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space after ,

conditions = ConditionUtils.parseConditions(UserAttribute.class, object);
}

return new Audience(id, name, conditions);
}
Expand Down
13 changes: 10 additions & 3 deletions core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.gson.JsonObject;
import com.google.gson.JsonParseException;
import com.google.gson.JsonParser;
import com.google.gson.internal.LinkedTreeMap;
import com.google.gson.reflect.TypeToken;
import com.optimizely.ab.bucketing.DecisionService;
import com.optimizely.ab.config.Experiment;
Expand Down Expand Up @@ -108,10 +109,16 @@ static Condition parseAudienceConditions(JsonObject experimentJson) {

JsonElement conditionsElement = experimentJson.get("audienceConditions");

List<Object> rawObjectList = gson.fromJson(conditionsElement, List.class);
Condition conditions = ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, rawObjectList);
if (conditionsElement.isJsonArray()) {
List<Object> rawObjectList = gson.fromJson(conditionsElement, List.class);
return ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, rawObjectList);
}
else if (conditionsElement.isJsonObject()) {
Object jsonObject = gson.fromJson(conditionsElement,Object.class);
return ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, jsonObject);
}

return conditions;
return null;
}

static Experiment parseExperiment(JsonObject experimentJson, String groupId, JsonDeserializationContext context) {
Expand Down
22 changes: 15 additions & 7 deletions core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.optimizely.ab.internal.ConditionUtils;
import org.json.JSONArray;
import org.json.JSONObject;
import org.json.JSONTokener;

import javax.annotation.Nonnull;
import java.util.ArrayList;
Expand Down Expand Up @@ -148,9 +149,7 @@ private List<Experiment> parseExperiments(JSONArray experimentJson, String group
Condition conditions = null;
if (experimentObject.has("audienceConditions")) {
Object jsonCondition = experimentObject.get("audienceConditions");
if (jsonCondition instanceof JSONArray) {
conditions = ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, (JSONArray) jsonCondition);
}
conditions = ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, jsonCondition);
}

// parse the child objects
Expand Down Expand Up @@ -289,9 +288,19 @@ private List<Audience> parseAudiences(JSONArray audienceJson) {
String id = audienceObject.getString("id");
String key = audienceObject.getString("name");
Object conditionsObject = audienceObject.get("conditions");
JSONArray conditionJson = new JSONArray((String)conditionsObject);
if (conditionsObject instanceof String) { // should always be true
JSONTokener tokener = new JSONTokener((String)conditionsObject);
char token = tokener.nextClean();
if (token =='[') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space after ==

// must be an array
conditionsObject = new JSONArray((String)conditionsObject);
}
else if (token =='{') {
conditionsObject = new JSONObject((String)conditionsObject);
}
}

Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, conditionJson);
Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, conditionsObject);
audiences.add(new Audience(id, key, conditions));
}

Expand All @@ -306,9 +315,8 @@ private List<Audience> parseTypedAudiences(JSONArray audienceJson) {
String id = audienceObject.getString("id");
String key = audienceObject.getString("name");
Object conditionsObject = audienceObject.get("conditions");
JSONArray conditionJson = (JSONArray)conditionsObject;

Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, conditionJson);
Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, conditionsObject);
audiences.add(new Audience(id, key, conditions));
}

Expand Down
17 changes: 7 additions & 10 deletions core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,11 @@ private List<Experiment> parseExperiments(JSONArray experimentJson, String group
Condition conditions = null;
if (experimentObject.containsKey("audienceConditions")) {
Object jsonCondition = experimentObject.get("audienceConditions");
if (jsonCondition instanceof JSONArray) {
try {
conditions = ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, (JSONArray)jsonCondition);
} catch (Exception e) {
// unable to parse conditions.
Logger.getAnonymousLogger().log(Level.ALL, "problem parsing audience conditions", e);
}
try {
conditions = ConditionUtils.<AudienceIdCondition>parseConditions(AudienceIdCondition.class, jsonCondition);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nchilada this is the only other one that will expect any type. audience.conditions as we mentioned in the ticket and offline will remain legacy.

} catch (Exception e) {
// unable to parse conditions.
Logger.getAnonymousLogger().log(Level.ALL, "problem parsing audience conditions", e);
}
}
// parse the child objects
Expand Down Expand Up @@ -303,7 +301,7 @@ private List<Audience> parseAudiences(JSONArray audienceJson) throws ParseExcept
String id = (String)audienceObject.get("id");
String key = (String)audienceObject.get("name");
Object conditionObject = audienceObject.get("conditions");
JSONArray conditionJson = (JSONArray)parser.parse((String)conditionObject);
Object conditionJson = parser.parse((String)conditionObject);
Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, conditionJson);
audiences.add(new Audience(id, key, conditions));
}
Expand All @@ -319,8 +317,7 @@ private List<Audience> parseTypedAudiences(JSONArray audienceJson) throws ParseE
String id = (String)audienceObject.get("id");
String key = (String)audienceObject.get("name");
Object conditionObject = audienceObject.get("conditions");
JSONArray conditionJson = (JSONArray)conditionObject;
Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, conditionJson);
Condition conditions = ConditionUtils.<UserAttribute>parseConditions(UserAttribute.class, conditionObject);
audiences.add(new Audience(id, key, conditions));
}

Expand Down
142 changes: 71 additions & 71 deletions core-api/src/main/java/com/optimizely/ab/internal/ConditionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,75 @@
import java.util.Map;

public class ConditionUtils {

static public <T> Condition parseConditions(Class<T> clazz, Object object) throws InvalidAudienceCondition {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO overloading a method that's already overloaded (twice) with Object type is not conducive to code that's easy to reason or pleasant to work with. Would be better off with a different method name.

Second. A method as complex as this needs to be clearly documented. There are lots of branches in here and right now it just looks like a kitchen sink.

if (object instanceof List) {
List<Object> objectList = (List<Object>)object;
return ConditionUtils.<T>parseConditions(clazz, objectList);
}
else if (object instanceof String) { // looking for audience conditions in experiment
AudienceIdCondition audienceIdCondition = new AudienceIdCondition<T>((String)object);
if (clazz.isInstance(audienceIdCondition)) {
return audienceIdCondition;
}
else {
throw new InvalidAudienceCondition(String.format("Expected AudienceIdCondition got %s", clazz.getCanonicalName()));
}
}
else if (object instanceof LinkedTreeMap) { // gson
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}

LinkedTreeMap<String, ?> conditionMap = (LinkedTreeMap<String, ?>)object;
return new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
(String)conditionMap.get("match"), conditionMap.get("value"));
}
else if (object instanceof JSONObject) {
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}

JSONObject conditionMap = (JSONObject)object;
return new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: space after (String)

(String)conditionMap.get("match"), conditionMap.get("value"));
}
else if (object instanceof org.json.JSONArray) {
return ConditionUtils.<T>parseConditions(clazz, (org.json.JSONArray) object);
}
else if (object instanceof org.json.JSONObject){
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}
org.json.JSONObject conditionMap = (org.json.JSONObject)object;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after (org.json.JSONObject)

String match = null;
Object value = null;
if (conditionMap.has("match")) {
match = (String) conditionMap.get("match");
}
if (conditionMap.has("value")) {
value = conditionMap.get("value");
}
return new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
Copy link
Contributor

Choose a reason for hiding this comment

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

indent of this line looks off.

match, value);
}

else { // looking for audience conditions in audience
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}

Map<String, ?> conditionMap = (Map<String, ?>)object;
return new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
(String)conditionMap.get("match"), conditionMap.get("value"));
}

}

/**
* parse conditions using List and Map
* @param rawObjectList list of conditions
Expand All @@ -55,49 +124,7 @@ static public <T> Condition parseConditions(Class<T> clazz, List<Object> rawObje

for (int i = startingParseIndex; i < rawObjectList.size(); i++) {
Object obj = rawObjectList.get(i);
if (obj instanceof List) {
List<Object> objectList = (List<Object>)rawObjectList.get(i);
conditions.add(ConditionUtils.<T>parseConditions(clazz, objectList));
}
else if (obj instanceof String) { // looking for audience conditions in experiment
AudienceIdCondition audienceIdCondition = new AudienceIdCondition<T>((String)obj);
if (clazz.isInstance(audienceIdCondition)) {
conditions.add(new AudienceIdCondition((String) obj));
}
else {
throw new InvalidAudienceCondition(String.format("Expected AudienceIdCondition got %s", clazz.getCanonicalName()));
}
}
else if (obj instanceof LinkedTreeMap) { // gson
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}

LinkedTreeMap<String, ?> conditionMap = (LinkedTreeMap<String, ?>)rawObjectList.get(i);
conditions.add(new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
(String)conditionMap.get("match"), conditionMap.get("value")));
}
else if (obj instanceof JSONObject) {
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}

JSONObject conditionMap = (JSONObject)obj;
conditions.add(new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
(String)conditionMap.get("match"), conditionMap.get("value")));
}
else { // looking for audience conditions in audience
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}

Map<String, ?> conditionMap = (Map<String, ?>)rawObjectList.get(i);
conditions.add(new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
(String)conditionMap.get("match"), conditionMap.get("value")));
}
conditions.add(parseConditions(clazz, obj));
}

Condition condition;
Expand Down Expand Up @@ -158,33 +185,7 @@ static public <T> Condition parseConditions(Class<T> clazz, org.json.JSONArray c

for (int i = startingParseIndex; i < conditionJson.length(); i++) {
Object obj = conditionJson.get(i);
if (obj instanceof org.json.JSONArray) {
conditions.add(ConditionUtils.<T>parseConditions(clazz, (org.json.JSONArray) conditionJson.get(i)));
} else if (obj instanceof String) {
AudienceIdCondition<T> audiencCondition = new AudienceIdCondition<T>((String)obj);
if (clazz.isInstance(audiencCondition)) {
conditions.add(audiencCondition);
}
else {
throw new InvalidAudienceCondition(String.format("Expected AudienceIdCondition got %s", clazz.getCanonicalName()));
}
} else {
if (clazz != UserAttribute.class) {
throw new InvalidAudienceCondition(String.format("Expected UserAttributes got %s", clazz.getCanonicalName()));

}
org.json.JSONObject conditionMap = (org.json.JSONObject)obj;
String match = null;
Object value = null;
if (conditionMap.has("match")) {
match = (String) conditionMap.get("match");
}
if (conditionMap.has("value")) {
value = conditionMap.get("value");
}
conditions.add(new UserAttribute((String)conditionMap.get("name"), (String)conditionMap.get("type"),
match, value));
}
conditions.add(parseConditions(clazz, obj));
}

Condition condition;
Expand All @@ -205,5 +206,4 @@ static public <T> Condition parseConditions(Class<T> clazz, org.json.JSONArray c

return condition;
}

}