Include flag id in flag data and other review fixes

5 files changed, 38 insertions, 24 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/DefinedFlags.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/DefinedFlags.java
index 6d1f9f452d5..9f0e39b7d43 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/DefinedFlags.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/DefinedFlags.java
@@ -6,6 +6,7 @@ import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.jdisc.Response;
+import com.yahoo.vespa.config.server.http.HttpConfigResponse;
 import com.yahoo.vespa.flags.FlagDefinition;
 import com.yahoo.vespa.flags.json.DimensionHelper;
 
@@ -44,6 +45,6 @@ public class DefinedFlags extends HttpResponse {
 
     @Override
     public String getContentType() {
-        return "application/json";
+        return HttpConfigResponse.JSON_CONTENT_TYPE;
     }
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagDataListResponse.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagDataListResponse.java
index 4d9ba96b791..ae72660ce9f 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagDataListResponse.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagDataListResponse.java
@@ -2,9 +2,11 @@
 package com.yahoo.vespa.config.server.http.flags;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.jdisc.Response;
+import com.yahoo.vespa.config.server.http.HttpConfigResponse;
 import com.yahoo.vespa.flags.FlagId;
 import com.yahoo.vespa.flags.json.FlagData;
 
@@ -22,24 +24,27 @@ public class FlagDataListResponse extends HttpResponse {
 
     private final String flagsV1Uri;
     private final Map<FlagId, FlagData> flags;
-    private final boolean showDataInsteadOfUrl;
+    private final boolean recursive;
 
-    public FlagDataListResponse(String flagsV1Uri, Map<FlagId, FlagData> flags, boolean showDataInsteadOfUrl) {
+    public FlagDataListResponse(String flagsV1Uri, Map<FlagId, FlagData> flags, boolean recursive) {
         super(Response.Status.OK);
         this.flagsV1Uri = flagsV1Uri;
         this.flags = flags;
-        this.showDataInsteadOfUrl = showDataInsteadOfUrl;
+        this.recursive = recursive;
     }
 
     @Override
     public void render(OutputStream outputStream) {
         ObjectNode rootNode = mapper.createObjectNode();
+        ArrayNode flagsArray = rootNode.putArray("flags");
         // Order flags by ID
-        new TreeMap<>(flags).forEach((flagId, flagData) -> {
-            if (showDataInsteadOfUrl) {
-                rootNode.set(flagId.toString(), flagData.toJsonNode());
+        new TreeMap<>(this.flags).forEach((flagId, flagData) -> {
+            if (recursive) {
+                flagsArray.add(flagData.toJsonNode());
             } else {
-                rootNode.putObject(flagId.toString()).put("url", flagsV1Uri + "/data/" + flagId.toString());
+                ObjectNode object = flagsArray.addObject();
+                object.put("id", flagId.toString());
+                object.put("url", flagsV1Uri + "/data/" + flagId.toString());
             }
         });
         uncheck(() -> mapper.writeValue(outputStream, rootNode));
@@ -47,6 +52,6 @@ public class FlagDataListResponse extends HttpResponse {
 
     @Override
     public String getContentType() {
-        return "application/json";
+        return HttpConfigResponse.JSON_CONTENT_TYPE;
     }
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagsHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagsHandler.java
index 5a59c4665b7..e64c356aa96 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagsHandler.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/FlagsHandler.java
@@ -75,9 +75,7 @@ public class FlagsHandler extends HttpHandler {
 
     private HttpResponse putFlagData(HttpRequest request, FlagId flagId) {
         flagsDb.setValue(flagId, FlagData.deserialize(request.getData()));
-
-        // The set & get is not atomic, but no harm is done in showing an outdated flag value in the response.
-        return getFlagData(flagId);
+        return new OKResponse();
     }
 
     private HttpResponse deleteFlagData(FlagId flagId) {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/OKResponse.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/OKResponse.java
index 19363fbdadc..87c02ae56f1 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/OKResponse.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/flags/OKResponse.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.config.server.http.flags;
 
 import com.yahoo.container.jdisc.EmptyResponse;
 import com.yahoo.jdisc.Response;
+import com.yahoo.vespa.config.server.http.HttpConfigResponse;
 
 /**
  * @author hakonhall
@@ -14,6 +15,6 @@ public class OKResponse extends EmptyResponse {
 
     @Override
     public String getContentType() {
-        return "application/json";
+        return HttpConfigResponse.JSON_CONTENT_TYPE;
     }
 }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/flags/FlagsHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/flags/FlagsHandlerTest.java
index e7bf36e1778..8781ec413d2 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/flags/FlagsHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/flags/FlagsHandlerTest.java
@@ -67,30 +67,31 @@ public class FlagsHandlerTest {
         // PUT flag with ID id1
         verifySuccessfulRequest(Method.PUT, "/data/" + FLAG1.id(),
                 "{\n" +
+                        "  \"id\": \"id1\",\n" +
                         "  \"rules\": [\n" +
                         "    {\n" +
                         "      \"value\": true\n" +
                         "    }\n" +
                         "  ]\n" +
                         "}",
-                "{\"rules\":[{\"value\":true}]}");
+                "");
 
-        // GET on ID id1 should return the same as the put (this will also issue a payload for the get,
-        // which we assume will be ignored).
+        // GET on ID id1 should return the same as the put.
         verifySuccessfulRequest(Method.GET, "/data/" + FLAG1.id(),
-                "", "{\"rules\":[{\"value\":true}]}");
+                "", "{\"id\":\"id1\",\"rules\":[{\"value\":true}]}");
 
         // List all flags should list only id1
         verifySuccessfulRequest(Method.GET, "/data",
-                "", "{\"id1\":{\"url\":\"https://foo.com:4443/flags/v1/data/id1\"}}");
+                "", "{\"flags\":[{\"id\":\"id1\",\"url\":\"https://foo.com:4443/flags/v1/data/id1\"}]}");
 
         // Should be identical to above: suffix / on path should be ignored
         verifySuccessfulRequest(Method.GET, "/data/",
-                "", "{\"id1\":{\"url\":\"https://foo.com:4443/flags/v1/data/id1\"}}");
+                "", "{\"flags\":[{\"id\":\"id1\",\"url\":\"https://foo.com:4443/flags/v1/data/id1\"}]}");
 
         // PUT id2
         verifySuccessfulRequest(Method.PUT, "/data/" + FLAG2.id(),
                 "{\n" +
+                        "  \"id\": \"id2\",\n" +
                         "  \"rules\": [\n" +
                         "    {\n" +
                         "      \"conditions\": [\n" +
@@ -112,34 +113,42 @@ public class FlagsHandlerTest {
                         "    \"zone\": \"zone1\"\n" +
                         "  }\n" +
                         "}\n",
-                "{\"rules\":[{\"conditions\":[{\"type\":\"whitelist\",\"dimension\":\"hostname\",\"values\":[\"host1\",\"host2\"]},{\"type\":\"blacklist\",\"dimension\":\"application\",\"values\":[\"app2\",\"app1\"]}],\"value\":true}],\"attributes\":{\"zone\":\"zone1\"}}");
+                "");
+
+        // GET on id2 should now return what was put
+        verifySuccessfulRequest(Method.GET, "/data/" + FLAG2.id(), "",
+                "{\"id\":\"id2\",\"rules\":[{\"conditions\":[{\"type\":\"whitelist\",\"dimension\":\"hostname\",\"values\":[\"host1\",\"host2\"]},{\"type\":\"blacklist\",\"dimension\":\"application\",\"values\":[\"app2\",\"app1\"]}],\"value\":true}],\"attributes\":{\"zone\":\"zone1\"}}");
 
         // The list of flag data should return id1 and id2
         verifySuccessfulRequest(Method.GET, "/data",
                 "",
-                "{\"id1\":{\"url\":\"https://foo.com:4443/flags/v1/data/id1\"},\"id2\":{\"url\":\"https://foo.com:4443/flags/v1/data/id2\"}}");
+                "{\"flags\":[{\"id\":\"id1\",\"url\":\"https://foo.com:4443/flags/v1/data/id1\"},{\"id\":\"id2\",\"url\":\"https://foo.com:4443/flags/v1/data/id2\"}]}");
 
         // Putting (overriding) id1 should work silently
         verifySuccessfulRequest(Method.PUT, "/data/" + FLAG1.id(),
                 "{\n" +
+                        "  \"id\": \"id1\",\n" +
                         "  \"rules\": [\n" +
                         "    {\n" +
                         "      \"value\": false\n" +
                         "    }\n" +
                         "  ]\n" +
                         "}\n",
-                "{\"rules\":[{\"value\":false}]}");
+                "");
+
+        // Verify PUT
+        verifySuccessfulRequest(Method.GET, "/data/" + FLAG1.id(), "", "{\"id\":\"id1\",\"rules\":[{\"value\":false}]}");
 
         // Get all recursivelly displays all flag data
         verifySuccessfulRequest(Method.GET, "/data?recursive=true", "",
-                "{\"id1\":{\"rules\":[{\"value\":false}]},\"id2\":{\"rules\":[{\"conditions\":[{\"type\":\"whitelist\",\"dimension\":\"hostname\",\"values\":[\"host1\",\"host2\"]},{\"type\":\"blacklist\",\"dimension\":\"application\",\"values\":[\"app2\",\"app1\"]}],\"value\":true}],\"attributes\":{\"zone\":\"zone1\"}}}");
+                "{\"flags\":[{\"id\":\"id1\",\"rules\":[{\"value\":false}]},{\"id\":\"id2\",\"rules\":[{\"conditions\":[{\"type\":\"whitelist\",\"dimension\":\"hostname\",\"values\":[\"host1\",\"host2\"]},{\"type\":\"blacklist\",\"dimension\":\"application\",\"values\":[\"app2\",\"app1\"]}],\"value\":true}],\"attributes\":{\"zone\":\"zone1\"}}]}");
 
         // Deleting both flags
         verifySuccessfulRequest(Method.DELETE, "/data/" + FLAG1.id(), "", "");
         verifySuccessfulRequest(Method.DELETE, "/data/" + FLAG2.id(), "", "");
 
         // And the list of data flags should now be empty
-        verifySuccessfulRequest(Method.GET, "/data", "", "{}");
+        verifySuccessfulRequest(Method.GET, "/data", "", "{\"flags\":[]}");
     }
 
     @Test