-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Updates for populateMap based on discussion in #279 and #264 #354
Conversation
JSONObject.java
Outdated
* </p><p> | ||
* Methods that return <code>void</code> as well as <code>static</code> | ||
* methods are ignored. | ||
* </p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for </p>
close tags in javadocs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I like them :) and it still works. Plus Java9 javadoc will support HTML 5 which requires the close tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see the javadoc command in java9 will insert the close tags for you. I'll get rid of them
a8e0c9e
to
f568c75
Compare
f568c75
to
641b68d
Compare
@johnjaylward I am on board with this. Cleaning up bean serialization is reasonable, and stack overflows that result from serialization must never occur. However, it would be better if, in each of the cases mentioned, a unit test is provided that causes incorrect processing that is fixed by the pull request. Do you agree, and is it practical to do this? |
That sounds good and relatively practical. Would you mind if I reorganized the classes in the test project a little? It's hard to quickly pick out the test classes from the classes used for mocking and testing. I'm thinking a sub package called |
@stleary I updated the test cases for supporting these changes. I believe I have decent coverage, but was unable to get the "isBridge" check in populateMap to get hit. If someone else could look at my GenericBean and GenericBeanInt classes/tests and let me know what I'm missing there it would be appreciated. I thought what I did would cover the isBridge check, but I apparently missed something I'm not understanding to get the compiler to generate a bridge method. Or maybe I am, but the bridge method is not public so it's being ignored by a previous check in the same |
Also, I updated the |
Ok, I figured out the bridge method. I just needed to override the getGenericValue method |
866def6
to
a129ebe
Compare
Sure, cleanup/reorg of the unit tests is fine. |
Starting 3 day comment timer. |
Key Changes:
populateMap
function to be more strict about what methods it calls.What problem does this code solve?
Fixes issues as noted in #279 and #264. Namely, this changes populateMap to now exclude the following:
void
I also tightened up the exception handling in the private method to only handle the checked exceptions. This should make debugging failed serializations easier as runtime exceptions will be propagated to the caller instead of creating an empty JSONObject.
Risks
Low. In general the method types now excluded should generally not be serialized. Even though in #279 it was noted that static should not be included, I did run unto an issue where serializing statics lead to a stack overflow as well.
Also, as noted in multiple places, static properties are considered part of a
class
not aninstance
. References:I believe keeping the current implementation of serializing static members would actually lead to unexpected behavior.
The argument @stleary made here for static methods doesn't hold up for me. If a static singleton class needs to be serialized, it should hopefully be easy enough for a developer to handle that as a one off case instead of the norm.
Changes to the API?
None
Will this require a new release?
No, can be rolled into the next release
Should the documentation be updated?
No
Does it break the unit tests?
No. All unit tests should pass. New unit tests were created that will fail against the current master, but pass with this PR. See stleary/JSON-Java-unit-test#75
Was any code refactored in this commit?
Yes, the private JSONObject.populateMap function
Review status
ACCEPTED