FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.52k stars 1.38k forks source link

Replacing UnwrappingBeanSerializer fails if @JsonIgnoreProperties, @JsonFilter or @JsonIdentityInfo is used #1427

Open paolobazzi opened 7 years ago

paolobazzi commented 7 years ago

UnwrappingBeanSerializer can be extended and replaced using a BeanSerializerModifier to replace the BeanSerializer and using the BeanSerializer method unwrappingSerializer() to replace the UnwrappingBeanSerializer:

public JsonSerializer<Object> unwrappingSerializer(NameTransformer unwrapper) {
  return new UnwrappingBeanSerializerWithTypeInformation(this, unwrapper);
}

This works for simple examples, unfortunately there are more methods on BeanSerializer which again creates new instances of BeanSerializers, without having the overridden unwrappingSerializer() Method to create new instances of the replaced UnwrappingBeanSerializer.

For example this method creates another BeanSerializer without passing through the BeanSerializerModifier allowing to intercept and modify the BeanSerializer:

@Override
protected BeanSerializerBase withIgnorals(Set<String> toIgnore) {
    return new BeanSerializer(this, toIgnore);
}

I've created a code example, which shows the problem using the following annotations together with a replaced UnwrappingBeanSerializer

@JsonIgnoreProperties, @JsonFilter @JsonIdentityInfo

As soon as you use one of the above annotations, the replaced UnwrappedBeanSerializer is not taken into account.

Executing Test-Case:

Testcase 1 OK Testcase 2 NOK: JSON representation of object using @JsonIgnoreProperties does not contain the 'customField' added by UnwrappingBeanSerializerWithCustomField Testcase 3 NOK: JSON representation of object using @JsonFilter does not contain the 'customField' added by UnwrappingBeanSerializerWithCustomField

Testcase 4 NOK: JSON representation of object using @JsonIdentityInfo does not contain the 'customField' added by UnwrappingBeanSerializerWithCustomField

Expected output:

Testcase 1 OK Testcase 2 OK Testcase 3 OK

Testcase 4 OK

A workaround to avoid the problem is added in class ReplaceUnwrappedBeanSerializer (commented lines 76-90).

Expected behavior:

Jackson Version: 2.8.1 See also discussion https://groups.google.com/forum/#!topic/jackson-user/Aw0y6XgToH8

Testcase:

ReplaceUnwrappedBeanSerializer.java.txt

cowtowncoder commented 7 years ago

I guess I do not understand the issue. Earlier I thought it was just a question of UnwrappingBeanSerializer missing overrides it should have, but this is not the case, So I misunderstoof the concern.

Now... while it is technically possible, I don't think extending of UnwrappingBeanSerializer is a good idea; and it is not a use case that is explicitly supported. But if developer wants to extend it, (s)he has to take to override relevant withXxx() methods: this is necessary to create appropriate subtypes. I agree that this is fragile if and when new variants are added.

One thing that could be added would be to add a check in withXxx() methods to throw an exception with:

if (getClass() != UnwrappingBeanSerializer.class) {
    ctxt.reportInvalidDefinition("Class %s should override method 'withIgnorals()'", getClass());
}

which would at least make it more obvious this is needed?

One other theoretical alternative would be to add a single withXxx() method that would take all possible arguments; but the problem here is that new methods are very likely to be added, and so signature would need to be changed. I guess it could be possible to define an argument wrapper to contain properties, in which case signature could remain unchanged (for "withXxx()" method), and just definition of that wrapper object.

But... would that be worth it?

paolobazzi commented 7 years ago

Sorry for my late answer. I see your point... one possiblity would be to use a createNewUnwrappingBeanSerializerInstance() method, and adding the various custom parameters to the UnwrappingBeanSerializer instance using a set of with() methods. Therefore overriding one factory method would be enough to replace the UnwrappingBeanSerializer instance in every case. Unfortunately, this would change the API of UnwrappingBeanSerializer from various constructor parameters to a configuration using with() methods.