Split ConfigOption read into two steps: parse() and convert() (#40)

also fix ConfigConvOption/ConfigListConvOption read bug

fix: github.com/hugegraph/hugegraph/issues/774
Change-Id: I716c9f187128be9b0d173f152da05e5bdc2208a3

pom.xml

@@ -6,7 +6,7 @@
 
     <groupId>com.baidu.hugegraph</groupId>
     <artifactId>hugegraph-common</artifactId>
-    <version>1.6.16</version>
+    <version>1.7.0</version>
 
     <name>hugegraph-common</name>
     <url>https://github.com/hugegraph/hugegraph-common</url>
@@ -218,7 +218,7 @@
                         <manifestEntries>
                             <!-- Must be on one line, otherwise the automatic
                                  upgrade script cannot replace the version number -->
-                            <Implementation-Version>1.6.16.0</Implementation-Version>
+                            <Implementation-Version>1.7.0.0</Implementation-Version>
                         </manifestEntries>
                     </archive>
                 </configuration>

src/main/java/com/baidu/hugegraph/config/ConfigConvOption.java

@@ -21,27 +21,29 @@
 
 import java.util.function.Function;
 
+import com.baidu.hugegraph.util.E;
 import com.google.common.base.Predicate;
 
-public class ConfigConvOption<R> extends TypedOption<String, R> {
+public class ConfigConvOption<T, R> extends TypedOption<T, R> {
 
-    private final Function<String, R> converter;
+    private final Function<T, R> converter;
 
-    public ConfigConvOption(String name, String desc, Predicate<String> pred,
-                            Function<String, R> convert, String value) {
+    public ConfigConvOption(String name, String desc, Predicate<T> pred,
+                            Function<T, R> convert, T value) {
         this(name, false, desc, pred, convert, value);
     }
 
+    @SuppressWarnings("unchecked")
     public ConfigConvOption(String name, boolean required, String desc,
-                            Predicate<String> pred, Function<String, R> convert,
-                            String value) {
-        super(name, required, desc, pred, String.class, value);
+                            Predicate<T> pred, Function<T, R> convert,
+                            T value) {
+        super(name, required, desc, pred, (Class<T>) value.getClass(), value);
+        E.checkNotNull(convert, "convert");
         this.converter = convert;
     }
 
     @Override
-    public R convert(Object value) {
-        assert value instanceof String;
-        return this.converter.apply((String) value);
+    public R convert(T value) {
+        return this.converter.apply(value);
     }
 }

src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java

@@ -35,15 +35,8 @@ public class ConfigListConvOption<T, R> extends TypedOption<List<T>, List<R>> {
     @SuppressWarnings("unchecked")
     public ConfigListConvOption(String name, String desc,
                                 Predicate<List<T>> pred, Function<T, R> convert,
-                                T value) {
-        this(name, desc, pred, convert, (Class<T>) value.getClass(), value);
-    }
-
-    @SuppressWarnings("unchecked")
-    public ConfigListConvOption(String name, String desc,
-                                Predicate<List<T>> pred, Function<T, R> convert,
-                                Class<T> clazz, T... values) {
-        this(name, false, desc, pred, convert, clazz, Arrays.asList(values));
+                                T... values) {
+        this(name, false, desc, pred, convert, null, Arrays.asList(values));
     }
 
     @SuppressWarnings("unchecked")
@@ -52,21 +45,33 @@ public ConfigListConvOption(String name, boolean required, String desc,
                                 Class<T> clazz, List<T> values) {
         super(name, required, desc, pred,
               (Class<List<T>>) values.getClass(), values);
+        E.checkNotNull(convert, "convert");
+        if (clazz == null && values.size() > 0) {
+            clazz = (Class<T>) values.get(0).getClass();
+        }
         E.checkArgumentNotNull(clazz, "Element class can't be null");
         this.elemClass = clazz;
         this.converter = convert;
     }
 
     @Override
-    public List<R> convert(Object value) {
-        List<T> results = ConfigListOption.convert(value, part -> {
-            return super.convert(part, this.elemClass);
+    protected boolean forList() {
+        return true;
+    }
+
+    @Override
+    protected List<T> parse(Object value) {
+        return ConfigListOption.convert(value, part -> {
+            return this.parse(part, this.elemClass);
         });
+    }
 
-        List<R> enums = new ArrayList<>(results.size());
-        for (T elem : results) {
-            enums.add(this.converter.apply(elem));
+    @Override
+    public List<R> convert(List<T> values) {
+        List<R> results = new ArrayList<>(values.size());
+        for (T value : values) {
+            results.add(this.converter.apply(value));
         }
-        return enums;
+        return results;
     }
 }

src/main/java/com/baidu/hugegraph/config/ConfigListOption.java

@@ -33,15 +33,8 @@ public class ConfigListOption<T> extends ConfigOption<List<T>> {
 
     @SuppressWarnings("unchecked")
     public ConfigListOption(String name, String desc,
-                            Predicate<List<T>> pred, T value) {
-        this(name, desc, pred, (Class<T>) value.getClass(), value);
-    }
-
-    @SuppressWarnings("unchecked")
-    public ConfigListOption(String name, String desc,
-                            Predicate<List<T>> pred, Class<T> clazz,
-                            T... values) {
-        this(name, false, desc, pred, clazz, Arrays.asList(values));
+                            Predicate<List<T>> pred, T... values) {
+        this(name, false, desc, pred, null, Arrays.asList(values));
     }
 
     @SuppressWarnings("unchecked")
@@ -50,13 +43,21 @@ public ConfigListOption(String name, boolean required, String desc,
                             List<T> values) {
         super(name, required, desc, pred,
               (Class<List<T>>) values.getClass(), values);
+        if (clazz == null && values.size() > 0) {
+            clazz = (Class<T>) values.get(0).getClass();
+        }
         E.checkArgumentNotNull(clazz, "Element class can't be null");
         this.elemClass = clazz;
     }
 
     @Override
-    public List<T> convert(Object value) {
-        return convert(value, part -> super.convert(part, this.elemClass));
+    protected boolean forList() {
+        return true;
+    }
+
+    @Override
+    protected List<T> parse(Object value) {
+        return convert(value, part -> this.parse(part, this.elemClass));
     }
 
     @SuppressWarnings("unchecked")

src/main/java/com/baidu/hugegraph/config/HugeConfig.java

@@ -48,9 +48,6 @@ public HugeConfig(Configuration config) {
         Iterator<String> keys = config.getKeys();
         while (keys.hasNext()) {
             String key = keys.next();
-            if (key.contains("..")) {
-                key = key.replace("..", ".");
-            }
             this.addProperty(key, config.getProperty(key));
         }
         this.checkRequiredOptions();
@@ -145,17 +142,7 @@ private Object validateOption(String key, Object value) {
                         "Invalid value for key '%s': %s", key, value);
 
         TypedOption<?, ?> option = OptionSpace.get(key);
-        Class<?> dataType = option.dataType();
-
-        if (List.class.isAssignableFrom(dataType)) {
-            E.checkState(option instanceof ConfigListOption,
-                         "List option must be registered with " +
-                         "class ConfigListOption");
-        }
-
-        value = option.convert(value);
-        option.check(value);
-        return value;
+        return option.parseConvert(value);
     }
 
     private void checkRequiredOptions() {

src/main/java/com/baidu/hugegraph/config/OptionHolder.java

@@ -40,6 +40,10 @@ public OptionHolder() {
 
     protected void registerOptions() {
         for (Field field : this.getClass().getFields()) {
+            if (!TypedOption.class.isAssignableFrom(field.getType())) {
+                // Skip if not option
+                continue;
+            }
             try {
                 TypedOption<?, ?> option = (TypedOption<?, ?>) field.get(this);
                 // Fields of subclass first, don't overwrite by superclass

src/main/java/com/baidu/hugegraph/config/OptionSpace.java

@@ -21,12 +21,15 @@
 
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.slf4j.Logger;
 
+import com.baidu.hugegraph.util.E;
 import com.baidu.hugegraph.util.Log;
 
 public final class OptionSpace {
@@ -62,7 +65,15 @@ public static void register(String module, String holder) {
         Exception exception = null;
         try {
             Method method = clazz.getMethod(INSTANCE_METHOD);
+            if (!Modifier.isStatic(method.getModifiers())) {
+                throw new NoSuchMethodException(INSTANCE_METHOD);
+            }
             instance = (OptionHolder) method.invoke(null);
+            if (instance == null) {
+                exception = new ConfigException(
+                                "Returned null from %s() method",
+                                INSTANCE_METHOD);
+            }
         } catch (NoSuchMethodException e) {
             LOG.warn("Class {} does not has static method {}.",
                      holder, INSTANCE_METHOD);
@@ -91,17 +102,18 @@ public static void register(String module, OptionHolder holder) {
             LOG.warn("Already registered option holder: {} ({})",
                      module, holders.get(module));
         }
+        E.checkArgumentNotNull(holder, "OptionHolder can't be null");
         holders.put(module, holder.getClass());
         options.putAll(holder.options());
         LOG.debug("Registered options for OptionHolder: {}",
                   holder.getClass().getSimpleName());
     }
 
     public static Set<String> keys() {
-        return options.keySet();
+        return Collections.unmodifiableSet(options.keySet());
     }
 
-    public static Boolean containKey(String key) {
+    public static boolean containKey(String key) {
         return options.containsKey(key);
     }
 

src/main/java/com/baidu/hugegraph/config/TypedOption.java

@@ -112,14 +112,24 @@ public R defaultValue() {
         return this.convert(this.defaultValue);
     }
 
+    public R parseConvert(Object value) {
+        T parsed = this.parse(value);
+        this.check(parsed);
+        return this.convert(parsed);
+    }
+
     @SuppressWarnings("unchecked")
-    public R convert(Object value) {
-        return (R) this.convert(value, this.dataType);
+    protected T parse(Object value) {
+        return (T) this.parse(value, this.dataType);
     }
 
-    public Object convert(Object value, Class<?> dataType) {
+    protected Object parse(Object value, Class<?> dataType) {
         if (dataType.equals(String.class)) {
             return value;
+        } else if (List.class.isAssignableFrom(dataType)) {
+            E.checkState(this.forList(),
+                         "List option can't be registered with class %s",
+                         this.getClass().getSimpleName());
         }
 
         // Use PropertyConverter method `toXXX` convert value
@@ -138,20 +148,35 @@ public Object convert(Object value, Class<?> dataType) {
         }
     }
 
-    public void check(Object value) {
+    protected void check(Object value) {
         E.checkNotNull(value, "value", this.name);
-        E.checkArgument(this.dataType.isInstance(value),
-                        "Invalid type of value '%s' for option '%s'",
-                        value, this.name);
+        if (!this.dataType.isInstance(value)) {
+            throw new ConfigException(
+                      "Invalid type of value '%s' for option '%s', " +
+                      "expect type %s but got %s", value, this.name,
+                      this.dataType.getSimpleName(),
+                      value.getClass().getSimpleName());
+        }
+
         if (this.checkFunc != null) {
             @SuppressWarnings("unchecked")
             T result = (T) value;
-            E.checkArgument(this.checkFunc.apply(result),
-                            "Invalid option value for '%s': %s",
-                            this.name, value);
+            if (!this.checkFunc.apply(result)) {
+                throw new ConfigException("Invalid option value for '%s': %s",
+                                          this.name, value);
+            }
         }
     }
 
+    @SuppressWarnings("unchecked")
+    protected R convert(T value) {
+        return (R) value;
+    }
+
+    protected boolean forList() {
+        return false;
+    }
+
     @Override
     public String toString() {
         return String.format("[%s]%s=%s", this.dataType.getSimpleName(),

src/main/java/com/baidu/hugegraph/version/CommonVersion.java

@@ -27,5 +27,5 @@ public class CommonVersion {
 
     // The second parameter of Version.of() is for all-in-one JAR
     public static final Version VERSION = Version.of(CommonVersion.class,
-                                                     "1.6.16");
+                                                     "1.7.0");
 }

src/test/java/com/baidu/hugegraph/unit/UnitTestSuite.java

@@ -24,9 +24,10 @@
 
 import com.baidu.hugegraph.testutil.AssertTest;
 import com.baidu.hugegraph.testutil.WhiteboxTest;
-import com.baidu.hugegraph.unit.concurrent.RowLockTest;
 import com.baidu.hugegraph.unit.concurrent.LockGroupTest;
+import com.baidu.hugegraph.unit.concurrent.RowLockTest;
 import com.baidu.hugegraph.unit.config.HugeConfigTest;
+import com.baidu.hugegraph.unit.config.OptionSpaceTest;
 import com.baidu.hugegraph.unit.date.SafeDateFormatTest;
 import com.baidu.hugegraph.unit.event.EventHubTest;
 import com.baidu.hugegraph.unit.iterator.ExtendableIteratorTest;
@@ -60,6 +61,7 @@
     LockGroupTest.class,
     RowLockTest.class,
     HugeConfigTest.class,
+    OptionSpaceTest.class,
     SafeDateFormatTest.class,
     EventHubTest.class,
     PerfUtilTest.class,