-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix #5152: support "iPhone" style properties -- add MapperFeature.FIX_FIELD_NAME_CASE_MISMATCH
#5187
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
base: 2.x
Are you sure you want to change the base?
Fix #5152: support "iPhone" style properties -- add MapperFeature.FIX_FIELD_NAME_CASE_MISMATCH
#5187
Changes from all commits
4069d05
3cf35db
88178c9
d5dca13
815c74d
35d852c
551501c
aadbca8
0c24f62
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 |
---|---|---|
|
@@ -218,6 +218,17 @@ public enum MapperFeature implements ConfigFeature | |
*/ | ||
INFER_CREATOR_FROM_CONSTRUCTOR_PROPERTIES(true), | ||
|
||
/** | ||
* Feature that when enabled will allow getters with is-Prefix also for | ||
* non-boolean return types; if disabled only methods that return | ||
* {@code boolean} or {@code Boolean} qualify as "is getters". | ||
* <p> | ||
* Feature is disabled by default for backwards compatibility. | ||
* | ||
* @since 2.14 | ||
*/ | ||
ALLOW_IS_GETTERS_FOR_NON_BOOLEAN(false), | ||
|
||
/** | ||
* Feature that determines whether nominal property type of {@link Void} is | ||
* allowed for Getter methods to indicate {@code null} valued pseudo-property | ||
|
@@ -541,15 +552,20 @@ public enum MapperFeature implements ConfigFeature | |
ALLOW_EXPLICIT_PROPERTY_RENAMING(false), | ||
|
||
/** | ||
* Feature that when enabled will allow getters with is-Prefix also for | ||
* non-boolean return types; if disabled only methods that return | ||
* {@code boolean} or {@code Boolean} qualify as "is getters". | ||
* Feature that can be enabled to solve problem where an upper-case letter in | ||
* the first 2 characters of Java field name (like {@code "IPhone"} or {@code "iPhone"}) | ||
* prevents match with property name derived from accessors (setter like | ||
* {@code getIPhone()} becomes {@code "iphone"}). | ||
* If enabled, additional checking is done with case-insensitive comparison (for | ||
* cases of the first or second letter of Field name being upper-case) to merge | ||
* accessors. If disabled, no special processing is done. | ||
* <p> | ||
* Feature is disabled by default for backwards compatibility. | ||
* Feature is disabled by default in 2.x for backwards-compatibility. | ||
* It will be enabled by default in 3.0. | ||
* | ||
* @since 2.14 | ||
* @since 2.20 | ||
*/ | ||
ALLOW_IS_GETTERS_FOR_NON_BOOLEAN(false), | ||
FIX_FIELD_NAME_CASE_MISMATCH(false), | ||
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. My concern is... Would the name imply we would fix any case difference issue? 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. Not really. So maybe should still consider name to use... In a way it is not necessarily limited to first two, but it only checks cases where Field name has first and/or second character upper case. Since those are ones where name-mangling can produce differences. But comparison itself consider whole implicit names. Alternatively could change logic itself to actually consider all cases of "lone" (unmatched) Fields, either with at least one upper-case letter, or possibly just any (since Getter/Setter name could have upper-case even without Field) |
||
|
||
/* | ||
/****************************************************** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -433,20 +433,23 @@ protected void collectAll() | |
_potentialCreators = new PotentialCreators(); | ||
|
||
// First: gather basic accessors | ||
LinkedHashMap<String, POJOPropertyBuilder> props = new LinkedHashMap<String, POJOPropertyBuilder>(); | ||
LinkedHashMap<String, POJOPropertyBuilder> props = new LinkedHashMap<>(); | ||
|
||
// 14-Nov-2024, tatu: Previously skipped checking fields for Records; with 2.18+ won't | ||
// (see [databind#3628], [databind#3895], [databind#3992], [databind#4626]) | ||
_addFields(props); // note: populates _fieldRenameMappings | ||
|
||
_addMethods(props); | ||
// 25-Jan-2016, tatu: Avoid introspecting (constructor-)creators for non-static | ||
// inner classes, see [databind#1502] | ||
// 14-Nov-2024, tatu: Similarly need Creators for Records too (2.18+) | ||
if (!_classDef.isNonStaticInnerClass()) { | ||
_addCreators(props); | ||
} | ||
|
||
// 11-Jun-2025, tatu: [databind#5152] May need to "fix" mis-matching leading case | ||
// wrt Fields vs Accessors | ||
if (_config.isEnabled(MapperFeature.FIX_FIELD_NAME_CASE_MISMATCH)) { | ||
_fixLeadingFieldNameCase(props); | ||
} | ||
// Remove ignored properties, first; this MUST precede annotation merging | ||
// since logic relies on knowing exactly which accessor has which annotation | ||
_removeUnwantedProperties(props); | ||
|
@@ -547,10 +550,9 @@ private Map<String, POJOPropertyBuilder> _putAnyGettersInTheEnd( | |
protected void _addFields(Map<String, POJOPropertyBuilder> props) | ||
{ | ||
final AnnotationIntrospector ai = _annotationIntrospector; | ||
/* 28-Mar-2013, tatu: For deserialization we may also want to remove | ||
* final fields, as often they won't make very good mutators... | ||
* (although, maybe surprisingly, JVM _can_ force setting of such fields!) | ||
*/ | ||
// 28-Mar-2013, tatu: For deserialization we may also want to remove | ||
// final fields, as often they won't make very good mutators... | ||
// (although, maybe surprisingly, JVM _can_ force setting of such fields!) | ||
final boolean pruneFinalFields = !_forSerialization && !_config.isEnabled(MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS); | ||
final boolean transientAsIgnoral = _config.isEnabled(MapperFeature.PROPAGATE_TRANSIENT_MARKER); | ||
|
||
|
@@ -1318,6 +1320,97 @@ private String _checkRenameByField(String implName) { | |
return implName; | ||
} | ||
|
||
/* | ||
/********************************************************** | ||
/* Internal methods; merging/fixing case-differences | ||
/********************************************************** | ||
*/ | ||
|
||
// @since 2.20 | ||
protected void _fixLeadingFieldNameCase(Map<String, POJOPropertyBuilder> props) | ||
{ | ||
// 11-Jun-2025, tatu: [databind#5152] May need to "fix" mis-matching leading case | ||
// wrt Fields vs Accessors | ||
|
||
// First: find possible candidates where: | ||
// | ||
// 1. Property only has Field | ||
// 2. Field does NOT have explicit name (renaming) | ||
// 3. Implicit name has upper-case for first and/or second character | ||
|
||
Map<String, POJOPropertyBuilder> fieldsToCheck = null; | ||
for (Map.Entry<String, POJOPropertyBuilder> entry : props.entrySet()) { | ||
POJOPropertyBuilder prop = entry.getValue(); | ||
|
||
// First: (1) and (2) | ||
if (!prop.hasFieldAndNothingElse() | ||
|| prop.isExplicitlyNamed()) { | ||
continue; | ||
} | ||
// Second: (3) | ||
if (!_firstOrSecondCharUpperCase(entry.getKey())) { | ||
continue; | ||
} | ||
if (fieldsToCheck == null) { | ||
fieldsToCheck = new HashMap<>(); | ||
} | ||
fieldsToCheck.put(entry.getKey(), prop); | ||
} | ||
/*// DEBUGGING | ||
if (fieldsToCheck == null) { | ||
System.err.println("_fixLeadingCase, candidates -> null; props -> "+props.keySet()); | ||
} else { | ||
System.err.println("_fixLeadingCase, candidates -> "+fieldsToCheck); | ||
} | ||
*/ | ||
|
||
if (fieldsToCheck == null) { | ||
return; | ||
} | ||
|
||
for (Map.Entry<String, POJOPropertyBuilder> fieldEntry : fieldsToCheck.entrySet()) { | ||
Iterator<Map.Entry<String, POJOPropertyBuilder>> it = props.entrySet().iterator(); | ||
final POJOPropertyBuilder fieldProp = fieldEntry.getValue(); | ||
final String fieldName = fieldEntry.getKey(); | ||
|
||
while (it.hasNext()) { | ||
Map.Entry<String, POJOPropertyBuilder> propEntry = it.next(); | ||
final POJOPropertyBuilder prop = propEntry.getValue(); | ||
|
||
// Skip anything that has Field (can't merge) | ||
if (prop == fieldProp || prop.hasField()) { | ||
continue; | ||
} | ||
if (fieldName.equalsIgnoreCase(propEntry.getKey())) { | ||
// Remove non-Field property; add its accessors to Field one | ||
it.remove(); | ||
fieldProp.addAll(prop); | ||
// Should we continue with possible other accessors? | ||
// For now assume only one merge needed/desired | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
|
||
// @since 2.20 | ||
private boolean _firstOrSecondCharUpperCase(String name) { | ||
switch (name.length()) { | ||
case 0: | ||
return false; | ||
default: | ||
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. Is this neccessary to place default statement in the middle? Unless strongly required, Could switch to if else statement 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. Could move 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. Yeah not a blocker. We can go on |
||
if (!Character.isLowerCase(name.charAt(1))) { | ||
return true; | ||
} | ||
// fall through | ||
case 1: | ||
if (!Character.isLowerCase(name.charAt(0))) { | ||
return true; | ||
} | ||
return false; | ||
} | ||
} | ||
|
||
/* | ||
/********************************************************** | ||
/* Internal methods; removing ignored properties | ||
|
@@ -1420,6 +1513,7 @@ protected void _renameProperties(Map<String, POJOPropertyBuilder> props) | |
// With renaming need to do in phases: first, find properties to rename | ||
Iterator<Map.Entry<String,POJOPropertyBuilder>> it = props.entrySet().iterator(); | ||
LinkedList<POJOPropertyBuilder> renamed = null; | ||
|
||
while (it.hasNext()) { | ||
Map.Entry<String, POJOPropertyBuilder> entry = it.next(); | ||
POJOPropertyBuilder prop = entry.getValue(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,9 @@ | ||
package com.fasterxml.jackson.databind.tofix; | ||
package com.fasterxml.jackson.databind.misc; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import com.fasterxml.jackson.annotation.JsonPropertyOrder; | ||
import com.fasterxml.jackson.databind.*; | ||
import com.fasterxml.jackson.databind.testutil.DatabindTestUtil; | ||
import com.fasterxml.jackson.databind.testutil.failure.JacksonTestFailureExpected; | ||
|
||
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
|
@@ -73,10 +71,27 @@ public void setPhone(String value) { | |
} | ||
} | ||
|
||
// [databind#2696] | ||
static class OAuthTokenBean { | ||
protected String oAuthToken; | ||
|
||
public OAuthTokenBean(String t) { | ||
oAuthToken = t; | ||
} | ||
|
||
public String getOAuthToken() { | ||
return this.oAuthToken; | ||
} | ||
|
||
public void setOAuthToken(String oAuthToken) { | ||
this.oAuthToken = oAuthToken; | ||
} | ||
} | ||
|
||
private final ObjectMapper MAPPER = jsonMapperBuilder() | ||
.enable(MapperFeature.FIX_FIELD_NAME_CASE_MISMATCH) | ||
.build(); | ||
|
||
@JacksonTestFailureExpected | ||
@Test | ||
public void testIPhoneStyleProperty() throws Exception { | ||
// Test with iPhone style property | ||
|
@@ -104,21 +119,19 @@ public void testRegularPojoProperty() throws Exception { | |
} | ||
|
||
// [databind#2835]: "dLogHeader" property | ||
@JacksonTestFailureExpected | ||
@Test | ||
public void testDLogHeaderStyleProperty() throws Exception { | ||
// Test with DLogHeader style property | ||
String json = "{\"dLogHeader\":\"Debug Log Header\"}"; | ||
String json = "{\"DLogHeader\":\"Debug Log Header\"}"; | ||
DLogHeaderBean result = MAPPER.readValue(json, DLogHeaderBean.class); | ||
assertNotNull(result); | ||
assertEquals("Debug Log Header", result.getDLogHeader()); | ||
|
||
// Test serialization | ||
String serialized = MAPPER.writeValueAsString(result); | ||
assertEquals("{\"dLogHeader\":\"Debug Log Header\"}", serialized); | ||
assertEquals("{\"DLogHeader\":\"Debug Log Header\"}", serialized); | ||
} | ||
|
||
@JacksonTestFailureExpected | ||
@Test | ||
public void testKBSBroadCastingStyleProperty() throws Exception { | ||
// Test with KBSBroadCasting style property | ||
|
@@ -132,7 +145,6 @@ public void testKBSBroadCastingStyleProperty() throws Exception { | |
assertEquals("{\"KBSBroadCasting\":\"Korean Broadcasting System\"}", serialized); | ||
} | ||
|
||
@JacksonTestFailureExpected | ||
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. If we make this test case valid then we may want to change JavaDoc for our new feature because it says first or second character are only supported 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. Yeah it's... bit tricky to explain logic. Basically only cases where name-mangling should produce mismatches are if name starts with one or more upper-case (non-lower case) letters -- like "KBSBroadcasting" -- or starts with lower-case but is immediately followed by one or more upper-case letters -- like "iPhone". But in either case, it can be a sequence of more than one upper-case letters that need to be considered in case-insensitive manner. So logic is:
|
||
@Test | ||
public void testPhoneStyleProperty() throws Exception { | ||
// Test with Phone style property | ||
|
@@ -146,4 +158,10 @@ public void testPhoneStyleProperty() throws Exception { | |
assertEquals("{\"Phone\":\"iPhone 15\"}", serialized); | ||
} | ||
|
||
} | ||
// [databind#2696] | ||
@Test | ||
public void testOAuthProperty() throws Exception { | ||
assertEquals(a2q("{'oAuthToken':'123'}"), | ||
MAPPER.writeValueAsString(new OAuthTokenBean("123"))); | ||
} | ||
} |
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.
enabled in 3.0 ? Let's add documentation label then
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.
Yes, need a follow-up issue for defaults changing after this gets merged.