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: a parameter in the rolodex should be evaluated using the index that the node is in #336

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

ThomasRooney
Copy link
Contributor

@ThomasRooney ThomasRooney commented Oct 4, 2024

This fixes vacuum rules that lookup operations in the index where:

  1. The original spec (root.yaml) has an external reference to a file in a different directory ./paths/mypath.yaml
  2. In that location, there is a relative reference to a path-item in a nearby file, e.g. ./components.yaml#/path-item

What was previously happening in [2] is that the ./components.yaml was being resolved in ./components.yaml (the parent directory) instead of ./paths/components.yaml (the right directory). This resulted in a failure to evaluate path item references and unexpected linting failures.

This changes adjusts the lookup to search through the index's nodeMap to find the location of the YAML $ref node value that's been evaluated. Once we find it, we lookup the associated index of that file and do a seek in that specific index.

We also fix a race condition with resolving the specification asyncronously -- if we are going to an external ref we must do it sequentially as otherwise an index might be added multiple times during an async FindComponent call. This latent bug was exposed as we tested this.

@TristanSpeakEasy
Copy link
Contributor

@ThomasRooney can you add a test to validate the correct behaviour

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (4d074c3) to head (f3ffd1a).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
- Coverage   99.65%   99.64%   -0.02%     
==========================================
  Files         164      164              
  Lines       16674    16697      +23     
==========================================
+ Hits        16616    16637      +21     
- Misses         53       55       +2     
  Partials        5        5              
Flag Coverage Δ
unittests 99.64% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThomasRooney
Copy link
Contributor Author

Test added. Just fighting a "line count" snapshot test.

Copy link
Member

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

I am going to test this locally first before I approve, because it looks solid.

If you have fixed this race condition bug, I will owe you a bottle of whiskey.

@daveshanley
Copy link
Member

I tested it, seems to work fine - however the bug is still there, when setting ExtractRefsSequentially: false there are still a bunch of un-bundled references.

So, the hunt still continues, but this LGTM.

@daveshanley daveshanley merged commit 4b93cff into pb33f:main Oct 8, 2024
3 of 4 checks passed
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.

3 participants