Fix handling of ObjectId-property in JsonIdentityInfo for uniform deserialization

[JooHyukKim]

Description

• Fix issue with object ID handling during setter-based deserialization.
• Closes Difference in the handling of ObjectId-property in JsonIdentityInfo depending on the deserialization route #3838.

[cowtowncoder]

Thank you for providing this @JooHyukKim ! I hope to find time to review it; looks straight-forward enough.

[JooHyukKim]

Thank you for providing this @JooHyukKim ! I hope to find time to review it; looks straight-forward enough.

🙏🏼🙏🏼. But regards to backward compatibility, would it be okay to throw an error for setter-based deserialization of empty JSON object?

[cowtowncoder]

Actually... no, I don't think this is right. Where does this logic come from? Is it based on similar pattern somewhere, or just trying to see if it resolves specific failing test case?

[JooHyukKim]

@cowtowncoder. I did refer to AbstractDeserializer's handling(below code) of object id, but I added p.getCurrentToken() == JsonToken.END_OBJECT to check the value to read is actually empty JSON object.

    @Override
    public Object deserializeWithType(JsonParser p, DeserializationContext ctxt,
            TypeDeserializer typeDeserializer)
        throws IOException
    {
        // Hmmh. One tricky question; for scalar, is it an Object Id, or "Natural" type?
        // for now, prefer Object Id:
        if (_objectIdReader != null) {
            JsonToken t = p.currentToken();
            if (t != null) {
        ...

[JooHyukKim]

Actually... no, I don't think this is right.

May I ask what makes you think that? 👀 I can assume maybe because current solution covers too many cases or too early in deserialization cases. 🤔

[cowtowncoder]

@JooHyukKim Let me try to go back to code -- maybe I am confused.
There are 2 parts to Object Id handling basically -- inclusion of Object Id as part of JSON Object (additional property); and then Object Id Reference (either scalar id or, in very limited cases, single-propert Object). So I want to make sure I understand which parts are being processed.

I'll add another note wrt timing/

[cowtowncoder]

Ok: due to timing -- I REALLY want to close out 2.15.0, and need to publish -rc3 -- I will not include this PR in 2.15.0 just to reduce risk. I will try to review & give feedback, but change itself most likely needs to go in 2.16. That gives us more time to make sure changes align with architecture & overall design.

[JooHyukKim]

Ok: due to timing -- I REALLY want to close out 2.15.0, and need to publish -rc3 -- I will not include this PR in 2.15.0 just to reduce risk. I will try to review & give feedback, but change itself most likely needs to go in 2.16. That gives us more time to make sure changes align with architecture & overall design.

I will also try to look for references regarding architecture & overall design. Please let me know if there is any that you recommend. Thank you! 🙏🏼

[cowtowncoder]

This PR seems to be in bit of messed up state?

[JooHyukKim]

This PR seems to be in bit of messed up state?

Is it? I suppose youre refering to the force push?

P.S. If you still think this solution seems broken 🔗 as commented here, maybe we can just keep the test cases under failing

[cowtowncoder]

@JooHyukKim Actually now diffs look good. Not sure what I was seeing earlier :)