From fbdf1b5ceab17ac7d1a1e5ad15bfef3f99b5bd91 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Mon, 24 Sep 2018 10:49:16 -0700 Subject: [PATCH 1/4] Performance improvements for JacksonConfigParser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This addresses some poor use of Jackson when deserializing JSON to `ProjectConfig` object and improves parse performance by about 15x-40x based on some benchmarks. The custom deserializers were creating new `ObjectMapper` instances on each `deserialize()` invocation. Very wasteful. Eliminates several cases of unecessary round trips of reading JSON string to JSON tree, then back to JSON string to parse a child node. We can eliminate the `GroupJacksonDeserializer` completly and rely on standard Jackson databind annotations. Deserialize Audience conditions using `JsonNode` interface to eliminate intermediate mapping to `List` and `Map`. Adds `JacksonConfigParserBenchmark` (fixes Gradle JMH setup). Benchmark results: Before: Benchmark Mode Cnt Score Error Units JacksonConfigParserBenchmark.parseV2 avgt 40 1668.109 ± 178.586 us/op JacksonConfigParserBenchmark.parseV3 avgt 40 1574.464 ± 25.816 us/op JacksonConfigParserBenchmark.parseV4 avgt 40 2166.621 ± 91.664 us/op After: Benchmark Mode Cnt Score Error Units JacksonConfigParserBenchmark.parseV2 avgt 40 41.292 ± 1.219 us/op JacksonConfigParserBenchmark.parseV3 avgt 40 54.360 ± 0.790 us/op JacksonConfigParserBenchmark.parseV4 avgt 40 128.589 ± 3.728 us/op --- .editorconfig | 9 +++ build.gradle | 11 ++- .../parser/JacksonConfigParserBenchmark.java | 44 ++++++++++ .../com/optimizely/ab/config/Experiment.java | 11 +-- .../java/com/optimizely/ab/config/Group.java | 23 +++++- .../parser/AudienceJacksonDeserializer.java | 68 +++++++++------- .../parser/GroupJacksonDeserializer.java | 80 ------------------- .../ab/config/parser/JacksonConfigParser.java | 29 +++++-- .../ab/config/parser/JacksonHelpers.java | 31 +++++++ .../ProjectConfigJacksonDeserializer.java | 52 ++++-------- gradle/wrapper/gradle-wrapper.properties | 3 +- 11 files changed, 193 insertions(+), 168 deletions(-) create mode 100644 .editorconfig create mode 100644 core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java delete mode 100644 core-api/src/main/java/com/optimizely/ab/config/parser/GroupJacksonDeserializer.java create mode 100644 core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 000000000..30117ed02 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,9 @@ +# EditorConfig is awesome: http://EditorConfig.org + +# top-most EditorConfig file +root = true + +# 4 space indentation +[*.{py,java}] +indent_style = space +indent_size = 4 \ No newline at end of file diff --git a/build.gradle b/build.gradle index 605826e37..c8391fa7d 100644 --- a/build.gradle +++ b/build.gradle @@ -14,7 +14,7 @@ buildscript { plugins { id 'com.github.kt3k.coveralls' version '2.8.2' id 'jacoco' - id 'me.champeau.gradle.jmh' version '0.3.1' + id 'me.champeau.gradle.jmh' version '0.4.5' id 'nebula.optional-base' version '3.2.0' } @@ -79,6 +79,10 @@ subprojects { } } + findbugs { + findbugsJmh.enabled = false + } + test { useJUnit { excludeCategories 'com.optimizely.ab.categories.ExhaustiveTest' @@ -104,9 +108,8 @@ subprojects { } dependencies { - afterEvaluate { - jmh configurations.testCompile.allDependencies - } + jmh 'org.openjdk.jmh:jmh-core:1.12' + jmh 'org.openjdk.jmh:jmh-generator-annprocess:1.12' } dependencies { diff --git a/core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java b/core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java new file mode 100644 index 000000000..55de9753d --- /dev/null +++ b/core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java @@ -0,0 +1,44 @@ +package com.optimizely.ab.config.parser; + +import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.config.ProjectConfigTestUtils; +import org.openjdk.jmh.annotations.*; + +import java.io.IOException; +import java.util.concurrent.TimeUnit; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +@Fork(2) +@Warmup(iterations = 10) +@Measurement(iterations = 20) +@State(Scope.Benchmark) +public class JacksonConfigParserBenchmark { + JacksonConfigParser parser; + String jsonV2; + String jsonV3; + String jsonV4; + + @Setup + public void setUp() throws IOException { + parser = new JacksonConfigParser(); + jsonV2 = ProjectConfigTestUtils.validConfigJsonV2(); + jsonV3 = ProjectConfigTestUtils.validConfigJsonV3(); + jsonV4 = ProjectConfigTestUtils.validConfigJsonV4(); + } + + @Benchmark + public ProjectConfig parseV2() throws ConfigParseException { + return parser.parseProjectConfig(jsonV2); + } + + @Benchmark + public ProjectConfig parseV3() throws ConfigParseException { + return parser.parseProjectConfig(jsonV3); + } + + @Benchmark + public ProjectConfig parseV4() throws ConfigParseException { + return parser.parseProjectConfig(jsonV4); + } +} diff --git a/core-api/src/main/java/com/optimizely/ab/config/Experiment.java b/core-api/src/main/java/com/optimizely/ab/config/Experiment.java index f82e78903..3441649f2 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/Experiment.java +++ b/core-api/src/main/java/com/optimizely/ab/config/Experiment.java @@ -16,17 +16,14 @@ */ package com.optimizely.ab.config; -import com.fasterxml.jackson.annotation.JsonCreator; -import com.fasterxml.jackson.annotation.JsonIgnoreProperties; -import com.fasterxml.jackson.annotation.JsonProperty; - -import java.util.Collections; -import java.util.List; -import java.util.Map; +import com.fasterxml.jackson.annotation.*; import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; +import java.util.Collections; +import java.util.List; +import java.util.Map; /** * Represents the Optimizely Experiment configuration. diff --git a/core-api/src/main/java/com/optimizely/ab/config/Group.java b/core-api/src/main/java/com/optimizely/ab/config/Group.java index cd41bc120..74017af27 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/Group.java +++ b/core-api/src/main/java/com/optimizely/ab/config/Group.java @@ -20,9 +20,9 @@ import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonProperty; -import java.util.List; - import javax.annotation.concurrent.Immutable; +import java.util.ArrayList; +import java.util.List; /** * Represents a Optimizely Group configuration @@ -48,7 +48,24 @@ public Group(@JsonProperty("id") String id, this.id = id; this.policy = policy; this.trafficAllocation = trafficAllocation; - this.experiments = experiments; + // populate experiment's groupId + this.experiments = new ArrayList<>(experiments.size()); + for (Experiment experiment : experiments) { + if (id != null && !id.equals(experiment.getGroupId())) { + experiment = new Experiment( + experiment.getId(), + experiment.getKey(), + experiment.getStatus(), + experiment.getLayerId(), + experiment.getAudienceIds(), + experiment.getVariations(), + experiment.getUserIdToVariationKeyMap(), + experiment.getTrafficAllocation(), + id + ); + } + this.experiments.add(experiment); + } } public String getId() { diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java b/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java index 1d0efc3e7..bf9cf1757 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java @@ -17,61 +17,71 @@ package com.optimizely.ab.config.parser; import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.ObjectCodec; import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JsonDeserializer; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; - -import com.optimizely.ab.config.audience.Audience; -import com.optimizely.ab.config.audience.AndCondition; -import com.optimizely.ab.config.audience.Condition; -import com.optimizely.ab.config.audience.UserAttribute; -import com.optimizely.ab.config.audience.NotCondition; -import com.optimizely.ab.config.audience.OrCondition; +import com.optimizely.ab.config.audience.*; import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; public class AudienceJacksonDeserializer extends JsonDeserializer { + private ObjectMapper objectMapper; + + public AudienceJacksonDeserializer() { + this(new ObjectMapper()); + } + + AudienceJacksonDeserializer(ObjectMapper objectMapper) { + this.objectMapper = objectMapper; + } @Override public Audience deserialize(JsonParser parser, DeserializationContext context) throws IOException { - ObjectMapper mapper = new ObjectMapper(); - JsonNode node = parser.getCodec().readTree(parser); + ObjectCodec codec = parser.getCodec(); + JsonNode node = codec.readTree(parser); String id = node.get("id").textValue(); String name = node.get("name").textValue(); - List rawObjectList = (List)mapper.readValue(node.get("conditions").textValue(), List.class); - Condition conditions = parseConditions(rawObjectList); + + String conditionsJson = node.get("conditions").textValue(); + JsonNode conditionsTree = objectMapper.readTree(conditionsJson); + Condition conditions = parseConditions(conditionsTree); return new Audience(id, name, conditions); } - private Condition parseConditions(List rawObjectList) { + private Condition parseConditions(JsonNode conditionNode) { List conditions = new ArrayList(); - String operand = (String)rawObjectList.get(0); + JsonNode opNode = conditionNode.get(0); + String operand = opNode.asText(); - for (int i = 1; i < rawObjectList.size(); i++) { - Object obj = rawObjectList.get(i); - if (obj instanceof List) { - List objectList = (List)rawObjectList.get(i); - conditions.add(parseConditions(objectList)); - } else { - HashMap conditionMap = (HashMap)rawObjectList.get(i); - conditions.add(new UserAttribute(conditionMap.get("name"), conditionMap.get("type"), - conditionMap.get("value"))); + for (int i = 1; i < conditionNode.size(); i++) { + JsonNode subNode = conditionNode.get(i); + if (subNode.isArray()) { + conditions.add(parseConditions(subNode)); + } else if (subNode.isObject()) { + conditions.add(new UserAttribute( + subNode.get("name").textValue(), + subNode.get("type").textValue(), + subNode.path("value").asText(null))); } } Condition condition; - if (operand.equals("and")) { - condition = new AndCondition(conditions); - } else if (operand.equals("or")) { - condition = new OrCondition(conditions); - } else { - condition = new NotCondition(conditions.get(0)); + switch (operand) { + case "and": + condition = new AndCondition(conditions); + break; + case "or": + condition = new OrCondition(conditions); + break; + default: // this makes two assumptions: operator is "not" and conditions is non-empty... + condition = new NotCondition(conditions.get(0)); + break; } return condition; diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/GroupJacksonDeserializer.java b/core-api/src/main/java/com/optimizely/ab/config/parser/GroupJacksonDeserializer.java deleted file mode 100644 index 714326fcc..000000000 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/GroupJacksonDeserializer.java +++ /dev/null @@ -1,80 +0,0 @@ -/** - * - * Copyright 2016-2017, 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 com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.core.type.TypeReference; -import com.fasterxml.jackson.databind.DeserializationContext; -import com.fasterxml.jackson.databind.JsonDeserializer; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; - -import com.optimizely.ab.config.Experiment; -import com.optimizely.ab.config.Group; -import com.optimizely.ab.config.TrafficAllocation; -import com.optimizely.ab.config.Variation; - -import java.io.IOException; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; - -public class GroupJacksonDeserializer extends JsonDeserializer { - - @Override - public Group deserialize(JsonParser parser, DeserializationContext context) throws IOException { - ObjectMapper mapper = new ObjectMapper(); - JsonNode node = parser.getCodec().readTree(parser); - - String id = node.get("id").textValue(); - String policy = node.get("policy").textValue(); - List trafficAllocations = mapper.readValue(node.get("trafficAllocation").toString(), - new TypeReference>(){}); - - JsonNode groupExperimentsJson = node.get("experiments"); - List groupExperiments = new ArrayList(); - if (groupExperimentsJson.isArray()) { - for (JsonNode groupExperimentJson : groupExperimentsJson) { - groupExperiments.add(parseExperiment(groupExperimentJson, id)); - } - } - - return new Group(id, policy, groupExperiments, trafficAllocations); - } - - private Experiment parseExperiment(JsonNode experimentJson, String groupId) throws IOException { - ObjectMapper mapper = new ObjectMapper(); - - String id = experimentJson.get("id").textValue(); - String key = experimentJson.get("key").textValue(); - String status = experimentJson.get("status").textValue(); - JsonNode layerIdJson = experimentJson.get("layerId"); - String layerId = layerIdJson == null ? null : layerIdJson.textValue(); - List audienceIds = mapper.readValue(experimentJson.get("audienceIds").toString(), - new TypeReference>(){}); - List variations = mapper.readValue(experimentJson.get("variations").toString(), - new TypeReference>(){}); - List trafficAllocations = mapper.readValue(experimentJson.get("trafficAllocation").toString(), - new TypeReference>(){}); - Map userIdToVariationKeyMap = mapper.readValue( - experimentJson.get("forcedVariations").toString(), new TypeReference>(){}); - - return new Experiment(id, key, status, layerId, audienceIds, variations, userIdToVariationKeyMap, - trafficAllocations, groupId); - } - -} diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java index 67ab86771..3bcbde2cf 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java @@ -18,8 +18,8 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.module.SimpleModule; - import com.optimizely.ab.config.ProjectConfig; +import com.optimizely.ab.config.audience.Audience; import javax.annotation.Nonnull; @@ -27,18 +27,33 @@ * {@code Jackson}-based config parser implementation. */ final class JacksonConfigParser implements ConfigParser { + private ObjectMapper objectMapper; + + public JacksonConfigParser() { + this(new ObjectMapper()); + } + + JacksonConfigParser(ObjectMapper objectMapper) { + this.objectMapper = objectMapper; + this.objectMapper.registerModule(new ProjectConfigModule()); + } @Override public ProjectConfig parseProjectConfig(@Nonnull String json) throws ConfigParseException { - ObjectMapper mapper = new ObjectMapper(); - SimpleModule module = new SimpleModule(); - module.addDeserializer(ProjectConfig.class, new ProjectConfigJacksonDeserializer()); - mapper.registerModule(module); - try { - return mapper.readValue(json, ProjectConfig.class); + return objectMapper.readValue(json, ProjectConfig.class); } catch (Exception e) { throw new ConfigParseException("Unable to parse datafile: " + json, e); } } + + class ProjectConfigModule extends SimpleModule { + private final static String NAME = "ProjectConfigModule"; + + public ProjectConfigModule() { + super(NAME); + addDeserializer(ProjectConfig.class, new ProjectConfigJacksonDeserializer()); + addDeserializer(Audience.class, new AudienceJacksonDeserializer(objectMapper)); + } + } } diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java new file mode 100644 index 000000000..9199574c1 --- /dev/null +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java @@ -0,0 +1,31 @@ +package com.optimizely.ab.config.parser; + +import com.fasterxml.jackson.core.ObjectCodec; +import com.fasterxml.jackson.databind.JsonNode; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +final class JacksonHelpers { + private JacksonHelpers() { + } + + static List arrayNodeToList(JsonNode arrayNode, Class itemClass, ObjectCodec codec) throws IOException { + if (arrayNode == null || arrayNode.isNull() || !arrayNode.isArray()) { + return null; + } + + List items = new ArrayList<>(arrayNode.size()); + + for (int i = 0; i < arrayNode.size(); i++) { + JsonNode itemNode = arrayNode.get(i); + if (itemNode.isNull()) { + continue; + } + items.add(codec.treeToValue(itemNode, itemClass)); + } + + return items; + } +} diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigJacksonDeserializer.java b/core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigJacksonDeserializer.java index 76cac7412..3a9ba8f52 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigJacksonDeserializer.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigJacksonDeserializer.java @@ -17,36 +17,21 @@ package com.optimizely.ab.config.parser; import com.fasterxml.jackson.core.JsonParser; -import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.core.ObjectCodec; import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JsonDeserializer; import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.module.SimpleModule; -import com.optimizely.ab.config.Attribute; -import com.optimizely.ab.config.EventType; -import com.optimizely.ab.config.Experiment; -import com.optimizely.ab.config.FeatureFlag; -import com.optimizely.ab.config.Group; -import com.optimizely.ab.config.LiveVariable; -import com.optimizely.ab.config.ProjectConfig; -import com.optimizely.ab.config.Rollout; +import com.optimizely.ab.config.*; import com.optimizely.ab.config.audience.Audience; import java.io.IOException; import java.util.List; class ProjectConfigJacksonDeserializer extends JsonDeserializer { - @Override public ProjectConfig deserialize(JsonParser parser, DeserializationContext context) throws IOException { - ObjectMapper mapper = new ObjectMapper(); - SimpleModule module = new SimpleModule(); - module.addDeserializer(Audience.class, new AudienceJacksonDeserializer()); - module.addDeserializer(Group.class, new GroupJacksonDeserializer()); - mapper.registerModule(module); - - JsonNode node = parser.getCodec().readTree(parser); + ObjectCodec codec = parser.getCodec(); + JsonNode node = codec.readTree(parser); String accountId = node.get("accountId").textValue(); String projectId = node.get("projectId").textValue(); @@ -54,23 +39,16 @@ public ProjectConfig deserialize(JsonParser parser, DeserializationContext conte String version = node.get("version").textValue(); int datafileVersion = Integer.parseInt(version); - List groups = mapper.readValue(node.get("groups").toString(), new TypeReference>() {}); - List experiments = mapper.readValue(node.get("experiments").toString(), - new TypeReference>() {}); - - List attributes; - attributes = mapper.readValue(node.get("attributes").toString(), new TypeReference>() {}); - - List events = mapper.readValue(node.get("events").toString(), - new TypeReference>() {}); - List audiences = mapper.readValue(node.get("audiences").toString(), - new TypeReference>() {}); + List groups = JacksonHelpers.arrayNodeToList(node.get("groups"), Group.class, codec); + List experiments = JacksonHelpers.arrayNodeToList(node.get("experiments"), Experiment.class, codec); + List attributes = JacksonHelpers.arrayNodeToList(node.get("attributes"), Attribute.class, codec); + List events = JacksonHelpers.arrayNodeToList(node.get("events"), EventType.class, codec); + List audiences = JacksonHelpers.arrayNodeToList(node.get("audiences"), Audience.class, codec); boolean anonymizeIP = false; List liveVariables = null; if (datafileVersion >= Integer.parseInt(ProjectConfig.Version.V3.toString())) { - liveVariables = mapper.readValue(node.get("variables").toString(), - new TypeReference>() {}); + liveVariables = JacksonHelpers.arrayNodeToList(node.get("variables"), LiveVariable.class, codec); anonymizeIP = node.get("anonymizeIP").asBoolean(); } @@ -78,12 +56,11 @@ public ProjectConfig deserialize(JsonParser parser, DeserializationContext conte List rollouts = null; Boolean botFiltering = null; if (datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())) { - featureFlags = mapper.readValue(node.get("featureFlags").toString(), - new TypeReference>() {}); - rollouts = mapper.readValue(node.get("rollouts").toString(), - new TypeReference>(){}); - if (node.hasNonNull("botFiltering")) + featureFlags = JacksonHelpers.arrayNodeToList(node.get("featureFlags"), FeatureFlag.class, codec); + rollouts = JacksonHelpers.arrayNodeToList(node.get("rollouts"), Rollout.class, codec); + if (node.hasNonNull("botFiltering")) { botFiltering = node.get("botFiltering").asBoolean(); + } } return new ProjectConfig( @@ -103,4 +80,5 @@ public ProjectConfig deserialize(JsonParser parser, DeserializationContext conte rollouts ); } + } \ No newline at end of file diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index 568c50bf3..d75ee9126 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,5 +1,6 @@ +#Mon Sep 24 09:56:45 PDT 2018 distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-4.5.1-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-4.5.1-all.zip From 6088dc00eda36e2a9cf2659b2aff98bfae980750 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Fri, 5 Oct 2018 12:02:38 -0700 Subject: [PATCH 2/4] Copyright headers --- .../parser/JacksonConfigParserBenchmark.java | 16 ++++++++++++++++ .../com/optimizely/ab/config/Experiment.java | 2 +- .../java/com/optimizely/ab/config/Group.java | 2 +- .../config/parser/AudienceGsonDeserializer.java | 2 +- .../ab/config/parser/JacksonConfigParser.java | 2 +- .../ab/config/parser/JacksonHelpers.java | 16 ++++++++++++++++ 6 files changed, 36 insertions(+), 4 deletions(-) diff --git a/core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java b/core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java index 55de9753d..e66a38efd 100644 --- a/core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java +++ b/core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java @@ -1,3 +1,19 @@ +/** + * + * Copyright 2018, 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 com.optimizely.ab.config.ProjectConfig; diff --git a/core-api/src/main/java/com/optimizely/ab/config/Experiment.java b/core-api/src/main/java/com/optimizely/ab/config/Experiment.java index 3441649f2..3b6fe61ba 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/Experiment.java +++ b/core-api/src/main/java/com/optimizely/ab/config/Experiment.java @@ -1,6 +1,6 @@ /** * - * Copyright 2016-2017, Optimizely and contributors + * Copyright 2016-2018, 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. diff --git a/core-api/src/main/java/com/optimizely/ab/config/Group.java b/core-api/src/main/java/com/optimizely/ab/config/Group.java index 74017af27..0192cc79e 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/Group.java +++ b/core-api/src/main/java/com/optimizely/ab/config/Group.java @@ -1,6 +1,6 @@ /** * - * Copyright 2016-2017, Optimizely and contributors + * Copyright 2016-2018, 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. diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceGsonDeserializer.java b/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceGsonDeserializer.java index 8158aeb4b..ef82e3509 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceGsonDeserializer.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceGsonDeserializer.java @@ -1,6 +1,6 @@ /** * - * Copyright 2016-2017, Optimizely and contributors + * Copyright 2016-2018, 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. diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java index 3bcbde2cf..21c51dfde 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonConfigParser.java @@ -1,6 +1,6 @@ /** * - * Copyright 2016-2017, Optimizely and contributors + * Copyright 2016-2018, 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. diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java index 9199574c1..5b1bd84b4 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java @@ -1,3 +1,19 @@ +/** + * + * Copyright 2018, 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 com.fasterxml.jackson.core.ObjectCodec; From 2689e8fe63d64dc5f08dce34dfb3a04c8a12b59f Mon Sep 17 00:00:00 2001 From: loganlinn Date: Fri, 5 Oct 2018 18:36:41 -0700 Subject: [PATCH 3/4] Polymorphic UserAttribute value parsing --- .../ab/config/audience/UserAttribute.java | 32 ++++++++++++++----- .../parser/AudienceJacksonDeserializer.java | 9 ++---- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java index 6bb457859..58fc8d876 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java @@ -16,6 +16,7 @@ */ package com.optimizely.ab.config.audience; +import com.optimizely.ab.annotations.VisibleForTesting; import com.optimizely.ab.config.audience.match.MatchType; import javax.annotation.Nonnull; @@ -29,10 +30,14 @@ @Immutable public class UserAttribute implements Condition { - private final String name; - private final String type; - private final String match; - private final Object value; + private String name; + private String type; + private String match; + private Object value; + + @VisibleForTesting + UserAttribute() { + } public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String match, @Nullable Object value) { this.name = name; @@ -58,11 +63,11 @@ public Object getValue() { } public @Nullable Boolean evaluate(Map attributes) { - // Valid for primative types, but needs to change when a value is an object or an array + // Valid for primitive types, but needs to change when a value is an object or an array Object userAttributeValue = attributes.get(name); if (!"custom_attribute".equals(type)) { - MatchType.logger.error(String.format("condition type not equal to `custom_attribute` %s", type != null ? type : "")); + MatchType.logger.error(String.format("condition type not equal to `custom_attribute` %s", type)); return null; // unknown type } // check user attribute value is equal @@ -73,14 +78,22 @@ public Object getValue() { MatchType.logger.error(String.format("attribute or value null for match %s", match != null ? match : "legacy condition"),np); return null; } - } @Override public String toString() { + final String valueStr; + if (value == null) { + valueStr = "null"; + } else if (value instanceof String) { + valueStr = String.format("'%s'", value); + } else { + valueStr = value.toString(); + } return "{name='" + name + "\'" + ", type='" + type + "\'" + - ", value='" + value.toString() + "\'" + + ", match='" + match + "\'" + + ", value=" + valueStr + "}"; } @@ -93,6 +106,8 @@ public boolean equals(Object o) { if (!name.equals(that.name)) return false; if (!type.equals(that.type)) return false; + //noinspection StringEquality + if (!(match == that.match || match != null && match.equals(that.match))) return false; return value != null ? value.equals(that.value) : that.value == null; } @@ -100,6 +115,7 @@ public boolean equals(Object o) { public int hashCode() { int result = name.hashCode(); result = 31 * result + type.hashCode(); + result = 31 * result + (match != null ? match.hashCode() : 0); result = 31 * result + (value != null ? value.hashCode() : 0); return result; } diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java b/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java index d47f922fa..54a9c6d4a 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java @@ -17,6 +17,7 @@ package com.optimizely.ab.config.parser; import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.ObjectCodec; import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JsonDeserializer; @@ -54,7 +55,7 @@ public Audience deserialize(JsonParser parser, DeserializationContext context) t return new Audience(id, name, conditions); } - private Condition parseConditions(JsonNode conditionNode) { + private Condition parseConditions(JsonNode conditionNode) throws JsonProcessingException { List conditions = new ArrayList(); JsonNode opNode = conditionNode.get(0); String operand = opNode.asText(); @@ -64,11 +65,7 @@ private Condition parseConditions(JsonNode conditionNode) { if (subNode.isArray()) { conditions.add(parseConditions(subNode)); } else if (subNode.isObject()) { - conditions.add(new UserAttribute( - subNode.get("name").textValue(), - subNode.get("type").textValue(), - subNode.path("match").asText(null), - subNode.path("value").asText(null))); + conditions.add(objectMapper.treeToValue(subNode, UserAttribute.class)); } } From d3517740af46c6a164e7345d400ef2b531d68d23 Mon Sep 17 00:00:00 2001 From: loganlinn Date: Tue, 9 Oct 2018 14:31:41 -0700 Subject: [PATCH 4/4] Fix FindBugs warnings --- .../ab/config/audience/UserAttribute.java | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java index 58fc8d876..2c03395ee 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java +++ b/core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java @@ -16,7 +16,9 @@ */ package com.optimizely.ab.config.audience; -import com.optimizely.ab.annotations.VisibleForTesting; +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; import com.optimizely.ab.config.audience.match.MatchType; import javax.annotation.Nonnull; @@ -28,18 +30,19 @@ * Represents a user attribute instance within an audience's conditions. */ @Immutable +@JsonIgnoreProperties(ignoreUnknown = true) public class UserAttribute implements Condition { - private String name; - private String type; - private String match; - private Object value; + private final String name; + private final String type; + private final String match; + private final Object value; - @VisibleForTesting - UserAttribute() { - } - - public UserAttribute(@Nonnull String name, @Nonnull String type, @Nullable String match, @Nullable Object value) { + @JsonCreator + public UserAttribute(@JsonProperty("name") @Nonnull String name, + @JsonProperty("type") @Nonnull String type, + @JsonProperty("match") @Nullable String match, + @JsonProperty("value") @Nullable Object value) { this.name = name; this.type = type; this.match = match; @@ -106,8 +109,7 @@ public boolean equals(Object o) { if (!name.equals(that.name)) return false; if (!type.equals(that.type)) return false; - //noinspection StringEquality - if (!(match == that.match || match != null && match.equals(that.match))) return false; + if (match != null ? !match.equals(that.match) : that.match != null) return false; return value != null ? value.equals(that.value) : that.value == null; }