fixup! address comments-3

apache · Nov 1, 2016 · 684c5e0 · 684c5e0
1 parent f8b672e
commit 684c5e0
Show file tree

Hide file tree

Showing 4 changed files with 147 additions and 107 deletions.
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
@@ -1119,10 +1119,9 @@ public Iterable<String> apply(final @Nonnull Method method) {
                           @Override
                           public String apply(@Nonnull Annotation annotation) {
                             return String.format(
-                                "[%s on %s.%s]",
-                                toStringForPrint(annotation),
-                                method.getDeclaringClass().getSimpleName(),
-                                method.getName());
+                                "[%s on %s]",
+                                ReflectHelpers.toStringForPrint(annotation),
+                                ReflectHelpers.CLASS_AND_METHOD_FORMATTER.apply(method));
                           }
                         });
 
@@ -1152,12 +1151,6 @@ public String apply(@Nonnull Annotation annotation) {
         inconsistentlyAnnotatedGetters, annotationPredicates.annotationClass);
   }
 
-  @VisibleForTesting
-  static String toStringForPrint(Annotation annotation) {
-    String toString = annotation.toString();
-    return toString.substring(toString.lastIndexOf('.') + 1).replace('$', '.');
-  }
-
   /**
    * Validates that setters don't have the given annotation.
    */

diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/common/ReflectHelpers.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/common/ReflectHelpers.java
@@ -21,11 +21,13 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static java.util.Arrays.asList;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Queues;
+import java.lang.annotation.Annotation;
 import java.lang.reflect.GenericArrayType;
 import java.lang.reflect.Method;
 import java.lang.reflect.ParameterizedType;
@@ -44,7 +46,9 @@ public class ReflectHelpers {
 
   private static final Joiner COMMA_SEPARATOR = Joiner.on(", ");
 
-  /** A {@link Function} that turns a method into a simple method signature. */
+  /**
+   * A {@link Function} that turns a method into a simple method signature.
+   */
   public static final Function<Method, String> METHOD_FORMATTER = new Function<Method, String>() {
     @Override
     public String apply(@Nonnull Method input) {
@@ -57,98 +61,106 @@ public String apply(@Nonnull Method input) {
     }
   };
 
-  /** A {@link Function} that turns a method into the declaring class + method signature. */
+  /**
+   * A {@link Function} that turns a method into the declaring class + method signature.
+   */
   public static final Function<Method, String> CLASS_AND_METHOD_FORMATTER =
       new Function<Method, String>() {
-    @Override
-    public String apply(@Nonnull Method input) {
-      return String.format("%s#%s",
-          CLASS_NAME.apply(input.getDeclaringClass()),
-          METHOD_FORMATTER.apply(input));
-    }
-  };
+        @Override
+        public String apply(@Nonnull Method input) {
+          return String.format("%s#%s",
+              CLASS_NAME.apply(input.getDeclaringClass()),
+              METHOD_FORMATTER.apply(input));
+        }
+      };
 
-  /** A {@link Function} with returns the classes name. */
+  /**
+   * A {@link Function} with returns the classes name.
+   */
   public static final Function<Class<?>, String> CLASS_NAME =
       new Function<Class<?>, String>() {
-    @Override
-    public String apply(@Nonnull Class<?> input) {
-      return input.getName();
-    }
-  };
+        @Override
+        public String apply(@Nonnull Class<?> input) {
+          return input.getName();
+        }
+      };
 
-  /** A {@link Function} with returns the classes name. */
+  /**
+   * A {@link Function} with returns the classes name.
+   */
   public static final Function<Class<?>, String> CLASS_SIMPLE_NAME =
       new Function<Class<?>, String>() {
-    @Override
-    public String apply(@Nonnull Class<?> input) {
-      return input.getSimpleName();
-    }
-  };
+        @Override
+        public String apply(@Nonnull Class<?> input) {
+          return input.getSimpleName();
+        }
+      };
 
