From c0aab386a55a69d30c5a282394ae766e74f8b698 Mon Sep 17 00:00:00 2001 From: Thomas Groh Date: Mon, 7 Mar 2016 13:47:30 -0800 Subject: [PATCH 1/2] Filter Synthetic Methods in PipelineOptionsFactory --- .../sdk/options/PipelineOptionsFactory.java | 41 ++++++--- .../PipelineOptionsFactoryJava8Test.java | 89 +++++++++++++++++++ 2 files changed, 119 insertions(+), 11 deletions(-) create mode 100644 sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java diff --git a/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactory.java b/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactory.java index e77b89f9a4ece..e9e94af19cbae 100644 --- a/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactory.java +++ b/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactory.java @@ -443,13 +443,21 @@ Class getProxyClass() { private static final Map>> SUPPORTED_PIPELINE_RUNNERS; /** Classes that are used as the boundary in the stack trace to find the callers class name. */ - private static final Set PIPELINE_OPTIONS_FACTORY_CLASSES = ImmutableSet.of( - PipelineOptionsFactory.class.getName(), - Builder.class.getName()); + private static final Set 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 IGNORED_METHODS; + /** A predicate that checks if a method is synthetic via {@link Method#isSynthetic()}. */ + private static final Predicate NOT_SYNTHETIC_PREDICATE = + new Predicate() { + @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> REGISTERED_OPTIONS = Sets.newConcurrentHashSet(); @@ -662,7 +670,9 @@ public static void printHelp(PrintStream out, Class i Preconditions.checkNotNull(iface); validateWellFormed(iface, REGISTERED_OPTIONS); - Iterable methods = ReflectHelpers.getClosureOfMethodsOnInterface(iface); + Iterable methods = + Iterables.filter( + ReflectHelpers.getClosureOfMethodsOnInterface(iface), NOT_SYNTHETIC_PREDICATE); ListMultimap, Method> ifaceToMethods = ArrayListMultimap.create(); for (Method method : methods) { // Process only methods that are not marked as hidden. @@ -876,7 +886,8 @@ private static List getPropertyDescriptors(Class beanClas throws IntrospectionException { // The sorting is important to make this method stable. SortedSet methods = Sets.newTreeSet(MethodComparator.INSTANCE); - methods.addAll(Arrays.asList(beanClass.getMethods())); + methods.addAll( + Collections2.filter(Arrays.asList(beanClass.getMethods()), NOT_SYNTHETIC_PREDICATE)); SortedMap propertyNamesToGetters = getPropertyNamesToGetters(methods); List descriptors = Lists.newArrayList(); @@ -1017,8 +1028,9 @@ private static List validateClass(Class klass) throws IntrospectionException { Set 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); } } @@ -1035,6 +1047,7 @@ private static List validateClass(Class interfaceMethods = FluentIterable .from(ReflectHelpers.getClosureOfMethodsOnInterface(iface)) + .filter(NOT_SYNTHETIC_PREDICATE) .toSortedSet(MethodComparator.INSTANCE); SortedSetMultimap methodNameToMethodMap = TreeMultimap.create(MethodNameComparator.INSTANCE, MethodComparator.INSTANCE); @@ -1059,10 +1072,13 @@ private static List validateClass(Class allInterfaceMethods = FluentIterable - .from(ReflectHelpers.getClosureOfMethodsOnInterfaces(validatedPipelineOptionsInterfaces)) - .append(ReflectHelpers.getClosureOfMethodsOnInterface(iface)) - .toSortedSet(MethodComparator.INSTANCE); + Iterable allInterfaceMethods = + FluentIterable.from( + ReflectHelpers.getClosureOfMethodsOnInterfaces( + validatedPipelineOptionsInterfaces)) + .append(ReflectHelpers.getClosureOfMethodsOnInterface(iface)) + .filter(NOT_SYNTHETIC_PREDICATE) + .toSortedSet(MethodComparator.INSTANCE); SortedSetMultimap methodNameToAllMethodMap = TreeMultimap.create(MethodNameComparator.INSTANCE, MethodComparator.INSTANCE); for (Method method : allInterfaceMethods) { @@ -1146,7 +1162,10 @@ private static List validateClass(Class 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), diff --git a/sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java b/sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java new file mode 100644 index 0000000000000..6c6553158ce84 --- /dev/null +++ b/sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java @@ -0,0 +1,89 @@ +/* + * 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 OptsWithDefault extends PipelineOptions { + default Number getValue() { + return 1024; + } + + void setValue(Number value); + } + + @Test + public void testDefaultMethodIgnoresDefaultImplementation() { + OptsWithDefault optsWithDefault = PipelineOptionsFactory.as(OptsWithDefault.class); + assertThat(optsWithDefault.getValue(), nullValue()); + + optsWithDefault.setValue(12.25); + assertThat(optsWithDefault.getValue(), equalTo(Double.valueOf(12.25))); + } + + private static interface ExtendedOptsWithDefault extends OptsWithDefault {} + + @Test + public void testDefaultMethodInExtendedClassIgnoresDefaultImplementation() { + OptsWithDefault extendedOptsWithDefault = + PipelineOptionsFactory.as(ExtendedOptsWithDefault.class); + assertThat(extendedOptsWithDefault.getValue(), nullValue()); + + extendedOptsWithDefault.setValue(Double.NEGATIVE_INFINITY); + assertThat(extendedOptsWithDefault.getValue(), equalTo(Double.NEGATIVE_INFINITY)); + } + + private static interface Opts extends PipelineOptions { + Number getValue(); + + void setValue(Number value); + } + + private static interface SubtypeReturingOpts extends Opts { + @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$" + + "SubtypeReturingOpts.getValue(), public abstract java.lang.Number " + + "com.google.cloud.dataflow.sdk.options.PipelineOptionsFactoryJava8Test$Opts" + + ".getValue()] with different return types for [" + + "com.google.cloud.dataflow.sdk.options.PipelineOptionsFactoryJava8Test$" + + "SubtypeReturingOpts]."); + PipelineOptionsFactory.as(SubtypeReturingOpts.class); + } +} From 343a3f8cf2c7e75ffc23237e6eb438e0aaa08367 Mon Sep 17 00:00:00 2001 From: Thomas Groh Date: Fri, 11 Mar 2016 16:51:38 -0800 Subject: [PATCH 2/2] fixup! Filter Synthetic Methods in PipelineOptionsFactory Opts -> Options for all class declarations Add a note that the value of a default method is ignored. --- .../dataflow/sdk/options/PipelineOptions.java | 3 +- .../PipelineOptionsFactoryJava8Test.java | 29 ++++++++++--------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptions.java b/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptions.java index 923033d5dadbf..8ff1fa9783a28 100644 --- a/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptions.java +++ b/sdk/src/main/java/com/google/cloud/dataflow/sdk/options/PipelineOptions.java @@ -137,7 +137,8 @@ * *

{@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. * *

{@link Hidden @Hidden} hides an option from being listed when {@code --help} * is invoked via {@link PipelineOptionsFactory#fromArgs(String[])}. diff --git a/sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java b/sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java index 6c6553158ce84..b7e1467436ed5 100644 --- a/sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java +++ b/sdk/src/test/java8/com/google/cloud/dataflow/sdk/options/PipelineOptionsFactoryJava8Test.java @@ -32,7 +32,7 @@ public class PipelineOptionsFactoryJava8Test { @Rule public ExpectedException thrown = ExpectedException.none(); - private static interface OptsWithDefault extends PipelineOptions { + private static interface OptionsWithDefaultMethod extends PipelineOptions { default Number getValue() { return 1024; } @@ -42,32 +42,33 @@ default Number getValue() { @Test public void testDefaultMethodIgnoresDefaultImplementation() { - OptsWithDefault optsWithDefault = PipelineOptionsFactory.as(OptsWithDefault.class); + 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 ExtendedOptsWithDefault extends OptsWithDefault {} + private static interface ExtendedOptionsWithDefault extends OptionsWithDefaultMethod {} @Test public void testDefaultMethodInExtendedClassIgnoresDefaultImplementation() { - OptsWithDefault extendedOptsWithDefault = - PipelineOptionsFactory.as(ExtendedOptsWithDefault.class); + 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 Opts extends PipelineOptions { + private static interface Options extends PipelineOptions { Number getValue(); void setValue(Number value); } - private static interface SubtypeReturingOpts extends Opts { + private static interface SubtypeReturingOptions extends Options { @Override Integer getValue(); void setValue(Integer value); @@ -78,12 +79,12 @@ 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$" - + "SubtypeReturingOpts.getValue(), public abstract java.lang.Number " - + "com.google.cloud.dataflow.sdk.options.PipelineOptionsFactoryJava8Test$Opts" - + ".getValue()] with different return types for [" - + "com.google.cloud.dataflow.sdk.options.PipelineOptionsFactoryJava8Test$" - + "SubtypeReturingOpts]."); - PipelineOptionsFactory.as(SubtypeReturingOpts.class); + + "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); } }