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 26 commits into
base: 2.19
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
f3d38b0
...
JooHyukKim Sep 2, 2024
e832d13
Improve test doc
JooHyukKim Sep 2, 2024
06f8b83
Merge branch '2.18' into fix4680
cowtowncoder Sep 5, 2024
a1fe9cc
Add check if is default key-deserializer
JooHyukKim Sep 5, 2024
9222ade
Improve validation to check if custom-key-deserializer is not null
JooHyukKim Sep 5, 2024
6e04144
Merge branch '2.18' into fix4680
cowtowncoder Sep 12, 2024
149180c
Merge branch '2.18' into fix4680
cowtowncoder Sep 20, 2024
03b365a
Minor renaming, tweaks
cowtowncoder Sep 20, 2024
f91f0a3
Merge branch '2.18' into fix4680
cowtowncoder Sep 20, 2024
8976fef
Remove dup test class
cowtowncoder Sep 20, 2024
e80b337
Minor streamlining
cowtowncoder Sep 20, 2024
1f7dfbc
Add and cover at least 5 keys cases
JooHyukKim Sep 21, 2024
874dd82
Update doc
JooHyukKim Sep 21, 2024
39aa9a1
Merge branch '2.18' into fix4680
cowtowncoder Sep 29, 2024
672c69e
Merge remote-tracking branch 'upstream/2.19' into fix4680
JooHyukKim Sep 29, 2024
2792f28
Add version note
JooHyukKim Sep 29, 2024
83af378
Add "since" to where needed
JooHyukKim Sep 29, 2024
c09b191
Merge branch '2.19' into fix4680
cowtowncoder Oct 2, 2024
517edba
tiny tweaks
cowtowncoder Oct 2, 2024
ef0e18c
some last (?) tweaks
cowtowncoder Oct 2, 2024
1a0cdbf
Add some JavaDoc
JooHyukKim Oct 2, 2024
212bc64
Add comments
JooHyukKim Oct 2, 2024
3c35bcd
Merge branch '2.19' into fix4680
cowtowncoder Oct 3, 2024
f58cdd6
Merge branch 'fix4680' of github.com:JooHyukKim/jackson-databind into…
cowtowncoder Oct 3, 2024
d48f5f6
Merge branch '2.19' into fix4680
cowtowncoder Oct 3, 2024
2989044
Merge branch '2.19' into fix4680
cowtowncoder Oct 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.fasterxml.jackson.databind.deser.ResolvableDeserializer;
import com.fasterxml.jackson.databind.jsontype.TypeDeserializer;
import com.fasterxml.jackson.databind.type.LogicalType;
import com.fasterxml.jackson.databind.type.SimpleType;
import com.fasterxml.jackson.databind.type.TypeFactory;
import com.fasterxml.jackson.databind.util.ClassUtil;
import com.fasterxml.jackson.databind.util.ObjectBuffer;
Expand Down Expand Up @@ -48,6 +49,13 @@ public class UntypedObjectDeserializer

protected JsonDeserializer<Object> _numberDeserializer;

/**
* Object.class may also have custom key deserializer
*
* @since 2.19
*/
private KeyDeserializer _customKeyDeserializer;

/**
* If {@link java.util.List} has been mapped to non-default implementation,
* we'll store type here
Expand All @@ -74,7 +82,7 @@ public class UntypedObjectDeserializer
*/
@Deprecated
public UntypedObjectDeserializer() {
this(null, null);
this(null, (JavaType) null);
}

