Repository: incubator-beam Updated Branches: refs/heads/master 6a05cf4a9 -> bdd71c4bc
[BEAM-790] Validate PipelineOptions Default annotation Project: http://git-wip-us.apache.org/repos/asf/incubator-beam/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-beam/commit/f605b02a Tree: http://git-wip-us.apache.org/repos/asf/incubator-beam/tree/f605b02a Diff: http://git-wip-us.apache.org/repos/asf/incubator-beam/diff/f605b02a Branch: refs/heads/master Commit: f605b02a72258aee20d607e54a443113bfb762ad Parents: 6a05cf4 Author: Pei He <pe...@google.com> Authored: Thu Oct 20 18:07:35 2016 -0700 Committer: Luke Cwik <lc...@google.com> Committed: Thu Nov 3 11:38:42 2016 -0700 ---------------------------------------------------------------------- .../sdk/options/PipelineOptionsFactory.java | 248 ++++++++++++++----- .../sdk/options/ProxyInvocationHandler.java | 5 +- .../beam/sdk/util/common/ReflectHelpers.java | 17 ++ .../sdk/options/PipelineOptionsFactoryTest.java | 239 ++++++++++++++++++ .../sdk/options/ProxyInvocationHandlerTest.java | 29 ++- .../sdk/util/common/ReflectHelpersTest.java | 25 ++ 6 files changed, 491 insertions(+), 72 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/f605b02a/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 eefe8c7..304e166 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 @@ -1057,56 +1057,130 @@ public class PipelineOptionsFactory { methodNameToAllMethodMap.put(method, method); } - List<InconsistentlyIgnoredGetters> incompletelyIgnoredGetters = new ArrayList<>(); - List<IgnoredSetter> ignoredSetters = new ArrayList<>(); + // Verify that there is no getter with a mixed @JsonIgnore annotation. + validateGettersHaveConsistentAnnotation( + methodNameToAllMethodMap, descriptors, AnnotationPredicates.JSON_IGNORE); - for (PropertyDescriptor descriptor : descriptors) { + // Verify that there is no getter with a mixed @Default annotation. + validateGettersHaveConsistentAnnotation( + methodNameToAllMethodMap, descriptors, AnnotationPredicates.DEFAULT_VALUE); + + // Verify that no setter has @JsonIgnore. + validateSettersDoNotHaveAnnotation( + methodNameToAllMethodMap, descriptors, AnnotationPredicates.JSON_IGNORE); + + // Verify that no setter has @Default. + validateSettersDoNotHaveAnnotation( + methodNameToAllMethodMap, descriptors, AnnotationPredicates.DEFAULT_VALUE); + } + + /** + * Validates that getters don't have mixed annotation. + */ + private static void validateGettersHaveConsistentAnnotation( + SortedSetMultimap<Method, Method> methodNameToAllMethodMap, + List<PropertyDescriptor> descriptors, + final AnnotationPredicates annotationPredicates) { + List<InconsistentlyAnnotatedGetters> inconsistentlyAnnotatedGetters = new ArrayList<>(); + for (final PropertyDescriptor descriptor : descriptors) { if (descriptor.getReadMethod() == null - || descriptor.getWriteMethod() == null - || IGNORED_METHODS.contains(descriptor.getReadMethod()) - || IGNORED_METHODS.contains(descriptor.getWriteMethod())) { + || IGNORED_METHODS.contains(descriptor.getReadMethod())) { continue; } - // Verify that there is no getter with a mixed @JsonIgnore annotation and verify - // that no setter has @JsonIgnore. + SortedSet<Method> getters = methodNameToAllMethodMap.get(descriptor.getReadMethod()); - SortedSet<Method> gettersWithJsonIgnore = Sets.filter(getters, JsonIgnorePredicate.INSTANCE); + SortedSet<Method> gettersWithTheAnnotation = + Sets.filter(getters, annotationPredicates.forMethod); + Set<Annotation> distinctAnnotations = Sets.newLinkedHashSet(FluentIterable + .from(gettersWithTheAnnotation) + .transformAndConcat(new Function<Method, Iterable<? extends Annotation>>() { + @Nonnull + @Override + public Iterable<? extends Annotation> apply(@Nonnull Method method) { + return FluentIterable.of(method.getAnnotations()); + } + }) + .filter(annotationPredicates.forAnnotation)); + + + if (distinctAnnotations.size() > 1) { + throw new IllegalArgumentException(String.format( + "Property [%s] is marked with contradictory annotations. Found [%s].", + descriptor.getName(), + FluentIterable.from(gettersWithTheAnnotation) + .transformAndConcat(new Function<Method, Iterable<String>>() { + @Nonnull + @Override + public Iterable<String> apply(final @Nonnull Method method) { + return FluentIterable.of(method.getAnnotations()) + .filter(annotationPredicates.forAnnotation) + .transform(new Function<Annotation, String>() { + @Nonnull + @Override + public String apply(@Nonnull Annotation annotation) { + return String.format( + "[%s on %s]", + ReflectHelpers.ANNOTATION_FORMATTER.apply(annotation), + ReflectHelpers.CLASS_AND_METHOD_FORMATTER.apply(method)); + } + }); + + } + }) + .join(Joiner.on(", ")))); + } Iterable<String> getterClassNames = FluentIterable.from(getters) .transform(MethodToDeclaringClassFunction.INSTANCE) .transform(ReflectHelpers.CLASS_NAME); - Iterable<String> gettersWithJsonIgnoreClassNames = FluentIterable.from(gettersWithJsonIgnore) + Iterable<String> gettersWithTheAnnotationClassNames = + FluentIterable.from(gettersWithTheAnnotation) .transform(MethodToDeclaringClassFunction.INSTANCE) .transform(ReflectHelpers.CLASS_NAME); - if (!(gettersWithJsonIgnore.isEmpty() || getters.size() == gettersWithJsonIgnore.size())) { - InconsistentlyIgnoredGetters err = new InconsistentlyIgnoredGetters(); + if (!(gettersWithTheAnnotation.isEmpty() + || getters.size() == gettersWithTheAnnotation.size())) { + InconsistentlyAnnotatedGetters err = new InconsistentlyAnnotatedGetters(); err.descriptor = descriptor; err.getterClassNames = getterClassNames; - err.gettersWithJsonIgnoreClassNames = gettersWithJsonIgnoreClassNames; - incompletelyIgnoredGetters.add(err); + err.gettersWithTheAnnotationClassNames = gettersWithTheAnnotationClassNames; + inconsistentlyAnnotatedGetters.add(err); } - if (!incompletelyIgnoredGetters.isEmpty()) { + } + throwForGettersWithInconsistentAnnotation( + inconsistentlyAnnotatedGetters, annotationPredicates.annotationClass); + } + + /** + * Validates that setters don't have the given annotation. + */ + private static void validateSettersDoNotHaveAnnotation( + SortedSetMultimap<Method, Method> methodNameToAllMethodMap, + List<PropertyDescriptor> descriptors, + 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()), + annotationPredicates.forMethod); - SortedSet<Method> settersWithJsonIgnore = - Sets.filter(methodNameToAllMethodMap.get(descriptor.getWriteMethod()), - JsonIgnorePredicate.INSTANCE); - - Iterable<String> settersWithJsonIgnoreClassNames = FluentIterable.from(settersWithJsonIgnore) + Iterable<String> settersWithTheAnnotationClassNames = + FluentIterable.from(settersWithTheAnnotation) .transform(MethodToDeclaringClassFunction.INSTANCE) .transform(ReflectHelpers.CLASS_NAME); - if (!settersWithJsonIgnore.isEmpty()) { - IgnoredSetter ignored = new IgnoredSetter(); - ignored.descriptor = descriptor; - ignored.settersWithJsonIgnoreClassNames = settersWithJsonIgnoreClassNames; - ignoredSetters.add(ignored); + if (!settersWithTheAnnotation.isEmpty()) { + AnnotatedSetter annotated = new AnnotatedSetter(); + annotated.descriptor = descriptor; + annotated.settersWithTheAnnotationClassNames = settersWithTheAnnotationClassNames; + annotatedSetters.add(annotated); } } - throwForGettersWithInconsistentJsonIgnore(incompletelyIgnoredGetters); - throwForSettersWithJsonIgnore(ignoredSetters); + throwForSettersWithTheAnnotation(annotatedSetters, annotationPredicates.annotationClass); } /** @@ -1221,53 +1295,62 @@ public class PipelineOptionsFactory { } } - private static class InconsistentlyIgnoredGetters { + private static class InconsistentlyAnnotatedGetters { PropertyDescriptor descriptor; Iterable<String> getterClassNames; - Iterable<String> gettersWithJsonIgnoreClassNames; + Iterable<String> gettersWithTheAnnotationClassNames; } - private static void throwForGettersWithInconsistentJsonIgnore( - List<InconsistentlyIgnoredGetters> getters) { + private static void throwForGettersWithInconsistentAnnotation( + List<InconsistentlyAnnotatedGetters> getters, + Class<? extends Annotation> annotationClass) { if (getters.size() == 1) { - InconsistentlyIgnoredGetters getter = getters.get(0); + InconsistentlyAnnotatedGetters getter = getters.get(0); throw new IllegalArgumentException(String.format( - "Expected getter for property [%s] to be marked with @JsonIgnore on all %s, " + "Expected getter for property [%s] to be marked with @%s on all %s, " + "found only on %s", - getter.descriptor.getName(), getter.getterClassNames, - getter.gettersWithJsonIgnoreClassNames)); + getter.descriptor.getName(), + annotationClass.getSimpleName(), + getter.getterClassNames, + getter.gettersWithTheAnnotationClassNames)); } else if (getters.size() > 1) { - StringBuilder errorBuilder = - 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())); + for (InconsistentlyAnnotatedGetters getter : getters) { errorBuilder.append( String.format("%n - Expected for property [%s] to be marked on all %s, " + "found only on %s", getter.descriptor.getName(), getter.getterClassNames, - getter.gettersWithJsonIgnoreClassNames)); + getter.gettersWithTheAnnotationClassNames)); } throw new IllegalArgumentException(errorBuilder.toString()); } } - private static class IgnoredSetter { + private static class AnnotatedSetter { PropertyDescriptor descriptor; - Iterable<String> settersWithJsonIgnoreClassNames; + Iterable<String> settersWithTheAnnotationClassNames; } - private static void throwForSettersWithJsonIgnore(List<IgnoredSetter> setters) { + private static void throwForSettersWithTheAnnotation( + List<AnnotatedSetter> setters, + Class<? extends Annotation> annotationClass) { if (setters.size() == 1) { - IgnoredSetter setter = setters.get(0); - throw new IllegalArgumentException( - String.format("Expected setter for property [%s] to not be marked with @JsonIgnore on %s", - 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", + setter.descriptor.getName(), + annotationClass.getSimpleName(), + setter.settersWithTheAnnotationClassNames)); } else if (setters.size() > 1) { - StringBuilder builder = new StringBuilder("Found setters marked with @JsonIgnore:"); - for (IgnoredSetter setter : setters) { - builder.append( - String.format("%n - Setter for property [%s] should not be marked with @JsonIgnore " - + "on %s", - setter.descriptor.getName(), setter.settersWithJsonIgnoreClassNames)); + StringBuilder builder = new StringBuilder( + String.format("Found setters marked with @%s:", annotationClass.getSimpleName())); + for (AnnotatedSetter setter : setters) { + builder.append(String.format( + "%n - Setter for property [%s] should not be marked with @%s on %s", + setter.descriptor.getName(), + annotationClass.getSimpleName(), + setter.settersWithTheAnnotationClassNames)); } throw new IllegalArgumentException(builder.toString()); } @@ -1353,14 +1436,61 @@ public class PipelineOptionsFactory { } /** - * A {@link Predicate} that returns true if the method is annotated with - * {@link JsonIgnore @JsonIgnore}. + * A {@link Predicate} that returns true if the method is annotated with {@code annotationClass}. */ - static class JsonIgnorePredicate implements Predicate<Method> { - static final JsonIgnorePredicate INSTANCE = new JsonIgnorePredicate(); - @Override - public boolean apply(Method input) { - return input.isAnnotationPresent(JsonIgnore.class); + static class AnnotationPredicates { + static final AnnotationPredicates JSON_IGNORE = new AnnotationPredicates( + JsonIgnore.class, + new Predicate<Annotation>() { + @Override + public boolean apply(@Nonnull Annotation input) { + return JsonIgnore.class.equals(input.annotationType()); + } + }, + 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()); + } + }, + new Predicate<Method> () { + @Override + public boolean apply(@Nonnull Method input) { + for (Annotation annotation : input.getAnnotations()) { + if (DEFAULT_ANNOTATION_CLASSES.contains(annotation.annotationType())) { + return true; + } + } + return false; + }}); + + final Class<? extends Annotation> annotationClass; + final Predicate<Annotation> forAnnotation; + final Predicate<Method> forMethod; + + AnnotationPredicates( + Class<? extends Annotation> annotationClass, + Predicate<Annotation> forAnnotation, + Predicate<Method> forMethod) { + this.annotationClass = annotationClass; + this.forAnnotation = forAnnotation; + this.forMethod = forMethod; } } http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/f605b02a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java ---------------------------------------------------------------------- diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index f7c6da2..86f9918 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -63,7 +63,7 @@ import java.util.TreeMap; import java.util.concurrent.ThreadLocalRandom; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; -import org.apache.beam.sdk.options.PipelineOptionsFactory.JsonIgnorePredicate; +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 @@ class ProxyInvocationHandler implements InvocationHandler { // Find all the method names that are annotated with JSON ignore. Set<String> jsonIgnoreMethodNames = FluentIterable.from( ReflectHelpers.getClosureOfMethodsOnInterfaces(interfaces)) - .filter(JsonIgnorePredicate.INSTANCE).transform(new Function<Method, String>() { + .filter(AnnotationPredicates.JSON_IGNORE.forMethod) + .transform(new Function<Method, String>() { @Override public String apply(Method input) { return input.getName(); http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/f605b02a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/common/ReflectHelpers.java ---------------------------------------------------------------------- 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 index 2d92162..2b08fee 100644 --- 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.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; @@ -86,6 +87,22 @@ public class ReflectHelpers { } }; + /** + * A {@link Function} that returns a concise string for a {@link Annotation}. + */ + public static final Function<Annotation, String> ANNOTATION_FORMATTER = + new Function<Annotation, String>() { + @Override + public String apply(@Nonnull 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); + } + }; + /** A {@link Function} that formats types. */ public static final Function<Type, String> TYPE_SIMPLE_DESCRIPTION = new Function<Type, String>() { http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/f605b02a/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 64d8c51..0a2324f 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 @@ -369,6 +369,16 @@ public class PipelineOptionsFactoryTest { void setObject(Object value); } + /** + * This class is has a conflicting {@link JsonIgnore @JsonIgnore} value with + * {@link GetterWithJsonIgnore}. + */ + public interface GetterWithInconsistentJsonIgnoreValue extends PipelineOptions { + @JsonIgnore(value = false) + Object getObject(); + void setObject(Object value); + } + @Test public void testNotAllGettersAnnotatedWithJsonIgnore() throws Exception { // Initial construction is valid. @@ -441,6 +451,235 @@ public class PipelineOptionsFactoryTest { options.as(MultipleGettersWithInconsistentJsonIgnore.class); } + /** Test interface that has {@link Default @Default} on a setter for a property. */ + public interface SetterWithDefault extends PipelineOptions { + String getValue(); + @Default.String("abc") + void setValue(String value); + } + + @Test + public void testSetterAnnotatedWithDefault() throws Exception { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage( + "Expected setter for property [value] to not be marked with @Default on [" + + "org.apache.beam.sdk.options.PipelineOptionsFactoryTest$SetterWithDefault]"); + PipelineOptionsFactory.as(SetterWithDefault.class); + } + + /** Test interface that has {@link Default @Default} on multiple setters. */ + public interface MultiSetterWithDefault extends SetterWithDefault { + Integer getOther(); + @Default.String("abc") + void setOther(Integer other); + } + + @Test + public void testMultipleSettersAnnotatedWithDefault() throws Exception { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Found setters marked with @Default:"); + expectedException.expectMessage( + "property [other] should not be marked with @Default on [" + + "org.apache.beam.sdk.options.PipelineOptionsFactoryTest$MultiSetterWithDefault]"); + expectedException.expectMessage( + "property [value] should not be marked with @Default on [" + + "org.apache.beam.sdk.options.PipelineOptionsFactoryTest$SetterWithDefault]"); + PipelineOptionsFactory.as(MultiSetterWithDefault.class); + } + + /** + * This class is has a conflicting field with {@link CombinedObject} that doesn't have + * {@link Default @Default}. + */ + 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 GetterWithInconsistentDefaultType 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. + GetterWithDefault options = PipelineOptionsFactory.as(GetterWithDefault.class); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage( + "Expected getter for property [object] to be marked with @Default on all [" + + "org.apache.beam.sdk.options.PipelineOptionsFactoryTest$GetterWithDefault, " + + "org.apache.beam.sdk.options.PipelineOptionsFactoryTest$MissingSetter], " + + "found only on [org.apache.beam.sdk.options." + + "PipelineOptionsFactoryTest$GetterWithDefault]"); + + // When we attempt to convert, we should error at this moment. + 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 contradictory annotations. Found [" + + "[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); + } + + @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 contradictory annotations. Found [" + + "[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); + } + + @Test + public void testGettersAnnotatedWithInconsistentJsonIgnoreValue() throws Exception { + // Initial construction is valid. + GetterWithJsonIgnore options = PipelineOptionsFactory.as(GetterWithJsonIgnore.class); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage( + "Property [object] is marked with contradictory annotations. Found [" + + "[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); + } + + 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( + "Property [object] is marked with contradictory annotations. Found [" + + "[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); + } + + private interface MultiGettersWithDefault extends PipelineOptions { + Object getObject(); + void setObject(Object value); + + @Default.Integer(1) + Integer getOther(); + void setOther(Integer value); + + Void getConsistent(); + void setConsistent(Void consistent); + } + + private interface MultipleGettersWithInconsistentDefault extends PipelineOptions { + @Default.Boolean(true) + Object getObject(); + void setObject(Object value); + + Integer getOther(); + void setOther(Integer value); + + Void getConsistent(); + void setConsistent(Void consistent); + } + + @Test + public void testMultipleGettersWithInconsistentDefault() { + // Initial construction is valid. + MultiGettersWithDefault options = PipelineOptionsFactory.as(MultiGettersWithDefault.class); + + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Property getters are inconsistently marked with @Default:"); + expectedException.expectMessage( + "property [object] to be marked on all"); + expectedException.expectMessage("found only on [org.apache.beam.sdk.options." + + "PipelineOptionsFactoryTest$MultiGettersWithDefault]"); + expectedException.expectMessage( + "property [other] to be marked on all"); + expectedException.expectMessage("found only on [org.apache.beam.sdk.options." + + "PipelineOptionsFactoryTest$MultipleGettersWithInconsistentDefault]"); + + expectedException.expectMessage(Matchers.anyOf( + containsString(java.util.Arrays.toString(new String[] + {"org.apache.beam.sdk.options." + + "PipelineOptionsFactoryTest$MultipleGettersWithInconsistentDefault", + "org.apache.beam.sdk.options.PipelineOptionsFactoryTest$MultiGettersWithDefault"})), + containsString(java.util.Arrays.toString(new String[] + {"org.apache.beam.sdk.options.PipelineOptionsFactoryTest$MultiGettersWithDefault", + "org.apache.beam.sdk.options." + + "PipelineOptionsFactoryTest$MultipleGettersWithInconsistentDefault"})))); + expectedException.expectMessage(not(containsString("property [consistent]"))); + + // When we attempt to convert, we should error immediately + options.as(MultipleGettersWithInconsistentDefault.class); + } + @Test public void testAppNameIsNotOverriddenWhenPassedInViaCommandLine() { ApplicationNameOptions options = PipelineOptionsFactory http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/f605b02a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java ---------------------------------------------------------------------- diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java index 52b98ee..5e97eed 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java @@ -235,38 +235,45 @@ public class ProxyInvocationHandlerTest { .testEquals(); } + /** A test interface for string with default. */ + public interface StringWithDefault extends PipelineOptions { + @Default.String("testString") + String getString(); + void setString(String value); + } + @Test public void testToString() throws Exception { ProxyInvocationHandler handler = new ProxyInvocationHandler(Maps.<String, Object>newHashMap()); - Simple proxy = handler.as(Simple.class); + StringWithDefault proxy = handler.as(StringWithDefault.class); proxy.setString("stringValue"); DefaultAnnotations proxy2 = proxy.as(DefaultAnnotations.class); proxy2.setLong(57L); assertEquals("Current Settings:\n" - + " long: 57\n" - + " string: stringValue\n", + + " long: 57\n" + + " string: stringValue\n", proxy.toString()); } @Test public void testToStringAfterDeserializationContainsJsonEntries() throws Exception { ProxyInvocationHandler handler = new ProxyInvocationHandler(Maps.<String, Object>newHashMap()); - Simple proxy = handler.as(Simple.class); + StringWithDefault proxy = handler.as(StringWithDefault.class); Long optionsId = proxy.getOptionsId(); proxy.setString("stringValue"); DefaultAnnotations proxy2 = proxy.as(DefaultAnnotations.class); proxy2.setLong(57L); assertEquals(String.format("Current Settings:\n" - + " long: 57\n" - + " optionsId: %d\n" - + " string: \"stringValue\"\n", optionsId), + + " long: 57\n" + + " optionsId: %d\n" + + " string: \"stringValue\"\n", optionsId), serializeDeserialize(PipelineOptions.class, proxy2).toString()); } @Test public void testToStringAfterDeserializationContainsOverriddenEntries() throws Exception { ProxyInvocationHandler handler = new ProxyInvocationHandler(Maps.<String, Object>newHashMap()); - Simple proxy = handler.as(Simple.class); + StringWithDefault proxy = handler.as(StringWithDefault.class); Long optionsId = proxy.getOptionsId(); proxy.setString("stringValue"); DefaultAnnotations proxy2 = proxy.as(DefaultAnnotations.class); @@ -274,9 +281,9 @@ public class ProxyInvocationHandlerTest { Simple deserializedOptions = serializeDeserialize(Simple.class, proxy2); deserializedOptions.setString("overriddenValue"); assertEquals(String.format("Current Settings:\n" - + " long: 57\n" - + " optionsId: %d\n" - + " string: overriddenValue\n", optionsId), + + " long: 57\n" + + " optionsId: %d\n" + + " string: overriddenValue\n", optionsId), deserializedOptions.toString()); } http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/f605b02a/sdks/java/core/src/test/java/org/apache/beam/sdk/util/common/ReflectHelpersTest.java ---------------------------------------------------------------------- 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 index e1073da..8a1708c 100644 --- 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 @@ package org.apache.beam.sdk.util.common; 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 class ReflectHelpersTest { ReflectHelpers.TYPE_SIMPLE_DESCRIPTION.apply( new TypeDescriptor<Map<? super InputT, ? extends OutputT>>() {}.getType())); } + + private interface Options extends PipelineOptions { + @Default.String("package.OuterClass$InnerClass#method()") + String getString(); + + @JsonIgnore + Object getObject(); + } + + @Test + public void testAnnotationFormatter() throws Exception { + assertEquals( + "Default.String(value=package.OuterClass$InnerClass#method())", + ReflectHelpers.ANNOTATION_FORMATTER.apply( + Options.class.getMethod("getString").getAnnotations()[0])); + + assertEquals( + "JsonIgnore(value=true)", + ReflectHelpers.ANNOTATION_FORMATTER.apply( + Options.class.getMethod("getObject").getAnnotations()[0])); + } + }