Prevent stack overflow on recursive schemas - #2543

Confidence Score: 5/5

The PR appears safe to merge.

Summary

The PR prevents unbounded schema-generation recursion by tracking active allOf compositions and resolving recursive re-entry through named Go types.

• Threads generation context through schema, union, array, property, and additional-properties processing.
• Preserves recursive object and union composition shapes rather than dropping referenced fields.
• Adds regression fixtures for direct, transitive, mutual, bystander, and map-based recursion.

_{Reviews (4) · Last reviewed commit: "fix: generate concrete recursive types f..."}

Once ☝🏼 are resolved, I'll take another look

Thanks for digging into this, and sorry for the slow turnaround — I went deep on it and came out wanting to change the direction, so let me lay out what I found. Everything below is verified against e1ba901d; I'd like to push these changes onto this branch rather than open a competing PR, so the work stays yours.

Two shapes the current check misses

The guard is RefPathToObjName(ref) == path[0] && len(path) >= 2 — "the member refs the component we're currently generating". That misses any cycle that doesn't pass through the root component. Both of these still overflow the stack on this branch:

Mutual recursion, A -> B -> C -> B:

A:
  type: object
  properties:
    b: { allOf: [ { $ref: '#/components/schemas/B' }, { type: object, properties: { tag: { type: string } } } ] }
B:
  type: object
  properties:
    c: { allOf: [ { $ref: '#/components/schemas/C' }, { type: object, properties: { x: { type: string } } } ] }
C:
  type: object
  properties:
    back: { allOf: [ { $ref: '#/components/schemas/B' }, { type: object, properties: { y: { type: string } } } ] }

Generating A, the refs we meet are B, C, B, C… — never A, so path[0] never matches.

The cycle reached from a bystander component:

Node:
  anyOf:
    - { type: object, properties: { leaf: { type: string } } }
    - type: object
      properties:
        children:
          type: array
          items:
            allOf:
              - { $ref: '#/components/schemas/Node' }
              - { type: object, properties: { extra: { type: string } } }
Wrapper:
  type: object
  properties:
    n: { allOf: [ { $ref: '#/components/schemas/Node' }, { type: object, properties: { extra: { type: string } } } ] }

Generating Wrapper inlines Node's body, and it's that inlined body that re-enters itself. The cycle is entirely inside Node.

There's also a smaller hole: len(path) >= 2 is meant to mean "not at the component root", but items and additionalProperties reuse the parent's path (schema.go:1489, :1214), so Tree: { additionalProperties: { allOf: [$ref Tree, …] } } is still at len(path) == 1 and slips through.

Worth knowing for the tests: these pass on this branch only because the fixture configs use paths: {} without skip-prune, so the pruner deletes A/Wrapper before generation ever reaches them. Adding output-options: { skip-prune: true } to the repro config is what surfaced all of the above. I'd suggest that for the new fixtures regardless.

The bigger thing: the union isn't the right output

This is the part I'd like to change, and it's not obvious — I had to go measure it.

Both your fix and mine represent the recursive member as a single-branch anyOf over the named type, which renders as union json.RawMessage + AsNode/FromNode/MergeNode. It works because a $ref inside a union is the one construct that reliably terminates. But it's a shape the generator emits nowhere else for allOf. Non-recursively, allOf: [$ref Base, {weight}] gives you a flattened concrete struct:

List *[]struct {
	Name   *string `json:"name,omitempty"`
	Weight *int    `json:"weight,omitempty"`
} `json:"list,omitempty"`

and even when the $ref'd member is itself a union, we don't emit a single-branch union over it — we regenerate the branches:

type Holder_Item struct { Extra *string; union json.RawMessage }
type HolderItem0 struct { Leaf *string }   // U's branches, regenerated
type HolderItem1 struct { N *int }

So the accessor shape is something users would meet only in the recursive case, for no reason intrinsic to their spec.

And we don't need it. Go does recursive types natively, and we already emit them — Document.Fields *map[string]Value in internal/test/schemas/recursive. For

Tree:
  type: object
  properties:
    name: { type: string }
    children:
      type: array
      items:
        allOf:
          - { $ref: '#/components/schemas/Tree' }
          - { type: object, properties: { weight: { type: integer } } }

the right output is:

type Tree struct {
	Children *[]Tree_Children_Item `json:"children,omitempty"`
	Name     *string               `json:"name,omitempty"`
}

type Tree_Children_Item struct {
	Children *[]Tree_Children_Item `json:"children,omitempty"`
	Name     *string               `json:"name,omitempty"`
	Weight   *int                  `json:"weight,omitempty"`
}

Same JSON, plain field access, and semantically exact: Tree_Children_Item = Tree ∧ {weight}, whose children are items of Tree.children — the same composition, so grandchildren carry weight too, which is what the spec says. The recursion is a genuine fixed point reached in one step.

