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
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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);
}
p.nextToken();
Object value2 = deserialize(p, ctxt);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
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;

// [databind#4680] Custom key deserialiser registered for `Object.class` is ignored on nested JSON
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_"));

// before fix, depth 2 does NOT work as expected
Map<String, Object> addressMap = (Map<String, Object>) result.get("address_");
// before fix, 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