fix: avoid stack overflow errors when using heavily recursive types - #1377
Merged
jamietanna merged 12 commits intoApr 3, 2026
Conversation
added 3 commits
September 20, 2024 12:33
jamietanna
changed the title
Member
|
Interestingly, this changes behaviour of an existing case: diff --git a/internal/test/all_of/v2/openapi.gen.go b/internal/test/all_of/v2/openapi.gen.go
index ab43034..618cec4 100644
--- a/internal/test/all_of/v2/openapi.gen.go
+++ b/internal/test/all_of/v2/openapi.gen.go
@@ -32,10 +32,10 @@ type PersonProperties struct {
// PersonWithID defines model for PersonWithID.
type PersonWithID struct {
- FirstName string `json:"FirstName"`
- GovernmentIDNumber *int64 `json:"GovernmentIDNumber,omitempty"`
- ID int64 `json:"ID"`
- LastName string `json:"LastName"`
+ FirstName *string `json:"FirstName,omitempty"`
+ GovernmentIDNumber *int64 `json:"GovernmentIDNumber,omitempty"`
+ ID int64 `json:"ID"`
+ LastName *string `json:"LastName,omitempty"`
}I need to confirm if that's expected |
jamietanna
enabled auto-merge (squash)
September 20, 2024 11:44
# Conflicts: # .github/workflows/generate.yml
Member
…e schemas Inline schemas have an empty Ref string. Without this guard, the seenSchemaRef map would key on empty string, causing all inline schemas after the first to be incorrectly skipped during allOf merging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR fixes a stack overflow in
Confidence Score: 3/5Not safe to merge as-is — the fix only handles the case where the self-ref is not the first allOf entry; placing the recursive ref first still stack overflows. The core approach (tracking visited refs to break cycles) is correct, but the implementation has a coverage gap: allOf[0]'s ref is never added to the seen-set, leaving a variant of the original bug unfixed for specs where the self-reference appears as the first allOf item. pkg/codegen/merge_schemas.go — specifically the seenSchemaRef initialization in mergeSchemas before and during the loop.
|
| Filename | Overview |
|---|---|
| pkg/codegen/merge_schemas.go | Core fix: seenSchemaRef map passed through recursive mergeAllOf/mergeOpenapiSchemas to break cycles, but allOf[0]'s ref is never seeded into the map, leaving a stack-overflow path when the self-ref is first in allOf. |
| pkg/codegen/merge_schemas_test.go | Existing tests updated to pass the new seenSchemaRef argument to mergeOpenapiSchemas; no new unit tests for the cycle-detection logic itself. |
| internal/test/issues/issue-1373/spec.yaml | New test fixture for the recursive-allOf scenario; contains a likely typo (FieldInRecursive::) that gives the property a colon in its name. |
| internal/test/issues/issue-1373/config.yaml | Minimal codegen config for the new issue-1373 test; no issues. |
| internal/test/issues/issue-1373/generate.go | Standard go:generate directive for the issue-1373 test package; no issues. |
Reviews (2): Last reviewed commit: "fix: only track named refs in cycle dete..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Member
mromaszewicz
approved these changes
Apr 3, 2026
Comment on lines
+42
to
+46
| seenSchemaRef := make(map[string]bool) | ||
| if allOf[i].Ref != "" { | ||
| seenSchemaRef[allOf[i].Ref] = true | ||
| } | ||
| schema, err = mergeOpenapiSchemas(schema, oneOfSchema, true, seenSchemaRef) |
Contributor
There was a problem hiding this comment.
allOf[0] self-ref not seeded into seenSchemaRef
allOf[0]'s ref is never added to any seenSchemaRef map before the loop. Inside mergeOpenapiSchemas, s1 is derived from allOf[0].Value, and if s1.AllOf contains a ref back to the same schema (i.e., the recursive self-ref is placed first in allOf), mergeAllOf(s1.AllOf, seenSchemaRef) won't detect the cycle — the ref isn't in the map — and will stack overflow again.
Concretely, a spec like the following would still crash:
RecursiveObject:
allOf:
- $ref: "#/components/schemas/RecursiveObject" # self-ref is allOf[0]
- $ref: "#/components/schemas/NonRecursiveObject"Seeding allOf[0].Ref into an initial map before iterating would close this gap:
seenTopLevel := make(map[string]bool)
if allOf[0].Ref != "" {
seenTopLevel[allOf[0].Ref] = true
}
for i := 1; i < n; i++ {
seenSchemaRef := make(map[string]bool)
for k, v := range seenTopLevel { seenSchemaRef[k] = v }
if allOf[i].Ref != "" {
seenSchemaRef[allOf[i].Ref] = true
seenTopLevel[allOf[i].Ref] = true
}
...
}
mromaszewicz
added a commit
to mromaszewicz/oapi-codegen
that referenced
this pull request
Apr 4, 2026
PR oapi-codegen#1377 was merged too early. This is a small follow-up tweak to make sure that oapi-codegen#1373 is fixed properly. seed allOf[0] ref into cycle detection to prevent stack overflow when self-ref is first When the self-referencing $ref appears at position 0 in allOf, its ref was never added to seenSchemaRef. If s1's own AllOf contained a back-reference to itself, mergeAllOf would not detect the cycle. Seed allOf[0].Ref into a top-level seen set that is copied into each iteration's seenSchemaRef map, closing this gap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mromaszewicz
added a commit
that referenced
this pull request
Apr 11, 2026
PR #1377 was merged too early. This is a small follow-up tweak to make sure that #1373 is fixed properly. seed allOf[0] ref into cycle detection to prevent stack overflow when self-ref is first When the self-referencing $ref appears at position 0 in allOf, its ref was never added to seenSchemaRef. If s1's own AllOf contained a back-reference to itself, mergeAllOf would not detect the cycle. Seed allOf[0].Ref into a top-level seen set that is copied into each iteration's seenSchemaRef map, closing this gap. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
#1373
Stack overflow error happens
merge_schemas.gowhenallOfcontains circular$ref.Cause & How to fix
https://github.com/deepmap/oapi-codegen/blob/1f53862bcc64573d3d0c4c105c71a8143e7b1816/pkg/codegen/merge_schemas.go#L87
mergeOpenapiSchemasis called recursively until all items inallOfare consumed. (allOfof item inallOfare also explored.) If same item is in the path of the exploration, stack overflow can happen.https://github.com/deepmap/oapi-codegen/blob/1f53862bcc64573d3d0c4c105c71a8143e7b1816/pkg/codegen/merge_schemas.go#L85-L86
This code flatten schemas and merge them to handle
allOfinallOf.My approach is just skip to merge same schema because that doesn't affect to the merge result.
https://github.com/deepmap/oapi-codegen/compare/master...yoshikipom:oapi-codegen:fix/recursive-error-allof?expand=1#diff-423bfd1d22f4994f4f0c03f76f15af8371bee0bc5da50efa3ab696e1a60d7d49R41-R43
Commit
Test
Before fix
After fix