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

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 this issue May 6, 2022 · 12 comments
Milestone

Comments

@AmiDavidW
Copy link
Contributor

AmiDavidW commented May 6, 2022

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

@AmiDavidW AmiDavidW added the to-evaluate Issue that has been received but not yet evaluated label May 6, 2022
@cowtowncoder
Copy link
Member

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

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

@cowtowncoder cowtowncoder added need-test-case To work on issue, a reproduction (ideally unit test) needed 2.13 and removed to-evaluate Issue that has been received but not yet evaluated labels May 6, 2022
@cowtowncoder
Copy link
Member

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
Copy link
Contributor Author

AmiDavidW commented May 8, 2022

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

Thank you very much. I will check both

@cowtowncoder cowtowncoder removed the need-test-case To work on issue, a reproduction (ideally unit test) needed label May 17, 2022
AmiDavidW pushed a commit to AmiDavidW/jackson-databind that referenced this issue May 17, 2022
URL: FasterXML#3481

1. Check JsonInclude.Include.CUSTOM filter before serialize null values
(BeanPropertyWriter).
2. Make JsonInclude.Include.CUSTOM and suppressNull independent
(PropertyBuilder).
3. Adapt the unit tests (JsonIncludeCustomTest).
AmiDavidW pushed a commit to AmiDavidW/jackson-databind that referenced this issue May 17, 2022
URL: FasterXML#3481

1. Check JsonInclude.Include.CUSTOM filter before serializing null
values
(BeanPropertyWriter).
2. Make JsonInclude.Include.CUSTOM and suppressNull independent
(PropertyBuilder).
3. Adapt the unit tests (JsonIncludeCustomTest).
@AmiDavidW
Copy link
Contributor Author

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

@AmiDavidW
Copy link
Contributor Author

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

@cowtowncoder
Copy link
Member

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

@cowtowncoder
Copy link
Member

Added a note on PR; apologies for slow followup.

@AmiDavidW
Copy link
Contributor Author

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

I replied on the PR #3486

AmiDavidW pushed a commit to AmiDavidW/jackson-databind that referenced this issue Jun 20, 2022
URL: FasterXML#3481

1. Check JsonInclude.Include.CUSTOM filter before serializing null
values
(BeanPropertyWriter).
2. Make JsonInclude.Include.CUSTOM and suppressNull independent
(PropertyBuilder).
3. Adapt the unit tests (JsonIncludeCustomTest).
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jun 20, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Jun 20, 2022
@cowtowncoder cowtowncoder changed the title Filter method only got called once if the field is null when using @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = SomeFieldFilter.class) Filter method only got called once if the field is null when using @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = SomeFieldFilter.class) Jun 20, 2022
cowtowncoder added a commit that referenced this issue Jun 20, 2022
@cowtowncoder cowtowncoder removed the 2.14 label Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants