Repository: incubator-beam Updated Branches: refs/heads/master 3fe3bc8eb -> 21c13d5f6
Correctly type collections in PipelineOptions. Project: http://git-wip-us.apache.org/repos/asf/incubator-beam/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-beam/commit/4e9987c4 Tree: http://git-wip-us.apache.org/repos/asf/incubator-beam/tree/4e9987c4 Diff: http://git-wip-us.apache.org/repos/asf/incubator-beam/diff/4e9987c4 Branch: refs/heads/master Commit: 4e9987c423f8934f802b44f3e82a6200978bdc37 Parents: 3fe3bc8 Author: sammcveety <sam.mcve...@gmail.com> Authored: Thu Aug 18 20:49:05 2016 -0400 Committer: Dan Halperin <dhalp...@google.com> Committed: Mon Sep 26 14:25:08 2016 -0700 ---------------------------------------------------------------------- .../sdk/options/PipelineOptionsFactory.java | 92 ++++++++------- .../sdk/options/PipelineOptionsFactoryTest.java | 111 +++++++++++++++++-- 2 files changed, 156 insertions(+), 47 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/4e9987c4/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java ---------------------------------------------------------------------- diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java index 43927bc..9fc6c2c 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java @@ -28,8 +28,8 @@ import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.base.Optional; import com.google.common.base.Predicate; +import com.google.common.base.Predicates; import com.google.common.base.Strings; -import com.google.common.collect.Collections2; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; @@ -55,6 +55,7 @@ import java.lang.annotation.Annotation; import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.lang.reflect.Proxy; +import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -843,13 +844,8 @@ public class PipelineOptionsFactory { * resolved. */ private static List<PropertyDescriptor> getPropertyDescriptors( - Class<? extends PipelineOptions> beanClass) + Set<Method> methods, Class<? extends PipelineOptions> beanClass) throws IntrospectionException { - // The sorting is important to make this method stable. - SortedSet<Method> methods = Sets.newTreeSet(MethodComparator.INSTANCE); - methods.addAll( - Collections2.filter(Arrays.asList(beanClass.getMethods()), NOT_SYNTHETIC_PREDICATE)); - SortedMap<String, Method> propertyNamesToGetters = new TreeMap<>(); for (Map.Entry<String, Method> entry : PipelineOptionsReflector.getPropertyNamesToGetters(methods).entries()) { @@ -858,6 +854,7 @@ public class PipelineOptionsFactory { List<PropertyDescriptor> descriptors = Lists.newArrayList(); List<TypeMismatch> mismatches = new ArrayList<>(); + Set<String> usedDescriptors = Sets.newHashSet(); /* * Add all the getter/setter pairs to the list of descriptors removing the getter once * it has been paired up. @@ -874,9 +871,9 @@ public class PipelineOptionsFactory { // Validate that the getter and setter property types are the same. if (getterMethod != null) { - Class<?> getterPropertyType = getterMethod.getReturnType(); - Class<?> setterPropertyType = method.getParameterTypes()[0]; - if (getterPropertyType != setterPropertyType) { + Type getterPropertyType = getterMethod.getGenericReturnType(); + Type setterPropertyType = method.getGenericParameterTypes()[0]; + if (!getterPropertyType.equals(setterPropertyType)) { TypeMismatch mismatch = new TypeMismatch(); mismatch.propertyName = propertyName; mismatch.getterPropertyType = getterPropertyType; @@ -885,9 +882,14 @@ public class PipelineOptionsFactory { continue; } } - - descriptors.add(new PropertyDescriptor( - propertyName, getterMethod, method)); + // Properties can appear multiple times with subclasses, and we don't + // want to add a bad entry if we have already added a good one (with both + // getter and setter). + if (!usedDescriptors.contains(propertyName)) { + descriptors.add(new PropertyDescriptor( + propertyName, getterMethod, method)); + usedDescriptors.add(propertyName); + } } throwForTypeMismatches(mismatches); @@ -901,8 +903,8 @@ public class PipelineOptionsFactory { private static class TypeMismatch { private String propertyName; - private Class<?> getterPropertyType; - private Class<?> setterPropertyType; + private Type getterPropertyType; + private Type setterPropertyType; } private static void throwForTypeMismatches(List<TypeMismatch> mismatches) { @@ -912,8 +914,8 @@ public class PipelineOptionsFactory { "Type mismatch between getter and setter methods for property [%s]. " + "Getter is of type [%s] whereas setter is of type [%s].", mismatch.propertyName, - mismatch.getterPropertyType.getName(), - mismatch.setterPropertyType.getName())); + mismatch.getterPropertyType, + mismatch.setterPropertyType)); } else if (mismatches.size() > 1) { StringBuilder builder = new StringBuilder( "Type mismatches between getters and setters detected:"); @@ -921,8 +923,8 @@ public class PipelineOptionsFactory { builder.append(String.format( "%n - Property [%s]: Getter is of type [%s] whereas setter is of type [%s].", mismatch.propertyName, - mismatch.getterPropertyType.getName(), - mismatch.setterPropertyType.getName())); + mismatch.getterPropertyType.toString(), + mismatch.setterPropertyType.toString())); } throw new IllegalArgumentException(builder.toString()); } @@ -977,14 +979,11 @@ public class PipelineOptionsFactory { methods.add(method); } } - // Ignore standard infrastructure methods on the generated class. + // Ignore methods on the base PipelineOptions interface. try { - methods.add(klass.getMethod("equals", Object.class)); - methods.add(klass.getMethod("hashCode")); - methods.add(klass.getMethod("toString")); - methods.add(klass.getMethod("as", Class.class)); - methods.add(klass.getMethod("cloneAs", Class.class)); - methods.add(klass.getMethod("populateDisplayData", DisplayData.Builder.class)); + methods.add(iface.getMethod("as", Class.class)); + methods.add(iface.getMethod("cloneAs", Class.class)); + methods.add(iface.getMethod("populateDisplayData", DisplayData.Builder.class)); } catch (NoSuchMethodException | SecurityException e) { throw new RuntimeException(e); } @@ -1017,7 +1016,7 @@ public class PipelineOptionsFactory { // Verify that there is no getter with a mixed @JsonIgnore annotation and verify // that no setter has @JsonIgnore. - Iterable<Method> allInterfaceMethods = + SortedSet<Method> allInterfaceMethods = FluentIterable.from( ReflectHelpers.getClosureOfMethodsOnInterfaces( validatedPipelineOptionsInterfaces)) @@ -1030,7 +1029,7 @@ public class PipelineOptionsFactory { methodNameToAllMethodMap.put(method, method); } - List<PropertyDescriptor> descriptors = getPropertyDescriptors(klass); + List<PropertyDescriptor> descriptors = getPropertyDescriptors(allInterfaceMethods, iface); List<InconsistentlyIgnoredGetters> incompletelyIgnoredGetters = new ArrayList<>(); List<IgnoredSetter> ignoredSetters = new ArrayList<>(); @@ -1104,13 +1103,25 @@ public class PipelineOptionsFactory { methods.add(propertyDescriptor.getWriteMethod()); } throwForMissingBeanMethod(iface, missingBeanMethods); + final Set<String> knownMethods = Sets.newHashSet(); + for (Method method : methods) { + knownMethods.add(method.getName()); + } // Verify that no additional methods are on an interface that aren't a bean property. + // Because methods can have multiple declarations, we do a name-based comparison + // here to prevent false positives. SortedSet<Method> unknownMethods = new TreeSet<>(MethodComparator.INSTANCE); unknownMethods.addAll( Sets.filter( - Sets.difference(Sets.newHashSet(klass.getMethods()), methods), - NOT_SYNTHETIC_PREDICATE)); + Sets.difference(Sets.newHashSet(iface.getMethods()), methods), + Predicates.and(NOT_SYNTHETIC_PREDICATE, + new Predicate<Method>() { + @Override + public boolean apply(@Nonnull Method input) { + return !knownMethods.contains(input.getName()); + } + }))); checkArgument(unknownMethods.isEmpty(), "Methods %s on [%s] do not conform to being bean properties.", FluentIterable.from(unknownMethods).transform(ReflectHelpers.METHOD_FORMATTER), @@ -1402,9 +1413,8 @@ public class PipelineOptionsFactory { klass, entry.getKey(), closestMatches)); } } - Method method = propertyNamesToGetters.get(entry.getKey()); - // Only allow empty argument values for String, String Array, and Collection. + // Only allow empty argument values for String, String Array, and Collection<String>. Class<?> returnType = method.getReturnType(); JavaType type = MAPPER.getTypeFactory().constructType(method.getGenericReturnType()); if ("runner".equals(entry.getKey())) { @@ -1441,25 +1451,29 @@ public class PipelineOptionsFactory { } }).toList(); - if (returnType.isArray() && !returnType.getComponentType().equals(String.class)) { + if (returnType.isArray() && !returnType.getComponentType().equals(String.class) + || Collection.class.isAssignableFrom(returnType)) { for (String value : values) { checkArgument(!value.isEmpty(), - "Empty argument value is only allowed for String, String Array, and Collection," - + " but received: " + returnType); + "Empty argument value is only allowed for String, String Array, " + + "and Collections of Strings, but received: %s", + method.getGenericReturnType()); } } convertedOptions.put(entry.getKey(), MAPPER.convertValue(values, type)); } else if (SIMPLE_TYPES.contains(returnType) || returnType.isEnum()) { String value = Iterables.getOnlyElement(entry.getValue()); checkArgument(returnType.equals(String.class) || !value.isEmpty(), - "Empty argument value is only allowed for String, String Array, and Collection," - + " but received: " + returnType); + "Empty argument value is only allowed for String, String Array, " + + "and Collections of Strings, but received: %s", + method.getGenericReturnType()); convertedOptions.put(entry.getKey(), MAPPER.convertValue(value, type)); } else { String value = Iterables.getOnlyElement(entry.getValue()); checkArgument(returnType.equals(String.class) || !value.isEmpty(), - "Empty argument value is only allowed for String, String Array, and Collection," - + " but received: " + returnType); + "Empty argument value is only allowed for String, String Array, " + + "and Collections of Strings, but received: %s", + method.getGenericReturnType()); try { convertedOptions.put(entry.getKey(), MAPPER.readValue(value, type)); } catch (IOException e) { http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/4e9987c4/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java ---------------------------------------------------------------------- diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java index 70c8983..f26667f 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java @@ -217,8 +217,7 @@ public class PipelineOptionsFactoryTest { expectedException.expectMessage("Property [value]: Getter is of type " + "[boolean] whereas setter is of type [int]."); expectedException.expectMessage("Property [other]: Getter is of type [long] " - + "whereas setter is of type [java.lang.String]."); - + + "whereas setter is of type [class java.lang.String]."); PipelineOptionsFactory.as(MultiGetterSetterTypeMismatch.class); } @@ -500,7 +499,8 @@ public class PipelineOptionsFactoryTest { "--byte="}; expectedException.expect(IllegalArgumentException.class); expectedException.expectMessage( - "Empty argument value is only allowed for String, String Array, and Collection"); + "Empty argument value is only allowed for String, String Array, and Collections" + + " of Strings"); PipelineOptionsFactory.fromArgs(args).as(Primitives.class); } @@ -708,7 +708,8 @@ public class PipelineOptionsFactoryTest { public void testEmptyInNonStringArrays() { expectedException.expect(IllegalArgumentException.class); expectedException.expectMessage( - "Empty argument value is only allowed for String, String Array, and Collection"); + "Empty argument value is only allowed for String, String Array, and Collections" + + " of Strings"); String[] args = new String[] { "--boolean=true", @@ -722,7 +723,8 @@ public class PipelineOptionsFactoryTest { public void testEmptyInNonStringArraysWithCommaList() { expectedException.expect(IllegalArgumentException.class); expectedException.expectMessage( - "Empty argument value is only allowed for String, String Array, and Collection"); + "Empty argument value is only allowed for String, String Array, and Collections" + + " of Strings"); String[] args = new String[] { "--int=1,,9"}; @@ -749,16 +751,57 @@ public class PipelineOptionsFactoryTest { public static interface Lists extends PipelineOptions { List<String> getString(); void setString(List<String> value); + List<Integer> getInteger(); + void setInteger(List<Integer> value); + List getList(); + void setList(List value); } @Test - public void testList() { - String[] args = + public void testListRawDefaultsToString() { + String[] manyArgs = + new String[] {"--list=stringValue1", "--list=stringValue2", "--list=stringValue3"}; + + Lists options = PipelineOptionsFactory.fromArgs(manyArgs).as(Lists.class); + assertEquals(ImmutableList.of("stringValue1", "stringValue2", "stringValue3"), + options.getList()); + } + + @Test + public void testListString() { + String[] manyArgs = new String[] {"--string=stringValue1", "--string=stringValue2", "--string=stringValue3"}; + String[] oneArg = new String[] {"--string=stringValue1"}; - Lists options = PipelineOptionsFactory.fromArgs(args).as(Lists.class); + Lists options = PipelineOptionsFactory.fromArgs(manyArgs).as(Lists.class); assertEquals(ImmutableList.of("stringValue1", "stringValue2", "stringValue3"), options.getString()); + + options = PipelineOptionsFactory.fromArgs(oneArg).as(Lists.class); + assertEquals(ImmutableList.of("stringValue1"), options.getString()); + } + + @Test + public void testListInt() { + String[] manyArgs = + new String[] {"--integer=1", "--integer=2", "--integer=3"}; + String[] manyArgsShort = + new String[] {"--integer=1,2,3"}; + String[] oneArg = new String[] {"--integer=1"}; + String[] missingArg = new String[] {"--integer="}; + + Lists options = PipelineOptionsFactory.fromArgs(manyArgs).as(Lists.class); + assertEquals(ImmutableList.of(1, 2, 3), options.getInteger()); + options = PipelineOptionsFactory.fromArgs(manyArgsShort).as(Lists.class); + assertEquals(ImmutableList.of(1, 2, 3), options.getInteger()); + options = PipelineOptionsFactory.fromArgs(oneArg).as(Lists.class); + assertEquals(ImmutableList.of(1), options.getInteger()); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage( + "Empty argument value is only allowed for String, String Array, and Collections of Strings," + + " but received: java.util.List<java.lang.Integer>"); + options = PipelineOptionsFactory.fromArgs(missingArg).as(Lists.class); } @Test @@ -806,6 +849,58 @@ public class PipelineOptionsFactoryTest { expectedLogs.verifyWarn("Strict parsing is disabled, ignoring option"); } + /** A test interface containing all supported List return types. */ + public static interface Maps extends PipelineOptions { + Map<Integer, Integer> getMap(); + void setMap(Map<Integer, Integer> value); + + Map<Integer, Map<Integer, Integer>> getNestedMap(); + void setNestedMap(Map<Integer, Map<Integer, Integer>> value); + } + + @Test + public void testMapIntInt() { + String[] manyArgsShort = + new String[] {"--map={\"1\":1,\"2\":2}"}; + String[] oneArg = new String[] {"--map={\"1\":1}"}; + String[] missingArg = new String[] {"--map="}; + + Maps options = PipelineOptionsFactory.fromArgs(manyArgsShort).as(Maps.class); + assertEquals(ImmutableMap.of(1, 1, 2, 2), options.getMap()); + options = PipelineOptionsFactory.fromArgs(oneArg).as(Maps.class); + assertEquals(ImmutableMap.of(1, 1), options.getMap()); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage( + "Empty argument value is only allowed for String, String Array, and " + + "Collections of Strings, but received: java.util.Map<java.lang.Integer, " + + "java.lang.Integer>"); + options = PipelineOptionsFactory.fromArgs(missingArg).as(Maps.class); + } + + @Test + public void testNestedMap() { + String[] manyArgsShort = + new String[] {"--nestedMap={\"1\":{\"1\":1},\"2\":{\"2\":2}}"}; + String[] oneArg = new String[] {"--nestedMap={\"1\":{\"1\":1}}"}; + String[] missingArg = new String[] {"--nestedMap="}; + + Maps options = PipelineOptionsFactory.fromArgs(manyArgsShort).as(Maps.class); + assertEquals(ImmutableMap.of(1, ImmutableMap.of(1, 1), + 2, ImmutableMap.of(2, 2)), + options.getNestedMap()); + options = PipelineOptionsFactory.fromArgs(oneArg).as(Maps.class); + assertEquals(ImmutableMap.of(1, ImmutableMap.of(1, 1)), + options.getNestedMap()); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage( + "Empty argument value is only allowed for String, String Array, and Collections of " + + "Strings, but received: java.util.Map<java.lang.Integer, " + + "java.util.Map<java.lang.Integer, java.lang.Integer>>"); + options = PipelineOptionsFactory.fromArgs(missingArg).as(Maps.class); + } + @Test public void testSettingRunner() { String[] args = new String[] {"--runner=" + RegisteredTestRunner.class.getSimpleName()};