fix #4680 : Custom key deserialiser registered for Object.class is ignored on nested JSON
#4684
Conversation
| if (keyDeser != null) { | ||
| if (untyped == null) { | ||
| untyped = new UntypedObjectDeserializer(this, keyDeser); | ||
| } else { | ||
| untyped = new UntypedObjectDeserializer(untyped, keyDeser); | ||
| } | ||
| } |
There was a problem hiding this comment.
@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
changed the title
Map<String, Object>Object.class is ignored on nested JSON
|
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); | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Thanks! Added commit according to your suggestio and we are down to one last case 👌🏼👌🏼
[ERROR] Errors:
[ERROR] UntypedDeserializationTest.testUntypedWithCustomScalarDesers:263 » NullPointer
There was a problem hiding this comment.
Ah, simple mistake, didn't check if we had _customKeyDeserializer not null
Makes sense! Made #4685 against 2.18 (so that we can verify behavior since 2.18) @cowtowncoder |
JooHyukKim
changed the title
Object.class is ignored on nested JSONObject.class is ignored on nested JSON
|
Marked as ready for review and waiting for 2.19 branch out :)) |
| @@ -508,6 +548,8 @@ protected Object mapObject(JsonParser p, DeserializationContext ctxt) throws IOE | |||
| LinkedHashMap<String, Object> result = new LinkedHashMap<>(2); | |||
| result.put(key1, value1); | |||
| return result; | |||
| } else if (_customKeyDeserializer != null) { | |||
| key2 = (String) _customKeyDeserializer.deserializeKey(key2, ctxt); | |||
There was a problem hiding this comment.
@JooHyukKim I think same check needed for 2 cases further down too?
(and test should check that first 5 keys per level get custom deserialized as well)
There was a problem hiding this comment.
Yes, seems like it. Will do
There was a problem hiding this comment.
Applied ur review and did minor plummeting (with a new helper function) via 1f7dfbc. Thanks!
|
Looks decent, need to re-base against 2.19 now. |
|
Rebase done! 👌🏼 |
|
Cleaned up and improved some comments. |
|
Hoping to get back to this issue once we resolve the pile of issues for 2.18.1. Not forgotten. :) |
|
@JooHyukKim minor merge conflicts to resolve. Hoping to get back to reviewing this. |
|
Resolved conflicts, thanks! |
|
I think this is ready -- will merge! |
fixes #4680
Still have below three tests failing.