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

Re-implement lookaside for DFS during resolution #1209

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Re-implement lookaside for DFS during resolution #1209

merged 1 commit into from
Dec 20, 2017

Conversation

dballance
Copy link
Contributor

@dballance dballance commented Dec 19, 2017

Description

cc @shockey - Lets try this again! 😆

In #1190 - the lookaside actually prevented traversal of all properties of a reference - because we marked it as traversed and checked for this traversal before fully iterating the properties in the patch. This means that DFS was never fully complete for a $ref that we marked as traversed. This moves the check for previous traversal out of the loop and allows a complete traversal but prevents future traversals. Should be accurate as far as I can tell.

I believe this was the root of "required" props not being parsed in swagger-api/swagger-ui#4000

Motivation and Context

Followup to #1201 and #1190. Again reduces resolution times to the #1190 range, but with greater accuracy.

How Has This Been Tested?

All tests pass. Tested with samples from swagger-api/swagger-editor#1585 & swagger-api/swagger-ui#4000 - doesn't appear to break those specs.

@shockey Unsure if there needs to be additional tests - so please grab and test as well! Might need to merge tests from #1201?

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@shockey
Copy link
Contributor

shockey commented Dec 20, 2017

Reviewing now - looks like my reversions did drop the tests from #1201, going to resolve that.

Edit: I must have been looking at this wrong locally 😄

@shockey shockey merged commit c4c82c7 into swagger-api:master Dec 20, 2017
@shockey
Copy link
Contributor

shockey commented Dec 20, 2017

Thanks @dballance! 😄

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