diff options
8 files changed, 279 insertions, 62 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..8ca4c37a85a 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)); } } @@ -234,6 +236,7 @@ public class SystemFlagsDataArchive { static String normalizeJson(String json, Set<ZoneId> zones) { JsonNode root = uncheck(() -> mapper.readTree(json)); removeCommentsRecursively(root); + removeNullRuleValues(root); verifyValues(root, zones); return root.toString(); } @@ -297,6 +300,22 @@ public class SystemFlagsDataArchive { node.forEach(SystemFlagsDataArchive::removeCommentsRecursively); } + private static void removeNullRuleValues(JsonNode root) { + if (root instanceof ObjectNode objectNode) { + JsonNode rules = objectNode.get("rules"); + if (rules != null) { + rules.forEach(ruleNode -> { + if (ruleNode instanceof ObjectNode rule) { + JsonNode value = rule.get("value"); + if (value != null && value.isNull()) { + rule.remove("value"); + } + } + }); + } + } + } + private static String toFilePath(FlagId flagId, String filename) { return "flags/" + flagId.toString() + "/" + filename; } 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..3417dc85224 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 @@ -32,6 +32,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -146,43 +147,110 @@ 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 remove_comments() { - assertTrue(JSON.equals("{\n" + - " \"a\": {\n" + - " \"b\": 1\n" + - " },\n" + - " \"list\": [\n" + - " {\n" + - " \"c\": 2\n" + - " },\n" + - " {\n" + - " }\n" + - " ]\n" + - "}", - SystemFlagsDataArchive.normalizeJson("{\n" + - " \"comment\": \"comment a\",\n" + - " \"a\": {\n" + - " \"comment\": \"comment b\",\n" + - " \"b\": 1\n" + - " },\n" + - " \"list\": [\n" + - " {\n" + - " \"comment\": \"comment c\",\n" + - " \"c\": 2\n" + - " },\n" + - " {\n" + - " \"comment\": \"comment d\"\n" + - " }\n" + - " ]\n" + - "}", Set.of()))); + void handles_absent_rule_value() { + SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-null-value/")); + + // west has null value on first rule + List<FlagData> westFlagData = archive.flagData(prodUsWestCfgTarget); + assertEquals(1, westFlagData.size()); + assertEquals(2, westFlagData.get(0).rules().size()); + assertEquals(Optional.empty(), westFlagData.get(0).rules().get(0).getValueToApply()); + + // east has no value on first rule + List<FlagData> eastFlagData = archive.flagData(prodUsEast3CfgTarget); + assertEquals(1, eastFlagData.size()); + assertEquals(2, eastFlagData.get(0).rules().size()); + assertEquals(Optional.empty(), eastFlagData.get(0).rules().get(0).getValueToApply()); + } + + @Test + void remove_comments_and_null_value_in_rules() { + assertTrue(JSON.equals(""" + { + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "hostname", + "values": [ "foo.com" ] + } + ] + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "prod.us-west-1" ] + } + ] + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "application", + "values": [ "f:o:o" ] + } + ], + "value": true + } + ] + }""", + SystemFlagsDataArchive.normalizeJson(""" + { + "comment": "bar", + "rules": [ + { + "comment": "bar", + "conditions": [ + { + "comment": "bar", + "type": "whitelist", + "dimension": "hostname", + "values": [ "foo.com" ] + } + ], + "value": null + }, + { + "comment": "bar", + "conditions": [ + { + "comment": "bar", + "type": "whitelist", + "dimension": "zone", + "values": [ "prod.us-west-1" ] + } + ] + }, + { + "comment": "bar", + "conditions": [ + { + "comment": "bar", + "type": "whitelist", + "dimension": "application", + "values": [ "f:o:o" ] + } + ], + "value": true + } + ] + }""", Set.of(ZoneId.from("prod.us-west-1"))))); } @Test diff --git a/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-east-3.json b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-east-3.json new file mode 100644 index 00000000000..b79e0913c22 --- /dev/null +++ b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-east-3.json @@ -0,0 +1,24 @@ +{ + "id" : "my-test-flag", + "rules" : [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "application", + "values": ["a:b:c"] + } + ] + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "hostname", + "values": ["foo.com"] + } + ], + "value" : true + } + ] +} 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 acda3b9db42..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,6 +69,7 @@ public class FlagData { } } } + newRules = optimizeRules(newRules); FetchVector newDefaultFetchVector = defaultFetchVector.without(fetchVector.dimensions()); @@ -188,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 c7da1abe7e2..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,10 +1,12 @@ // 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; +import java.util.List; import java.util.Optional; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -212,6 +214,62 @@ public class FlagDataTest { assertTrue(fullyResolved.isEmpty()); } + @Test + void testRemovalOfSentinelRuleWithNullValue() { + FlagData data = FlagData.deserialize(""" + { + "id": "id1", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "zone1", "zone2" ] + } + ], + "value": null + } + ] + }"""); + 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()); + } + private void verify(Optional<String> expectedValue, FetchVector vector) { FlagData data = FlagData.deserialize(json); assertEquals("id1", data.id().toString()); 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\"}")); } } |