[BEAM-790] Validate PipelineOptions Default annotation. #1159

peihe · 2016-10-22T00:01:04Z

No description provided.

peihe · 2016-10-22T00:11:01Z

lukecwik · 2016-10-25T16:43:52Z

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

+  /**
+   * Validates that getters don't have mixed annotation.
+   */
+  private static void validateGettersNoMixedAnnotations(


validateGettersNoMixedAnnotations -> validateGettersHaveConsistentAnnotation

lukecwik · 2016-10-25T16:44:25Z

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

+      SortedSetMultimap<Method, Method> methodNameToAllMethodMap,
+      List<PropertyDescriptor> descriptors,
+      AnnotationPredicate annotationPredicate) {
+    List<InconsistentlyAnnotatedGetters> incompletelyAnnotatedGetters = new ArrayList<>();


incompletelyAnnotatedGetters -> inconsistentlyAnnotatedGetters

lukecwik · 2016-10-25T16:46:27Z

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

+  /**
+   * Validates that setters don't have the given annotation.
+   */
+  private static void validateSettersNoAnnotations(


validateSettersNoAnnotations -> validateSettersDoNotHaveAnnotation

lukecwik · 2016-10-25T16:47:44Z

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

      SortedSet<Method> getters = methodNameToAllMethodMap.get(descriptor.getReadMethod());
-      SortedSet<Method> gettersWithJsonIgnore = Sets.filter(getters, JsonIgnorePredicate.INSTANCE);
+      SortedSet<Method> gettersWithTheAnnotation = Sets.filter(getters, annotationPredicate);


In the case of @default, this is missing the logic to make sure that the two distinct annotation instances are not equal.

So this will currently say that @Default.String("a") and @Default.String("b") are equivalent which is incorrect.

Conveniently you can use Annotation#equals as it does the right thing:
https://docs.oracle.com/javase/7/docs/api/java/lang/annotation/Annotation.html#equals(java.lang.Object)

done

I think I didn't check the equivalency of the Default at all. Having @Default.String("a") and @Default.Integer(0) on a getter was considered okay.

fixed

lukecwik · 2016-10-25T16:48:43Z

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

-        + "  optionsId: %d\n"
-        + "  string: overriddenValue\n", optionsId),
-        deserializedOptions.toString());
+  public void testMultipleDefaultsThrow() throws Exception {


Why is this test here and why are the other tests removed?

All three tests becomes illegal (cast between Simple.class and DefaultAnnotations.class).

So, only one is needed.

I think your removing the purpose of those existing tests which was to testToString in various cases. It seems as though using two valid combinations of classes which are compatible is all that needs to be done to fix the test.

lukecwik · 2016-10-25T16:50:09Z

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

+    Void getConsistent();
+    void setConsistent(Void consistent);
+  }
+


Add a test case where the default annotation is the same class but has a different value.

done + more cases

lukecwik · 2016-10-25T16:51:59Z

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

