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: handle multi-file spec circular reference edge case #333

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

dpopp07
Copy link
Member

@dpopp07 dpopp07 commented Sep 27, 2021

The validator chokes on a specific scenario - when a multi-file spec
has schemas that reference each other in a ciruclar manner across files.
The bundler we use to put together mutli-file specs doesn't store the
reference in a helpful way, so there's not really a way to report the
location of the circular reference the way we do for other scenarios.
All we can really do is avoid crashing completely and send the user the
fully resolved path to the problematic schema that they will need to
trace manually.

A test is added to capture the behavior. This test case would exhibit the
crash without the fix.

Resolves #326

@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #333 (1ee7897) into main (8210134) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #333      +/-   ##
==========================================
+ Coverage   91.90%   91.92%   +0.01%     
==========================================
  Files          73       73              
  Lines        2348     2353       +5     
  Branches      598      600       +2     
==========================================
+ Hits         2158     2163       +5     
  Misses        157      157              
  Partials       33       33              
Impacted Files Coverage Δ
src/cli-validator/utils/circular-references-ibm.js 96.72% <100.00%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8210134...1ee7897. Read the comment docs.

Copy link
Contributor

@barrett-schonefeld barrett-schonefeld left a comment

Choose a reason for hiding this comment

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

Looks good! I see that you plan to add a test, but approving so you can merge once you add the test.

The validator chokes on a specific scenario - when a multi-file spec
has schemas that reference each other in a ciruclar manner across files.
The bundler we use to put together mutli-file specs doesn't store the
reference in a helpful way, so there's not really a way to report the
location of the circular reference the way we do for other scenarios.
All we can really do is avoid crashing completely and send the user the
fully resolved path to the problematic schema that they will need to
trace manually.

A test is added to capture the behavior. This test case would exhibit the
crash without the fix.
@dpopp07 dpopp07 force-pushed the dp/fix-circular-ref-edge-case branch from 1ee7897 to 681ae05 Compare September 29, 2021 17:00
@dpopp07 dpopp07 merged commit 172c027 into main Sep 29, 2021
@dpopp07 dpopp07 deleted the dp/fix-circular-ref-edge-case branch September 29, 2021 17:05
ibm-devx-sdk pushed a commit that referenced this pull request Sep 29, 2021
## [0.50.1](v0.50.0...v0.50.1) (2021-09-29)

### Bug Fixes

* handle multi-file spec circular reference edge case ([#333](#333)) ([172c027](172c027))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 0.50.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read properties of undefined (reading 'get')
4 participants