fix(openapi): keep a discriminated union variant an object when it is a bare allOf reference - #17577
Conversation
…llOf reference
A variant of a discriminated union that is spelled as an allOf with nothing but the
reference to its base:
ProcessError:
allOf:
- $ref: '#/components/schemas/ApiError'
was short-circuited to an alias of the base, so the fern definition declared
`ProcessError: ApiError` - the variant referred to the union it belongs to. The IR
generator only emits a variant as `samePropertiesAsObject` when the referenced type is an
object, so the variant degraded to `singleProperty` and every generator then expected the
variant's payload under a `value` key that the wire format does not have.
Spelling the same subtype with a trailing empty object:
ProcessError:
allOf:
- $ref: '#/components/schemas/ApiError'
- type: object
never reached the short-circuit and converted to an object all along. The two spellings
are equivalent in OpenAPI, so this brings them into agreement rather than inventing a
behaviour: the fixed output for the bare form is byte for byte what the trailing-empty-
object form already produced, both as a type declaration and as a union variant.
The short-circuit is kept for every other single-reference allOf. The guard fires only
when the referenced schema declares a discriminator whose mapping names the schema being
converted, so `{allOf: [$ref X], nullable: true}` - the common way to attach `nullable` to
a reference - still collapses to a reference to X.
The v3 importer already produced `samePropertiesAsObject` for the bare form, so this also
removes a divergence between the two importers.
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Narrow, well-justified fix to keep bare allOf: [$ref Base] discriminator variants as objects. The main concern is the identity-based comparison in isVariantOfDiscriminatedBase, which is fragile in a couple of ways: it relies on resolveSchemaReference returning the same object instance, and it only compares against schema (the whole converted schema object), which may not hold for inline/nested cases. Also worth confirming the mapping-key vs. $ref-resolution path and the perf of resolving every mapping entry.
- 🟡 1 warning(s)
- 🔵 2 suggestion(s)
To request another review, comment /ai-review on this pull request.
| return Object.values(mapping).some((target) => { | ||
| // A mapping value is either a reference or a bare schema name. | ||
| const $ref = target.startsWith("#/") ? target : `${SCHEMA_REFERENCE_PREFIX}${target}`; | ||
| try { | ||
| // resolveSchemaReference indexes into the parsed document, so the resolved variant | ||
| // and the schema being converted are the same object when they are the same schema. | ||
| return context.resolveSchemaReference({ $ref }) === schema; |
There was a problem hiding this comment.
🟡 warning
Identity comparison (=== schema) is doing a lot of load-bearing work here. It depends on resolveSchemaReference returning the exact same object instance from the parsed document rather than a clone/merged copy, and on schema still being that same instance by the time convertSchemaObject runs (no upstream {...schema} spread, e.g. from maybeInjectDescriptionOrGroupName or nullable/description normalization). If either assumption breaks, the guard silently returns false and the bug returns without any test signal.
A more robust alternative: resolve the base's discriminator.mapping and compare the mapping target $ref against the name of the schema currently being converted (breadcrumbs/wrapAsNullable context already carries it in most call sites), falling back to identity only if the name is unavailable. At minimum, add a comment asserting the no-copy invariant, or a test fixture that exercises the variant with a description on the allOf wrapper (which is a common trigger for schema cloning).
There was a problem hiding this comment.
Addressed in af4faa1 — the invariant now has test signal and a comment, though the audit came out cleaner than the warning suggests.
I traced the copy paths rather than assuming. Inside convertSchemaObject there are exactly two reassignments of schema: { type: schema } when it arrives as a string, and { ...schema, format: "binary" } — and that one lives inside the if (schema.type === "string") block, which returns before any allOf handling is reached. So no path that reaches the guard has copied the schema, and resolveSchemaReference indexes into this.document and returns the stored object rather than a clone or merge.
maybeInjectDescriptionOrGroupName was a good thing to point at, but it operates on the converted result on the way out, not on the input schema, so it cannot affect identity going in.
Took the fixture suggestion, which was the useful half. The fixture gains two more variants carrying description and title alongside the allOf — sibling keys being the plausible trigger for a copy. Both stay properties: {} rather than collapsing to an alias, so identity survives them, and if that ever stops being true the snapshot fails rather than silently reverting. I also wrote the invariant down where the comparison happens.
I did not take the name-based alternative. breadcrumbs is not a reliable identifier here — it is a display path, inline schemas can produce colliding names, and swapping a precise check for a fuzzy one to guard against a copy that does not happen would trade a real property for a heuristic. Identity is exact; when it fails it fails closed, back to today's behaviour.
| const mapping = context.resolveSchemaReference(element).discriminator?.mapping; | ||
| if (mapping == null) { | ||
| return false; | ||
| } | ||
| return Object.values(mapping).some((target) => { |
There was a problem hiding this comment.
🔵 suggestion
This resolves every mapping entry on each call, and it runs on both short-circuit checks for every single-element allOf in the document — O(variants) resolutions per variant, so O(n²) for a union with many members. For large specs with big discriminated unions this could get noticeable. Consider caching the resolved mapping-target set per base $ref on the context, or bailing early once a match is found (the .some already short-circuits, but resolution order means the average is half the mapping).
There was a problem hiding this comment.
Good catch, fixed in af4faa1.
The resolved mapping targets are now memoized per base schema, so a union's variants resolve the mapping once between them instead of once each:
const discriminatedVariantsByBase = new WeakMap<OpenAPIV3.SchemaObject, ReadonlySet<OpenAPIV3.SchemaObject>>();Keyed on the base object rather than its $ref string so two documents parsed in one process cannot collide, and weak so it does not outlive the parse. Membership is then a Set lookup.
On the "bail early" half — .some already short-circuits, but that only helps the matching variant; the non-matching ones still walked the whole mapping. Caching is what actually removes the quadratic term, so I did that instead.
| } | ||
| return Object.values(mapping).some((target) => { | ||
| // A mapping value is either a reference or a bare schema name. | ||
| const $ref = target.startsWith("#/") ? target : `${SCHEMA_REFERENCE_PREFIX}${target}`; |
There was a problem hiding this comment.
🔵 suggestion
target.startsWith("#/") misses two legal mapping forms: an absolute/relative external ref (common.yaml#/components/schemas/Foo) and a URI-ish ref. Those will get mangled into #/components/schemas/common.yaml#/... and then fail to resolve — caught by the catch, so it just falls back to false (i.e. the old buggy behaviour) rather than crashing. Probably fine to leave, but target.includes("#/") would be a cheap widening.
There was a problem hiding this comment.
Looked into this one and the widening would not buy anything, so I left the condition and documented the behaviour instead.
resolveSchemaReference opens with schema.$ref.substring(2) — it assumes the ref starts with #/ and indexes into the current document from there. Hand it common.yaml#/components/schemas/Foo and it splits on the mangled remainder, walks off the document, and returns a sentinel. So target.includes("#/") would pass the external ref through unmangled only to have the resolver mangle it one line later. Both spellings reach the same place.
Also worth correcting one detail: it is not the catch that saves us, because resolveSchemaReference never throws. An unresolvable reference returns a fresh {"x-fern-type": "unknown"} object, and a fresh object is by construction not the schema being converted, so the guard returns false and the short-circuit stays — which is the pre-existing behaviour for that variant, exactly as you say.
That made the try/catch dead code, so af4faa1 removes it and replaces it with a comment stating why an unresolvable target degrades safely. Following external references is a limitation of this resolver generally — every other resolveSchemaReference call in this file shares it — so it is not something this guard can fix locally.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…keys Review follow-ups on the variant guard. Memoize the resolved mapping targets per base schema. The guard runs for every single-element allOf in the document and each of a union's variants asks the same question of the same base, so the resolution work was quadratic in the number of variants. Keyed on the base object rather than its $ref so two documents in one process cannot collide, and weak so it does not outlive the parse. Drop the try/catch around resolveSchemaReference. It never throws: an unresolvable reference returns a fresh `x-fern-type: unknown` sentinel, which by construction is not the schema being converted, so an unresolvable mapping target - a reference into another document, which this resolver cannot follow - already degrades to leaving the short-circuit in place. Say so in a comment instead of catching an error that cannot happen. Record why membership by object identity is sound, and cover it: the fixture gains two more variants carrying `description` and `title` alongside the allOf, since sibling keys are the plausible trigger for a schema being copied before conversion. Both stay objects, so the identity holds.
wiebren
changed the title
|
CI notes, since two checks were red.
These need
Same five: Happy to refresh those For what it is worth, the checks that do exercise this change are green: |
1 participant
Description
Linear ticket: n/a — found while running the same OpenAPI document through all six SDK
generators and comparing what each one did with a discriminated error union.
A variant of a discriminated union that is spelled as an
allOfcontaining nothing but thereference to its base does not deserialize. The generated client expects the variant's
payload nested under a
valuekey that the wire format does not have.Given
{"type": "ProcessError", "message": "The process failed"}, python raisesValidationError: 1 validation error for tagged-union[...], and csharp raisesKeyNotFoundException: The given key was not present in the dictionary.The two spellings disagree
The same subtype written with a trailing empty object has always worked:
These are equivalent in OpenAPI, so the divergence is the bug.
convertSchemas.tsshort-circuits an
allOfthat reduces to a single element and converts that elementdirectly. The bare form reduces to one element and is short-circuited into an alias of
the base; the trailing-
type: objectform has two elements, never reaches theshort-circuit, and converts to an object.
The alias is what breaks it. The fern definition ends up declaring
ProcessError: ApiError— the variant refers to the union it belongs to — and the IR generator only emits a variant
as
samePropertiesAsObjectwhen the referenced type is an object. A non-object variantdegrades to
singleProperty, which is the wrapped shape that puts the payload undervalue:allOf: [$ref, {type: object}]samePropertiesAsObjectallOf: [$ref]singleProperty←This is one importer bug, not six generator bugs
Every generator reads the same broken IR, so all six emit the wrapped shape; they differ only
in how loudly they fail.
Each client was generated twice from the same document — once with the released cli 5.106.1,
once with a locally built cli carrying this fix — and handed
{"type":"BareRef","message":"The process failed"}. Every row below was executed:ValidationError: 1 validation error for tagged-union[...]messagepresentKeyNotFoundException: The given key was not present in the dictionary.IsBareRef=TrueException: JSON data is missing property 'BareRef'messagepresentBaseError{value: null};getBareRef()is null so the accessor NPEsBaseError{value: {"message": "..."}},getBareRef().isPresent()GetBareRef()is nil andAccept(visitor)returnstype *api.BaseError does not define a non-empty union typeTS2322 ... Property 'value' is missing in type ... but required in type 'BareRef'go and typescript are worth calling out because neither validates the response body at
runtime — go leaves the zero value, typescript does no runtime checking at all — so this can
look like it only affects three languages. It does not: go's visitor dispatch still fails, and
typescript rejects the actual wire payload at compile time.
In every language
BareRefis also emitted as an alias of the union (type BareRef = BaseError),so the
valuea caller would have to supply is the union itself.Worth noting the v3 importer (
--from-openapi) already producessamePropertiesAsObjectfor the bare form. This change removes a divergence between the two importers rather than
introducing a new behaviour.
Changes Made
convertSchemas.ts: skip the single-elementallOfshort-circuit when the schema beingconverted is named in the discriminator mapping of the schema it references. Both
short-circuits (before and after the non-object elements are filtered out) are guarded.
discriminator whose mapping names this schema, so the common non-variant use of a
single-reference
allOf—{allOf: [$ref X], nullable: true}, used to attachnullableto a reference — still short-circuits to a reference to
X. An earlier, broader versionof this guard (any reference to a schema with a discriminator) changed the
vellumfixture's
WorkflowResultEvent.outputfromnullable<WorkflowResultEventOutputData>to anew inline type; the mapping check is what keeps that fixture byte-identical.
Testing
New fixture
discriminated-union-variant-bare-allof-refcovers all four shapes in onedocument: a variant that adds properties, the same variant with a trailing empty object, the
bare-
$refvariant, and a non-variant{allOf: [$ref], nullable: true}that must keepshort-circuiting. It is picked up by both
openapi.test.tsandopenapi-ir.test.ts.The snapshot pins both halves:
Verified the new test fails without the fix by stashing only
convertSchemas.ts: 2 failed,317 passed. With the fix, 763 tests pass across
@fern-api/openapi-ir-parser(96),@fern-api/openapi-ir-to-fern-tests(319),@fern-api/v3-importer-tests(336) and@fern-api/openapi-to-ir(12), with no snapshot churn anywhere else. (Two fixtures logTest Failed: Error processing fixture ...inside passing v3 tests; confirmed identicalwithout this change.)
End to end, all six generated clients were built and run against the payload, before and
after, as tabulated above — python under pydantic, csharp on .NET 9, java on JDK 21 with
Jackson, go under
go teston 1.23, php 8.4 under composer, and typescript throughtsc --strict. All six fail before and pass after.The decisive structural check is that the bare form and the trailing-
type: objectform nowproduce byte-identical IR — both the type declaration and the union variant, modulo the
name — so the fixed output is exactly the shape that already worked in every language rather
than a new one.
Notes
unreleaseddirectory beingempty. Happy to add one if you would like it.
singlePropertypath they implement is stillcorrect for variants that genuinely are non-object types.
Generated with Claude Code