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

Global @JsonInclude(Include.NON_NULL) for all properties with a specific type #1522

Closed
CarstenWickner opened this issue Feb 8, 2017 · 18 comments
Milestone

Comments

@CarstenWickner
Copy link
Contributor

CarstenWickner commented Feb 8, 2017

When trying to optionally skip the certain fields when serialising certain beans, I put them into Java-8 Optional<> wrappers, as that's what allows me to differentiate between omitted and explicit null values on deserialisation.

As described in my respective Stackoverflow question and answer, I struggled to achieve this in a generic way via MixIns, as the (default) JacksonAnnotationIntrospector is ignoring them (at least for the @JsonInclude annotation I was looking at).
My final workaround is far from ideal, but at least works for this one isolated use case.

Are MixIns intentionally left out there or is it a bug?


Confirmed this under 2.7.4, 2.7.8, 2.8.5, and 2.8.6.

@cowtowncoder
Copy link
Member

Annotation introspector does not have any annotation-specific behavior: any and all are applied without knowledge of intended semantics.

@cowtowncoder
Copy link
Member

Ok. I think it is still possible there is a subtle bug that you are seeing, although it is not in mix-in handling at all. Rather it may be in logic of determining details of @JsonInclude handling.

Could you simplify reproduction with minimal case: does not need to be Optional I think (since that's supported by an extension, so can't be used unit tests here).
If so, I can see how easy it'd be to resolve.

@CarstenWickner
Copy link
Contributor Author

CarstenWickner commented Feb 8, 2017

Okay, here we go. Might not be a minimal case, but it's simulating the Optional behavior I want and therefore covers my use case. I also included my workaround (as per the SO answer):

public class AnnotationIntrospectorSampleTest {

    public static class MyBean {
        public Integer one;
        public NullableInteger two;
        public NullableInteger three;
        public NullableInteger four;
    }

    public static class NullableInteger {
        public Integer value;

        public NullableInteger(Integer value) {
            this.value = value;
        }
    }

    @JsonInclude(JsonInclude.Include.NON_NULL)
    public static class NullableIntegerMixIn {
    }

    private ObjectMapper initObjectMapper(JacksonAnnotationIntrospector workaround) {
        SimpleModule optionalBeanModule = new SimpleModule();
        // custom serializer to simulate behavior of Java-8 Optional<>
        optionalBeanModule.addSerializer(NullableInteger.class, new JsonSerializer<NullableInteger>() {

            @Override
            public void serialize(NullableInteger nullable, JsonGenerator gen, SerializerProvider prov) throws IOException {
                if (nullable.value == null) {
                    gen.writeNull();
                } else {
                    gen.writeNumber(nullable.value);
                }
            }
        });
        // custom deserializer to simulate behavior of Java-8 Optional<>
        optionalBeanModule.addDeserializer(NullableInteger.class, new JsonDeserializer<NullableInteger>() {

            @Override
            public NullableInteger deserialize(JsonParser parser, DeserializationContext context) throws IOException, JsonProcessingException {
                Integer value = parser.getCodec().readValue(parser, Integer.class);
                return new NullableInteger(value);
            }

            @Override
            public NullableInteger getNullValue(DeserializationContext ctxt) throws JsonMappingException {
                return new NullableInteger(null);
            }
        });

        ObjectMapper mapper = new ObjectMapper();
        // default: include everything (even NULL values)
        mapper.setSerializationInclusion(JsonInclude.Include.ALWAYS);
        // register specific serializer/deserializer to mock Java-8 Optional<> behavior
        mapper.registerModule(optionalBeanModule);
        // add the mix-in with the non-default JsonInclude.Include.NON_NULL annotation
        mapper.addMixIn(NullableInteger.class, NullableIntegerMixIn.class);
        if (workaround != null) {
            mapper.setAnnotationIntrospector(workaround);
        }
        return mapper;
    }

    @Test
    public void testDeserialization() throws JsonParseException, JsonMappingException, IOException {
        String json = "{\"one\":null,\"two\":2,\"three\":null}";
        MyBean bean = initObjectMapper(null).readValue(json, MyBean.class);
        Assert.assertNull(bean.one);
        Assert.assertNotNull(bean.two);
        Assert.assertEquals(Integer.valueOf(2), bean.two.value);
        Assert.assertNotNull(bean.three);
        Assert.assertNull(bean.three.value);
        Assert.assertNull(bean.four);
    }

