FasterXML/jackson-databind#1296 @JsonIncludeProperties

[sp4ce]

Fix for #1296 (@JsonIncludeProperties)

Serialization

[sp4ce]

This PR is not for being merged as-is, it's just to get early input

[sp4ce]

Removing the [WIP] as this is now ready

[cowtowncoder]

Ok this looks pretty good, all around; only missing piece I noticed was that AnnotationIntrospectorPair also needs additions; question of how to combine settings might be interesting -- I assume non-null answer from primary should be used as-is; and only if it returns null, check out secondary.
This leads to bigger question of how to express things.

So: I wonder if there is need to indicate "just include all" case; at first I assumed so (from annotations), but I guess that is simply equivalent to leaving out annotation altogether.
However, I can see some potential use for specifying empty set, to mean "include no properties", resulting in { } serialization (except if there might be Type Id or Object Id or such).

I also mentioned possibility of adding enabled property, to use with mix-ins, to override annotation use. It may not really be needed yet; and it is possible to add after the fact, I think, given that there is Value object (which I think is a good idea).

Another possible change could be to pass JsonIncludedProperties.Value itself, instead of Set<String>, when passing things to deserializers (and not just by AnnotationIntrospector): the reasons to do so would be for expansion. I am not sure that would be valuable now, and it's probably not good to over-design for possible unknown future use.
But I like definition and use of Value at least during introspection. I have found this to be useful pattern.

I'll think about these things a bit more, but so far I think this looks pretty good!

[cowtowncoder]

Ok, so: AnnotationIntrospector... It is used to allow a few different use cases: original use was to allow support for both Jackson and JAXB annotations, for mostly overlapping usage.
Since then also allows various modules to change aspects usually configured via annotations, but that may come from other sources too (f.ex Scala and Kotlin modules inferring implied @JsonCreator for primary constructors).

In case of multiple introspectors all of them are arranged in logical chain, pair-wise, with precedence. For most annotations, primary introspectors is called to see if it finds an answer (for, say, property name via @JsonProperty); if yes, that is used, if not, secondary is asked.
In some cases it is also possible that both are checked, and if both return existing value(s), those are combined: this is the case for @JsonIgnoreProperties, which are additive.
But it need not necessarily be the case for @JsonIncludeProperties.

So, distinction between "not found" and "found empty" is often significant; and I think it may be important here too because not having annotation essentially means no-inclusion restrictions. But I think it looks like it depends on whether choice of both introspectors providing answer should be:

1. Primary one wins (just take first existing setting, use that): if so, AnnotationIntrospector must indicate "no annotation" with null (... or marker within Value object to indicate virtual "nothing" instance)
2. Combine the answers: if so, both values are taken and merged, no null value should be used since there is combination logic anyway.

Does this make more sense?

[cowtowncoder]

(note to self, mostly)

Makes sense to use different name, I think, withPropertyInclusions() (to indicate both ignoral and/or inclusion contained), for the new method.

[cowtowncoder]

Not 100% sure if this is the right default: seems like there would be 3 possibilities:

1. Intersection (must be in class defaults/annotations AND property)
2. Union (add anything property defines)
3. Overwrite/replace (use whatever property defines)

Now, adding "mode" or something in JsonIncludeProperties (and .Value) would allow specifying these modes easily if we want to. But even without adding that, I wonder what the most likely expected use case would be?
My guess would have been either (3) or (2).

[cowtowncoder]

As per my note on annotations PR (FasterXML/jackson-annotations#174), I am ready to merge the PRs and get this in for 2.12! Excited to add the feature, given its popularity.

[cowtowncoder]

Merged, changed in places. But now need to figure out why there is one fail for Guava Multimaps wrt ignoral -- probably something to do with this change (maybe something about constructor changes)

[cowtowncoder]

After looking at the issue, not a problem with this PR but changes I made afterwards. :)

[sp4ce]

Ok glad to hear it, I saw your message in #1296 but could not answer. If you need that I do some follow up on the PR, I can have a look as well.

[cowtowncoder]

@sp4ce thanks! I think we are good, I found the specific problem with changes I made (to add MapperConfig as argument for an existing method in AnnotationInstropector [or rather, overload])