Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: JSON deserialization setter case priority #1831

Merged
merged 2 commits into from Mar 14, 2023
Merged

fix: JSON deserialization setter case priority #1831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70aacc3
fix: JSON deserialization setter case priority
meltsufin Mar 11, 2023
e4f17f2
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Mar 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 19 additions & 13 deletions google-http-client/src/main/java/com/google/api/client/util/FieldInfo.java
View file
Open in desktop
Expand Up @@ -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));
Copy link

@blakeli0 blakeli0 Mar 13, 2023 •
edited

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 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().

Copy link
Member Author

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 be setUtcTime(). So, I think this logic would work. The default setter would take precedence over setUTCTime().

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))) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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]);
Expand Down Expand Up @@ -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
}
}
}
Expand Down
33 changes: 33 additions & 0 deletions google-http-client/src/test/java/com/google/api/client/util/FieldInfoTest.java
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@

package com.google.api.client.util;

import com.google.api.client.json.GenericJson;
import junit.framework.TestCase;

/**
Expand Down Expand Up @@ -49,4 +50,36 @@ public void testEnumValue() {
assertEquals(E.OTHER_VALUE, FieldInfo.of(E.OTHER_VALUE).<E>enumValue());
assertEquals(E.NULL, FieldInfo.of(E.NULL).<E>enumValue());
}

public static final class Data extends GenericJson {
@Key String passcode;
@Key String passCode;

public Data setPasscode(String passcode) {
this.passcode = passcode;
return this;
}

public Data setPassCode(String passCode) {
this.passCode = passCode;
return this;
}
}

public void testSetValueCaseSensitivityPriority() {
Data data = new Data();
data.setPasscode("pass1");
data.setPassCode("pass2");
data.set("passCode", "passX");

assertEquals(data.passcode, "pass1");
assertEquals(data.passCode, "passX");

data.setPasscode("pass1");
data.setPassCode("pass2");
data.set("passcode", "passX");

assertEquals(data.passcode, "passX");
assertEquals(data.passCode, "pass2");
}
}