diff options
author | Håkon Hallingstad <hakon@yahooinc.com> | 2023-08-02 12:23:15 +0200 |
---|---|---|
committer | Håkon Hallingstad <hakon@yahooinc.com> | 2023-08-02 12:23:15 +0200 |
commit | 697a8647a79b9a8038217801cebb3b336d4a278a (patch) | |
tree | 952472490d02beb1098a17119269c53ab39495c6 /controller-api | |
parent | 0adede88d0aa7bbb7280a5c26a5279fc08246652 (diff) |
Treat null value on rule like comment
Diffstat (limited to 'controller-api')
3 files changed, 131 insertions, 42 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 f6ffd711f51..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 @@ -236,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(); } @@ -299,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 f1e356eead2..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; @@ -158,51 +159,98 @@ public class SystemFlagsDataArchiveTest { } @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()); + 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() { - 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 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 + } + ] +} |