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

Handle null type id for polymorphic values that use external type id #942

Closed
wants to merge 1 commit into from

Conversation

stormboy
Copy link

Remove exceptions thrown when polymorphic value is null.
If there is a need to force non-null value, this could be provided as an extra property in @JsonTypeInfo, or perhaps use an existing property such as JsonProperty.required.

Remove exceptions thrown when polymorphic value is null.
@cowtowncoder cowtowncoder changed the title Handle null polymorphic value Handle null type id for polymorphic values that use external type id Sep 24, 2015
@cowtowncoder
Copy link
Member

What actual problem is this fixing? Could you provide a test case to show the problem you have. I am not sure, looking at the patch alone, that this is a valid change. Type Ids are typically required, unless there is defaultImpl defined. How should actual expected polymorphic type determined, if no type id is available?

@stormboy
Copy link
Author

This is a case when there exists a type Id via an EXTERNAL_PROPERTY but the polymorphic value itself is null. See http://stackoverflow.com/questions/28089484/deserialization-with-jsonsubtypes-for-no-value-missing-property-error for an example.

@cowtowncoder
Copy link
Member

Hmmh ok. That's bit unusual (nulls are not typed, so there really should not be an id IMO), but I guess it could be supported.

Would it be possible to add a simple unit test, with commentary, both to verify the fix and as bit of documentation?

@cowtowncoder cowtowncoder modified the milestones: 1.9.13, 2.6.3 Sep 30, 2015
@cowtowncoder
Copy link
Member

@stormboy Thank you for suggesting this, it makes sense. I ended up solving the issue slightly differently, so I won't be merging the patch as is, but the end result should be same. Fix will be in 2.6.3.

@stormboy
Copy link
Author

Thanks @cowtowncoder Sorry I didn't get back with a unit test and commentary. I had been planning to, but have been busy working on other things.

@cowtowncoder
Copy link
Member

@stormboy No problem -- SO issue and your comments clarified this enough so it was not a big deal in the end. But if you do get a chance, it would be great to verify this against 2.6.3-SNAPSHOT, if you can do a local build.

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

Successfully merging this pull request may close these issues.

2 participants