Treat null value on rule like comment

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
+        }
+    ]
+}