diff --git a/examples/authenticated-api/stdhttp/go.mod b/examples/authenticated-api/stdhttp/go.mod index e66d4374b5..5217b22290 100644 --- a/examples/authenticated-api/stdhttp/go.mod +++ b/examples/authenticated-api/stdhttp/go.mod @@ -19,6 +19,7 @@ require ( github.com/dprotaso/go-yit v0.0.0-20220510233725-9ba8df137936 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect github.com/go-openapi/swag v0.23.0 // indirect + github.com/go-test/deep v1.0.8 // indirect github.com/goccy/go-json v0.10.2 // indirect github.com/gorilla/mux v1.8.1 // indirect github.com/invopop/yaml v0.3.1 // indirect diff --git a/examples/go.mod b/examples/go.mod index a73a316fdc..a34dcd638e 100644 --- a/examples/go.mod +++ b/examples/go.mod @@ -51,6 +51,7 @@ require ( github.com/go-playground/locales v0.14.1 // indirect github.com/go-playground/universal-translator v0.18.1 // indirect github.com/go-playground/validator/v10 v10.20.0 // indirect + github.com/go-test/deep v1.0.8 // indirect github.com/goccy/go-json v0.10.2 // indirect github.com/golang-jwt/jwt v3.2.2+incompatible // indirect github.com/golang/snappy v0.0.4 // indirect diff --git a/examples/minimal-server/stdhttp/go.mod b/examples/minimal-server/stdhttp/go.mod index 9b14e36694..1804571337 100644 --- a/examples/minimal-server/stdhttp/go.mod +++ b/examples/minimal-server/stdhttp/go.mod @@ -11,6 +11,7 @@ require ( github.com/getkin/kin-openapi v0.128.0 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect github.com/go-openapi/swag v0.23.0 // indirect + github.com/go-test/deep v1.0.8 // indirect github.com/invopop/yaml v0.3.1 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/mailru/easyjson v0.7.7 // indirect diff --git a/examples/petstore-expanded/stdhttp/go.mod b/examples/petstore-expanded/stdhttp/go.mod index e29c76831e..a21ce05bde 100644 --- a/examples/petstore-expanded/stdhttp/go.mod +++ b/examples/petstore-expanded/stdhttp/go.mod @@ -19,6 +19,7 @@ require ( github.com/dprotaso/go-yit v0.0.0-20220510233725-9ba8df137936 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect github.com/go-openapi/swag v0.23.0 // indirect + github.com/go-test/deep v1.0.8 // indirect github.com/google/uuid v1.4.0 // indirect github.com/gorilla/mux v1.8.1 // indirect github.com/invopop/yaml v0.3.1 // indirect diff --git a/go.mod b/go.mod index 914750e087..de4c531c9a 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.21.0 require ( github.com/getkin/kin-openapi v0.128.0 + github.com/go-test/deep v1.0.8 github.com/speakeasy-api/openapi-overlay v0.9.0 github.com/stretchr/testify v1.10.0 golang.org/x/text v0.20.0 diff --git a/internal/test/go.mod b/internal/test/go.mod index d470e4ed6c..1de589a3a5 100644 --- a/internal/test/go.mod +++ b/internal/test/go.mod @@ -43,6 +43,7 @@ require ( github.com/go-playground/locales v0.14.1 // indirect github.com/go-playground/universal-translator v0.18.1 // indirect github.com/go-playground/validator/v10 v10.14.1 // indirect + github.com/go-test/deep v1.0.8 // indirect github.com/goccy/go-json v0.10.2 // indirect github.com/golang-jwt/jwt v3.2.2+incompatible // indirect github.com/golang/snappy v0.0.4 // indirect diff --git a/internal/test/strict-server/stdhttp/go.mod b/internal/test/strict-server/stdhttp/go.mod index d161abd8d6..ab828521d7 100644 --- a/internal/test/strict-server/stdhttp/go.mod +++ b/internal/test/strict-server/stdhttp/go.mod @@ -21,6 +21,7 @@ require ( github.com/dprotaso/go-yit v0.0.0-20220510233725-9ba8df137936 // indirect github.com/go-openapi/jsonpointer v0.21.0 // indirect github.com/go-openapi/swag v0.23.0 // indirect + github.com/go-test/deep v1.0.8 // indirect github.com/google/uuid v1.4.0 // indirect github.com/invopop/yaml v0.3.1 // indirect github.com/josharian/intern v1.0.0 // indirect diff --git a/pkg/codegen/codegen.go b/pkg/codegen/codegen.go index ef175afdd5..f19de25c64 100644 --- a/pkg/codegen/codegen.go +++ b/pkg/codegen/codegen.go @@ -31,6 +31,7 @@ import ( "time" "github.com/getkin/kin-openapi/openapi3" + "github.com/go-test/deep" "golang.org/x/tools/imports" "github.com/oapi-codegen/oapi-codegen/v2/pkg/util" @@ -719,8 +720,16 @@ func GenerateTypes(t *template.Template, types []TypeDefinition) (string, error) continue } // We want to create an error when we try to define the same type twice. - return "", fmt.Errorf("duplicate typename '%s' detected, can't auto-rename, "+ - "please use x-go-name to specify your own name for one of them", typ.TypeName) + diff := deep.Equal(prevType.Schema.OAPISchema, typ.Schema.OAPISchema) + return "", fmt.Errorf( + "duplicate typename '%s', can't auto-rename, "+ + "please use x-go-name to specify your own name for one of them"+ + "\n\tpath1:\n\t\t%s\n\tpath2:\n\t\t%s\n\tdiff:\n\t\t%s", + typ.TypeName, + prevType.JsonName, + typ.JsonName, + strings.Join(diff, "\n\t\t"), + ) } m[typ.TypeName] = typ diff --git a/pkg/codegen/merge_schemas.go b/pkg/codegen/merge_schemas.go index 04e7b2fa2b..b7d14fc1e0 100644 --- a/pkg/codegen/merge_schemas.go +++ b/pkg/codegen/merge_schemas.go @@ -37,7 +37,7 @@ func mergeSchemas(allOf []*openapi3.SchemaRef, path []string) (Schema, error) { if err != nil { return Schema{}, err } - schema, err = mergeOpenapiSchemas(schema, oneOfSchema, true) + schema, err = mergeOpenapiSchemas(schema, oneOfSchema) if err != nil { return Schema{}, fmt.Errorf("error merging schemas for AllOf: %w", err) } @@ -74,7 +74,7 @@ func mergeAllOf(allOf []*openapi3.SchemaRef) (openapi3.Schema, error) { var schema openapi3.Schema for _, schemaRef := range allOf { var err error - schema, err = mergeOpenapiSchemas(schema, *schemaRef.Value, true) + schema, err = mergeOpenapiSchemas(schema, *schemaRef.Value) if err != nil { return openapi3.Schema{}, fmt.Errorf("error merging schemas for AllOf: %w", err) } @@ -84,7 +84,7 @@ func mergeAllOf(allOf []*openapi3.SchemaRef) (openapi3.Schema, error) { // mergeOpenapiSchemas merges two openAPI schemas and returns the schema // all of whose fields are composed. -func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool) (openapi3.Schema, error) { +func mergeOpenapiSchemas(s1, s2 openapi3.Schema) (openapi3.Schema, error) { var result openapi3.Schema result.Extensions = make(map[string]interface{}) @@ -96,10 +96,13 @@ func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool) (openapi3.Schema, e result.Extensions[k] = v } - result.OneOf = append(s1.OneOf, s2.OneOf...) - // We are going to make AllOf transitive, so that merging an AllOf that // contains AllOf's will result in a flat object. + // TODO: this does not account for other schema fields at the same level as + // an AllOf - they will be lost, see also where MergeSchemas is used + // in GenerateGoSchema (all fields from the parent are disregarded + // and instead only the schema containing the merged AllOfs is + // returned) var err error if s1.AllOf != nil { var merged openapi3.Schema @@ -120,6 +123,17 @@ func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool) (openapi3.Schema, e result.AllOf = append(s1.AllOf, s2.AllOf...) + // NOTE: this must happen _after_ AllOf merging above or OneOfs can be lost, for example: + // SchemaFoo: {allOf: [{allOf: [{oneOf: [A, B]}], SchemaBar]} + // the resulting schema will only include SchemaBar if OneOf merging is done before + // AllOf recursion - this is because we won't have any deeply nested OneOfs flattened + // into s1/s2 yet at that point + // TODO: this does not accurately merge OneOf/AnyOf, for example if an AllOf contains 2 + // OneOf schema children, the result should require one from each set, but this will + // require only one from the combined set + result.OneOf = append(s1.OneOf, s2.OneOf...) + result.AnyOf = append(s1.AnyOf, s2.AnyOf...) + if s1.Type.Slice() != nil && s2.Type.Slice() != nil && !equalTypes(s1.Type, s2.Type) { return openapi3.Schema{}, fmt.Errorf("can not merge incompatible types: %v, %v", s1.Type.Slice(), s2.Type.Slice()) } @@ -222,11 +236,6 @@ func mergeOpenapiSchemas(s1, s2 openapi3.Schema, allOf bool) (openapi3.Schema, e } } - // Allow discriminators for allOf merges, but disallow for one/anyOfs. - if !allOf && (s1.Discriminator != nil || s2.Discriminator != nil) { - return openapi3.Schema{}, errors.New("merging two schemas with discriminators is not supported") - } - return result, nil } diff --git a/pkg/codegen/merge_schemas_test.go b/pkg/codegen/merge_schemas_test.go new file mode 100644 index 0000000000..6c792acff2 --- /dev/null +++ b/pkg/codegen/merge_schemas_test.go @@ -0,0 +1,401 @@ +package codegen + +import ( + "encoding/json" + "errors" + "slices" + "testing" + + "github.com/getkin/kin-openapi/openapi3" + "github.com/go-test/deep" +) + +func TestAllOf(t *testing.T) { + suite := []testCase{ + // just properties + makeTestCase( + "when 1 level deep, it merges schemas", + `[ + { "properties": { "a_foo": { "type": "string" } } }, + { "properties": { "b_foo": { "type": "string" } } } + ]`, + `{ "a_foo": { "type": "string" }, "b_foo": { "type": "string" } }`, + "", + nil, + ), + makeTestCase( + "when 1 level deep with 1 schema, it returns single schema", + `[ + { "properties": { "a_foo": { "type": "string" } } } + ]`, + `{ "a_foo": { "type": "string" } }`, + "", + nil, + ), + makeTestCase( + "when 3 levels deep, it merges schemas", + `[ + { + "allOf": [ + { + "allOf": [ + {"properties": { "a0_foo": { "type": "string" } }}, + {"properties": { "a1_foo": { "type": "string" } }} + ] + } + ] + }, + { + "allOf": [ + { + "allOf": [ + {"properties": { "b0_foo": { "type": "string" } }}, + {"properties": { "b1_foo": { "type": "string" } }} + ] + } + ] + } + ]`, + `{ + "a0_foo": { "type": "string" }, + "a1_foo": { "type": "string" }, + "b0_foo": { "type": "string" }, + "b1_foo": { "type": "string" } + }`, + "", + nil, + ), + makeTestCase( + "when 3 levels deep with single schema, it flattens to single schema", + `[ + { + "allOf": [ + { + "allOf": [ + {"properties": { "a0_foo": { "type": "string" } }}, + {"properties": { "a1_foo": { "type": "string" } }} + ] + } + ] + } + ]`, + `{ + "a0_foo": { "type": "string" }, + "a1_foo": { "type": "string" } + }`, + "", + nil, + ), + // include single oneOf + makeTestCase( + "when 1 level deep with oneOf, it merges schemas", + `[ + { + "properties": { "a_foo": { "type": "string" } }, + "oneOf": [ + { "properties": { "a_foo_one_of_0": { "type": "string" } }}, + { "properties": { "a_foo_one_of_1": { "type": "string" } }} + ] + }, + { "properties": { "b_foo": { "type": "string" } } } + ]`, + `{ "a_foo": { "type": "string" }, "b_foo": { "type": "string" } }`, + `[ + { "properties": { "a_foo_one_of_0": { "type": "string" } }}, + { "properties": { "a_foo_one_of_1": { "type": "string" } }} + ]`, + nil, + ), + makeTestCase( + "when 3 levels deep with oneOf at leaf, it merges schemas", + `[ + { + "allOf": [ + { + "allOf": [ + { + "properties": { "a0_foo": { "type": "string" } }, + "oneOf": [ + { "properties": { "a_foo_one_of_0": { "type": "string" } }}, + { "properties": { "a_foo_one_of_1": { "type": "string" } }} + ] + }, + { "properties": { "a1_foo": { "type": "string" } } } + ] + } + ] + }, + { + "allOf": [ + { + "allOf": [ + {"properties": { "b0_foo": { "type": "string" } }}, + {"properties": { "b1_foo": { "type": "string" } }} + ] + } + ] + } + ]`, + `{ + "a0_foo": { "type": "string" }, + "a1_foo": { "type": "string" }, + "b0_foo": { "type": "string" }, + "b1_foo": { "type": "string" } + }`, + `[ + { "properties": { "a_foo_one_of_0": { "type": "string" } }}, + { "properties": { "a_foo_one_of_1": { "type": "string" } }} + ]`, + nil, + ), + makeTestCase( + "when 3 levels deep with oneOf in the middle, it merges schemas", + `[ + { + "allOf": [ + { + "oneOf": [ + { "properties": { "a_foo_one_of_0": { "type": "string" } }}, + { "properties": { "a_foo_one_of_1": { "type": "string" } }} + ] + }, + { + "allOf": [ + {"properties": { "a0_foo": { "type": "string" } }}, + {"properties": { "a1_foo": { "type": "string" } }} + ] + } + ] + }, + { + "allOf": [ + { + "allOf": [ + {"properties": { "b0_foo": { "type": "string" } }}, + {"properties": { "b1_foo": { "type": "string" } }} + ] + } + ] + } + ]`, + `{ + "a0_foo": { "type": "string" }, + "a1_foo": { "type": "string" }, + "b0_foo": { "type": "string" }, + "b1_foo": { "type": "string" } + }`, + `[ + { "properties": { "a_foo_one_of_0": { "type": "string" } }}, + { "properties": { "a_foo_one_of_1": { "type": "string" } }} + ]`, + nil, + ), + makeTestCase( + "with multi-level oneOf, it merges schemas", + `[ + { + "allOf": [ + { + "allOf": [ + {"properties": { "a0_foo": { "type": "string" } }}, + {"properties": { "a1_foo": { "type": "string" } }} + ] + }, + { + "oneOf": [ + { + "allOf": [ + {"properties": { "a_foo_one_of_a_0": { "type": "string" } }}, + {"properties": { "a_foo_one_of_a_1": { "type": "string" } }} + ] + }, + { + "allOf": [ + {"properties": { "a_foo_one_of_b_0": { "type": "string" } }}, + {"properties": { "a_foo_one_of_b_1": { "type": "string" } }} + ] + } + ] + } + ] + }, + { + "allOf": [ + { + "allOf": [ + {"properties": { "b0_foo": { "type": "string" } }}, + {"properties": { "b1_foo": { "type": "string" } }} + ] + } + ] + } + ]`, + `{ + "a0_foo": { "type": "string" }, + "a1_foo": { "type": "string" }, + "b0_foo": { "type": "string" }, + "b1_foo": { "type": "string" } + }`, + `[ + { + "properties": { + "a_foo_one_of_a_0": { "type": "string" }, + "a_foo_one_of_a_1": { "type": "string" } + } + }, + { + "properties": { + "a_foo_one_of_b_0": { "type": "string" }, + "a_foo_one_of_b_1": { "type": "string" } + } + } + ]`, + nil, + ), + // other test cases + makeTestCase( + "should preserve nested oneOf (https://github.com/oapi-codegen/oapi-codegen/issues/1905)", + `[ + { + "allOf": [ + { + "allOf": [ + { + "oneOf": [ + {"properties": { "a_foo_one_of_0": { "type": "string" } }}, + {"properties": { "a_foo_one_of_1": { "type": "string" } }} + ] + } + ] + }, + { "properties": { "a_foo": { "type": "string" } } } + ] + } + ]`, + `{ + "a_foo": { "type": "string" } + }`, + `[ + { "properties": { "a_foo_one_of_0": { "type": "string" } }}, + { "properties": { "a_foo_one_of_1": { "type": "string" } }} + ]`, + nil, + ), + } + + runSuite(t, suite) +} + +func runSuite(t *testing.T, suite []testCase) { + for _, test := range suite { + t.Run(test.name, func(t *testing.T) { + result, err := MergeSchemas(test.inputAllOf, make([]string, 0)) + + if diff := deep.Equal(err, test.expectedError); diff != nil { + var errString = "Error validation failed. diff:" + for _, diffItem := range diff { + errString += "\n\t" + diffItem + } + t.Fatal(errString) + return + } + if err != nil { + return + } + + // result.OAPISchema is an intermediate schema (it's currently the schema after recursing + // AllOf, but but before recursing Properties/Enums/everything else that needs recursing), + // so we need to pull merged props off the "codegen" version + // NOTE: normally result.OAPISchema would be the "original" schema when calling + // `GenerateGoSchema`, but we are testing `MergeSchemas` directly here + err = validateProperties(&test.expectedProperties, &result) + if err != nil { + t.Fatal(err) + return + } + + // similar to above, the OneOfs are after the first pass or merging, so we extract + // the properties for comparison + // TODO: current tests only care about properties, but we should also test other fields + if len(result.UnionElements) != len(test.expectedOneOf) { + t.Fatalf("Expected %d oneOfs, got %d", len(test.expectedOneOf), len(result.UnionElements)) + return + } + for i, oneOf := range result.UnionElements { + typeIdx := slices.IndexFunc(result.AdditionalTypes, func(t TypeDefinition) bool { return t.TypeName == string(oneOf) }) + if typeIdx == -1 { + t.Fatalf("Expected oneOf %d to have type %s, but it was not found", i, oneOf) + return + } + oneOfType := result.AdditionalTypes[typeIdx] + expectedOneOf := test.expectedOneOf[i] + // see notes on validateProperties above + err = validateProperties(&expectedOneOf.Value.Properties, &oneOfType.Schema) + if err != nil { + t.Fatal(err) + return + } + } + }) + } +} + +func validateProperties(expectedProperties *openapi3.Schemas, result *Schema) error { + props := openapi3.Schemas{} + for _, prop := range result.Properties { + props[prop.JsonFieldName] = openapi3.NewSchemaRef("", prop.Schema.OAPISchema) + } + if diff := deep.Equal(props, *expectedProperties); diff != nil { + var errString = "Properties validation failed. diff:" + for _, diffItem := range diff { + errString += "\n\t" + diffItem + } + return errors.New(errString) + } + + return nil +} + +type testCase struct { + name string + inputAllOf openapi3.SchemaRefs + expectedProperties openapi3.Schemas + expectedOneOf openapi3.SchemaRefs + expectedError error +} + +func makeTestCase( + name string, + inputAllOfSchema string, + expectedPropertiesSchema string, + expectedOneOfSchema string, + expectedError error, +) testCase { + inputAllOf := openapi3.SchemaRefs{} + if err := json.Unmarshal([]byte(inputAllOfSchema), &inputAllOf); err != nil { + panic(err) + } + + var expectedProperties openapi3.Schemas + if len(expectedPropertiesSchema) > 0 { + if err := json.Unmarshal([]byte(expectedPropertiesSchema), &expectedProperties); err != nil { + panic(err) + } + } + + var expectedOneOf openapi3.SchemaRefs = nil + if len(expectedOneOfSchema) > 0 { + expectedOneOf = openapi3.SchemaRefs{} + if err := json.Unmarshal([]byte(expectedOneOfSchema), &expectedOneOf); err != nil { + panic(err) + } + } + + return testCase{ + name: name, + inputAllOf: inputAllOf, + expectedProperties: expectedProperties, + expectedOneOf: expectedOneOf, + expectedError: expectedError, + } +}