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

Add DeserializationFeature.FAIL_ON_MISSING_EXTERNAL_TYPE_ID_PROPERTY #1341

Merged
merged 1 commit into from
Aug 25, 2016

Conversation

ckuhn
Copy link
Contributor

@ckuhn ckuhn commented Aug 17, 2016

This property controls the behavior of deserializing subtype properties
that use the EXTERNAL_PROPERTY type id.

This property controls the behavior of deserializing subtype properties
that use the EXTERNAL_PROPERTY type id.
@ckuhn
Copy link
Contributor Author

ckuhn commented Aug 17, 2016

If this shouldn't be configurable with a DeserializationProperty, let me know and I will fix it so that it only throws the exception in case of a required property.

@cowtowncoder
Copy link
Member

Do you have a separate issue for PR to explain why this is needed, thinking behind needing the feature?

@ckuhn
Copy link
Contributor Author

ckuhn commented Aug 17, 2016

The issue is that when the subtype property is missing, an exception is thrown. I had some legacy input to take that sometimes included the suptype property and sometimes didn't. I couldn't figure out any other way to fix the issue besides adding a feature, or simply not throwing the exception.

Is there another way around this issue?

Quick example:

No issue with this

{
   "type": "orange",
   "fruit": {
      "name" : "Mandarin"
   }
}

Exception thrown, when 'fruit' property could just be null.

{
   "type": "orange"
}

@cowtowncoder
Copy link
Member

Ok, so this would be for cases where "type id" itself is specified, but no actual payload (data)?
That sounds legit to me. The only question is whether feature name is optimal to give the idea of its working (that is, is there an even better name to use :) ).

Thank you for explaining the intent.

@cowtowncoder
Copy link
Member

Before merging the patch, the only process thing we need is to get a filled Contributor License Agreement from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

(unless I have asked for one before?)

The usual method is to download, print, fill + sign & scan it, and email to info at fasterxml dot com.
With that I can proceed with merge, plus CLA is good for any other contributions for Jackson projects as well.

Thank you for submitting this PR!

@ckuhn
Copy link
Contributor Author

ckuhn commented Aug 18, 2016

I can change the feature name if needed. Any suggestions?

@cowtowncoder
Copy link
Member

Thanks, I can merge, consider the change.

One problem wrt 2.8 is that I think the default must be true, actually, to avoid any behavioral change, and switch default in 2.9.
Or wait for 2.9 to add this in, with false straight away.
Given that if merged for 2.8 it will be "hidden" feature (documented only for 2.9 as new functionality ought to be added in minor versions, not patches), approaches are quite similar; so I think what I'll do is just to merge in and make default true.

I noticed the CLA, so I can proceed with merging soon, hopefully tomorrow. Thanks again!

@cowtowncoder cowtowncoder merged commit 44bc080 into FasterXML:master Aug 25, 2016
cowtowncoder added a commit that referenced this pull request Aug 25, 2016
…nce we switch master to be 2.9.0-SNAPSHOT
@cowtowncoder cowtowncoder changed the title Added FAIL_ON_EXTERNAL_TYPE_ID_MISSING_PROPERTY deser property Add DeserializationFeature.FAIL_ON_MISSING_EXTERNAL_TYPE_ID_PROPERTY Aug 25, 2016
@cowtowncoder cowtowncoder added this to the 2.8.2 milestone Aug 25, 2016
@cowtowncoder
Copy link
Member

Renamed feature as FAIL_ON_MISSING_EXTERNAL_TYPE_ID_PROPERTY.

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