Node above still gets a union, because Node genuinely is an anyOf — but it'd be an honest union over Node's own branches, matching the Holder_Item shape, with the loop closing on Node_1_Children_Item.

For contrast, here's what the union approach produces for the mutual-recursion spec — it unrolls a level and a half and then goes opaque:

type A struct {
	B *struct {
		C *struct {
			Back *A_B_C_Back `json:"back,omitempty"`
			X    *string     `json:"x,omitempty"`
		} `json:"c,omitempty"`
		Tag *string `json:"tag,omitempty"`
	} `json:"b,omitempty"`
}

type A_B_C_Back struct {
	Y     *string `json:"y,omitempty"`
	union json.RawMessage
}

How to get there

The enabling detail is that both naming sites are guarded the same way:

// schema.go:1497 (array items), :1264 (property)
if ( /* needs a name */ ) && arrayType.RefType == "" {
	typeName := PathToTypeName(append(path, "Item"))
	…
	arrayType.RefType = typeName
}

So if mergeSchemas names the type itself — sets RefType and appends a TypeDefinition — the callers stand down and use it, and AdditionalTypes already propagates to the output. No changes needed at either call site. That gives:

1. A memo in the recursion context: map[*openapi3.Schema]string, keyed on the schema node that owns the allOf, mapping to the Go type name being generated for it. It has to be the owning node — the sibling-injection path at schema.go:1091 builds a fresh slice with a fresh &s member on every call, so keying on the slice or its members won't be stable.
2. mergeSchemas registers node -> name before generating the merged body.
3. Re-entry returns Schema{RefType: name} — a reference to the type under construction — instead of inlining again.
4. Afterwards, check whether the memo entry was actually consulted. Consulted: emit the named TypeDefinition and return RefType. Not consulted: return the anonymous inline struct exactly as today.

Step 4 is what keeps this safe — every non-recursive allOf keeps byte-identical output, so there's no SemVer surface. Termination holds because the memo is keyed on syntactic allOf nodes, of which a document has finitely many, and re-entry resolves to a name instead of descending.

One consequence worth stating up front: the same composition reached while generating two different components gets named once per component (A_B_C vs B_C), because the memo is scoped to the in-progress stack. That's duplication, but it's exactly what we already do for anonymous nested structures today.

Value recursion should be a diagnostic, not a compile error

If the recursive occurrence is reached with no indirection in between, the Go type is infinitely sized:

T:
  type: object
  required: [child]
  properties:
    child: { allOf: [ { $ref: '#/components/schemas/T' }, { type: object, properties: { w: { type: integer } } } ] }

would want type T_Child struct { Child T_Child; W *int } — invalid recursive type. That spec describes impossible data, so failing is right, but it should fail in the generator with a clear message rather than in the user's compiler.

That falls out of the same context: record an indirection counter at registration, bump it whenever we descend through something that introduces a pointer, slice or map — array items, additionalProperties, an optional property — and at re-entry compare against the registered value. Equal means no indirection was crossed, so error with the path and a hint (make it optional, wrap it in an array, or give it its own schema). The one to be careful with is x-go-type-skip-optional-pointer, which turns an otherwise-indirect optional property back into a value; nullable-type needs checking too.

Keeping the exported API intact

GenerateGoSchema, MergeSchemas and GenerateTypesForSchemas are all exported from pkg/codegen and people do import them, so their signatures shouldn't move. The context wants to be threaded through unexported entry points — generateGoSchema(ctx, …), mergeSchemas(ctx, …, nameHint) — with the exported functions kept as thin wrappers that build a fresh root context and delegate. External callers keep working and get exactly today's behaviour.

The name hint has to ride that context rather than path: mergeSchemas sees path = ["Tree","children"] but the type is named Tree_Children_Item, and extending path at items/additionalProperties would rename nested types across every spec.

Also, I suspect this lets the self-composition special case go away entirely. RecursiveObject: allOf[$ref NonRecursiveObject, $ref RecursiveObject, {…}] (#1373) folds to a flat struct via the existing seenSchemaRef dedup during flattening, before a memo keyed on property/items re-entry would ever fire. Worth confirming once it's built.

One correction to the framing

This isn't a regression from #2471. The oneOf variant, the plain-object Tree shape and the mutual-recursion case all overflow on v2.7.2 as well — I checked against a build of 823a7a7b^. What #2471 changed is that mergeOpenapiSchemas now propagates anyOf/oneOf; before, an anyOf inside an allOf was silently dropped, which is why the reporter's exact spec "worked" on v2.7.2 while quietly losing the back-reference. So #2471 exposed one more shape of an older bug, and the fix here also recovers data that used to go missing.

Happy to push the above onto this branch if you'd rather not take it on — say the word either way. Your fixtures and comments are good and I'd keep them; it's mainly merge_schemas.go and the context plumbing that change.

_{🤖 Investigated and drafted with Claude Code; the repros above were verified locally against e1ba901d.}