-
Notifications
You must be signed in to change notification settings - Fork 30
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(oas): account for deep $ref
pointers when reducing an API def
#926
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smol comment below, potentially outside the scope of this PR
} | ||
} | ||
|
||
console.logx = (obj: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol wut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this snuck in in a commit i made this morning where i added that skipped failing test. console.logx
is a tool i use to dump full objects.
if (!$refs.has(`#/components/${componentType}/${component}`)) { | ||
// If our `$ref` either is a full, or deep match, then we should preserve it. | ||
const refIsUsed = | ||
$refs.has(`#/components/${componentType}/${component}`) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non-blocking) i feel like i've seen ref syntax that uses #components
and not #/components
— is that valid? should we account for that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have also seen that before but it is not valid. our parser doesn't pick up on it now, but should at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had a note about this in one of my gitto PRs over the past two weeks but can't link those here.
@@ -39,6 +39,12 @@ function accumulateUsedRefs(schema: Record<string, unknown>, $refs: Set<string>, | |||
} | |||
|
|||
getUsedRefs($refSchema).forEach(({ value: currRef }) => { | |||
// Because it's possible to have a parameter named `$ref`, which our lookup would pick up as a | |||
// false positive, we want to exclude that from `$ref` matching as it's not really a reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course you can do that 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we love it here, folks
🧰 Changes
This updates
oas/reducer
to account for deep$ref
pointers like#/components/examples/event-min/value
. Normally when we're running through components to remove we only look at#/components/examples/event-min
, however when a schema like this is deeply referenced we won't pick up that it's used and end up removingevent-min
from the components block, resulting in a corrupted schema that will no longer validate.