Fix setter selection in presence of multiple setters (#2386)

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,25 +109,24 @@ 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()));
     }
 
     /**
      * Locates the property with the given name as a writable property. Searches only public properties.
      *
      * 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;

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            | _

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<String> 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) {}
 }

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'
+    }
 }

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<String, PropertyMutator> mutators = Maps.newHashMap();
+        private final Map<Pair<String, ? extends Class<?>>, PropertyMutator> mutators = Maps.newHashMap();
         private Class<? extends Project> 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<String, ? extends Class<?>> 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;

subprojects/language-jvm/src/main/java/org/gradle/api/tasks/compile/AbstractOptions.java

@@ -38,7 +38,7 @@ public void define(@Nullable Map<String, Object> args) {
             return;
         }
         for (Map.Entry<String, Object> 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<String, Object> map, Field field) {
         Object value = JavaReflectionUtil.readableProperty(this, Object.class, field.getName()).getValue(this);
         if (value != null) {