Skip to content

Fix IonValueDeserializer's getNullValue when bean is missing a property #320

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

Closed

Conversation

atokuzAmzn
Copy link
Contributor

This PR aims to resolve the issue #317

Changes:

  • Do not call getEmbeddedObject when current token is END_OBJECT
  • Add unit test case exposing the issue

@atokuzAmzn
Copy link
Contributor Author

@mcliedtke Can you review this?

Copy link
Contributor

@mcliedtke mcliedtke left a comment

Choose a reason for hiding this comment

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

Overall LGTM, one small comment on including an additional test assertion

@mcliedtke
Copy link
Contributor

Can you also comment on what branches you believe this change to be added to. Are you targeting for updating 2.13?

@atokuzAmzn
Copy link
Contributor Author

@mcliedtke good catch on missing check. I have added the extra assertion.
Yes we are targeting to update 2.13

@mcliedtke
Copy link
Contributor

LGTM, I think these changes can be merged. Given the nature of the change, I think merging to 2.13 is pretty safe. @cowtowncoder do you want to handle the merge itself?

@atokuzAmzn
Copy link
Contributor Author

@cowtowncoder or @mcliedtke
Can we merge this into 2.13?

@atokuzAmzn
Copy link
Contributor Author

Following up with this merge request @cowtowncoder @mcliedtke

@cowtowncoder
Copy link
Member

Ok, I am back from my vacation. @atokuzAmzn If you want this merged against 2.13, please rebase or recreate.
While cherry-picking sometimes works, the usual practice is to start with the oldest branch & merge forward.

@atokuzAmzn atokuzAmzn changed the base branch from 2.14 to 2.13 April 23, 2022 21:25
@atokuzAmzn
Copy link
Contributor Author

This is my first time working with github. I changed the base of this PR but I am not sure if this is what you wanted @cowtowncoder. If needed I can recreate a new PR based on 2.13

@cowtowncoder
Copy link
Member

@atokuzAmzn Alas, no: this would merge back all changes from 2.14 -- and not just ones you are making -- so that wouldn't work. I am not 100% sure what'd be the easiest way to change the base for existing PR. Maybe others can suggest something?

But it may be easier to simply create a new PR, if that works?

@atokuzAmzn
Copy link
Contributor Author

I created a new PR based on 2.13 here : #322

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.

3 participants