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

> JSONObject(Map): Throws NPE if key is null #678

Closed
stleary opened this issue Apr 30, 2022 · 5 comments · Fixed by #703
Closed

> JSONObject(Map): Throws NPE if key is null #678

stleary opened this issue Apr 30, 2022 · 5 comments · Fixed by #703

Comments

@stleary
Copy link
Owner

stleary commented Apr 30, 2022

JSONObject(Map): Throws NPE if key is null

A bit late here, but I would consider throwing an exception for an input that used to be accepted a breaking API change. In my company's codebase we had some tests that verified this behavior that were broken as a result, and a currently unknown amount of code that could actually be depending on this behavior.

Obviously this ship has already sailed (and rejecting null keys makes much more sense), but can we at least point out the breaking change in the release history documentation? Would be very useful as a heads-up for anyone trying to upgrade from an old version.

Originally posted by @data-enabler in #405 (comment)

@stleary
Copy link
Owner Author

stleary commented Apr 30, 2022

I think it would be helpful to have a Breaking Changes section or page that includes the above comment. Will look into it.

@JayseMayne
Copy link
Contributor

JayseMayne commented Oct 8, 2022 via email

@TomerPacific
Copy link
Contributor

@stleary - I'd like to help out here (if it's still relevant). What exactly are you looking for?

@stleary
Copy link
Owner Author

stleary commented Oct 22, 2022

@TomerPacific This is a documentation issue.
docs/RELEASES.md should include text when potentially breaking changes are introduced.
In this issue, #405 was updated to throw an NPE from JSONObject(Map), and was released in 20180813. I have already updated the releases page, but the corresponding entry in RELEASES.md should have a similar change.

@TomerPacific
Copy link
Contributor

@stleary, I opened a PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants