Issue 652 cycle detection doesnt catch cycles through collections #699
Conversation
|
The Java 8 test build did fail due to those two intentionally failing tests; let me know if you want me to add |
|
@hendrixjoseph Sorry for not responding sooner. Failing unit tests are a concern. |
| new JSONObject(new RecursiveList()); | ||
| } | ||
|
|
||
| @Ignore |
There was a problem hiding this comment.
Why the new JSONObject(new RecursiveList()); or why the @Ignore?
The new JSONObject(new RecursiveList()); is testing if it throws the right exception if the list contains the object which has the list.
The @Ignore (and there's two of them) are for two additional locations where I identified stack overflows when having the object-within-something - one case is within a Map, one case is within an array. Since I didn't fix them as part of this PR (I felt it was out of scope), I added the @Ignore annotation so I could show where the stack overflows were happening, but not have the test fail.
There was a problem hiding this comment.
Why do we need code that will be ignored? It may be an exception check or just remove it.
I added an As far as performance costs - I didn't measure anything, but it does do a linear search through each list to see if there's an object that will be processes recursively. So it slows it down O(n) for each list (where n is the size of the list). If there are no lists, then there are no performance costs. |
|
@hendrixjoseph Thanks for the PR, but I think it will impact execution time for large, nested collections. |
For issue #652:
This PR detects cycles through collections. It also adds a unit test to verify that this cycle is detected.
The PR also adds three additional unit tests - one of which passes, two of which fail. The passing one verifies that cycles are detected for simple Objects - this was competed with issue #632 / PR #645.
The two failing ones attempt to verify that a cycle is detected in arrays and maps. This is currently not implemented, and a felt it was outside the scope of this issue and PR. If the tests need to pass before the code is merged, I can add an
@Ignoreannotation to the two failing tests pending another issue & PR.