Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
*/
package com.optimizely.ab.config.parser;

import com.google.gson.JsonElement;
import com.google.gson.JsonIOException;
import com.google.gson.JsonSyntaxException;
import com.google.gson.stream.JsonWriter;
import com.optimizely.ab.config.ProjectConfig;

import javax.annotation.Nonnull;
Expand All @@ -38,4 +42,10 @@ public interface ConfigParser {
* @throws ConfigParseException when there's an issue parsing the provided project config
*/
ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParseException;

/**
* OptimizelyJSON parsing
*/
String toJson(Object src) throws ConfigParseException;
<T> T fromJson(String json, Class<T> clazz) throws ConfigParseException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create a new exception JsonParseException for these?

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2017, 2019, Optimizely and contributors
* Copyright 2016-2017, 2019-2020, Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -18,6 +18,7 @@

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.JsonSyntaxException;
import com.optimizely.ab.config.*;
import com.optimizely.ab.config.audience.Audience;
import com.optimizely.ab.config.audience.TypedAudience;
Expand All @@ -27,7 +28,7 @@
/**
* {@link Gson}-based config parser implementation.
*/
final class GsonConfigParser implements ConfigParser {
final public class GsonConfigParser implements ConfigParser {

@Override
public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParseException {
Expand All @@ -52,4 +53,17 @@ public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParse
throw new ConfigParseException("Unable to parse datafile: " + json, e);
}
}

public String toJson(Object src) {
return new Gson().toJson(src);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just realizing that we're not caching our Gson objects. I think this is worth benchmarking as this can be a performance bottleneck. Note that the JacksonConfigParser stores an ObjectMapper on the class implementation. This might have been ok when periodically parsing the Datafile, but if OptimizelyJSON is used heavily this could be an issue.

}

public <T> T fromJson(String json, Class<T> clazz) throws ConfigParseException {
try {
return new Gson().fromJson(json, clazz);
} catch (Exception e) {
throw new ConfigParseException("Unable to parse JSON string: " + e.toString());
}
}

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2018, Optimizely and contributors
* Copyright 2016-2018, 2020, Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,6 +16,7 @@
*/
package com.optimizely.ab.config.parser;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.module.SimpleModule;
import com.optimizely.ab.config.DatafileProjectConfig;
Expand All @@ -25,11 +26,12 @@
import com.optimizely.ab.config.audience.TypedAudience;

import javax.annotation.Nonnull;
import java.io.IOException;

/**
* {@code Jackson}-based config parser implementation.
*/
final class JacksonConfigParser implements ConfigParser {
final public class JacksonConfigParser implements ConfigParser {
private ObjectMapper objectMapper;

public JacksonConfigParser() {
Expand Down Expand Up @@ -61,4 +63,23 @@ public ProjectConfigModule() {
addDeserializer(Condition.class, new ConditionJacksonDeserializer(objectMapper));
}
}

@Override
public String toJson(Object src) throws ConfigParseException {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're missing unit tests on these new implementations.

try {
return objectMapper.writeValueAsString(src);
} catch (JsonProcessingException e) {
throw new ConfigParseException("Serialization failed: " + e.toString());
}
}

@Override
public <T> T fromJson(String json, Class<T> clazz) throws ConfigParseException {
try {
return objectMapper.readValue(json, clazz);
} catch (IOException e) {
throw new ConfigParseException("Unable to parse JSON string: " + e.toString());
}
}

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2019, Optimizely and contributors
* Copyright 2016-2019, 2020, Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -28,17 +28,12 @@
import org.json.JSONTokener;

import javax.annotation.Nonnull;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.*;

/**
* {@code org.json}-based config parser implementation.
*/
final class JsonConfigParser implements ConfigParser {
final public class JsonConfigParser implements ConfigParser {

@Override
public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParseException {
Expand Down Expand Up @@ -385,4 +380,22 @@ private List<Rollout> parseRollouts(JSONArray rolloutsJson) {

return rollouts;
}

@Override
public String toJson(Object src) {
JSONObject json = (JSONObject)JsonHelpers.convertToJsonObject(src);
return json.toString();
}

@Override
public <T> T fromJson(String json, Class<T> clazz) throws ConfigParseException {
if (Map.class.isAssignableFrom(clazz)) {
JSONObject obj = new JSONObject(json);
return (T)JsonHelpers.jsonObjectToMap(obj);
}

// org.json parser does not support parsing to user objects
throw new ConfigParseException("Parsing fails with a unsupported type");
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
*
* Copyright 2020, Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.config.parser;

import org.json.JSONArray;
import org.json.JSONObject;

import java.util.*;

final class JsonHelpers {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a corresponding JsonHelpersTest class for unit tests.


static Object convertToJsonObject(Object obj) {
if (obj instanceof Map) {
Map<Object, Object> map = (Map)obj;
JSONObject jObj = new JSONObject();
for (Map.Entry entry : map.entrySet()) {
jObj.put(entry.getKey().toString(), convertToJsonObject(entry.getValue()));
}
return jObj;
} else if (obj instanceof List) {
List list = (List)obj;
JSONArray jArray = new JSONArray();
for (Object value : list) {
jArray.put(convertToJsonObject(value));
}
return jArray;
} else {
return obj;
}
}

static Map<String, Object> jsonObjectToMap(JSONObject jObj) {
Map<String, Object> map = new HashMap<>();

Iterator<String> keys = jObj.keys();
while(keys.hasNext()) {
String key = keys.next();
Object value = jObj.get(key);

if (value instanceof JSONArray) {
value = jsonArrayToList((JSONArray)value);
} else if (value instanceof JSONObject) {
value = jsonObjectToMap((JSONObject)value);
}

map.put(key, value);
}

return map;
}

static List<Object> jsonArrayToList(JSONArray array) {
List<Object> list = new ArrayList<>();
for(Object value : array) {
if (value instanceof JSONArray) {
value = jsonArrayToList((JSONArray)value);
} else if (value instanceof JSONObject) {
value = jsonObjectToMap((JSONObject)value);
}

list.add(value);
}

return list;
}

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2019, Optimizely and contributors
* Copyright 2016-2019, 2020, Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -26,6 +26,7 @@
import com.optimizely.ab.internal.ConditionUtils;
import org.json.simple.JSONArray;
import org.json.simple.JSONObject;
import org.json.simple.JSONValue;
import org.json.simple.parser.JSONParser;
import org.json.simple.parser.ParseException;

Expand All @@ -41,7 +42,7 @@
/**
* {@code json-simple}-based config parser implementation.
*/
final class JsonSimpleConfigParser implements ConfigParser {
final public class JsonSimpleConfigParser implements ConfigParser {

@Override
public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParseException {
Expand Down Expand Up @@ -371,5 +372,22 @@ private List<Rollout> parseRollouts(JSONArray rolloutsJson) {

return rollouts;
}

@Override
public String toJson(Object src) {
return JSONValue.toJSONString(src);
}

@Override
public <T> T fromJson(String json, Class<T> clazz) throws ConfigParseException {
if (Map.class.isAssignableFrom(clazz)) {
org.json.JSONObject obj = new org.json.JSONObject(json);
Copy link
Contributor

Choose a reason for hiding this comment

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

You're using org.json.JSONObject and not org.json.simple.

return (T)JsonHelpers.jsonObjectToMap(obj);
}

// org.json.simple does not support parsing to user objects
throw new ConfigParseException("Parsing fails with a unsupported type");
}

}

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
*
* Copyright 2020, Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.optimizely.ab.config.parser;

public final class UnsupportedOperationException extends Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

UnsupportedOperationException is also a built-in Java runtime exception that we shouldn't implement our own checked exception with the same name as this is confusing.

public UnsupportedOperationException(String message) {
super(message);
}

public UnsupportedOperationException(String message, Throwable cause) {
super(message, cause);
}
}
Loading