Fix setter selection in presence of multiple setters #2386
Conversation
|
I'm still wondering if this can be applied to user types where there could be multiple setters for a given property, none taking
|
|
Could you elaborate why you think that this is still a problem? The implementation now uses |
|
I missed that. Awesome! |
| property.type == Iterable.class | ||
| } | ||
|
|
||
| def "can handle null as property type"() { |
There was a problem hiding this comment.
❌ null behaviour feels underspecified here.
The test should make clear which setter gets called, specially when there's more than a possible null receiver (like with the multiValue property, for instance).
Another question that comes to mind: What's the effect of passing null to a property of a primitive type?
There was a problem hiding this comment.
Unfortunately, it is not deterministic which setter gets called. All are equally correct. Depends again of the order the methods are returned by Class.getMethods() - see implementation of getMatchingAccessibleMethod() in commons lang. I think we need to accept this.
I added a test for the primitive type case. (It will not find a suitable setter as null cannot be assigned.)
There was a problem hiding this comment.
Actually, getMatchingAccessibleMethod() does its best to do the right thing here.
It won't return the first match but the best match according to MemberUtils.compareParameterTypes().
Well, in some cases were the "distance between types" is equal for several setters then the order of Class.getMethods() will prevail, but it should not be that bad.
| if (!method.getName().equals(setterName) || PropertyAccessorType.of(method) != PropertyAccessorType.SETTER) { | ||
| continue; | ||
| } | ||
| Method method = MethodUtils.getMatchingAccessibleMethod(target, setterName, new Class<?>[]{valueType}); |
There was a problem hiding this comment.
Nice 👍
Array formatting looks off.
There was a problem hiding this comment.
Checkstyle wanted it that way... :)
| @@ -38,7 +38,8 @@ 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()); | |||
| Object value = arg.getValue(); | |||
| JavaReflectionUtil.writeableProperty(getClass(), arg.getKey(), value == null ? null : value.getClass()).setValue(this, value); | |||
There was a problem hiding this comment.
I'd extract a method here:
setProperty(this, arg.getKey(), arg.getValue())
@eskatos good idea. I added a test. |
There was a problem hiding this comment.
👍
⭕️ A test asserting we use the closest matching setter and not the most generic one would also be nice, see #2386 (comment)
|
|
||
| then: | ||
| def e = thrown(NoSuchPropertyException) | ||
| e.message == "Could not find setter method for property 'myBooleanProperty' on class JavaTestSubject." |
There was a problem hiding this comment.
It might be confusing to read that message when you're sure there's a setter method just not one that accepts null.
How about mentioning the value was null? Something like:
Could not find setter method for property 'myBooleanProperty' accepting null value on class JavaTestSubject."
There was a problem hiding this comment.
Good suggestion. I changed it to your proposal.
I missed that one. I added another test now. |
|
I was more thinking about some thing like: interface JavaSubjectType {
CharSequence getMyProp();
void setMyProp(CharSequence value);
void setMyProp(Collection<?> value);
void setMyProp(Object value);
}And asserting that: subject.setMyProp(Arrays.asList("foo", "bar"));
// goes to the `Collection` setter, not the `Object` one
subject.setMyProp("bazar");
// goes to the `CharSequence` setter, not the `Object` one |
|
@eskatos I added one more test :) |
No description provided.