    @Test
    public void testSerialization() throws JsonProcessingException {
        MyBean bean = new MyBean();
        bean.one = null;
        bean.two = new NullableInteger(2);
        bean.three = new NullableInteger(null);
        bean.four = null;
        String result = initObjectMapper(null).writeValueAsString(bean);
        String expected = "{\"one\":null,\"two\":2,\"three\":null}";
        // FAILS, due to result = "{one:null,two:2,three:null,four:null}"
        Assert.assertEquals(expected, result);
    }

    @Test
    public void testSerializationWorkaround() throws JsonProcessingException {
        MyBean bean = new MyBean();
        bean.one = null;
        bean.two = new NullableInteger(2);
        bean.three = new NullableInteger(null);
        bean.four = null;
        String result = initObjectMapper(new JacksonAnnotationIntrospector() {

            @Override
            public JsonInclude.Value findPropertyInclusion(Annotated a) {
                if (NullableInteger.class.equals(a.getRawType())) {
                    return JsonInclude.Value.construct(JsonInclude.Include.NON_NULL, JsonInclude.Include.NON_NULL);
                }
                return super.findPropertyInclusion(a);
            }
        }).writeValueAsString(bean);
        String expected = "{\"one\":null,\"two\":2,\"three\":null}";
        // SUCCESS (thanks to workaround)
        Assert.assertEquals(expected, result);
    }
}

@cowtowncoder
Copy link
Member

@CarstenWickner Thank you! While I can't guarantee quick resolution here (lots to work on right now), having a reproduction puts this close to head of my queue here, help much appreciated.

@cowtowncoder
Copy link
Member

@CarstenWickner Ok so I can reproduce the issue. But it seems to me that failure would result even by normal application of @JsonInclude on class, regardless of use of mix-in?
That's not a big problem I just want to make sure I understand problem fully and try to eliminate parts that are not essential.

@CarstenWickner
Copy link
Contributor Author

CarstenWickner commented Mar 11, 2017

Since I'm using the actual Java Optional<> in the real application, I don't even have the option to put the annotation on the class. But it works by adding the @JsonInclude annotation on the field it is being used on.

In the above example, the following would do the trick as well:

public static class MyBean {
    public Integer one;
    @JsonInclude(JsonInclude.Include.NON_NULL)
    public NullableInteger two;
    @JsonInclude(JsonInclude.Include.NON_NULL)
    public NullableInteger three;
    @JsonInclude(JsonInclude.Include.NON_NULL)
    public NullableInteger four;
}

So the MixIn would only make the Optional<> (or here NullableInteger) include only non-null contents and not nonnull instances of itself?

@cowtowncoder
Copy link
Member

@CarstenWickner Yes, but with reproduction you can easily add @JsonInclude for NullableInteger, and that behaves same as mix-in. So I am reasonably confident this is not related to mix-in handling but the fact that class-annotation (mix-in is simply a trick to make class annotation stick to a class without direct addition) is not used in such a setup for some reason.

I did also observe that property-annotation does indeed work.

@cowtowncoder cowtowncoder changed the title JacksonAnnotationIntrospector ignores MixIns @JsonInclude(Include.NON_NULL) does not work as class annotation Mar 11, 2017
@CarstenWickner
Copy link
Contributor Author

How would one differentiate between those two use cases now though?
This seems to be one of the classic "It's not a bug, it's a feature" cases. Simply treating the @JsonInclude annotation on each class (via mix-in or not) to apply to every instance where this class is being used would probably break heaps of existing applications.

I'm assuming one would have to provide an additional (optional) parameter on addMixIn() to specify whether the mix-in should be applied on every occurence as field/method and/or on the type's contained fields/methods.
Sounds like a lot of work to respect that everywhere then. 😒

@cowtowncoder
Copy link
Member

@CarstenWickner Ah. At first I thought you might not have understood the issue... and then going back to working, realized you did more than I did. :)

