Fix for issue 3481

[AmiDavidW]

URL: #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). I didn't understand the BrokenFilter Test. Please let me knonw if the change is proper.

[cowtowncoder]

Ok, I finally had a chance to look. I am pretty torn here... I think semantically this is wrong, as _suppressableValue is meant for non-nulls, and _suppressNulls for nulls. Yet I think you are right that this seems to work -- the main/only concern being that now _suppressableValue implementations may be called with null (keeping in mind it is used for all options, not just CUSTOM).

Unfortunately I cannot think of a straight-forward alternative; it seems necessary to add bit of new config/state in BeanPropertyWriter if filtering for nulls was to be done dynamically without overloading _suppressableValue like suggested here.

So I will have to think more about this. Apologies for slowness and thank you for your patience.

[AmiDavidW]

No worries, @cowtowncoder. Thanks a lot for your feedback.

I don't understand why _suppressableValue is limited for non-nulls.

The assumption of this fix is that _suppressableValue is independent to and has higher priority over _suppressNulls,
because _suppressableValue is customized and should override the general rules.

If the users do not want to suppress the value in a particular way, the _suppressNulls should still work as before.
I'm not sure this fix keeps this behavior.

Actually, all the tests are passing when I run mvn clean package locally with Java 8.
I assumed nothing is broken.

However, why there are two failing checks above?
Can I replicate them locally and how to fix them?

[cowtowncoder]

Original design started with _suppressNulls, and _suppressableValue is a later addition.
And as such they started strictly separate; former is for handling of nulls, and latter for non-nulls.
Latter was never meant to be used for nulls. This is why overloading the definition now is against my design intent; these are not strictly independent.

This is not to say that the design could not change; all I am changing is that I am hesitant because quite often there are subtle reasons why certain things work in certain ways: I may be forgetting some detail here.

But I agree that since only a small number of tests -- and in particular, ones that are quite specifically trying to drill into low level usage of these configuration settings -- fail, that's promising.
And I don't think any code outside jackson-core should be relying on these settings... however, given that it is possible to sub-class deserializers, it is of course possible some code somewhere does that. Despite being quite a fragile thing to do, developers tend to find such unintended (and unsupported) extension points when they are the only ways (they find) to implement certain functionality.
So depending on one's views on degree of compatibility required, this can be seen as an extension point, or not.

[AmiDavidW]

I understand that there could be subtle reasons why things are done as they are.

Can we say it's a safe change if all the tests are passing?

How can I replicate the errors in the PR build locally?
Are there any documents for it?

[cowtowncoder]

We can say whatever we want; the question is whether tests actually cover use cases that exist outside test suite.
Or put another way: unit/regression test suite may not be sufficient verification for some features. This is something I have learned over time. I am not saying it is necessarily the case for this particular change.

Now: there is something really weird going on with Github actions: errors seen there are bogus. I don't know when they started happening, or what might be causing them. It's... odd. And did not happen until very recently.
Perhaps I need to see which JDK to use; currently temurin is used as "distribution" for actions/setup-java@v3.

Running ./mvnw clean test shows what has to pass so PR checks are currently -- unfortunately! -- broken and needs to be ignored.

[AmiDavidW]

I would suggest acting based on what we know, which is the test results.

Definitely, there are always cases that we do not know.
If something is broken because of this change,
we can fix it and add new tests to cover it, as we did for this case.

Yes, ./mvnw clean test passes for me with Java 8

[AmiDavidW]

Hi @cowtowncoder,

I was thinking about the possible impacts of this change as in the cases below.

1. Not using customized filter: no impact
2. Using customized filter when fields are non-null: no impact
3. Using customized filter when fields are null: the fields may or may not be serialized as before

If so, the consequence of the third case is not harmful and easy to fix.
Just adding an additional null check in the filter.

Please correct me if I'm wrong.
Thanks

[cowtowncoder]

Hi @AmiDavidW! Sorry for the slow responses again. I think I am actually ok with the minor possibility of regression and we can go ahead. But one last thing: this has to be against 2.14 branch just so we can get proper testing; I don't want this to go in 2.13 patch. Could you rebase it, and I can merge it, assuming unit tests pass? (I think I figure out a small tweak to CI to avoid those odd transient problems).

[AmiDavidW]

Thank you very much @cowtowncoder!

I rebased on 2.14 and the ./mvnw clean test passed locally.
Just let me know if there's anything else to update.

[cowtowncoder]

Merged for inclusion in 2.14.0. Thank you @AmiDavidW for your patience here in getting this done. :)

[AmiDavidW]

Great :-) Thank you very much @cowtowncoder! The explanation and discussion are very interesting and helpful. I really appreciate it.