feature: add OptimizelyJSON #371

jaeopt · 2020-04-21T18:33:09Z

Summary

Add OptimizelyJSON for JSON feature variable access

coveralls · 2020-04-21T18:50:44Z

Pull Request Test Coverage Report for Build 1389

115 of 132 (87.12%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.09%) to 89.222%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java	4	6	66.67%
core-api/src/main/java/com/optimizely/ab/config/parser/JsonParseException.java	2	4	50.0%
core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java	7	9	77.78%
core-api/src/main/java/com/optimizely/ab/config/parser/JsonHelpers.java	34	37	91.89%
core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java	45	53	84.91%

Totals
Change from base Build 1387:	-0.09%
Covered Lines:	3874
Relevant Lines:	4342

💛 - Coveralls

pawels-optimizely

getValue should not return anything, but it should modify the object passed in by reference.

Can you also make tests similar to the tests in go sdk ?
https://github.com/optimizely/go-sdk/blob/master/pkg/optimizelyjson/optimizely_json_test.go
Also please include the test to check if you don't destroy original object (it's also in my go tests)

core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java

mikecdavis

Overall looks fine, but there's a larger question around JSON serialization libraries that we need to address.

mikecdavis · 2020-04-23T18:15:45Z

core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java

+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;


We can't assume that Gson is available. I see two options:

We mimic the approach taken in DefaultConfigParser

Take a stance on the serializer and include as a shadow dependency

Option 1 is the safer approach, since it has some precedence, but adds SDK complexity.
Option 2 is preferred but has a larger impact to the customer (my main concern is mobile).

cc @mikeng13

pawels-optimizely · 2020-04-30T23:52:46Z

core-api/src/test/java/com/optimizely/ab/optimizelyjson/OptimizelyJSONExtendedTest.java

+        assertEquals((Double)md1.k3.kk2.kkk4[0], 5.7, 0.01);
+        assertEquals(md1.k3.kk2.kkk4[2], "vvv4");
+
+        // verify previous getValue does not destroy the data


the purpose of this test is to call getValue twice with non empty json path on the same object. That will check if we don't modify the object by traversing and returning just a subset of the object. Both getValue calls need to have at least the same root of the json path (or the same json path)

mikecdavis

I've requested some cleanup on the tests to be more inline with the testing framework best practices. We're also missing a lot of unit test coverage on the parsers directly.

mikecdavis · 2020-05-01T19:12:42Z

core-api/src/main/java/com/optimizely/ab/config/parser/ConfigParser.java

+    String toJson(Object src) throws ConfigParseException;
+    <T> T fromJson(String json, Class<T> clazz) throws ConfigParseException;


Should we create a new exception JsonParseException for these?

mikecdavis · 2020-05-01T19:14:56Z

core-api/src/main/java/com/optimizely/ab/config/parser/GsonConfigParser.java

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


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.

mikecdavis · 2020-05-01T19:19:41Z

core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java

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


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

mikecdavis · 2020-05-01T19:21:55Z

core-api/src/main/java/com/optimizely/ab/config/parser/UnsupportedOperationException.java

+ */
+package com.optimizely.ab.config.parser;
+
+public final class UnsupportedOperationException extends Exception {


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.

mikecdavis · 2020-05-01T19:29:40Z

core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java

+    @Nullable
+    public <T> T getValue(@Nullable String jsonKey, Class<T> clazz) throws UnsupportedOperationException {
+        if (!(parser instanceof GsonConfigParser || parser instanceof JacksonConfigParser)) {
+            throw new UnsupportedOperationException("A proper JSON parser is not available. Use Gson or Jackson parser for this operation.");


Per previous comment re: UnsupportedOperationException consider throwing a runtime exception or renaming to something like a JsonParserException.

mikecdavis · 2020-05-01T20:06:27Z

core-api/src/test/java/com/optimizely/ab/optimizelyjson/OptimizelyJSONExtendedTest.java

+
+import static org.junit.Assert.*;
+
+public class OptimizelyJSONExtendedTest extends OptimizelyJSONCoreTest {


Instead of Core vs Extended you can use assumeThat to conditionally ignore a parameterized variant.

mikecdavis · 2020-05-01T20:08:27Z

core-api/src/test/java/com/optimizely/ab/optimizelyjson/OptimizelyJSONWithGsonParserTest.java

+        return new GsonConfigParser();
+    }
+
+    // Tests for GSON only


I guess these classes are ok, but per the comments above, they should not be extending from OptimizelyJSONExtendedTest.

mikecdavis · 2020-05-01T20:12:46Z

core-api/src/test/java/com/optimizely/ab/optimizelyjson/types/MD1.java

+ */
+package com.optimizely.ab.optimizelyjson.types;
+
+public class MD1 {


It's not clear what these test classes are supposed to represent. It might be clearer if you subclass these under something like TestTypes.MD1 etc and add some doc strings.

mikecdavis · 2020-05-01T20:13:33Z

core-api/src/main/java/com/optimizely/ab/config/parser/JsonHelpers.java

+
+import java.util.*;
+
+final class JsonHelpers {


We should have a corresponding JsonHelpersTest class for unit tests.

mikecdavis · 2020-05-01T20:14:42Z

core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java

@@ -61,4 +63,23 @@ public ProjectConfigModule() {
            addDeserializer(Condition.class, new ConditionJacksonDeserializer(objectMapper));
        }
    }
+
+    @Override
+    public String toJson(Object src) throws ConfigParseException {


We're missing unit tests on these new implementations.

mikecdavis

Looks great!

pawels-optimizely

LGTM

jaeopt added 2 commits April 20, 2020 16:07

add OptimizelyJSON

032c270

clean up

99014d2

jaeopt requested review from thomaszurkan-optimizely and pawels-optimizely April 21, 2020 18:33

jaeopt requested a review from a team as a code owner April 21, 2020 18:33

jaeopt self-assigned this Apr 21, 2020

pawels-optimizely suggested changes Apr 22, 2020

View reviewed changes

core-api/src/main/java/com/optimizely/ab/optimizelyjson/OptimizelyJSON.java Show resolved Hide resolved

jaeopt added 2 commits April 22, 2020 13:55

add doc comments

9de2535

Merge branch 'master' into jae/optimizely-json

641d215

mikecdavis suggested changes Apr 23, 2020

View reviewed changes

jaeopt added 11 commits April 24, 2020 14:26

add more tests

69144a0

fix test

f319fdc

(wip) add multiple parser support to OptimizelyJSON

096bcb9

split getvalue tests

5be5f3f

disable getValue for legacy parsers

01970f5

add legacy parser tests

2401784

fix tests for all parsers

afdb8be

Merge branch 'master' into jae/optimizely-json

2952834

cleanup

24ed0d6

fix findbugs issues

5d45085

cleanup

2af6f14

jaeopt requested review from mikecdavis and pawels-optimizely April 30, 2020 23:09

pawels-optimizely reviewed Apr 30, 2020

View reviewed changes

fix test per review

b5db429

jaeopt requested a review from pawels-optimizely May 1, 2020 00:43

mikecdavis suggested changes May 1, 2020

View reviewed changes

jaeopt added 2 commits May 4, 2020 15:27

fix JsonSimple parser

b6fc66c

add unit tests for all parsers

ef7963b

jaeopt added 7 commits May 4, 2020 17:27

add unit tests for JsonHelpers

880111b

fix test types

14ace05

refact json parser tests

f0df4e2

cleanup parser tests

ceddf49

cleanup

37efc58

change to parameterized tests

8cd3b55

fix merge conflict

c92459f

mikecdavis approved these changes May 5, 2020

View reviewed changes

fix getValue destroy test

e5d792a

pawels-optimizely approved these changes May 5, 2020

View reviewed changes

jaeopt merged commit 8609f3d into master May 5, 2020

This was referenced Jul 2, 2020

(chore): prepare for release 3.5.0-beta #381

Merged

(chore): prepare for release 3.5.0 #383

Merged

jaeopt deleted the jae/optimizely-json branch October 22, 2020 21:07

		import com.google.gson.Gson;
		import com.google.gson.GsonBuilder;

		String toJson(Object src) throws ConfigParseException;
		<T> T fromJson(String json, Class<T> clazz) throws ConfigParseException;


		import static org.junit.Assert.*;

		public class OptimizelyJSONExtendedTest extends OptimizelyJSONCoreTest {

feature: add OptimizelyJSON #371

feature: add OptimizelyJSON #371

Uh oh!

Conversation

jaeopt commented Apr 21, 2020

Summary

Uh oh!

coveralls commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1389

💛 - Coveralls

Uh oh!

pawels-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawels-optimizely Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

pawels-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Apr 21, 2020 •

edited

Loading

pawels-optimizely Apr 30, 2020 •

edited

Loading