beam-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dhalp...@apache.org
Subject [1/2] incubator-beam git commit: Correctly type collections in PipelineOptions.
Date Mon, 26 Sep 2016 21:25:41 GMT
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.mcveety@gmail.com>
Authored: Thu Aug 18 20:49:05 2016 -0400
Committer: Dan Halperin <dhalperi@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()};


Mime
View raw message