Fix cycles causing stack overflows

egoodhall commented 6 years ago

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:

Pet.parent references #/definitions/Pet (a self-reference cycle)
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

coveralls commented 6 years ago

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%
<!--	Total:	65	69	94.2%	-->

Totals
Change from base Build 47:	0.9%
Covered Lines:	688
Relevant Lines:	743

💛 - Coveralls

ghost commented 6 years ago

Can this get merged please?

Sayi / swagger-diff