Fix Jackson 3 regression for @JsonIdentityInfo #5238
#5299
Conversation
| if (getObjectIdInfo() != null) { | ||
| renamed.add(prop); | ||
| } else { | ||
| renamed.add(prop.withName(n)); | ||
| } |
There was a problem hiding this comment.
Turns out was serializing id as prefixId (from creator).
So I thought... if objectId, let's not rename?
There was a problem hiding this comment.
Hmmmh. Actually, I think this test is invalid! Renaming should be done, as described.
And would occur in 2.x if ParameterNamesModule was registered.
Sorry for wasting you time here @JooHyukKim :-(
There was a problem hiding this comment.
No issue, but I am still concerened there might be issue because the serialization output changed from "id" to "prefixId" 🤔
Or could that also be affected by ALLOW_FINAL_FIELDS_AS_GETTERS?
There was a problem hiding this comment.
Difference is, I think, that 2.x does not get implicit names from constructor parameters (without ParameterNamesModule); 3.x does. So renaming works as -- I think -- it actually should, in 3.x (and in 2.x if PNM was registered).
Why @JsonProperty("prefixId") was added I do not know: seems like a mistake.
JooHyukKim
changed the title
@JsonIdentityInfo #5238
|
Closed since there isn't a bug to fix, I think. Not sure why original test had that rename... but that seems wrong. |
|
Ah so it just worked in 2.x then? |
Yes and no; 2.x just did not apply rename due to missing constructor parameter name (due to not registering |
|
Okay, this is still a behavior change from Jackson 2 to 3. So I am still wondering how we should be concluding this case, if...
WDYT? |
|
Test case would have failed in 2.x too if But I don't think this was a valid test case: the original problem was fixed. And changes in handling of invalid usage is not something we need or should document. |
|
@JooHyukKim but just to be sure: is there part of handling wrt the original reported problem (#5238) you think did get worse? I hope I am not missing something obvious: I just do not recall use of "prefixId" there. |
|
If we are talking about directly part of the original report, not much issue I guess. But one thing finding it odd. Is that assumption that we possibly start considering paramnames module affecting behaviors by default. I dont think we made warning or suggest alternative (or as far as I remember). Would be good to "disable" already integrated paramnames module functionaity like MapperBuilder.disable() method. |
|
@JooHyukKim To be honest, I think lack of parameter names is the flaw and everyone should have been using parameter names module. Having said that, if there was a new |
Wrote #5315 as a follow up. Implementation seems clean (I think) |
Fix ---#5238--- #5262