Fixes #632: Detect cycles when constructing a JSONObject

[guptnis]

1. Added a method layer to check before converting an object to a JsonObject if there exists a cyclical dependency somewhere.
2. If yes, throws Exception
3. Changes to testGenericBean and testGenericIntBean. The "get" method is now expected to be called twice.

[johnjaylward]

Please undo the formatting changes and have your code changes match the current project formatting.

[johnjaylward]

There are still a lot of unrelated changes due to white space differences

[guptnis]

@johnjaylward I did not add any spaces/indents. I think intellij is doing it by default. Any pointers on how to prevent intellij from changing the formatting?

[johnjaylward]

I don't use intellij, but perhaps the suggestions here may help you: https://stackoverflow.com/questions/30045874/how-to-disable-intellij-auto-change-my-code-format

[guptnis]

@johnjaylward @stleary corrected the file indentations

[johnjaylward]

Overall I think your approach is in the right direction, however I think it can use some optimization.

1. For bean processing, your check makes us cycle through the entire bean twice, which seems unnecessary. If you instead just keep the setOfInstanceVariables locally for the bean check, you can keep everything in the 1 loop and just throw the exception immediately from the building of the JSON object instead or pre-processing.
2. For the map processing, I feel like there is a gap in the logic there. I think more use-cases need to be thought of for a complete view here. I don't see any test cases to confirm that the changes in place do what's expected. i.e Map_A -> Map_B -> Map_C -> Map_A would throw an exception or Map_A -> Bean_B -> Bean_C -> Map_A throws an exception
3. Overall on the testing, I'd like to see a few more thorough tests. The 2 test cases provided are very simple and don't cover some more complex mapping that may be cyclical. For example Bean_A -> Bean_B -> Bean_C -> Bean_A

Another test that should not throw an exception may be something with a structure like this:

       Bean_A
      /      \
Bean_B       Bean_C
                \
              Bean_B

Even though Bean_B shows up twice in the graph, it's not cyclical

[johnjaylward]

This processing the close of the value may not be safe here since we are not using the value at this time. If for some reason the value does need to be used after this method is called, it may not be available since it's already been closed by this method.

[johnjaylward]

I'm not confident this is doing what you expect and I fear this is adding a lot of overhead for deep maps. This appears to check every child every time a new depth is processed instead of checking the entire map 1 time.

Also, the "cyclic" check doesn't seem to take into account the use of sub-maps.

[stleary]

Closing this PR since an alternate solution has been accepted.