From 38e5dc0f772daecca1d2681885d3d85414eb6826 Mon Sep 17 00:00:00 2001 From: Jendrik Johannes Date: Thu, 6 Jul 2017 16:48:41 +0200 Subject: [PATCH] Fix setter selection in presence of multiple setters (#2386) --- .../internal/reflect/JavaReflectionUtil.java | 17 ++-- .../reflect/JavaReflectionUtilTest.groovy | 93 +++++++++++++++++-- .../internal/reflect/JavaTestSubject.java | 29 ++++++ .../PropertiesLoaderIntegrationTest.groovy | 9 ++ .../ProjectPropertySettingBuildLoader.java | 13 ++- .../api/tasks/compile/AbstractOptions.java | 6 +- 6 files changed, 144 insertions(+), 23 deletions(-) diff --git a/subprojects/base-services/src/main/java/org/gradle/internal/reflect/JavaReflectionUtil.java b/subprojects/base-services/src/main/java/org/gradle/internal/reflect/JavaReflectionUtil.java index 2b4c95effbfe..d9e16f2e4797 100644 --- a/subprojects/base-services/src/main/java/org/gradle/internal/reflect/JavaReflectionUtil.java +++ b/subprojects/base-services/src/main/java/org/gradle/internal/reflect/JavaReflectionUtil.java @@ -16,7 +16,9 @@ package org.gradle.internal.reflect; +import org.apache.commons.lang.reflect.MethodUtils; import org.gradle.api.JavaVersion; +import org.gradle.api.Nullable; import org.gradle.api.specs.Spec; import org.gradle.internal.Cast; import org.gradle.internal.Factory; @@ -107,12 +109,13 @@ private static Method findGetterMethod(Class target, String property) { * * @throws NoSuchPropertyException when the given property does not exist. */ - public static PropertyMutator writeableProperty(Class target, String property) throws NoSuchPropertyException { - PropertyMutator mutator = writeablePropertyIfExists(target, property); + public static PropertyMutator writeableProperty(Class target, String property, @Nullable Class valueType) throws NoSuchPropertyException { + PropertyMutator mutator = writeablePropertyIfExists(target, property, valueType); if (mutator != null) { return mutator; } - throw new NoSuchPropertyException(String.format("Could not find setter method for property '%s' on class %s.", property, target.getSimpleName())); + throw new NoSuchPropertyException(String.format("Could not find setter method for property '%s' %s on class %s.", + property, valueType == null ? "accepting null value" : "of type " + valueType.getSimpleName(), target.getSimpleName())); } /** @@ -120,12 +123,10 @@ public static PropertyMutator writeableProperty(Class target, String property * * Returns null if no such property exists. */ - public static PropertyMutator writeablePropertyIfExists(Class target, String property) throws NoSuchPropertyException { + public static PropertyMutator writeablePropertyIfExists(Class target, String property, @Nullable Class valueType) throws NoSuchPropertyException { String setterName = toMethodName("set", property); - for (final Method method : target.getMethods()) { - if (!method.getName().equals(setterName) || PropertyAccessorType.of(method) != PropertyAccessorType.SETTER) { - continue; - } + Method method = MethodUtils.getMatchingAccessibleMethod(target, setterName, new Class[]{valueType}); + if (method != null) { return new MethodBackedPropertyMutator(property, method); } return null; diff --git a/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaReflectionUtilTest.groovy b/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaReflectionUtilTest.groovy index f5ae8f798d55..32e77856167d 100644 --- a/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaReflectionUtilTest.groovy +++ b/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaReflectionUtilTest.groovy @@ -31,10 +31,10 @@ class JavaReflectionUtilTest extends Specification { def "property names"() { expect: - propertyNames(new JavaTestSubject()) == ['myBooleanProperty', 'myOtherBooleanProperty', 'myProperty', 'protectedProperty', 'writeOnly'] as Set + propertyNames(new JavaTestSubject()) == ['myBooleanProperty', 'myOtherBooleanProperty', 'myProperty', 'myProperty2', 'myProperty3', 'protectedProperty', 'writeOnly', 'multiValue'] as Set and: - propertyNames(new JavaTestSubjectSubclass()) == ['myBooleanProperty', 'myOtherBooleanProperty', 'myProperty', 'protectedProperty', 'writeOnly', 'subclassBoolean'] as Set + propertyNames(new JavaTestSubjectSubclass()) == ['myBooleanProperty', 'myOtherBooleanProperty', 'myProperty', 'myProperty2', 'myProperty3', 'protectedProperty', 'writeOnly', 'multiValue', 'subclassBoolean'] as Set and: propertyNames(new WithProperties()) == ['metaClass', 'prop1', 'prop2', 'something', 'somethingElse', 'writeOnly'] as Set @@ -62,12 +62,88 @@ class JavaReflectionUtilTest extends Specification { def "write property"() { when: - writeableProperty(JavaTestSubject, "myProperty").setValue(myProperties, "otherValue") + writeableProperty(JavaTestSubject, "myProperty", String.class).setValue(myProperties, "otherValue") then: readableProperty(JavaTestSubject, String, "myProperty").getValue(myProperties) == "otherValue" } + def "write property with multiple setters"() { + when: + writeableProperty(JavaTestSubject, "myProperty2", String.class).setValue(myProperties, "stringValue") + + then: + readableProperty(JavaTestSubject, String, "myProperty2").getValue(myProperties) == "stringValue" + + when: + writeableProperty(JavaTestSubject, "myProperty2", File.class).setValue(myProperties, new File("fileValue")) + + then: + readableProperty(JavaTestSubject, String, "myProperty2").getValue(myProperties) == "fileValue" + } + + def "picks the generic object setter if the typed setter does not match the value type"() { + when: + def property = writeableProperty(JavaTestSubject, "myProperty", File.class) + + then: + property.type == Object.class + } + + def "picks the typed setter if it is the better match"() { + when: + def property = writeableProperty(JavaTestSubject, "myProperty", String.class) + + then: + property.type == String.class + } + + def "picks the best matching typed setter"() { + when: + def property = writeableProperty(JavaTestSubject, "myProperty3", Arrays.asList("foo", "bar").class) + + then: + property.type == Collection.class + + when: + property = writeableProperty(JavaTestSubject, "myProperty3", "bar".class) + + then: + property.type == CharSequence.class + + when: + property = writeableProperty(JavaTestSubject, "myProperty3", int.class) + + then: + property.type == Object.class + } + + def "picks the generic iterable setter if the typed setter does not match the value type"() { + when: + def property = writeableProperty(JavaTestSubject, "multiValue", List.class) + + then: + property.type == Iterable.class + } + + def "can handle null as property type"() { + when: + writeableProperty(JavaTestSubject, "myProperty", null) + + then: + //we do not know which 'myProperty' setter is picked, as both fit equally well + noExceptionThrown() + } + + def "cannot write primitive type properties if type is unknown"() { + when: + writeableProperty(JavaTestSubject, "myBooleanProperty", null) + + then: + def e = thrown(NoSuchPropertyException) + e.message == "Could not find setter method for property 'myBooleanProperty' accepting null value on class JavaTestSubject." + } + def "read boolean property"() { expect: readableProperty(JavaTestSubject, Boolean, "myBooleanProperty").getValue(myProperties) == true @@ -94,10 +170,9 @@ class JavaReflectionUtilTest extends Specification { thrown(NoSuchPropertyException); } - def "write boolean property"() { when: - writeableProperty(JavaTestSubject, "myBooleanProperty").setValue(myProperties, false) + writeableProperty(JavaTestSubject, "myBooleanProperty", Boolean.class).setValue(myProperties, false) then: readableProperty(JavaTestSubject, Boolean, "myBooleanProperty").getValue(myProperties) == false @@ -137,11 +212,11 @@ class JavaReflectionUtilTest extends Specification { def "cannot write property that doesn't have a well formed setter"() { when: - writeableProperty(JavaTestSubject, property) + writeableProperty(JavaTestSubject, property, Object.class) then: NoSuchPropertyException e = thrown() - e.message == "Could not find setter method for property '${property}' on class JavaTestSubject." + e.message == "Could not find setter method for property '${property}' of type Object on class JavaTestSubject." where: property | _ @@ -153,11 +228,11 @@ class JavaReflectionUtilTest extends Specification { def "cannot write property that is not public"() { when: - writeableProperty(JavaTestSubject, property) + writeableProperty(JavaTestSubject, property, Object.class) then: NoSuchPropertyException e = thrown() - e.message == "Could not find setter method for property '${property}' on class JavaTestSubject." + e.message == "Could not find setter method for property '${property}' of type Object on class JavaTestSubject." where: property | _ diff --git a/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaTestSubject.java b/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaTestSubject.java index d3f3bbe2c1da..1c5bf9a15a64 100644 --- a/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaTestSubject.java +++ b/subprojects/base-services/src/test/groovy/org/gradle/internal/reflect/JavaTestSubject.java @@ -16,11 +16,16 @@ package org.gradle.internal.reflect; +import java.io.File; +import java.util.Collection; +import java.util.Set; + @SuppressWarnings("UnusedDeclaration") public class JavaTestSubject { final IllegalStateException failure = new IllegalStateException(); private String myProp = "myValue"; + private String myProp2; private boolean myBooleanProp = true; public String myField = "myFieldValue"; public boolean myBooleanField = true; @@ -40,6 +45,10 @@ public void setMyProperty(String value) { myProp = value; } + public void setMyProperty(Object value) { + myProp = value.toString(); + } + public boolean isMyBooleanProperty() { return myBooleanProp; } @@ -115,4 +124,24 @@ private String getPrivateProperty() { private void setPrivateProperty(String value) { } + + public void setMultiValue(Set values) { } + + public void setMultiValue(Iterable values) { } + + public void setMyProperty2(String value) { + myProp2 = value; + } + + public void setMyProperty2(File value) { + myProp2 = value.toString(); + } + + public String getMyProperty2() { + return myProp2; + } + + public void setMyProperty3(CharSequence value) {} + public void setMyProperty3(Collection value) {} + public void setMyProperty3(Object value) {} } diff --git a/subprojects/core/src/integTest/groovy/org/gradle/initialization/PropertiesLoaderIntegrationTest.groovy b/subprojects/core/src/integTest/groovy/org/gradle/initialization/PropertiesLoaderIntegrationTest.groovy index 6a0eade49f89..bc11e41310ff 100644 --- a/subprojects/core/src/integTest/groovy/org/gradle/initialization/PropertiesLoaderIntegrationTest.groovy +++ b/subprojects/core/src/integTest/groovy/org/gradle/initialization/PropertiesLoaderIntegrationTest.groovy @@ -132,4 +132,13 @@ task printSystemProp { then: result.assertOutputContains('mySystemProp=commandline') } + + def "can always change buildDir in properties file"() { + when: + file('gradle.properties') << """ + buildDir=otherBuild + """ + then: + succeeds ':help' + } } diff --git a/subprojects/core/src/main/java/org/gradle/initialization/ProjectPropertySettingBuildLoader.java b/subprojects/core/src/main/java/org/gradle/initialization/ProjectPropertySettingBuildLoader.java index 9f30048ac8f1..30a98d420218 100644 --- a/subprojects/core/src/main/java/org/gradle/initialization/ProjectPropertySettingBuildLoader.java +++ b/subprojects/core/src/main/java/org/gradle/initialization/ProjectPropertySettingBuildLoader.java @@ -22,6 +22,7 @@ import org.gradle.api.internal.GradleInternal; import org.gradle.api.internal.initialization.ClassLoaderScope; import org.gradle.api.plugins.ExtraPropertiesExtension; +import org.gradle.internal.Pair; import org.gradle.internal.reflect.JavaReflectionUtil; import org.gradle.internal.reflect.PropertyMutator; import org.gradle.util.GUtil; @@ -82,7 +83,7 @@ private void addPropertiesToProject(Project project, CachingPropertyApplicator a * to avoid too many searches. */ private static class CachingPropertyApplicator { - private final Map mutators = Maps.newHashMap(); + private final Map>, PropertyMutator> mutators = Maps.newHashMap(); private Class projectClazz; void configureProperty(Project project, String name, Object value) { @@ -91,13 +92,15 @@ void configureProperty(Project project, String name, Object value) { mutators.clear(); projectClazz = clazz; } - PropertyMutator propertyMutator = mutators.get(name); + Class valueType = value == null ? null : value.getClass(); + Pair> key = Pair.of(name, valueType); + PropertyMutator propertyMutator = mutators.get(key); if (propertyMutator != null) { propertyMutator.setValue(project, value); } else { - if (!mutators.containsKey(name)) { - propertyMutator = JavaReflectionUtil.writeablePropertyIfExists(clazz, name); - mutators.put(name, propertyMutator); + if (!mutators.containsKey(key)) { + propertyMutator = JavaReflectionUtil.writeablePropertyIfExists(clazz, name, valueType); + mutators.put(key, propertyMutator); if (propertyMutator != null) { propertyMutator.setValue(project, value); return; diff --git a/subprojects/language-jvm/src/main/java/org/gradle/api/tasks/compile/AbstractOptions.java b/subprojects/language-jvm/src/main/java/org/gradle/api/tasks/compile/AbstractOptions.java index 95ebfc065e62..60a62bce2cfa 100644 --- a/subprojects/language-jvm/src/main/java/org/gradle/api/tasks/compile/AbstractOptions.java +++ b/subprojects/language-jvm/src/main/java/org/gradle/api/tasks/compile/AbstractOptions.java @@ -38,7 +38,7 @@ public void define(@Nullable Map args) { return; } for (Map.Entry arg: args.entrySet()) { - JavaReflectionUtil.writeableProperty(getClass(), arg.getKey()).setValue(this, arg.getValue()); + setProperty(arg.getKey(), arg.getValue()); } } @@ -73,6 +73,10 @@ protected Object getAntPropertyValue(String fieldName, Object value) { return value; } + private void setProperty(String property, Object value) { + JavaReflectionUtil.writeableProperty(getClass(), property, value == null ? null : value.getClass()).setValue(this, value); + } + private void addValueToMapIfNotNull(Map map, Field field) { Object value = JavaReflectionUtil.readableProperty(this, Object.class, field.getName()).getValue(this); if (value != null) {