Serialization behavior changed in 2.14 when mixing @JsonIgnore and @JsonProperty on different levels of class hierarchy and/or accessors

[RB14]

Describe the bug
In our projects we rely very heavily on Jackson. In one of our recent dependency upgrades, Jackson was bumped from version 2.13.3 to 2.14.0, and we started seeing some odd behavior (mostly in serialization, but it's unclear if this affect deserialization as well). I believe the problems are related to #3357.

The problems manifest when there is a mix of a public field and a getter, and most of the times, it also involves a base class (possibly abstract), that defines the getters as well (as abstract). It happens when there is a mix of @JsonIgnore and @JsonProperty/@JsonView on either of the field/concrete getter/abstract getter (e.g. @JsonProperty on the abstract getter, and then @JsonIgnore on the overridden getter). I had to write a small test to demonstrate the issues... this test demonstrate only some of the issues we experienced, but there could be other issues, since, if I understand correctly, you'v changed the logic to decide when to serialize in cases where there is a mix of @JsonIgnore and @JsonProperty. I must say that, in some cases, when there is inheritance involved, it makes sense to have this kind of mix if the different annotations are added in different levels in the class hierarchy.

See test how to reproduce below.

Version information
2.14.0

To Reproduce

I ran the following test both with version 2.13.3 and 2.14.0:

public class JacksonJsonIgnoreTest {

  @Test
  public void testChildClassSerialization() throws JsonProcessingException {
    ObjectMapper mapper = new ObjectMapper().configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false);
    Child child = new Child();
    System.out.println(mapper.writeValueAsString(child));
  }

  abstract static class Base {

    @JsonProperty
    abstract String getField1();
    @JsonProperty
    abstract String getField2();
    @JsonProperty
    abstract String getField3();

  }

  static class Child extends Base {

    // ---------------------------------------------------
    // Getter exists in base class, no annotation on field
    // ---------------------------------------------------

    public String field1 = "field1";

    @Override
    @JsonIgnore
    public String getField1() {
      return field1 + "Getter";
    }

    // -----------------------------------------------
    // Getter exists in base class, field is annotated
    // -----------------------------------------------

    @JsonProperty
    public String field2 = "field2";

    @Override
    @JsonIgnore
    public String getField2() {
      return field2 + "Getter";
    }

    // ----------------------------------------------------------------------
    // Getter exists in base class, field is ignored, no annotation on getter
    // ----------------------------------------------------------------------

    @JsonIgnore
    public String field3 = "field3";

    @Override
    public String getField3() {
      return field3 + "Getter";
    }

    // ----------------------------------
    // Getter doesn't exist in base class
    // ----------------------------------

    public String field4 = "field4";

    @JsonIgnore
    public String getField4() {
      return field4 + "Getter";
    }
  }
}

Results:

• 2.13.3: {"field1":"field1","field2":"field2","field3":"field3Getter"}
• 2.14.0: {"field2":"field2","field3":"field3Getter"}

The interesting things to note here:

• Comparing field1 and field4 - they have the exact same annotations on the field and getters, but the only difference is that field1 is also defined in the base class (with @JsonProperty) and field4 is not. In version 2.13.3, field1 was serialized, whereas in 2.14.0 it is not, whereas in both versions field4 is not serialized. It seems that the annotation over getField1 in the base class somehow affected serialization of the child class, even though we overrode it (so I would expect the annotation in the child class to win). In 2.14.0 this works "correctly", but while this behavior is more correct, it's still a big change in semantics (which might be considered a breaking change actually). Notice that there is no annotation on field1 itself (there no explicit @JsonProperty there), so basically we relied on the default behavior.
• field2 is exactly like field1, except that it has an explicit @JsonProperty annotation on the field. In this case, both versions decided to serialize it. So I'm raising a question, what is the difference between explicitly setting the @JsonProperty annotation (field2) to not setting it at all, and using the default semantics (field1)? Our code relies on the default behavior in lots of places (unfortunately...)
• field3 is the opposite case of field1 - the field has an explicit @JsonIgnore annotation, whereas the getter does not have any annotation (we rely on the default behavior here, again). Apparently both versions serialize it, and treat it as if it has a @JsonProperty annotation (maybe because of the base class? either way it's very confusing, especially when comparing it to the reverse direction, which is field1, and to the fact that if I override a method I expect not to inherit its annotations).

These are just a few samples, but of course I didn't test cases where the base class methods don't have annotations (in my tests all of them have @JsonProperty, what if they didn't have this set explicitly, and just rely on the default behavior?). And I haven't tested how this affects deserialization (like, what would happen if there is a contradiction between the annotations on the field and the annotations on the setter? or contradiction between an abstract setter and the concrete setter?)

Expected behavior
TBH it's unclear what should be the correct behavior, as there are many kinds of scenarios. Notice that, as I wrote above, it might make sense that the behavior for field1 will be the one implemented in 2.14.x, but I can't say the same about field3 (even though it's identical in both versions, I would expect some consistency in behavior between the 2 symmetrical cases of field1 and field3). In any case, since the behavior used to be like in version 2.13.x for a very long time, and now it has changed, I think we should consider this a breaking change, and considering the potential risks it impose, maybe worth reverting it to the original behavior. Accepting the changes in 2.14.x would require us to do (probably) lots of changes in our huge code base... and of course understanding the new behavior, and how it's different from the behavior in previous versions.

[cowtowncoder]

Some things that I can comment in, which I hope might be useful in understanding what you see:

1. Since annotations are "flattened", there should be no difference in whether something is defined in base type or super type: in the end only the "topmost" annotation is visible to Jackson introspection. This is straight-forward with exception of interfaces where ordering may seem arbitrary.
2. If there are different annotations on different types of accessors (field vs setter vs getter), then use of @JsonIgnore and @JsonProperty will lead to "split" case: one accessor is completely ignored and the other one included -- but logical property itself is included.
3. Annotations from all accessors are combined (unified) during processing, with methods (getter, setter) having higher precedence than fields. This, however, occurs after handling "split" case.
4. There were no changes intended to handling of visibility, so changes are almost certainly side-effect of another fix. I agree that @JsonIgnore does not if together with @JsonProperty or @JsonFormat #3357 looks like the change.

I also wonder if it might be good to actually split this up into separate issues for fields where there is observed difference in behavior.

[cowtowncoder]

Ok. So, on field1: I think that the correct behavior is that of 2.14.0: it should NOT be serialized. That it used to be serialized was a bug to fix, not a feature to maintain.
But I can see how it can be ambiguous; and especially so if ordering of annotations was reversed (base<->sub): behavior would still be the same, but it might look like override failed.

Fundamentally, however, what happens is that due to merging/flattening of annotations (which is done in general way with no knowledge of semantics of annotations, and especially without relationships between ignore/property) both @JsonIgnore and @JsonProperty is included for logical property field1.
And to me ignoral should have precedence over inclusion, if both are indicated.
(there is a way "override" ignoral annotation in sub-class, if need be, by using @JsonIgnore(enabled = false) FWTW).

[cowtowncoder]

And field2 should be serialized since it is a "split" case: "getter" is ignored (and completely unused), but field is included. This is more commonly used to separate "getter" and "setter", but here field acts as "accessor" (field or getter could either work).

So there is the definite difference between 2 cases:

1. One accessor has both @JsonIgnore and @JsonProperty (or equivalent): in this case, ignoral wins
2. One accessor has @JsonIgnore, another @JsonProperty: if so, one is ignored, the other included. And if there are un-annotated accessors (field vs getter vs setter), that gets grouped with included accessor

Rules also differentiate between explicitly annotated case (annotation indicates inclusion or ignoral) and "implicit" cases (accessor is Visible): implicit case is, by default, included; but it will not change explicit annotations.
So:

1. If there is ONLY ignoral, then all implicit accessors (and the whole property) is ignored
2. Otherwise -- explicit inclusion annotation, or no relevant annotations -- implicit accessors are included.

[cowtowncoder]

And then field3: that is another split case -- Field vs Getter -- so getter stays, field is ignored.

And... field4: ignored because there is only one explicit annotation (@JsonIgnore) for all accessors, so although field is visible, and would be included by default, it is overruled by explicit annotation.
This would be the case even if annotation was in Field.

[cowtowncoder]

It seems to me that the behavior, at least for these 4 cases, is as I'd expected and as intended, in 2.14.0.

The case of single accessors having both @JsonIgnore and @JsonProperty (either directly or through inheritance hierarchy) is fundamentally ambiguous, but the intent from my part has always been to give precedence to ignoral, as that seems like the most common use case (sub-classes adding ignoral as override).
The reverse -- overriding ignoral -- does require use of extra annotation (@JsonIgnore(enabled = false)) but that seems like a rare case.

But in your case there is further complication of different accessors, not just overrides over inheritance hierarchy.
That can get tricky, but I hope my explanation of "split accessors" case (keeping in mind that annotation flattening occurs first, and then accessors are compared without any consideration of inheritance hierarchy) helps understand logic. I am happy to expand more as necessary.

[RB14]

Hi @cowtowncoder ,

Really appreciate your detailed response!

TBH, I always thought that in Jackson, if you override a method, you also override all of its annotations, and if you want to preserve them, you have to redefined them. Maybe I may have gotten it wrong... I think it's kinda counter intuitive, because it doesn't allow to define scenarios like you described in your comment above (e.g. having @JsonIgnore in the base class, and overriding it with @JsonProperty in child class).

I think what bothers me most is the fact that behavior has changed (specifically for the case of field1) after it's been like that for a very long time. We've been using Jackson for many many years, and some old parts of our code rely on this behavior.

I'm trying to understand what other changes I should expect to behavior following the changes in 2.14.0. There are just too many scenarios to check. I'm trying to compile a list of scenarios I need to check in my code to detect all cases that broke (we have plenty of tests, but unfortunately they don't cover everything, so can't rely only on them).

BTW, specifically regarding field1 case, which is, I think, our main problem... Can you explain why it serialized the field in version 2.13 (btw, also if I reverse the order of @JsonIgnore and @JsonProperty between child<->parent), whereas in 2.14 it stopped serializing it? Was that the intended behavior of your fix? Note that both in version <= 2.13, and 2.14, the getter itself was ignored (as it should be). Only the field was serialized. So even prior to version 2.14, you handled the mix of @JsonProperty and @JsonIgnore correctly, you just didn't apply it to all accessors , was that part of the fix?

[cowtowncoder]

@RB14 Right, annotation merging is done on per-annotation basis, not on per-method -- so overriding one annotation does not override all. In my opinion this is better as it avoids having to always add each and every annotation from base types (esp. interfaces). But be that as it may, it has always been this way.

As to change in behavior: I was surprised that anyone would ever add conflicting definitions (@JsonIgnore and @JsonProperty), and that this would be a rare edge case. I also still do not see valid usage for it. But I understand that from your perspective things "were working" just fine as things were.
But this is recurring theme: if there are no unit tests verifying specific behavior, that behavior should be assume undefined. Put another way it'd be great if users could and did provide unit tests for specific usage -- otherwise I have no way of knowing if specific feature or setting is used in certain way out in the wild.

Still: as far as I know, there is just one, single, specific change (in #3357). It may have bigger effect in your codebase of course, but fundamentally it seems like a relatively limited change for user base overall.

On inclusion: first, as I said, parent/child relationship does not matter across annotations, only "within". So, if child has same annotation as parent then child one overrides parent setting.
But specific relative locations of @JsonProperty and @JsonInclude with respect to each other has no significance -- there is no override across annotations: they are flattened individually, first, and then comparison is based on ultimate final value.
This means that @JsonIgnore on parent or child is non-distinguishable when considering with @JsonProperty from parent or child.

So why did 2.13 include field1? Because existing code considered explicit inclusion annotation first and didn't consider possible ignoral at all (code short-circuited).
This is wrong, I think, and effectively it would have given @JsonProperty precedence over @JsonIgnore if both were included.

And yes, looking at code, I think change is what I expected to achieve. It also seems like what your example attempted to do (child having @JsonIgnore, parent @JsonProperty).

One thing I will do is to add a note on 2.14 release notes wrt this actual change in behavior.

[RB14]

@cowtowncoder, Thanks once again fro the detailed response and for your engagement!

I totally agree that the new behavior makes a lot of sense, and actually it might be a good thing because it exposed this wrong usage we had in our code, which shouldn't have worked to begin with. So my only concern is with the implicit change in behavior (after such a long time it behaved like that). We are probably using Jackson wrong in several other places, but I have to admit that we have a really huge code base, which also contains many legacy parts, some of which are not 100% covered by unit/integration tests, so it's going to be very difficult to fix everything to behave correctly.

However, that being said, I completely accept and agree with your reasoning. Especially the part where you talked about what should be considered "expected" behavior in general... if you don't have unit test for this, and you didn't think about it, and it's not documented as part of the official semantics of the library, we should not considered it to be "expected". This is fair.

Regarding:

As to change in behavior: I was surprised that anyone would ever add conflicting definitions (@JsonIgnore and @JsonProperty), and that this would be a rare edge case. I also still do not see valid usage for it.

Well, I think at least one valid use-case would be to override a @JsonIgnore which is defined in the base class, with a @JsonProperty in the child. But this use-case assumes wrong annotation semantics of Jackson (since, in reality, annotations are collected from across all levels), so might still be considered invalid. I was actually surprised by this behavior now that you described it, but this behavior hasn't changed (I mean the way Jackson collects and merges the annotations from all levels), so I can't rally complain. But I still insist that this use case does make sense (note how it differs from the use-case your described in the other ticket... you said that it doesn't make sense to have conflicting annotations on the same accessor, but from user POV, if the different annotations are on different levels of the hierarchy, I can see how some developers would think it DOES make sense to do that).

To decide on the best course of action for my team, I would like to ask a few questions:

1. Regarding the behavior of field1 in version 2.13.... According to your explanation, you said that the reason it was serialized in 2.13 is that, I quote, "it didn't consider possible ignoral at all" - if that's the case, why did it use the FIELD in the serialization, and not the GETTER? The @JsonIgnore annotation was put on the getter, and, evidently, it didn't use the getter for the serialization... so it actually did see the ignoral... But maybe in version 2.13 it just didn't apply it to the logical property, but just to the getter accessor. So I'm asking again, was this part of your fix? (I assume yes, please just approve again)
2. I'm trying to devise a plan for how to fix our code (assuming our tests won't catch all cases that broke). Would it be correct to assume that all we need to do is go over accessors that have conflicting annotations on some level of hierarchy, but we should not search for conflicting annotations on different accessors of the same property (cause this behavior hasn't changed). In other words, is it correct to assume that I should expect a change in behavior only on properties that have a certain (specific) accessor that has conflicting annotations? My plan is to go over all such properties, that have such accessors, and decide, one by one, what should be the correct behavior for them. I don't intend to go over properties that have conflicting annotations on the field and the getter or the field and the setter, or the getter and the setter, because, I assume the behavior here hasn't changed following the fix (sorry for repeating myself, just want to make sure I'm clear, this is delicate).

Adding a note to the release notes of 2.14 would be great!

Appreciate all your help here!

[cowtowncoder]

Ok, yes, fair enough: I do agree that it would be intuitive that annotations at different levels in hierarchy would be taken into account. And given complexity of actual resolution it is possible to assume different logic from observed behavior.
Similarly my statement of "if it isn't tested it isn't defined" (wrt specific behavior) is more a statement of how things are, baseline of minimum assumptions, and not a claim how everything should be. I do try to retain existing behavior beyond test cases; it is just much easier to maintain behavior where change results in a test failure.

Anyway, the big challenge in supporting things like inheritance-aware application of annotations is dimensionality: annotations are considered along (at least?) 3 dimensions:

1. Inheritance hierarchy (parent/child)
2. Multiple accessors of a single logical property (Field, setter, getter, Creator parameter)
3. Multiple annotation types (@JsonIgnore, @JsonProperty and a dozen others; some related, others not, with precedence

So system that takes into account the fact that, say, @JsonProperty of getter of field1 is in Child class, to give it priority over @JsonIgnore in Field field1 in Parent class would be quite complicated.
Part of the limitation then is the way in which dimensions are resolved: per-annotation-type inheritance hierarchy is first resolved ("flattened") before considering accessor-dimension. And third dimension is resolved last.

Anyway, back to field1: yes, you may be right in that my explanation was inaccurate. I did not notice reference to using Field for field1, but if that was the case, yes, it was sort of "false split":

1. Ignoral on getter was explicit, found
2. Inclusion of field (due to public visibility) was implicit but ended up considered explicit (effectively, but not by design)

whereas with 2.14 Field's status is determined to be only implicitly included (in absence of any explicit annotations included). At least I think that is how that's resolved.

And then, finally, to the question of how to find problem cases in your codebase: yes, I think you got it right -- the only changed case should be one where there are both @JsonProperty and @JsonIgnore for same class of accessors (getters, setters or fields). Other case -- getter having one, field another -- should not see any change in behavior.

[RB14]

@cowtowncoder , thanks so much for all this discussion!

I have no further questions at this point, but really appreciate all your help.

We've started adapting our code to the new behavior, and hopefully we will find all broken cases fast enough. For the time being though we decided to force a downgrade of Jackson to 2.13.4, but we are actively working on adapting our code to 2.14.

Feel free to close.

Thank you!

[cowtowncoder]

Sounds good @RB14.