Remove Optional classes

After review & new ideas:
 - All Optional classes have been removed: They must take a default argument
   to avoid having clients test whether the flag has been set or not, in order
   to allow local file flag source to undo a file flag setting from config
   server.
 - FeatureFlag is now backed by FlagSource::getString, and
   FlagSource::hasFeature has been removed.
 - FeatureFlag now allows default-true to facilitate default-enabled features

7 files changed, 72 insertions, 148 deletions

diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FeatureFlag.java b/flags/src/main/java/com/yahoo/vespa/flags/FeatureFlag.java
index 957faee41cc..164b0ad96e7 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/FeatureFlag.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/FeatureFlag.java
@@ -4,20 +4,21 @@ package com.yahoo.vespa.flags;
 import java.util.function.Function;
 
 /**
- * A FeatureFlag is true only if set (enabled).
+ * A FeatureFlag defaults to false (but see {@link #defaultToTrue()}).
  *
  * @author hakonhall
  */
 public class FeatureFlag implements Flag {
+    private final boolean defaultValue;
     private final FlagId id;
     private final FlagSource source;
 
-    public static Function<FlagSource, FeatureFlag> createUnbound(String flagId) {
-        return createUnbound(new FlagId(flagId));
+    public static Function<FlagSource, FeatureFlag> createUnbound(String flagId, boolean defaultValue) {
+        return createUnbound(new FlagId(flagId), defaultValue);
     }
 
-    public static Function<FlagSource, FeatureFlag> createUnbound(FlagId flagId) {
-        return source -> new FeatureFlag(flagId, source);
+    public static Function<FlagSource, FeatureFlag> createUnbound(FlagId id, boolean defaultValue) {
+        return source -> new FeatureFlag(id, defaultValue, source);
     }
 
     public FeatureFlag(String flagId, FlagSource source) {
@@ -25,23 +26,44 @@ public class FeatureFlag implements Flag {
     }
 
     public FeatureFlag(FlagId id, FlagSource source) {
+        this(id, false, source);
+    }
+
+    private FeatureFlag(FlagId id, boolean defaultValue, FlagSource source) {
         this.id = id;
+        this.defaultValue = defaultValue;
         this.source = source;
     }
 
+    public FeatureFlag defaultToTrue() {
+        return new FeatureFlag(id, true, source);
+    }
+
     @Override
     public FlagId id() {
         return id;
     }
 
-    public boolean isSet() {
-        return source.hasFeature(id);
+    public boolean value() {
+        return source.getString(id).map(FeatureFlag::booleanFromString).orElse(defaultValue);
+    }
+
+    private static boolean booleanFromString(String string) {
+        String canonicalString = string.trim().toLowerCase();
+        if (canonicalString.equals("true")) {
+            return true;
+        } else if (canonicalString.equals("false")) {
+            return false;
+        }
+
+        throw new IllegalArgumentException("Unable to convert to true or false: '" + string + "'");
     }
 
     @Override
     public String toString() {
-        return "FeatureFlag{" +
+        return "IntFlag{" +
                 "id=" + id +
+                ", defaultValue=" + defaultValue +
                 ", source=" + source +
                 '}';
     }
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FileFlagSource.java b/flags/src/main/java/com/yahoo/vespa/flags/FileFlagSource.java
index 93413c74fa9..25df93843d8 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/FileFlagSource.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/FileFlagSource.java
@@ -36,11 +36,6 @@ public class FileFlagSource implements FlagSource {
     }
 
     @Override
-    public boolean hasFeature(FlagId id) {
-        return Files.exists(getPath(id));
-    }
-
-    @Override
     public Optional<String> getString(FlagId id) {
         return getBytes(id).map(bytes -> new String(bytes, StandardCharsets.UTF_8));
     }
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java b/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java
index 56f4b5ee0ae..509db40c4d4 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java
@@ -7,9 +7,6 @@ import java.util.Optional;
  * @author hakonhall
  */
 public interface FlagSource {
-    /** Whether the source has the feature flag with the given id. */
-    boolean hasFeature(FlagId id);
-
-    /** The String value of a flag. */
+    /** The String value of a flag, or empty if not set by the source. */
     Optional<String> getString(FlagId id);
 }
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/OptionalJacksonFlag.java b/flags/src/main/java/com/yahoo/vespa/flags/OptionalJacksonFlag.java
deleted file mode 100644
index 9b25a5d6786..00000000000
--- a/flags/src/main/java/com/yahoo/vespa/flags/OptionalJacksonFlag.java
+++ /dev/null
@@ -1,60 +0,0 @@
-// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.flags;
-
-import com.fasterxml.jackson.databind.ObjectMapper;
-
-import java.util.Optional;
-import java.util.function.Function;
-
-import static com.yahoo.yolean.Exceptions.uncheck;
-
-/**
- * @author hakonhall
- */
-public class OptionalJacksonFlag<T> implements Flag {
-    private final static ObjectMapper mapper = new ObjectMapper();
-
-    private final FlagId id;
-    private final Class<T> jacksonClass;
-    private final FlagSource source;
-
-    public static <T> Function<FlagSource, OptionalJacksonFlag<T>> createUnbound(String flagId, Class<T> jacksonClass) {
-        return createUnbound(new FlagId(flagId), jacksonClass);
-    }
-
-    public static <T> Function<FlagSource, OptionalJacksonFlag<T>> createUnbound(FlagId id, Class<T> jacksonClass) {
-        return source -> new OptionalJacksonFlag<>(id, jacksonClass, source);
-    }
-
-    public OptionalJacksonFlag(String flagId, Class<T> jacksonClass, FlagSource source) {
-        this(new FlagId(flagId), jacksonClass, source);
-    }
-
-    public OptionalJacksonFlag(FlagId id, Class<T> jacksonClass, FlagSource source) {
-        this.id = id;
-        this.jacksonClass = jacksonClass;
-        this.source = source;
-    }
-
-    @Override
-    public FlagId id() {
-        return id;
-    }
-
-    public Optional<T> value() {
-        return source.getString(id).map(string -> uncheck(() -> mapper.readValue(string, jacksonClass)));
-    }
-
-    public JacksonFlag<T> withDefault(T defaultValue) {
-        return new JacksonFlag<T>(id, jacksonClass, defaultValue, source);
-    }
-
-    @Override
-    public String toString() {
-        return "OptionalJacksonFlag{" +
-                "id=" + id +
-                ", jacksonClass=" + jacksonClass +
-                ", source=" + source +
-                '}';
-    }
-}
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/OptionalStringFlag.java b/flags/src/main/java/com/yahoo/vespa/flags/OptionalStringFlag.java
deleted file mode 100644
index 3e25b08cf9d..00000000000
--- a/flags/src/main/java/com/yahoo/vespa/flags/OptionalStringFlag.java
+++ /dev/null
@@ -1,53 +0,0 @@
-// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.flags;
-
-import java.util.Optional;
-import java.util.function.Function;
-
-/**
- * An OptionalStringFlag is a flag which is either not set (empty), or set (String present).
- *
- * @author hakonhall
- */
-public class OptionalStringFlag implements Flag {
-    private final FlagId id;
-    private final FlagSource source;
-
-    public static Function<FlagSource, OptionalStringFlag> createUnbound(String flagId) {
-        return createUnbound(new FlagId(flagId));
-    }
-
-    public static Function<FlagSource, OptionalStringFlag> createUnbound(FlagId id) {
-        return source -> new OptionalStringFlag(id, source);
-    }
-
-    public OptionalStringFlag(String flagId, FlagSource source) {
-        this(new FlagId(flagId), source);
-    }
-
-    public OptionalStringFlag(FlagId id, FlagSource source) {
-        this.id = id;
-        this.source = source;
-    }
-
-    @Override
-    public FlagId id() {
-        return id;
-    }
-
-    public StringFlag bindDefault(String defaultValue) {
-        return new StringFlag(id, defaultValue, source);
-    }
-
-    public Optional<String> value() {
-        return source.getString(id);
-    }
-
-    @Override
-    public String toString() {
-        return "OptionalStringFlag{" +
-                "id=" + id +
-                ", source=" + source +
-                '}';
-    }
-}
diff --git a/flags/src/test/java/com/yahoo/vespa/flags/FileFlagSourceTest.java b/flags/src/test/java/com/yahoo/vespa/flags/FileFlagSourceTest.java
index e422057f5fe..4b7c8c0bbb3 100644
--- a/flags/src/test/java/com/yahoo/vespa/flags/FileFlagSourceTest.java
+++ b/flags/src/test/java/com/yahoo/vespa/flags/FileFlagSourceTest.java
@@ -9,41 +9,70 @@ import java.nio.file.FileSystem;
 import java.nio.file.Files;
 import java.nio.file.Path;
 
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 public class FileFlagSourceTest {
     private final FileSystem fileSystem = TestFileSystem.create();
     private final FileFlagSource source = new FileFlagSource(fileSystem);
+    private final FlagId id = new FlagId("foo");
 
     @Test
-    public void absentThenSet() throws IOException {
-        FlagId id = new FlagId("foo");
+    public void testFeatureLikeFlags() throws IOException {
         FeatureFlag featureFlag = new FeatureFlag(id, source);
+        FeatureFlag byDefaultTrue = featureFlag.defaultToTrue();
+
+        assertFalse(featureFlag.value());
+        assertTrue(byDefaultTrue.value());
+
+        writeFlag(id.toString(), "True\n");
+
+        assertTrue(featureFlag.value());
+        assertTrue(byDefaultTrue.value());
+
+        writeFlag(id.toString(), "False\n");
+
+        assertFalse(featureFlag.value());
+        assertFalse(byDefaultTrue.value());
+    }
+
+    @Test
+    public void testIntegerLikeFlags() throws IOException {
         StringFlag stringFlag = new StringFlag(id, "default", source);
-        OptionalStringFlag optionalStringFlag = new OptionalStringFlag(id, source);
         IntFlag intFlag = new IntFlag(id, -1, source);
         LongFlag longFlag = new LongFlag(id, -2L, source);
 
-        assertFalse(source.hasFeature(id));
         assertFalse(source.getString(id).isPresent());
-        assertFalse(featureFlag.isSet());
         assertEquals("default", stringFlag.value());
-        assertFalse(optionalStringFlag.value().isPresent());
         assertEquals(-1, intFlag.value());
         assertEquals(-2L, longFlag.value());
 
-        Path featurePath = fileSystem.getPath(FileFlagSource.FLAGS_DIRECTORY).resolve(id.toString());
-        Files.createDirectories(featurePath.getParent());
-        Files.write(featurePath, "1\n".getBytes());
+        writeFlag(id.toString(), "1\n");
 
-        assertTrue(source.hasFeature(id));
         assertTrue(source.getString(id).isPresent());
-        assertTrue(featureFlag.isSet());
         assertEquals("1\n", stringFlag.value());
-        assertEquals("1\n", optionalStringFlag.value().get());
         assertEquals(1, intFlag.value());
         assertEquals(1L, longFlag.value());
     }
+
+    @Test
+    public void parseFailure() throws IOException {
+        FeatureFlag featureFlag = new FeatureFlag(id, source);
+        writeFlag(featureFlag.id().toString(), "garbage");
+
+        try {
+            featureFlag.value();
+        } catch (IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("garbage"));
+        }
+    }
+
+    private void writeFlag(String flagId, String value) throws IOException {
+        Path featurePath = fileSystem.getPath(FileFlagSource.FLAGS_DIRECTORY).resolve(flagId);
+        Files.createDirectories(featurePath.getParent());
+        Files.write(featurePath, value.getBytes());
+    }
 }
\ No newline at end of file
diff --git a/flags/src/test/java/com/yahoo/vespa/flags/JacksonFlagTest.java b/flags/src/test/java/com/yahoo/vespa/flags/JacksonFlagTest.java
index e4424d9886a..8bd486f5b47 100644
--- a/flags/src/test/java/com/yahoo/vespa/flags/JacksonFlagTest.java
+++ b/flags/src/test/java/com/yahoo/vespa/flags/JacksonFlagTest.java
@@ -9,8 +9,6 @@ import java.util.Objects;
 import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -19,7 +17,6 @@ public class JacksonFlagTest {
     private final ExampleJacksonClass defaultValue = new ExampleJacksonClass();
     private final FlagSource source = mock(FlagSource.class);
     private final JacksonFlag<ExampleJacksonClass> jacksonFlag = new JacksonFlag<>(id.toString(), ExampleJacksonClass.class, defaultValue, source);
-    private final OptionalJacksonFlag<ExampleJacksonClass> optionalJacksonFlag = new OptionalJacksonFlag<>(id, ExampleJacksonClass.class, source);
 
     @Test
     public void unsetThenSet() {
@@ -28,7 +25,6 @@ public class JacksonFlagTest {
         assertEquals(1, value.integer);
         assertEquals("2", value.string);
         assertEquals("3", value.dummy);
-        assertFalse(optionalJacksonFlag.value().isPresent());
 
         when(source.getString(id)).thenReturn(Optional.of("{\"integer\": 4, \"string\": \"foo\", \"stray\": 6}"));
         value = jacksonFlag.value();
@@ -36,8 +32,6 @@ public class JacksonFlagTest {
         assertEquals("foo", value.string);
         assertEquals("3", value.dummy);
 
-        assertTrue(optionalJacksonFlag.value().isPresent());
-        value = optionalJacksonFlag.value().get();
         assertEquals(4, value.integer);
         assertEquals("foo", value.string);
         assertEquals("3", value.dummy);