Skip to content

refactor(downgrader): rewrite converters on one table-driven engine - #26

Merged
dinwwwh merged 2 commits into
mainfrom
claude/downgrader-rewrite-e3deff
Sep 28, 2026
Merged

dinwwwh merged 2 commits into
mainfrom
claude/downgrader-rewrite-e3deff

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Rewrites @openapi-spec/downgrader on one small table-driven engine. 3.1 → 3.0 no longer makes a schema stricter when it drops keywords under not or oneOf, and both converters inline or remove more $refs that used to dangle. Speed stays within 7% of main.

Fixes

  • 3.1 → 3.0 removes a not whose operand lost a keyword and turns a oneOf with such a branch into anyOf. For example, not: { prefixItems: … } used to become not: {}, which rejects everything.
  • 3.1 → 3.0 inlines $refs into $defs and other dropped or moved schema parts.
  • 3.2 → 3.1 inlines Path Item $refs into callbacks of query and additionalOperations operations.
  • Both converters remove Links and discriminator mapping entries that point into removed parts, such as a query operation or $defs.
  • 3.2 → 3.1 removes parameter and header examples beside content, which failed 3.1 validation. It also converts xml.nodeType and removes discriminator.defaultMapping, including in downgradeSchemaV32ToV31, which used to only clone.

Behavior changes

  • 3.1 → 3.0 removes a Link whose operationRef points into webhooks or components.pathItems even when that operation is inlined into paths. Main rewrote it to an operationId.
  • 3.1 → 3.0 no longer lets an inlined Reference Object's summary and description override the target's.
  • A $ref loop through a removed part is left as written, so it dangles. On main, 3.2 → 3.1 deleted it and 3.1 → 3.0 rewrote it into a self-reference.
  • 3.1 → 3.0 turns a recursive oneOf into anyOf and removes a recursive not in dereferenced input with object cycles, and nests a malformed allOf instead of keeping or dropping it.
  • A few recursive references are cut to {} one level earlier or later.

Performance

On a 500-path document, 3.2 → 3.1 is slightly faster, and 3.1 → 3.0 is up to 7% slower because it now tracks loosened schemas.

Testing

  • 484 tests pass with 100% coverage (438 on main). Relative to main, the operationFields test is removed with its helper, 6 pointer tests are merged into 4, and about 25 expectations change for the behavior above.
  • Corpus tests also check that no reference that resolved in the input dangles in the output.

Both converters are now field tables that convert or drop each field,
plus one rule that inlines any local $ref the conversion would leave
dangling. A 3.1 to 3.0 schema never rejects a value its source accepts:
`not` over a loosened operand is removed and `oneOf` with a loosened
branch becomes `anyOf`. References into every removed or renumbered part
are inlined, and 3.2 to 3.1 output validates as 3.1.
@codecov

codecov Bot commented Sep 28, 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.

ℹ️ Two documentation-precision suggestions — no code issues found. The engine rewrite and both converters check out.

Reviewed changes

Read the entire diff end-to-end, ran the suite, typecheck, and lint, and probed the two load-bearing invariants (loosening propagation and reference inlining) with specialists.

  • One table-driven engine — shared.ts replaces convertRecord/deepClone/mapRecord/mapArray/convertInlined with defineFields + convertObject(value, ctx, fields, finish), plus a fixpoint downgrade() driver that re-runs while danglesIn finds references whose targets the conversion removed or shifted.
  • Loosening tracking — LOOSE/isLoose/hasLoose/isLooseSchema remove a not whose operand was loosened and rewrite a loosened oneOf to anyOf, so dropping 2020-12 keywords no longer tightens (v3.1-to-v3.0.ts:187-315).
  • Expanded reference inlining — references into $defs, components.mediaTypes, query operations, and shifted parameter lists are now inlined, not only those into webhooks/components.pathItems.
  • 3.2 → 3.1 schema conversion — xml.nodeType → attribute/wrapped, discriminator.defaultMapping removed, and parameter/header example/examples beside content removed; downgradeSchemaV32ToV31 is no longer a plain clone.
  • Documented behavior changes — Links into removed parts are removed rather than rewritten to an operationId; inlined Reference Object summary/description dropped; reference loops left as written.
  • Tests — 483 pass with a new expectNoNewDanglingRefs corpus assertion. I verified pnpm test, pnpm type:check, and pnpm lint all pass, and that no stale references to the removed exports remain.

Both specialist probes came back clean for in-scope behavior: the for(;;) loops in mergeRef and downgrade are structurally bounded (cyclic refs are gated out by aliasEnd → isInlinable, and dangling grows monotonically), and reference-free schemas propagate looseness exhaustively.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

Comment thread packages/downgrader/README.md Outdated
Comment thread packages/downgrader/README.md Outdated
…k path items in components

Rewrites the README contract, References, and Known limitations sections
so every claim matches the converters, and moves Known limitations under
its own heading. A Path Item $ref into a removed operation inside a
components.callbacks entry is now inlined instead of left dangling.

@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

Incremental review of 0142bc5 since the prior pullfrog review at 516cab6. Read the range-diff and the full PR context; ran pnpm test (484 pass), pnpm type:check, and pnpm lint (all clean).

  • Callback Path Item recognition — isPathItemPointer now recognizes #/components/callbacks/{cb}/{expr} (the 4-token case) as a Path Item, so a Path Item $ref into a callback of a removed query/additionalOperations operation is inlined instead of left dangling. The relaxed tokens.length > 3 plus the explicit first === 'components' branch only adds that one case; the x- exclusion still holds.
  • Test coverage — the new v3.2-to-v3.1.test.ts case exercises the 4-token branch, recursion through a query operation, and the x--prefixed callback key staying untouched. It fails against the previous code, so it is real coverage.
  • README rewrite — tightened wording and reorganized the contract, References, and Known limitations sections. Both prior doc threads are addressed: the "adds no dangling references" guarantee now defers to References (which lists looping $ref chains), and the not/oneOf limitation now says "reaches a loosened schema through a $ref kept in the output".
  • New documentation claims checked — the added "except as additionalProperties" exception matches SCHEMA_FIELDS.additionalProperties keeping booleans as-is, and the content-map $ref removal wording matches convertContentEntry's inline/skipAliases behavior.

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

@dinwwwh
dinwwwh merged commit 5b9d4e9 into main Sep 28, 2026
7 checks passed
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