Merge pull request #11483 from vespa-engine/bjorncs/system-flags-improvements

Bjorncs/system flags improvements

11 files changed, 97 insertions, 30 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 83e3c03ffaa..7e9da53b1c9 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
@@ -9,6 +9,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -48,10 +49,8 @@ public class SystemFlagsDataArchive {
                 String name = entry.getName();
                 if (!entry.isDirectory() && name.startsWith("flags/") && name.endsWith(".json")) {
                     Path filePath = Paths.get(name);
-                    String filename = filePath.getFileName().toString();
-                    FlagData flagData = FlagData.deserializeUtf8Json(zipIn.readAllBytes());
-                    verifyFlagDataMatchesDirectoryName(filePath, flagData);
-                    builder.addFile(filename, flagData);
+                    String rawData = new String(zipIn.readAllBytes(), StandardCharsets.UTF_8);
+                    addFile(builder, rawData, filePath);
                 }
             }
             return builder.build();
@@ -70,13 +69,10 @@ public class SystemFlagsDataArchive {
             Builder builder = new Builder();
             directoryStream.forEach(absolutePath -> {
                 Path relativePath = root.relativize(absolutePath);
-                if (!Files.isDirectory(absolutePath) && relativePath.startsWith("flags")) {
-                    String filename = relativePath.getFileName().toString();
-                    if (filename.endsWith(".json")) {
-                        FlagData flagData = FlagData.deserializeUtf8Json(uncheck(() -> Files.readAllBytes(absolutePath)));
-                        verifyFlagDataMatchesDirectoryName(relativePath, flagData);
-                        builder.addFile(filename, flagData);
-                    }
+                if (!Files.isDirectory(absolutePath) &&
+                        relativePath.startsWith("flags") && relativePath.toString().endsWith(".json")) {
+                    String rawData = uncheck(() -> Files.readString(absolutePath, StandardCharsets.UTF_8));
+                    addFile(builder, rawData, relativePath);
                 }
             });
             return builder.build();
@@ -106,20 +102,31 @@ public class SystemFlagsDataArchive {
             for (String filename : filenames) {
                 FlagData data = fileMap.get(filename);
                 if (data != null) {
-                    targetData.add(data);
-                    break;
+                    if (!data.isEmpty()) {
+                        targetData.add(data);
+                    }
+                    return;
                 }
             }
         });
         return targetData;
     }
 
