diff --git a/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModel.java b/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModel.java index 1e919cfe..403fb46a 100644 --- a/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModel.java +++ b/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModel.java @@ -365,11 +365,11 @@ static final class Clause { private final ContextKind contextKind; private final AttributeRef attribute; private final Operator op; - private final List values; //interpreted as an OR of values + private List values; // OR semantics. The preprocessor can replace it via releaseValues(). private final boolean negate; - + transient ClausePreprocessed preprocessed; - + Clause(ContextKind contextKind, AttributeRef attribute, Operator op, List values, boolean negate) { this.contextKind = contextKind; this.attribute = attribute; @@ -377,24 +377,30 @@ static final class Clause { this.values = values == null ? emptyList() : values; this.negate = negate; } - + ContextKind getContextKind() { return contextKind; } - + AttributeRef getAttribute() { return attribute; } - + Operator getOp() { return op; } - + // Guaranteed non-null List getValues() { return values; } - + + // The preprocessor calls this when the preprocessed value set holds the same values. The set's + // cached list view replaces the original list, which frees the memory of the duplicate list. + void releaseValues(List valuesFromSet) { + values = valuesFromSet; + } + boolean isNegate() { return negate; } diff --git a/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModelPreprocessing.java b/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModelPreprocessing.java index e24a07d7..2a73685c 100644 --- a/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModelPreprocessing.java +++ b/lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/DataModelPreprocessing.java @@ -40,6 +40,37 @@ */ abstract class DataModelPreprocessing { private DataModelPreprocessing() {} + + /** + * The name of the system property that turns on the release of "in" clause value lists. + *

+ * When the property is "true", the preprocessor replaces the value list of each "in" clause that + * has a preprocessed value set with the set's list view. This frees the memory of the duplicate + * list. The view keeps the values in first-appearance order, so a released clause serializes its + * values in the original order, but without duplicate values. Equal numbers in different forms, + * such as 1 and 1.0, count as duplicates. + *

+ * The SDK reads the property once, at class load. The default is off. With the default, a + * clause serializes its values exactly as received. + */ + static final String RELEASE_CLAUSE_VALUES_PROPERTY = "com.launchdarkly.sdk.server.releaseClauseValues"; + + private static volatile boolean releaseClauseValues = readReleaseClauseValues(); + + // A security policy can deny reads of non-standard system properties. + // A denied read means the feature stays off. + private static boolean readReleaseClauseValues() { + try { + return Boolean.getBoolean(RELEASE_CLAUSE_VALUES_PROPERTY); + } catch (SecurityException e) { + return false; + } + } + + // For tests. Production code reads the system property once, at class load. + static void setReleaseClauseValues(boolean enabled) { + releaseClauseValues = enabled; + } static final class EvalResultsForSingleVariation { private final EvalResult regularResult; @@ -220,9 +251,16 @@ static void preprocessClause(Clause c) { // This is a special case where the clause is testing for an exact match against any of the // clause values. Converting the value list to a Set allows us to do a fast lookup instead of // a linear search. We do not do this for other operators (or if there are fewer than two - // values) because the slight extra overhead of a Set is not worthwhile in those case. + // values) because the slight extra overhead of a Set is not worthwhile in those cases. if (values.size() > 1) { - c.preprocessed = new ClausePreprocessed(ImmutableSet.copyOf(values), null); + ImmutableSet valuesSet = ImmutableSet.copyOf(values); + c.preprocessed = new ClausePreprocessed(valuesSet, null); + if (releaseClauseValues) { + // Replace the list with the set's cached list view, which frees the duplicate list. + // Other operators always keep their list, because they read values by position or as + // segment keys. + c.releaseValues(valuesSet.asList()); + } } } else if (op == matches) { c.preprocessed = preprocessClauseValues(c.getValues(), v -> diff --git a/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelPreprocessingTest.java b/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelPreprocessingTest.java index 0fb0fc9c..b018833b 100644 --- a/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelPreprocessingTest.java +++ b/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelPreprocessingTest.java @@ -15,6 +15,7 @@ import com.launchdarkly.sdk.server.DataModel.Target; import com.launchdarkly.sdk.server.DataModelPreprocessing.ClausePreprocessed; +import org.junit.After; import org.junit.Test; import java.time.Instant; @@ -274,6 +275,88 @@ public void preprocessFlagDoesNotCreateClauseValuesMapForNonEqualityOperators() } } + @Test + public void preprocessFlagKeepsClauseValuesListByDefault() { + Clause c = clause("x", Operator.in, LDValue.of("a"), LDValue.of(0), LDValue.of("a")); + + FeatureFlag f = flagFromClause(c); + f.afterDeserialized(); + + Clause pc = f.getRules().get(0).getClauses().get(0); + assertNotNull(pc.preprocessed.valuesSet); + assertEquals(ImmutableList.of(LDValue.of("a"), LDValue.of(0), LDValue.of("a")), pc.getValues()); + } + + @After + public void restoreReleaseClauseValues() { + DataModelPreprocessing.setReleaseClauseValues( + Boolean.getBoolean(DataModelPreprocessing.RELEASE_CLAUSE_VALUES_PROPERTY)); + } + + @Test + public void preprocessFlagReleasesClauseValuesListWhenReleaseIsEnabled() { + DataModelPreprocessing.setReleaseClauseValues(true); + + Clause c = clause("x", Operator.in, LDValue.of("a"), LDValue.of(0), LDValue.of("a")); + + FeatureFlag f = flagFromClause(c); + f.afterDeserialized(); + + Clause pc = f.getRules().get(0).getClauses().get(0); + assertEquals(ImmutableSet.of(LDValue.of("a"), LDValue.of(0)), pc.preprocessed.valuesSet); + // the set's list view replaces the list, so the duplicate value is gone + assertEquals(ImmutableList.of(LDValue.of("a"), LDValue.of(0)), pc.getValues()); + } + + @Test + public void releaseKeepsClauseValuesListWhenThereIsNoValueSet() { + DataModelPreprocessing.setReleaseClauseValues(true); + + Clause c = clause("x", Operator.in, LDValue.of("a")); + + FeatureFlag f = flagFromClause(c); + f.afterDeserialized(); + + Clause pc = f.getRules().get(0).getClauses().get(0); + assertNull(pc.preprocessed); + assertEquals(ImmutableList.of(LDValue.of("a")), pc.getValues()); + } + + @Test + public void releaseKeepsClauseValuesListForNonEqualityOperators() { + // Evaluation and dependency analysis read the list directly for every operator other than + // "in" (this includes segmentMatch), so the release must not touch these clauses. + DataModelPreprocessing.setReleaseClauseValues(true); + + for (Operator op: Operator.getBuiltins()) { + if (op == Operator.in) { + continue; + } + Clause c = clause("x", op, LDValue.of("a"), LDValue.of("b")); + + FeatureFlag f = flagFromClause(c); + f.afterDeserialized(); + + Clause pc = f.getRules().get(0).getClauses().get(0); + assertEquals(op.toString(), ImmutableList.of(LDValue.of("a"), LDValue.of("b")), pc.getValues()); + } + } + + @Test + public void releaseAppliesToSegmentRuleClauses() { + DataModelPreprocessing.setReleaseClauseValues(true); + + Clause c = clause("x", Operator.in, LDValue.of("a"), LDValue.of("b"), LDValue.of("a")); + SegmentRule rule = segmentRuleBuilder().clauses(c).build(); + Segment s = segmentBuilder("key").disablePreprocessing(true).rules(rule).build(); + + s.afterDeserialized(); + + Clause pc = s.getRules().get(0).getClauses().get(0); + assertEquals(ImmutableList.of(LDValue.of("a"), LDValue.of("b")), pc.getValues()); + assertEquals(ImmutableSet.of(LDValue.of("a"), LDValue.of("b")), pc.preprocessed.valuesSet); + } + @Test public void preprocessFlagParsesClauseDate() { String time1Str = "2016-04-16T17:09:12-07:00"; diff --git a/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelSerializationTest.java b/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelSerializationTest.java index c2a56ec9..342f1e39 100644 --- a/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelSerializationTest.java +++ b/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/DataModelSerializationTest.java @@ -24,6 +24,7 @@ import com.launchdarkly.sdk.server.subsystems.DataStoreTypes.ItemDescriptor; import com.launchdarkly.sdk.server.subsystems.SerializationException; +import org.junit.After; import org.junit.Test; import java.util.Collections; @@ -611,6 +612,108 @@ public void parsingFullDataSetFlagsAndSegments() throws Exception { .addAny(FEATURES, flag1, flag2).addAny(SEGMENTS, segment1, segment2).build(), allData); } + @After + public void restoreReleaseClauseValues() { + DataModelPreprocessing.setReleaseClauseValues( + Boolean.getBoolean(DataModelPreprocessing.RELEASE_CLAUSE_VALUES_PROPERTY)); + } + + @Test + public void clauseWithReleasedValuesListIsSerializedFromValueSet() { + // With the release enabled, an "in" clause with two or more values replaces its value list + // with the preprocessed set's list view. The view keeps first-appearance order, so the + // serialized output keeps the original order and drops only the duplicate value. + DataModelPreprocessing.setReleaseClauseValues(true); + + LDValue clauseJson = LDValue.buildObject() + .put("attribute", "name") + .put("op", "in") + .put("values", LDValue.buildArray() + .add("a").add("b").add(1).add(true).add("a") + .build()) + .put("negate", false) + .build(); + assertClauseFromJson(clauseJson, c -> { + // the release dropped the duplicate "a", so 4 of the 5 input values remain + assertEquals(4, c.getValues().size()); + }); + + LDValue flagJson = baseBuilder("flag-key") + .put("rules", LDValue.buildArray() + .add(LDValue.buildObject() + .put("id", "ruleid") + .put("clauses", LDValue.arrayOf(clauseJson)) + .build()) + .build()) + .build(); + assertFlagFromJson(flagJson, flag -> { + String json = FEATURES.serialize(new ItemDescriptor(flag.getVersion(), flag)); + LDValue values = LDValue.parse(json) + .get("rules").get(0).get("clauses").get(0).get("values"); + assertEquals(LDValue.buildArray().add("a").add("b").add(1).add(true).build(), values); + + // Parsing the released JSON gives the same values again. + assertFlagFromJson(LDValue.parse(json), flag2 -> { + String json2 = FEATURES.serialize(new ItemDescriptor(flag2.getVersion(), flag2)); + assertJsonEquals(json, json2); + }); + }); + } + + @Test + public void segmentClauseWithReleasedValuesListIsSerializedFromValueSet() { + DataModelPreprocessing.setReleaseClauseValues(true); + + LDValue segmentJson = baseBuilder("segment-key") + .put("rules", LDValue.buildArray() + .add(LDValue.buildObject() + .put("clauses", LDValue.buildArray() + .add(LDValue.buildObject() + .put("attribute", "name") + .put("op", "in") + .put("values", LDValue.buildArray().add("a").add("b").add("a").build()) + .put("negate", false) + .build()) + .build()) + .build()) + .build()) + .build(); + assertSegmentFromJson(segmentJson, segment -> { + // the release dropped the duplicate "a", so 2 of the 3 input values remain + assertEquals(2, segment.getRules().get(0).getClauses().get(0).getValues().size()); + + String json = SEGMENTS.serialize(new ItemDescriptor(segment.getVersion(), segment)); + LDValue values = LDValue.parse(json) + .get("rules").get(0).get("clauses").get(0).get("values"); + assertEquals(LDValue.buildArray().add("a").add("b").build(), values); + }); + } + + @Test + public void clauseValuesAreSerializedUnchangedByDefault() { + LDValue clauseJson = LDValue.buildObject() + .put("attribute", "name") + .put("op", "in") + .put("values", LDValue.buildArray().add("a").add("b").add("a").build()) + .put("negate", false) + .build(); + LDValue flagJson = baseBuilder("flag-key") + .put("rules", LDValue.buildArray() + .add(LDValue.buildObject() + .put("id", "ruleid") + .put("clauses", LDValue.arrayOf(clauseJson)) + .build()) + .build()) + .build(); + assertFlagFromJson(flagJson, flag -> { + String json = FEATURES.serialize(new ItemDescriptor(flag.getVersion(), flag)); + LDValue values = LDValue.parse(json) + .get("rules").get(0).get("clauses").get(0).get("values"); + // serialization keeps the duplicate value and the original order + assertEquals(LDValue.buildArray().add("a").add("b").add("a").build(), values); + }); + } + private void assertFlagFromJson(LDValue flagJson, Consumer action) { FeatureFlag flag = (FeatureFlag)FEATURES.deserialize(flagJson.toJsonString()).getItem(); action.accept(flag); diff --git a/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/EvaluatorClauseTest.java b/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/EvaluatorClauseTest.java index 4c1317f5..0f2a374d 100644 --- a/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/EvaluatorClauseTest.java +++ b/lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/EvaluatorClauseTest.java @@ -12,6 +12,7 @@ import com.launchdarkly.sdk.server.DataModel.Rule; import com.launchdarkly.sdk.server.DataModel.Segment; +import org.junit.After; import org.junit.Test; import static com.launchdarkly.sdk.EvaluationDetail.fromValue; @@ -301,4 +302,42 @@ public void testSegmentMatchClauseIgnoresNonStringValues() throws Exception { Evaluator e = evaluatorBuilder().withStoredSegments(segment).build(); assertMatch(e, flag, context, true); } + + @After + public void restoreReleaseClauseValues() { + DataModelPreprocessing.setReleaseClauseValues( + Boolean.getBoolean(DataModelPreprocessing.RELEASE_CLAUSE_VALUES_PROPERTY)); + } + + @Test + public void clauseCanMatchWhenValuesListIsReleased() throws Exception { + DataModelPreprocessing.setReleaseClauseValues(true); + + Clause clause = clause("name", Operator.in, LDValue.of("Bob"), LDValue.of("Carol"), LDValue.of("Bob")); + FeatureFlag f = booleanFlagWithClauses("flag", clause); + // the release replaced the list with the set's view, which drops the duplicate "Bob" + assertEquals(2, f.getRules().get(0).getClauses().get(0).getValues().size()); + + assertMatch(BASE_EVALUATOR, f, LDContext.builder("key").name("Carol").build(), true); + assertMatch(BASE_EVALUATOR, f, LDContext.builder("key").name("Don").build(), false); + } + + @Test + public void segmentMatchClauseStillMatchesWhenReleaseIsEnabled() throws Exception { + // The release must not touch segmentMatch clauses, because segment matching reads the + // values list directly. + DataModelPreprocessing.setReleaseClauseValues(true); + + String segmentKey = "segkey"; + Clause inClause = clause("name", Operator.in, LDValue.of("Bob"), LDValue.of("Carol")); + Clause segmentClause = clauseMatchingSegment(segmentKey); + FeatureFlag flag = booleanFlagWithClauses("flag", segmentClause); + FeatureFlag flagWithBoth = booleanFlagWithClauses("flag2", inClause, segmentClause); + Segment segment = makeSegmentThatMatchesUser(segmentKey, "foo"); + + Evaluator e = evaluatorBuilder().withStoredSegments(segment).build(); + assertMatch(e, flag, LDContext.create("foo"), true); + assertMatch(e, flag, LDContext.create("bar"), false); + assertMatch(e, flagWithBoth, LDContext.builder("foo").name("Carol").build(), true); + } }