Skip to content

fix(resolver): generate allOf $ref patches before merging members - #4281

Open
htgylzhq wants to merge 1 commit into
swagger-api:mainfrom
htgylzhq:fix/all-of-ref-patches-array-merge
Open

htgylzhq wants to merge 1 commit into
swagger-api:mainfrom
htgylzhq:fix/all-of-ref-patches-array-merge

Conversation

@htgylzhq

Copy link
Copy Markdown

Description

The allOf plugin absolutifies $ref/$$ref values via generateAbsoluteRefPatches. Those patches were generated after the mergeDeep patch for each member, using paths relative to that member, with allOf dropped from the path.

deepmerge concatenates arrays, so a member's array keyword does not stay where it was: once an earlier member has already contributed to oneOf, the second member's oneOf/0 lives at oneOf/2 in the merged parent. The patch then targets a branch that has no such node, and applying it throws:

Resolver error
Cannot read properties of undefined (reading 'items')

This PR generates those patches up front instead, while every member is still in place at allOf/<i>, so their paths cannot go stale. Nothing about the merge itself changes.

Two notes for reviewers:

  • The patches cannot be replaced by resolving the values inline before merging. I tried that first: every allOf test still passed, but test/resolver/specmap/complex.js then ran without finishing (~12s before, killed at 180s). Applying these patches also re-runs the refs plugin over each $ref, which is what keeps the cycle detection in refs.js fed; without it, recursive schemas expand without bound. There is a comment in the code about this.
  • String(i) is deliberate. A numeric path token throws token.replace is not a function in escapeJsonPointerToken(), since specmap paths are strings everywhere else.

helpers.js is unchanged: the default getBaseUrlForNodePath already resolves the member-relative base URL correctly.

Motivation and Context

Fixes swagger-api/swagger-ui#11018

The reproduction in that issue uses an external Path Item pointing at an external schema, but the external files are not required — a single-document spec with local $refs inside allOf members reproduces it just as well. The added unit tests cover both.

How Has This Been Tested?

  • npm run test:unit: 91 suites / 1072 tests pass in ~27s (1070 before, +2 new tests). npm run lint clean.
  • Two regression tests added to test/resolver/specmap/all-of.js: a minimal local-$ref case, and one mirroring the issue's external Path Item to external schema chain, mocked with undici.MockAgent.
  • Confirmed both new tests fail on main with exactly the error from the issue.
  • End to end against Swagger UI: built es/, dropped it into a Swagger UI checkout, and ran a new Cypress spec with the issue's three documents. It fails on swagger-client 3.38.2 and passes with this change; 14 other specs covering allOf, circular refs and external file refs stay green.

Screenshots (if appropriate):

n/a

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

🤖 Generated with Claude Code

@htgylzhq

Copy link
Copy Markdown
Author

One note on the contribution guide: it asks for a bug/ branch prefix, and this branch is fix/all-of-ref-patches-array-merge. I went with fix/ to match what the repository actually uses — recent branches here are fix/, chore/ and ci/. Happy to re-open from a bug/ branch if you'd rather have the guide followed to the letter.

The rest of the guide is covered: single atomic commit, Conventional Commits, 65-character imperative subject, no closing keywords in the commit message (only in this PR's body), and tests for the change.

The allOf plugin generated the patches that absolutify $ref/$$ref values
after pushing the mergeDeep patch, using paths relative to the allOf
member. deepmerge concatenates arrays, so a member's `oneOf/0` ends up at
`oneOf/2` once an earlier member has already contributed to `oneOf`, and
the patch then targets a branch that has no such node:

    Resolver error
    Cannot read properties of undefined (reading 'items')

Generate these patches up front instead, while every member is still in
place at `allOf/<i>`, so their paths cannot go stale. The refs plugin
strips `allOf/<n>/` from the paths it records, so cycle detection keeps
working exactly as before.

Refs swagger-api/swagger-ui#11018

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@htgylzhq
htgylzhq force-pushed the fix/all-of-ref-patches-array-merge branch from 15d779b to bf53c86 Compare September 18, 2026 04:42
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.

[Bug]: Resolver error for array item refs in nested external OpenAPI documents

1 participant