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

JsonTypeInfo.As.PROPERTY type id, property with same name, result in duplicate JSON property #2022

Closed
marcelstoer opened this issue May 4, 2018 · 10 comments

Comments

@marcelstoer
Copy link

marcelstoer commented May 4, 2018

Origin: https://stackoverflow.com/q/50163882/131929

The Swagger Java codegen produces a class like this:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "errorType", visible = true)
@JsonSubTypes({
    @JsonSubTypes.Type(value = SpecificError.class, name = "SpecificError"),
})
public class GeneralError {
    @JsonProperty("errorType")
    private String errorType = null;
    // accessors, even for errorType!, follow here

The Jackson serializer, version 2.9.4, produces JSON that includes the errorType property twice:

{"errorType":"SpecificError","message":"message","errorType":null}

My test bed is as follows:

SpecificError specificError = (SpecificError) new SpecificError().message("message")
ObjectMapper objectMapper = new ObjectMapper();
ObjectWriter writer = objectMapper.writer();
System.out.println(writer.writeValueAsString(clientError));

I'm not 100% sure if Jackson is supposed to behave like that given the annotations you see above. Based on what I read in the JsonTypeInfo Javadoc it seems incorrect. However, I also raised this as swagger-api/swagger-codegen#8137.

@cowtowncoder
Copy link
Member

I think Swagger should use As.EXISTING_PROPERTY type for this case, where there is both type id (metadata) and actual property (data). Ideally PROPERTY would be able to do this automatically too, of course (I think #1410 is for that), but it turns out to be rather difficult to implement as things are.

@cowtowncoder cowtowncoder changed the title JsonTypeInfo.As.PROPERTY incorrectly serialized JsonTypeInfo.As.PROPERTY type id, property with same name, result in duplicate JSON property May 5, 2018
@marcelstoer
Copy link
Author

I experimented with the various JsonTypeInfo.As.xxx options before I created this issue to get a feel for how the behavior changes.

When I use EXISTING_PROPERTY instead of PROPERTY errorType is created only once but its value is null. So, for

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "errorType", visible = true)
@JsonSubTypes({
    @JsonSubTypes.Type(value = SpecificError.class, name = "SpecificError"),
})
public class GeneralError {
    @JsonProperty("errorType")
    private String errorType = null;

I'll get this:

{"errorType" : null, "message" : "message"}

@cowtowncoder
Copy link
Member

Yes, the intention is that EXISTING_PROPERTY will expect there is an actual property in data model, and will not write type id separately (but rely on regular property value being written). On deserialization side behavior will not differ, and if type id is also to be read as value for regular property, visible property of @JsonTypeInfo needs to be set to true.

Now... the problem that results in double-writing for PROPERTY case is that the regular property and virtual type id property are not handled by same entity. Type ids are written by TypeSerializer, which delegates to JsonSerializer for actual content (regular properties). The two are very loosely bound, by one calling the other. Implementation of EXISTING_PROPERTY does not change this, it simply suppresses serialization by TypeSerializer.
Ideally it would be good to have a mechanism to synchronize the two, esp. since this is still an open problem for case of EXTERNAL_PROPERTY. But changing this part of control flow and API is not very easy.

@marcelstoer
Copy link
Author

Thanks for your valuable input and the analysis that was necessary for that.

The only work-around I see so far is to use PROPERTY (to have Jackson write the type id) but then to ignore null values on the discriminator property like so:

    @JsonProperty("errorType")
    @JsonInclude(JsonInclude.Include.NON_NULL)
    private String errorType = null;

However, as this is generated code it'll involve some ugly post-generating code manipulation. I know I can set the Include.NON_NULL property in the serialization config but as that affects the behavior globally it'd be too big of hammer.

@cowtowncoder
Copy link
Member

One suggestion: if you have not considered mix-in annotations yet, maybe consider those? They are designed to be used on 3rd party libraries where author has no control.

@nickshoe
Copy link

@cowtowncoder My 1 or 2 cents worth on this topic: should we consider implementing a new strategy that behaves like PROPERTY but doesn't require a dedicated POJO property to be defined? Would it be difficult to implement a new one with the current API?

@cowtowncoder
Copy link
Member

@nickshoe I am not sure what you mean. PROPERTY specifically is metadata that does not match to a POJO property. This is the original idea as POJOs do not typically have type discriminator as that is implied by type (class) hierarchy.
Only EXISTING_PROPERTY expects there to be such POJO property.

@nickshoe
Copy link

@cowtowncoder ok, but would it be working without errors also during deserialization? If I remember correctly, removing the discriminator property from the POJO will make the deserialization process raise an "unrecognized field" exception.
If it's so, then the intended usage of PROPERTY only delivers in the serialization process.

@nickshoe
Copy link

@cowtowncoder I was wrong in my last comment, that happens only if one uses visible = true in the @JsonTypeInfo annotation. Since the default value of the visible attribute is false, then PROPERTY works as expected.

@cowtowncoder
Copy link
Member

@nickshoe Yes, I think you are right: visible is meant to "expose" type id metadata as a property. It's bit of a work-around really, over the original case where type id was never exposed to other processing; and this before addition of EXISTING_PROPERTY. So the use case that was not covered originally (where type id maps to/from regular property) is supported by a combination of slightly cumbersome additions, as opposed to having been designed to work from the beginning.

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

3 participants