diff options
7 files changed, 138 insertions, 44 deletions
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java index 1c547fea8ba..f6ffd711f51 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java @@ -213,12 +213,14 @@ public class SystemFlagsDataArchive { String serializedData = flagData.serializeToJson(); if (!JSON.equals(serializedData, normalizedRawData)) { - throw new IllegalArgumentException(filePath + " contains unknown non-comment fields: " + - "after removing any comment fields the JSON is:\n " + - normalizedRawData + - "\nbut deserializing this ended up with a JSON that are missing some of the fields:\n " + - serializedData + - "\nSee https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax"); + throw new IllegalArgumentException(""" + %s contains unknown non-comment fields or rules with null values: after removing any comment fields the JSON is: + %s + but deserializing this ended up with: + %s + These fields may be spelled wrong, or remove them? + See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax + """.formatted(filePath, normalizedRawData, serializedData)); } } diff --git a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java index a24bed54a8a..f1e356eead2 100644 --- a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java +++ b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java @@ -146,11 +146,31 @@ public class SystemFlagsDataArchiveTest { Throwable exception = assertThrows(IllegalArgumentException.class, () -> { SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-unknown-field-name/")); }); - assertTrue(exception.getMessage().contains("flags/my-test-flag/main.prod.us-west-1.json contains unknown non-comment fields: after removing any comment fields the JSON is:\n" + - " {\"id\":\"my-test-flag\",\"rules\":[{\"condition\":[{\"type\":\"whitelist\",\"dimension\":\"hostname\",\"values\":[\"foo.com\"]}],\"value\":\"default\"}]}\n" + - "but deserializing this ended up with a JSON that are missing some of the fields:\n" + - " {\"id\":\"my-test-flag\",\"rules\":[{\"value\":\"default\"}]}\n" + - "See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax")); + assertEquals(""" + flags/my-test-flag/main.prod.us-west-1.json contains unknown non-comment fields or rules with null values: after removing any comment fields the JSON is: + {"id":"my-test-flag","rules":[{"condition":[{"type":"whitelist","dimension":"hostname","values":["foo.com"]}],"value":"default"}]} + but deserializing this ended up with: + {"id":"my-test-flag","rules":[{"value":"default"}]} + These fields may be spelled wrong, or remove them? + See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax + """, + exception.getMessage()); + } + + @Test + void throws_on_null_value() { + Throwable exception = assertThrows(IllegalArgumentException.class, () -> { + SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-null-value/")); + }); + assertEquals(""" + flags/my-test-flag/main.prod.us-west-1.json contains unknown non-comment fields or rules with null values: after removing any comment fields the JSON is: + {"id":"my-test-flag","rules":[{"conditions":[{"type":"whitelist","dimension":"application","values":["a:b:c"]}],"value":null},{"conditions":[{"type":"whitelist","dimension":"hostname","values":["foo.com"]}],"value":true}]} + but deserializing this ended up with: + {"id":"my-test-flag","rules":[{"conditions":[{"type":"whitelist","dimension":"application","values":["a:b:c"]}]},{"conditions":[{"type":"whitelist","dimension":"hostname","values":["foo.com"]}],"value":true}]} + These fields may be spelled wrong, or remove them? + See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax + """, + exception.getMessage()); } @Test diff --git a/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-west-1.json b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-west-1.json new file mode 100644 index 00000000000..75cffdea009 --- /dev/null +++ b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-west-1.json @@ -0,0 +1,25 @@ +{ + "id" : "my-test-flag", + "rules" : [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "application", + "values": ["a:b:c"] + } + ], + "value" : null + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "hostname", + "values": ["foo.com"] + } + ], + "value" : true + } + ] +} diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java b/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java index 8a5366c65e2..bc103c2d68a 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java @@ -69,16 +69,7 @@ public class FlagData { } } } - - // Remove trailing rules that have no conditions and no value to apply. - while (newRules.size() > 0) { - Rule lastRule = newRules.get(newRules.size() - 1); - if (lastRule.conditions().isEmpty() && lastRule.getValueToApply().isEmpty()) { - newRules.remove(newRules.size() - 1); - } else { - break; - } - } + newRules = optimizeRules(newRules); FetchVector newDefaultFetchVector = defaultFetchVector.without(fetchVector.dimensions()); @@ -198,7 +189,26 @@ public class FlagData { private static List<Rule> rulesFromWire(List<WireRule> wireRules) { if (wireRules == null) return List.of(); - return wireRules.stream().map(Rule::fromWire).toList(); + return optimizeRules(wireRules.stream().map(Rule::fromWire).toList()); + } + + /** Take a raw list of rules from e.g. deserialization or partial resolution and normalize/simplify it. */ + private static List<Rule> optimizeRules(List<Rule> rules) { + // Remove trailing rules without value, as absent value implies the code default. + // Removing trailing rules may further simplify when e.g. this results in no rules, + // which is equivalent to no flag data at all, and flag data may be deleted from a zone. + if (rules.isEmpty()) return rules; + if (rules.get(rules.size() - 1).getValueToApply().isPresent()) return rules; + var newRules = new ArrayList<>(rules); + while (newRules.size() > 0) { + Rule lastRule = newRules.get(newRules.size() - 1); + if (lastRule.getValueToApply().isEmpty()) { + newRules.remove(newRules.size() - 1); + } else { + break; + } + } + return newRules; } } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java b/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java index 127c2b4f9da..b1b15faa938 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java @@ -83,9 +83,11 @@ public class Rule { public static Rule fromWire(WireRule wireRule) { List<Condition> conditions = wireRule.andConditions == null ? - Collections.emptyList() : + List.of() : wireRule.andConditions.stream().map(Condition::fromWire).toList(); - Optional<RawFlag> value = wireRule.value == null ? Optional.empty() : Optional.of(JsonNodeRawFlag.fromJsonNode(wireRule.value)); + Optional<RawFlag> value = wireRule.value == null || wireRule.value.isNull() ? + Optional.empty() : + Optional.of(JsonNodeRawFlag.fromJsonNode(wireRule.value)); return new Rule(value, conditions); } diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java index 4ec0ea0652f..3ca7f59c759 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.flags.json; +import com.yahoo.text.JSON; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.RawFlag; import org.junit.jupiter.api.Test; @@ -231,7 +232,40 @@ public class FlagDataTest { } ] }"""); - FlagData flagData = data.partialResolve(vector.with(FetchVector.Dimension.ZONE_ID, "zone3")); + assertEquals(data, new FlagData(data.id(), new FetchVector(), List.of())); + assertTrue(data.isEmpty()); + } + + @Test + void testRemovalOfSentinelRuleWithoutValue() { + String json = """ + { + "id": "id1", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "zone1", "zone2" ] + } + ] + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "cloud", + "values": [ "aws" ] + } + ], + "value": true + } + ] + }"""; + FlagData data = FlagData.deserialize(json); + assertTrue(JSON.equals(data.serializeToJson(), json)); + FlagData flagData = data.partialResolve(vector.with(FetchVector.Dimension.CLOUD, "gcp")); assertEquals(flagData, new FlagData(data.id(), new FetchVector(), List.of())); assertTrue(flagData.isEmpty()); } diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java index 2cc19917793..2192739c96c 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/SerializationTest.java @@ -108,24 +108,25 @@ public class SerializationTest { @Test void jsonWithStrayFields() { - String json = "{\n" + - " \"id\": \"id3\",\n" + - " \"foo\": true,\n" + - " \"rules\": [\n" + - " {\n" + - " \"conditions\": [\n" + - " {\n" + - " \"type\": \"whitelist\",\n" + - " \"dimension\": \"zone\",\n" + - " \"bar\": \"zoo\"\n" + - " }\n" + - " ],\n" + - " \"other\": true\n" + - " }\n" + - " ],\n" + - " \"attributes\": {\n" + - " }\n" + - "}"; + String json = """ + { + "id": "id3", + "foo": true, + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "bar": "zoo" + } + ], + "other": true + } + ], + "attributes": { + } + }"""; WireFlagData wireData = WireFlagData.deserialize(json); @@ -140,6 +141,6 @@ public class SerializationTest { assertThat(wireData.serializeToJson(), equalTo("{\"id\":\"id3\",\"rules\":[{\"conditions\":[{\"type\":\"whitelist\",\"dimension\":\"zone\"}]}],\"attributes\":{}}")); - assertThat(FlagData.deserialize(json).serializeToJson(), equalTo("{\"id\":\"id3\",\"rules\":[{\"conditions\":[{\"type\":\"whitelist\",\"dimension\":\"zone\"}]}]}")); + assertThat(FlagData.deserialize(json).serializeToJson(), equalTo("{\"id\":\"id3\"}")); } } |