-  /** A {@link Function} that formats types. */
+  /**
+   * A {@link Function} that formats types.
+   */
   public static final Function<Type, String> TYPE_SIMPLE_DESCRIPTION =
       new Function<Type, String>() {
-    @Override
-    @Nullable
-    public String apply(@Nonnull Type input) {
-      StringBuilder builder = new StringBuilder();
-      format(builder, input);
-      return builder.toString();
-    }
+        @Override
+        @Nullable
+        public String apply(@Nonnull Type input) {
+          StringBuilder builder = new StringBuilder();
+          format(builder, input);
+          return builder.toString();
+        }
 
-    private void format(StringBuilder builder, Type t) {
-      if (t instanceof Class) {
-        formatClass(builder, (Class<?>) t);
-      } else if (t instanceof TypeVariable) {
-        formatTypeVariable(builder, (TypeVariable<?>) t);
-      } else if (t instanceof WildcardType) {
-        formatWildcardType(builder, (WildcardType) t);
-      } else if (t instanceof ParameterizedType) {
-        formatParameterizedType(builder, (ParameterizedType) t);
-      } else if (t instanceof GenericArrayType) {
-        formatGenericArrayType(builder, (GenericArrayType) t);
-      } else {
-        builder.append(t.toString());
-      }
-    }
+        private void format(StringBuilder builder, Type t) {
+          if (t instanceof Class) {
+            formatClass(builder, (Class<?>) t);
+          } else if (t instanceof TypeVariable) {
+            formatTypeVariable(builder, (TypeVariable<?>) t);
+          } else if (t instanceof WildcardType) {
+            formatWildcardType(builder, (WildcardType) t);
+          } else if (t instanceof ParameterizedType) {
+            formatParameterizedType(builder, (ParameterizedType) t);
+          } else if (t instanceof GenericArrayType) {
+            formatGenericArrayType(builder, (GenericArrayType) t);
+          } else {
+            builder.append(t.toString());
+          }
+        }
 
-    private void formatClass(StringBuilder builder, Class<?> clazz) {
-      builder.append(clazz.getSimpleName());
-    }
+        private void formatClass(StringBuilder builder, Class<?> clazz) {
+          builder.append(clazz.getSimpleName());
+        }
 
-    private void formatTypeVariable(StringBuilder builder, TypeVariable<?> t) {
-      builder.append(t.getName());
-    }
+        private void formatTypeVariable(StringBuilder builder, TypeVariable<?> t) {
+          builder.append(t.getName());
+        }
 
-    private void formatWildcardType(StringBuilder builder, WildcardType t) {
-      builder.append("?");
-      for (Type lowerBound : t.getLowerBounds()) {
-        builder.append(" super ");
-        format(builder, lowerBound);
-      }
-      for (Type upperBound : t.getUpperBounds()) {
-        if (!Object.class.equals(upperBound)) {
-          builder.append(" extends ");
-          format(builder, upperBound);
+        private void formatWildcardType(StringBuilder builder, WildcardType t) {
+          builder.append("?");
+          for (Type lowerBound : t.getLowerBounds()) {
+            builder.append(" super ");
+            format(builder, lowerBound);
+          }
+          for (Type upperBound : t.getUpperBounds()) {
+            if (!Object.class.equals(upperBound)) {
+              builder.append(" extends ");
+              format(builder, upperBound);
+            }
+          }
         }
-      }
-    }
 
-    private void formatParameterizedType(StringBuilder builder, ParameterizedType t) {
-      format(builder, t.getRawType());
-      builder.append('<');
-      COMMA_SEPARATOR.appendTo(builder,
-          FluentIterable.from(asList(t.getActualTypeArguments()))
-          .transform(TYPE_SIMPLE_DESCRIPTION));
-      builder.append('>');
-    }
+        private void formatParameterizedType(StringBuilder builder, ParameterizedType t) {
+          format(builder, t.getRawType());
+          builder.append('<');
+          COMMA_SEPARATOR.appendTo(builder,
+              FluentIterable.from(asList(t.getActualTypeArguments()))
+                  .transform(TYPE_SIMPLE_DESCRIPTION));
+          builder.append('>');
+        }
 
-    private void formatGenericArrayType(StringBuilder builder, GenericArrayType t) {
-      format(builder, t.getGenericComponentType());
-      builder.append("[]");
-    }
-  };
+        private void formatGenericArrayType(StringBuilder builder, GenericArrayType t) {
+          format(builder, t.getGenericComponentType());
+          builder.append("[]");
+        }
+      };
 
   /**
    * Returns all the methods visible from the provided interfaces.
@@ -164,7 +176,7 @@ public static Iterable<Method> getClosureOfMethodsOnInterfaces(
           public Iterable<Method> apply(@Nonnull Class<?> input) {
             return getClosureOfMethodsOnInterface(input);
           }
-    });
+        });
   }
 
   /**
@@ -186,4 +198,19 @@ public static Iterable<Method> getClosureOfMethodsOnInterface(Class<?> iface) {
     }
     return builder.build();
   }
+
+  /**
+   * Returns a concise string for printing a {@link Annotation}.
+   *
+   * @param annotation The {@link Annotation} to print.
+   * @return a concise string representation that doesn't contain the package name.
+   */
+  public static String toStringForPrint(Annotation annotation) {
+    String annotationName = annotation.annotationType().getName();
+    String annotationNameWithoutPackage =
+        annotationName.substring(annotationName.lastIndexOf('.') + 1).replace('$', '.');
+    String annotationToString = annotation.toString();
+    String values = annotationToString.substring(annotationToString.indexOf('('));
+    return String.format("%s%s", annotationNameWithoutPackage, values);
+  }
 }
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
@@ -561,8 +561,10 @@ public void testGettersAnnotatedWithInconsistentDefault() throws Exception {
     expectedException.expect(IllegalArgumentException.class);
     expectedException.expectMessage(
         "Property [object] is marked with contradictory annotations. Found ["
-            + "[Default.Integer(value=1) on GetterWithDefault.getObject], "
-            + "[Default.String(value=abc) on GetterWithInconsistentDefaultType.getObject]].");
+            + "[Default.Integer(value=1) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GetterWithDefault#getObject()], "
+            + "[Default.String(value=abc) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GetterWithInconsistentDefaultType#getObject()]].");
 
     // When we attempt to convert, we should error at this moment.
     options.as(GetterWithInconsistentDefaultType.class);
