Skip to content

fix: apply sibling keywords when a schema resolves a $ref - #867

Open
lazerg wants to merge 5 commits into
fastify:mainfrom
lazerg:fix/ref-sibling-keywords
Open

fix: apply sibling keywords when a schema resolves a $ref#867
lazerg wants to merge 5 commits into
fastify:mainfrom
lazerg:fix/ref-sibling-keywords

Conversation

@lazerg

@lazerg lazerg commented Aug 8, 2026

Copy link
Copy Markdown

Problem

A property that both references another schema and adds its own keywords loses those keywords. resolveRef swaps the whole location for the reference target, so anything sitting next to $ref is gone before any code is generated:

const stringify = build({
  type: 'object',
  properties: {
    x: { $ref: 'Foo#', required: ['extra'] }
  }
}, {
  schema: {
    Foo: {
      $id: 'Foo',
      type: 'object',
      properties: { value: { type: 'integer' }, extra: { type: 'string' } },
      required: ['value']
    }
  }
})

stringify({ x: { value: 1 } })
// ACTUAL:   {"x":{"value":1}}
// EXPECTED: throws '"extra" is required!'

value is enforced because it comes from Foo, extra is not, so incomplete data serializes as if it were valid.

Fix

A $ref that carries siblings now gets the resolved target and those siblings merged through mergeLocations, the same path allOf already takes. Everything else keeps the direct dereference it always had:

  • siblings that cannot change the output (title, description, $comment, examples, deprecated, readOnly, writeOnly) are skipped, so annotated refs still share one serializer
  • when the sibling keywords and the target genuinely conflict (say a sibling type the target contradicts) the merge is dropped and the plain target is used, which is what happens today

Merged refs are cached by content rather than by object identity, since a merge clones its input and a recursive $ref would otherwise be merged again on every level.

Test

Two cases in test/ref.test.js, internal and external $ref with a sibling required: the sibling constraint is enforced and the target's own required still is. Both fail on the current main.

Fixes #866

@lazerg
lazerg force-pushed the fix/ref-sibling-keywords branch from 2b5d0bb to ed8b59a Compare August 8, 2026 05:55
@lazerg

lazerg commented Sep 10, 2026

Copy link
Copy Markdown
Author

Hey @Tony133, could you take a look at this fix for sibling keywords not applying once a schema resolves a $ref? It's been open a month with no feedback.

@Tony133 Tony133 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, and sorry for the delay. The fix looks correct.

Two small changes before merging, see inline comments.

Comment thread index.js Outdated
Comment thread index.js
@Tony133 Tony133 added the semver-minor Issue or PR that should land as semver minor label Sep 11, 2026
@Tony133

Tony133 commented Sep 11, 2026

Copy link
Copy Markdown
Member

CI is red

@lazerg

lazerg commented Sep 11, 2026

Copy link
Copy Markdown
Author

CI failed on the 100% coverage gate, not on a failing test. The catch in resolveRef that falls back to the plain $ref target had no test behind it. I added two: one where a sibling keyword conflicts with the target, one where the merge throws an error that must not be swallowed. Fixed in b19f087.

@Tony133 Tony133 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor Issue or PR that should land as semver minor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sibling required keyword is dropped when a property uses $ref to an external schema

2 participants