Skip to content

Commit

Permalink
This closes #41
Browse files Browse the repository at this point in the history
  • Loading branch information
lukecwik committed Mar 19, 2016
2 parents a461e00 + 91de072 commit f7aaee2
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@
*
* <p>{@link Default @Default} represents a set of annotations that can be used to annotate getter
* properties on {@link PipelineOptions} with information representing the default value to be
* returned if no value is specified.
* returned if no value is specified. Any default implementation (using the {@code default} keyword)
* is ignored.
*
* <p>{@link Hidden @Hidden} hides an option from being listed when {@code --help}
* is invoked via {@link PipelineOptionsFactory#fromArgs(String[])}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,13 +445,21 @@ Class<T> getProxyClass() {
private static final Map<String, Class<? extends PipelineRunner<?>>> SUPPORTED_PIPELINE_RUNNERS;

/** Classes that are used as the boundary in the stack trace to find the callers class name. */
private static final Set<String> PIPELINE_OPTIONS_FACTORY_CLASSES = ImmutableSet.of(
PipelineOptionsFactory.class.getName(),
Builder.class.getName());
private static final Set<String> PIPELINE_OPTIONS_FACTORY_CLASSES =
ImmutableSet.of(PipelineOptionsFactory.class.getName(), Builder.class.getName());

/** Methods that are ignored when validating the proxy class. */
private static final Set<Method> IGNORED_METHODS;

/** A predicate that checks if a method is synthetic via {@link Method#isSynthetic()}. */
private static final Predicate<Method> NOT_SYNTHETIC_PREDICATE =
new Predicate<Method>() {
@Override
public boolean apply(Method input) {
return !input.isSynthetic();
}
};

/** The set of options that have been registered and visible to the user. */
private static final Set<Class<? extends PipelineOptions>> REGISTERED_OPTIONS =
Sets.newConcurrentHashSet();
Expand Down Expand Up @@ -664,7 +672,9 @@ public static void printHelp(PrintStream out, Class<? extends PipelineOptions> i
Preconditions.checkNotNull(iface);
validateWellFormed(iface, REGISTERED_OPTIONS);

Iterable<Method> methods = ReflectHelpers.getClosureOfMethodsOnInterface(iface);
Iterable<Method> methods =
Iterables.filter(
ReflectHelpers.getClosureOfMethodsOnInterface(iface), NOT_SYNTHETIC_PREDICATE);
ListMultimap<Class<?>, Method> ifaceToMethods = ArrayListMultimap.create();
for (Method method : methods) {
// Process only methods that are not marked as hidden.
Expand Down Expand Up @@ -878,7 +888,8 @@ private static List<PropertyDescriptor> getPropertyDescriptors(Class<?> beanClas
throws IntrospectionException {
// The sorting is important to make this method stable.
SortedSet<Method> methods = Sets.newTreeSet(MethodComparator.INSTANCE);
methods.addAll(Arrays.asList(beanClass.getMethods()));
methods.addAll(
Collections2.filter(Arrays.asList(beanClass.getMethods()), NOT_SYNTHETIC_PREDICATE));
SortedMap<String, Method> propertyNamesToGetters = getPropertyNamesToGetters(methods);
List<PropertyDescriptor> descriptors = Lists.newArrayList();

Expand Down Expand Up @@ -1019,8 +1030,9 @@ private static List<PropertyDescriptor> validateClass(Class<? extends PipelineOp
Class<?> klass) throws IntrospectionException {
Set<Method> methods = Sets.newHashSet(IGNORED_METHODS);
// Ignore static methods, "equals", "hashCode", "toString" and "as" on the generated class.
// Ignore synthetic methods
for (Method method : klass.getMethods()) {
if (Modifier.isStatic(method.getModifiers())) {
if (Modifier.isStatic(method.getModifiers()) || method.isSynthetic()) {
methods.add(method);
}
}
Expand All @@ -1037,6 +1049,7 @@ private static List<PropertyDescriptor> validateClass(Class<? extends PipelineOp
// Verify that there are no methods with the same name with two different return types.
Iterable<Method> interfaceMethods = FluentIterable
.from(ReflectHelpers.getClosureOfMethodsOnInterface(iface))
.filter(NOT_SYNTHETIC_PREDICATE)
.toSortedSet(MethodComparator.INSTANCE);
SortedSetMultimap<Method, Method> methodNameToMethodMap =
TreeMultimap.create(MethodNameComparator.INSTANCE, MethodComparator.INSTANCE);
Expand All @@ -1061,10 +1074,13 @@ private static List<PropertyDescriptor> validateClass(Class<? extends PipelineOp

// Verify that there is no getter with a mixed @JsonIgnore annotation and verify
// that no setter has @JsonIgnore.
Iterable<Method> allInterfaceMethods = FluentIterable
.from(ReflectHelpers.getClosureOfMethodsOnInterfaces(validatedPipelineOptionsInterfaces))
.append(ReflectHelpers.getClosureOfMethodsOnInterface(iface))
.toSortedSet(MethodComparator.INSTANCE);
Iterable<Method> allInterfaceMethods =
FluentIterable.from(
ReflectHelpers.getClosureOfMethodsOnInterfaces(
validatedPipelineOptionsInterfaces))
.append(ReflectHelpers.getClosureOfMethodsOnInterface(iface))
.filter(NOT_SYNTHETIC_PREDICATE)
.toSortedSet(MethodComparator.INSTANCE);
SortedSetMultimap<Method, Method> methodNameToAllMethodMap =
TreeMultimap.create(MethodNameComparator.INSTANCE, MethodComparator.INSTANCE);
for (Method method : allInterfaceMethods) {
Expand Down Expand Up @@ -1148,7 +1164,10 @@ private static List<PropertyDescriptor> validateClass(Class<? extends PipelineOp

// Verify that no additional methods are on an interface that aren't a bean property.
SortedSet<Method> unknownMethods = new TreeSet<>(MethodComparator.INSTANCE);
unknownMethods.addAll(Sets.difference(Sets.newHashSet(klass.getMethods()), methods));
unknownMethods.addAll(
Sets.filter(
Sets.difference(Sets.newHashSet(klass.getMethods()), methods),
NOT_SYNTHETIC_PREDICATE));
Preconditions.checkArgument(unknownMethods.isEmpty(),
"Methods %s on [%s] do not conform to being bean properties.",
FluentIterable.from(unknownMethods).transform(ReflectHelpers.METHOD_FORMATTER),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Copyright (C) 2016 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package com.google.cloud.dataflow.sdk.options;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertThat;

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/**
* Java 8 tests for {@link PipelineOptionsFactory}.
*/
@RunWith(JUnit4.class)
public class PipelineOptionsFactoryJava8Test {
@Rule public ExpectedException thrown = ExpectedException.none();

private static interface OptionsWithDefaultMethod extends PipelineOptions {
default Number getValue() {
return 1024;
}

void setValue(Number value);
}

@Test
public void testDefaultMethodIgnoresDefaultImplementation() {
OptionsWithDefaultMethod optsWithDefault =
PipelineOptionsFactory.as(OptionsWithDefaultMethod.class);
assertThat(optsWithDefault.getValue(), nullValue());

optsWithDefault.setValue(12.25);
assertThat(optsWithDefault.getValue(), equalTo(Double.valueOf(12.25)));
}

private static interface ExtendedOptionsWithDefault extends OptionsWithDefaultMethod {}

@Test
public void testDefaultMethodInExtendedClassIgnoresDefaultImplementation() {
OptionsWithDefaultMethod extendedOptsWithDefault =
PipelineOptionsFactory.as(ExtendedOptionsWithDefault.class);
assertThat(extendedOptsWithDefault.getValue(), nullValue());

extendedOptsWithDefault.setValue(Double.NEGATIVE_INFINITY);
assertThat(extendedOptsWithDefault.getValue(), equalTo(Double.NEGATIVE_INFINITY));
}

private static interface Options extends PipelineOptions {
Number getValue();

void setValue(Number value);
}

private static interface SubtypeReturingOptions extends Options {
@Override
Integer getValue();
void setValue(Integer value);
}

@Test
public void testReturnTypeConflictThrows() throws Exception {
thrown.expect(IllegalArgumentException.class);
thrown.expectMessage(
"Method [getValue] has multiple definitions [public abstract java.lang.Integer "
+ "com.google.cloud.dataflow.sdk.options.PipelineOptionsFactoryJava8Test$"
+ "SubtypeReturingOptions.getValue(), public abstract java.lang.Number "
+ "com.google.cloud.dataflow.sdk.options.PipelineOptionsFactoryJava8Test$Options"
+ ".getValue()] with different return types for ["
+ "com.google.cloud.dataflow.sdk.options.PipelineOptionsFactoryJava8Test$"
+ "SubtypeReturingOptions].");
PipelineOptionsFactory.as(SubtypeReturingOptions.class);
}
}

0 comments on commit f7aaee2

Please sign in to comment.