Merge pull request #27943 from vespa-engine/hakonhall/remove-trailing-conditionless-null-valued-rules

Remove trailing rules without value

4 files changed, 103 insertions, 22 deletions

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\"}"));
     }
 }