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

Handle circular references in Java beans #645

Merged
merged 7 commits into from
Nov 21, 2021

Conversation

Zetmas
Copy link
Contributor

@Zetmas Zetmas commented Nov 18, 2021

Try to achieve the recursive check within one pass
Added some related test cases

new JSONObject(ObjA);
}
@Test(expected=JSONException.class)
public void testRepeatObjectRecursive() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should not throw an exception. There is no recursion here.

Copy link
Contributor Author

@Zetmas Zetmas Nov 18, 2021

Choose a reason for hiding this comment

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

Thanks for your reply @johnjaylward ! Could you please explain a little more about why this test case is not recursive? I assumed this test case was recursive because it starts with C and ends with C in both children branches, creating loops of C-B-A-D-C... I also ran the test and it seems there would be an Exception thrown. Thanks for your patience!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you are correct! My eyes glazed over the first C for some reason.

As additional tests, can you make these:

Self recursion, should throw exception:

A -> A
B -> A -> A

Complex graph, duplicate object(s), but no recursion (no exception thrown):

E -> B -> A -> D -> C
       -> D -> C

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additional test cases added

@johnjaylward
Copy link
Contributor

This looks like a much better approach for implementation than the first go-around. Just needs a a few corrections to make that one test not throw an exception.

@johnjaylward
Copy link
Contributor

@stleary can you approve the workflow?

@stleary
Copy link
Owner

stleary commented Nov 18, 2021

What problem does this code solve?
Circular references when parsing Java beans to JSON result in a StackOverflowException. This code change detects the cycle and throws a JSONException instead.

Risks
Low.
Functional risk: Low. The implementation looks correct.
Performance risk: Moderate. All Java bean parsing will have to pay the overhead for cycle checking. This is considered an acceptable tradeoff for not having StackOverflowExceptions.

Changes to the API?
No. A new private API function is added,

Will this require a new release?
No

Should the documentation be updated?
Probably not (TBD - check for references to handling of circular references)

Does it break the unit tests?
No.
New unit tests were added

Was any code refactored in this commit?
The existing code was modified to implement the cycle checking.

Review status
APPROVED

@stleary
Copy link
Owner

stleary commented Nov 18, 2021

Starting 3 day comment window

@stleary stleary changed the title Try to fix issue#632 Detect and handle circular references when parsing Java beans Nov 18, 2021
@stleary stleary merged commit bc623e3 into stleary:master Nov 21, 2021
@stleary stleary changed the title Detect and handle circular references when parsing Java beans Handle circular references in Java beans Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants