Skip to content

fix(downgrader): convert shared schemas once in dereferenced documents - #23

Merged
dinwwwh merged 3 commits into
mainfrom
claude/conversion-performance-fix-6ba180
Sep 27, 2026
Merged

dinwwwh merged 3 commits into
mainfrom
claude/conversion-performance-fix-6ba180

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 27, 2026

Copy link
Copy Markdown
Member

Both downgraders now convert a schema that is shared in the input only once per call, so the output of a $ref dereferencer no longer blows up. Before, a shared schema was converted again for every path that reached it. A depth-22 schema diamond took 10.7 s and 750 MB going from 3.1 to 3.0. It now takes 1 ms. The README already said dereferenced documents were supported. This makes that true.

Fixes

  • 3.1 → 3.0 reuses a converted schema wherever the same source object appears. The output stays a new object that never shares anything with the input.
  • 3.2 → 3.1 copies a shared subtree once per call instead of once for every place it appears.
  • A converted result is reused only for the same field table and finish function. Converters whose result depends on where the object sits, such as xml under an array schema, are still converted separately.
  • Objects that were shared in the input can now be shared in the result too. The README says so under "Never mutates".

Performance

Input Before After
Schema diamond, depth 22 (3.1 → 3.0) 10.7 s, 12.6M output objects 1 ms, 50 objects
40 schemas, each referencing the next two (3.1 → 3.0) out of memory 2 ms
500 dereferenced schemas used by 2,000 operations (3.2 → 3.1) 8.7 s 61 ms
Ordinary $ref document, 2,000 operations ~29 ms ~40 ms

The cost on ordinary documents is extra garbage collection from the per-call cache. The fastest runs take the same time as before. Four alternatives were measured and none of them removed it.

Testing

  • New unit tests cover reuse only for the same table and finish, a fresh result on every call, and clearing both caches after a converter throws. Six deliberate breakages of the cache (mutation tests) each make the suite fail.
  • New end-to-end tests cover a depth-64 schema diamond, which ran out of memory before, and a schema shared between components and a response in both converters.
  • 368 tests pass with 100% coverage of packages/downgrader/src. pnpm lint and pnpm type:check pass.

Known limits

Nested, shared callbacks and headers → content → encoding → headers chains are still converted once per path. Their field tables are built on each call, so they are never reused. Real documents don't nest these deeply.

Dereferenced documents no longer blow up: a record reached along several
paths is converted once per call and its copies are shared, so a depth-22
schema diamond converts in 1 ms instead of 10.7 s and 3.2 to 3.1 on a
dereferenced document drops from 8.7 s to 61 ms.
# Conflicts:
#	packages/downgrader/src/v3.1-to-v3.0.test.ts
…gaps

Drop the depth counter in favour of the conversions map size, and add
tests that fail when reuse ignores the finish function, when deepClone
shares copies outside a conversion, or when either cache survives a
throw.
@codecov

codecov Bot commented Sep 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

Reviewed all three commits on claude/conversion-performance-fix-6ba180 against main.

  • Per-call conversion cache (shared.ts) — convertRecord now reuses a finished result for a source object only when fields and finish are the same references; in-progress entries still terminate cycles. deepClone borrows a shared clones map for the duration of a call. Both module-level maps are cleared by the outermost convertRecord frame, including on throw.
  • Shared output — because v3.1-to-v3.0.ts drives every schema through the stable SCHEMA_FIELDS/finishSchema pair, a schema reached from multiple paths yields one converted object. Context-dependent converters (xml) allocate fresh field/finish closures per call and so are never reused. v3.2-to-v3.1.ts builds every field table inline, so it reuses nothing from the conversion cache — its win is entirely shared deepClone output.
  • Tests — unit tests pin reuse-per-call, fresh-result-per-call, the same-table/same-finish gate, and cache clearing after a throw; e2e tests pin a shared schema across components + a response and a depth-64 diamond in both converters.
  • README — the Never mutates bullet is updated to note that objects shared in the input may stay shared in the result.

