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

Allow disabling native type ids when deserializing for #270 #271

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

manaigrn-amzn
Copy link
Contributor

This allows the check for native type ids to be disabled while deserializing. I would like to get feedback on this approach.

With the following example, native type ids will not be expected when deserializing.

IonObjectMapper mapper = new IonObjectMapper();
mapper.disable(IonParser.Feature.USE_NATIVE_TYPE_ID);

The full explanation of the problem this is solving can be found in #270

*
* @see <a href="https://amzn.github.io/ion-docs/docs/spec.html#annot">The Ion Specification</a>
*
* @since 2.12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If added at this point, must be marked as since 2.12.3 (but I'll add another note on this)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I will update it.

@cowtowncoder
Copy link
Member

Sounds reasonable. The only concern I have is that ideally new functionality should be in a new minor version (2.13).
Ion format has had a few challenges and this has not really been enforced, so it may be necessary to have one more exception here (since this may well be blocking problem), but I mention this just for future reference.
(2.13.0 release is still out 2 months or more, no release candidate yet planned).

Would like to have another opinion from other contributors, but makes sense to me. I'll add this to my todo list to re-review and make sure it can be merge before 2.12.3 is finalized (there are a few other changes and I plan to release that within 2 weeks or so).

@cowtowncoder
Copy link
Member

@manaigrn-amzn quick note: I assume this is under amazon ccla, so no need for separate cla.

@manaigrn-amzn
Copy link
Contributor Author

[...] so it may be necessary to have one more exception here (since this may well be blocking problem) [...]

Yeah, it would be greatly appreciated to get this change released as fast as possible. We are on a tight deadline to migrate to 2.12.x, and we are blocked without this change.

@manaigrn-amzn quick note: I assume this is under amazon ccla, so no need for separate cla.

Yes, that is correct.

}

IonParser(IonReader r, IonSystem system, IOContext ctxt, ObjectCodec codec) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should retain this intermediate constructor (at least for 2.12.x); will add a deprecated variant after merge.

@cowtowncoder cowtowncoder merged commit 625398d into FasterXML:2.12 Apr 6, 2021
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