Skip to content
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(references): fixing circular reference errors #607

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

dbanksdesign
Copy link
Member

@dbanksdesign dbanksdesign commented Apr 20, 2021

Issue #, if available: #608

Description of changes: Due to the changes we made with for transitive transforms, the code that checks for a circular reference was breaking because it was deferring resolution and therefore created an infinite loop of trying to resolve references but deferring some. This change fixes that by keeping track of how many deferred properties there are and if that number does not decrease with a pass then there is a circular reference. We then try to resolve the object one more time, without deferring any references so that the resolveObject method can record the circular references.

This also adds some integration testing for console output and doing the circular reference check in a real-world integration test rather than a unit test. This is because the unit test for resolveObject was working correctly with circular references, but when they happened in a real world case it broke.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Collaborator

@chazzmoney chazzmoney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Good catch :)

(Also, I love all the hard work you've put into the integration tests)

@dbanksdesign dbanksdesign merged commit 9af17f4 into 3.0 Apr 26, 2021
@dbanksdesign dbanksdesign deleted the fix-circular-references branch April 26, 2021 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants