Fix #5152: support "iPhone" style properties -- add MapperFeature.FIX_FIELD_NAME_CASE_MISMATCH
#5187
Conversation
cowtowncoder
changed the title
|
Initial implementation complete; need to consider naming still, at least. |
cowtowncoder
changed the title
MapperFeature.FIX_FIELD_NAME_CASE_MISMATCH
cowtowncoder
added
most-wanted
|
@JooHyukKim WDYT? Naming of |
| * <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. |
There was a problem hiding this comment.
enabled in 3.0 ? Let's add documentation label then
There was a problem hiding this comment.
Yes, need a follow-up issue for defaults changing after this gets merged.
| */ | ||
| ALLOW_IS_GETTERS_FOR_NON_BOOLEAN(false), | ||
| FIX_FIELD_NAME_CASE_MISMATCH(false), |
There was a problem hiding this comment.
My concern is... Would the name imply we would fix any case difference issue?
There was a problem hiding this comment.
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.
So bit tricky to explain.
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)
| @@ -132,7 +145,6 @@ public void testKBSBroadCastingStyleProperty() throws Exception { | |||
| assertEquals("{\"KBSBroadCasting\":\"Korean Broadcasting System\"}", serialized); | |||
| } | |||
|
|
|||
| @JacksonTestFailureExpected | |||
There was a problem hiding this comment.
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.
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:
- Check if and only if:
- Field name has first and/or second letter upper-case; and that name does not yet match any other accessor
- If checking, see if Field name matches, case excluded to ANY other accessor sets (that do not have Field matched)
- If check matches, combine Field and non-Field accessors, using Field name as-is (not mangled getter/setter name)
| switch (name.length()) { | ||
| case 0: | ||
| return false; | ||
| default: |
There was a problem hiding this comment.
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.
Could move 0 as last, making default first entry. Not sure if that's any better.
There was a problem hiding this comment.
Yeah not a blocker. We can go on
|
Even though this might be subjective , just one concern regarding the feature name. Having keywords like UPPER_CASE or PREFIX would narrow the scope. @cowtowncoder other than the feature name, implementation looks reasonable already. Sorry, late reply! |
Fixes #5152: adds a new
MapperFeature.