Yes: there are 2 possible interpretations for what a @JsonInclude for a class (direct or via mix-in, no difference) could mean:

  1. Apply as default for all properties unless they have more specific property-annotations
  2. Apply as default for all properties of this type (unless there's per-property override)

Currently code does (1), I think. But I'll spend bit more time to try to recall if I had some ideas of how to do (2).

@cowtowncoder
Copy link
Member

Ok. I suspect this usage is, after all, unsupported. While class-annotation is used as the default for properties contained, it is not used as the default of all properties of annotated type.
This is true even when adding inclusion settings via

ObjectMapper.configOverride(type).setInclude(JsonInclude.Value.construct(....)

which is an alternative for mix-ins for @JsonInclude use case (and couple of others, but only a subset of all annotations).

However it might be possible to add desired functionality via configOverride(...) API, for Jackson 2.9.
In theory it would also be possible to add something via annotation system, but that might get bit unwieldy.

@CarstenWickner
Copy link
Contributor Author

In a nutshell, @JsonInclude (as surely a number of others) has different semantics when applied as type-annotation vs. property-annotation.

  1. A solution based on ObjectMapper.configOverride() could solve this for @JsonInclude specifically, but not generally.
  2. Allowing mix-ins for property-annotations would basically be a completely new feature with considerable amount of code necessary. This would only be needed for those annotations that have different semantics when applied as type-annotation vs. property-annotation.

For my particular use case, both solutions are acceptable. In general, option (2) would add more value.
Did I understand that correctly?

@cowtowncoder
Copy link
Member

@CarstenWickner I don't think we still agree on mix-in part. What I am saying is that effect of

@JsonInclude(Include.NON_NULL
class MyType {
}

is identical to

class MyType {
}

@JsonInclude(Include.NON_NULL
class MixIn {
}

mapper.addMixIn(MyType.class, MixIn.class);

so from Jackson perspective. It's just definition of what class annotation (direct or indirect via mix-in) means. But I understand that from user perspective application matters so you would need it to apply using mix-ins.

With that settled, yes, change of semantics would be somewhat far reaching.

Existing interpretation was meant to support convenience of suppressing "all nulls" of values of specific POJOs. So like:

@JsonInclude(Include.NON_NULL
public class CompactPOJO {
   public String nameMaybe; // only serialized if not null
   public Map<String,String> props; // same
   // and so on
}

But what you are looking for, equally useful, would be to specify that any and all properties value NullableInteger would default to similar behavior.
Unfortunately this is not easy to support as is, since it would either require removing existing functionality, or overloading it to mean 2 things (and possibly preventing some combinations).

What could be possible, however, could be adding something like:

// hypothetical, does not exist yet:
mapper.configOverride(NullableInteger.class)
    .setInclusionAsProperty(Include.NON_NULL)

which would then acts as override between POJO-inclusion-default (existing class annotation), and property overrides. Or, possibly, as default to be overridden by POJO-inclusion-default... this is bit tricky.

Does this make more sense?

@CarstenWickner
Copy link
Contributor Author

CarstenWickner commented Mar 12, 2017

  1. Regarding mix-ins and how they work we're on the same page now, thanks. 😉
  2. The configOverride() approach sounds very good and properly sufficient as there aren't that many other annotations that would really need to be supported in this way.

Regarding the override behaviour, I'd prefer this override to overrule the POJO-inclusion-default (at least for my particular use case).
Do you reckon this could be handled via a boolean flag on this new .setInclusionAsProperty() method? I.e. to decide per type whether the override should be more or less important than the POJO-inclusion-default.


For now, one could keep it simple for this new feature and make the value provided via configOverride().setInclusionAsProperty() always override the POJO-inclusion-default. I.e. only a specific property-annotation could still change it. If needed, this could be made configurable in the future.

@CarstenWickner CarstenWickner changed the title @JsonInclude(Include.NON_NULL) does not work as class annotation Global @JsonInclude(Include.NON_NULL) for all properties with a specific type Mar 12, 2017
@cowtowncoder
Copy link
Member

@CarstenWickner it seems to me that your preference would be good default preference, and for simplicity we could start with it.

@cowtowncoder cowtowncoder added 2.9 and removed 2.8 labels Mar 15, 2017
@CarstenWickner
Copy link
Contributor Author

@cowtowncoder, I created PR #1564 to illustrate what I imagine this to look like. Maybe you can have a look.

@CarstenWickner
Copy link
Contributor Author

PR #1566 is basically the same proof-of-concept, but based on the current master branch (i.e. version 2.9) instead of version 2.8.

@cowtowncoder
Copy link
Member

Thanks! Yes, it should be based for master. I hope to have a look soon -- I just pushed 2.9.0.pr2 last night.

@cowtowncoder
Copy link
Member

Included now for next 2.9.0 version: either 2.9.0.pr4, or, if no more pre-releases made, 2.9.0 itself.

cowtowncoder added a commit that referenced this issue May 1, 2017
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