public UntypedObjectDeserializer(JavaType listType, JavaType mapType) {
Expand All @@ -96,6 +104,7 @@ public UntypedObjectDeserializer(UntypedObjectDeserializer base,
_numberDeserializer = (JsonDeserializer<Object>) numberDeser;
_listType = base._listType;
_mapType = base._mapType;
_customKeyDeserializer = base._customKeyDeserializer;
_nonMerging = base._nonMerging;
}

Expand All @@ -112,9 +121,27 @@ protected UntypedObjectDeserializer(UntypedObjectDeserializer base,
_numberDeserializer = base._numberDeserializer;
_listType = base._listType;
_mapType = base._mapType;
_customKeyDeserializer = base._customKeyDeserializer;
_nonMerging = nonMerging;
}

/**
* @since 2.19
*/
protected UntypedObjectDeserializer(UntypedObjectDeserializer base,
KeyDeserializer keyDeser)
{
super(Object.class);
_mapDeserializer = base._mapDeserializer;
_listDeserializer = base._listDeserializer;
_stringDeserializer = base._stringDeserializer;
_numberDeserializer = base._numberDeserializer;
_listType = base._listType;
_mapType = base._mapType;
_nonMerging = base._nonMerging;
_customKeyDeserializer = keyDeser;
}

/*
/**********************************************************
/* Initialization
Expand Down Expand Up @@ -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

// 20-Apr-2014, tatu: If nothing custom, let's use "vanilla" instance,
// simpler and can avoid some of delegation
if ((_stringDeserializer == null) && (_numberDeserializer == null)
&& (_mapDeserializer == null) && (_listDeserializer == null)
&& getClass() == UntypedObjectDeserializer.class) {
return UntypedObjectDeserializerNR.instance(preventMerge);
if (keyDeser == null) {
return UntypedObjectDeserializerNR.instance(preventMerge);
}
}

UntypedObjectDeserializer untyped = null;
if (preventMerge != _nonMerging) {
return new UntypedObjectDeserializer(this, preventMerge);
untyped = new UntypedObjectDeserializer(this, preventMerge);
}
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.

if (untyped != null) {
return untyped;
}

return this;
}

Expand Down Expand Up @@ -496,6 +537,8 @@ protected Object mapObject(JsonParser p, DeserializationContext ctxt) throws IOE
if (key1 == null) {
// empty map might work; but caller may want to modify... so better just give small modifiable
return new LinkedHashMap<>(2);
} else {
key1 = (String) _customKeyDeserializer.deserializeKey(key1, ctxt);
}
// minor optimization; let's handle 1 and 2 entry cases separately
// 24-Mar-2015, tatu: Ideally, could use one of 'nextXxx()' methods, but for
Expand All @@ -508,6 +551,8 @@ protected Object mapObject(JsonParser p, DeserializationContext ctxt) throws IOE
LinkedHashMap<String, Object> result = new LinkedHashMap<>(2);
result.put(key1, value1);
return result;
} else {
key2 = (String) _customKeyDeserializer.deserializeKey(key2, ctxt);
Copy link
Member

Choose a reason for hiding this comment

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

@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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, seems like it. Will do

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied ur review and did minor plummeting (with a new helper function) via 1f7dfbc. Thanks!

}
p.nextToken();
Object value2 = deserialize(p, ctxt);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package com.fasterxml.jackson.databind.deser;

import java.util.Map;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.KeyDeserializer;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.databind.module.SimpleModule;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class CustomKeyDeserializer4680Test
{

@Test
void customKeyDeserializerShouldBeUsedWhenTypeNotDefined() throws Exception {
// GIVEN
String json = "{\n" +
" \"name*\": \"Erik\",\n" +
" \"address*\": {\n" +
" \"city*\": {\n" +
" \"id*\": 1,\n" +
" \"name*\": \"Berlin\"\n" +
" },\n" +
" \"street*\": \"Elvirastr\"\n" +
" }\n" +
" }";

SimpleModule keySanitizationModule = new SimpleModule("key-sanitization");
keySanitizationModule.addKeyDeserializer(String.class, new KeyDeserializer() {
@Override
public String deserializeKey(String key, DeserializationContext ctxt) {
return key.replace("*", "_");
}
});

keySanitizationModule.addKeyDeserializer(Object.class, new KeyDeserializer() {
@Override
public Object deserializeKey(String key, DeserializationContext ctxt) {
return key.replace("*", "_");
}
});

ObjectMapper mapper = JsonMapper.builder().addModule(keySanitizationModule).build();

// WHEN
Map<String, Object> result = mapper.readValue(json, new TypeReference<Map<String, Object>>() {
});

// THEN
// depth 1 works as expected
Assertions.assertEquals("Erik", result.get("name_"));

// depth 2 does NOT work as expected
Map<String, Object> addressMap = (Map<String, Object>) result.get("address_");
// null?? Fails here
Assertions.assertEquals("Elvirastr", addressMap.get("street_"));
Map<String, Object> cityMap = (Map<String, Object>) addressMap.get("city_");
Assertions.assertEquals(1, cityMap.get("id_"));
Assertions.assertEquals("Berlin", cityMap.get("name_"));
}

}
Loading