[BEAM-790] Validate PipelineOptions Default annotation.

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java

@@ -74,6 +74,7 @@
 import java.util.TreeMap;
 import java.util.TreeSet;
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 import org.apache.beam.sdk.options.Validation.Required;
 import org.apache.beam.sdk.runners.PipelineRunner;
 import org.apache.beam.sdk.runners.PipelineRunnerRegistrar;
@@ -1058,37 +1059,66 @@ private static void validateMethodAnnotations(
     }
 
     // Verify that there is no getter with a mixed @JsonIgnore annotation.
-    validateGettersNoMixedAnnotations(
-        methodNameToAllMethodMap, descriptors, AnnotationPredicate.JSON_IGNORE);
+    validateGettersHaveConsistentAnnotation(
+        methodNameToAllMethodMap, descriptors, AnnotationPredicates.JSON_IGNORE);
 
     // Verify that there is no getter with a mixed @Default annotation.
-    validateGettersNoMixedAnnotations(
-        methodNameToAllMethodMap, descriptors, AnnotationPredicate.DEFAULT_VALUE);
+    validateGettersHaveConsistentAnnotation(
+        methodNameToAllMethodMap, descriptors, AnnotationPredicates.DEFAULT_VALUE);
 
     // Verify that no setter has @JsonIgnore.
-    validateSettersNoAnnotations(
-        methodNameToAllMethodMap, descriptors, AnnotationPredicate.JSON_IGNORE);
+    validateSettersDoNotHaveAnnotation(
+        methodNameToAllMethodMap, descriptors, AnnotationPredicates.JSON_IGNORE);
 
     // Verify that no setter has @Default.
-    validateSettersNoAnnotations(
-        methodNameToAllMethodMap, descriptors, AnnotationPredicate.DEFAULT_VALUE);
+    validateSettersDoNotHaveAnnotation(
+        methodNameToAllMethodMap, descriptors, AnnotationPredicates.DEFAULT_VALUE);
   }
 
   /**
    * Validates that getters don't have mixed annotation.
    */
