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

Filter method only got called once if the field is null when using `@JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = SomeFieldFilter.class)` #3481

Open AmiDavidW opened 2 years ago

AmiDavidW commented 2 years ago

Describe the bug The filter method only get called once if the field value is NULL, when using @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = SomeFieldFilter.class)

Version information 2.12.6.1

To Reproduce

  1. create an HTTP get endpoint to return an instance of ObjectDto which has a NULL value for fieldDto:
public class ObjectDto {

    @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = SomeFieldFilter.class)
    private FieldDto fieldDto;

}
  1. Send requests to the endpoint.
    The first time, the equals() method of SomeFieldFilter is called during the serialization. The same method never get called for the HTTP requests after.

  2. For instances that have a non-NULL value for fieldDto, the equals() method of SomeFieldFilter` gets called all the time during the serialization

Expected behavior For instances that have a NULL value for fieldDto, the equals() method of SomeFieldFilter also gets called all the time during the serialization

Additional context N/A

cowtowncoder commented 2 years ago

Thank you for reporting this issue. It sounds like possible bug.

It would be great to have a reproduction (unit test), if possible.

cowtowncoder commented 2 years ago

Ah, ok. This is a feature, not bug -- you are right, null check is only done once: this is an optimization to make null handling (more) efficient, partly, but also needed because value filtering has different code path for null values in general (many of default filters deal with nulls).

Calling filtering for repeated nulls would seem to serve no purpose, given that no meaningful arguments are passed to equals() (since it gets null). But I will try to see how to improve Javadocs, if possible.

AmiDavidW commented 2 years ago

Thank you very much for the prompt reply.

It's great that we have more optimized code running faster. I would hope the optimization does not change the behavior based on values.

I believe JsonInclude.Include.CUSTOM is a very cool feature that the JsonInclude can use more complex logic in the filter class to decide if a field shows in the serialized result or not, no matter what the value of the field is. The equals method of the filter class may return different results from time to time.

So, with this optimization, it behaves like this for non-null values, but not for the null value.

Which version introduced this optimization? Is it possible to check the filter every time even if the value is null?

Thanks in advance.

cowtowncoder commented 2 years ago

This has always been the behavior, since the addition of CUSTOM, as far as I remember. The skipping of call for null is partly due to implementation details -- null check is on distinct, separate code path all other types of values. So trying to make the call for every null is both unnecessary as far as I can see -- why would that ever need to vary? (keep in mind that filter instance is stateless and gets passed no contextual information) -- as well as a minor optimization, due to the way filtering code works.

Given above I don't think I have time to dig into whether change would be possible. But if you want to see if that is possible and propose a patch, I am not against that. I would, however, want to know why such a change is beneficial: that is, what exactly would be the use case?

AmiDavidW commented 2 years ago

I see. Thank you very much for the explanation.

I guess I didn't notice it before because the setting was only serializing non-null fields and using JsonInclude.Include.CUSTOM to do further customization. Now, the setting is always serializing everything as long as JsonInclude.Include.CUSTOM allows or there's no JsonInclude.Include.CUSTOM.

Actually, the results of equals of the filter change from time to time. For example, some fields are included during the weekdays, but not on weekends.

Yes, I can take a look to see if it's feasible. Could you show me where to start?

cowtowncoder commented 2 years ago

@AmiDavidW I don't remember specifically where, but you can search for Include.CUSTOM references in the codebase.

But as to filtering: if your filters are more complicated, you may want to check out use of @JsonFilter instead. @JsonInclude is meant for somewhat static use cases, where the value alone determines the logic. There are examples f.ex:

https://www.logicbig.com/tutorials/misc/jackson/json-filter-annotation.html

AmiDavidW commented 2 years ago

Thank you very much. I will check both

AmiDavidW commented 2 years ago

@cowtowncoder I provided a fix. Please review it and let me know if I need to make changes.

AmiDavidW commented 2 years ago

@cowtowncoder I signed and sent the CLA. Please let me know if there's anything else I need to do.

cowtowncoder commented 2 years ago

Thank you. I hope to have time to check the PR in near future -- thank you for contributing it and sending CLA!

cowtowncoder commented 2 years ago

Added a note on PR; apologies for slow followup.

AmiDavidW commented 2 years ago

No worries, @cowtowncoder. Thanks a lot for checking it.

I replied on the PR #3486