-
Notifications
You must be signed in to change notification settings - Fork 448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: JSON deserialization setter case priority #1831
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,12 +134,20 @@ public static FieldInfo of(Field field) { | |
} | ||
|
||
/** Creates list of setter methods for a field only in declaring class. */ | ||
private Method[] settersMethodForField(Field field) { | ||
private Method[] settersMethodForField(final Field field) { | ||
List<Method> methods = new ArrayList<>(); | ||
String fieldSetter = "set" + Ascii.toUpperCase(field.getName().substring(0, 1)); | ||
if (field.getName().length() > 1) { | ||
fieldSetter += field.getName().substring(1); | ||
} | ||
for (Method method : field.getDeclaringClass().getDeclaredMethods()) { | ||
if (Ascii.toLowerCase(method.getName()).equals("set" + Ascii.toLowerCase(field.getName())) | ||
&& method.getParameterTypes().length == 1) { | ||
methods.add(method); | ||
if (method.getParameterTypes().length == 1) { | ||
// add case-sensitive matches first in the list | ||
if (method.getName().equals(fieldSetter)) { | ||
methods.add(0, method); | ||
} else if (Ascii.toLowerCase(method.getName()).equals(Ascii.toLowerCase(fieldSetter))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish I understood the reason behind this lowercase comparison better. It feels like it was an arbitrary implementation choice while fixing an issue, but I'm not sure. I don't think we can change it after all this time, though. Original PR: #538 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @suztomo I would like you opinion on whether this would considered a breaking change. |
||
methods.add(method); | ||
} | ||
} | ||
} | ||
return methods.toArray(new Method[0]); | ||
|
@@ -216,15 +224,13 @@ public Object getValue(Object obj) { | |
* value. | ||
*/ | ||
public void setValue(Object obj, Object value) { | ||
if (setters.length > 0) { | ||
for (Method method : setters) { | ||
if (value == null || method.getParameterTypes()[0].isAssignableFrom(value.getClass())) { | ||
try { | ||
method.invoke(obj, value); | ||
return; | ||
} catch (IllegalAccessException | InvocationTargetException e) { | ||
// try to set field directly | ||
} | ||
for (Method method : setters) { | ||
if (value == null || method.getParameterTypes()[0].isAssignableFrom(value.getClass())) { | ||
try { | ||
method.invoke(obj, value); | ||
return; | ||
} catch (IllegalAccessException | InvocationTargetException e) { | ||
// try to set field directly | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach works for this issue, however, is it possible to have a scenario that the field name is
utcTime
, and the setter issetUTCTime
? We may have to look at how GenericJson is implemented to confirm this. So I think it would be safer to split themethod.getName()
byset
and compare the name withfield.getName()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default setter for
utcTime
would besetUtcTime()
. So, I think this logic would work. The default setter would take precedence oversetUTCTime()
.