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

Allow using @JsonPropertyOrder with any-property (@JsonAnyGetter) #4396

Open
wants to merge 26 commits into
base: 2.18
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
6b13aeb
Add test
JooHyukKim Feb 19, 2024
85422ed
Clean up test utils
JooHyukKim Feb 19, 2024
8fe3a1f
Rename test to include issue related
cowtowncoder Feb 19, 2024
7a067d9
Change test to show how "Any properties" are not ordered as expected.
cowtowncoder Feb 19, 2024
8f3a301
Implement @JsonProperyOrder fixer
JooHyukKim Feb 22, 2024
d62c154
Add test wrt @JsonIgnore and @JsonIgnoreProperties
JooHyukKim Feb 24, 2024
0cfc627
Add test wrt @JsonPropertyOrder
JooHyukKim Feb 24, 2024
daf659d
Add test with link/unlink property
JooHyukKim Feb 27, 2024
7e97e1f
Add more tests
JooHyukKim Mar 2, 2024
9262fde
Remove _anyGetterWriter entirely
JooHyukKim Mar 3, 2024
e6dfd8d
Apply filter functionality
JooHyukKim Mar 3, 2024
6ce6510
Add tests
JooHyukKim Mar 3, 2024
6a0e2fd
Simplify logic
JooHyukKim Mar 3, 2024
9b7a14a
Modify test name
JooHyukKim Mar 3, 2024
941b320
Clean up
JooHyukKim Mar 3, 2024
b3947a3
Add more comment
JooHyukKim Mar 3, 2024
46af6cb
Fix test POJO annotated with @Test
JooHyukKim May 31, 2024
871bf62
Merge branch '2.18' into fix-4388
cowtowncoder May 31, 2024
9f86204
Merge branch '2.18' into fix-4388
JooHyukKim Jun 9, 2024
0c5671d
Merge branch '2.18' into fix-4388
cowtowncoder Jul 1, 2024
9a04fa6
Merge branch '2.18' into fix-4388
cowtowncoder Jul 11, 2024
6f6db89
Merge branch '2.18' into fix-4388
JooHyukKim Jul 20, 2024
9411e9c
Minor comment adds
cowtowncoder Jul 21, 2024
47da548
Merge branch '2.18' into fix-4388
cowtowncoder Jul 25, 2024
f850530
Merge branch '2.18' into fix-4388
cowtowncoder Aug 22, 2024
d615b2c
Merge branch '2.18' into fix-4388
JooHyukKim Sep 24, 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 @@ -558,8 +558,10 @@ protected void _addFields(Map<String, POJOPropertyBuilder> props)
_anySetterField = new LinkedList<>();
}
_anySetterField.add(f);
// 20-July-2024: [databind#4396]: Skip the rest of processing, but only
// for "any-setter', not any-getter
continue;
}
continue;
}
String implName = ai.findImplicitPropertyName(f);
if (implName == null) {
Expand Down Expand Up @@ -1092,7 +1094,8 @@ protected void _addGetterMethod(Map<String, POJOPropertyBuilder> props,
_anyGetters = new LinkedList<>();
}
_anyGetters.add(m);
return;
// 20-Jul-2024: [databind#4396] Do not stop processing here
// (used to return)
}
// @JsonKey?
if (Boolean.TRUE.equals(ai.hasAsKey(_config, m))) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
* for serializing {@link com.fasterxml.jackson.annotation.JsonAnyGetter} annotated
* (Map) properties
*/
public class AnyGetterWriter
public class AnyGetterWriter extends BeanPropertyWriter
{
protected final BeanProperty _property;

Expand All @@ -25,10 +25,14 @@ public class AnyGetterWriter

protected MapSerializer _mapSerializer;

/**
* @since 2.19
*/
@SuppressWarnings("unchecked")
public AnyGetterWriter(BeanProperty property,
public AnyGetterWriter(BeanPropertyWriter parent, BeanProperty property,
AnnotatedMember accessor, JsonSerializer<?> serializer)
{
super(parent);
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually safe? Doesn't it copy settings of "parent"?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was intended actually. Copying seemed safer than holding reference to original BeanPropertyWriterpropert propert that is soon to be a any-getter.
I referenced same pattern in VirtualBeanPropertyWriter, UnwrappingBeanPropertyWriter, like below.
Do you think there would be side effect?

    public UnwrappingBeanPropertyWriter(BeanPropertyWriter base, NameTransformer unwrapper) {
        super(base);
        _nameTransformer = unwrapper;
    }

    protected UnwrappingBeanPropertyWriter(UnwrappingBeanPropertyWriter base, NameTransformer transformer,
            SerializedString name) {
        super(base, name);
        _nameTransformer = transformer;
    }

_accessor = accessor;
_property = property;
_serializer = (JsonSerializer<Object>) serializer;
Expand All @@ -37,6 +41,16 @@ public AnyGetterWriter(BeanProperty property,
}
}

/**
* @deprecated Since 2.19, use one that takes {@link BeanPropertyWriter} instead.
*/
@SuppressWarnings("unchecked")
public AnyGetterWriter(BeanProperty property,
AnnotatedMember accessor, JsonSerializer<?> serializer)
{
this(null, property, accessor, serializer);
}

/**
* @since 2.8.3
*/
Expand Down Expand Up @@ -65,6 +79,11 @@ public void getAndSerialize(Object bean, JsonGenerator gen, SerializerProvider p
_serializer.serialize(value, gen, provider);
}

@Override
public void serializeAsField(Object bean, JsonGenerator gen, SerializerProvider prov) throws Exception {
getAndSerialize(bean, gen, prov);
}

/**
* @since 2.3
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ protected BeanSerializerBase asArraySerializer()
* - have per-property filters
*/
if ((_objectIdWriter == null)
&& (_anyGetterWriter == null)
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved
&& (_propertyFilterId == null)
) {
return new BeanAsArraySerializer(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@
import com.fasterxml.jackson.databind.ser.std.StdDelegatingSerializer;
import com.fasterxml.jackson.databind.ser.std.ToEmptyObjectSerializer;
import com.fasterxml.jackson.databind.type.ReferenceType;
import com.fasterxml.jackson.databind.util.BeanUtil;
import com.fasterxml.jackson.databind.util.ClassUtil;
import com.fasterxml.jackson.databind.util.Converter;
import com.fasterxml.jackson.databind.util.IgnorePropertiesUtil;
import com.fasterxml.jackson.databind.util.NativeImageUtil;
import com.fasterxml.jackson.databind.util.*;

/**
* Factory class that can provide serializers for any regular Java beans
Expand Down Expand Up @@ -457,7 +453,33 @@ protected JsonSerializer<Object> constructBeanOrAddOnSerializer(SerializerProvid
PropertyName name = PropertyName.construct(anyGetter.getName());
BeanProperty.Std anyProp = new BeanProperty.Std(name, valueType, null,
anyGetter, PropertyMetadata.STD_OPTIONAL);
builder.setAnyGetter(new AnyGetterWriter(anyProp, anyGetter, anySer));

JooHyukKim marked this conversation as resolved.
Show resolved Hide resolved
// Check if there is a accessor exposed for the anyGetter
BeanPropertyWriter anyGetterProp = null;
int anyGetterIndex = -1;
for (int i = 0; i < props.size(); i++) {
BeanPropertyWriter prop = props.get(i);
// Either any-getter as field...
if (Objects.equals(prop.getName(), anyGetter.getName())
// or as method
|| Objects.equals(prop.getMember().getMember(), anyGetter.getMember()))
{
anyGetterProp = prop;
anyGetterIndex = i;
break;
}
}
if (anyGetterIndex != -1) {
// There is prop is already in place, just need to replace it
AnyGetterWriter anyGetterWriter = new AnyGetterWriter(anyGetterProp, anyProp, anyGetter, anySer);
props.set(anyGetterIndex, anyGetterWriter);
} else {
// Otherwise just add it at the end, but won't be sorted...
// This is case where JsonAnyGetter is private/protected,
BeanPropertyDefinition anyGetterPropDef = SimpleBeanPropertyDefinition.construct(config, anyGetter, name);
BeanPropertyWriter anyPropWriter = _constructWriter(prov, anyGetterPropDef, new PropertyBuilder(config, beanDesc), staticTyping, anyGetter);
props.add(new AnyGetterWriter(anyPropWriter, anyProp, anyGetter, anySer));
}
}
// Next: need to gather view information, if any:
processViews(config, builder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,10 +215,6 @@ protected final void serializeAsArray(Object bean, JsonGenerator gen, Serializer
prop.serializeAsElement(bean, gen, provider);
}
}
// NOTE: any getters cannot be supported either
//if (_anyGetterWriter != null) {
// _anyGetterWriter.getAndSerialize(bean, gen, provider);
//}
} catch (Exception e) {
wrapAndThrow(provider, e, bean, props[i].getName());
} catch (StackOverflowError e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,8 @@ public void serializeAsField(Object pojo, JsonGenerator jgen,
writer.serializeAsField(pojo, jgen, provider);
} else if (!jgen.canOmitFields()) { // since 2.3
writer.serializeAsOmittedField(pojo, jgen, provider);
} else if (writer instanceof AnyGetterWriter) {
Copy link
Member Author

Choose a reason for hiding this comment

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

WIP : Contemplating on how should we optimize this check...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried referring to other writer/serializers and it turns out instanceof here check seems sort of reasonable.

((AnyGetterWriter) writer).getAndFilter(pojo, jgen, provider, this);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,6 @@ public abstract class BeanSerializerBase
*/
final protected BeanPropertyWriter[] _filteredProps;

/**
* Handler for {@link com.fasterxml.jackson.annotation.JsonAnyGetter}
* annotated properties
*/
final protected AnyGetterWriter _anyGetterWriter;

/**
* Id of the bean property filter to use, if any; null if none.
*/
Expand Down Expand Up @@ -119,13 +113,11 @@ protected BeanSerializerBase(JavaType type, BeanSerializerBuilder builder,
// 20-Sep-2019, tatu: Actually not just that but also "dummy" serializer for
// case of no bean properties, too
_typeId = null;
_anyGetterWriter = null;
_propertyFilterId = null;
_objectIdWriter = null;
_serializationShape = null;
} else {
_typeId = builder.getTypeId();
_anyGetterWriter = builder.getAnyGetter();
_propertyFilterId = builder.getFilterId();
_objectIdWriter = builder.getObjectIdWriter();
final JsonFormat.Value format = builder.getBeanDescription().findExpectedFormat();
Expand All @@ -142,7 +134,6 @@ protected BeanSerializerBase(BeanSerializerBase src,
_filteredProps = filteredProperties;

_typeId = src._typeId;
_anyGetterWriter = src._anyGetterWriter;
_objectIdWriter = src._objectIdWriter;
_propertyFilterId = src._propertyFilterId;
_serializationShape = src._serializationShape;
Expand All @@ -166,7 +157,6 @@ protected BeanSerializerBase(BeanSerializerBase src,
_filteredProps = src._filteredProps;

_typeId = src._typeId;
_anyGetterWriter = src._anyGetterWriter;
_objectIdWriter = objectIdWriter;
_propertyFilterId = filterId;
_serializationShape = src._serializationShape;
Expand Down Expand Up @@ -210,7 +200,6 @@ protected BeanSerializerBase(BeanSerializerBase src, Set<String> toIgnore, Set<S
_filteredProps = (fpropsOut == null) ? null : fpropsOut.toArray(new BeanPropertyWriter[fpropsOut.size()]);

_typeId = src._typeId;
_anyGetterWriter = src._anyGetterWriter;
_objectIdWriter = src._objectIdWriter;
_propertyFilterId = src._propertyFilterId;
_serializationShape = src._serializationShape;
Expand Down Expand Up @@ -402,9 +391,11 @@ public void resolve(SerializerProvider provider)
}

// also, any-getter may need to be resolved
if (_anyGetterWriter != null) {
// 23-Feb-2015, tatu: Misleading, as this actually triggers call to contextualization...
_anyGetterWriter.resolve(provider);
for (int i = 0; i < _props.length; i++) {
BeanPropertyWriter prop = _props[i];
if (prop instanceof AnyGetterWriter) {
((AnyGetterWriter) prop).resolve(provider);
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

Expand Down Expand Up @@ -770,9 +761,6 @@ protected void serializeFields(Object bean, JsonGenerator gen, SerializerProvide
prop.serializeAsField(bean, gen, provider);
}
}
if (_anyGetterWriter != null) {
_anyGetterWriter.getAndSerialize(bean, gen, provider);
}
} catch (Exception e) {
String name = (i == props.length) ? "[anySetter]" : props[i].getName();
wrapAndThrow(provider, e, bean, name);
Expand Down Expand Up @@ -821,9 +809,6 @@ protected void serializeFieldsFiltered(Object bean, JsonGenerator gen,
filter.serializeAsField(bean, gen, provider, prop);
}
}
if (_anyGetterWriter != null) {
_anyGetterWriter.getAndFilter(bean, gen, provider, filter);
}
} catch (Exception e) {
String name = (i == props.length) ? "[anySetter]" : props[i].getName();
wrapAndThrow(provider, e, bean, name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,24 @@ public void set(String name, String value) {
}
}

// [databind#1458]: Allow `@JsonAnyGetter` on fields too
static class DynaFieldOrderedBean {
public int id = 123;

@JsonPropertyOrder(alphabetic = true)
@JsonAnyGetter
@JsonAnySetter
private HashMap<String,String> other = new LinkedHashMap<>();

public Map<String,String> any() {
return other;
}

public void set(String name, String value) {
other.put(name, value);
}
}

// [databind#1458]: Allow `@JsonAnyGetter` on fields too
@Test
public void testDynaFieldBean() throws Exception
Expand All @@ -199,6 +217,18 @@ public void testDynaFieldBean() throws Exception
assertEquals("Joe", result.other.get("name"));
}

// [databind#4388]: Allow `@JsonPropertyOrder` AND `@JsonAnyGetter` on fields too
@Test
public void testDynaFieldOrderedBean() throws Exception
{
DynaFieldOrderedBean b = new DynaFieldOrderedBean();
b.set("nameC", "Cilly");
b.set("nameB", "Billy");
b.set("nameA", "Ailly");

assertEquals("{\"id\":123,\"nameA\":\"Ailly\",\"nameB\":\"Billy\",\"nameC\":\"Cilly\"}", MAPPER.writeValueAsString(b));
}

/*
/**********************************************************
/* Test methods
Expand Down Expand Up @@ -299,8 +329,7 @@ public void testAnyGetterWithMapperDefaultIncludeNonEmptyAndFilterOnBean() throw
input.add("empty", "");
input.add("null", null);
String json = mapper.writeValueAsString(input);
// Unfortunately path for bean with filter is different. It still skips nulls.
assertEquals("{\"non-empty\":\"property\",\"empty\":\"\"}", json);
assertEquals("{\"non-empty\":\"property\"}", json);
}

// [databind#2592]
Expand Down
Loading