+    private final Predicate<Method> predicate;
+    private final Class<? extends Annotation> annotationClass;
+
+    AnnotationPredicate(Predicate<Method> predicate, Class<? extends Annotation> annotationClass) {


If you make the AnnotationPredicate an abstract class, you don't need to pass in the Predicate into the constructor and don't need the delegate calling implementation defined below.

I found I need two predicates, one for method and another for annotation.
So, I need to use composition.

Are you sure?, your currently not using composition within the AnnotationPredicates class.

lukecwik · 2016-10-25T16:55:52Z

.travis.yml

@@ -45,6 +45,7 @@ matrix:

 before_install:
  # Workaround for https://github.com/travis-ci/travis-ci/issues/4629
+  - touch ~/.m2/settings.xml


Why is this included in this PR?

removed

It was to test the fix for travis.

peihe

PTAL

lukecwik · 2016-10-26T16:01:17Z

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

+            @Nullable
+            @Override
+            public Annotation apply(@Nonnull Method method) {
+              FluentIterable<Annotation> methodAnnotations = FluentIterable


Instead of doing the filtering here, why not chain this into the FluentIterable above

.from(...)
.transformAndConcat(Method -> Iterable)
.filter(annotationPredicates.forAnnotation)

would also allow you to drop the 'final' declaration on annotationPredicates

done
but need to keep final for another code

lukecwik · 2016-10-26T16:04:13Z

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

      SortedSetMultimap<Method, Method> methodNameToAllMethodMap,
      List<PropertyDescriptor> descriptors,
-      AnnotationPredicate annotationPredicate) {
+      final AnnotationPredicates annotationPredicates) {


lukecwik · 2016-10-26T16:09:05Z

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

+        new Predicate<Annotation>() {
+          @Override
+          public boolean apply(@Nonnull Annotation input) {
+            return input.annotationType().equals(JsonIgnore.class);


use instanceof or swap the direction so its JsonIgnore.class.equals(input.annotationType)

lukecwik · 2016-10-26T16:19:36Z

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

+          .filter(Predicates.notNull()));
+
+      checkArgument(propertyAnnotations.size() <= 1,
+          "Property [%s] is marked with multiple annotations: %s.",


the error isn't that there are multiple annotations but they aren't all equivalent.
with multiple annotations -> with contradictory annotations

also, your going to make people search for where they are if you don't include the mapping from annotation to methods.

Something like:
Property [%s] is marked with contradictory annotations. Found [annotation1] on [methods], [annotation2] on [other methods], ...

lukecwik · 2016-10-26T16:20:57Z

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

+   * This class is inconsistent with {@link GetterWithDefault} that has a different
+   * {@link Default @Default}.
+   */
+  private interface GetterWithInconsistentDefault extends PipelineOptions {


GetterWithInconsistentDefault -> GetterWithInconsistentDefaultType

lukecwik · 2016-10-26T16:33:35Z

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

-              setter.descriptor.getName(), setter.settersWithJsonIgnoreClassNames));
+      AnnotatedSetter setter = setters.get(0);
+      throw new IllegalArgumentException(String.format(
+          "Expected setter for property [%s] to not be marked with @%s on %s",


using the simple name drops the outer class name, would be good if it said @Default.String and not just @default

lukecwik · 2016-10-26T16:35:13Z

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

-                + "on %s",
-                setter.descriptor.getName(), setter.settersWithJsonIgnoreClassNames));
+      StringBuilder builder = new StringBuilder(
+          String.format("Found setters marked with @%s:", annotationClass.getSimpleName()));


Ditto here on usage of simplename since it will drop the outer class name

lukecwik · 2016-10-26T16:35:46Z

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

-          getter.descriptor.getName(), getter.getterClassNames,
-          getter.gettersWithJsonIgnoreClassNames));
+          getter.descriptor.getName(),
+          annotationClass.getSimpleName(),


Simple name will drop inner class name.

I think inner name is less useful here. When users see this error it is because one getter missed @Default.something.

After users added it, we have validation to verify the all Default annotations have to be the same.

Using toStringForPrint here (and elsewhere) would improve the consistency in how we represent information to the user.

The annotationClass is Default.class.
It doesn't fit toStringForPrint, which takes a annotation instance.

lukecwik · 2016-10-26T16:35:54Z

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

-          new StringBuilder("Property getters are inconsistently marked with @JsonIgnore:");
-      for (InconsistentlyIgnoredGetters getter : getters) {
+      StringBuilder errorBuilder = new StringBuilder(String.format(
+          "Property getters are inconsistently marked with @%s:", annotationClass.getSimpleName()));


Simple name will drop inner class name.

lukecwik · 2016-10-26T16:41:31Z

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

+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage(
+        "Property [object] is marked with multiple annotations: ["
+            + "@org.apache.beam.sdk.options.Default$Integer(value=1), "


If you use the canonical name and drop the org.apache.sdk.options inside then this would say @Default.Integer(value=1) which is easier to read.

peihe

PTAL

peihe · 2016-10-27T18:35:12Z

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

+            @Nullable
+            @Override
+            public Annotation apply(@Nonnull Method method) {
+              FluentIterable<Annotation> methodAnnotations = FluentIterable


done
but need to keep final for another code

peihe · 2016-10-27T18:35:17Z

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

+          .filter(Predicates.notNull()));
+
+      checkArgument(propertyAnnotations.size() <= 1,
+          "Property [%s] is marked with multiple annotations: %s.",


peihe · 2016-10-27T18:35:22Z

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

      SortedSetMultimap<Method, Method> methodNameToAllMethodMap,
      List<PropertyDescriptor> descriptors,
-      AnnotationPredicate annotationPredicate) {
+      final AnnotationPredicates annotationPredicates) {


peihe · 2016-10-27T18:35:28Z

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

+        new Predicate<Annotation>() {
+          @Override
+          public boolean apply(@Nonnull Annotation input) {
+            return input.annotationType().equals(JsonIgnore.class);


peihe · 2016-10-27T18:35:38Z

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

+   * This class is inconsistent with {@link GetterWithDefault} that has a different
+   * {@link Default @Default}.
+   */
+  private interface GetterWithInconsistentDefault extends PipelineOptions {


peihe · 2016-10-27T18:36:00Z

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

+    expectedException.expect(IllegalArgumentException.class);
+    expectedException.expectMessage(
+        "Property [object] is marked with multiple annotations: ["
+            + "@org.apache.beam.sdk.options.Default$Integer(value=1), "


peihe · 2016-10-27T18:41:15Z

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

-          getter.descriptor.getName(), getter.getterClassNames,
-          getter.gettersWithJsonIgnoreClassNames));
+          getter.descriptor.getName(),
+          annotationClass.getSimpleName(),


I think inner name is less useful here. When users see this error it is because one getter missed @Default.something.

After users added it, we have validation to verify the all Default annotations have to be the same.

peihe · 2016-10-27T18:41:40Z

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

-          new StringBuilder("Property getters are inconsistently marked with @JsonIgnore:");
-      for (InconsistentlyIgnoredGetters getter : getters) {
+      StringBuilder errorBuilder = new StringBuilder(String.format(
+          "Property getters are inconsistently marked with @%s:", annotationClass.getSimpleName()));


peihe · 2016-10-27T18:42:06Z

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

-              setter.descriptor.getName(), setter.settersWithJsonIgnoreClassNames));
+      AnnotatedSetter setter = setters.get(0);
+      throw new IllegalArgumentException(String.format(
+          "Expected setter for property [%s] to not be marked with @%s on %s",


peihe · 2016-10-27T18:42:14Z

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

-                + "on %s",
-                setter.descriptor.getName(), setter.settersWithJsonIgnoreClassNames));
+      StringBuilder builder = new StringBuilder(
+          String.format("Found setters marked with @%s:", annotationClass.getSimpleName()));


lukecwik · 2016-10-31T23:27:54Z

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

+                          @Override
+                          public String apply(@Nonnull Annotation annotation) {
+                            return String.format(
+                                "[%s on %s.%s]",


please rely on ReflectHelper.CLASS_AND_METHOD_FORMATTER for consistency in how we print out class+method names.

lukecwik · 2016-10-31T23:30:18Z

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

+  }
+
+  @VisibleForTesting
+  static String toStringForPrint(Annotation annotation) {


Add this as a function to ReflectHelpers like the other to string methods so we get pretty printed annotations done consistently elsewhere when it comes up.

lukecwik · 2016-10-31T23:35:30Z

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

-          getter.descriptor.getName(), getter.getterClassNames,
-          getter.gettersWithJsonIgnoreClassNames));
+          getter.descriptor.getName(),
+          annotationClass.getSimpleName(),


Using toStringForPrint here (and elsewhere) would improve the consistency in how we represent information to the user.

lukecwik · 2016-10-31T23:38:41Z

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

+  @VisibleForTesting
+  static String toStringForPrint(Annotation annotation) {
+    String toString = annotation.toString();
+    return toString.substring(toString.lastIndexOf('.') + 1).replace('$', '.');


what if the annotation is a string containing . and or $?

lukecwik · 2016-11-01T18:47:34Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/common/ReflectHelpers.java

+   * @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) {


Please make this a function object like the others in this class

peihe · 2016-11-01T18:58:09Z

PTAL

lukecwik

Minor comments and then LGTM

lukecwik · 2016-11-03T15:57:38Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/common/ReflectHelpers.java

+  /**
+   * A {@link Function} that returns a concise string for a {@link Annotation}.
+   */
+  public static final Function<Annotation, String> ANNOTATION_CONCISE_STRING =


ANNOTATION_CONCISE_STRING -> ANNOTATION_FORMATTER

lukecwik · 2016-11-03T15:58:21Z

sdks/java/core/src/test/java/org/apache/beam/sdk/util/common/ReflectHelpersTest.java

@@ -139,12 +139,12 @@ public void testTypeFormatterWithGenerics() throws Exception {
  public void testToStringForPrint() throws Exception {


testToStringForPrint -> testAnnotationFormatter

Add a test for a default value with a . and ( and $ in it so that we remember why we have string parsing logic setup the way we do.

peihe · 2016-11-03T18:06:40Z

PTAL

lukecwik · 2016-11-03T18:38:08Z

LGTM, merging

peihe force-pushed the validate-default-2 branch from 0e3d981 to ea63661 Compare October 22, 2016 00:09

peihe force-pushed the validate-default-2 branch 6 times, most recently from 9f9168e to d18d4fa Compare October 24, 2016 23:36

lukecwik requested changes Oct 25, 2016

View reviewed changes

lukecwik reviewed Oct 25, 2016

View reviewed changes

peihe added 2 commits October 25, 2016 20:12

[BEAM-790] Validate PipelineOptions Default annotation

e0dcb2a

fixup! address comments and more Default validations.

3e87aa6

peihe force-pushed the validate-default-2 branch from d18d4fa to 3e87aa6 Compare October 26, 2016 03:13

peihe commented Oct 26, 2016

View reviewed changes

lukecwik requested changes Oct 26, 2016

View reviewed changes

peihe commented Oct 27, 2016

View reviewed changes

fixup! address comments-2

f8b672e

peihe force-pushed the validate-default-2 branch from f886096 to f8b672e Compare October 27, 2016 18:57

lukecwik requested changes Oct 31, 2016

View reviewed changes

fixup! address comments-3

75ccba8

peihe force-pushed the validate-default-2 branch from 684c5e0 to 75ccba8 Compare November 1, 2016 18:46

lukecwik reviewed Nov 1, 2016

View reviewed changes

fixup! address comments

f7b108b

peihe force-pushed the validate-default-2 branch from 3514a89 to f7b108b Compare November 2, 2016 21:09

lukecwik requested changes Nov 3, 2016

View reviewed changes

fixup! address comments

0038aa5

asfgit closed this in bdd71c4 Nov 3, 2016

markflyhigh mentioned this pull request Nov 4, 2016

[BEAM-898] Fix Jenkins BigQueryTornadoes IT Failure #1279

Closed

4 tasks

		@@ -139,12 +139,12 @@ public void testTypeFormatterWithGenerics() throws Exception {
		public void testToStringForPrint() throws Exception {

[BEAM-790] Validate PipelineOptions Default annotation. #1159

[BEAM-790] Validate PipelineOptions Default annotation. #1159

Conversation

peihe commented Oct 22, 2016

peihe commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peihe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukecwik Oct 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukecwik Oct 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peihe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peihe commented Nov 1, 2016

lukecwik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peihe commented Nov 3, 2016

lukecwik commented Nov 3, 2016

lukecwik Oct 26, 2016 •

edited

lukecwik Oct 26, 2016 •

edited