Typed flag classes

This reintroduces the non-generic flag classes:
 - a value() returns the primitive type for flags wrapping a primitive type
 - easier to use in testing
 - Serializer is moved to internals of typed class

Defines the flag backed by boolean BooleanFlag instead of FeatureFlag since not
all boolean flags are necessarily guarding a feature.

4 files changed, 55 insertions, 34 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java
index 2d5d5296762..2463cca190a 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/Application.java
@@ -23,9 +23,9 @@ import com.yahoo.vespa.config.server.rpc.ConfigResponseFactory;
 import com.yahoo.vespa.config.server.rpc.UncompressedConfigResponseFactory;
 import com.yahoo.vespa.config.server.tenant.TenantRepository;
 import com.yahoo.vespa.config.util.ConfigUtils;
+import com.yahoo.vespa.flags.BooleanFlag;
 import com.yahoo.vespa.flags.FetchVector;
 import com.yahoo.vespa.flags.FileFlagSource;
-import com.yahoo.vespa.flags.Flag;
 import com.yahoo.vespa.flags.FlagSource;
 import com.yahoo.vespa.flags.Flags;
 
@@ -49,7 +49,7 @@ public class Application implements ModelResult {
     private final ServerCache cache;
     private final MetricUpdater metricUpdater;
     private final ApplicationId app;
-    private final Flag<Boolean> useConfigServerCache;
+    private final BooleanFlag useConfigServerCache;
 
     public Application(Model model, ServerCache cache, long appGeneration, boolean internalRedeploy,
                        Version vespaVersion, MetricUpdater metricUpdater, ApplicationId app) {
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 9f0e39b7d43..6d5d0df8f1a 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
@@ -20,12 +20,12 @@ import java.util.List;
  */
 public class DefinedFlags extends HttpResponse {
     private static ObjectMapper mapper = new ObjectMapper();
-    private static final Comparator<FlagDefinition<?>> sortByFlagId =
+    private static final Comparator<FlagDefinition> sortByFlagId =
             (left, right) -> left.getUnboundFlag().id().compareTo(right.getUnboundFlag().id());
 
-    private final List<FlagDefinition<?>> flags;
+    private final List<FlagDefinition> flags;
 
-    public DefinedFlags(List<FlagDefinition<?>> flags) {
+    public DefinedFlags(List<FlagDefinition> flags) {
         super(Response.Status.OK);
         this.flags = flags;
     }
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 e64c356aa96..e5efc74fb75 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
@@ -6,13 +6,17 @@ import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.container.jdisc.LoggingRequestHandler;
 import com.yahoo.restapi.Path;
+import com.yahoo.vespa.config.server.http.HttpErrorResponse;
 import com.yahoo.vespa.config.server.http.HttpHandler;
 import com.yahoo.vespa.config.server.http.NotFoundException;
 import com.yahoo.vespa.configserver.flags.FlagsDb;
+import com.yahoo.vespa.flags.FlagDefinition;
 import com.yahoo.vespa.flags.FlagId;
 import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.flags.json.FlagData;
+import com.yahoo.yolean.Exceptions;
 
+import java.io.UncheckedIOException;
 import java.net.URI;
 import java.util.Objects;
 
@@ -74,7 +78,19 @@ public class FlagsHandler extends HttpHandler {
     }
 
     private HttpResponse putFlagData(HttpRequest request, FlagId flagId) {
-        flagsDb.setValue(flagId, FlagData.deserialize(request.getData()));
+        FlagData data;
+        try {
+            data = FlagData.deserialize(request.getData());
+        } catch (UncheckedIOException e) {
+            return HttpErrorResponse.badRequest("Failed to deserialize request data: " + Exceptions.toMessageString(e));
+        }
+
+        if (!isForce(request)) {
+            FlagDefinition definition = Flags.getFlag(flagId).get(); // FlagId has been validated in findFlagId()
+            data.validate(definition.getUnboundFlag().serializer());
+        }
+
+        flagsDb.setValue(flagId, data);
         return new OKResponse();
     }
 
@@ -86,12 +102,15 @@ public class FlagsHandler extends HttpHandler {
     private FlagId findFlagId(HttpRequest request, Path path) {
         FlagId flagId = new FlagId(path.get("flagId"));
 
-        if (!Objects.equals(request.getProperty("force"), "true")) {
-            if (Flags.getAllFlags().stream().noneMatch(definition -> flagId.equals(definition.getUnboundFlag().id()))) {
-                throw new NotFoundException("There is no flag '" + flagId + "' (use ?force=true to override)");
-            }
+        if (!isForce(request)) {
+            Flags.getFlag(flagId).orElseThrow(() ->
+                    new NotFoundException("There is no flag '" + flagId + "' (use ?force=true to override)"));
         }
 
         return flagId;
     }
+
+    private boolean isForce(HttpRequest request) {
+        return Objects.equals(request.getProperty("force"), "true");
+    }
 }
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 8781ec413d2..71caed77dbe 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
@@ -11,7 +11,7 @@ import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.flags.FetchVector;
 import com.yahoo.vespa.flags.FlagId;
 import com.yahoo.vespa.flags.Flags;
-import com.yahoo.vespa.flags.UnboundFlag;
+import com.yahoo.vespa.flags.UnboundBooleanFlag;
 import org.junit.Test;
 
 import java.io.ByteArrayInputStream;
@@ -21,6 +21,7 @@ import java.util.stream.Stream;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertEquals;
 
@@ -28,11 +29,11 @@ import static org.junit.Assert.assertEquals;
  * @author hakonhall
  */
 public class FlagsHandlerTest {
-    private static final UnboundFlag<Boolean> FLAG1 =
-            Flags.defineBoolean("id1", false, "desc1", "mod1");
-    private static final UnboundFlag<Boolean> FLAG2 =
-            Flags.defineBoolean("id2", true, "desc2", "mod2",
-                    FetchVector.Dimension.HOSTNAME, FetchVector.Dimension.APPLICATION_ID);
+    private static final UnboundBooleanFlag FLAG1 = Flags.defineFeatureFlag(
+            "id1", false, "desc1", "mod1");
+    private static final UnboundBooleanFlag FLAG2 = Flags.defineFeatureFlag(
+            "id2", true, "desc2", "mod2",
+            FetchVector.Dimension.HOSTNAME, FetchVector.Dimension.APPLICATION_ID);
 
     private static final String FLAGS_V1_URL = "https://foo.com:4443/flags/v1";
 
@@ -54,7 +55,7 @@ public class FlagsHandlerTest {
     public void testDefined() {
         try (Flags.Replacer replacer = Flags.clearFlagsForTesting()) {
             fixUnusedWarning(replacer);
-            Flags.defineBoolean("id", false, "desc", "mod", FetchVector.Dimension.HOSTNAME);
+            Flags.defineFeatureFlag("id", false, "desc", "mod", FetchVector.Dimension.HOSTNAME);
             verifySuccessfulRequest(Method.GET, "/defined", "",
                     "{\"id\":{\"description\":\"desc\",\"modification-effect\":\"mod\",\"dimensions\":[\"hostname\"]}}");
         }
@@ -153,32 +154,33 @@ public class FlagsHandlerTest {
 
     @Test
     public void testForcing() {
-        FlagId undefinedFlagId = new FlagId("undef");
-        HttpResponse response = handle(Method.PUT, "/data/" + undefinedFlagId, "");
+        assertThat(handle(Method.PUT, "/data/" + new FlagId("undef"), "", 404),
+                containsString("There is no flag 'undef'"));
 
-        assertEquals(404, response.getStatus());
-        assertEquals("application/json", response.getContentType());
+        assertThat(handle(Method.PUT, "/data/" + new FlagId("undef") + "?force=true", "", 400),
+                containsString("No content to map due to end-of-input"));
 
-    }
+        assertThat(handle(Method.PUT, "/data/" + FLAG1.id(), "{}", 400),
+                containsString("Flag ID missing"));
 
-    private void verifySuccessfulRequest(Method method, String pathSuffix, String requestBody, String expectedResponseBody) {
-        HttpResponse response = handle(method, pathSuffix, requestBody);
+        assertThat(handle(Method.PUT, "/data/" + FLAG1.id(), "{\"id\": \"id1\",\"rules\": [{\"value\":\"string\"}]}", 400),
+                containsString("Wrong type of JsonNode: STRING"));
 
-        assertEquals(200, response.getStatus());
-        assertEquals("application/json", response.getContentType());
-        String actualResponse = uncheck(() -> SessionHandlerTest.getRenderedString(response));
+        assertThat(handle(Method.PUT, "/data/" + FLAG1.id() + "?force=true", "{\"id\": \"id1\",\"rules\": [{\"value\":\"string\"}]}", 200),
+                is(""));
+    }
 
-        assertThat(actualResponse, is(expectedResponseBody));
+    private void verifySuccessfulRequest(Method method, String pathSuffix, String requestBody, String expectedResponseBody) {
+        assertThat(handle(method, pathSuffix, requestBody, 200), is(expectedResponseBody));
     }
 
-    private HttpResponse handle(Method method, String pathSuffix, String requestBody) {
+    private String handle(Method method, String pathSuffix, String requestBody, int expectedStatus) {
         String uri = FLAGS_V1_URL + pathSuffix;
         HttpRequest request = HttpRequest.createTestRequest(uri, method, makeInputStream(requestBody));
-        return handler.handle(request);
-    }
-
-    private String makeUrl(String component) {
-        return FLAGS_V1_URL + "/" + component;
+        HttpResponse response = handler.handle(request);
+        assertEquals(expectedStatus, response.getStatus());
+        assertEquals("application/json", response.getContentType());
+        return uncheck(() -> SessionHandlerTest.getRenderedString(response));
     }
 
     private InputStream makeInputStream(String content) {