fix(parser): resolve nested $ref objects in allOf flattening during v3 OpenAPI parsing#16162
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
E2E Test ResultsTested by creating a minimal OpenAPI fixture matching the BigCommerce Before/After Comparison🔴 On 🟢 On fix branch (FIXED) — The 3 grandparent properties ( Test Assertions
|
Docs Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on
Docs generation runs |
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
…I parsing When a parent schema in an allOf composition itself uses allOf with $ref elements (e.g., userPost → userBase → userStrict), the v3 parser's flattenNestedAllOf was silently dropping those nested $ref objects. This caused parent properties and required arrays to be lost in docs rendering. The fix adds an optional resolveRef callback to mergeAllOfSchemas that flattenNestedAllOf uses to resolve nested $ref objects instead of filtering them out. Cycle detection via a visited set prevents infinite recursion. Co-Authored-By: will.kendall@buildwithfern.com <wpk235@gmail.com>
…le url-reference snapshots Guard against ref chains (local alias → external URL ref) that resolve to another ReferenceObject instead of a schema. The resolver now returns undefined for these, preserving the existing filter-out behavior. Also updates the pre-existing stale url-reference test snapshots (the external spec changed upstream; identical output on main). Co-Authored-By: will.kendall@buildwithfern.com <wpk235@gmail.com>
- Add permanent v3-importer-tests fixture (allof-nested-ref) with 3 test cases: three-level chain, sibling properties + nested ref, multiple refs in single allOf - Add 4 new unit test edge cases: required merging across ref boundaries, multiple refs in single allOf, sibling properties preservation, undefined resolver - Extend existing allof seed test fixture with Cases 7-8 (nested $ref chain, multiple nested refs in parent allOf) - Run seed tests through TS, Python, Java, Go generators - all pass - Total: 26 unit tests, 286 v3-importer-tests, 4 seed generators verified Co-Authored-By: will.kendall@buildwithfern.com <wpk235@gmail.com>
d9b780f to
91a8771
Compare
|
@claude review once |
There was a problem hiding this comment.
This is a well-tested logic fix in the v3 parser's allOf flattening, but it changes core IR generation behavior that flows into every SDK generator — worth a human look before merging.
Extended reasoning...
Overview
This PR fixes a real bug in flattenNestedAllOf where nested $ref objects inside an allOf array were silently filtered out, causing grandparent properties to be lost (the BigCommerce userPost → userBase → userStrict pattern). The fix threads an optional resolveRef callback through mergeAllOfSchemas and flattenNestedAllOf, with cycle detection via a shared visitedRefs set, and SchemaConverter wires up context.resolveMaybeReference with a guard that skips results that remain references (e.g. URL ref aliases). The diff is 79 files but most of that is seed snapshot churn — the actual logic change is small and self-contained in mergeAllOfSchemas.ts and SchemaConverter.ts.
Security risks
No security risks. This is internal parser/IR-generation logic; no auth, crypto, network, or untrusted-input handling is involved. The cycle-detection set bounds recursion correctly, so there's no DoS surface from circular $ref chains either.
Level of scrutiny
This warrants careful human review. mergeAllOfSchemas is core IR-generation code used by every downstream SDK generator (TS/Python/Java/Go/C#/Ruby/PHP/Swift/Rust). The previous "filter $refs" behavior was intentional and defensive — now we're resolving them, which by design changes the IR output for any spec that uses this nesting pattern. The seed snapshot updates (allof fixture, url-reference snapshot regen) are the visible blast radius, but real-world customer specs that happened to rely on the buggy behavior (e.g. duplicating grandparent props inline as a workaround) could see SDK output changes. The author has done good due diligence — 26 unit tests including cycle detection, undefined-resolver fallback, and multi-$ref cases; a dedicated allof-nested-ref fixture exercising three patterns; before/after IR comparison showing 5→8 props recovered on the BigCommerce repro.
Other factors
The optional-callback design preserves backward compat for any other caller of mergeAllOfSchemas (the test filters out $ref objects from nested allOf arrays when no resolver provided locks this in). The isReferenceObject guard in the SchemaConverter callback is a thoughtful touch for URL ref aliases. The maintainer (fern-support) explicitly tagged for review (@claude review once), suggesting they want a second pair of eyes on the behavior change. The bug-hunting pass found nothing, and I don't see correctness issues either — deferring purely on blast-radius and the fact that this is a deliberate behavioral change in shared parser infrastructure.
Description
When a parent schema in an
allOfcomposition itself usesallOfwith$refelements (e.g.,userPost → userBase → userStrict), the v3 parser'sflattenNestedAllOfsilently drops those nested$refobjects. This causes parent properties andrequiredarrays to be lost in docs rendering.Live example: https://docs.bigcommerce.com/developer/api-reference/rest/b2b/management/users/companies-users-create
Changes Made
resolveRefcallback parameter tomergeAllOfSchemasandflattenNestedAllOf$refentries are resolved instead of filtered outvisitedRefsset prevents infinite recursion on circular$refchainsSchemaConverterpasses itscontext.resolveMaybeReferenceas the resolver callback, with a guard that skips results that are still references (e.g. URL ref aliases)Root cause in
mergeAllOfSchemas.tsflattenNestedAllOf():The original filtering was intentional as a defensive measure — merging an unresolved
{"$ref": "..."}into the result would corrupt it with a spurious$refkey. The issue was thatflattenNestedAllOfhad no way to resolve refs; the fix adds that capability via an optional callback while preserving the safe fallback.Note on
url-referencesnapshot diffThe large snapshot deletion (6234 lines) in
url-reference.json/url-reference-baseline.jsonis a pre-existing stale snapshot — not caused by this change. That fixture fetches an external spec fromhttps://raw.githubusercontent.com/fern-api/external-ref-fixture-specs/, and the upstream spec changed after the snapshot was last committed. I verified this by checking outmain, regenerating the snapshot, and diffing — the output is byte-for-byte identical betweenmainand this branch.Testing
allof-nested-ref) with 3 test cases: three-level chain (UserPost → UserBase → UserStrict), sibling properties + nested ref (CompanyPost → CompanyBase → AddressBase), multiple refs in single allOf (PlantRecord → PlantBase → [Identifiable, Describable])alloffixture (nested$refchain, multiple nested refs in parent allOf)main: bug confirmed (5 props on main, 8 on fix branch)biome lint --error-on-warnings,biome format)Link to Devin session: https://app.devin.ai/sessions/52f4bc574f654a6a8c40f70ea454cca6