-  private static void validateGettersNoMixedAnnotations(
+  private static void validateGettersHaveConsistentAnnotation(
       SortedSetMultimap<Method, Method> methodNameToAllMethodMap,
       List<PropertyDescriptor> descriptors,
-      AnnotationPredicate annotationPredicate) {
-    List<InconsistentlyAnnotatedGetters> incompletelyAnnotatedGetters = new ArrayList<>();
-    for (PropertyDescriptor descriptor : descriptors) {
+      final AnnotationPredicates annotationPredicates) {
+    List<InconsistentlyAnnotatedGetters> inconsistentlyAnnotatedGetters = new ArrayList<>();
+    for (final PropertyDescriptor descriptor : descriptors) {
       if (descriptor.getReadMethod() == null
           || IGNORED_METHODS.contains(descriptor.getReadMethod())) {
         continue;
       }
+
       SortedSet<Method> getters = methodNameToAllMethodMap.get(descriptor.getReadMethod());
-      SortedSet<Method> gettersWithTheAnnotation = Sets.filter(getters, annotationPredicate);
+      SortedSet<Method> gettersWithTheAnnotation =
+          Sets.filter(getters, annotationPredicates.forMethod);
+      Set<Annotation> propertyAnnotations = Sets.newLinkedHashSet(FluentIterable
+          .from(gettersWithTheAnnotation)
+          .transform(new Function<Method, Annotation>() {
+            @Nullable
+            @Override
+            public Annotation apply(@Nonnull Method method) {
+              FluentIterable<Annotation> methodAnnotations = FluentIterable
+                  .of(method.getAnnotations())
+                  .filter(annotationPredicates.forAnnotation);
+              int annotationCount = methodAnnotations.size();
+              if (annotationCount == 0) {
+                return null;
+              } else if (annotationCount == 1) {
+                return methodAnnotations.get(0);
+              } else {
+                throw new IllegalArgumentException(String.format(
+                    "Method [%s] is marked with multiple annotations: %s.",
+                    method.getName(),
+                    methodAnnotations));
+              }
+            }})
+          .filter(Predicates.notNull()));
+
+      checkArgument(propertyAnnotations.size() <= 1,
+          "Property [%s] is marked with multiple annotations: %s.",
+          descriptor.getName(),
+          propertyAnnotations);
 
       Iterable<String> getterClassNames = FluentIterable.from(getters)
           .transform(MethodToDeclaringClassFunction.INSTANCE)
@@ -1104,29 +1134,29 @@ private static void validateGettersNoMixedAnnotations(
         err.descriptor = descriptor;
         err.getterClassNames = getterClassNames;
         err.gettersWithTheAnnotationClassNames = gettersWithTheAnnotationClassNames;
-        incompletelyAnnotatedGetters.add(err);
+        inconsistentlyAnnotatedGetters.add(err);
       }
     }
     throwForGettersWithInconsistentAnnotation(
-        incompletelyAnnotatedGetters, annotationPredicate.annotationClass);
+        inconsistentlyAnnotatedGetters, annotationPredicates.annotationClass);
   }
 
   /**
    * Validates that setters don't have the given annotation.
    */
-  private static void validateSettersNoAnnotations(
+  private static void validateSettersDoNotHaveAnnotation(
       SortedSetMultimap<Method, Method> methodNameToAllMethodMap,
       List<PropertyDescriptor> descriptors,
-      AnnotationPredicate annotationPredicate) {
+      final AnnotationPredicates annotationPredicates) {
     List<AnnotatedSetter> annotatedSetters = new ArrayList<>();
     for (PropertyDescriptor descriptor : descriptors) {
       if (descriptor.getWriteMethod() == null
           || IGNORED_METHODS.contains(descriptor.getWriteMethod())) {
         continue;
       }
-      SortedSet<Method> settersWithTheAnnotation =
-          Sets.filter(methodNameToAllMethodMap.get(descriptor.getWriteMethod()),
-              annotationPredicate);
+      SortedSet<Method> settersWithTheAnnotation = Sets.filter(
+          methodNameToAllMethodMap.get(descriptor.getWriteMethod()),
+          annotationPredicates.forMethod);
 
       Iterable<String> settersWithTheAnnotationClassNames =
           FluentIterable.from(settersWithTheAnnotation)
@@ -1140,7 +1170,7 @@ private static void validateSettersNoAnnotations(
         annotatedSetters.add(annotated);
       }
     }
-    throwForSettersWithTheAnnotation(annotatedSetters, annotationPredicate.annotationClass);
+    throwForSettersWithTheAnnotation(annotatedSetters, annotationPredicates.annotationClass);
   }
 
   /**
@@ -1398,42 +1428,59 @@ public Class<?> apply(Method input) {
   /**
    * A {@link Predicate} that returns true if the method is annotated with {@code annotationClass}.
    */
-  static class AnnotationPredicate implements Predicate<Method> {
-    static final AnnotationPredicate JSON_IGNORE = new AnnotationPredicate(
+  static class AnnotationPredicates {
+    static final AnnotationPredicates JSON_IGNORE = new AnnotationPredicates(
+        JsonIgnore.class,
+        new Predicate<Annotation>() {
+          @Override
+          public boolean apply(@Nonnull Annotation input) {
+            return input.annotationType().equals(JsonIgnore.class);
+          }
+        },
         new Predicate<Method>() {
           @Override
           public boolean apply(@Nonnull Method input) {
             return input.isAnnotationPresent(JsonIgnore.class);
+          }});
+
+    private static final Set<Class<?>> DEFAULT_ANNOTATION_CLASSES = Sets.newHashSet(
+        FluentIterable.of(Default.class.getDeclaredClasses())
+        .filter(new Predicate<Class<?>>() {
+          @Override
+          public boolean apply(@Nonnull Class<?> klass) {
+            return klass.isAnnotation();
+          }}));
+
+    static final AnnotationPredicates DEFAULT_VALUE = new AnnotationPredicates(
+        Default.class,
+        new Predicate<Annotation>() {
+          @Override
+          public boolean apply(@Nonnull Annotation input) {
+            return DEFAULT_ANNOTATION_CLASSES.contains(input.annotationType());
           }
         },
-        JsonIgnore.class);
-
-    static final AnnotationPredicate DEFAULT_VALUE = new AnnotationPredicate(
-        new Predicate<Method>() {
+        new Predicate<Method> () {
           @Override
           public boolean apply(@Nonnull Method input) {
-            for (Class<?> klass : Default.class.getDeclaredClasses()) {
-              if (klass.isAnnotation()
-                  && input.isAnnotationPresent((Class<? extends Annotation>) klass)) {
+            for (Annotation annotation : input.getAnnotations()) {
+              if (DEFAULT_ANNOTATION_CLASSES.contains(annotation.annotationType())) {
                 return true;
               }
             }
             return false;
-          }
-        },
-        Default.class);
+          }});
 
-    private final Predicate<Method> predicate;
-    private final Class<? extends Annotation> annotationClass;
+    final Class<? extends Annotation> annotationClass;
+    final Predicate<Annotation> forAnnotation;
+    final Predicate<Method> forMethod;
 
-    AnnotationPredicate(Predicate<Method> predicate, Class<? extends Annotation> annotationClass) {
-      this.predicate = predicate;
+    AnnotationPredicates(
+        Class<? extends Annotation> annotationClass,
+        Predicate<Annotation> forAnnotation,
+        Predicate<Method> forMethod) {
       this.annotationClass = annotationClass;
-    }
-
-    @Override
-    public boolean apply(Method input) {
-      return predicate.apply(input);
+      this.forAnnotation = forAnnotation;
+      this.forMethod = forMethod;
     }
   }
 

sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java

@@ -63,7 +63,7 @@
 import java.util.concurrent.ThreadLocalRandom;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.ThreadSafe;
-import org.apache.beam.sdk.options.PipelineOptionsFactory.AnnotationPredicate;
+import org.apache.beam.sdk.options.PipelineOptionsFactory.AnnotationPredicates;
 import org.apache.beam.sdk.options.PipelineOptionsFactory.Registration;
 import org.apache.beam.sdk.options.ValueProvider.RuntimeValueProvider;
 import org.apache.beam.sdk.options.ValueProvider.StaticValueProvider;
@@ -638,7 +638,8 @@ private void removeIgnoredOptions(
       // Find all the method names that are annotated with JSON ignore.
       Set<String> jsonIgnoreMethodNames = FluentIterable.from(
           ReflectHelpers.getClosureOfMethodsOnInterfaces(interfaces))
-          .filter(AnnotationPredicate.JSON_IGNORE).transform(new Function<Method, String>() {
+          .filter(AnnotationPredicates.JSON_IGNORE.forMethod)
+          .transform(new Function<Method, String>() {
             @Override
             public String apply(Method input) {
               return input.getName();

sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java

@@ -481,12 +481,42 @@ public void testMultipleSettersAnnotatedWithDefault() throws Exception {
    * This class is has a conflicting field with {@link CombinedObject} that doesn't have
    * {@link Default @Default}.
    */
-  public interface GetterWithDefault extends PipelineOptions {
+  private interface GetterWithDefault extends PipelineOptions {
     @Default.Integer(1)
     Object getObject();
     void setObject(Object value);
   }
 
+  /**
+   * This class is consistent with {@link GetterWithDefault} that has the same
+   * {@link Default @Default}.
+   */
+  private interface GetterWithConsistentDefault extends PipelineOptions {
+    @Default.Integer(1)
+    Object getObject();
+    void setObject(Object value);
+  }
+
+  /**
+   * This class is inconsistent with {@link GetterWithDefault} that has a different
+   * {@link Default @Default}.
+   */
+  private interface GetterWithInconsistentDefault extends PipelineOptions {
+    @Default.String("abc")
+    Object getObject();
+    void setObject(Object value);
+  }
+
+  /**
+   * This class is inconsistent with {@link GetterWithDefault} that has a different
+   * {@link Default @Default} value.
+   */
+  private interface GetterWithInconsistentDefaultValue extends PipelineOptions {
+    @Default.Integer(0)
+    Object getObject();
+    void setObject(Object value);
+  }
+
   @Test
   public void testNotAllGettersAnnotatedWithDefault() throws Exception {
     // Initial construction is valid.
@@ -504,6 +534,64 @@ public void testNotAllGettersAnnotatedWithDefault() throws Exception {
     options.as(CombinedObject.class);
   }
 
+  @Test
+  public void testGettersAnnotatedWithConsistentDefault() throws Exception {
+    GetterWithConsistentDefault options = PipelineOptionsFactory
+        .as(GetterWithDefault.class)
+        .as(GetterWithConsistentDefault.class);
+
+    assertEquals(1, options.getObject());
+  }
+
+  @Test
+  public void testGettersAnnotatedWithInconsistentDefault() throws Exception {
+    // Initial construction is valid.
+    GetterWithDefault options = PipelineOptionsFactory.as(GetterWithDefault.class);
+
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage(
+        "Property [object] is marked with multiple annotations: ["
+            + "@org.apache.beam.sdk.options.Default$Integer(value=1), "
+            + "@org.apache.beam.sdk.options.Default$String(value=abc)].");
+
+    // When we attempt to convert, we should error at this moment.
+    options.as(GetterWithInconsistentDefault.class);
+  }
+
+  @Test
+  public void testGettersAnnotatedWithInconsistentDefaultValue() throws Exception {
+    // Initial construction is valid.
+    GetterWithDefault options = PipelineOptionsFactory.as(GetterWithDefault.class);
+
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage(
+        "Property [object] is marked with multiple annotations: ["
+            + "@org.apache.beam.sdk.options.Default$Integer(value=1), "
+            + "@org.apache.beam.sdk.options.Default$Integer(value=0)].");
+
+    // When we attempt to convert, we should error at this moment.
+    options.as(GetterWithInconsistentDefaultValue.class);
+  }
+
+  private interface GettersWithMultipleDefault extends PipelineOptions {
+    @Default.String("abc")
+    @Default.Integer(0)
+    Object getObject();
+    void setObject(Object value);
+  }
+
+  @Test
+  public void testGettersWithMultipleDefaults() throws Exception {
+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage(
+        "Method [getObject] is marked with multiple annotations: ["
+            + "@org.apache.beam.sdk.options.Default$String(value=abc), "
+            + "@org.apache.beam.sdk.options.Default$Integer(value=0)].");
+
+    // When we attempt to create, we should error at this moment.
+    PipelineOptionsFactory.as(GettersWithMultipleDefault.class);
+  }
+
   private interface MultiGettersWithDefault extends PipelineOptions {
     Object getObject();
     void setObject(Object value);