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

Fix cycles causing stack overflows #18

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@emm035

emm035 commented Sep 17, 2018

This PR fixes the bug where cyclical references in the definitions of models would cause stack overflows as both ModelDiff#diff and ModelDiff#convert2ElPropertys would recurse infinitely through the cycle.

This is now mitigated by passing a set containing the models that have already been "visited" in the current branch of the recursion tree. If a model has already been visited, the method will short circuit, and prevent any further traversal of the cycle. If a model is either inserted or missing, its tree will not be traversed (preventing all subfields being marked as added/removed).

To test that the stack overflows are fixed, I added several cyclical references to the test swagger documents:

  1. Pet.parent references #/definitions/Pet (a self-reference cycle)
  2. The Pet.owner references #/definitions/User, and User.favorite references #/definitions/Pet, creating a cycle between the two.

All tests as written still pass, and upon examination of the swagger, you can see that there are fields from User showing up in the changes to Pets and vice versa, eg:

What's Changed


PUT /pet Update an existing pet
Parameters

    Insert body.newFeild //a feild demo by sayi
    Insert body.category.newCatFeild
    Insert body.owner.newUserFeild //a new user feild demo
    Delete body.category.name
    Delete body.owner.phone
    Modify body.name

This should address the issue found in #13

emm035 added some commits Sep 10, 2018

Rendering and simplification
- Inserted/removed models don't have their structure diffed anymore, in
  order to prevent massive amounts of output from adding just one prop
  to a given model
- Fixed some rendering so it's consistent across parameters and return
  types
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 17, 2018

Pull Request Test Coverage Report for Build 56

  • 65 of 69 (94.2%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 92.598%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/java/com/deepoove/swagger/diff/compare/ModelDiff.java 22 24 91.67%
src/main/java/com/deepoove/swagger/diff/compare/PropertyDiff.java 3 5 60.0%
Totals Coverage Status
Change from base Build 47: 0.9%
Covered Lines: 688
Relevant Lines: 743

💛 - Coveralls

coveralls commented Sep 17, 2018

Pull Request Test Coverage Report for Build 56

  • 65 of 69 (94.2%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 92.598%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/java/com/deepoove/swagger/diff/compare/ModelDiff.java 22 24 91.67%
src/main/java/com/deepoove/swagger/diff/compare/PropertyDiff.java 3 5 60.0%
Totals Coverage Status
Change from base Build 47: 0.9%
Covered Lines: 688
Relevant Lines: 743

💛 - Coveralls
@sharokh-gamebasics

This comment has been minimized.

Show comment
Hide comment
@sharokh-gamebasics

sharokh-gamebasics Oct 2, 2018

Can this get merged please?

sharokh-gamebasics commented Oct 2, 2018

Can this get merged please?

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