@@ -576,8 +578,10 @@ public void testGettersAnnotatedWithInconsistentDefaultValue() throws Exception
     expectedException.expect(IllegalArgumentException.class);
     expectedException.expectMessage(
         "Property [object] is marked with contradictory annotations. Found ["
-            + "[Default.Integer(value=1) on GetterWithDefault.getObject], "
-            + "[Default.Integer(value=0) on GetterWithInconsistentDefaultValue.getObject]].");
+            + "[Default.Integer(value=1) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GetterWithDefault#getObject()], "
+            + "[Default.Integer(value=0) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GetterWithInconsistentDefaultValue#getObject()]].");
 
     // When we attempt to convert, we should error at this moment.
     options.as(GetterWithInconsistentDefaultValue.class);
@@ -591,8 +595,10 @@ public void testGettersAnnotatedWithInconsistentJsonIgnoreValue() throws Excepti
     expectedException.expect(IllegalArgumentException.class);
     expectedException.expectMessage(
         "Property [object] is marked with contradictory annotations. Found ["
-            + "[JsonIgnore(value=false) on GetterWithInconsistentJsonIgnoreValue.getObject], "
-            + "[JsonIgnore(value=true) on GetterWithJsonIgnore.getObject]].");
+            + "[JsonIgnore(value=false) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GetterWithInconsistentJsonIgnoreValue#getObject()], "
+            + "[JsonIgnore(value=true) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GetterWithJsonIgnore#getObject()]].");
 
     // When we attempt to convert, we should error at this moment.
     options.as(GetterWithInconsistentJsonIgnoreValue.class);
@@ -610,8 +616,10 @@ public void testGettersWithMultipleDefaults() throws Exception {
     expectedException.expect(IllegalArgumentException.class);
     expectedException.expectMessage(
         "Property [object] is marked with contradictory annotations. Found ["
-            + "[Default.String(value=abc) on GettersWithMultipleDefault.getObject], "
-            + "[Default.Integer(value=0) on GettersWithMultipleDefault.getObject]].");
+            + "[Default.String(value=abc) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GettersWithMultipleDefault#getObject()], "
+            + "[Default.Integer(value=0) on org.apache.beam.sdk.options.PipelineOptionsFactoryTest"
+            + "$GettersWithMultipleDefault#getObject()]].");
 
     // When we attempt to create, we should error at this moment.
     PipelineOptionsFactory.as(GettersWithMultipleDefault.class);
@@ -672,19 +680,6 @@ public void testMultipleGettersWithInconsistentDefault() {
     options.as(MultipleGettersWithInconsistentDefault.class);
   }
 
-  @Test
-  public void testToStringForPrint() throws Exception {
-    assertEquals(
-        "Default.Integer(value=1)",
-        PipelineOptionsFactory.toStringForPrint(
-            GetterWithDefault.class.getMethod("getObject").getAnnotations()[0]));
-
-    assertEquals(
-        "JsonIgnore(value=true)",
-        PipelineOptionsFactory.toStringForPrint(
-            GetterWithJsonIgnore.class.getMethod("getObject").getAnnotations()[0]));
-  }
-
   @Test
   public void testAppNameIsNotOverriddenWhenPassedInViaCommandLine() {
     ApplicationNameOptions options = PipelineOptionsFactory

diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/util/common/ReflectHelpersTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/util/common/ReflectHelpersTest.java
@@ -19,8 +19,11 @@
 
 import static org.junit.Assert.assertEquals;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import java.util.List;
 import java.util.Map;
+import org.apache.beam.sdk.options.Default;
+import org.apache.beam.sdk.options.PipelineOptions;
 import org.apache.beam.sdk.values.TypeDescriptor;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -123,4 +126,26 @@ public <InputT, OutputT> void testTypeFormatterWithMultipleWildcards() throws Ex
         ReflectHelpers.TYPE_SIMPLE_DESCRIPTION.apply(
             new TypeDescriptor<Map<? super InputT, ? extends OutputT>>() {}.getType()));
   }
+
+  private interface Options extends PipelineOptions {
+    @Default.Integer(1)
+    Integer getInteger();
+
+    @JsonIgnore
+    Object getObject();
+  }
+
+  @Test
+  public void testToStringForPrint() throws Exception {
+    assertEquals(
+        "Default.Integer(value=1)",
+        ReflectHelpers.toStringForPrint(
+            Options.class.getMethod("getInteger").getAnnotations()[0]));
+
+    assertEquals(
+        "JsonIgnore(value=true)",
+        ReflectHelpers.toStringForPrint(
+            Options.class.getMethod("getObject").getAnnotations()[0]));
+  }
+
 }