-    private static void verifyFlagDataMatchesDirectoryName(Path filePath, FlagData flagData) {
-        String flagDirectoryName = filePath.getName(1).toString();
-        if (!flagDirectoryName.equals(flagData.id().toString())) {
-            throw new IllegalArgumentException(
-                    String.format("Flag data file with flag id '%s' in directory for '%s'", flagData.id(), flagDirectoryName));
+    private static void addFile(Builder builder, String rawData, Path filePath) {
+        String filename = filePath.getFileName().toString();
+        FlagId directoryDeducedFlagId = new FlagId(filePath.getName(1).toString());
+        FlagData flagData;
+        if (rawData.isBlank()) {
+            flagData = new FlagData(directoryDeducedFlagId);
+        } else {
+            flagData = FlagData.deserialize(rawData);
+            if (!directoryDeducedFlagId.equals(flagData.id())) {
+                throw new IllegalArgumentException(
+                        String.format("Flag data file with flag id '%s' in directory for '%s'",
+                                      flagData.id(), directoryDeducedFlagId.toString()));
+            }
         }
+        builder.addFile(filename, flagData);
     }
 
     public static class Builder {
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 35fec04e4c5..14584650d3b 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
@@ -8,6 +8,7 @@ import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.athenz.api.AthenzService;
 import com.yahoo.vespa.flags.FetchVector;
+import com.yahoo.vespa.flags.FlagId;
 import com.yahoo.vespa.flags.RawFlag;
 import com.yahoo.vespa.flags.json.FlagData;
 import org.junit.Rule;
@@ -24,9 +25,10 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.URI;
 import java.nio.file.Paths;
+import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
+import static java.util.stream.Collectors.toList;
 import static org.assertj.core.api.Assertions.assertThat;
 
 /**
@@ -35,6 +37,8 @@ import static org.assertj.core.api.Assertions.assertThat;
 public class SystemFlagsDataArchiveTest {
 
     private static final SystemName SYSTEM = SystemName.main;
+    private static final FlagId MY_TEST_FLAG = new FlagId("my-test-flag");
+    private static final FlagId FLAG_WITH_EMPTY_DATA = new FlagId("flag-with-empty-data");
 
     @Rule
     public final TemporaryFolder temporaryFolder = new TemporaryFolder();
@@ -61,29 +65,49 @@ public class SystemFlagsDataArchiveTest {
         }
         try (InputStream in = new BufferedInputStream(new FileInputStream(tempFile))) {
             SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromZip(in);
-            assertArchiveReturnsCorrectDataForTarget(archive);
+            assertArchiveReturnsCorrectTestFlagDataForTarget(archive);
         }
     }
 
     @Test
     public void retrieves_correct_flag_data_for_target() {
         var archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags/"));
-        assertArchiveReturnsCorrectDataForTarget(archive);
+        assertArchiveReturnsCorrectTestFlagDataForTarget(archive);
     }
 
-    private static void assertArchiveReturnsCorrectDataForTarget(SystemFlagsDataArchive archive) {
-        assertFlagDataHasValue(archive, mainControllerTarget, "main.controller");
-        assertFlagDataHasValue(archive, prodUsWestCfgTarget, "main.prod.us-west-1.json");
-        assertFlagDataHasValue(archive, prodUsEast3CfgTarget, "main.prod");
-        assertFlagDataHasValue(archive, devUsEast1CfgTarget, "main");
+    @Test
+    public void empty_files_are_handled_as_no_flag_data_for_target() {
+        var archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags/"));
+        assertNoFlagData(archive, FLAG_WITH_EMPTY_DATA, mainControllerTarget);
+        assertFlagDataHasValue(archive, FLAG_WITH_EMPTY_DATA, prodUsWestCfgTarget, "main.prod.us-west-1");
+        assertNoFlagData(archive, FLAG_WITH_EMPTY_DATA, prodUsEast3CfgTarget);
+        assertFlagDataHasValue(archive, FLAG_WITH_EMPTY_DATA, devUsEast1CfgTarget, "main");
+    }
+
+    private static void assertArchiveReturnsCorrectTestFlagDataForTarget(SystemFlagsDataArchive archive) {
+        assertFlagDataHasValue(archive, MY_TEST_FLAG, mainControllerTarget, "main.controller");
+        assertFlagDataHasValue(archive, MY_TEST_FLAG, prodUsWestCfgTarget, "main.prod.us-west-1");
+        assertFlagDataHasValue(archive, MY_TEST_FLAG, prodUsEast3CfgTarget, "main.prod");
+        assertFlagDataHasValue(archive, MY_TEST_FLAG, devUsEast1CfgTarget, "main");
     }
 
-    private static void assertFlagDataHasValue(SystemFlagsDataArchive archive, FlagsTarget target, String value) {
-        Set<FlagData> data = archive.flagData(target);
+    private static void assertFlagDataHasValue(SystemFlagsDataArchive archive, FlagId flagId, FlagsTarget target, String value) {
+        List<FlagData> data = getData(archive, flagId, target);
         assertThat(data).hasSize(1);
-        FlagData flagData = data.iterator().next();
+        FlagData flagData = data.get(0);
         RawFlag rawFlag = flagData.resolve(FetchVector.fromMap(Map.of())).get();
         assertThat(rawFlag.asJson()).isEqualTo(String.format("\"%s\"", value));
     }
 
+    private static void assertNoFlagData(SystemFlagsDataArchive archive, FlagId flagId, FlagsTarget target) {
+        List<FlagData> data = getData(archive, flagId, target);
+        assertThat(data).isEmpty();
+    }
+
+    private static List<FlagData> getData(SystemFlagsDataArchive archive, FlagId flagId, FlagsTarget target) {
+        return archive.flagData(target).stream()
+                .filter(d -> d.id().equals(flagId))
+                .collect(toList());
+    }
+
 }
\ No newline at end of file
diff --git a/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/default.json b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/default.json
new file mode 100644
index 00000000000..54aba0d9923
--- /dev/null
+++ b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/default.json
@@ -0,0 +1,8 @@
+{
+  "id" : "flag-with-empty-data",
+  "rules" : [
+    {
+      "value" : "default"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.controller.json b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.controller.json
new file mode 100644
index 00000000000..c9b46d68ace
--- /dev/null
+++ b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.controller.json
@@ -0,0 +1,4 @@
+{
+  "id" : "flag-with-empty-data",
+  "comment": "empty data using only id field"
+}
\ No newline at end of file
diff --git a/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.json b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.json
new file mode 100644
index 00000000000..cef75be02b7
--- /dev/null
+++ b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.json
@@ -0,0 +1,8 @@
+{
+  "id" : "flag-with-empty-data",
+  "rules" : [
+    {
+      "value" : "main"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.prod.json b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.prod.json
new file mode 100644
index 00000000000..e69de29bb2d
--- /dev/null
+++ b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.prod.json
diff --git a/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.prod.us-west-1.json b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.prod.us-west-1.json
new file mode 100644
index 00000000000..fc2690c9c04
--- /dev/null
+++ b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.prod.us-west-1.json
@@ -0,0 +1,8 @@
+{
+  "id" : "flag-with-empty-data",
+  "rules" : [
+    {
+      "value" : "main.prod.us-west-1"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-api/src/test/resources/system-flags/flags/my-test-flag/main.prod.us-west-1.json b/controller-api/src/test/resources/system-flags/flags/my-test-flag/main.prod.us-west-1.json
index 87b435cdab1..45989773df8 100644
--- a/controller-api/src/test/resources/system-flags/flags/my-test-flag/main.prod.us-west-1.json
+++ b/controller-api/src/test/resources/system-flags/flags/my-test-flag/main.prod.us-west-1.json
@@ -2,7 +2,7 @@
   "id" : "my-test-flag",
   "rules" : [
     {
-      "value" : "main.prod.us-west-1.json"
+      "value" : "main.prod.us-west-1"
     }
   ]
 }
\ No newline at end of file
diff --git a/flags/pom.xml b/flags/pom.xml
index 6afa920e261..87fbc30d92d 100644
--- a/flags/pom.xml
+++ b/flags/pom.xml
@@ -96,6 +96,10 @@
                 <artifactId>bundle-plugin</artifactId>
                 <extensions>true</extensions>
             </plugin>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-compiler-plugin</artifactId>
+            </plugin>
         </plugins>
     </build>
 </project>
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java
index 4cce7a9bc2a..866d4782a2a 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java
@@ -82,6 +82,8 @@ public class FetchVector {
         return map;
     }
 
+    public boolean isEmpty() { return map.isEmpty(); }
+
     /** Returns a new FetchVector, identical to {@code this} except for its value in {@code dimension}. */
     public FetchVector with(Dimension dimension, String value) {
         return makeFetchVector(merged -> merged.put(dimension, value));
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 64c4bbe7616..c4079380a8c 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
@@ -52,6 +52,8 @@ public class FlagData {
         return id;
     }
 
+    public boolean isEmpty() { return rules.isEmpty() && defaultFetchVector.isEmpty(); }
+
     public Optional<RawFlag> resolve(FetchVector fetchVector) {
         return rules.stream()
                 .filter(rule -> rule.match(defaultFetchVector.with(fetchVector)))