oapi-codegen icon indicating copy to clipboard operation
oapi-codegen copied to clipboard

fix: avoid stack overflow errors when using heavily recursive types

Open yoshikipom opened this issue 2 years ago • 1 comments

Issue

https://github.com/deepmap/oapi-codegen/issues/1373 Stack overflow error happens merge_schemas.go when allOf contains circular $ref.

Cause & How to fix

https://github.com/deepmap/oapi-codegen/blob/1f53862bcc64573d3d0c4c105c71a8143e7b1816/pkg/codegen/merge_schemas.go#L87 mergeOpenapiSchemas is called recursively until all items in allOf are consumed. (allOf of item in allOf are 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 allOf in allOf.

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

  • Add test e2ade0308d30e0b475a0964f405ea7c1982d0a4a
    • I confirmed this test failed correctly before changes in next commit.
  • Fix stack overflow by skipping to merge same object c56f541baf93a9c7f3591fa80919b2c38fa93f2a

Test

Before fix

$ go test ./internal/test/issues/issue-1373/issue_test.go     
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0x140203785d0 stack=[0x14020378000, 0x14040378000]
fatal error: stack overflow

runtime stack: ...

After fix

$ go test ./internal/test/issues/issue-1373/issue_test.go
ok      command-line-arguments  0.308s

yoshikipom avatar Dec 07 '23 01:12 yoshikipom

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 avatar Sep 20 '24 11:09 jamietanna