Add support for circular references

Issue #6 open
Ben Sayers created an issue

At the moment it is not clear when the team will have time to implement this feature, however any updates on progress will be made here.

Comments (13)

  1. Ryan Molnar Account Deactivated

    Is there anything specific stopping this PR from being merged? I’d love to diff two specs w/ circular dependencies ❤

  2. Former user Account Deleted

    @Hidde Wieringa Hello! I agree with the above comments and would love to see this PR merged! This would be a huge help. Thanks!

  3. Hidde Wieringa

    I would love to see the PR merged as well. But there are no reviews, and there are no comments from the library maintainers.

  4. Ben Sayers reporter

    We made an attempt at supporting circular references in json schema diff (the underlying tool that finds differences in the request/response bodies), you can see the attempt here:
    https://bitbucket.org/atlassian/json-schema-diff/branch/circular-ref#diff

    It works for some simple cases but doesn’t work for more complex ones so isn’t ready to be used. This is currently a side project so it isn’t being worked on very often, expect progress to be slow. If someone wants to try and carry on with the work you are very welcome to, although I warn you it’s a very complex undertaking which is why progress on this has been so slow.

    The specific circles it works for can be seen in the tests:
    https://bitbucket.org/atlassian/json-schema-diff/branch/circular-ref#chg-test/unit/json-schema-diff/diff-schemas-references.spec.ts

    I had a look at the PR, it doesn’t follow our coding standards as it has no tests. I also tried it quickly by attempting to diff a swagger containing a circular reference in a response body with itself and still got an error, so I don’t even think it works. But more importantly it doesn’t add support for circular references, it tries to modify the schema by removing the circles before diffing. This strategy will cause the tool stop failing but it won’t make it support circular references, it will make it ignore them.

    The PR tries to make the case that ignoring them in this way will make the tool calculate the diff one level deep then stop. This might be true when a circle is in exactly the same place in both specs, but does not work when the circle moves around or is introduced or removed for the first time. In these cases the tool will misbehave and give you false positive or false negative results, so I don’t think we can use this approach.

    I’m currently of the opinion that you are all better off being blocked from using the tool instead of having it give you incorrect results that give you the impression your change is safe to make when it is not. But I’m open to discussing this if there is disagreement on that.

  5. Hidde Wieringa

    Thank you for the reply. I agree, solving the real problem of circular references is difficult.

    The pull request was mostly meant as a workaround to get ‘some’ result. I will close the pull request because I understand you do want to solve the problem fully.

  6. Log in to comment