fix: keep sibling keywords when a referenced schema has a root-level $ref - #2965
fix: keep sibling keywords when a referenced schema has a root-level $ref#2965RomanHotsiy wants to merge 21 commits into
Conversation
🦋 Changeset detectedLatest commit: b704b0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Performance Benchmark (Lower is Faster)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…opping resolution
…ocly/openapi-cli into fix/bundle-root-ref-sibling-keywords
Nice performance gain. Do you know the reason? |
|
Verified locally and bisected it — the gain is real and comes from one commit: 094ff48, the sibling-collision fix in 🤖 Addressed by Claude Code |
…alking from resolved children
…ocly/openapi-cli into fix/bundle-root-ref-sibling-keywords
| | Record<string, any>; | ||
|
|
||
| // A composed $ref (with sibling keys) the resolution chased through on the way to `node`. | ||
| export type ResolvedChainHop = ResolvedRefChainHop; |
There was a problem hiding this comment.
Why do we need new type? Let's get rid of it.
There was a problem hiding this comment.
agree (to trigger the update)
| continue; | ||
| } | ||
| if (nodeIsRef && resolvedChain?.length) { | ||
| // the target composes another $ref: the hop is walked as a ref node from its own |
There was a problem hiding this comment.
make those comments simpler to understand
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6fbd859. Configure here.
|
Okay, now it's ready to reveiw. |
vadyvas
left a comment
vadyvas
left a comment
There was a problem hiding this comment.
One stylistic thing, non-blocking: the new code more comments than these files usually are, and a few of the comments are hard to read in themselves
| return true; | ||
| if (isRef(node)) { | ||
| const resolved = ctx.resolve(node, rootLocation.absolutePointer); | ||
| const effectiveLocation = resolved.chain?.[0]?.location ?? resolved.location; |
There was a problem hiding this comment.
i think creating a utility or helper could improve readability in several places
const effectiveTarget = (resolved: ResolveResult<NonUndefined>): ComponentTarget =>
resolved.chain?.[0] ?? resolved;| } = resolve(node); | ||
| const enteredContexts: Set<VisitorLevelContext> = new Set(); | ||
|
|
||
| if (nodeIsRef && Object.keys(node).length > 1) { |
There was a problem hiding this comment.
i think creating a util in ref-utils.ts could improve readability
export function isRefWithSiblings(node: unknown): node is OasRef & Record<string, unknown> {
return isRef(node) && Object.keys(node).length > 1;
}| }); | ||
|
|
||
| expect(problems).toHaveLength(0); | ||
| expect((res.parsed as any).components.schemas.BadRequest).toEqual({ |
There was a problem hiding this comment.
wouldn't it be better to use snapshot check?
| // by dropping the $ref and keeping the sibling keys | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| delete ref.$ref; |
There was a problem hiding this comment.
wdyt
export function replaceRef(ref: OasRef, resolved: ResolveResult<any>, ctx: UserContext) {
if (!isPlainObject(resolved.node) && ctx.parent) {
ctx.parent[ctx.key] = resolved.node;
return;
}
delete (ref as Partial<OasRef>).$ref;
if (isPlainObject(resolved.node)) {
Object.assign(ref, { ...resolved.node, ...ref });
}
}| | Record<string, any>; | ||
|
|
||
| // A composed $ref (with sibling keys) the resolution chased through on the way to `node`. | ||
| export type ResolvedChainHop = ResolvedRefChainHop; |
There was a problem hiding this comment.
agree (to trigger the update)
6 participants
What/Why/How?
bundledropped the sibling keywords of a root-level$refwhen the referenced schema itself started with a$ref— for example an external schema file that composes another file ($ref: ./BaseProblem.yamlplustitle/type/properties/required).The bundled output collapsed such schemas to a bare
$refpointing at the end of the chain, silently losing the sibling constraints.In OpenAPI 3.1, Schema Objects follow JSON Schema draft 2020-12, where
$refwith siblings is a composition, not a transparent alias.Root cause:
followRefinresolve.tschased every chained$refto the end of the chain, discarding the intermediate nodes together with their sibling keys.The fix — resolution results now carry the composition as an additive
chainfield, soctx.resolve()keeps returning the chain-end node exactly as before and nothing changes for existing rules, decorators, or plugins:resolve.ts—followRefstill chases to the end of the chain, but records every composed$refit passes through as achainhop (ResolvedRefChainHop) on the resolve result.walk.ts— the walker walks each chain hop as a ref node from its own location, so hop siblings and their inner$refs are processed;ResolveResultexposes the chain (withLocations) to visitors.bundle-visitor.ts— a chained ref is rewritten to point at the first composed hop (saved as a component with siblings intact) instead of the chain end; the hop's own$refis rewritten by the walker pass. Discriminator mappings and component-name dedup use the same effective target.no-required-schema-properties-undefined.ts— the one rule that needs the composition: it validatesrequiredon chain hops (they are never visited as Schema nodes) and follows chains and siblingproperties/allOf/anyOf/oneOfwhen looking up required properties.rules/utils.ts—resolveSchemapasses the chain through for chain-aware rules.Because
resolve()behavior is unchanged for consumers, the rule regressions found on the earlier revision of this PR (array-parameter-serialization,struct, discriminator checks) cannot occur — verified explicitly for each.The
remove-unused-componentsdecorators are chain-safe by construction (they build their usage graph from raw$refpointers), covered by a new test.Composed schemas are now preserved as components with their sibling keywords intact, references keep pointing at the composed schema, and
--dereferencedmerges the composition in chain order.Reference
Fixes #2964
Testing
--remove-unused-components, a resolver test asserting the recorded chain, and regression tests forstruct,no-required-schema-properties-undefined(sibling-defined, chain-defined, allOf-sibling-defined, and genuinely-missing required properties), andno-unused-components.sibling-refs-root-external-file(fails on unfixed code) andsibling-refs-root-in-file.bundle,bundle --dereferenced,linton the issue repro, and the rule-regression repros from the review.Check yourself
Security
🤖 Generated with Claude Code
Note
Medium Risk
Changes core ref resolution, document walking, and bundling behavior used by lint and CLI; risk is mitigated by additive
chainAPI, broad tests, and unchanged chain-endnodefor most consumers.Overview
Fixes
bundledropping sibling schema keywords (title,properties,required, etc.) when a resolved schema is itself a$refplus siblings (OpenAPI 3.1 / JSON Schema 2020-12 composition), including multi-hop and external-file cases.Resolution still follows refs to the chain end, but now records each composed hop on an additive
chainfield (ResolvedRefChainHop).walk.tswalks those hops and$refsiblings from the correct locations;bundle-visitor.tssaves/rewrites components using the first composed hop (so siblings survive) and applies the same logic to discriminator mappings;replaceRefhandles dereference when a hop has no parent.Lint:
no-required-schema-properties-undefinedvalidatesrequiredon composed refs and chain hops;resolveSchemaexposeschain. Unit, rule, and e2e bundle tests cover collisions, null targets, unused components, and--dereferenced.Reviewed by Cursor Bugbot for commit b704b0b. Bugbot is set up for automated code reviews on this repo. Configure here.