Add a way of constructing OptionsBase subclass instances from maps
Added toMap()/fromMap() to OptionsParser, and moved the implementation of OptionsBase#asMap away from OptionsParserImpl.
RELNOTES: None
PiperOrigin-RevId: 153602479
diff --git a/src/main/java/com/google/devtools/common/options/OptionsBase.java b/src/main/java/com/google/devtools/common/options/OptionsBase.java
index 0ca9e44..48658f7 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsBase.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsBase.java
@@ -16,15 +16,15 @@
import com.google.common.escape.CharEscaperBuilder;
import com.google.common.escape.Escaper;
-
+import java.lang.reflect.Field;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
/**
- * Base class for all options classes. Extend this class, adding public
- * instance fields annotated with @Option. Then you can create instances
- * either programmatically:
+ * Base class for all options classes. Extend this class, adding public instance fields annotated
+ * with {@link Option}. Then you can create instances either programmatically:
*
* <pre>
* X x = Options.getDefaults(X.class);
@@ -40,11 +40,10 @@
* X x = parser.getOptions(X.class);
* </pre>
*
- * <p>Subclasses of OptionsBase <b>must</b> be constructed reflectively,
- * i.e. using not {@code new MyOptions}, but one of the two methods above
- * instead. (Direct construction creates an empty instance, not containing
- * default values. This leads to surprising behavior and often
- * NullPointerExceptions, etc.)
+ * <p>Subclasses of {@code OptionsBase} <i>must</i> be constructed reflectively, i.e. using not
+ * {@code new MyOptions()}, but one of the above methods instead. (Direct construction creates an
+ * empty instance, not containing default values. This leads to surprising behavior and often {@code
+ * NullPointerExceptions}, etc.)
*/
public abstract class OptionsBase {
@@ -61,13 +60,24 @@
}
/**
- * Returns this options object in the form of a (new) mapping from option
- * names, including inherited ones, to option values. If the public fields
- * are mutated, this will be reflected in subsequent calls to {@code asMap}.
- * Mutation of this map by the caller does not affect this options object.
+ * Returns a mapping from option names to values, for each option on this object, including
+ * inherited ones. The mapping is a copy, so subsequent mutations to it or to this object are
+ * independent. Entries are sorted alphabetically.
*/
- public final Map<String, Object> asMap() {
- return OptionsParserImpl.optionsAsMap(this);
+ public final <O extends OptionsBase> Map<String, Object> asMap() {
+ // Generic O is needed to tell the type system that the toMap() call is safe.
+ // The casts are safe because this <= O <= OptionsBase.
+ @SuppressWarnings("unchecked")
+ O castThis = (O) this;
+ @SuppressWarnings("unchecked")
+ Class<O> castClass = (Class<O>) getClass();
+
+ Map<String, Object> map = new LinkedHashMap<>();
+ for (Map.Entry<Field, Object> entry : OptionsParser.toMap(castClass, castThis).entrySet()) {
+ String name = entry.getKey().getAnnotation(Option.class).name();
+ map.put(name, entry.getValue());
+ }
+ return map;
}
@Override
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index a871fd1..355deea 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -21,6 +21,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
import com.google.common.escape.Escaper;
+import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.nio.file.FileSystem;
import java.util.ArrayList;
@@ -28,6 +29,8 @@
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
@@ -781,4 +784,109 @@
public List<String> canonicalize() {
return impl.asCanonicalizedList();
}
+
+ /**
+ * Returns a mapping from each option {@link Field} in {@code optionsClass} (including inherited
+ * ones) to its value in {@code options}.
+ *
+ * <p>The map is a mutable copy; changing the map won't affect {@code options} and vice versa.
+ * The map entries appear sorted alphabetically by option name.
+ *
+ * If {@code options} is an instance of a subclass of {@code optionsClass}, any options defined
+ * by the subclass are not included in the map.
+ *
+ * @throws IllegalArgumentException if {@code options} is not an instance of {@code optionsClass}
+ */
+ static <O extends OptionsBase> Map<Field, Object> toMap(Class<O> optionsClass, O options) {
+ OptionsData data = getOptionsDataInternal(optionsClass);
+ // Alphabetized due to getFieldsForClass()'s order.
+ Map<Field, Object> map = new LinkedHashMap<>();
+ for (Field field : data.getFieldsForClass(optionsClass)) {
+ try {
+ map.put(field, field.get(options));
+ } catch (IllegalAccessException e) {
+ // All options fields of options classes should be public.
+ throw new IllegalStateException(e);
+ } catch (IllegalArgumentException e) {
+ // This would indicate an inconsistency in the cached OptionsData.
+ throw new IllegalStateException(e);
+ }
+ }
+ return map;
+ }
+
+ /**
+ * Given a mapping as returned by {@link #toMap}, and the options class it that its entries
+ * correspond to, this constructs the corresponding instance of the options class.
+ *
+ * @throws IllegalArgumentException if {@code map} does not contain exactly the fields of {@code
+ * optionsClass}, with values of the appropriate type
+ */
+ static <O extends OptionsBase> O fromMap(Class<O> optionsClass, Map<Field, Object> map) {
+ // Instantiate the options class.
+ OptionsData data = getOptionsDataInternal(optionsClass);
+ O optionsInstance;
+ try {
+ Constructor<O> constructor = data.getConstructor(optionsClass);
+ Preconditions.checkNotNull(constructor, "No options class constructor available");
+ optionsInstance = constructor.newInstance();
+ } catch (ReflectiveOperationException e) {
+ throw new IllegalStateException("Error while instantiating options class", e);
+ }
+
+ List<Field> fields = data.getFieldsForClass(optionsClass);
+ // Ensure all fields are covered, no extraneous fields.
+ validateFieldsSets(new LinkedHashSet<>(fields), new LinkedHashSet<>(map.keySet()));
+ // Populate the instance.
+ for (Field field : fields) {
+ // Non-null as per above check.
+ Object value = map.get(field);
+ try {
+ field.set(optionsInstance, value);
+ } catch (IllegalAccessException e) {
+ throw new IllegalStateException(e);
+ }
+ // May also throw IllegalArgumentException if map value is ill typed.
+ }
+ return optionsInstance;
+ }
+
+ /**
+ * Raises a pretty {@link IllegalArgumentException} if the two sets of fields are not equal.
+ *
+ * <p>The entries in {@code fieldsFromMap} may be ill formed by being null or lacking an {@link
+ * Option} annotation. (This isn't done for {@code fieldsFromClass} because they come from an
+ * {@link OptionsData} object.)
+ */
+ private static void validateFieldsSets(
+ LinkedHashSet<Field> fieldsFromClass, LinkedHashSet<Field> fieldsFromMap) {
+ if (!fieldsFromClass.equals(fieldsFromMap)) {
+ List<String> extraNamesFromClass = new ArrayList<>();
+ List<String> extraNamesFromMap = new ArrayList<>();
+ for (Field field : fieldsFromClass) {
+ if (!fieldsFromMap.contains(field)) {
+ extraNamesFromClass.add("'" + field.getAnnotation(Option.class).name() + "'");
+ }
+ }
+ for (Field field : fieldsFromMap) {
+ // Extra validation on the map keys since they don't come from OptionsData.
+ if (!fieldsFromClass.contains(field)) {
+ if (field == null) {
+ extraNamesFromMap.add("<null field>");
+ } else {
+ Option annotation = field.getAnnotation(Option.class);
+ if (annotation == null) {
+ extraNamesFromMap.add("<non-Option field>");
+ } else {
+ extraNamesFromMap.add("'" + annotation.name() + "'");
+ }
+ }
+ }
+ }
+ throw new IllegalArgumentException(
+ "Map keys do not match fields of options class; extra map keys: {"
+ + Joiner.on(", ").join(extraNamesFromMap) + "}; extra options class options: {"
+ + Joiner.on(", ").join(extraNamesFromClass) + "}");
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index 2d442a1..c5f31c4 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -85,7 +85,7 @@
private final List<String> warnings = new ArrayList<>();
private boolean allowSingleDashLongOptions = false;
-
+
private ArgsPreProcessor argsPreProcessor =
new ArgsPreProcessor() {
@Override
@@ -93,7 +93,7 @@
return args;
}
};
-
+
/**
* Create a new parser object
*/
@@ -112,32 +112,13 @@
void setAllowSingleDashLongOptions(boolean allowSingleDashLongOptions) {
this.allowSingleDashLongOptions = allowSingleDashLongOptions;
}
-
+
/** Sets the ArgsPreProcessor for manipulations of the options before parsing. */
void setArgsPreProcessor(ArgsPreProcessor preProcessor) {
this.argsPreProcessor = Preconditions.checkNotNull(preProcessor);
}
/**
- * The implementation of {@link OptionsBase#asMap}.
- */
- static Map<String, Object> optionsAsMap(OptionsBase optionsInstance) {
- Class<? extends OptionsBase> optionsClass = optionsInstance.getClass();
- OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
- Map<String, Object> map = new HashMap<>();
- for (Field field : data.getFieldsForClass(optionsClass)) {
- try {
- String name = field.getAnnotation(Option.class).name();
- Object value = field.get(optionsInstance);
- map.put(name, value);
- } catch (IllegalAccessException e) {
- throw new IllegalStateException(e); // unreachable
- }
- }
- return map;
- }
-
- /**
* Implements {@link OptionsParser#asListOfUnparsedOptions()}.
*/
List<UnparsedOptionValueDescription> asListOfUnparsedOptions() {
@@ -707,8 +688,8 @@
return null;
}
optionsInstance = constructor.newInstance();
- } catch (Exception e) {
- throw new IllegalStateException(e);
+ } catch (ReflectiveOperationException e) {
+ throw new IllegalStateException("Error while instantiating options class", e);
}
// Set the fields
diff --git a/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
new file mode 100644
index 0000000..ea60323
--- /dev/null
+++ b/src/test/java/com/google/devtools/common/options/OptionsMapConversionTest.java
@@ -0,0 +1,199 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// Licensed 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.google.devtools.common.options;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+
+import com.google.common.collect.ImmutableMap;
+import java.lang.reflect.Field;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for converting {@link OptionsBase} subclass instances to and from maps. */
+@RunWith(JUnit4.class)
+public class OptionsMapConversionTest {
+
+ private static Map<String, Object> keysToStrings(Map<Field, Object> map) {
+ Map<String, Object> result = new LinkedHashMap<>();
+ for (Map.Entry<Field, Object> entry : map.entrySet()) {
+ String name = entry.getKey().getAnnotation(Option.class).name();
+ result.put(name, entry.getValue());
+ }
+ return result;
+ }
+
+ private static Map<Field, Object> keysToFields(
+ Class<? extends OptionsBase> optionsClass, Map<String, Object> map) {
+ OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass);
+ Map<Field, Object> result = new LinkedHashMap<>();
+ for (Map.Entry<String, Object> entry : map.entrySet()) {
+ Field field = data.getFieldFromName(entry.getKey());
+ result.put(field, entry.getValue());
+ }
+ return result;
+ }
+
+ /** Dummy options base class. */
+ public static class FooOptions extends OptionsBase {
+ @Option(name = "foo", defaultValue = "false")
+ public boolean foo;
+ }
+
+ /** Dummy options derived class. */
+ public static class BazOptions extends FooOptions {
+ @Option(name = "bar", defaultValue = "true")
+ public boolean bar;
+
+ @Option(name = "baz", defaultValue = "5")
+ public int baz;
+ }
+
+ @Test
+ public void toMap_Basic() {
+ FooOptions foo = Options.getDefaults(FooOptions.class);
+ assertThat(keysToStrings(OptionsParser.toMap(FooOptions.class, foo)))
+ .containsExactly("foo", false);
+ }
+
+ @Test
+ public void asMap_Basic() {
+ FooOptions foo = Options.getDefaults(FooOptions.class);
+ assertThat(foo.asMap())
+ .containsExactly("foo", false);
+ }
+
+ @Test
+ public void toMap_Inheritance() {
+ BazOptions baz = Options.getDefaults(BazOptions.class);
+ assertThat(keysToStrings(OptionsParser.toMap(BazOptions.class, baz)))
+ .containsExactly("foo", false, "bar", true, "baz", 5);
+ }
+
+ @Test
+ public void asMap_Inheritance() {
+ // Static type is base class, dynamic type is derived. We still get the derived fields.
+ FooOptions foo = Options.getDefaults(BazOptions.class);
+ assertThat(foo.asMap())
+ .containsExactly("foo", false, "bar", true, "baz", 5);
+ }
+
+ @Test
+ public void toMap_InheritanceBaseFieldsOnly() {
+ BazOptions baz = Options.getDefaults(BazOptions.class);
+ assertThat(keysToStrings(OptionsParser.toMap(FooOptions.class, baz)))
+ .containsExactly("foo", false);
+ }
+
+ /**
+ * Dummy options class for checking alphabetizing.
+ *
+ * <p>Note that field name order differs from option name order.
+ */
+ public static class AlphaOptions extends OptionsBase {
+
+ @Option(name = "c", defaultValue = "0")
+ public int v;
+
+ @Option(name = "d", defaultValue = "0")
+ public int w;
+
+ @Option(name = "a", defaultValue = "0")
+ public int x;
+
+ @Option(name = "e", defaultValue = "0")
+ public int y;
+
+ @Option(name = "b", defaultValue = "0")
+ public int z;
+ }
+
+ @Test
+ public void toMap_AlphabeticalOrder() {
+ AlphaOptions alpha = Options.getDefaults(AlphaOptions.class);
+ assertThat(keysToStrings(OptionsParser.toMap(AlphaOptions.class, alpha)))
+ .containsExactly("a", 0, "b", 0, "c", 0, "d", 0, "e", 0).inOrder();
+ }
+
+ @Test
+ public void asMap_AlphabeticalOrder() {
+ AlphaOptions alpha = Options.getDefaults(AlphaOptions.class);
+ assertThat(alpha.asMap())
+ .containsExactly("a", 0, "b", 0, "c", 0, "d", 0, "e", 0).inOrder();
+ }
+
+ @Test
+ public void fromMap_Basic() {
+ Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true);
+ Map<Field, Object> fieldMap = keysToFields(FooOptions.class, map);
+ FooOptions foo = OptionsParser.fromMap(FooOptions.class, fieldMap);
+ assertThat(foo.foo).isTrue();
+ }
+
+ /** Dummy subclass of foo. */
+ public static class SubFooAOptions extends FooOptions {
+ @Option(name = "a", defaultValue = "false")
+ public boolean a;
+ }
+
+ /** Dummy subclass of foo. */
+ public static class SubFooBOptions extends FooOptions {
+ @Option(name = "b1", defaultValue = "false")
+ public boolean b1;
+
+ @Option(name = "b2", defaultValue = "false")
+ public boolean b2;
+ }
+
+ @Test
+ public void fromMap_FailsOnWrongKeys() {
+ Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true, "a", false);
+ Map<Field, Object> fieldMap = keysToFields(SubFooAOptions.class, map);
+ try {
+ OptionsParser.fromMap(SubFooBOptions.class, fieldMap);
+ fail("Should have failed due to the given map's fields not matching the ones on the class");
+ } catch (IllegalArgumentException e) {
+ assertThat(e.getMessage()).contains(
+ "Map keys do not match fields of options class; extra map keys: {'a'}; "
+ + "extra options class options: {'b1', 'b2'}");
+ }
+ }
+
+ @Test
+ public void fromMap_FailsOnWrongTypes() {
+ Map<String, Object> map = ImmutableMap.<String, Object>of("foo", 5);
+ Map<Field, Object> fieldMap = keysToFields(SubFooAOptions.class, map);
+ try {
+ OptionsParser.fromMap(FooOptions.class, fieldMap);
+ fail("Should have failed due to trying to assign a field value with the wrong type");
+ } catch (IllegalArgumentException e) {
+ assertThat(e.getMessage()).matches(
+ "Can not set boolean field .*\\.foo to java\\.lang\\.Integer");
+ }
+ }
+
+ @Test
+ public void fromMap_Inheritance() {
+ Map<String, Object> map = ImmutableMap.<String, Object>of("foo", true, "bar", true, "baz", 3);
+ Map<Field, Object> fieldMap = keysToFields(BazOptions.class, map);
+ BazOptions baz = OptionsParser.fromMap(BazOptions.class, fieldMap);
+ assertThat(baz.foo).isTrue();
+ assertThat(baz.bar).isTrue();
+ assertThat(baz.baz).isEqualTo(3);
+ }
+}
