Skip to content

Commit

Permalink
fixup! address comments-2
Browse files Browse the repository at this point in the history
  • Loading branch information
@peihe
peihe committed Oct 27, 2016
1 parent 3e87aa6 commit f886096
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 48 deletions.
70 changes: 44 additions & 26 deletions sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1092,33 +1092,45 @@ private static void validateGettersHaveConsistentAnnotation(
SortedSet<Method> getters = methodNameToAllMethodMap.get(descriptor.getReadMethod());
SortedSet<Method> gettersWithTheAnnotation =
Sets.filter(getters, annotationPredicates.forMethod);
Set<Annotation> propertyAnnotations = Sets.newLinkedHashSet(FluentIterable
Set<Annotation> distinctAnnotations = Sets.newLinkedHashSet(FluentIterable
.from(gettersWithTheAnnotation)
.transform(new Function<Method, Annotation>() {
@Nullable
.transformAndConcat(new Function<Method, Iterable<? extends Annotation>>() {
@Nonnull
@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()));
public Iterable<? extends Annotation> apply(@Nonnull Method method) {
return FluentIterable.of(method.getAnnotations());
}
})
.filter(annotationPredicates.forAnnotation));


checkArgument(propertyAnnotations.size() <= 1,
"Property [%s] is marked with multiple annotations: %s.",
descriptor.getName(),
propertyAnnotations);
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.%s]",
toStringForPrint(annotation),
method.getDeclaringClass().getSimpleName(),
method.getName());
}
});

}
})
.join(Joiner.on(", "))));
}

Iterable<String> getterClassNames = FluentIterable.from(getters)
.transform(MethodToDeclaringClassFunction.INSTANCE)
Expand All @@ -1141,13 +1153,19 @@ public Annotation apply(@Nonnull Method method) {
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.
*/
private static void validateSettersDoNotHaveAnnotation(
SortedSetMultimap<Method, Method> methodNameToAllMethodMap,
List<PropertyDescriptor> descriptors,
final AnnotationPredicates annotationPredicates) {
AnnotationPredicates annotationPredicates) {
List<AnnotatedSetter> annotatedSetters = new ArrayList<>();
for (PropertyDescriptor descriptor : descriptors) {
if (descriptor.getWriteMethod() == null
Expand Down Expand Up @@ -1434,7 +1452,7 @@ static class AnnotationPredicates {
new Predicate<Annotation>() {
@Override
public boolean apply(@Nonnull Annotation input) {
return input.annotationType().equals(JsonIgnore.class);
return JsonIgnore.class.equals(input.annotationType());
}
},
new Predicate<Method>() {
Expand Down
61 changes: 50 additions & 11 deletions sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import com.google.common.collect.ListMultimap;
import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.lang.annotation.Annotation;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -369,6 +370,16 @@ public interface GetterWithJsonIgnore extends PipelineOptions {
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.
Expand Down Expand Up @@ -501,7 +512,7 @@ private interface GetterWithConsistentDefault extends PipelineOptions {
* This class is inconsistent with {@link GetterWithDefault} that has a different
* {@link Default @Default}.
*/
private interface GetterWithInconsistentDefault extends PipelineOptions {
private interface GetterWithInconsistentDefaultType extends PipelineOptions {
@Default.String("abc")
Object getObject();
void setObject(Object value);
Expand Down Expand Up @@ -550,12 +561,12 @@ public void testGettersAnnotatedWithInconsistentDefault() throws Exception {

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)].");
"Property [object] is marked with contradictory annotations. Found ["
+ "[Default.Integer(value=1) on GetterWithDefault.getObject], "
+ "[Default.String(value=abc) on GetterWithInconsistentDefaultType.getObject]].");

// When we attempt to convert, we should error at this moment.
options.as(GetterWithInconsistentDefault.class);
options.as(GetterWithInconsistentDefaultType.class);
}

@Test
Expand All @@ -565,14 +576,29 @@ public void testGettersAnnotatedWithInconsistentDefaultValue() throws Exception

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)].");
"Property [object] is marked with contradictory annotations. Found ["
+ "[Default.Integer(value=1) on GetterWithDefault.getObject], "
+ "[Default.Integer(value=0) on 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 GetterWithInconsistentJsonIgnoreValue.getObject], "
+ "[JsonIgnore(value=true) on 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)
Expand All @@ -584,9 +610,9 @@ private interface GettersWithMultipleDefault extends PipelineOptions {
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)].");
"Property [object] is marked with contradictory annotations. Found ["
+ "[Default.String(value=abc) on GettersWithMultipleDefault.getObject], "
+ "[Default.Integer(value=0) on GettersWithMultipleDefault.getObject]].");

// When we attempt to create, we should error at this moment.
PipelineOptionsFactory.as(GettersWithMultipleDefault.class);
Expand Down Expand Up @@ -647,6 +673,19 @@ 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
Expand Down
60 changes: 49 additions & 11 deletions sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,18 +235,56 @@ public void testEqualsAndHashCode() throws Exception {
.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 testMultipleDefaultsThrow() throws Exception {
expectedException.expect(RuntimeException.class);
expectedException.expectMessage(
"Expected getter for property [string] to be marked with @Default on all "
+ "[org.apache.beam.sdk.options.ProxyInvocationHandlerTest$DefaultAnnotations, "
+ "org.apache.beam.sdk.options.ProxyInvocationHandlerTest$Simple], found only on "
+ "[org.apache.beam.sdk.options.ProxyInvocationHandlerTest$DefaultAnnotations]");

new ProxyInvocationHandler(Maps.<String, Object>newHashMap())
.as(Simple.class)
.as(DefaultAnnotations.class);
public void testToString() throws Exception {
ProxyInvocationHandler handler = new ProxyInvocationHandler(Maps.<String, Object>newHashMap());
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",
proxy.toString());
}

@Test
public void testToStringAfterDeserializationContainsJsonEntries() throws Exception {
ProxyInvocationHandler handler = new ProxyInvocationHandler(Maps.<String, Object>newHashMap());
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),
serializeDeserialize(PipelineOptions.class, proxy2).toString());
}

@Test
public void testToStringAfterDeserializationContainsOverriddenEntries() throws Exception {
ProxyInvocationHandler handler = new ProxyInvocationHandler(Maps.<String, Object>newHashMap());
StringWithDefault proxy = handler.as(StringWithDefault.class);
Long optionsId = proxy.getOptionsId();
proxy.setString("stringValue");
DefaultAnnotations proxy2 = proxy.as(DefaultAnnotations.class);
proxy2.setLong(57L);
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),
deserializedOptions.toString());
}

/** A test interface containing an unknown method. */
Expand Down

0 comments on commit f886096

Please sign in to comment.