fixup! address comments-3

apache · Nov 1, 2016 · 75ccba8 · 75ccba8
1 parent f8b672e
commit 75ccba8
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 31 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
@@ -26,6 +26,7 @@
 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;
@@ -186,4 +187,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]));
+  }
+
 }