From a6fe4c33239408189245a4680a22b61d7c0b003c Mon Sep 17 00:00:00 2001 From: Zhangmei Li Date: Sun, 1 Dec 2019 23:48:37 +0800 Subject: [PATCH 1/5] Split ConfigOption read into two steps: parse() and convert() also fix ConfigConvOption/ConfigListConvOption read bug fix: github.com/hugegraph/hugegraph/issues/774 Change-Id: I716c9f187128be9b0d173f152da05e5bdc2208a3 --- pom.xml | 4 +- .../hugegraph/config/ConfigConvOption.java | 22 ++++++----- .../config/ConfigListConvOption.java | 37 ++++++++++--------- .../hugegraph/config/ConfigListOption.java | 21 +++++------ .../baidu/hugegraph/config/HugeConfig.java | 12 +----- .../baidu/hugegraph/config/TypedOption.java | 33 ++++++++++++++--- .../hugegraph/version/CommonVersion.java | 2 +- .../hugegraph/unit/config/HugeConfigTest.java | 17 +++++---- .../com/baidu/hugegraph/unit/config/test.conf | 3 ++ 9 files changed, 85 insertions(+), 66 deletions(-) diff --git a/pom.xml b/pom.xml index 034fce13e..cf647667d 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ com.baidu.hugegraph hugegraph-common - 1.6.16 + 1.7.0 hugegraph-common https://github.com/hugegraph/hugegraph-common @@ -218,7 +218,7 @@ - 1.6.16.0 + 1.7.0.0 diff --git a/src/main/java/com/baidu/hugegraph/config/ConfigConvOption.java b/src/main/java/com/baidu/hugegraph/config/ConfigConvOption.java index 9f354d086..f67d0e478 100644 --- a/src/main/java/com/baidu/hugegraph/config/ConfigConvOption.java +++ b/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 extends TypedOption { +public class ConfigConvOption extends TypedOption { - private final Function converter; + private final Function converter; - public ConfigConvOption(String name, String desc, Predicate pred, - Function convert, String value) { + public ConfigConvOption(String name, String desc, Predicate pred, + Function convert, T value) { this(name, false, desc, pred, convert, value); } + @SuppressWarnings("unchecked") public ConfigConvOption(String name, boolean required, String desc, - Predicate pred, Function convert, - String value) { - super(name, required, desc, pred, String.class, value); + Predicate pred, Function convert, + T value) { + super(name, required, desc, pred, (Class) 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); } } diff --git a/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java b/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java index 6d5fa9589..353c2b692 100644 --- a/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java +++ b/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java @@ -35,15 +35,9 @@ public class ConfigListConvOption extends TypedOption, List> { @SuppressWarnings("unchecked") public ConfigListConvOption(String name, String desc, Predicate> pred, Function convert, - T value) { - this(name, desc, pred, convert, (Class) value.getClass(), value); - } - - @SuppressWarnings("unchecked") - public ConfigListConvOption(String name, String desc, - Predicate> pred, Function convert, - Class clazz, T... values) { - this(name, false, desc, pred, convert, clazz, Arrays.asList(values)); + T... values) { + this(name, false, desc, pred, convert, + (Class) values[0].getClass(), Arrays.asList(values)); } @SuppressWarnings("unchecked") @@ -52,21 +46,30 @@ public ConfigListConvOption(String name, boolean required, String desc, Class clazz, List values) { super(name, required, desc, pred, (Class>) values.getClass(), values); - E.checkArgumentNotNull(clazz, "Element class can't be null"); + E.checkNotNull(clazz, "element class"); + E.checkNotNull(convert, "convert"); this.elemClass = clazz; this.converter = convert; } @Override - public List convert(Object value) { - List results = ConfigListOption.convert(value, part -> { - return super.convert(part, this.elemClass); + protected boolean forList() { + return true; + } + + @Override + protected List parse(Object value) { + return ConfigListOption.convert(value, part -> { + return this.parse(part, this.elemClass); }); + } - List enums = new ArrayList<>(results.size()); - for (T elem : results) { - enums.add(this.converter.apply(elem)); + @Override + public List convert(List values) { + List results = new ArrayList<>(values.size()); + for (T value : values) { + results.add(this.converter.apply(value)); } - return enums; + return results; } } diff --git a/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java b/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java index 9add0bc94..35d21446f 100644 --- a/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java +++ b/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java @@ -33,15 +33,9 @@ public class ConfigListOption extends ConfigOption> { @SuppressWarnings("unchecked") public ConfigListOption(String name, String desc, - Predicate> pred, T value) { - this(name, desc, pred, (Class) value.getClass(), value); - } - - @SuppressWarnings("unchecked") - public ConfigListOption(String name, String desc, - Predicate> pred, Class clazz, - T... values) { - this(name, false, desc, pred, clazz, Arrays.asList(values)); + Predicate> pred, T... values) { + this(name, false, desc, pred, + (Class) values[0].getClass(), Arrays.asList(values)); } @SuppressWarnings("unchecked") @@ -55,8 +49,13 @@ public ConfigListOption(String name, boolean required, String desc, } @Override - public List convert(Object value) { - return convert(value, part -> super.convert(part, this.elemClass)); + protected boolean forList() { + return true; + } + + @Override + protected List parse(Object value) { + return convert(value, part -> this.parse(part, this.elemClass)); } @SuppressWarnings("unchecked") diff --git a/src/main/java/com/baidu/hugegraph/config/HugeConfig.java b/src/main/java/com/baidu/hugegraph/config/HugeConfig.java index 7dc6ec8c1..66dae0e6d 100644 --- a/src/main/java/com/baidu/hugegraph/config/HugeConfig.java +++ b/src/main/java/com/baidu/hugegraph/config/HugeConfig.java @@ -145,17 +145,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() { diff --git a/src/main/java/com/baidu/hugegraph/config/TypedOption.java b/src/main/java/com/baidu/hugegraph/config/TypedOption.java index 91fb7ea33..182bcaf2b 100644 --- a/src/main/java/com/baidu/hugegraph/config/TypedOption.java +++ b/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,11 +148,13 @@ 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); + "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; @@ -152,6 +164,15 @@ public void check(Object 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(), diff --git a/src/main/java/com/baidu/hugegraph/version/CommonVersion.java b/src/main/java/com/baidu/hugegraph/version/CommonVersion.java index 0e4d2be9f..27048fd3a 100644 --- a/src/main/java/com/baidu/hugegraph/version/CommonVersion.java +++ b/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"); } diff --git a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java index 131cedc9c..7e2242aaa 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java +++ b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java @@ -105,15 +105,15 @@ public void testHugeConfig() throws Exception { Assert.assertEquals(true, config.get(TestOptions.bool)); - Assert.assertEquals(WeekDay.WEDNESDAY, config.get(TestOptions.weekday)); - Assert.assertEquals(Arrays.asList(WeekDay.SATURDAY, WeekDay.SUNDAY), - config.get(TestOptions.weekdays)); - Assert.assertEquals(Arrays.asList("list-value1", "list-value2"), config.get(TestOptions.list)); Assert.assertEquals(ImmutableMap.of("key1", "value1", "key2", "value2"), config.getMap(TestOptions.map)); + + Assert.assertEquals(WeekDay.WEDNESDAY, config.get(TestOptions.weekday)); + Assert.assertEquals(Arrays.asList(WeekDay.SATURDAY, WeekDay.SUNDAY), + config.get(TestOptions.weekdays)); } @Test @@ -140,6 +140,10 @@ public void testHugeConfigWithFile() throws Exception { Assert.assertEquals(ImmutableMap.of("key1", "value1", "key3", "value3"), config.getMap(TestOptions.map)); + + Assert.assertEquals(WeekDay.SUNDAY, config.get(TestOptions.weekday)); + Assert.assertEquals(Arrays.asList(WeekDay.SATURDAY, WeekDay.FRIDAY), + config.get(TestOptions.weekdays)); } @Test @@ -258,7 +262,7 @@ public static synchronized TestOptions instance() { true ); - public static final ConfigConvOption weekday = + public static final ConfigConvOption weekday = new ConfigConvOption<>( "group1.weekday", "description of group1.weekday", @@ -275,7 +279,6 @@ public static synchronized TestOptions instance() { inValues("SUNDAY", "MONDAY", "TUESDAY", "WEDNESDAY", "THURSDAY", "FRIDAY", "SATURDAY"), WeekDay::valueOf, - String.class, "SATURDAY", "SUNDAY" ); @@ -284,7 +287,6 @@ public static synchronized TestOptions instance() { "group1.list", "description of group1.list", disallowEmpty(), - String.class, "list-value1", "list-value2" ); @@ -293,7 +295,6 @@ public static synchronized TestOptions instance() { "group1.map", "description of group1.map", disallowEmpty(), - String.class, "key1:value1", "key2:value2" ); } diff --git a/src/test/java/com/baidu/hugegraph/unit/config/test.conf b/src/test/java/com/baidu/hugegraph/unit/config/test.conf index bf31c70f6..7c59e7af8 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/test.conf +++ b/src/test/java/com/baidu/hugegraph/unit/config/test.conf @@ -14,3 +14,6 @@ group1.bool=false group1.list=[file-v1, file-v2, file-v3] group1.map=[key1:value1, key3:value3] + +group1.weekday=SUNDAY +group1.weekdays=[SATURDAY, FRIDAY] From 748b46f07c69cf2644f4a51574f1130015b260e1 Mon Sep 17 00:00:00 2001 From: Zhangmei Li Date: Sun, 22 Dec 2019 02:19:45 +0800 Subject: [PATCH 2/5] improve test Change-Id: Id7aed5ed610d310a901b306e1c05f65074190c8d --- .../baidu/hugegraph/config/HugeConfig.java | 3 - .../baidu/hugegraph/config/OptionHolder.java | 4 + .../baidu/hugegraph/config/OptionSpace.java | 12 +- .../baidu/hugegraph/config/TypedOption.java | 20 +- .../baidu/hugegraph/unit/UnitTestSuite.java | 4 +- .../hugegraph/unit/config/HugeConfigTest.java | 245 +++++++++++++++++- .../unit/config/OptionSpaceTest.java | 216 +++++++++++++++ .../unit/config/test-check-error.conf | 1 + .../unit/config/test-list-error.conf | 1 + .../unit/config/test-type-error.conf | 1 + .../com/baidu/hugegraph/unit/config/test.conf | 2 + 11 files changed, 491 insertions(+), 18 deletions(-) create mode 100644 src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java create mode 100644 src/test/java/com/baidu/hugegraph/unit/config/test-check-error.conf create mode 100644 src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf create mode 100644 src/test/java/com/baidu/hugegraph/unit/config/test-type-error.conf diff --git a/src/main/java/com/baidu/hugegraph/config/HugeConfig.java b/src/main/java/com/baidu/hugegraph/config/HugeConfig.java index 66dae0e6d..1c9b16b8e 100644 --- a/src/main/java/com/baidu/hugegraph/config/HugeConfig.java +++ b/src/main/java/com/baidu/hugegraph/config/HugeConfig.java @@ -48,9 +48,6 @@ public HugeConfig(Configuration config) { Iterator keys = config.getKeys(); while (keys.hasNext()) { String key = keys.next(); - if (key.contains("..")) { - key = key.replace("..", "."); - } this.addProperty(key, config.getProperty(key)); } this.checkRequiredOptions(); diff --git a/src/main/java/com/baidu/hugegraph/config/OptionHolder.java b/src/main/java/com/baidu/hugegraph/config/OptionHolder.java index 6b5c398d9..e5c5be6a6 100644 --- a/src/main/java/com/baidu/hugegraph/config/OptionHolder.java +++ b/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 diff --git a/src/main/java/com/baidu/hugegraph/config/OptionSpace.java b/src/main/java/com/baidu/hugegraph/config/OptionSpace.java index 485fb81d2..9b2aef85f 100644 --- a/src/main/java/com/baidu/hugegraph/config/OptionSpace.java +++ b/src/main/java/com/baidu/hugegraph/config/OptionSpace.java @@ -21,12 +21,14 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +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 { @@ -63,6 +65,11 @@ public static void register(String module, String holder) { try { Method method = clazz.getMethod(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,6 +98,7 @@ 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: {}", @@ -98,10 +106,10 @@ public static void register(String module, OptionHolder holder) { } public static Set 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); } diff --git a/src/main/java/com/baidu/hugegraph/config/TypedOption.java b/src/main/java/com/baidu/hugegraph/config/TypedOption.java index 182bcaf2b..57d77c6fa 100644 --- a/src/main/java/com/baidu/hugegraph/config/TypedOption.java +++ b/src/main/java/com/baidu/hugegraph/config/TypedOption.java @@ -150,17 +150,21 @@ protected Object parse(Object value, Class dataType) { 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', " + - "expect type %s but got %s", value, this.name, - this.dataType.getSimpleName(), - value.getClass().getSimpleName()); + 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); + } } } diff --git a/src/test/java/com/baidu/hugegraph/unit/UnitTestSuite.java b/src/test/java/com/baidu/hugegraph/unit/UnitTestSuite.java index a8d597a6e..4d292d626 100644 --- a/src/test/java/com/baidu/hugegraph/unit/UnitTestSuite.java +++ b/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, diff --git a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java index 7e2242aaa..a2621022f 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java +++ b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java @@ -28,6 +28,7 @@ import static com.baidu.hugegraph.config.OptionChecker.rangeInt; import java.util.Arrays; +import java.util.List; import org.apache.commons.configuration.Configuration; import org.apache.commons.configuration.PropertiesConfiguration; @@ -35,6 +36,7 @@ import org.junit.Test; import com.baidu.hugegraph.config.ConfigConvOption; +import com.baidu.hugegraph.config.ConfigException; import com.baidu.hugegraph.config.ConfigListConvOption; import com.baidu.hugegraph.config.ConfigListOption; import com.baidu.hugegraph.config.ConfigOption; @@ -44,18 +46,51 @@ import com.baidu.hugegraph.testutil.Assert; import com.baidu.hugegraph.testutil.Whitebox; import com.baidu.hugegraph.unit.BaseUnitTest; +import com.google.common.base.Predicate; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; public class HugeConfigTest extends BaseUnitTest { - private static final String CONF = - "src/test/java/com/baidu/hugegraph/unit/config/test.conf"; + private static final String PATH = + "src/test/java/com/baidu/hugegraph/unit/config/"; + private static final String CONF = PATH + "test.conf"; @BeforeClass public static void init() { OptionSpace.register("test", TestOptions.class.getName()); } + @Test + public void testOptionDataType() { + Assert.assertEquals(String.class, TestOptions.text1.dataType()); + Assert.assertEquals(Integer.class, TestOptions.int1.dataType()); + Assert.assertEquals(Long.class, TestOptions.long1.dataType()); + Assert.assertEquals(Float.class, TestOptions.float1.dataType()); + Assert.assertEquals(Double.class, TestOptions.double1.dataType()); + Assert.assertEquals(Boolean.class, TestOptions.bool.dataType()); + + Assert.assertEquals(List.class, TestOptions.list.dataType()); + Assert.assertEquals(List.class, TestOptions.map.dataType()); + + Assert.assertEquals(String.class, TestOptions.weekday.dataType()); + Assert.assertEquals(List.class, TestOptions.weekdays.dataType()); + } + + @Test + public void testOptionDesc() { + Assert.assertEquals("description of group1.text1", + TestOptions.text1.desc()); + Assert.assertEquals("description of group1.text2 sub", + TestSubOptions.text2.desc()); + } + + @Test + public void testOptionRequired() { + Assert.assertEquals(false, TestOptions.text1.required()); + Assert.assertEquals(true, TestSubOptions.text2.required()); + } + @Test public void testOptionsToString() { Assert.assertEquals("[String]group1.text1=text1-value", @@ -83,6 +118,92 @@ public void testOptionsToString() { TestSubOptions.textsub.toString()); } + @Test + public void testOptionWithError() { + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.text", + "description of group1.text", + disallowEmpty(), + "" + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.choice", + "description of group1.choice", + allowValues("CHOICE-1", "CHOICE-2", "CHOICE-3"), + "CHOICE-4" + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigListOption<>( + "group1.list", + true, + "description of group1.list", + disallowEmpty(), + String.class, + ImmutableList.of() + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.int", + "description of group1.int", + positiveInt(), + 0 + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.int", + "description of group1.int", + nonNegativeInt(), + -1 + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.long", + "description of group1.long", + rangeInt(1L, 100L), + 0L + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.long", + "description of group1.long", + rangeInt(1L, 100L), + 101L + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.double", + "description of group1.double", + rangeDouble(1D, 10D), + 0D + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigOption<>( + "group1.double", + "description of group1.double", + rangeDouble(1D, 10D), + 11D + ); + }); + } + @Test public void testHugeConfig() throws Exception { Configuration conf = new PropertiesConfiguration(); @@ -114,6 +235,10 @@ public void testHugeConfig() throws Exception { Assert.assertEquals(WeekDay.WEDNESDAY, config.get(TestOptions.weekday)); Assert.assertEquals(Arrays.asList(WeekDay.SATURDAY, WeekDay.SUNDAY), config.get(TestOptions.weekdays)); + + Assert.assertThrows(ConfigException.class, () -> { + new HugeConfig((Configuration) null); + }); } @Test @@ -170,6 +295,36 @@ public void testHugeConfigWithOverride() throws Exception { config.get(TestSubOptions.textsub)); } + @Test + public void testHugeConfigWithTypeError() { + OptionSpace.register("test-type-error", + TestOptionsWithTypeError.class.getName()); + + Assert.assertThrows(ConfigException.class, () -> { + new HugeConfig(PATH + "test-type-error.conf"); + }); + } + + @Test + public void testHugeConfigWithCheckError() throws Exception { + OptionSpace.register("test-check-error", + TestOptionsWithCheckError.class.getName()); + + Assert.assertThrows(ConfigException.class, () -> { + new HugeConfig(PATH + "test-check-error.conf"); + }); + } + + @Test + public void testHugeConfigWithListOptionError() throws Exception { + OptionSpace.register("test-list-error", + TestOptionsWithListError.class.getName()); + + Assert.assertThrows(IllegalStateException.class, () -> { + new HugeConfig(PATH + "test-list-error.conf"); + }); + } + public static class TestOptions extends OptionHolder { private static volatile TestOptions instance; @@ -194,7 +349,6 @@ public static synchronized TestOptions instance() { new ConfigOption<>( "group1.text2", "description of group1.text2", - disallowEmpty(), "text2-value" ); @@ -304,8 +458,10 @@ public static class TestSubOptions extends TestOptions { public static final ConfigOption text2 = new ConfigOption<>( "group1.text2", - "description of group1.text2", + true, + "description of group1.text2 sub", disallowEmpty(), + String.class, "text2-value-override" ); @@ -318,6 +474,87 @@ public static class TestSubOptions extends TestOptions { ); } + public static class TestOptionsWithTypeError extends OptionHolder { + + private static volatile TestOptionsWithTypeError instance; + + public static synchronized TestOptionsWithTypeError instance() { + if (instance == null) { + instance = new TestOptionsWithTypeError(); + instance.registerOptions(); + } + return instance; + } + + public static final ConfigOption intError = + new ConfigOption<>( + "group1.int_type_error", + "description of group1.int_type_error", + rangeInt(1, 100), + 1 + ); + } + + public static class TestOptionsWithCheckError extends OptionHolder { + + private static volatile TestOptionsWithCheckError instance; + + public static synchronized TestOptionsWithCheckError instance() { + if (instance == null) { + instance = new TestOptionsWithCheckError(); + instance.registerOptions(); + } + return instance; + } + + public static final ConfigOption intError = + new ConfigOption<>( + "group1.int_check_error", + "description of group1.int_check_error", + rangeInt(1, 100), + 1 + ); + } + + public static class TestOptionsWithListError extends OptionHolder { + + private static volatile TestOptionsWithListError instance; + + public static synchronized TestOptionsWithListError instance() { + if (instance == null) { + instance = new TestOptionsWithListError(); + instance.registerOptions(); + } + return instance; + } + + public static final InvalidConfigListOption listError = + new InvalidConfigListOption<>( + "group1.list_for_list_error", + "description of group1.list_for_list_error", + disallowEmpty(), + 1 + ); + + static class InvalidConfigListOption extends ConfigOption> { + + @SuppressWarnings("unchecked") + public InvalidConfigListOption(String name, String desc, + Predicate> pred, + T... values) { + super(name, false, desc, pred, + (Class>) Arrays.asList(values).getClass(), + Arrays.asList(values)); + } + + @Override + protected boolean forList() { + return false; + } + } + } + + public enum WeekDay { SUNDAY, MONDAY, TUESDAY, WEDNESDAY, THURSDAY, FRIDAY, SATURDAY; diff --git a/src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java b/src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java new file mode 100644 index 000000000..72d8d0ce9 --- /dev/null +++ b/src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java @@ -0,0 +1,216 @@ +/* + * Copyright 2017 HugeGraph Authors + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.baidu.hugegraph.unit.config; + +import static com.baidu.hugegraph.config.OptionChecker.disallowEmpty; + +import org.junit.Test; + +import com.baidu.hugegraph.config.ConfigException; +import com.baidu.hugegraph.config.ConfigOption; +import com.baidu.hugegraph.config.OptionHolder; +import com.baidu.hugegraph.config.OptionSpace; +import com.baidu.hugegraph.testutil.Assert; +import com.baidu.hugegraph.unit.BaseUnitTest; +import com.google.common.base.Predicate; + +public class OptionSpaceTest extends BaseUnitTest { + + @Test + public void tesRegister() { + int oldSize = OptionSpace.keys().size(); + + OptionSpace.register("testgroup1", OptionHolder1.class.getName()); + Assert.assertEquals(oldSize + 2, OptionSpace.keys().size()); + Assert.assertTrue(OptionSpace.containKey("group1.text1")); + Assert.assertTrue(OptionSpace.containKey("group1.text2")); + + OptionSpace.register("testgroup1", new OptionHolder1()); + Assert.assertEquals(oldSize + 2, OptionSpace.keys().size()); + Assert.assertTrue(OptionSpace.containKey("group1.text1")); + Assert.assertTrue(OptionSpace.containKey("group1.text2")); + + OptionSpace.register("testgroup2", OptionHolder2.class.getName()); + Assert.assertEquals(oldSize + 4, OptionSpace.keys().size()); + + Assert.assertTrue(OptionSpace.containKey("testgroup1.text1")); + Assert.assertTrue(OptionSpace.containKey("testgroup1.text2")); + Assert.assertTrue(OptionSpace.containKey("testgroup2.text1")); + Assert.assertTrue(OptionSpace.containKey("testgroup2.text2")); + + Assert.assertEquals("text1 value", + OptionSpace.get("testgroup1.text1").defaultValue()); + Assert.assertEquals("text2 value", + OptionSpace.get("testgroup1.text2").defaultValue()); + Assert.assertEquals("text1 value", + OptionSpace.get("testgroup2.text1").defaultValue()); + Assert.assertEquals("text2 value", + OptionSpace.get("testgroup2.text2").defaultValue()); + } + + @Test + public void testRegisterWithError() { + Assert.assertThrows(ConfigException.class, () -> { + OptionSpace.register("test-error", "fake"); + }); + + Assert.assertThrows(ConfigException.class, () -> { + OptionSpace.register("test-error", Exception.class.getName()); + }); + + class OptionHolderWithoutInstance extends OptionHolder { + // no instance() + } + Assert.assertThrows(ConfigException.class, () -> { + OptionSpace.register("test-error", + OptionHolderWithoutInstance.class.getName()); + }); + + class OptionHolderWithNonStaticInstance extends OptionHolder { + + // not static instance() + @SuppressWarnings("unused") + public OptionHolderWithNonStaticInstance instance() { + return new OptionHolderWithNonStaticInstance(); + } + } + Assert.assertThrows(ConfigException.class, () -> { + OptionSpace.register("test-error", OptionHolderWithNonStaticInstance + .class.getName()); + }); + + Assert.assertThrows(ConfigException.class, () -> { + OptionSpace.register("test-error", + OptionHolderWithInstanceNull.class.getName()); + }); + + Assert.assertThrows(ConfigException.class, () -> { + OptionSpace.register("test-error", + OptionHolderWithInstanceThrow.class.getName()); + }); + + Assert.assertThrows(ConfigException.class, () -> { + OptionSpace.register("test-error", + OptionHolderWithInvalidOption.class.getName()); + }); + } + + public static class OptionHolderWithInstanceNull extends OptionHolder { + + public static OptionHolderWithInstanceNull instance() { + return null; + } + } + + public static class OptionHolderWithInstanceThrow extends OptionHolder { + + public static OptionHolderWithInstanceNull instance() { + throw new RuntimeException("test error"); + } + } + + public static class OptionHolderWithInvalidOption extends OptionHolder { + + public static OptionHolderWithInvalidOption instance() { + return new OptionHolderWithInvalidOption(); + } + + OptionHolderWithInvalidOption() { + this.registerOptions(); + } + + public static final String fake = "fake"; + + public static final ConfigOption invalid = + new InvalidOption<>( + "group1.text1", + "description of group1.text1", + disallowEmpty(), + "value" + ); + + public static class InvalidOption extends ConfigOption { + + public InvalidOption(String name, String desc, + Predicate pred, T value) { + super(name, desc, pred, value); + } + + @Override + public String name() { + throw new RuntimeException("fake"); + } + } + } + + public static class OptionHolder1 extends OptionHolder { + + public static OptionHolder1 instance() { + return new OptionHolder1(); + } + + OptionHolder1() { + this.registerOptions(); + } + + public static final ConfigOption text1 = + new ConfigOption<>( + "testgroup1.text1", + "description of testgroup1.text1", + disallowEmpty(), + "text1 value" + ); + + public static final ConfigOption text2 = + new ConfigOption<>( + "testgroup1.text2", + "description of testgroup1.text2", + disallowEmpty(), + "text2 value" + ); + } + + public static class OptionHolder2 extends OptionHolder { + + public static OptionHolder2 instance() { + return new OptionHolder2(); + } + + OptionHolder2() { + this.registerOptions(); + } + + public static final ConfigOption text1 = + new ConfigOption<>( + "testgroup2.text1", + "description of testgroup2.text1", + disallowEmpty(), + "text1 value" + ); + + public static final ConfigOption text2 = + new ConfigOption<>( + "testgroup2.text2", + "description of testgroup2.text2", + disallowEmpty(), + "text2 value" + ); + } +} diff --git a/src/test/java/com/baidu/hugegraph/unit/config/test-check-error.conf b/src/test/java/com/baidu/hugegraph/unit/config/test-check-error.conf new file mode 100644 index 000000000..53aec1846 --- /dev/null +++ b/src/test/java/com/baidu/hugegraph/unit/config/test-check-error.conf @@ -0,0 +1 @@ +group1.int_check_error=101 diff --git a/src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf b/src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf new file mode 100644 index 000000000..aa57b3bf0 --- /dev/null +++ b/src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf @@ -0,0 +1 @@ +group1.list_for_list_error=[1,2,3] \ No newline at end of file diff --git a/src/test/java/com/baidu/hugegraph/unit/config/test-type-error.conf b/src/test/java/com/baidu/hugegraph/unit/config/test-type-error.conf new file mode 100644 index 000000000..ac70d4891 --- /dev/null +++ b/src/test/java/com/baidu/hugegraph/unit/config/test-type-error.conf @@ -0,0 +1 @@ +group1.int_type_error=string diff --git a/src/test/java/com/baidu/hugegraph/unit/config/test.conf b/src/test/java/com/baidu/hugegraph/unit/config/test.conf index 7c59e7af8..bd2ea2a85 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/test.conf +++ b/src/test/java/com/baidu/hugegraph/unit/config/test.conf @@ -17,3 +17,5 @@ group1.map=[key1:value1, key3:value3] group1.weekday=SUNDAY group1.weekdays=[SATURDAY, FRIDAY] + +group1.no-used=value From a082d4b8f5bff70c5daff032f2797ec5873166d9 Mon Sep 17 00:00:00 2001 From: Zhangmei Li Date: Sun, 29 Dec 2019 23:44:59 +0800 Subject: [PATCH 3/5] tiny improve Change-Id: Ic40290aa9d0e9795aa27f5b25724ad3ed38c18f7 --- .../java/com/baidu/hugegraph/unit/config/test-list-error.conf | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf b/src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf index aa57b3bf0..907b197f4 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf +++ b/src/test/java/com/baidu/hugegraph/unit/config/test-list-error.conf @@ -1 +1,2 @@ -group1.list_for_list_error=[1,2,3] \ No newline at end of file +group1.list_for_list_error=[1,2,3] + From 00f08c86af0f0509bde7ee9056f50f0245e71cd0 Mon Sep 17 00:00:00 2001 From: Zhangmei Li Date: Mon, 30 Dec 2019 00:10:04 +0800 Subject: [PATCH 4/5] fix ConfigListOption values is empty Change-Id: Ifff6198d18488864ad65f33209a3187056675b7b --- .../config/ConfigListConvOption.java | 8 +++-- .../hugegraph/config/ConfigListOption.java | 6 ++-- .../baidu/hugegraph/config/OptionSpace.java | 4 +++ .../hugegraph/unit/config/HugeConfigTest.java | 34 +++++++++++++++++++ .../unit/config/OptionSpaceTest.java | 33 +++++++++--------- 5 files changed, 64 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java b/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java index 353c2b692..8162500c7 100644 --- a/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java +++ b/src/main/java/com/baidu/hugegraph/config/ConfigListConvOption.java @@ -36,8 +36,7 @@ public class ConfigListConvOption extends TypedOption, List> { public ConfigListConvOption(String name, String desc, Predicate> pred, Function convert, T... values) { - this(name, false, desc, pred, convert, - (Class) values[0].getClass(), Arrays.asList(values)); + this(name, false, desc, pred, convert, null, Arrays.asList(values)); } @SuppressWarnings("unchecked") @@ -46,8 +45,11 @@ public ConfigListConvOption(String name, boolean required, String desc, Class clazz, List values) { super(name, required, desc, pred, (Class>) values.getClass(), values); - E.checkNotNull(clazz, "element class"); E.checkNotNull(convert, "convert"); + if (clazz == null && values.size() > 0) { + clazz = (Class) values.get(0).getClass(); + } + E.checkArgumentNotNull(clazz, "Element class can't be null"); this.elemClass = clazz; this.converter = convert; } diff --git a/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java b/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java index 35d21446f..565e9c71b 100644 --- a/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java +++ b/src/main/java/com/baidu/hugegraph/config/ConfigListOption.java @@ -34,8 +34,7 @@ public class ConfigListOption extends ConfigOption> { @SuppressWarnings("unchecked") public ConfigListOption(String name, String desc, Predicate> pred, T... values) { - this(name, false, desc, pred, - (Class) values[0].getClass(), Arrays.asList(values)); + this(name, false, desc, pred, null, Arrays.asList(values)); } @SuppressWarnings("unchecked") @@ -44,6 +43,9 @@ public ConfigListOption(String name, boolean required, String desc, List values) { super(name, required, desc, pred, (Class>) values.getClass(), values); + if (clazz == null && values.size() > 0) { + clazz = (Class) values.get(0).getClass(); + } E.checkArgumentNotNull(clazz, "Element class can't be null"); this.elemClass = clazz; } diff --git a/src/main/java/com/baidu/hugegraph/config/OptionSpace.java b/src/main/java/com/baidu/hugegraph/config/OptionSpace.java index 9b2aef85f..8e633fa26 100644 --- a/src/main/java/com/baidu/hugegraph/config/OptionSpace.java +++ b/src/main/java/com/baidu/hugegraph/config/OptionSpace.java @@ -21,6 +21,7 @@ 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; @@ -64,6 +65,9 @@ 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( diff --git a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java index a2621022f..295932d64 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java +++ b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java @@ -202,6 +202,40 @@ public void testOptionWithError() { 11D ); }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigListOption<>( + "group1.list", + "description of list with invalid default values", + disallowEmpty() + ); + }); + + Assert.assertThrows(IllegalArgumentException.class, () -> { + new ConfigListOption<>( + "group1.list", + "description of list with invalid default values", + null + ); + }); + + Assert.assertThrows(ConfigException.class, () -> { + new ConfigListConvOption( + "group1.list_conv", + "description of list_conv with invalid default values", + disallowEmpty(), + s -> WeekDay.valueOf(s) + ); + }); + + Assert.assertThrows(IllegalArgumentException.class, () -> { + new ConfigListConvOption( + "group1.list_conv", + "description of list_conv with invalid default values", + null, + s -> WeekDay.valueOf(s) + ); + }); } @Test diff --git a/src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java b/src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java index 72d8d0ce9..3ff2c0512 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java +++ b/src/test/java/com/baidu/hugegraph/unit/config/OptionSpaceTest.java @@ -39,13 +39,13 @@ public void tesRegister() { OptionSpace.register("testgroup1", OptionHolder1.class.getName()); Assert.assertEquals(oldSize + 2, OptionSpace.keys().size()); - Assert.assertTrue(OptionSpace.containKey("group1.text1")); - Assert.assertTrue(OptionSpace.containKey("group1.text2")); + Assert.assertTrue(OptionSpace.containKey("testgroup1.text1")); + Assert.assertTrue(OptionSpace.containKey("testgroup1.text2")); OptionSpace.register("testgroup1", new OptionHolder1()); Assert.assertEquals(oldSize + 2, OptionSpace.keys().size()); - Assert.assertTrue(OptionSpace.containKey("group1.text1")); - Assert.assertTrue(OptionSpace.containKey("group1.text2")); + Assert.assertTrue(OptionSpace.containKey("testgroup1.text1")); + Assert.assertTrue(OptionSpace.containKey("testgroup1.text2")); OptionSpace.register("testgroup2", OptionHolder2.class.getName()); Assert.assertEquals(oldSize + 4, OptionSpace.keys().size()); @@ -75,22 +75,11 @@ public void testRegisterWithError() { OptionSpace.register("test-error", Exception.class.getName()); }); - class OptionHolderWithoutInstance extends OptionHolder { - // no instance() - } Assert.assertThrows(ConfigException.class, () -> { OptionSpace.register("test-error", OptionHolderWithoutInstance.class.getName()); }); - class OptionHolderWithNonStaticInstance extends OptionHolder { - - // not static instance() - @SuppressWarnings("unused") - public OptionHolderWithNonStaticInstance instance() { - return new OptionHolderWithNonStaticInstance(); - } - } Assert.assertThrows(ConfigException.class, () -> { OptionSpace.register("test-error", OptionHolderWithNonStaticInstance .class.getName()); @@ -112,6 +101,18 @@ public OptionHolderWithNonStaticInstance instance() { }); } + public static class OptionHolderWithoutInstance extends OptionHolder { + // no instance() + } + + public static class OptionHolderWithNonStaticInstance extends OptionHolder { + + // not static instance() + public OptionHolderWithNonStaticInstance instance() { + return new OptionHolderWithNonStaticInstance(); + } + } + public static class OptionHolderWithInstanceNull extends OptionHolder { public static OptionHolderWithInstanceNull instance() { @@ -132,7 +133,7 @@ public static OptionHolderWithInvalidOption instance() { return new OptionHolderWithInvalidOption(); } - OptionHolderWithInvalidOption() { + private OptionHolderWithInvalidOption() { this.registerOptions(); } From 585adb64217b6c21bc8a6dbad2a8318901f17361 Mon Sep 17 00:00:00 2001 From: Zhangmei Li Date: Mon, 30 Dec 2019 22:37:17 +0800 Subject: [PATCH 5/5] delete unused empty line Change-Id: Ic8e487e5f9f81b18d981d5d3b8b4c26664b35fec --- .../java/com/baidu/hugegraph/unit/config/HugeConfigTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java index 295932d64..30369d091 100644 --- a/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java +++ b/src/test/java/com/baidu/hugegraph/unit/config/HugeConfigTest.java @@ -588,7 +588,6 @@ protected boolean forList() { } } - public enum WeekDay { SUNDAY, MONDAY, TUESDAY, WEDNESDAY, THURSDAY, FRIDAY, SATURDAY;