fix: JSON deserialization setter case priority #1831
Conversation
Fixes the problem of JSON deserialization ignoring the case of setters. Now case-sensitive setter method matches are considered first. Fixes: #1830.
| // 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.
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.
@suztomo I would like you opinion on whether this would considered a breaking change.
| List<Method> methods = new ArrayList<>(); | ||
| String fieldSetter = "set" + Ascii.toUpperCase(field.getName().substring(0, 1)); |
There was a problem hiding this comment.
This approach works for this issue, however, is it possible to have a scenario that the field name is utcTime, and the setter is setUTCTime? We may have to look at how GenericJson is implemented to confirm this. So I think it would be safer to split the method.getName() by set and compare the name with field.getName().
There was a problem hiding this comment.
The default setter for utcTime would be setUtcTime(). So, I think this logic would work. The default setter would take precedence over setUTCTime().
|
I didn’t know this case priority behavior. If not done yet, can you write down the desired behavior about the setter case priority in the Javadoc? Include how it helps the users of this HTTP client library. If the current implementation was not doing the job in the spec, then this PR is a bug fix. |
It's a bugfix. Internal ref: b/267807198. |
Fixes the problem of JSON deserialization ignoring the case of setters. Now case-sensitive setter method matches are considered first.
Fixes: #1830.