From ddbcc84b563e3ff28d1138aa966773fae2658d00 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Wed, 19 Jun 2024 13:44:34 +0200 Subject: [PATCH 1/3] feat: Change fractional custom op from percentage-based to relative weighting. #828 Signed-off-by: Simon Schrottner --- .../process/targeting/Fractional.java | 46 ++++++++-------- .../process/targeting/FractionalTest.java | 52 +++++++++++++++++-- 2 files changed, 72 insertions(+), 26 deletions(-) diff --git a/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/Fractional.java b/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/Fractional.java index 9f2d1859f..675ac7f36 100644 --- a/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/Fractional.java +++ b/providers/flagd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/Fractional.java @@ -50,37 +50,32 @@ public Object evaluate(List arguments, Object data) throws JsonLogicEvaluationEx } final List propertyList = new ArrayList<>(); + int totalWeight = 0; - double distribution = 0; try { for (Object dist : distibutions) { FractionProperty fractionProperty = new FractionProperty(dist); propertyList.add(fractionProperty); - distribution += fractionProperty.getPercentage(); + totalWeight += fractionProperty.getWeight(); } } catch (JsonLogicException e) { log.debug("Error parsing fractional targeting rule", e); return null; } - if (distribution != 100) { - log.debug("Fractional properties do not sum to 100"); - return null; - } - // find distribution - return distributeValue(bucketBy, propertyList); + return distributeValue(bucketBy, propertyList, totalWeight); } - private static String distributeValue(final String hashKey, final List propertyList) + private static String distributeValue(final String hashKey, final List propertyList, int totalWeight) throws JsonLogicEvaluationException { byte[] bytes = hashKey.getBytes(StandardCharsets.UTF_8); int mmrHash = MurmurHash3.hash32x86(bytes, 0, bytes.length, 0); - int bucket = (int) ((Math.abs(mmrHash) * 1.0f / Integer.MAX_VALUE) * 100); + float bucket = (Math.abs(mmrHash) * 1.0f / Integer.MAX_VALUE) * 100; - int bucketSum = 0; + float bucketSum = 0; for (FractionProperty p : propertyList) { - bucketSum += p.getPercentage(); + bucketSum += p.getPercentage(totalWeight); if (bucket < bucketSum) { return p.getVariant(); @@ -95,7 +90,7 @@ private static String distributeValue(final String hashKey, final List array = (List) from; - if (array.size() != 2) { - throw new JsonLogicException("Fraction property does not have two elements"); + if (array.isEmpty()) { + throw new JsonLogicException("Fraction property needs at least one element"); } // first must be a string @@ -117,14 +112,23 @@ protected final void finalize() { throw new JsonLogicException("First element of the fraction property is not a string variant"); } - // second element must be a number - if (!(array.get(1) instanceof Number)) { - throw new JsonLogicException("Second element of the fraction property is not a number"); - } - variant = (String) array.get(0); - percentage = ((Number) array.get(1)).intValue(); + if(array.size() >= 2) { + // second element must be a number + if (!(array.get(1) instanceof Number)) { + throw new JsonLogicException("Second element of the fraction property is not a number"); + } + weight = ((Number) array.get(1)).intValue(); + } else { + weight = 1; + } } + float getPercentage(int totalWeight) { + if (weight == 0) { + return 0; + } + return (float) (weight * 100) / totalWeight; + } } } diff --git a/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java b/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java index 9ef877d41..fd758f967 100644 --- a/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java +++ b/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java @@ -151,7 +151,7 @@ void targetingBackedFractional() throws JsonLogicEvaluationException { @Test - void invalidRuleSumNot100() throws JsonLogicEvaluationException { + void invalidRuleSumGreater100() throws JsonLogicEvaluationException { // given Fractional fractional = new Fractional(); @@ -189,7 +189,49 @@ void invalidRuleSumNot100() throws JsonLogicEvaluationException { Object evaluate = fractional.evaluate(rule, data); // then - assertNull(evaluate); + assertEquals("blue", evaluate); + } + + @Test + void invalidRuleSumlower100() throws JsonLogicEvaluationException { + // given + Fractional fractional = new Fractional(); + + /* Rule + * [ + * [ + * "blue", + * 50 + * ], + * [ + * "green", + * 30 + * ] + * ] + * */ + + final List rule = new ArrayList<>(); + + final List bucket1 = new ArrayList<>(); + bucket1.add("blue"); + bucket1.add(50); + + final List bucket2 = new ArrayList<>(); + bucket2.add("green"); + bucket2.add(70); + + rule.add(bucket1); + rule.add(bucket2); + + Map data = new HashMap<>(); + data.put(FLAG_KEY, "headerColor"); + data.put(TARGET_KEY, "foo@foo.com"); + + // when + Object evaluate = fractional.evaluate(rule, data); + + // then + assertEquals("blue", evaluate); } @Test @@ -227,7 +269,7 @@ void notEnoughBuckets() throws JsonLogicEvaluationException { @Test - void invalidRule() throws JsonLogicEvaluationException { + void prefillingRuleWithPlaceHolderValue() throws JsonLogicEvaluationException { // given Fractional fractional = new Fractional(); @@ -263,7 +305,7 @@ void invalidRule() throws JsonLogicEvaluationException { Object evaluate = fractional.evaluate(rule, data); // then - assertNull(evaluate); + assertEquals("blue", evaluate); } -} \ No newline at end of file +} From 7665ad5ee45d09cbbd3efcdeb83816989c13f06f Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Fri, 21 Jun 2024 09:06:01 +0200 Subject: [PATCH 2/3] Update providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java Co-authored-by: Kavindu Dodanduwa Signed-off-by: Simon Schrottner --- .../flagd/resolver/process/targeting/FractionalTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java b/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java index fd758f967..5ea4e546a 100644 --- a/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java +++ b/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java @@ -218,7 +218,7 @@ void invalidRuleSumlower100() throws JsonLogicEvaluationException { final List bucket2 = new ArrayList<>(); bucket2.add("green"); - bucket2.add(70); + bucket2.add(30); rule.add(bucket1); rule.add(bucket2); From fae24a61c770f995d0dd3549e2ef65dd83a679da Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Thu, 27 Jun 2024 11:31:36 +0200 Subject: [PATCH 3/3] add more test cases, and reduce boilerplate Signed-off-by: Simon Schrottner --- .../process/targeting/FractionalTest.java | 334 +++--------------- .../test/resources/fractional/1-1-1-1.json | 21 ++ .../resources/fractional/25-25-25-25.json | 21 ++ .../src/test/resources/fractional/50-50.json | 13 + .../fractional/notEnoughBuckets.json | 9 + .../fractional/selfContainedFractionalA.json | 14 + .../fractional/selfContainedFractionalB.json | 14 + .../resources/fractional/sum-greater-100.json | 13 + .../fractional/sum-lower-100.json.json | 13 + .../test/resources/fractional/template.json | 5 + .../fractional/weighting-not-set.json | 12 + 11 files changed, 187 insertions(+), 282 deletions(-) create mode 100644 providers/flagd/src/test/resources/fractional/1-1-1-1.json create mode 100644 providers/flagd/src/test/resources/fractional/25-25-25-25.json create mode 100644 providers/flagd/src/test/resources/fractional/50-50.json create mode 100644 providers/flagd/src/test/resources/fractional/notEnoughBuckets.json create mode 100644 providers/flagd/src/test/resources/fractional/selfContainedFractionalA.json create mode 100644 providers/flagd/src/test/resources/fractional/selfContainedFractionalB.json create mode 100644 providers/flagd/src/test/resources/fractional/sum-greater-100.json create mode 100644 providers/flagd/src/test/resources/fractional/sum-lower-100.json.json create mode 100644 providers/flagd/src/test/resources/fractional/template.json create mode 100644 providers/flagd/src/test/resources/fractional/weighting-not-set.json diff --git a/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java b/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java index 5ea4e546a..9f1002fe2 100644 --- a/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java +++ b/providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/FractionalTest.java @@ -1,311 +1,81 @@ package dev.openfeature.contrib.providers.flagd.resolver.process.targeting; -import static dev.openfeature.contrib.providers.flagd.resolver.process.targeting.Operator.FLAGD_PROPS_KEY; -import static dev.openfeature.contrib.providers.flagd.resolver.process.targeting.Operator.FLAG_KEY; -import static dev.openfeature.contrib.providers.flagd.resolver.process.targeting.Operator.TARGET_KEY; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNull; - -import java.util.ArrayList; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.ObjectMapper; +import io.github.jamsesso.jsonlogic.evaluator.JsonLogicEvaluationException; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.converter.ArgumentConversionException; +import org.junit.jupiter.params.converter.ConvertWith; +import org.junit.jupiter.params.converter.TypedArgumentConverter; +import org.junit.jupiter.params.provider.MethodSource; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; -import org.junit.jupiter.api.Test; - -import io.github.jamsesso.jsonlogic.evaluator.JsonLogicEvaluationException; +import static dev.openfeature.contrib.providers.flagd.resolver.process.targeting.Operator.*; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Named.named; +import static org.junit.jupiter.params.provider.Arguments.arguments; class FractionalTest { - @Test - void selfContainedFractionalA() throws JsonLogicEvaluationException { + @ParameterizedTest + @MethodSource("allFilesInDir") + void validate_emptyJson_targetingReturned(@ConvertWith(FileContentConverter.class) TestData testData) + throws JsonLogicEvaluationException { // given Fractional fractional = new Fractional(); - /* Rule - * [ - * "flagAbucketKeyA", // this is resolved value of an expression - * [ - * "red", - * 50 - * ], - * [ - * "blue", - * 50 - * ] - * ] - * */ - - final List rule = new ArrayList<>(); - rule.add("flagAbucketKeyA"); - - final List bucket1 = new ArrayList<>(); - bucket1.add("red"); - bucket1.add(50); - - final List bucket2 = new ArrayList<>(); - bucket2.add("green"); - bucket2.add(50); - - rule.add(bucket1); - rule.add(bucket2); - - Map flagdProperties = new HashMap<>(); - flagdProperties.put(FLAG_KEY, "flagA"); Map data = new HashMap<>(); - data.put(FLAGD_PROPS_KEY, flagdProperties); - - // when - Object evaluate = fractional.evaluate(rule, data); - - // then - assertEquals("red", evaluate); - } - - @Test - void selfContainedFractionalB() throws JsonLogicEvaluationException { - // given - Fractional fractional = new Fractional(); - - /* Rule - * [ - * "flagAbucketKeyB", // this is resolved value of an expression - * [ - * "red", - * 50 - * ], - * [ - * "blue", - * 50 - * ] - * ] - * */ - - final List rule = new ArrayList<>(); - rule.add("flagAbucketKeyB"); - - final List bucket1 = new ArrayList<>(); - bucket1.add("red"); - bucket1.add(50); - - final List bucket2 = new ArrayList<>(); - bucket2.add("green"); - bucket2.add(50); - - rule.add(bucket1); - rule.add(bucket2); + data.put(FLAG_KEY, "headerColor"); + data.put(TARGET_KEY, "foo@foo.com"); Map flagdProperties = new HashMap<>(); flagdProperties.put(FLAG_KEY, "flagA"); - Map data = new HashMap<>(); data.put(FLAGD_PROPS_KEY, flagdProperties); // when - Object evaluate = fractional.evaluate(rule, data); - - // then - assertEquals("green", evaluate); - } - - @Test - void targetingBackedFractional() throws JsonLogicEvaluationException { - // given - Fractional fractional = new Fractional(); - - /* Rule - * [ - * [ - * "blue", - * 50 - * ], - * [ - * "green", - * 50 - * ] - * ] - * */ - - final List rule = new ArrayList<>(); - - final List bucket1 = new ArrayList<>(); - bucket1.add("blue"); - bucket1.add(50); - - final List bucket2 = new ArrayList<>(); - bucket2.add("green"); - bucket2.add(50); - - rule.add(bucket1); - rule.add(bucket2); - - Map data = new HashMap<>(); - data.put(FLAG_KEY, "headerColor"); - data.put(TARGET_KEY, "foo@foo.com"); - - // when - Object evaluate = fractional.evaluate(rule, data); - - // then - assertEquals("blue", evaluate); - } - - - @Test - void invalidRuleSumGreater100() throws JsonLogicEvaluationException { - // given - Fractional fractional = new Fractional(); - - /* Rule - * [ - * [ - * "blue", - * 50 - * ], - * [ - * "green", - * 70 - * ] - * ] - * */ - - final List rule = new ArrayList<>(); - - final List bucket1 = new ArrayList<>(); - bucket1.add("blue"); - bucket1.add(50); - - final List bucket2 = new ArrayList<>(); - bucket2.add("green"); - bucket2.add(70); - - rule.add(bucket1); - rule.add(bucket2); - - Map data = new HashMap<>(); - data.put(FLAG_KEY, "headerColor"); - data.put(TARGET_KEY, "foo@foo.com"); - - // when - Object evaluate = fractional.evaluate(rule, data); + Object evaluate = fractional.evaluate(testData.rule, data); // then - assertEquals("blue", evaluate); + assertEquals(testData.result, evaluate); } - @Test - void invalidRuleSumlower100() throws JsonLogicEvaluationException { - // given - Fractional fractional = new Fractional(); - - /* Rule - * [ - * [ - * "blue", - * 50 - * ], - * [ - * "green", - * 30 - * ] - * ] - * */ - - final List rule = new ArrayList<>(); - - final List bucket1 = new ArrayList<>(); - bucket1.add("blue"); - bucket1.add(50); - - final List bucket2 = new ArrayList<>(); - bucket2.add("green"); - bucket2.add(30); - - rule.add(bucket1); - rule.add(bucket2); - - Map data = new HashMap<>(); - data.put(FLAG_KEY, "headerColor"); - data.put(TARGET_KEY, "foo@foo.com"); - - // when - Object evaluate = fractional.evaluate(rule, data); - - // then - assertEquals("blue", evaluate); + public static Stream allFilesInDir() throws IOException { + return Files.list(Paths.get("src", "test", "resources", "fractional")) + .map(path -> arguments(named(path.getFileName().toString(), path))); } - @Test - void notEnoughBuckets() throws JsonLogicEvaluationException { - // given - Fractional fractional = new Fractional(); - - /* Rule - * [ - * [ - * "blue", - * 100 - * ] - * ] - * */ - - final List rule = new ArrayList<>(); - - final List bucket1 = new ArrayList<>(); - bucket1.add("blue"); - bucket1.add(50); - - rule.add(bucket1); - - Map data = new HashMap<>(); - data.put(FLAG_KEY, "headerColor"); - data.put(TARGET_KEY, "foo@foo.com"); - - // when - Object evaluate = fractional.evaluate(rule, data); - - // then - assertNull(evaluate); + static class FileContentConverter extends TypedArgumentConverter { + protected FileContentConverter() { + super(Path.class, TestData.class); + } + + @Override + protected TestData convert(Path path) throws ArgumentConversionException { + try { + Stream lines = Files.lines(path); + String data = lines.collect(Collectors.joining("\n")); + lines.close(); + ObjectMapper mapper = new ObjectMapper(); + return mapper.readValue(data, TestData.class); + } catch (IOException e) { + throw new RuntimeException(e); + } + } } - - @Test - void prefillingRuleWithPlaceHolderValue() throws JsonLogicEvaluationException { - // given - Fractional fractional = new Fractional(); - - /* Rule - * [ - * [ - * "blue", - * 50 - * ], - * [ - * "green" - * ] - * ] - * */ - - final List rule = new ArrayList<>(); - - final List bucket1 = new ArrayList<>(); - bucket1.add("blue"); - bucket1.add(50); - - final List bucket2 = new ArrayList<>(); - bucket2.add("green"); - - rule.add(bucket1); - rule.add(bucket2); - - Map data = new HashMap<>(); - data.put(FLAG_KEY, "headerColor"); - data.put(TARGET_KEY, "foo@foo.com"); - - // when - Object evaluate = fractional.evaluate(rule, data); - - // then - assertEquals("blue", evaluate); + static class TestData { + @JsonProperty("result") + Object result; + @JsonProperty("rule") + List rule; } - } diff --git a/providers/flagd/src/test/resources/fractional/1-1-1-1.json b/providers/flagd/src/test/resources/fractional/1-1-1-1.json new file mode 100644 index 000000000..74f0a1b67 --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/1-1-1-1.json @@ -0,0 +1,21 @@ +{ + "rule": [ + [ + "blue", + 1 + ], + [ + "green", + 1 + ], + [ + "red", + 1 + ], + [ + "yellow", + 1 + ] + ], + "result": "blue" +} diff --git a/providers/flagd/src/test/resources/fractional/25-25-25-25.json b/providers/flagd/src/test/resources/fractional/25-25-25-25.json new file mode 100644 index 000000000..a849ba602 --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/25-25-25-25.json @@ -0,0 +1,21 @@ +{ + "rule": [ + [ + "blue", + 25 + ], + [ + "green", + 25 + ], + [ + "red", + 25 + ], + [ + "yellow", + 25 + ] + ], + "result": "blue" +} diff --git a/providers/flagd/src/test/resources/fractional/50-50.json b/providers/flagd/src/test/resources/fractional/50-50.json new file mode 100644 index 000000000..5f158006c --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/50-50.json @@ -0,0 +1,13 @@ +{ + "rule": [ + [ + "blue", + 50 + ], + [ + "green", + 50 + ] + ], + "result": "blue" +} diff --git a/providers/flagd/src/test/resources/fractional/notEnoughBuckets.json b/providers/flagd/src/test/resources/fractional/notEnoughBuckets.json new file mode 100644 index 000000000..b29de7a83 --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/notEnoughBuckets.json @@ -0,0 +1,9 @@ +{ + "rule": [ + [ + "blue", + 50 + ] + ], + "result": null +} diff --git a/providers/flagd/src/test/resources/fractional/selfContainedFractionalA.json b/providers/flagd/src/test/resources/fractional/selfContainedFractionalA.json new file mode 100644 index 000000000..88868c6f3 --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/selfContainedFractionalA.json @@ -0,0 +1,14 @@ +{ + "rule": [ + "flagAbucketKeyA", + [ + "red", + 50 + ], + [ + "blue", + 50 + ] + ], + "result": "red" +} diff --git a/providers/flagd/src/test/resources/fractional/selfContainedFractionalB.json b/providers/flagd/src/test/resources/fractional/selfContainedFractionalB.json new file mode 100644 index 000000000..2beb7e5be --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/selfContainedFractionalB.json @@ -0,0 +1,14 @@ +{ + "rule": [ + "flagAbucketKeyB", + [ + "red", + 50 + ], + [ + "blue", + 50 + ] + ], + "result": "blue" +} diff --git a/providers/flagd/src/test/resources/fractional/sum-greater-100.json b/providers/flagd/src/test/resources/fractional/sum-greater-100.json new file mode 100644 index 000000000..806dae532 --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/sum-greater-100.json @@ -0,0 +1,13 @@ +{ + "rule": [ + [ + "blue", + 50 + ], + [ + "green", + 70 + ] + ], + "result": "blue" +} diff --git a/providers/flagd/src/test/resources/fractional/sum-lower-100.json.json b/providers/flagd/src/test/resources/fractional/sum-lower-100.json.json new file mode 100644 index 000000000..06bdb8f14 --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/sum-lower-100.json.json @@ -0,0 +1,13 @@ +{ + "rule": [ + [ + "blue", + 50 + ], + [ + "green", + 30 + ] + ], + "result": "blue" +} diff --git a/providers/flagd/src/test/resources/fractional/template.json b/providers/flagd/src/test/resources/fractional/template.json new file mode 100644 index 000000000..8258e04de --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/template.json @@ -0,0 +1,5 @@ +{ + "rule": [ + ], + "result": null +} diff --git a/providers/flagd/src/test/resources/fractional/weighting-not-set.json b/providers/flagd/src/test/resources/fractional/weighting-not-set.json new file mode 100644 index 000000000..9e2278791 --- /dev/null +++ b/providers/flagd/src/test/resources/fractional/weighting-not-set.json @@ -0,0 +1,12 @@ +{ + "rule": [ + [ + "blue", + 50 + ], + [ + "green" + ] + ], + "result": "blue" +}