I traced the reuse condition against every field converter in both modules and could not construct an input where a context-dependent result is wrongly reused, and verified the module-level state cannot be left uncleared via the public API. pnpm test (368 passing), pnpm run lint, and pnpm run type:check are green.

ℹ️ Output is no longer guaranteed to be an unshared tree

The converted document can now contain the same object reference at multiple positions (e.g. a dereferenced schema under components and inside a response), which is the intended fix and is documented in the README. Worth keeping in mind for any downstream consumer that post-processes the converted document by mutating it in place, since edits to one position would now be visible at the others.

Technical details
# Output reference sharing is a documented behavior change

## Affected sites
- `packages/downgrader/src/shared.ts:84` — `deepClone` reuses the per-call `clones` map, so a
  repeated source object yields the same clone.
- `packages/downgrader/src/shared.ts:92` — finished results are reused for the same
  `(fields, finish)` reference pair.
- `packages/downgrader/src/v3.1-to-v3.0.ts:190` — module-level `SCHEMA_FIELDS`/`finishSchema`
  make every schema cache-eligible.
- `packages/downgrader/README.md:26` — documents the loosened guarantee.

## Required outcome
- None required for this PR. This entry exists only to make the semantic change explicit for
  release notes / downstream maintainers.

## Suggested approach (optional)
- Mention the shared-output behavior in the release notes for the next published version.

Pullfrog  | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh
dinwwwh merged commit 2a22d31 into main Sep 27, 2026
7 checks passed
dinwwwh added a commit that referenced this pull request Sep 27, 2026
3.2 → 3.1 no longer leaves `$ref`s pointing at parts it removes. A
reference into `components.mediaTypes`, a `query` or
`additionalOperations` operation, a removed parameter, or a moved
`itemSchema` is now replaced by its converted target, so the output
validates as 3.1. Documents without such references convert exactly as
before.

## Fixes

- Reference Objects and Schema Object `$ref`s into removed parts are
inlined in converted form, following reference chains.
`#/components/mediaTypes/Pet/schema` becomes the Pet schema; beside
other schema keywords the target is appended to `allOf`.
- A ref loop through a removed part is cut by dropping the reference,
instead of producing a circular object that `JSON.stringify` cannot
serialize.
- References into a parameter list that lost a `querystring` entry no
longer point at the wrong parameter.
- References that resolve to a removed parameter or header through any
pointer are removed, not only aliases inside `components`.
- Content-map `$ref`s resolve any local media type, including escaped
names such as `a~1b`.
- Shared schemas in dereferenced documents are still converted once
(#23), in both passes.

## Performance

| Document (200 schemas, 100 paths) | main | this PR |
| --- | --- | --- |
| No dangling refs | 2.57 ms | 3.28 ms |
| One dangling ref | 2.58 ms | 6.67 ms |

Every distinct `$ref` is checked against the first-pass result, and
schemas are walked rather than copied. A second pass runs only when
something dangles.

## Testing

- 400 tests pass with 100% coverage of `packages/downgrader/src`; lint
and type-check pass.
- An end-to-end 3.2 document with refs into `mediaTypes`, `query`,
`additionalOperations` and a shifted parameter index validates as 3.1
and, chained, as 3.0.
- Both routes into the ref-cycle bug have regression tests that fail
with the old guard; a depth-40 shared schema diamond converts once with
a dangling ref present.

## Known limits

- A recursive schema defined only under a removed part keeps one level
and its repeat becomes `{}`, since 3.1 has nowhere to keep it without
inventing a component name. Where the cut lands can depend on key order.
- Link `operationRef` and discriminator `mapping` values pointing into
removed parts pass through unchanged; the README lists them.
- 3.1 → 3.0 has the same class of problem (refs into dropped `$defs`
dangle). That is a separate change.
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.

1 participant