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.

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

fix #4680 : Custom key deserialiser registered for Object.class is ignored on nested JSON #4684

Open
wants to merge 6 commits into
base: 2.18
Choose a base branch
from

Conversation

JooHyukKim
Copy link
Member

fixes #4680

Still have below three tests failing.

[ERROR] Errors: 
[ERROR]   DeepNestingUntypedDeserTest.testFormerlyTooDeepUntypedWithArray » StackOverflow
[ERROR]   DeepNestingUntypedDeserTest.testFormerlyTooDeepUntypedWithObject » StackOverflow
[ERROR]   TestAbstractTypes.testDefaultingRecursive:137 » JsonMapping (was java.lang.NullPointerException) (through reference chain: java.util.TreeMap["x"])

@JooHyukKim JooHyukKim marked this pull request as draft September 2, 2024 15:11
Comment on lines 237 to 243
if (keyDeser != null) {
if (untyped == null) {
untyped = new UntypedObjectDeserializer(this, keyDeser);
} else {
untyped = new UntypedObjectDeserializer(untyped, keyDeser);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cowtowncoder This is what I came up with atm. I was hoping maybe you might think of better approach than this? 😅

First, null/non-null check for keyDeser is done at line 228 and line 237
Second, declaring local variable untyped seems a bit unnatural.

@JooHyukKim JooHyukKim changed the title [DRAFT] fix #4680 : Custom keyDeserializer for nested JSON deserialized as Map<String, Object> [DRAFT] fix #4680 : Custom key deserialiser registered for Object.class is ignored on nested JSON Sep 2, 2024
@cowtowncoder
Copy link
Member

Ok, we might be able to make this work. But one quick idea: how about adding failing unit test first, in a separate PR? I could check that in first. And then hopefully help figure out what to do for actual problem.

FWTW, I suspect this will need to go in 2.19. But we can start PR against 2.18.

@@ -191,18 +218,32 @@ public JsonDeserializer<?> createContextual(DeserializationContext ctxt,
// 14-Jun-2017, tatu: [databind#1625]: may want to block merging, for root value
boolean preventMerge = (property == null)
&& Boolean.FALSE.equals(ctxt.getConfig().getDefaultMergeable(Object.class));
// 31-Aug-2024: [databind#4680] Allow custom key deserializer for Object.class deserialization
KeyDeserializer keyDeser = ctxt.findKeyDeserializer(SimpleType.constructUnsafe(Object.class), property);
Copy link
Member

@cowtowncoder cowtowncoder Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might need to see if we get the "default" key deserializer -- I forget how exactly it is done, maybe regular MapDeserializer has an example -- and if so, ignore it (re-set to null).

(I may be wrong wrt above, but basically wondering if ctxt.findKeyDeserializer() ever returns null or not)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added commit according to your suggestio and we are down to one last case 👌🏼👌🏼

[ERROR] Errors: 
[ERROR]   UntypedDeserializationTest.testUntypedWithCustomScalarDesers:263 » NullPointer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, simple mistake, didn't check if we had _customKeyDeserializer not null

@JooHyukKim
Copy link
Member Author

how about adding failing unit test first, in a separate PR?

Makes sense! Made #4685 against 2.18 (so that we can verify behavior since 2.18) @cowtowncoder

@JooHyukKim JooHyukKim marked this pull request as ready for review September 5, 2024 23:57
@JooHyukKim JooHyukKim changed the title [DRAFT] fix #4680 : Custom key deserialiser registered for Object.class is ignored on nested JSON fix #4680 : Custom key deserialiser registered for Object.class is ignored on nested JSON Sep 5, 2024
@JooHyukKim
Copy link
Member Author

Marked as ready for review and waiting for 2.19 branch out :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom key deserialiser registered for String.class is ignored when Map key type not defined
2 participants