From 5c71239467c0a50cfa991ee8b14f4469c95eb933 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Wed, 5 Aug 2026 09:16:55 +0800 Subject: [PATCH 1/4] fix: valid specs generate unusable params, bodies, and enum constants Closes #15, #16, #17, all reported against the Mealie OpenAPI 3.1 spec. An enum of comparison operators sanitized every value to the same identifier, so the constants came out numbered (RelationalOperator2..7) with nothing to say which was which. Punctuation-only values are now spelled out: "<>" becomes RelationalOperatorNotEqual. A parameter typed `anyOf: [{type: string}, {type: "null"}]` -- how OpenAPI 3.1 spells "nullable string" -- generated *any, which callers had no way to construct a value for. Such a union collapses to the variant's own type, as does one whose members are all refinements of a single type (a uuid-formatted string or a plain string). Members that are inline schemas now keep their Go type instead of degrading to any, and a `type: null` member no longer becomes a variant of its own. A multipart/form-data body was JSON-marshaled and sent as application/json. The declared media type now selects the encoding: multipart is written part by part, url-encoded bodies become form pairs, and a body in any other media type is sent as the bytes or text it already is. Binary properties of a multipart body generate a FormFile carrying a filename and content type, which is what makes a server treat the part as an upload at all. Fixed along the way, each of which produced a client that did not compile or silently lost data: - Mutually recursive schemas emitted `type A = B; type B = A`. Go expands aliases eagerly, so no indirection saves them -- `[]` included. - A body offered as both JSON and multipart typed its binary properties as FormFile while emitting no such type. - A spec-controlled media type was interpolated into Go source unquoted. - additionalProperties never reached the wire on a non-JSON body. - A CRLF in a filename or content type could inject MIME headers. Generated clients are exercised, not just compiled: the e2e tests run the client against a server that parses what it sent. --- internal/analyzer/aliascycles.go | 87 ++++ internal/analyzer/analyzer.go | 10 + internal/analyzer/enumnames.go | 103 ++++ internal/analyzer/enumnames_test.go | 98 ++++ internal/analyzer/operations.go | 155 ++++-- internal/analyzer/schemas.go | 353 +++++++++---- .../analyzer/schemas_nullable_union_test.go | 457 +++++++++++++++++ internal/generator/e2e_request_body_test.go | 473 ++++++++++++++++++ internal/generator/funcmap.go | 33 ++ internal/generator/generator_test.go | 8 +- internal/ir/operations.go | 1 - internal/templates/client.go.tmpl | 21 +- internal/templates/helpers.go.tmpl | 293 ++++++++++- internal/templates/operations.go.tmpl | 4 +- internal/templates/types.go.tmpl | 12 +- 15 files changed, 1931 insertions(+), 177 deletions(-) create mode 100644 internal/analyzer/aliascycles.go create mode 100644 internal/analyzer/enumnames.go create mode 100644 internal/analyzer/enumnames_test.go create mode 100644 internal/analyzer/schemas_nullable_union_test.go create mode 100644 internal/generator/e2e_request_body_test.go diff --git a/internal/analyzer/aliascycles.go b/internal/analyzer/aliascycles.go new file mode 100644 index 0000000..5e6a6d4 --- /dev/null +++ b/internal/analyzer/aliascycles.go @@ -0,0 +1,87 @@ +package analyzer + +import ( + "strings" + + "github.com/parallelworks/openapi-client-generator/internal/ir" +) + +// breakAliasCycles degrades to any every alias that can reach itself. A generated +// alias is a true Go alias (`type A = B`), which the compiler expands eagerly, so +// a cycle through one is an "invalid recursive type" no matter how many slices, +// pointers, or maps sit between the two ends. Only aliases can form such a cycle: +// a struct, enum, or union names a real definition that terminates the chain. +func breakAliasCycles(types []*ir.TypeDef) { + aliases := make(map[string]*ir.TypeDef, len(types)) + for _, td := range types { + if td != nil && td.Kind == ir.TypeKindAlias { + aliases[td.Name] = td + } + } + + const ( + visiting = 1 + done = 2 + ) + state := make(map[string]int, len(aliases)) + + // Reports whether the alias named by name sits on a cycle, breaking the edge + // that closes one. + var walk func(name string) bool + walk = func(name string) bool { + td, ok := aliases[name] + if !ok { + return false + } + switch state[name] { + case visiting: + return true + case done: + return false + } + + state[name] = visiting + if target := aliasTarget(td.GoType); target != "" && walk(target) { + td.GoType = "any" + } + state[name] = done + return false + } + + for _, td := range types { + if td != nil && td.Kind == ir.TypeKindAlias { + walk(td.Name) + } + } +} + +// aliasTarget returns the named type an alias's Go type expression refers to, +// peeling the slice, pointer, and map wrappers that do not stop a Go alias from +// expanding. It returns "" for a builtin or a composite with no single referent. +func aliasTarget(goType string) string { + for { + switch { + case strings.HasPrefix(goType, "[]"): + goType = goType[2:] + case strings.HasPrefix(goType, "*"): + goType = goType[1:] + case strings.HasPrefix(goType, "map["): + end := strings.Index(goType, "]") + if end < 0 { + return "" + } + goType = goType[end+1:] + default: + return namedType(goType) + } + } +} + +// namedType returns goType when it is a bare type name rather than a builtin or +// a qualified type from another package. +func namedType(goType string) string { + if goType == "" || goType == "any" || strings.ContainsAny(goType, ".[]*{} ") { + return "" + } + return goType +} diff --git a/internal/analyzer/analyzer.go b/internal/analyzer/analyzer.go index 0818532..3ebb84d 100644 --- a/internal/analyzer/analyzer.go +++ b/internal/analyzer/analyzer.go @@ -20,6 +20,8 @@ type Analyzer struct { // deduplicated on their variant set and discriminator. synthesized []*ir.TypeDef synthesizedByKey map[string]*ir.TypeDef + // multipartBodies holds the schema names a multipart request body refers to. + multipartBodies map[string]bool } // New creates an Analyzer for the given high-level OpenAPI model. @@ -54,6 +56,11 @@ func (a *Analyzer) Analyze(packageName string) (*ir.Package, error) { } } + // A multipart body's binary properties are generated as file parts rather + // than as byte slices, which has to be settled before the schemas holding + // them are converted. + a.multipartBodies = a.collectMultipartBodySchemas() + // Analyze component schemas. if err := a.analyzeComponentSchemas(pkg); err != nil { return nil, err @@ -72,6 +79,9 @@ func (a *Analyzer) Analyze(packageName string) (*ir.Package, error) { // Append union types synthesized for inline oneOf/anyOf schemas. pkg.Types = append(pkg.Types, a.synthesized...) + // A spec is free to define a type in terms of itself; Go aliases are not. + breakAliasCycles(pkg.Types) + // Detect paginated operations. a.detectPagination(pkg) diff --git a/internal/analyzer/enumnames.go b/internal/analyzer/enumnames.go new file mode 100644 index 0000000..ba4c066 --- /dev/null +++ b/internal/analyzer/enumnames.go @@ -0,0 +1,103 @@ +package analyzer + +import ( + "strings" + "unicode" + + naming "github.com/giraffesyo/openapi-go-naming" +) + +// operatorWords names the multi-character operators that turn up as enum values +// in filter and comparison DSLs, so they read as one idea rather than as their +// spelled-out parts. +var operatorWords = map[string]string{ + "=": "Equal", + "==": "Equal", + "!=": "NotEqual", + "<>": "NotEqual", + "<": "LessThan", + "<=": "LessThanOrEqual", + ">": "GreaterThan", + ">=": "GreaterThanOrEqual", + "&&": "And", + "||": "Or", +} + +// symbolWords names individual punctuation runes. +var symbolWords = map[rune]string{ + ' ': "Space", + '!': "Not", + '"': "Quote", + '#': "Hash", + '$': "Dollar", + '%': "Percent", + '&': "And", + '\'': "Apostrophe", + '(': "OpenParen", + ')': "CloseParen", + '*': "Star", + '+': "Plus", + ',': "Comma", + '-': "Minus", + '.': "Dot", + '/': "Slash", + ':': "Colon", + ';': "Semicolon", + '<': "Less", + '=': "Equal", + '>': "Greater", + '?': "Question", + '@': "At", + '[': "OpenBracket", + '\\': "Backslash", + ']': "CloseBracket", + '^': "Caret", + '_': "Underscore", + '`': "Backtick", + '{': "OpenBrace", + '|': "Or", + '}': "CloseBrace", + '~': "Tilde", +} + +// enumConstName builds the Go constant name for one member of an enum. Values +// made only of punctuation ("=", "<>") are spelled out, because sanitizing them +// leaves nothing to name the constant after and every member of such an enum +// would want the same identifier. +func enumConstName(typeName, raw string) string { + if raw == "" { + return naming.Exported(typeName + " Empty") + } + if !hasAlphanumeric(raw) { + if words := punctuationWords(raw); words != "" { + return naming.Exported(typeName + " " + words) + } + } + return naming.Exported(typeName + " " + raw) +} + +// hasAlphanumeric reports whether s carries at least one rune that survives +// conversion to a Go identifier. +func hasAlphanumeric(s string) bool { + return strings.ContainsFunc(s, func(r rune) bool { + return unicode.IsLetter(r) || unicode.IsDigit(r) + }) +} + +// punctuationWords spells a punctuation-only value as words, returning "" when +// any rune has no name to spell it with. +func punctuationWords(raw string) string { + if words, ok := operatorWords[raw]; ok { + return words + } + var b strings.Builder + b.Grow(len(raw) * 8) + for _, r := range raw { + word, ok := symbolWords[r] + if !ok { + return "" + } + b.WriteString(word) + } + return b.String() +} diff --git a/internal/analyzer/enumnames_test.go b/internal/analyzer/enumnames_test.go new file mode 100644 index 0000000..ee2b430 --- /dev/null +++ b/internal/analyzer/enumnames_test.go @@ -0,0 +1,98 @@ +package analyzer + +import "testing" + +// TestEnumConstName pins how an enum value becomes a Go constant name. The +// punctuation rows are the ones that used to collapse onto a single identifier +// (issue #15): every member of a comparison-operator enum sanitized to the same +// name, so the generated const block did not compile. +func TestEnumConstName(t *testing.T) { + tests := []struct { + name string + typeName string + raw string + want string + }{ + // The reported case: an enum of relational operators. + {"equal", "RelationalOperator", "=", "RelationalOperatorEqual"}, + {"angle not equal", "RelationalOperator", "<>", "RelationalOperatorNotEqual"}, + {"greater", "RelationalOperator", ">", "RelationalOperatorGreaterThan"}, + {"less", "RelationalOperator", "<", "RelationalOperatorLessThan"}, + {"greater or equal", "RelationalOperator", ">=", "RelationalOperatorGreaterThanOrEqual"}, + {"less or equal", "RelationalOperator", "<=", "RelationalOperatorLessThanOrEqual"}, + {"bang not equal", "RelationalOperator", "!=", "RelationalOperatorNotEqual"}, + {"double equal", "RelationalOperator", "==", "RelationalOperatorEqual"}, + + // Punctuation with no operator spelling falls back to rune names. + {"star", "Wildcard", "*", "WildcardStar"}, + {"slash", "Sep", "/", "SepSlash"}, + {"double colon", "Sep", "::", "SepColonColon"}, + {"arrow", "Dir", "->", "DirMinusGreater"}, + {"empty", "Blank", "", "BlankEmpty"}, + + // Alphanumeric values keep the ordinary naming; punctuation inside them is + // still just a word separator. + {"word", "Status", "active", "StatusActive"}, + {"hyphenated", "Status", "in-progress", "StatusInProgress"}, + {"mixed", "Status", "n/a", "StatusNA"}, + {"initialism", "Format", "json", "FormatJSON"}, + {"numeric value", "Version", "2", "Version2"}, + + // A value made of runes with no name still yields something; the caller's + // uniquing scope resolves any collision. + {"unnameable", "Sym", "€", "Sym"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := enumConstName(tt.typeName, tt.raw); got != tt.want { + t.Errorf("enumConstName(%q, %q) = %q, want %q", tt.typeName, tt.raw, got, tt.want) + } + }) + } +} + +// TestConvertEnum_SymbolicValuesGetDistinctNames checks the whole enum path: the +// six operator values must produce six distinct, non-numeric constant names. +func TestConvertEnum_SymbolicValuesGetDistinctNames(t *testing.T) { + _, typeMap := analyzeSpec(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + RelationalOperator: + type: string + enum: ["=", "<>", ">", "<", ">=", "<="] +`) + + td := typeMap["RelationalOperator"] + if td == nil { + t.Fatal("RelationalOperator type not found") + } + if len(td.EnumValues) != 6 { + t.Fatalf("enum values = %d, want 6", len(td.EnumValues)) + } + + want := map[string]string{ + "RelationalOperatorEqual": `"="`, + "RelationalOperatorNotEqual": `"<>"`, + "RelationalOperatorGreaterThan": `">"`, + "RelationalOperatorLessThan": `"<"`, + "RelationalOperatorGreaterThanOrEqual": `">="`, + "RelationalOperatorLessThanOrEqual": `"<="`, + } + seen := make(map[string]bool, len(td.EnumValues)) + for _, ev := range td.EnumValues { + if seen[ev.Name] { + t.Errorf("duplicate constant name %q", ev.Name) + } + seen[ev.Name] = true + literal, ok := want[ev.Name] + if !ok { + t.Errorf("unexpected constant %q = %s", ev.Name, ev.Literal) + continue + } + if ev.Literal != literal { + t.Errorf("%s = %s, want %s", ev.Name, ev.Literal, literal) + } + } +} diff --git a/internal/analyzer/operations.go b/internal/analyzer/operations.go index 31516b6..06eba19 100644 --- a/internal/analyzer/operations.go +++ b/internal/analyzer/operations.go @@ -2,10 +2,12 @@ package analyzer import ( "fmt" + "slices" "strings" highbase "github.com/pb33f/libopenapi/datamodel/high/base" v3high "github.com/pb33f/libopenapi/datamodel/high/v3" + "github.com/pb33f/libopenapi/orderedmap" naming "github.com/giraffesyo/openapi-go-naming" "github.com/parallelworks/openapi-client-generator/internal/ir" @@ -18,24 +20,7 @@ func (a *Analyzer) analyzeOperations(pkg *ir.Package) error { } for path, pathItem := range a.model.Paths.PathItems.FromOldest() { - methods := []struct { - method string - op *v3high.Operation - }{ - {"GET", pathItem.Get}, - {"POST", pathItem.Post}, - {"PUT", pathItem.Put}, - {"DELETE", pathItem.Delete}, - {"PATCH", pathItem.Patch}, - {"HEAD", pathItem.Head}, - {"OPTIONS", pathItem.Options}, - } - - for _, m := range methods { - if m.op == nil { - continue - } - + for _, m := range pathOperations(pathItem) { opDef, err := a.convertOperation(m.method, path, pathItem, m.op) if err != nil { return fmt.Errorf("converting %s %s: %w", m.method, path, err) @@ -47,6 +32,74 @@ func (a *Analyzer) analyzeOperations(pkg *ir.Package) error { return nil } +type pathOperation struct { + method string + op *v3high.Operation +} + +// pathOperations returns the operations of a path item that get a generated +// method. Everything that reasons about operations ahead of analyzeOperations +// walks this same set, so the two can't disagree about what exists. +func pathOperations(pathItem *v3high.PathItem) []pathOperation { + all := []pathOperation{ + {"GET", pathItem.Get}, + {"POST", pathItem.Post}, + {"PUT", pathItem.Put}, + {"DELETE", pathItem.Delete}, + {"PATCH", pathItem.Patch}, + {"HEAD", pathItem.Head}, + {"OPTIONS", pathItem.Options}, + } + return slices.DeleteFunc(all, func(m pathOperation) bool { return m.op == nil }) +} + +// collectMultipartBodySchemas returns the component schema names that a request +// body sends as multipart form data. Only the content type convertRequestBody +// would pick counts: a schema also offered as JSON is encoded as JSON, so its +// binary properties must stay byte slices. +func (a *Analyzer) collectMultipartBodySchemas() map[string]bool { + names := make(map[string]bool) + if a.model.Paths == nil || a.model.Paths.PathItems == nil { + return names + } + + for _, pathItem := range a.model.Paths.PathItems.FromOldest() { + for _, m := range pathOperations(pathItem) { + if m.op.RequestBody == nil { + continue + } + contentType, mediaType := preferredContent(m.op.RequestBody.Content) + if !strings.HasPrefix(contentType, "multipart/") || mediaType == nil || mediaType.Schema == nil { + continue + } + if name := refToSchemaName(mediaType.Schema.GetReference()); name != "" { + names[name] = true + } + } + } + return names +} + +// preferredContent picks the media type a request body is sent as: JSON when the +// spec offers a choice, otherwise the first one it lists. +func preferredContent(content *orderedmap.Map[string, *v3high.MediaType]) (string, *v3high.MediaType) { + if content == nil { + return "", nil + } + var name string + var chosen *v3high.MediaType + for contentType, mediaType := range content.FromOldest() { + isJSON := strings.Contains(contentType, "json") + if name == "" || isJSON { + name, chosen = contentType, mediaType + } + if isJSON { + break + } + } + return name, chosen +} + // convertOperation converts a single OpenAPI operation into an ir.OperationDef. func (a *Analyzer) convertOperation(httpMethod, path string, pathItem *v3high.PathItem, op *v3high.Operation) (*ir.OperationDef, error) { name := a.operationName(httpMethod, path, op) @@ -264,35 +317,42 @@ func (a *Analyzer) convertRequestBody(rb *v3high.RequestBody, nameHint string) ( Description: rb.Description, } - if rb.Content == nil { + // The chosen content type decides how the body is encoded on the wire. + contentType, mediaType := preferredContent(rb.Content) + if contentType == "" { return def, nil } + def.ContentType = contentType + def.TypeName = rawBodyType(contentType, a.resolveMediaTypeSchema(mediaType, nameHint)) - // Prefer application/json content type. - for contentType, mediaType := range rb.Content.FromOldest() { - if strings.Contains(contentType, "json") { - def.ContentType = contentType - def.TypeName = a.resolveMediaTypeSchema(mediaType, nameHint) - break - } - if strings.Contains(contentType, "multipart") { - def.ContentType = contentType - def.IsMultipart = true - def.TypeName = a.resolveMediaTypeSchema(mediaType, nameHint) - break - } - } + return def, nil +} - // If no JSON or multipart found, take the first content type. - if def.ContentType == "" { - for contentType, mediaType := range rb.Content.FromOldest() { - def.ContentType = contentType - def.TypeName = a.resolveMediaTypeSchema(mediaType, nameHint) - break - } - } +// structuredContentType reports whether the generated client can encode a Go +// value into contentType from the schema alone. +func structuredContentType(contentType string) bool { + return strings.Contains(contentType, "json") || + strings.HasPrefix(contentType, "multipart/") || + strings.HasPrefix(contentType, "application/x-www-form-urlencoded") +} - return def, nil +// rawBodyType keeps a body the client cannot structurally encode — XML, say — +// out of a generated struct it would have no encoder for. Handing the caller +// []byte (or a string for text) says what the client can actually send, rather +// than accepting a struct and marshaling it as JSON under a media type that +// promises something else. +func rawBodyType(contentType, typeName string) string { + if typeName == "" || structuredContentType(contentType) { + return typeName + } + switch typeName { + case "string", "[]byte": + return typeName + } + if strings.HasPrefix(contentType, "text/") { + return "string" + } + return "[]byte" } // convertResponses converts operation responses into the OperationDef fields. @@ -367,15 +427,8 @@ func (a *Analyzer) resolveMediaTypeSchema(mt *v3high.MediaType, nameHint string) } // Check for a $ref first. - ref := mt.Schema.GetReference() - if ref != "" { - refName := refToSchemaName(ref) - if refName != "" { - if td, ok := a.typesBySchema[refName]; ok { - return td.Name - } - return naming.Exported(refName) - } + if goType := a.goTypeForRef(mt.Schema.GetReference()); goType != "" { + return goType } schema, err := mt.Schema.BuildSchema() diff --git a/internal/analyzer/schemas.go b/internal/analyzer/schemas.go index 9a2e638..09d40df 100644 --- a/internal/analyzer/schemas.go +++ b/internal/analyzer/schemas.go @@ -23,9 +23,24 @@ func (a *Analyzer) convertSchema(goName, specName string, schema *highbase.Schem return a.convertEnum(goName, schema, nullable) } + // A oneOf/anyOf whose only other member is `type: null` is how OpenAPI 3.1 + // spells "nullable T"; it offers no choice to model, so generate T itself. + if variant, ok := nullableUnionVariant(schema); ok { + return a.convertNullableUnion(goName, specName, schema, variant) + } + if goType, ok := a.uniformUnionGoType(schema, goName); ok { + return &ir.TypeDef{ + Name: goName, + Description: schema.Description, + Kind: ir.TypeKindAlias, + GoType: goType, + IsNullable: nullable, + }, nil + } + // Composition types: allOf, oneOf, anyOf. if len(schema.AllOf) > 0 { - return a.convertAllOf(goName, schema, nullable) + return a.convertAllOf(goName, schema, nullable, a.multipartBodies[specName]) } if len(schema.OneOf) > 0 { return a.convertOneOf(goName, schema, nullable) @@ -38,7 +53,7 @@ func (a *Analyzer) convertSchema(goName, specName string, schema *highbase.Schem switch primaryType { case "object": - return a.convertObject(goName, schema, nullable) + return a.convertObject(goName, schema, nullable, a.multipartBodies[specName]) case "array": return a.convertArray(goName, schema, nullable) case "string", "integer", "number", "boolean": @@ -94,7 +109,7 @@ func (a *Analyzer) convertEnum(goName string, schema *highbase.Schema, nullable // Unique keeps the const unique against package types/other consts — // two values that sanitize to the same identifier ("a-b"/"a b"), or a const // that matches a schema-named type, would otherwise fail to compile. - constName := a.namer.Unique(naming.Exported(goName + " " + raw)) + constName := a.namer.Unique(enumConstName(goName, raw)) td.EnumValues = append(td.EnumValues, &ir.EnumVal{ Name: constName, Literal: literal, @@ -155,7 +170,7 @@ func enumConstLiteral(goType, raw string) (string, bool) { // convertAllOf creates a struct TypeDef from an allOf composition. // $ref entries become embedded fields; inline schemas have their properties merged. -func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable bool) (*ir.TypeDef, error) { +func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable, multipartBody bool) (*ir.TypeDef, error) { td := &ir.TypeDef{ Name: goName, Description: schema.Description, @@ -175,10 +190,7 @@ func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable if refName != "" { // $ref to a known component schema: add as embedded field. - goTypeName := naming.Exported(refName) - if td, ok := a.typesBySchema[refName]; ok { - goTypeName = td.Name - } + goTypeName := a.goTypeForSchemaName(refName) td.Fields = append(td.Fields, &ir.Field{ Name: goTypeName, Type: goTypeName, @@ -214,32 +226,42 @@ func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable continue } - required := requiredSet[propName] - propNullable := isNullable(propSchema) - goType := a.resolveGoType(propSchema, goName+naming.Exported(propName)) - isPointer := !required || propNullable + td.Fields = append(td.Fields, a.convertProperty(goName, propName, propSchema, requiredSet[propName], multipartBody)) + } + } - if isPointer && goType != "any" && !isSliceType(goType) && !isMapType(goType) { - goType = "*" + goType - } + return td, nil +} - td.Fields = append(td.Fields, &ir.Field{ - Name: naming.Exported(propName), - JSONName: propName, - Type: goType, - Description: propSchema.Description, - Required: required, - IsPointer: isPointer, - OmitEmpty: !required, - Deprecated: propSchema.Deprecated != nil && *propSchema.Deprecated, - ReadOnly: propSchema.ReadOnly != nil && *propSchema.ReadOnly, - WriteOnly: propSchema.WriteOnly != nil && *propSchema.WriteOnly, - PrimaryErrorMessage: isPrimaryErrorMessage(propSchema), - }) +// convertProperty converts one object property into a struct field. multipartBody +// marks a schema sent as multipart form data, whose binary properties are file +// parts rather than byte slices. +func (a *Analyzer) convertProperty(goName, propName string, propSchema *highbase.Schema, required, multipartBody bool) *ir.Field { + goType := a.resolveGoType(propSchema, goName+naming.Exported(propName)) + if multipartBody { + if fileType, ok := formFileType(propSchema); ok { + goType = fileType } } - return td, nil + isPointer := !required || isNullable(propSchema) + if isPointer && goType != "any" && !isSliceType(goType) && !isMapType(goType) { + goType = "*" + goType + } + + return &ir.Field{ + Name: naming.Exported(propName), + JSONName: propName, + Type: goType, + Description: propSchema.Description, + Required: required, + IsPointer: isPointer, + OmitEmpty: !required, + Deprecated: propSchema.Deprecated != nil && *propSchema.Deprecated, + ReadOnly: propSchema.ReadOnly != nil && *propSchema.ReadOnly, + WriteOnly: propSchema.WriteOnly != nil && *propSchema.WriteOnly, + PrimaryErrorMessage: isPrimaryErrorMessage(propSchema), + } } // isPrimaryErrorMessage reports whether a property schema carries Kiota's @@ -292,12 +314,7 @@ func (a *Analyzer) convertUnion(goName string, schema *highbase.Schema, variants discMapping = make(map[string]string) for k, v := range schema.Discriminator.Mapping.FromOldest() { // v is a $ref like "#/components/schemas/Circle" - refName := refToSchemaName(v) - goTypeName := naming.Exported(refName) - if existing, ok := a.typesBySchema[refName]; ok { - goTypeName = existing.Name - } - td.Discriminator.Mapping[k] = goTypeName + td.Discriminator.Mapping[k] = a.goTypeForSchemaName(refToSchemaName(v)) discMapping[v] = k } } @@ -312,29 +329,29 @@ func (a *Analyzer) convertUnion(goName string, schema *highbase.Schema, variants if refName == "" { continue } - goTypeName := naming.Exported(refName) - if existing, ok := a.typesBySchema[refName]; ok { - goTypeName = existing.Name - } - td.Discriminator.Mapping[refName] = goTypeName + td.Discriminator.Mapping[refName] = a.goTypeForSchemaName(refName) discMapping[ref] = refName } } } for _, proxy := range variants { + // A `type: null` member says the union is nullable; it is not one of the + // shapes the value can take, so it gets no variant of its own. + if isNullVariant(proxy) { + continue + } + ref := proxy.GetReference() refName := refToSchemaName(ref) - var typeName string + typeName := "any" if refName != "" { - typeName = naming.Exported(refName) - if existing, ok := a.typesBySchema[refName]; ok { - typeName = existing.Name - } - } else { - // Inline variant: use "any" as the type. - typeName = "any" + typeName = a.goTypeForSchemaName(refName) + } else if variantSchema, err := proxy.BuildSchema(); err == nil && variantSchema != nil { + // An inline variant still has a Go type; without one it would decode + // into nothing and the payloads it covers would fail to unmarshal. + typeName = a.resolveGoType(variantSchema, suffixHint(goName, "Variant")) } variant := &ir.UnionVariant{ @@ -353,7 +370,7 @@ func (a *Analyzer) convertUnion(goName string, schema *highbase.Schema, variants } // convertObject creates a struct TypeDef from an object schema. -func (a *Analyzer) convertObject(goName string, schema *highbase.Schema, nullable bool) (*ir.TypeDef, error) { +func (a *Analyzer) convertObject(goName string, schema *highbase.Schema, nullable, multipartBody bool) (*ir.TypeDef, error) { // If no defined properties and additionalProperties is set, generate a map alias. hasProperties := schema.Properties != nil && schema.Properties.Len() > 0 if !hasProperties && allowsAdditionalProperties(schema) { @@ -385,28 +402,7 @@ func (a *Analyzer) convertObject(goName string, schema *highbase.Schema, nullabl continue } - required := requiredSet[propName] - propNullable := isNullable(propSchema) - goType := a.resolveGoType(propSchema, goName+naming.Exported(propName)) - isPointer := !required || propNullable - - if isPointer && goType != "any" && !isSliceType(goType) && !isMapType(goType) { - goType = "*" + goType - } - - td.Fields = append(td.Fields, &ir.Field{ - Name: naming.Exported(propName), - JSONName: propName, - Type: goType, - Description: propSchema.Description, - Required: required, - IsPointer: isPointer, - OmitEmpty: !required, - Deprecated: propSchema.Deprecated != nil && *propSchema.Deprecated, - ReadOnly: propSchema.ReadOnly != nil && *propSchema.ReadOnly, - WriteOnly: propSchema.WriteOnly != nil && *propSchema.WriteOnly, - PrimaryErrorMessage: isPrimaryErrorMessage(propSchema), - }) + td.Fields = append(td.Fields, a.convertProperty(goName, propName, propSchema, requiredSet[propName], multipartBody)) } // If the object has both properties and additionalProperties, add an extra field. @@ -450,6 +446,29 @@ func catchAllFieldName(fields []*ir.Field) string { return name } +// formFileType returns the Go type for a multipart property carrying file content. +func formFileType(schema *highbase.Schema) (string, bool) { + if variant, ok := nullableUnionVariant(schema); ok { + return formFileType(variant) + } + if isBinarySchema(schema) { + return "FormFile", true + } + if primaryType(schema) == "array" && schema.Items != nil && schema.Items.IsA() { + items, err := schema.Items.A.BuildSchema() + if err == nil && items != nil && isBinarySchema(items) { + return "[]FormFile", true + } + } + return "", false +} + +// isBinarySchema reports whether a schema is `type: string, format: binary`, +// which inside a multipart body means file content rather than text. +func isBinarySchema(schema *highbase.Schema) bool { + return primaryType(schema) == "string" && schema.Format == "binary" +} + // convertAdditionalPropertiesMap creates a map alias when an object has // additionalProperties but no defined properties. func (a *Analyzer) convertAdditionalPropertiesMap(goName string, schema *highbase.Schema, nullable bool) (*ir.TypeDef, error) { @@ -524,18 +543,17 @@ func (a *Analyzer) convertPrimitive(goName, primaryType string, schema *highbase // pass "" when no context is available. func (a *Analyzer) resolveGoType(schema *highbase.Schema, nameHint string) string { // Check if this schema is a $ref pointing to a known component schema. - if schema.ParentProxy != nil { - ref := schema.ParentProxy.GetReference() - if ref != "" { - refName := refToSchemaName(ref) - if refName != "" { - if td, ok := a.typesBySchema[refName]; ok { - return td.Name - } - // Not yet converted, use the Go name directly. - return naming.Exported(refName) - } - } + if goType := a.refGoType(schema); goType != "" { + return goType + } + + // "nullable T" spelled as a union resolves to T; the pointer that carries the + // null comes from the field or parameter being optional/nullable. + if variant, ok := nullableUnionVariant(schema); ok { + return a.resolveGoType(variant, nameHint) + } + if goType, ok := a.uniformUnionGoType(schema, nameHint); ok { + return goType } // Inline oneOf/anyOf: synthesize a named union so $ref variants stay typed. @@ -597,20 +615,34 @@ func (a *Analyzer) synthesizeInlineUnion(schema *highbase.Schema, nameHint strin return "", false } - refs := make([]string, 0, len(variants)) - hasRef := false + // Key on what each member resolves to rather than on its $ref, so two inline + // unions of different shapes don't collapse onto one synthesized type. + members := make([]string, 0, len(variants)) + typed := false for _, proxy := range variants { - ref := proxy.GetReference() - if ref != "" { - hasRef = true + if isNullVariant(proxy) { + continue } - refs = append(refs, ref) + if ref := proxy.GetReference(); ref != "" { + members = append(members, ref) + typed = true + continue + } + variantSchema, err := proxy.BuildSchema() + if err != nil || variantSchema == nil { + return "", false + } + goType := a.resolveGoType(variantSchema, suffixHint(nameHint, "Variant")) + members = append(members, goType) + typed = typed || goType != "any" } - if !hasRef { + // A union whose members all decode into any is just any; naming it would add a + // type that carries no more information than the bare interface. + if !typed || len(members) < 2 { return "", false } - key := kind + "|" + strings.Join(refs, ",") + key := kind + "|" + strings.Join(members, ",") if schema.Discriminator != nil { key += "|" + schema.Discriminator.PropertyName } @@ -649,12 +681,147 @@ func primaryType(schema *highbase.Schema) string { } // isNullable checks whether a schema is nullable. In OpenAPI 3.1, this is -// indicated by type: ["string", "null"]. In 3.0, it's nullable: true. +// indicated by type: ["string", "null"] or a {"type": "null"} member of a +// oneOf/anyOf. In 3.0, it's nullable: true. func isNullable(schema *highbase.Schema) bool { if schema.Nullable != nil && *schema.Nullable { return true } - return slices.Contains(schema.Type, "null") + if slices.Contains(schema.Type, "null") { + return true + } + return slices.ContainsFunc(unionVariants(schema), isNullVariant) +} + +// unionVariants returns a schema's oneOf variants, or its anyOf variants when it +// has no oneOf. +func unionVariants(schema *highbase.Schema) []*highbase.SchemaProxy { + if len(schema.OneOf) > 0 { + return schema.OneOf + } + return schema.AnyOf +} + +// isNullVariant reports whether a union member is the bare {"type": "null"} +// schema that makes the union nullable. +func isNullVariant(proxy *highbase.SchemaProxy) bool { + schema, err := proxy.BuildSchema() + if err != nil || schema == nil { + return false + } + return primaryType(schema) == "" && slices.Contains(schema.Type, "null") +} + +// nullableUnionVariant returns the sole non-null member of a oneOf/anyOf — the +// OpenAPI 3.1 spelling of "nullable T"; a real choice returns ok=false. +func nullableUnionVariant(schema *highbase.Schema) (*highbase.Schema, bool) { + variants := unionVariants(schema) + if len(variants) == 0 { + return nil, false + } + + var only *highbase.Schema + for _, proxy := range variants { + if isNullVariant(proxy) { + continue + } + if only != nil { + return nil, false + } + variant, err := proxy.BuildSchema() + if err != nil || variant == nil { + return nil, false + } + only = variant + } + if only == nil { + return nil, false + } + return only, true +} + +// uniformUnionGoType returns the Go type of a oneOf/anyOf whose non-null members +// all resolve to it — several refinements of one type are that type, not a choice. +func (a *Analyzer) uniformUnionGoType(schema *highbase.Schema, nameHint string) (string, bool) { + variants := unionVariants(schema) + if len(variants) < 2 { + return "", false + } + + // Resolving a member can synthesize a type for it, so use the same hint the + // union path would: probing must not let a member claim the name the union + // itself will need when the members turn out to disagree. + memberHint := suffixHint(nameHint, "Variant") + + goType := "" + for _, proxy := range variants { + if isNullVariant(proxy) { + continue + } + variant, err := proxy.BuildSchema() + if err != nil || variant == nil { + return "", false + } + resolved := a.resolveGoType(variant, memberHint) + if resolved == "any" || (goType != "" && resolved != goType) { + return "", false + } + goType = resolved + } + return goType, goType != "" +} + +// convertNullableUnion converts the collapsed "nullable T" union at goName; a +// $ref variant aliases the type it points at. +func (a *Analyzer) convertNullableUnion(goName, specName string, schema, variant *highbase.Schema) (*ir.TypeDef, error) { + nullable := isNullable(schema) + if goType := a.refGoType(variant); goType != "" { + return &ir.TypeDef{ + Name: goName, + Description: schema.Description, + Kind: ir.TypeKindAlias, + GoType: goType, + IsNullable: nullable, + }, nil + } + + td, err := a.convertSchema(goName, specName, variant) + if err != nil { + return nil, err + } + td.IsNullable = nullable + if td.Description == "" { + td.Description = schema.Description + } + return td, nil +} + +// refGoType returns the Go type name a $ref schema resolves to, or "" when the +// schema is not a reference to a component schema. +func (a *Analyzer) refGoType(schema *highbase.Schema) string { + if schema.ParentProxy == nil { + return "" + } + return a.goTypeForRef(schema.ParentProxy.GetReference()) +} + +// goTypeForRef returns the Go type name a "#/components/schemas/..." reference +// resolves to, or "" when it points elsewhere. +func (a *Analyzer) goTypeForRef(ref string) string { + refName := refToSchemaName(ref) + if refName == "" { + return "" + } + return a.goTypeForSchemaName(refName) +} + +// goTypeForSchemaName returns the Go type name of a component schema, falling +// back to its exported spelling when it has not been converted yet. +func (a *Analyzer) goTypeForSchemaName(refName string) string { + if td, ok := a.typesBySchema[refName]; ok { + return td.Name + } + return naming.Exported(refName) } // goTypeForPrimitive maps an OpenAPI type + format to a Go type. diff --git a/internal/analyzer/schemas_nullable_union_test.go b/internal/analyzer/schemas_nullable_union_test.go new file mode 100644 index 0000000..f8b6bb9 --- /dev/null +++ b/internal/analyzer/schemas_nullable_union_test.go @@ -0,0 +1,457 @@ +package analyzer + +import ( + "os" + "path/filepath" + "testing" + + "github.com/parallelworks/openapi-client-generator/internal/ir" + "github.com/parallelworks/openapi-client-generator/internal/parser" +) + +// analyzeSpec analyzes an inline spec and returns the package alongside its +// types keyed by Go name. +func analyzeSpec(t *testing.T, spec string) (*ir.Package, map[string]*ir.TypeDef) { + t.Helper() + specPath := filepath.Join(t.TempDir(), "spec.yaml") + if err := os.WriteFile(specPath, []byte(spec), 0o644); err != nil { + t.Fatalf("writing spec: %v", err) + } + result, err := parser.Parse(specPath, parser.Config{}) + if err != nil { + t.Fatalf("Parse: %v", err) + } + pkg, err := New(result.Model).Analyze("test") + if err != nil { + t.Fatalf("Analyze: %v", err) + } + typeMap := make(map[string]*ir.TypeDef, len(pkg.Types)) + for _, td := range pkg.Types { + typeMap[td.Name] = td + } + return pkg, typeMap +} + +const nullableUnionSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /recipes: + get: + operationId: listRecipes + parameters: + - name: search + in: query + required: false + schema: + anyOf: [{ type: string }, { type: "null" }] + title: Search + - name: limit + in: query + required: true + schema: + anyOf: [{ type: integer, format: int32 }, { type: "null" }] + - name: owner + in: query + required: false + schema: + anyOf: [{ $ref: "#/components/schemas/Person" }, { type: "null" }] + - name: either + in: query + required: false + schema: + anyOf: [{ type: string }, { type: integer }, { type: "null" }] + - name: cookbook + in: query + required: false + schema: + anyOf: + - { type: string, format: uuid4 } + - { type: string } + - { type: "null" } + - name: categories + in: query + required: false + schema: + anyOf: + - type: array + items: + anyOf: [{ type: string, format: uuid4 }, { type: string }] + - { type: "null" } + responses: + "200": { description: ok } +components: + schemas: + Person: + type: object + properties: + name: { type: string } + MaybeName: + anyOf: [{ type: string }, { type: "null" }] + MaybePerson: + oneOf: [{ $ref: "#/components/schemas/Person" }, { type: "null" }] + Recipe: + type: object + required: [name] + properties: + name: + anyOf: [{ type: string }, { type: "null" }] + tags: + anyOf: [{ type: array, items: { type: string } }, { type: "null" }] + cook: + anyOf: [{ $ref: "#/components/schemas/Person" }, { type: "null" }] +` + +// TestRequestBodyContentType checks which media type an operation sends its body +// as, now that the choice decides the encoding rather than just a header. +func TestRequestBodyContentType(t *testing.T) { + pkg, _ := analyzeSpec(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /upload: + post: + operationId: upload + requestBody: + content: + multipart/form-data: + schema: { type: object } + responses: { "204": { description: ok } } + /both: + post: + operationId: both + requestBody: + content: + multipart/form-data: + schema: { type: object } + application/json: + schema: { type: object } + responses: { "204": { description: ok } } + /raw: + post: + operationId: raw + requestBody: + content: + application/octet-stream: + schema: { type: string, format: binary } + responses: { "204": { description: ok } } +`) + + byName := make(map[string]*ir.OperationDef, len(pkg.Operations)) + for _, op := range pkg.Operations { + byName[op.Name] = op + } + + tests := []struct { + op string + want string + }{ + {"Upload", "multipart/form-data"}, + // JSON wins when the spec offers a choice, whatever order it lists them in. + {"Both", "application/json"}, + {"Raw", "application/octet-stream"}, + } + for _, tt := range tests { + op := byName[tt.op] + if op == nil { + t.Errorf("operation %q not found", tt.op) + continue + } + if op.RequestBody == nil { + t.Errorf("%s has no request body", tt.op) + continue + } + if op.RequestBody.ContentType != tt.want { + t.Errorf("%s content type = %q, want %q", tt.op, op.RequestBody.ContentType, tt.want) + } + } +} + +// TestNullableUnion_QueryParamsResolveToTheVariantType covers issue #16: a query +// parameter typed `anyOf: [{type: string}, {type: "null"}]` used to land in the +// params struct as *any, which callers had no way to construct a value for. +func TestNullableUnion_QueryParamsResolveToTheVariantType(t *testing.T) { + pkg, _ := analyzeSpec(t, nullableUnionSpec) + + if len(pkg.Operations) != 1 { + t.Fatalf("operations = %d, want 1", len(pkg.Operations)) + } + params := make(map[string]*ir.ParamDef) + for _, p := range pkg.Operations[0].QueryParams { + params[p.OrigName] = p + } + + tests := []struct { + param string + want string + }{ + {"search", "string"}, + {"limit", "int32"}, + {"owner", "Person"}, + // Variants that are refinements of one Go type collapse to that type. + {"cookbook", "string"}, + {"categories", "[]string"}, + // A union with a real choice keeps its union handling. + {"either", "any"}, + } + for _, tt := range tests { + p := params[tt.param] + if p == nil { + t.Errorf("query param %q not found", tt.param) + continue + } + if p.Type != tt.want { + t.Errorf("param %q type = %q, want %q", tt.param, p.Type, tt.want) + } + } +} + +// TestNullableUnion_ComponentSchemasCollapse checks the same collapse when the +// union is a named component schema rather than an inline parameter schema. +func TestNullableUnion_ComponentSchemasCollapse(t *testing.T) { + _, typeMap := analyzeSpec(t, nullableUnionSpec) + + name := typeMap["MaybeName"] + if name == nil { + t.Fatal("MaybeName type not found") + } + if name.Kind != ir.TypeKindAlias || name.GoType != "string" { + t.Errorf("MaybeName = %v %q, want alias string", name.Kind, name.GoType) + } + if !name.IsNullable { + t.Error("MaybeName.IsNullable = false, want true") + } + + // A $ref variant aliases the type it points at instead of restating it. + person := typeMap["MaybePerson"] + if person == nil { + t.Fatal("MaybePerson type not found") + } + if person.Kind != ir.TypeKindAlias || person.GoType != "Person" { + t.Errorf("MaybePerson = %v %q, want alias Person", person.Kind, person.GoType) + } +} + +// TestNullableUnion_StructFieldsArePointersToTheVariantType checks that a +// collapsed property still carries the null through a pointer, including when +// the property is required. +func TestNullableUnion_StructFieldsArePointersToTheVariantType(t *testing.T) { + _, typeMap := analyzeSpec(t, nullableUnionSpec) + + recipe := typeMap["Recipe"] + if recipe == nil { + t.Fatal("Recipe type not found") + } + fields := make(map[string]*ir.Field, len(recipe.Fields)) + for _, f := range recipe.Fields { + fields[f.JSONName] = f + } + + tests := []struct { + field string + want string + }{ + // Required, but nullable through the union, so still a pointer. + {"name", "*string"}, + {"tags", "[]string"}, + {"cook", "*Person"}, + } + for _, tt := range tests { + f := fields[tt.field] + if f == nil { + t.Errorf("Recipe field %q not found", tt.field) + continue + } + if f.Type != tt.want { + t.Errorf("Recipe.%s type = %q, want %q", tt.field, f.Type, tt.want) + } + } +} + +// TestUnion_InlineVariantsGetNamedTypes covers the rest of what issue #16 asked +// for: a union whose members are inline schemas used to degrade to a bare any, +// which callers could neither construct nor decode into. +func TestUnion_InlineVariantsGetNamedTypes(t *testing.T) { + _, typeMap := analyzeSpec(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Person: + type: object + properties: + name: { type: string } + Filter: + type: object + properties: + value: + anyOf: [{ type: string }, { type: array, items: { type: string } }, { type: "null" }] + title: Value + loc: + type: array + items: + anyOf: [{ type: string }, { type: integer }] + who: + anyOf: [{ $ref: "#/components/schemas/Person" }, { type: string }] +`) + + filter := typeMap["Filter"] + if filter == nil { + t.Fatal("Filter type not found") + } + fields := make(map[string]*ir.Field, len(filter.Fields)) + for _, f := range filter.Fields { + fields[f.JSONName] = f + } + + for _, tt := range []struct{ field, want string }{ + {"value", "*Value"}, + {"loc", "[]FilterLocItem"}, + {"who", "*FilterWho"}, + } { + f := fields[tt.field] + if f == nil { + t.Errorf("Filter field %q not found", tt.field) + continue + } + if f.Type != tt.want { + t.Errorf("Filter.%s type = %q, want %q", tt.field, f.Type, tt.want) + } + } + + // The `type: null` member says the union is nullable; it is not a shape the + // value can take, so it gets no variant. + value := typeMap["Value"] + if value == nil { + t.Fatal("synthesized union Value not found") + } + if value.Kind != ir.TypeKindUnion { + t.Fatalf("Value kind = %v, want union", value.Kind) + } + got := make([]string, 0, len(value.UnionTypes)) + for _, v := range value.UnionTypes { + got = append(got, v.TypeName) + } + if len(got) != 2 || got[0] != "string" || got[1] != "[]string" { + t.Errorf("Value variants = %v, want [string []string]", got) + } +} + +// TestMultipartBody_BinaryPropertiesAreFiles checks that a binary property of a +// multipart body is generated as a file the caller can name, while the same +// format elsewhere stays a byte slice. +func TestMultipartBody_BinaryPropertiesAreFiles(t *testing.T) { + _, typeMap := analyzeSpec(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /image: + put: + operationId: putImage + requestBody: + required: true + content: + multipart/form-data: + schema: { $ref: "#/components/schemas/ImageUpload" } + responses: { "204": { description: ok } } + /doc: + put: + operationId: putDoc + requestBody: + required: true + content: + application/json: + schema: { $ref: "#/components/schemas/JSONUpload" } + responses: { "204": { description: ok } } +components: + schemas: + ImageUpload: + type: object + required: [image] + properties: + image: { type: string, format: binary } + thumbnail: + anyOf: [{ type: string, format: binary }, { type: "null" }] + attachments: { type: array, items: { type: string, format: binary } } + extension: { type: string } + JSONUpload: + type: object + required: [blob] + properties: + blob: { type: string, format: binary } +`) + + upload := typeMap["ImageUpload"] + if upload == nil { + t.Fatal("ImageUpload type not found") + } + fields := make(map[string]*ir.Field, len(upload.Fields)) + for _, f := range upload.Fields { + fields[f.JSONName] = f + } + for _, tt := range []struct{ field, want string }{ + {"image", "FormFile"}, + {"thumbnail", "*FormFile"}, + {"attachments", "[]FormFile"}, + {"extension", "*string"}, + } { + f := fields[tt.field] + if f == nil { + t.Errorf("ImageUpload field %q not found", tt.field) + continue + } + if f.Type != tt.want { + t.Errorf("ImageUpload.%s type = %q, want %q", tt.field, f.Type, tt.want) + } + } + + // Outside a multipart body, binary is still a byte slice. + jsonUpload := typeMap["JSONUpload"] + if jsonUpload == nil { + t.Fatal("JSONUpload type not found") + } + if got := jsonUpload.Fields[0].Type; got != "[]byte" { + t.Errorf("JSONUpload.blob type = %q, want []byte", got) + } +} + +// TestNullableUnion_SelfReferentialAliasesCompile pins that a schema whose only +// non-null member refers back to itself does not emit `type A = B; type B = A`, +// which Go rejects as an invalid recursive type. +func TestNullableUnion_SelfReferentialAliasesCompile(t *testing.T) { + _, typeMap := analyzeSpec(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Loop: + anyOf: [{ $ref: "#/components/schemas/Loop" }, { type: "null" }] + A: + anyOf: [{ $ref: "#/components/schemas/B" }, { type: "null" }] + B: + anyOf: [{ $ref: "#/components/schemas/A" }, { type: "null" }] + C: + anyOf: [{ $ref: "#/components/schemas/D" }, { type: "null" }] + D: + anyOf: [{ type: array, items: { $ref: "#/components/schemas/C" } }, { type: "null" }] +`) + + // Follow every alias chain; none may return to a name already on it. + for _, start := range []string{"Loop", "A", "B", "C", "D"} { + td := typeMap[start] + if td == nil { + t.Errorf("%s type not found", start) + continue + } + seen := map[string]bool{start: true} + for td != nil && td.Kind == ir.TypeKindAlias { + next := typeMap[aliasTarget(td.GoType)] + if next == nil { + break + } + if seen[next.Name] { + t.Errorf("alias chain from %s cycles back to %s", start, next.Name) + break + } + seen[next.Name] = true + td = next + } + } +} diff --git a/internal/generator/e2e_request_body_test.go b/internal/generator/e2e_request_body_test.go new file mode 100644 index 0000000..70172a7 --- /dev/null +++ b/internal/generator/e2e_request_body_test.go @@ -0,0 +1,473 @@ +package generator + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/parallelworks/openapi-client-generator/internal/analyzer" + "github.com/parallelworks/openapi-client-generator/internal/parser" +) + +const requestBodySpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /recipes/{slug}/image: + put: + operationId: updateRecipeImage + parameters: [{ name: slug, in: path, required: true, schema: { type: string } }] + requestBody: + required: true + content: + multipart/form-data: + schema: { $ref: "#/components/schemas/ImageUpload" } + responses: + "200": + description: ok + content: + application/json: + schema: { $ref: "#/components/schemas/UploadResult" } + /token: + post: + operationId: createToken + requestBody: + required: true + content: + application/x-www-form-urlencoded: + schema: { $ref: "#/components/schemas/TokenRequest" } + responses: + "200": + description: ok + content: + application/json: + schema: { $ref: "#/components/schemas/UploadResult" } + /recipes: + post: + operationId: createRecipe + requestBody: + required: true + content: + application/json: + schema: { $ref: "#/components/schemas/TokenRequest" } + responses: + "200": + description: ok + content: + application/json: + schema: { $ref: "#/components/schemas/UploadResult" } + /notes: + post: + operationId: createNote + requestBody: + required: true + content: + application/xml: + schema: { $ref: "#/components/schemas/ImageMeta" } + responses: + "200": + description: ok + content: + application/json: + schema: { $ref: "#/components/schemas/UploadResult" } + /labels: + post: + operationId: createLabel + requestBody: + required: true + content: + multipart/form-data: + schema: { $ref: "#/components/schemas/Label" } + responses: + "200": + description: ok + content: + application/json: + schema: { $ref: "#/components/schemas/UploadResult" } +components: + schemas: + ImageUpload: + type: object + required: [image, extension] + properties: + image: { type: string, format: binary } + extension: { type: string } + attempt: { type: integer, format: int32 } + tags: { type: array, items: { type: string } } + meta: { $ref: "#/components/schemas/ImageMeta" } + attachments: { type: array, items: { type: string, format: binary } } + ImageMeta: + type: object + required: [source] + properties: + source: { type: string } + TokenRequest: + type: object + required: [username, password] + properties: + username: { type: string } + password: { type: string } + scopes: { type: array, items: { type: string } } + client: { $ref: "#/components/schemas/ImageMeta" } + Label: + type: object + required: [name] + properties: + name: { type: string } + additionalProperties: { type: string } + UploadResult: + type: object + properties: + ok: { type: boolean } +` + +// TestE2E_RequestBodyContentTypes covers issue #17: an operation whose spec +// declares multipart/form-data used to marshal its body as JSON and send it with +// a Content-Type of application/json. The generated client is compiled and RUN +// against a real server that parses the request, so a template that encodes the +// wrong thing fails here rather than at a user's API. +func TestE2E_RequestBodyContentTypes(t *testing.T) { + specDir := t.TempDir() + specPath := filepath.Join(specDir, "spec.yaml") + if err := os.WriteFile(specPath, []byte(requestBodySpec), 0o644); err != nil { + t.Fatalf("writing spec: %v", err) + } + + result, err := parser.Parse(specPath, parser.Config{}) + if err != nil { + t.Fatalf("Parse: %v", err) + } + + pkg, err := analyzer.New(result.Model).Analyze("uploads") + if err != nil { + t.Fatalf("Analyze: %v", err) + } + + gen, err := New(pkg) + if err != nil { + t.Fatalf("New generator: %v", err) + } + files, err := gen.Generate() + if err != nil { + t.Fatalf("Generate: %v", err) + } + + tmpDir := t.TempDir() + goMod := []byte("module requestbody-e2e-test\n\ngo 1.25.5\n") + if err := os.WriteFile(filepath.Join(tmpDir, "go.mod"), goMod, 0o644); err != nil { + t.Fatalf("writing go.mod: %v", err) + } + if err := WriteFiles(tmpDir, files); err != nil { + t.Fatalf("WriteFiles: %v", err) + } + + runtimeTest := []byte(`package uploads + +import ( + "encoding/json" + "io" + "mime" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" +) + +type capture struct { + contentType string + body []byte +} + +// serve records the one request the client makes and answers it with an empty +// JSON object. +func serve(t *testing.T, got *capture) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("reading request body: %v", err) + } + got.contentType = r.Header.Get("Content-Type") + got.body = body + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(` + "`" + `{"ok":true}` + "`" + `)) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestMultipartBodyIsSentAsMultipart(t *testing.T) { + var got capture + srv := serve(t, &got) + + client := NewClient(srv.URL) + res, err := client.UpdateRecipeImage(t.Context(), "carrot-cake", ImageUpload{ + Image: FormFile{ + Filename: "carrot cake.png", + ContentType: "image/png", + Content: []byte("\x89PNG\r\n binary \x00 payload"), + }, + Extension: "png", + Attempt: ptr(int32(2)), + Tags: []string{"dinner", "quick"}, + Meta: &ImageMeta{Source: "phone"}, + Attachments: []FormFile{{Filename: "notes.txt", Content: []byte("first")}, {Content: []byte("second")}}, + }) + if err != nil { + t.Fatalf("UpdateRecipeImage: %v", err) + } + if res == nil || res.Ok == nil || !*res.Ok { + t.Errorf("response = %+v, want ok=true", res) + } + + mediaType, params, err := mime.ParseMediaType(got.contentType) + if err != nil { + t.Fatalf("parsing Content-Type %q: %v", got.contentType, err) + } + if mediaType != "multipart/form-data" { + t.Fatalf("Content-Type = %q, want multipart/form-data", mediaType) + } + if params["boundary"] == "" { + t.Fatal("Content-Type carries no boundary") + } + + // Parse the body the way a real server would. + req := httptest.NewRequest("PUT", "/", strings.NewReader(string(got.body))) + req.Header.Set("Content-Type", got.contentType) + if err := req.ParseMultipartForm(1 << 20); err != nil { + t.Fatalf("ParseMultipartForm: %v", err) + } + + fileHeaders := req.MultipartForm.File["image"] + if len(fileHeaders) != 1 { + t.Fatalf("image file parts = %d, want 1", len(fileHeaders)) + } + if name := fileHeaders[0].Filename; name != "carrot cake.png" { + t.Errorf("image filename = %q, want carrot cake.png", name) + } + if ct := fileHeaders[0].Header.Get("Content-Type"); ct != "image/png" { + t.Errorf("image part Content-Type = %q, want image/png", ct) + } + f, err := fileHeaders[0].Open() + if err != nil { + t.Fatalf("opening image part: %v", err) + } + defer f.Close() + content, err := io.ReadAll(f) + if err != nil { + t.Fatalf("reading image part: %v", err) + } + if string(content) != "\x89PNG\r\n binary \x00 payload" { + t.Errorf("image part = %q, want the raw bytes verbatim", content) + } + + values := req.MultipartForm.Value + if got := values["extension"]; len(got) != 1 || got[0] != "png" { + t.Errorf("extension part = %v, want [png]", got) + } + if got := values["attempt"]; len(got) != 1 || got[0] != "2" { + t.Errorf("attempt part = %v, want [2]", got) + } + if got := values["tags"]; len(got) != 2 || got[0] != "dinner" || got[1] != "quick" { + t.Errorf("tags parts = %v, want one part per element", got) + } + if got := values["meta"]; len(got) != 1 || got[0] != ` + "`" + `{"source":"phone"}` + "`" + ` { + t.Errorf("meta part = %v, want the object as JSON", got) + } + + // An array of files becomes one file part per element, and a file with no + // name of its own falls back to the property name. + attachments := req.MultipartForm.File["attachments"] + if len(attachments) != 2 { + t.Fatalf("attachment parts = %d, want 2", len(attachments)) + } + if attachments[0].Filename != "notes.txt" { + t.Errorf("attachment[0] filename = %q, want notes.txt", attachments[0].Filename) + } + if attachments[1].Filename != "attachments" { + t.Errorf("attachment[1] filename = %q, want the property name", attachments[1].Filename) + } + if ct := attachments[1].Header.Get("Content-Type"); ct != "application/octet-stream" { + t.Errorf("attachment[1] Content-Type = %q, want application/octet-stream", ct) + } +} + +func TestOptionalMultipartFieldsAreOmitted(t *testing.T) { + var got capture + srv := serve(t, &got) + + client := NewClient(srv.URL) + if _, err := client.UpdateRecipeImage(t.Context(), "carrot-cake", ImageUpload{ + Image: FormFile{Content: []byte("data")}, + Extension: "png", + }); err != nil { + t.Fatalf("UpdateRecipeImage: %v", err) + } + + req := httptest.NewRequest("PUT", "/", strings.NewReader(string(got.body))) + req.Header.Set("Content-Type", got.contentType) + if err := req.ParseMultipartForm(1 << 20); err != nil { + t.Fatalf("ParseMultipartForm: %v", err) + } + for _, unset := range []string{"attempt", "meta"} { + if _, ok := req.MultipartForm.Value[unset]; ok { + t.Errorf("unset optional field %q was sent", unset) + } + } + if _, ok := req.MultipartForm.File["attachments"]; ok { + t.Error("unset optional file field was sent") + } + // A file with no name of its own is still a file part, not a text part. + if files := req.MultipartForm.File["image"]; len(files) != 1 || files[0].Filename != "image" { + t.Errorf("image parts = %v, want one named after the property", files) + } +} + +func TestFormURLEncodedBodyIsSentAsFormValues(t *testing.T) { + var got capture + srv := serve(t, &got) + + client := NewClient(srv.URL) + if _, err := client.CreateToken(t.Context(), TokenRequest{ + Username: "ada", + Password: "a b&c=d", + Scopes: []string{"read", "write"}, + Client: &ImageMeta{Source: "cli"}, + }); err != nil { + t.Fatalf("CreateToken: %v", err) + } + + if mediaType, _, err := mime.ParseMediaType(got.contentType); err != nil || mediaType != "application/x-www-form-urlencoded" { + t.Fatalf("Content-Type = %q, want application/x-www-form-urlencoded", got.contentType) + } + values, err := url.ParseQuery(string(got.body)) + if err != nil { + t.Fatalf("parsing form body %q: %v", got.body, err) + } + if values.Get("username") != "ada" { + t.Errorf("username = %q, want ada", values.Get("username")) + } + if values.Get("password") != "a b&c=d" { + t.Errorf("password = %q, want the value escaped, not split", values.Get("password")) + } + // An array is one pair per element (form/explode, the OpenAPI default), not a + // single comma-joined value. + if got := values["scopes"]; len(got) != 2 || got[0] != "read" || got[1] != "write" { + t.Errorf("scopes = %v, want one pair per element", got) + } + if got := values.Get("client"); got != ` + "`" + `{"source":"cli"}` + "`" + ` { + t.Errorf("client = %q, want the object as JSON", got) + } + // A space is '+' in x-www-form-urlencoded, not the query string's %20. + if !strings.Contains(string(got.body), "password=a+b") { + t.Errorf("body = %q, want a space encoded as '+'", got.body) + } +} + +// TestSharedBodySchemaSentBothWays covers a schema a spec offers as both JSON and +// multipart: the generated code has to compile and each operation has to send the +// media type it declared. +func TestSharedBodySchemaSentBothWays(t *testing.T) { + var got capture + srv := serve(t, &got) + + client := NewClient(srv.URL) + if _, err := client.CreateRecipe(t.Context(), TokenRequest{Username: "ada", Password: "p"}); err != nil { + t.Fatalf("CreateRecipe: %v", err) + } + if mediaType, _, _ := mime.ParseMediaType(got.contentType); mediaType != "application/json" { + t.Errorf("Content-Type = %q, want application/json", got.contentType) + } +} + +func TestJSONBodyStillSentAsJSON(t *testing.T) { + var got capture + srv := serve(t, &got) + + client := NewClient(srv.URL) + if _, err := client.CreateRecipe(t.Context(), TokenRequest{ + Username: "ada", + Password: "secret", + }); err != nil { + t.Fatalf("CreateRecipe: %v", err) + } + + if mediaType, _, err := mime.ParseMediaType(got.contentType); err != nil || mediaType != "application/json" { + t.Fatalf("Content-Type = %q, want application/json", got.contentType) + } + var decoded map[string]any + if err := json.Unmarshal(got.body, &decoded); err != nil { + t.Fatalf("body %q is not JSON: %v", got.body, err) + } + if decoded["username"] != "ada" || decoded["password"] != "secret" { + t.Errorf("body = %v, want the JSON object", decoded) + } +} + +// TestUnencodableMediaTypeTakesRawBytes covers a body whose media type the client +// cannot build from the schema: it must take raw bytes and send them verbatim +// rather than accept a struct and marshal it as JSON under an XML Content-Type. +func TestUnencodableMediaTypeTakesRawBytes(t *testing.T) { + var got capture + srv := serve(t, &got) + + client := NewClient(srv.URL) + if _, err := client.CreateNote(t.Context(), []byte("phone")); err != nil { + t.Fatalf("CreateNote: %v", err) + } + if mediaType, _, _ := mime.ParseMediaType(got.contentType); mediaType != "application/xml" { + t.Errorf("Content-Type = %q, want application/xml", got.contentType) + } + if string(got.body) != "phone" { + t.Errorf("body = %q, want the bytes verbatim", got.body) + } +} + +// TestCatchAllPropertiesReachTheWire covers additionalProperties on a multipart +// body: the catch-all map is tagged json:"-" so that encoding/json inlines it by +// hand, and the form encoders have to inline it too rather than drop it. +func TestCatchAllPropertiesReachTheWire(t *testing.T) { + var got capture + srv := serve(t, &got) + + client := NewClient(srv.URL) + if _, err := client.CreateLabel(t.Context(), Label{ + Name: "ada", + AdditionalProperties: map[string]string{"team": "core"}, + }); err != nil { + t.Fatalf("CreateLabel: %v", err) + } + + req := httptest.NewRequest("POST", "/", strings.NewReader(string(got.body))) + req.Header.Set("Content-Type", got.contentType) + if err := req.ParseMultipartForm(1 << 20); err != nil { + t.Fatalf("ParseMultipartForm: %v", err) + } + if v := req.MultipartForm.Value["team"]; len(v) != 1 || v[0] != "core" { + t.Errorf("team = %v, want [core]: an undeclared property was dropped", v) + } + if _, ok := req.MultipartForm.Value["-"]; ok { + t.Error("the catch-all map was sent under a literal \"-\" part name") + } +} + +func ptr[T any](v T) *T { return &v } +`) + if err := os.WriteFile(filepath.Join(tmpDir, "request_body_test.go"), runtimeTest, 0o644); err != nil { + t.Fatalf("writing runtime test: %v", err) + } + + cmd := exec.Command("go", "test", "./...") + cmd.Dir = tmpDir + output, err := cmd.CombinedOutput() + if err != nil { + for _, f := range files { + if f.Name == "helpers.go" || f.Name == "operations.go" || f.Name == "types.go" { + t.Logf("=== %s ===\n%s", f.Name, string(f.Content)) + } + } + t.Fatalf("go test on generated code failed: %v\n%s", err, string(output)) + } + t.Logf("request body encoding test passed:\n%s", string(output)) +} diff --git a/internal/generator/funcmap.go b/internal/generator/funcmap.go index f0a1a98..dc76192 100644 --- a/internal/generator/funcmap.go +++ b/internal/generator/funcmap.go @@ -19,6 +19,7 @@ func FuncMap() template.FuncMap { "paramDocComment": paramDocComment, "indent": indent, "jsonTag": jsonTag, + "fieldTag": fieldTag, "hasOperations": hasOperations, "successType": successType, "hasBody": hasBody, @@ -40,6 +41,8 @@ func FuncMap() template.FuncMap { "errorMessageField": errorMessageField, "errorType": errorType, "successContentType": successContentType, + "requestContentType": requestContentType, + "hasNonJSONBody": hasNonJSONBody, } } @@ -177,6 +180,17 @@ func indent(s string) string { return strings.Join(lines, "\n") } +// fieldTag returns a struct field's full tag. The catch-all carries a marker +// because its json tag is "-": the body encoders have no other way to tell it +// apart from a field the schema genuinely excludes. +func fieldTag(f *ir.Field) string { + tag := `json:"` + jsonTag(f) + `"` + if f.CatchAll { + tag += ` openapi:"additionalProperties"` + } + return tag +} + // jsonTag returns the JSON struct tag value for a field. // It returns "fieldName,omitempty" for optional fields and "fieldName" for required ones. func jsonTag(f *ir.Field) string { @@ -366,3 +380,22 @@ func successContentType(op *ir.OperationDef) string { } return op.SuccessResponse.ContentType } + +// requestContentType returns the media type an operation sends its request body +// as, or "" when it has no body. +func requestContentType(op *ir.OperationDef) string { + if op.RequestBody == nil { + return "" + } + return op.RequestBody.ContentType +} + +// hasNonJSONBody reports whether any operation sends a request body in a media +// type other than JSON, which is what pulls the extra body encoders into the +// generated helpers. +func hasNonJSONBody(pkg *ir.Package) bool { + return slices.ContainsFunc(pkg.Operations, func(op *ir.OperationDef) bool { + ct := requestContentType(op) + return ct != "" && !strings.Contains(ct, "json") + }) +} diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go index 25a0cf4..27c8cfe 100644 --- a/internal/generator/generator_test.go +++ b/internal/generator/generator_test.go @@ -296,11 +296,13 @@ func TestGenerate_UnionType(t *testing.T) { if !strings.Contains(content, "func (u *PetOrError) UnmarshalJSON(data []byte)") { t.Error("output missing UnmarshalJSON method") } - // Without discriminator, should try each variant. - if !strings.Contains(content, "valPet") { + // Without discriminator, should try each variant. The locals are numbered + // rather than named after the type, so a variant whose Go type is not an + // identifier (say []string) still declares a legal one. + if !strings.Contains(content, "var variant0 Pet") { t.Error("output missing try-each-variant logic for Pet") } - if !strings.Contains(content, "valError") { + if !strings.Contains(content, "var variant1 Error") { t.Error("output missing try-each-variant logic for Error") } diff --git a/internal/ir/operations.go b/internal/ir/operations.go index 198a7b8..23c86a1 100644 --- a/internal/ir/operations.go +++ b/internal/ir/operations.go @@ -41,7 +41,6 @@ type RequestBodyDef struct { Description string ContentType string // Primary content type (e.g., "application/json") TypeName string // Go type for the body - IsMultipart bool } // ResponseDef describes one response. diff --git a/internal/templates/client.go.tmpl b/internal/templates/client.go.tmpl index b9f00d5..82c4009 100644 --- a/internal/templates/client.go.tmpl +++ b/internal/templates/client.go.tmpl @@ -36,16 +36,17 @@ func NewClient(baseURL string, opts ...ClientOption) *Client { return c } -// do executes an HTTP request and decodes the response. -func (c *Client) do(ctx context.Context, method string, path string, body any, result any, accept string, headers ...http.Header) error { +// do executes an HTTP request and decodes the response. contentType selects the +// request body encoding and is sent as the Content-Type header. +func (c *Client) do(ctx context.Context, method string, path string, body any, contentType string, result any, accept string, headers ...http.Header) error { fullURL := c.baseURL + path - var jsonBody []byte + var payload []byte if body != nil { var err error - jsonBody, err = json.Marshal(body) + payload, contentType, err = encodeRequestBody(body, contentType) if err != nil { - return fmt.Errorf("encoding request body: %w", err) + return err } } @@ -75,8 +76,8 @@ func (c *Client) do(ctx context.Context, method string, path string, body any, r } var bodyReader io.Reader - if jsonBody != nil { - bodyReader = bytes.NewReader(jsonBody) + if payload != nil { + bodyReader = bytes.NewReader(payload) } req, err := http.NewRequestWithContext(ctx, method, fullURL, bodyReader) @@ -84,10 +85,10 @@ func (c *Client) do(ctx context.Context, method string, path string, body any, r return fmt.Errorf("creating request: %w", err) } - if jsonBody != nil { - req.Header.Set("Content-Type", "application/json") + if payload != nil { + req.Header.Set("Content-Type", contentType) // Set GetBody so request body can be re-read on retries. - bodyBytes := jsonBody + bodyBytes := payload req.GetBody = func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(bodyBytes)), nil } diff --git a/internal/templates/helpers.go.tmpl b/internal/templates/helpers.go.tmpl index 4ded978..eeacc08 100644 --- a/internal/templates/helpers.go.tmpl +++ b/internal/templates/helpers.go.tmpl @@ -4,6 +4,7 @@ package {{ .Name }} import ( "encoding/base64" + "encoding/json" "fmt" "net/http" "net/url" @@ -12,6 +13,13 @@ import ( "strconv" "strings" "time" +{{- if hasNonJSONBody . }} + "bytes" + "encoding" + "io" + "mime/multipart" + "net/textproto" +{{- end }} ) // pathReplace substitutes a {param} placeholder in a URL path, encoding the value @@ -44,7 +52,13 @@ func derefParam(value any) (reflect.Value, bool) { // isSlice reports whether rv is a multi-value slice; a []byte is a scalar // (formatScalar renders it as base64), not a list of bytes. func isSlice(rv reflect.Value) bool { - return rv.Kind() == reflect.Slice && rv.Type().Elem().Kind() != reflect.Uint8 + return rv.Kind() == reflect.Slice && !isByteSlice(rv) +} + +// isByteSlice reports whether rv is a []byte, the Go type of an OpenAPI +// format: byte or format: binary value. +func isByteSlice(rv reflect.Value) bool { + return rv.Kind() == reflect.Slice && rv.Type().Elem().Kind() == reflect.Uint8 } // isObject reports whether rv is a struct or map to serialize property-by-property; @@ -71,7 +85,7 @@ func formatScalar(rv reflect.Value) string { } switch rv.Kind() { case reflect.Slice: - if rv.Type().Elem().Kind() == reflect.Uint8 { + if isByteSlice(rv) { return base64.StdEncoding.EncodeToString(rv.Bytes()) } case reflect.Float32: @@ -115,11 +129,18 @@ func sliceValues(rv reflect.Value) []string { return out } -// objectPairs returns an object's (property, value) pairs in a stable order: -// struct fields in declaration order keyed by their json tag (an embedded field's -// properties are flattened in), map keys sorted. A nil pointer property is skipped. -func objectPairs(rv reflect.Value) [][2]string { - var pairs [][2]string +// namedValue is one property of an object: its wire name and its value, with any +// pointer already unwrapped. +type namedValue struct { + name string + value reflect.Value +} + +// objectValues returns an object's properties in a stable order: struct fields in +// declaration order keyed by their json tag (an embedded field's properties are +// flattened in), map keys sorted. A nil pointer property is skipped. +func objectValues(rv reflect.Value) []namedValue { + var values []namedValue switch rv.Kind() { case reflect.Struct: t := rv.Type() @@ -132,8 +153,10 @@ func objectPairs(rv reflect.Value) [][2]string { if !ok { continue } - if f.Anonymous && isObject(fv) { - pairs = append(pairs, objectPairs(fv)...) + // An embedded struct and the additionalProperties catch-all both hold + // properties of this object, not properties of their own. + if (f.Anonymous || f.Tag.Get("openapi") == "additionalProperties") && isObject(fv) { + values = append(values, objectValues(fv)...) continue } name, _, _ := strings.Cut(f.Tag.Get("json"), ",") @@ -143,7 +166,7 @@ func objectPairs(rv reflect.Value) [][2]string { if name == "-" { continue } - pairs = append(pairs, [2]string{name, formatValue(fv)}) + values = append(values, namedValue{name, fv}) } case reflect.Map: keys := make([]string, 0, rv.Len()) @@ -156,10 +179,23 @@ func objectPairs(rv reflect.Value) [][2]string { sort.Strings(keys) for _, k := range keys { if fv, ok := derefParam(byKey[k].Interface()); ok { - pairs = append(pairs, [2]string{k, formatValue(fv)}) + values = append(values, namedValue{k, fv}) } } } + return values +} + +// objectPairs renders an object's properties as flat (property, value) pairs. +func objectPairs(rv reflect.Value) [][2]string { + values := objectValues(rv) + if len(values) == 0 { + return nil + } + pairs := make([][2]string, 0, len(values)) + for _, v := range values { + pairs = append(pairs, [2]string{v.name, formatValue(v.value)}) + } return pairs } @@ -338,6 +374,241 @@ func setHeader(headers http.Header, name string, explode bool, value any) { headers.Set(name, encodeSimple(rv, explode)) } +// encodeRequestBody renders a request body under the media type its operation +// declares, returning the payload and the Content-Type to send it with. +func encodeRequestBody(body any, contentType string) ([]byte, string, error) { + if contentType == "" { + contentType = "application/json" + } +{{- if hasNonJSONBody . }} + if !strings.Contains(contentType, "json") { + return encodeNonJSONBody(body, contentType) + } +{{- end }} + data, err := json.Marshal(body) + if err != nil { + return nil, "", fmt.Errorf("encoding request body: %w", err) + } + return data, contentType, nil +} +{{- if hasNonJSONBody . }} + +// encodeNonJSONBody renders a body whose media type is not JSON. +func encodeNonJSONBody(body any, contentType string) ([]byte, string, error) { + switch { + case strings.HasPrefix(contentType, "multipart/"): + return encodeMultipart(body, contentType) + case strings.HasPrefix(contentType, "application/x-www-form-urlencoded"): + return encodeFormValues(body, contentType) + } + + rv, ok := derefParam(body) + if !ok { + return nil, contentType, nil + } + if m, ok := rv.Interface().(encoding.TextMarshaler); ok { + data, err := m.MarshalText() + if err != nil { + return nil, "", fmt.Errorf("encoding %s body: %w", contentType, err) + } + return data, contentType, nil + } + switch { + case isByteSlice(rv): + return rv.Bytes(), contentType, nil + case !isObject(rv) && !isSlice(rv): + return []byte(formatScalar(rv)), contentType, nil + } + // Marshaling the value as JSON here would send JSON under a Content-Type that + // promises something else; the schema alone does not say how to encode it. + return nil, "", fmt.Errorf("encoding %s body: no encoder for this media type, pass a []byte or string body", contentType) +} + +// encodeFormValues renders a body as url-encoded form data. +func encodeFormValues(body any, contentType string) ([]byte, string, error) { + rv, ok := derefParam(body) + if !ok || !isObject(rv) { + return nil, "", fmt.Errorf("encoding %s body: want an object, got %T", contentType, body) + } + values := url.Values{} + for _, v := range objectValues(rv) { + if err := addFormValue(values, v.name, v.value); err != nil { + return nil, "", fmt.Errorf("encoding form field %q: %w", v.name, err) + } + } + // Encode (unlike encodeQuery) spells a space '+', which is what + // x-www-form-urlencoded defines; the RFC 3986 rewrite applies to query strings. + return []byte(values.Encode()), contentType, nil +} + +// addFormValue adds one property of a url-encoded body: an array one pair per +// element, an object as JSON, everything else as a scalar. +func addFormValue(values url.Values, name string, rv reflect.Value) error { + if rv.Kind() == reflect.Interface { + if rv.IsNil() { + return nil + } + rv = rv.Elem() + } + switch { + case isSlice(rv): + for i := 0; i < rv.Len(); i++ { + if err := addFormValue(values, name, rv.Index(i)); err != nil { + return err + } + } + case isObject(rv): + data, err := json.Marshal(rv.Interface()) + if err != nil { + return err + } + values.Add(name, string(data)) + default: + values.Add(name, formatScalar(rv)) + } + return nil +} + +// FormFile is one file in a multipart request body. Filename defaults to the +// property name and ContentType to application/octet-stream when left empty. +type FormFile struct { + Filename string + ContentType string + Content []byte +} + +// MarshalJSON encodes a FormFile the way the []byte it stands in for would be, +// so a schema shared between a multipart body and a JSON one still round-trips. +func (f FormFile) MarshalJSON() ([]byte, error) { + return json.Marshal(f.Content) +} + +// UnmarshalJSON decodes a base64 JSON string into the file's content. +func (f *FormFile) UnmarshalJSON(data []byte) error { + return json.Unmarshal(data, &f.Content) +} + +// encodeMultipart renders a body as multipart form data. A FormFile property +// (OpenAPI format: binary) becomes a file part; every other property becomes a +// text part, an array one part per element. +func encodeMultipart(body any, contentType string) ([]byte, string, error) { + rv, ok := derefParam(body) + if !ok || !isObject(rv) { + return nil, "", fmt.Errorf("encoding %s body: want an object, got %T", contentType, body) + } + + var buf bytes.Buffer + w := multipart.NewWriter(&buf) + for _, v := range objectValues(rv) { + if err := writeMultipartField(w, v.name, v.value); err != nil { + return nil, "", fmt.Errorf("encoding multipart field %q: %w", v.name, err) + } + } + if err := w.Close(); err != nil { + return nil, "", fmt.Errorf("encoding multipart body: %w", err) + } + return buf.Bytes(), contentType + "; boundary=" + w.Boundary(), nil +} + +// writeMultipartField writes one property of a multipart body. +func writeMultipartField(w *multipart.Writer, name string, rv reflect.Value) error { + if rv.Kind() == reflect.Interface { + if rv.IsNil() { + return nil + } + rv = rv.Elem() + } + + // A FormFile is a struct, so it has to be recognized before isObject would + // send it through as a JSON part. + if file, ok := rv.Interface().(FormFile); ok { + return writeFilePart(w, name, file) + } + + switch { + case isByteSlice(rv): + // A nil slice is an unset optional property; an empty non-nil one is an + // empty file the caller asked for. + if rv.IsNil() { + return nil + } + return writeFilePart(w, name, FormFile{Content: rv.Bytes()}) + case isSlice(rv): + for i := 0; i < rv.Len(); i++ { + if err := writeMultipartField(w, name, rv.Index(i)); err != nil { + return err + } + } + return nil + case isObject(rv): + // OpenAPI encodes an object-valued part as JSON unless the spec says otherwise. + data, err := json.Marshal(rv.Interface()) + if err != nil { + return err + } + part, err := createPart(w, name, "", "application/json") + if err != nil { + return err + } + _, err = part.Write(data) + return err + default: + part, err := createPart(w, name, "", "") + if err != nil { + return err + } + _, err = io.WriteString(part, formatScalar(rv)) + return err + } +} + +// writeFilePart writes a file part; a server that keys on Content-Disposition's +// filename will not treat a part without one as an upload at all. +func writeFilePart(w *multipart.Writer, name string, file FormFile) error { + filename := file.Filename + if filename == "" { + filename = name + } + contentType := file.ContentType + if contentType == "" { + contentType = "application/octet-stream" + } + part, err := createPart(w, name, filename, contentType) + if err != nil { + return err + } + _, err = part.Write(file.Content) + return err +} + +// createPart starts a multipart part with the headers that apply to it. +func createPart(w *multipart.Writer, name, filename, contentType string) (io.Writer, error) { + disposition := `form-data; name="` + escapePartName(name) + `"` + if filename != "" { + disposition += `; filename="` + escapePartName(filename) + `"` + } + header := make(textproto.MIMEHeader, 2) + header.Set("Content-Disposition", disposition) + if contentType != "" { + header.Set("Content-Type", stripHeaderBreaks.Replace(contentType)) + } + return w.CreatePart(header) +} + +// stripHeaderBreaks drops the line breaks that would otherwise let a value chosen +// at runtime — a map key, a caller's content type — inject headers of its own; +// multipart.Writer writes part headers verbatim. +var stripHeaderBreaks = strings.NewReplacer("\r", "", "\n", "") + +// partNameEscaper additionally quotes the characters that would end a +// Content-Disposition parameter early. +var partNameEscaper = strings.NewReplacer(`\`, `\\`, `"`, `\"`, "\r", "", "\n", "") + +func escapePartName(name string) string { + return partNameEscaper.Replace(name) +} +{{- end }} + // addCookieHeader appends a cookie to the Cookie header, skipping nil optional // values. A Cookie header is a single name=value, so arrays and objects are // flattened (form style, unexploded); http.Cookie sanitizes invalid octets. diff --git a/internal/templates/operations.go.tmpl b/internal/templates/operations.go.tmpl index 25798fa..58281b2 100644 --- a/internal/templates/operations.go.tmpl +++ b/internal/templates/operations.go.tmpl @@ -45,11 +45,11 @@ type {{ .Name }}Params struct { {{ end }}{{ end }} {{- $errType := errorType . -}} {{ if successType . }} var result {{ successType . }} - if err := c.do(ctx, "{{ .HTTPMethod }}", path, {{ if hasBody . }}body{{ else }}nil{{ end }}, &result, "{{ successContentType . }}"{{ if $needHeaders }}, headers{{ end }}); err != nil { + if err := c.do(ctx, "{{ .HTTPMethod }}", path, {{ if hasBody . }}body{{ else }}nil{{ end }}, {{ printf "%q" (requestContentType .) }}, &result, {{ printf "%q" (successContentType .) }}{{ if $needHeaders }}, headers{{ end }}); err != nil { return nil, {{ if $errType }}parse{{ $errType }}Response(err){{ else }}err{{ end }} } return &result, nil -{{ else }} if err := c.do(ctx, "{{ .HTTPMethod }}", path, {{ if hasBody . }}body{{ else }}nil{{ end }}, nil, "{{ successContentType . }}"{{ if $needHeaders }}, headers{{ end }}); err != nil { +{{ else }} if err := c.do(ctx, "{{ .HTTPMethod }}", path, {{ if hasBody . }}body{{ else }}nil{{ end }}, {{ printf "%q" (requestContentType .) }}, nil, {{ printf "%q" (successContentType .) }}{{ if $needHeaders }}, headers{{ end }}); err != nil { return {{ if $errType }}parse{{ $errType }}Response(err){{ else }}err{{ end }} } return nil diff --git a/internal/templates/types.go.tmpl b/internal/templates/types.go.tmpl index dcefa51..dec1244 100644 --- a/internal/templates/types.go.tmpl +++ b/internal/templates/types.go.tmpl @@ -34,7 +34,7 @@ func deleteDeclaredProperties(obj map[string]json.RawMessage, declared []string) {{ end }}type {{ .Name }} struct { {{ range .Fields }}{{ $fDoc := fieldDocComment . }}{{ if $fDoc }}{{ indent $fDoc }} {{ end }}{{ if .Embedded }} {{ .Type }} -{{ else }} {{ .Name }} {{ .Type }} `json:"{{ jsonTag . }}"` +{{ else }} {{ .Name }} {{ .Type }} `{{ fieldTag . }}` {{ end }}{{ end }}} {{ with catchAllField . }} // MarshalJSON implements json.Marshaler for {{ $typeName }}, inlining @@ -191,11 +191,11 @@ func (u *{{ $typeName }}) UnmarshalJSON(data []byte) error { } {{- else }} var errors []error -{{- range .UnionTypes }} -{{- if ne .TypeName "any" }} - var val{{ .TypeName }} {{ .TypeName }} - if err := json.Unmarshal(data, &val{{ .TypeName }}); err == nil { - u.Value = val{{ .TypeName }} +{{- range $i, $v := .UnionTypes }} +{{- if ne $v.TypeName "any" }} + var variant{{ $i }} {{ $v.TypeName }} + if err := json.Unmarshal(data, &variant{{ $i }}); err == nil { + u.Value = variant{{ $i }} return nil } else { errors = append(errors, err) From 1950f28f5897a1bb5c91b0bf5bb6048558430bb5 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Thu, 6 Aug 2026 00:45:02 +0800 Subject: [PATCH 2/4] fix: a schema named like a generated type redeclares it The templates always declare Client, APIError, RetryConfig and friends at package scope. A schema of the same name was emitted alongside them, and Go has one package scope, so the client did not compile. Those names are now reserved before any schema is converted, so such a schema is renamed the same way any other collision is. FormFile, added for multipart bodies, joined that set and is reserved with them. Reserving surfaced a second bug in the same area: a reference to a schema converted later fell back to its exported spelling rather than the name the schema was actually assigned. Two schemas differing only in punctuation share one spelling, so `foo-bar` and `foo_bar` both became FooBar and a forward reference to the second silently pointed at the first -- it compiled, and decoded into the wrong type. Every component's Go name is now settled up front and referenced from there. --- internal/analyzer/analyzer.go | 12 +- internal/analyzer/schemas.go | 7 +- internal/generator/e2e_name_collision_test.go | 134 ++++++++++++++++++ internal/templates/reserved.go | 30 ++++ 4 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 internal/generator/e2e_name_collision_test.go create mode 100644 internal/templates/reserved.go diff --git a/internal/analyzer/analyzer.go b/internal/analyzer/analyzer.go index 3ebb84d..dba9e37 100644 --- a/internal/analyzer/analyzer.go +++ b/internal/analyzer/analyzer.go @@ -8,6 +8,7 @@ import ( naming "github.com/giraffesyo/openapi-go-naming" "github.com/parallelworks/openapi-client-generator/internal/ir" + "github.com/parallelworks/openapi-client-generator/internal/templates" ) // Analyzer walks a parsed OpenAPI 3.1 model and produces IR types. @@ -22,15 +23,20 @@ type Analyzer struct { synthesizedByKey map[string]*ir.TypeDef // multipartBodies holds the schema names a multipart request body refers to. multipartBodies map[string]bool + // goNameBySchema maps every component schema to its Go type name, filled in + // before any conversion so a reference to a schema that has not been converted + // yet still resolves to the name it will end up with. + goNameBySchema map[string]string } // New creates an Analyzer for the given high-level OpenAPI model. func New(model *v3high.Document) *Analyzer { return &Analyzer{ model: model, - namer: naming.NewScope(), + namer: naming.NewScope(templates.ReservedIdentifiers...), typesBySchema: make(map[string]*ir.TypeDef), synthesizedByKey: make(map[string]*ir.TypeDef), + goNameBySchema: make(map[string]string), } } @@ -112,7 +118,9 @@ func (a *Analyzer) analyzeComponentSchemas(pkg *ir.Package) error { if schema == nil { continue } - pending = append(pending, pendingSchema{name, a.namer.Unique(naming.Exported(name)), schema}) + goName := a.namer.Unique(naming.Exported(name)) + a.goNameBySchema[name] = goName + pending = append(pending, pendingSchema{name, goName, schema}) } for _, p := range pending { diff --git a/internal/analyzer/schemas.go b/internal/analyzer/schemas.go index 09d40df..c385c2c 100644 --- a/internal/analyzer/schemas.go +++ b/internal/analyzer/schemas.go @@ -816,11 +816,16 @@ func (a *Analyzer) goTypeForRef(ref string) string { } // goTypeForSchemaName returns the Go type name of a component schema, falling -// back to its exported spelling when it has not been converted yet. +// back to its exported spelling when the schema is not one of the components. func (a *Analyzer) goTypeForSchemaName(refName string) string { if td, ok := a.typesBySchema[refName]; ok { return td.Name } + // Not converted yet: the name it was assigned, which a renamed schema needs + // for the reference to land on the right type. + if goName, ok := a.goNameBySchema[refName]; ok { + return goName + } return naming.Exported(refName) } diff --git a/internal/generator/e2e_name_collision_test.go b/internal/generator/e2e_name_collision_test.go new file mode 100644 index 0000000..4584ed0 --- /dev/null +++ b/internal/generator/e2e_name_collision_test.go @@ -0,0 +1,134 @@ +package generator + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/parallelworks/openapi-client-generator/internal/analyzer" + "github.com/parallelworks/openapi-client-generator/internal/parser" + "github.com/parallelworks/openapi-client-generator/internal/templates" +) + +// generateAndBuild generates a client for spec, builds it, and returns the +// compiler output ("" when it built) alongside types.go. +func generateAndBuild(t *testing.T, spec string) (buildOutput, types string) { + t.Helper() + + specPath := filepath.Join(t.TempDir(), "spec.yaml") + if err := os.WriteFile(specPath, []byte(spec), 0o644); err != nil { + t.Fatalf("writing spec: %v", err) + } + result, err := parser.Parse(specPath, parser.Config{}) + if err != nil { + t.Fatalf("Parse: %v", err) + } + pkg, err := analyzer.New(result.Model).Analyze("probe") + if err != nil { + t.Fatalf("Analyze: %v", err) + } + gen, err := New(pkg) + if err != nil { + t.Fatalf("New generator: %v", err) + } + files, err := gen.Generate() + if err != nil { + t.Fatalf("Generate: %v", err) + } + + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "go.mod"), []byte("module probe\n\ngo 1.25.5\n"), 0o644); err != nil { + t.Fatalf("writing go.mod: %v", err) + } + if err := WriteFiles(dir, files); err != nil { + t.Fatalf("WriteFiles: %v", err) + } + + cmd := exec.Command("go", "build", "./...") + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err == nil { + out = nil + } + for _, f := range files { + if f.Name == "types.go" { + types = string(f.Content) + } + } + return string(out), types +} + +// TestE2E_SchemaNamedLikeGeneratedType checks that a schema whose name matches +// one of the identifiers the templates always declare is renamed rather than +// redeclared. Go has one package scope, so the collision would not compile. +func TestE2E_SchemaNamedLikeGeneratedType(t *testing.T) { + for _, name := range templates.ReservedIdentifiers { + t.Run(name, func(t *testing.T) { + build, types := generateAndBuild(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /u: + post: + operationId: upload + requestBody: + required: true + content: + multipart/form-data: + schema: { $ref: "#/components/schemas/Upload" } + responses: { "204": { description: ok } } +components: + schemas: + Upload: + type: object + required: [file] + properties: + file: { type: string, format: binary } + `+name+`: + type: object + properties: + x: { type: string } +`) + if build != "" { + t.Errorf("a schema named %q does not compile:\n%s", name, build) + } + if !strings.Contains(types, "type "+name+"2 struct") { + t.Errorf("schema %q was not renamed out of the way:\n%s", name, types) + } + }) + } +} + +// TestE2E_ForwardReferenceUsesTheRenamedType checks that a reference to a schema +// converted later still resolves to the name that schema ends up with. Two +// schemas that differ only in punctuation share one exported spelling, so the +// second is renamed — and a forward reference used to silently point at the first. +func TestE2E_ForwardReferenceUsesTheRenamedType(t *testing.T) { + build, types := generateAndBuild(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Holder: + type: object + properties: + a: { $ref: "#/components/schemas/foo-bar" } + b: { $ref: "#/components/schemas/foo_bar" } + foo-bar: + type: object + properties: { p: { type: string } } + foo_bar: + type: object + properties: { q: { type: integer } } +`) + if build != "" { + t.Fatalf("generated client does not compile:\n%s", build) + } + if !strings.Contains(types, "A *FooBar `") { + t.Errorf("Holder.a does not reference FooBar:\n%s", types) + } + if !strings.Contains(types, "B *FooBar2 `") { + t.Errorf("Holder.b does not reference the renamed FooBar2:\n%s", types) + } +} diff --git a/internal/templates/reserved.go b/internal/templates/reserved.go new file mode 100644 index 0000000..7d7bd14 --- /dev/null +++ b/internal/templates/reserved.go @@ -0,0 +1,30 @@ +package templates + +// ReservedIdentifiers are the exported package-level names the templates always +// declare. A schema whose name lands on one of these is renamed, because Go has +// a single package scope and the collision would not compile. Unexported helpers +// need no entry: a generated type name is always exported. +// +// Keep in sync with the templates. +var ReservedIdentifiers = []string{ + "APIError", + "APIKeyAuth", + "AuthProvider", + "BasicAuth", + "BearerAuth", + "Client", + "ClientOption", + "DefaultRetryConfig", + "FormFile", + "Middleware", + "NewClient", + "PageIterator", + "RetryConfig", + "RoundTripFunc", + "WithAuth", + "WithDefaultRetry", + "WithHTTPClient", + "WithMiddleware", + "WithRetry", + "WithUserAgent", +} From a6fe68658bcd7a356b5c02163d92ef8eed97598a Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Thu, 6 Aug 2026 01:01:34 +0800 Subject: [PATCH 3/4] fix: recursive schemas, schemaless bodies, and composed catch-alls Five bugs, each of which produced a client that did not compile or lost data the payload carried. A required property referencing its own type generated `type Node struct { Child Node }`. Go allows a type to contain itself only through an indirection, so such a field is now a pointer -- the tree, threaded comment, and category-hierarchy shapes every spec of any size has. A slice or map between the two ends already breaks the recursion and is left alone. A request body the spec declares without a schema generated a method with an untyped parameter, which is a syntax error. It now takes the bytes or text its media type implies, or any under JSON; a body with no content at all is no body, not an empty one. Discriminated unions stopped being comparable when the preserved payload of an unrecognized variant was stored as json.RawMessage. A union is an ordinary field of the structs that hold it, so that made every one of those structs uncomparable too -- consumers could no longer compare them or use them as map keys. The payload is kept as a string, which also stops Raw() from handing out an alias of the union's own buffer. Composed schemas dropped additionalProperties entirely: only plain objects collected them. They are collected for allOf now, and the wire names an embedded schema contributes are recognized as declared, so inherited properties are not also re-collected and emitted twice. A typed additionalProperties failed the whole decode when one undeclared property did not match the declared value type, losing the response along with it. Such a property is skipped instead, matching how an unrecognized discriminator is already handled. Composed schemas whose embedded schema also collects undeclared properties keep the previous behavior, which is not yet correct: the embedded marshalers are promoted onto the outer struct and swallow its declared fields. Fixing that means replacing the shadow-type trick with field-by-field marshaling; tracked separately. --- internal/analyzer/aliascycles.go | 119 +++++++++++++++ internal/analyzer/analyzer.go | 5 +- internal/analyzer/operations.go | 34 +++-- internal/analyzer/schemas.go | 33 ++-- .../e2e_additional_properties_test.go | 67 ++++++++ internal/generator/e2e_inline_union_test.go | 39 +++++ internal/generator/e2e_name_collision_test.go | 18 +-- internal/generator/e2e_recursive_test.go | 143 ++++++++++++++++++ internal/generator/funcmap.go | 39 ++++- internal/templates/types.go.tmpl | 23 ++- 10 files changed, 469 insertions(+), 51 deletions(-) create mode 100644 internal/generator/e2e_recursive_test.go diff --git a/internal/analyzer/aliascycles.go b/internal/analyzer/aliascycles.go index 5e6a6d4..bd3f44f 100644 --- a/internal/analyzer/aliascycles.go +++ b/internal/analyzer/aliascycles.go @@ -1,6 +1,7 @@ package analyzer import ( + "slices" "strings" "github.com/parallelworks/openapi-client-generator/internal/ir" @@ -55,6 +56,124 @@ func breakAliasCycles(types []*ir.TypeDef) { } } +// breakStructCycles turns into a pointer every struct field that would make its +// type contain itself by value, which Go rejects the same way as a recursive +// alias. A field that is already a pointer, slice, or map stops the recursion on +// its own; a required $ref to the enclosing type does not. +func breakStructCycles(types []*ir.TypeDef) { + byName := make(map[string]*ir.TypeDef, len(types)) + for _, td := range types { + if td != nil { + byName[td.Name] = td + } + } + + // containedStruct returns the struct a field of this type holds by value, + // following aliases to the definition they stand for. + containedStruct := func(goType string) *ir.TypeDef { + for range len(types) + 1 { + td, ok := byName[namedType(goType)] + if !ok { + return nil + } + switch td.Kind { + case ir.TypeKindStruct: + return td + case ir.TypeKindAlias: + goType = td.GoType + default: + return nil + } + } + return nil + } + + const ( + visiting = 1 + done = 2 + ) + state := make(map[string]int, len(byName)) + + var walk func(td *ir.TypeDef) + walk = func(td *ir.TypeDef) { + state[td.Name] = visiting + for _, f := range td.Fields { + next := containedStruct(f.Type) + if next == nil { + continue + } + if state[next.Name] == visiting { + // This field closes the cycle, so it is the one to indirect. + f.Type = "*" + f.Type + f.IsPointer = true + continue + } + if state[next.Name] != done { + walk(next) + } + } + state[td.Name] = done + } + + for _, td := range types { + if td != nil && td.Kind == ir.TypeKindStruct && state[td.Name] == 0 { + walk(td) + } + } +} + +// dropShadowedCatchAlls removes the catch-all from a struct that embeds a type +// which already has one. The generated marshalers shadow the struct to reach +// encoding/json, and a shadow still promotes an embedded type's MarshalJSON — so +// the two catch-alls would fight and the embedded one would win, emitting only +// its own fields. Leaving the outer schema's undeclared properties uncollected +// is the narrower loss, and it is what the generator did before composed schemas +// collected any at all. +// +// Handling both at once needs the shadow replaced with field-by-field marshaling. +func dropShadowedCatchAlls(types []*ir.TypeDef) { + byName := make(map[string]*ir.TypeDef, len(types)) + for _, td := range types { + if td != nil && td.Kind == ir.TypeKindStruct { + byName[td.Name] = td + } + } + + hasCatchAll := func(td *ir.TypeDef) bool { + return slices.ContainsFunc(td.Fields, func(f *ir.Field) bool { return f.CatchAll }) + } + + // Reports whether td or anything it embeds carries a catch-all. + var embedsCatchAll func(td *ir.TypeDef, depth int) bool + embedsCatchAll = func(td *ir.TypeDef, depth int) bool { + if td == nil || depth > len(types) { + return false + } + for _, f := range td.Fields { + if !f.Embedded { + continue + } + embedded := byName[namedType(strings.TrimPrefix(f.Type, "*"))] + if embedded == nil { + continue + } + if hasCatchAll(embedded) || embedsCatchAll(embedded, depth+1) { + return true + } + } + return false + } + + for _, td := range types { + if td == nil || td.Kind != ir.TypeKindStruct || !hasCatchAll(td) { + continue + } + if embedsCatchAll(td, 0) { + td.Fields = slices.DeleteFunc(td.Fields, func(f *ir.Field) bool { return f.CatchAll }) + } + } +} + // aliasTarget returns the named type an alias's Go type expression refers to, // peeling the slice, pointer, and map wrappers that do not stop a Go alias from // expanding. It returns "" for a builtin or a composite with no single referent. diff --git a/internal/analyzer/analyzer.go b/internal/analyzer/analyzer.go index dba9e37..59d326c 100644 --- a/internal/analyzer/analyzer.go +++ b/internal/analyzer/analyzer.go @@ -85,8 +85,11 @@ func (a *Analyzer) Analyze(packageName string) (*ir.Package, error) { // Append union types synthesized for inline oneOf/anyOf schemas. pkg.Types = append(pkg.Types, a.synthesized...) - // A spec is free to define a type in terms of itself; Go aliases are not. + // A spec is free to define a type in terms of itself; Go aliases are not, and + // a struct may only do it through an indirection. breakAliasCycles(pkg.Types) + breakStructCycles(pkg.Types) + dropShadowedCatchAlls(pkg.Types) // Detect paginated operations. a.detectPagination(pkg) diff --git a/internal/analyzer/operations.go b/internal/analyzer/operations.go index 06eba19..3c24419 100644 --- a/internal/analyzer/operations.go +++ b/internal/analyzer/operations.go @@ -312,20 +312,20 @@ func effectiveStyleExplode(param *v3high.Parameter) (string, bool) { // convertRequestBody converts an OpenAPI request body to an ir.RequestBodyDef. func (a *Analyzer) convertRequestBody(rb *v3high.RequestBody, nameHint string) (*ir.RequestBodyDef, error) { - def := &ir.RequestBodyDef{ - Required: rb.Required != nil && *rb.Required, - Description: rb.Description, - } - // The chosen content type decides how the body is encoded on the wire. contentType, mediaType := preferredContent(rb.Content) if contentType == "" { - return def, nil + // The spec declares a body but no content to put in it, so there is + // nothing for the caller to pass and no type to pass it as. + return nil, nil } - def.ContentType = contentType - def.TypeName = rawBodyType(contentType, a.resolveMediaTypeSchema(mediaType, nameHint)) - return def, nil + return &ir.RequestBodyDef{ + Required: rb.Required != nil && *rb.Required, + Description: rb.Description, + ContentType: contentType, + TypeName: bodyGoType(contentType, a.resolveMediaTypeSchema(mediaType, nameHint)), + }, nil } // structuredContentType reports whether the generated client can encode a Go @@ -336,13 +336,15 @@ func structuredContentType(contentType string) bool { strings.HasPrefix(contentType, "application/x-www-form-urlencoded") } -// rawBodyType keeps a body the client cannot structurally encode — XML, say — -// out of a generated struct it would have no encoder for. Handing the caller -// []byte (or a string for text) says what the client can actually send, rather -// than accepting a struct and marshaling it as JSON under a media type that -// promises something else. -func rawBodyType(contentType, typeName string) string { - if typeName == "" || structuredContentType(contentType) { +// bodyGoType is the Go type a request body is accepted as. A body the client +// cannot structurally encode — XML, say — is taken as the bytes or text it +// already is rather than as a struct there would be no encoder for, and a body +// the spec declares without a schema still needs some type to be passed as. +func bodyGoType(contentType, typeName string) string { + if structuredContentType(contentType) { + if typeName == "" { + return "any" + } return typeName } switch typeName { diff --git a/internal/analyzer/schemas.go b/internal/analyzer/schemas.go index c385c2c..9c10efb 100644 --- a/internal/analyzer/schemas.go +++ b/internal/analyzer/schemas.go @@ -230,9 +230,26 @@ func (a *Analyzer) convertAllOf(goName string, schema *highbase.Schema, nullable } } + a.addCatchAllField(td, schema, goName) + return td, nil } +// addCatchAllField gives a struct the synthetic field that holds whatever the +// schema does not declare, when the schema admits such properties at all. +func (a *Analyzer) addCatchAllField(td *ir.TypeDef, schema *highbase.Schema, goName string) { + if !allowsAdditionalProperties(schema) { + return + } + td.Fields = append(td.Fields, &ir.Field{ + Name: catchAllFieldName(td.Fields), + JSONName: "-", + Type: "map[string]" + a.resolveAdditionalPropertiesType(schema, goName), + Description: "Properties not defined by the schema.", + CatchAll: true, + }) +} + // convertProperty converts one object property into a struct field. multipartBody // marks a schema sent as multipart form data, whose binary properties are file // parts rather than byte slices. @@ -405,19 +422,9 @@ func (a *Analyzer) convertObject(goName string, schema *highbase.Schema, nullabl td.Fields = append(td.Fields, a.convertProperty(goName, propName, propSchema, requiredSet[propName], multipartBody)) } - // If the object has both properties and additionalProperties, add an extra field. - // The generator gives such structs MarshalJSON/UnmarshalJSON so the map is - // inlined into the object rather than nested under a key of its own. - if allowsAdditionalProperties(schema) { - mapValueType := a.resolveAdditionalPropertiesType(schema, goName) - td.Fields = append(td.Fields, &ir.Field{ - Name: catchAllFieldName(td.Fields), - JSONName: "-", - Type: "map[string]" + mapValueType, - Description: "Properties not defined by the schema.", - CatchAll: true, - }) - } + // The generator gives a struct with a catch-all MarshalJSON/UnmarshalJSON so + // the map is inlined into the object rather than nested under a key of its own. + a.addCatchAllField(td, schema, goName) return td, nil } diff --git a/internal/generator/e2e_additional_properties_test.go b/internal/generator/e2e_additional_properties_test.go index 5915b82..0a94de4 100644 --- a/internal/generator/e2e_additional_properties_test.go +++ b/internal/generator/e2e_additional_properties_test.go @@ -38,6 +38,17 @@ components: properties: only: { type: string } additionalProperties: false + Base: + type: object + properties: + id: { type: string } + Composed: + allOf: + - $ref: "#/components/schemas/Base" + - type: object + properties: + name: { type: string } + additionalProperties: true ` const additionalPropertiesRuntimeTest = `package petsapi @@ -160,6 +171,62 @@ func TestEmptyAdditionalPropertiesOmitsNothingExtra(t *testing.T) { t.Errorf("marshal = %s, want {\"name\":\"rex\"}", out) } } + +// A composed schema collects undeclared properties just like a plain one, and +// the properties it inherits from the schema it embeds are not re-collected. +func TestComposedSchemaKeepsUnknownKeys(t *testing.T) { + var c Composed + if err := json.Unmarshal([]byte(` + "`" + `{"id":"x","name":"n","extra":"kept"}` + "`" + `), &c); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if c.ID == nil || *c.ID != "x" { + t.Errorf("inherited property lost: %v", c.ID) + } + if c.Name == nil || *c.Name != "n" { + t.Errorf("declared property lost: %v", c.Name) + } + if got := c.AdditionalProperties["extra"]; got != "kept" { + t.Errorf("AdditionalProperties[extra] = %v, want kept", got) + } + if _, ok := c.AdditionalProperties["id"]; ok { + t.Error("a property inherited from the embedded schema landed in the catch-all") + } + + out, err := json.Marshal(c) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if n := strings.Count(string(out), ` + "`" + `"id"` + "`" + `); n != 1 { + t.Errorf("id emitted %d times: %s", n, out) + } + var got map[string]any + if err := json.Unmarshal(out, &got); err != nil { + t.Fatalf("re-unmarshal: %v", err) + } + for key, want := range map[string]any{"id": "x", "name": "n", "extra": "kept"} { + if got[key] != want { + t.Errorf("round trip[%s] = %v, want %v", key, got[key], want) + } + } +} + +// One undeclared property of the wrong type must not cost the caller the whole +// response; the declared fields are what consumers depend on. +func TestOffTypeExtraDoesNotFailTheDecode(t *testing.T) { + var l Labels + if err := json.Unmarshal([]byte(` + "`" + `{"owner":"me","count":3,"env":"prod"}` + "`" + `), &l); err != nil { + t.Fatalf("one off-type extra failed the whole decode: %v", err) + } + if l.Owner == nil || *l.Owner != "me" { + t.Errorf("Owner = %v, want me", l.Owner) + } + if l.AdditionalProperties["env"] != "prod" { + t.Errorf("AdditionalProperties[env] = %q, want prod", l.AdditionalProperties["env"]) + } + if _, ok := l.AdditionalProperties["count"]; ok { + t.Error("a property that does not match the declared value type was kept anyway") + } +} ` // TestE2E_AdditionalPropertiesRoundTrip generates a client for schemas that mix diff --git a/internal/generator/e2e_inline_union_test.go b/internal/generator/e2e_inline_union_test.go index 1f646b6..fa0e5ab 100644 --- a/internal/generator/e2e_inline_union_test.go +++ b/internal/generator/e2e_inline_union_test.go @@ -145,6 +145,45 @@ func TestMissingDiscriminatorIsAnError(t *testing.T) { } } +// A union must stay comparable: it is an ordinary field of the structs that hold +// it, so storing the preserved raw payload in a slice would make every one of +// those structs uncomparable too -- a compile error for consumers. +func TestUnionIsComparable(t *testing.T) { + var a, b ShapeCollectionShapesValue + if a != b { + t.Error("zero unions should be equal") + } + if !map[ShapeCollectionShapesValue]bool{a: true}[b] { + t.Error("a union should be usable as a map key") + } +} + +func TestUnknownVariantRawIsACopy(t *testing.T) { + payload := []byte("{\"shapeType\":\"hexagon\",\"sides\":6}") + var v ShapeCollectionShapesValue + if err := json.Unmarshal(payload, &v); err != nil { + t.Fatalf("unmarshal: %v", err) + } + raw := v.Raw() + if len(raw) == 0 { + t.Fatal("Raw() lost the unrecognized payload") + } + raw[0] = 'X' + if again := v.Raw(); again[0] == 'X' { + t.Error("Raw() aliases the union's own buffer") + } +} + +func TestKnownVariantHasNoRaw(t *testing.T) { + var v ShapeCollectionShapesValue + if err := json.Unmarshal([]byte("{\"shapeType\":\"circle\",\"radius\":1}"), &v); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if v.Raw() != nil { + t.Errorf("Raw() = %s, want nil for a recognized variant", v.Raw()) + } +} + func TestNullUnionDecodesToTheZeroValue(t *testing.T) { var v ShapeCollectionShapesValue if err := json.Unmarshal([]byte("null"), &v); err != nil { diff --git a/internal/generator/e2e_name_collision_test.go b/internal/generator/e2e_name_collision_test.go index 4584ed0..5854460 100644 --- a/internal/generator/e2e_name_collision_test.go +++ b/internal/generator/e2e_name_collision_test.go @@ -13,8 +13,8 @@ import ( ) // generateAndBuild generates a client for spec, builds it, and returns the -// compiler output ("" when it built) alongside types.go. -func generateAndBuild(t *testing.T, spec string) (buildOutput, types string) { +// compiler output ("" when it built) alongside the generated files by name. +func generateAndBuild(t *testing.T, spec string) (buildOutput string, generated map[string]string) { t.Helper() specPath := filepath.Join(t.TempDir(), "spec.yaml") @@ -52,12 +52,11 @@ func generateAndBuild(t *testing.T, spec string) (buildOutput, types string) { if err == nil { out = nil } + generated = make(map[string]string, len(files)) for _, f := range files { - if f.Name == "types.go" { - types = string(f.Content) - } + generated[f.Name] = string(f.Content) } - return string(out), types + return string(out), generated } // TestE2E_SchemaNamedLikeGeneratedType checks that a schema whose name matches @@ -66,7 +65,7 @@ func generateAndBuild(t *testing.T, spec string) (buildOutput, types string) { func TestE2E_SchemaNamedLikeGeneratedType(t *testing.T) { for _, name := range templates.ReservedIdentifiers { t.Run(name, func(t *testing.T) { - build, types := generateAndBuild(t, `openapi: 3.1.0 + build, files := generateAndBuild(t, `openapi: 3.1.0 info: { title: t, version: "1" } paths: /u: @@ -93,7 +92,7 @@ components: if build != "" { t.Errorf("a schema named %q does not compile:\n%s", name, build) } - if !strings.Contains(types, "type "+name+"2 struct") { + if types := files["types.go"]; !strings.Contains(types, "type "+name+"2 struct") { t.Errorf("schema %q was not renamed out of the way:\n%s", name, types) } }) @@ -105,7 +104,7 @@ components: // schemas that differ only in punctuation share one exported spelling, so the // second is renamed — and a forward reference used to silently point at the first. func TestE2E_ForwardReferenceUsesTheRenamedType(t *testing.T) { - build, types := generateAndBuild(t, `openapi: 3.1.0 + build, files := generateAndBuild(t, `openapi: 3.1.0 info: { title: t, version: "1" } paths: {} components: @@ -125,6 +124,7 @@ components: if build != "" { t.Fatalf("generated client does not compile:\n%s", build) } + types := files["types.go"] if !strings.Contains(types, "A *FooBar `") { t.Errorf("Holder.a does not reference FooBar:\n%s", types) } diff --git a/internal/generator/e2e_recursive_test.go b/internal/generator/e2e_recursive_test.go new file mode 100644 index 0000000..bbb8362 --- /dev/null +++ b/internal/generator/e2e_recursive_test.go @@ -0,0 +1,143 @@ +package generator + +import ( + "strings" + "testing" +) + +// TestE2E_RecursiveSchemasCompile covers the shapes a spec can use to define a +// type in terms of itself. Go allows that only through an indirection, so each +// of these used to generate an "invalid recursive type". +func TestE2E_RecursiveSchemasCompile(t *testing.T) { + tests := []struct { + name string + spec string + expect string + }{ + { + // A tree node: the most common recursive shape there is. + name: "required self reference", + spec: ` + Node: + type: object + required: [child, label] + properties: + label: { type: string } + child: { $ref: "#/components/schemas/Node" }`, + expect: "Child *Node `", + }, + { + name: "mutual reference", + spec: ` + Parent: + type: object + required: [kid] + properties: + kid: { $ref: "#/components/schemas/Kid" } + Kid: + type: object + required: [parent] + properties: + parent: { $ref: "#/components/schemas/Parent" }`, + expect: "Parent *Parent `", + }, + { + // An alias between the two ends still closes the loop. + name: "self reference through an alias", + spec: ` + Wrapper: + type: object + required: [inner] + properties: + inner: { $ref: "#/components/schemas/AliasToWrapper" } + AliasToWrapper: + anyOf: [{ $ref: "#/components/schemas/Wrapper" }, { type: "null" }]`, + expect: "Inner *AliasToWrapper `", + }, + { + // A slice already breaks the recursion, so nothing should change. + name: "self reference through a slice stays a value", + spec: ` + Branch: + type: object + required: [children] + properties: + children: + type: array + items: { $ref: "#/components/schemas/Branch" }`, + expect: "Children []Branch `", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + build, files := generateAndBuild(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas:`+tt.spec+"\n") + if build != "" { + t.Fatalf("generated client does not compile:\n%s", build) + } + if types := files["types.go"]; !strings.Contains(types, tt.expect) { + t.Errorf("types.go missing %q:\n%s", tt.expect, types) + } + }) + } +} + +// TestE2E_RequestBodyWithoutSchemaCompiles covers a body the spec declares +// without saying what goes in it. The operation still needs a parameter type, or +// the method signature is a syntax error. +func TestE2E_RequestBodyWithoutSchemaCompiles(t *testing.T) { + tests := []struct { + name string + body string + expect string + }{ + { + name: "no schema under a raw media type", + body: " content:\n application/xml: {}", + expect: "func (c *Client) Send(ctx context.Context, body []byte) error", + }, + { + name: "no schema under a text media type", + body: " content:\n text/plain: {}", + expect: "func (c *Client) Send(ctx context.Context, body string) error", + }, + { + name: "no schema under json", + body: " content:\n application/json: {}", + expect: "func (c *Client) Send(ctx context.Context, body any) error", + }, + { + // Nothing to send at all, so the method takes no body. + name: "no content at all", + body: " description: nothing", + expect: "func (c *Client) Send(ctx context.Context) error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + build, files := generateAndBuild(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /s: + post: + operationId: send + requestBody: + required: true +`+tt.body+` + responses: { "204": { description: ok } } +`) + if build != "" { + t.Fatalf("generated client does not compile:\n%s", build) + } + ops := files["operations.go"] + if !strings.Contains(ops, tt.expect) { + t.Errorf("operations.go missing %q:\n%s", tt.expect, ops) + } + }) + } +} diff --git a/internal/generator/funcmap.go b/internal/generator/funcmap.go index dc76192..55950b5 100644 --- a/internal/generator/funcmap.go +++ b/internal/generator/funcmap.go @@ -222,17 +222,46 @@ func catchAllValueType(f *ir.Field) string { return strings.TrimPrefix(f.Type, "map[string]") } -func declaredJSONNames(td *ir.TypeDef) []string { +// declaredJSONNames returns the wire names a struct already consumes into +// fields, an embedded type's included: those are promoted onto the struct, so a +// catch-all that re-collected them would emit each one twice. +func declaredJSONNames(pkg *ir.Package, td *ir.TypeDef) []string { var names []string - for _, f := range td.Fields { - if f.CatchAll || f.Embedded || f.JSONName == "" || f.JSONName == "-" { - continue + visited := make(map[string]bool) + + var walk func(td *ir.TypeDef) + walk = func(td *ir.TypeDef) { + if td == nil || visited[td.Name] { + return + } + visited[td.Name] = true + for _, f := range td.Fields { + switch { + case f.CatchAll: + case f.Embedded: + walk(structByName(pkg, f.Type)) + case f.JSONName == "" || f.JSONName == "-": + default: + names = append(names, f.JSONName) + } } - names = append(names, f.JSONName) } + walk(td) return names } +// structByName returns the generated struct of the given Go type, which may be +// written as a pointer where an indirection broke a reference cycle. +func structByName(pkg *ir.Package, goType string) *ir.TypeDef { + goType = strings.TrimPrefix(goType, "*") + for _, td := range pkg.Types { + if td.Name == goType && td.Kind == ir.TypeKindStruct { + return td + } + } + return nil +} + func hasCatchAllTypes(types []*ir.TypeDef) bool { return slices.ContainsFunc(types, func(td *ir.TypeDef) bool { return catchAllField(td) != nil diff --git a/internal/templates/types.go.tmpl b/internal/templates/types.go.tmpl index dec1244..198b77d 100644 --- a/internal/templates/types.go.tmpl +++ b/internal/templates/types.go.tmpl @@ -57,7 +57,7 @@ func (t {{ $typeName }}) MarshalJSON() ([]byte, error) { } extra[key] = raw } - deleteDeclaredProperties(extra, []string{ {{ range $i, $n := declaredJSONNames $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} }) + deleteDeclaredProperties(extra, []string{ {{ range $i, $n := declaredJSONNames $ $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} }) if len(extra) == 0 { return data, nil } @@ -89,18 +89,24 @@ func (t *{{ $typeName }}) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &obj); err != nil { return err } - deleteDeclaredProperties(obj, []string{ {{ range $i, $n := declaredJSONNames $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} }) + deleteDeclaredProperties(obj, []string{ {{ range $i, $n := declaredJSONNames $ $td }}{{ if $i }}, {{ end }}{{ printf "%q" $n }}{{ end }} }) if len(obj) == 0 { return nil } t.{{ .Name }} = make({{ .Type }}, len(obj)) for key, raw := range obj { var value {{ catchAllValueType . }} + // A property that is both undeclared and not of the type the schema gives + // additionalProperties is the least useful thing in the payload, so it is + // dropped rather than failing the decode of everything alongside it. if err := json.Unmarshal(raw, &value); err != nil { - return fmt.Errorf("unmarshaling additional property %q: %w", key, err) + continue } t.{{ .Name }}[key] = value } + if len(t.{{ .Name }}) == 0 { + t.{{ .Name }} = nil + } return nil } {{ end }} @@ -121,7 +127,7 @@ type {{ .Name }} struct { {{- if .Discriminator }} unknownDiscriminator string - raw json.RawMessage + raw string {{- end }} } {{ if .Discriminator }} @@ -140,14 +146,17 @@ func (u {{ .Name }}) UnknownDiscriminator() string { // Raw returns the original JSON of an unrecognized variant, or nil. func (u {{ .Name }}) Raw() json.RawMessage { - return u.raw + if u.raw == "" { + return nil + } + return json.RawMessage(u.raw) } {{ end }} // MarshalJSON implements json.Marshaler for {{ .Name }}. func (u {{ .Name }}) MarshalJSON() ([]byte, error) { {{- if .Discriminator }} if u.IsUnknownVariant() { - return u.raw, nil + return []byte(u.raw), nil } {{- end }} return json.Marshal(u.Value) @@ -185,7 +194,7 @@ func (u *{{ $typeName }}) UnmarshalJSON(data []byte) error { // of the whole payload it happens to appear in. *u = {{ $typeName }}{ unknownDiscriminator: disc.{{ discriminatorFieldName .Discriminator.PropertyName }}, - raw: append(json.RawMessage(nil), data...), + raw: string(data), } return nil } From d8fab4e4aaa83e4cd7bd2f7d29e4b3beb4873024 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Thu, 6 Aug 2026 01:46:46 +0800 Subject: [PATCH 4/4] fix: schemas named like a derived type, and form bodies nothing can encode The identifiers reserved against schema names covered only the ones the templates always declare. The templates also build names at render time -- Params for an operation's parameters and Response for an error body -- which a static list cannot hold. Those are reserved as the spec is walked, before schema names are assigned, so a schema called ListUsersParams no longer redeclares the struct of that name. The ten Err* sentinels errors.go always declares were missing from the list outright. The test could not have caught either, because it iterated the list it was checking; it now parses the generated files and asserts every exported package-level name is reserved, which fails when a template gains a declaration nothing reserved. A form or multipart body whose schema is not an object generated a method taking that scalar, which every call then failed to encode, since the encoders build parts out of an object's properties. Such a body takes a map instead. A trace operation was silently skipped: the walk that is meant to be the single source of truth for a path item omitted it. A byte slice in a multipart body is now sent as the base64 text OpenAPI's format: byte calls for, rather than as a file part. Files arrive as FormFile, and the schemas a multipart body composes with allOf are marked alongside it, so a binary property inherited through composition is still generated as one. Breaking an alias cycle keeps the shape the alias carried: a self referencing array is `= []any` rather than `= any`. --- internal/analyzer/aliascycles.go | 66 ++----- internal/analyzer/analyzer.go | 4 + internal/analyzer/operations.go | 120 ++++++++++-- internal/analyzer/schemas.go | 49 +++-- .../analyzer/schemas_nullable_union_test.go | 43 +++++ .../e2e_additional_properties_test.go | 29 +++ internal/generator/e2e_inline_union_test.go | 48 +++++ internal/generator/e2e_request_body_test.go | 10 + internal/generator/e2e_reserved_names_test.go | 181 ++++++++++++++++++ internal/generator/funcmap.go | 25 ++- internal/ir/types.go | 43 +++++ internal/templates/helpers.go.tmpl | 6 +- internal/templates/reserved.go | 10 + internal/templates/types.go.tmpl | 11 ++ 14 files changed, 544 insertions(+), 101 deletions(-) create mode 100644 internal/generator/e2e_reserved_names_test.go diff --git a/internal/analyzer/aliascycles.go b/internal/analyzer/aliascycles.go index bd3f44f..456ebbc 100644 --- a/internal/analyzer/aliascycles.go +++ b/internal/analyzer/aliascycles.go @@ -43,7 +43,9 @@ func breakAliasCycles(types []*ir.TypeDef) { state[name] = visiting if target := aliasTarget(td.GoType); target != "" && walk(target) { - td.GoType = "any" + // Only the cyclic referent has to go; the slice or map around it is + // still what the caller gets. + td.GoType = strings.TrimSuffix(td.GoType, target) + "any" } state[name] = done return false @@ -61,32 +63,7 @@ func breakAliasCycles(types []*ir.TypeDef) { // alias. A field that is already a pointer, slice, or map stops the recursion on // its own; a required $ref to the enclosing type does not. func breakStructCycles(types []*ir.TypeDef) { - byName := make(map[string]*ir.TypeDef, len(types)) - for _, td := range types { - if td != nil { - byName[td.Name] = td - } - } - - // containedStruct returns the struct a field of this type holds by value, - // following aliases to the definition they stand for. - containedStruct := func(goType string) *ir.TypeDef { - for range len(types) + 1 { - td, ok := byName[namedType(goType)] - if !ok { - return nil - } - switch td.Kind { - case ir.TypeKindStruct: - return td - case ir.TypeKindAlias: - goType = td.GoType - default: - return nil - } - } - return nil - } + byName := ir.TypesByName(types) const ( visiting = 1 @@ -98,7 +75,9 @@ func breakStructCycles(types []*ir.TypeDef) { walk = func(td *ir.TypeDef) { state[td.Name] = visiting for _, f := range td.Fields { - next := containedStruct(f.Type) + // A field already written as a pointer, slice, or map stops the + // recursion on its own, and ir.StructNamed rejects all three. + next := ir.StructNamed(byName, f.Type) if next == nil { continue } @@ -122,22 +101,10 @@ func breakStructCycles(types []*ir.TypeDef) { } } -// dropShadowedCatchAlls removes the catch-all from a struct that embeds a type -// which already has one. The generated marshalers shadow the struct to reach -// encoding/json, and a shadow still promotes an embedded type's MarshalJSON — so -// the two catch-alls would fight and the embedded one would win, emitting only -// its own fields. Leaving the outer schema's undeclared properties uncollected -// is the narrower loss, and it is what the generator did before composed schemas -// collected any at all. -// -// Handling both at once needs the shadow replaced with field-by-field marshaling. +// dropShadowedCatchAlls removes the catch-all from a struct that embeds one, +// whose promoted marshalers would otherwise win and emit only their own fields. func dropShadowedCatchAlls(types []*ir.TypeDef) { - byName := make(map[string]*ir.TypeDef, len(types)) - for _, td := range types { - if td != nil && td.Kind == ir.TypeKindStruct { - byName[td.Name] = td - } - } + byName := ir.TypesByName(types) hasCatchAll := func(td *ir.TypeDef) bool { return slices.ContainsFunc(td.Fields, func(f *ir.Field) bool { return f.CatchAll }) @@ -153,7 +120,7 @@ func dropShadowedCatchAlls(types []*ir.TypeDef) { if !f.Embedded { continue } - embedded := byName[namedType(strings.TrimPrefix(f.Type, "*"))] + embedded := ir.StructNamed(byName, strings.TrimPrefix(f.Type, "*")) if embedded == nil { continue } @@ -191,16 +158,7 @@ func aliasTarget(goType string) string { } goType = goType[end+1:] default: - return namedType(goType) + return ir.NamedType(goType) } } } - -// namedType returns goType when it is a bare type name rather than a builtin or -// a qualified type from another package. -func namedType(goType string) string { - if goType == "" || goType == "any" || strings.ContainsAny(goType, ".[]*{} ") { - return "" - } - return goType -} diff --git a/internal/analyzer/analyzer.go b/internal/analyzer/analyzer.go index 59d326c..d0eab7a 100644 --- a/internal/analyzer/analyzer.go +++ b/internal/analyzer/analyzer.go @@ -67,6 +67,10 @@ func (a *Analyzer) Analyze(packageName string) (*ir.Package, error) { // them are converted. a.multipartBodies = a.collectMultipartBodySchemas() + // Schema names are assigned next, and must avoid the identifiers the templates + // derive from operations and error bodies. + a.reserveDerivedNames() + // Analyze component schemas. if err := a.analyzeComponentSchemas(pkg); err != nil { return nil, err diff --git a/internal/analyzer/operations.go b/internal/analyzer/operations.go index 3c24419..1e887f3 100644 --- a/internal/analyzer/operations.go +++ b/internal/analyzer/operations.go @@ -49,6 +49,7 @@ func pathOperations(pathItem *v3high.PathItem) []pathOperation { {"PATCH", pathItem.Patch}, {"HEAD", pathItem.Head}, {"OPTIONS", pathItem.Options}, + {"TRACE", pathItem.Trace}, } return slices.DeleteFunc(all, func(m pathOperation) bool { return m.op == nil }) } @@ -72,14 +73,79 @@ func (a *Analyzer) collectMultipartBodySchemas() map[string]bool { if !strings.HasPrefix(contentType, "multipart/") || mediaType == nil || mediaType.Schema == nil { continue } - if name := refToSchemaName(mediaType.Schema.GetReference()); name != "" { - names[name] = true - } + a.markMultipartSchema(names, refToSchemaName(mediaType.Schema.GetReference()), 0) } } return names } +// reserveDerivedNames keeps a schema off the identifiers the templates build out +// of an operation or an error body, which share the one package scope with it. +func (a *Analyzer) reserveDerivedNames() { + if a.model.Paths == nil || a.model.Paths.PathItems == nil { + return + } + + for path, pathItem := range a.model.Paths.PathItems.FromOldest() { + for _, m := range pathOperations(pathItem) { + a.namer.Reserve(a.operationName(m.method, path, m.op) + "Params") + + if m.op.Responses == nil || m.op.Responses.Codes == nil { + continue + } + for code, resp := range m.op.Responses.Codes.FromOldest() { + if isErrorCode(code) { + a.reserveErrorResponseName(resp) + } + } + a.reserveErrorResponseName(m.op.Responses.Default) + } + } +} + +// reserveErrorResponseName reserves the wrapper type errors.go declares for an +// error body. +func (a *Analyzer) reserveErrorResponseName(resp *v3high.Response) { + if resp == nil || resp.Content == nil { + return + } + for _, mediaType := range resp.Content.FromOldest() { + if mediaType == nil || mediaType.Schema == nil { + continue + } + if refName := refToSchemaName(mediaType.Schema.GetReference()); refName != "" { + a.namer.Reserve(naming.Exported(refName) + "Response") + } + } +} + +// markMultipartSchema marks a schema and everything it composes with allOf, so a +// binary property inherited through composition is still generated as a file. +func (a *Analyzer) markMultipartSchema(names map[string]bool, refName string, depth int) { + if refName == "" || names[refName] || depth > maxSchemaDepth { + return + } + names[refName] = true + + if a.model.Components == nil || a.model.Components.Schemas == nil { + return + } + proxy, ok := a.model.Components.Schemas.Get(refName) + if !ok || proxy == nil { + return + } + schema, err := proxy.BuildSchema() + if err != nil || schema == nil { + return + } + for _, entry := range schema.AllOf { + a.markMultipartSchema(names, refToSchemaName(entry.GetReference()), depth+1) + } +} + +// maxSchemaDepth bounds a walk over schemas that may refer to one another. +const maxSchemaDepth = 32 + // preferredContent picks the media type a request body is sent as: JSON when the // spec offers a choice, otherwise the first one it lists. func preferredContent(content *orderedmap.Map[string, *v3high.MediaType]) (string, *v3high.MediaType) { @@ -324,28 +390,60 @@ func (a *Analyzer) convertRequestBody(rb *v3high.RequestBody, nameHint string) ( Required: rb.Required != nil && *rb.Required, Description: rb.Description, ContentType: contentType, - TypeName: bodyGoType(contentType, a.resolveMediaTypeSchema(mediaType, nameHint)), + TypeName: bodyGoType(contentType, a.resolveMediaTypeSchema(mediaType, nameHint), mediaTypeSchema(mediaType)), }, nil } -// structuredContentType reports whether the generated client can encode a Go -// value into contentType from the schema alone. -func structuredContentType(contentType string) bool { - return strings.Contains(contentType, "json") || - strings.HasPrefix(contentType, "multipart/") || +// formEncodedContentType reports whether a body is sent as form data, whose +// encoders walk the value property by property. +func formEncodedContentType(contentType string) bool { + return strings.HasPrefix(contentType, "multipart/") || strings.HasPrefix(contentType, "application/x-www-form-urlencoded") } +// mediaTypeSchema builds a media type's schema, resolving a reference to the +// schema it names. +func mediaTypeSchema(mt *v3high.MediaType) *highbase.Schema { + if mt == nil || mt.Schema == nil { + return nil + } + schema, err := mt.Schema.BuildSchema() + if err != nil { + return nil + } + return schema +} + +// isObjectLike reports whether a schema describes something with properties to +// walk rather than a scalar or a list. +func isObjectLike(schema *highbase.Schema) bool { + if schema == nil { + return false + } + if primaryType(schema) == "object" || len(schema.AllOf) > 0 { + return true + } + return schema.Properties != nil && schema.Properties.Len() > 0 +} + // bodyGoType is the Go type a request body is accepted as. A body the client // cannot structurally encode — XML, say — is taken as the bytes or text it // already is rather than as a struct there would be no encoder for, and a body // the spec declares without a schema still needs some type to be passed as. -func bodyGoType(contentType, typeName string) string { - if structuredContentType(contentType) { +func bodyGoType(contentType, typeName string, schema *highbase.Schema) string { + switch { + case strings.Contains(contentType, "json"): if typeName == "" { return "any" } return typeName + case formEncodedContentType(contentType): + // The form encoders build parts and pairs out of an object's properties, + // so a body that is not an object gives them nothing to work from. + if isObjectLike(schema) { + return typeName + } + return "map[string]any" } switch typeName { case "string", "[]byte": diff --git a/internal/analyzer/schemas.go b/internal/analyzer/schemas.go index 9c10efb..80499ab 100644 --- a/internal/analyzer/schemas.go +++ b/internal/analyzer/schemas.go @@ -23,31 +23,32 @@ func (a *Analyzer) convertSchema(goName, specName string, schema *highbase.Schem return a.convertEnum(goName, schema, nullable) } - // A oneOf/anyOf whose only other member is `type: null` is how OpenAPI 3.1 - // spells "nullable T"; it offers no choice to model, so generate T itself. - if variant, ok := nullableUnionVariant(schema); ok { - return a.convertNullableUnion(goName, specName, schema, variant) - } - if goType, ok := a.uniformUnionGoType(schema, goName); ok { - return &ir.TypeDef{ - Name: goName, - Description: schema.Description, - Kind: ir.TypeKindAlias, - GoType: goType, - IsNullable: nullable, - }, nil + if isPureUnion(schema) { + // A oneOf/anyOf whose only other member is `type: null` is how OpenAPI 3.1 + // spells "nullable T"; it offers no choice to model, so generate T itself. + if variant, ok := nullableUnionVariant(schema); ok { + return a.convertNullableUnion(goName, specName, schema, variant) + } + if goType, ok := a.uniformUnionGoType(schema, goName); ok { + return &ir.TypeDef{ + Name: goName, + Description: schema.Description, + Kind: ir.TypeKindAlias, + GoType: goType, + IsNullable: nullable, + }, nil + } + if len(schema.OneOf) > 0 { + return a.convertOneOf(goName, schema, nullable) + } + if len(schema.AnyOf) > 0 { + return a.convertAnyOf(goName, schema, nullable) + } } - // Composition types: allOf, oneOf, anyOf. if len(schema.AllOf) > 0 { return a.convertAllOf(goName, schema, nullable, a.multipartBodies[specName]) } - if len(schema.OneOf) > 0 { - return a.convertOneOf(goName, schema, nullable) - } - if len(schema.AnyOf) > 0 { - return a.convertAnyOf(goName, schema, nullable) - } primaryType := primaryType(schema) @@ -700,6 +701,14 @@ func isNullable(schema *highbase.Schema) bool { return slices.ContainsFunc(unionVariants(schema), isNullVariant) } +// isPureUnion reports whether a schema's oneOf/anyOf is the whole of what it is. +// A schema that also composes or declares properties uses the union to constrain +// the object the rest of it describes, so collapsing to a member would throw that +// away. +func isPureUnion(schema *highbase.Schema) bool { + return len(schema.AllOf) == 0 && (schema.Properties == nil || schema.Properties.Len() == 0) +} + // unionVariants returns a schema's oneOf variants, or its anyOf variants when it // has no oneOf. func unionVariants(schema *highbase.Schema) []*highbase.SchemaProxy { diff --git a/internal/analyzer/schemas_nullable_union_test.go b/internal/analyzer/schemas_nullable_union_test.go index f8b6bb9..50d22bb 100644 --- a/internal/analyzer/schemas_nullable_union_test.go +++ b/internal/analyzer/schemas_nullable_union_test.go @@ -455,3 +455,46 @@ components: } } } + +// TestUnion_AlongsideCompositionKeepsTheObject pins that a oneOf/anyOf used to +// constrain an object -- "exactly one of these is required", or a refinement of +// an allOf -- does not collapse the schema onto one of its members, throwing the +// declared properties and the composition away. +func TestUnion_AlongsideCompositionKeepsTheObject(t *testing.T) { + _, typeMap := analyzeSpec(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Wrapper: + type: object + properties: { w: { type: string } } + ConstrainedObject: + type: object + properties: + a: { type: string } + b: { type: string } + oneOf: + - required: [a] + - required: [b] + ComposedWithUnion: + allOf: [{ $ref: "#/components/schemas/Wrapper" }] + oneOf: [{ type: string }, { type: string, format: date }] +`) + + obj := typeMap["ConstrainedObject"] + if obj == nil || obj.Kind != ir.TypeKindStruct { + t.Fatalf("ConstrainedObject = %+v, want a struct", obj) + } + if len(obj.Fields) != 2 { + t.Errorf("ConstrainedObject has %d fields, want its two declared properties", len(obj.Fields)) + } + + composed := typeMap["ComposedWithUnion"] + if composed == nil || composed.Kind != ir.TypeKindStruct { + t.Fatalf("ComposedWithUnion = %+v, want a struct", composed) + } + if len(composed.Fields) != 1 || !composed.Fields[0].Embedded || composed.Fields[0].Type != "Wrapper" { + t.Errorf("ComposedWithUnion fields = %+v, want the embedded Wrapper", composed.Fields) + } +} diff --git a/internal/generator/e2e_additional_properties_test.go b/internal/generator/e2e_additional_properties_test.go index 0a94de4..4c02984 100644 --- a/internal/generator/e2e_additional_properties_test.go +++ b/internal/generator/e2e_additional_properties_test.go @@ -49,6 +49,15 @@ components: properties: name: { type: string } additionalProperties: true + NullableBase: + anyOf: [{ $ref: "#/components/schemas/Base" }, { type: "null" }] + ComposedThroughAlias: + allOf: + - $ref: "#/components/schemas/NullableBase" + - type: object + properties: + note: { type: string } + additionalProperties: true ` const additionalPropertiesRuntimeTest = `package petsapi @@ -227,6 +236,26 @@ func TestOffTypeExtraDoesNotFailTheDecode(t *testing.T) { t.Error("a property that does not match the declared value type was kept anyway") } } + +// The embedded schema is reached through an alias, so the catch-all still has to +// recognize the properties that alias promotes as already declared. +func TestPropertiesInheritedThroughAnAliasAreNotRecollected(t *testing.T) { + var c ComposedThroughAlias + if err := json.Unmarshal([]byte(` + "`" + `{"id":"x","note":"n","extra":"kept"}` + "`" + `), &c); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if _, ok := c.AdditionalProperties["id"]; ok { + t.Error("a property promoted through an aliased embed landed in the catch-all") + } + + out, err := json.Marshal(c) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if n := strings.Count(string(out), ` + "`" + `"id"` + "`" + `); n != 1 { + t.Errorf("id emitted %d times: %s", n, out) + } +} ` // TestE2E_AdditionalPropertiesRoundTrip generates a client for schemas that mix diff --git a/internal/generator/e2e_inline_union_test.go b/internal/generator/e2e_inline_union_test.go index fa0e5ab..f86c05f 100644 --- a/internal/generator/e2e_inline_union_test.go +++ b/internal/generator/e2e_inline_union_test.go @@ -206,3 +206,51 @@ func TestNullUnionDecodesToTheZeroValue(t *testing.T) { } t.Logf("runtime dispatch test passed:\n%s", string(output)) } + +const untypedVariantSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + schemas: + Mixed: + oneOf: + - { type: string } + - type: object + properties: + q: { type: string } +` + +// TestE2E_UntypedUnionVariantStillDecodes covers a union member the analyzer +// cannot name: it still covers payloads the spec calls valid, and rejecting them +// would fail the whole response they arrive in. +func TestE2E_UntypedUnionVariantStillDecodes(t *testing.T) { + files, _ := generateFromSpec(t, untypedVariantSpec, "mixedapi") + runGeneratedWireTest(t, files, "untypedvariant", `package mixedapi + +import ( + "encoding/json" + "testing" +) + +func TestUntypedVariantDecodes(t *testing.T) { + var m Mixed + if err := json.Unmarshal([]byte(`+"`"+`{"q":"x"}`+"`"+`), &m); err != nil { + t.Fatalf("a payload matching the inline object variant failed to decode: %v", err) + } + obj, ok := m.Value.(map[string]any) + if !ok || obj["q"] != "x" { + t.Errorf("Value = %#v, want the decoded object", m.Value) + } +} + +func TestTypedVariantStillWins(t *testing.T) { + var m Mixed + if err := json.Unmarshal([]byte(`+"`"+`"plain"`+"`"+`), &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if m.Value != "plain" { + t.Errorf("Value = %#v, want the string variant", m.Value) + } +} +`) +} diff --git a/internal/generator/e2e_request_body_test.go b/internal/generator/e2e_request_body_test.go index 70172a7..fc0afb9 100644 --- a/internal/generator/e2e_request_body_test.go +++ b/internal/generator/e2e_request_body_test.go @@ -96,6 +96,7 @@ components: tags: { type: array, items: { type: string } } meta: { $ref: "#/components/schemas/ImageMeta" } attachments: { type: array, items: { type: string, format: binary } } + signature: { type: string, format: byte } ImageMeta: type: object required: [source] @@ -213,6 +214,7 @@ func TestMultipartBodyIsSentAsMultipart(t *testing.T) { Tags: []string{"dinner", "quick"}, Meta: &ImageMeta{Source: "phone"}, Attachments: []FormFile{{Filename: "notes.txt", Content: []byte("first")}, {Content: []byte("second")}}, + Signature: []byte("sig"), }) if err != nil { t.Fatalf("UpdateRecipeImage: %v", err) @@ -276,6 +278,14 @@ func TestMultipartBodyIsSentAsMultipart(t *testing.T) { t.Errorf("meta part = %v, want the object as JSON", got) } + // format: byte is base64 text, not an upload, so it is a value part. + if got := values["signature"]; len(got) != 1 || got[0] != "c2ln" { + t.Errorf("signature part = %v, want the base64 text [c2ln]", got) + } + if _, ok := req.MultipartForm.File["signature"]; ok { + t.Error("format: byte was sent as a file part") + } + // An array of files becomes one file part per element, and a file with no // name of its own falls back to the property name. attachments := req.MultipartForm.File["attachments"] diff --git a/internal/generator/e2e_reserved_names_test.go b/internal/generator/e2e_reserved_names_test.go new file mode 100644 index 0000000..7f1a52b --- /dev/null +++ b/internal/generator/e2e_reserved_names_test.go @@ -0,0 +1,181 @@ +package generator + +import ( + "go/ast" + "go/parser" + "go/token" + "slices" + "strings" + "testing" + + "github.com/parallelworks/openapi-client-generator/internal/templates" +) + +// reservedNamesSpec declares no operations and no error bodies, so every +// exported package-level name in the output but types.go is one the templates +// always declare, with nothing derived mixed in. +const reservedNamesSpec = `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: {} +components: + securitySchemes: + bearer: { type: http, scheme: bearer } + apiKey: { type: apiKey, name: X-Key, in: header } + basic: { type: http, scheme: basic } + schemas: + Thing: + type: object + properties: + name: { type: string } +` + +// TestReservedIdentifiersCoversWhatTheTemplatesDeclare parses the generated +// files and asserts every exported package-level name is reserved. Asserting +// against templates.ReservedIdentifiers by iterating it can only confirm the +// entries already there; this asks the output what it actually declares, so a +// name added to a template without being reserved fails here. +func TestReservedIdentifiersCoversWhatTheTemplatesDeclare(t *testing.T) { + _, files := generateAndBuild(t, reservedNamesSpec) + + fset := token.NewFileSet() + for name, src := range files { + // types.go holds the schemas themselves, which are meant to be spec-named. + if name == "types.go" { + continue + } + file, err := parser.ParseFile(fset, name, src, 0) + if err != nil { + t.Fatalf("parsing generated %s: %v", name, err) + } + for _, declared := range exportedPackageNames(file) { + if !slices.Contains(templates.ReservedIdentifiers, declared) { + t.Errorf("%s declares %q at package scope but it is not in templates.ReservedIdentifiers, "+ + "so a schema of that name would redeclare it", name, declared) + } + } + } +} + +// exportedPackageNames returns the exported types, funcs, vars, and consts a +// file declares at package scope. Methods take no package-scope name. +func exportedPackageNames(file *ast.File) []string { + var names []string + add := func(name string) { + if ast.IsExported(name) { + names = append(names, name) + } + } + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + if d.Recv == nil { + add(d.Name.Name) + } + case *ast.GenDecl: + for _, spec := range d.Specs { + switch s := spec.(type) { + case *ast.TypeSpec: + add(s.Name.Name) + case *ast.ValueSpec: + for _, ident := range s.Names { + add(ident.Name) + } + } + } + } + } + return names +} + +// TestE2E_SchemaNamedLikeADerivedType covers the identifiers the templates build +// at render time rather than always declaring: a params struct is named after +// its operation and an error wrapper after the body it wraps, so neither can sit +// in a static list. +func TestE2E_SchemaNamedLikeADerivedType(t *testing.T) { + build, files := generateAndBuild(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /u: + get: + operationId: listUsers + parameters: [{ name: q, in: query, schema: { type: string } }] + responses: + "200": { description: ok } + "404": + description: nf + content: + application/json: + schema: { $ref: "#/components/schemas/Error" } +components: + schemas: + Error: + type: object + properties: { message: { type: string } } + ErrorResponse: + type: object + properties: { y: { type: string } } + ListUsersParams: + type: object + properties: { z: { type: string } } +`) + if build != "" { + t.Fatalf("generated client does not compile:\n%s", build) + } + types := files["types.go"] + for _, want := range []string{"type ErrorResponse2 struct", "type ListUsersParams2 struct"} { + if !strings.Contains(types, want) { + t.Errorf("types.go missing %q — the schema was not renamed off the derived name:\n%s", want, types) + } + } +} + +// TestE2E_TraceOperationIsGenerated covers the one HTTP method the path-item +// walk used to skip. +func TestE2E_TraceOperationIsGenerated(t *testing.T) { + build, files := generateAndBuild(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /u: + trace: + operationId: traceUsers + responses: { "204": { description: ok } } +`) + if build != "" { + t.Fatalf("generated client does not compile:\n%s", build) + } + if ops := files["operations.go"]; !strings.Contains(ops, "func (c *Client) TraceUsers(") { + t.Errorf("operations.go has no method for the trace operation:\n%s", ops) + } +} + +// TestE2E_FormBodyIsAlwaysEncodable covers a form body whose schema is not an +// object: the encoders walk properties, so a scalar would compile and then fail +// on every call. +func TestE2E_FormBodyIsAlwaysEncodable(t *testing.T) { + for _, tt := range []struct{ name, contentType string }{ + {"urlencoded", "application/x-www-form-urlencoded"}, + {"multipart", "multipart/form-data"}, + } { + t.Run(tt.name, func(t *testing.T) { + build, files := generateAndBuild(t, `openapi: 3.1.0 +info: { title: t, version: "1" } +paths: + /f: + post: + operationId: postForm + requestBody: + content: + `+tt.contentType+`: + schema: { type: string } + responses: { "204": { description: ok } } +`) + if build != "" { + t.Fatalf("generated client does not compile:\n%s", build) + } + ops := files["operations.go"] + if !strings.Contains(ops, "body *map[string]any") { + t.Errorf("a %s body that is not an object should still take something the encoder accepts:\n%s", tt.contentType, ops) + } + }) + } +} diff --git a/internal/generator/funcmap.go b/internal/generator/funcmap.go index 55950b5..58d81d8 100644 --- a/internal/generator/funcmap.go +++ b/internal/generator/funcmap.go @@ -28,6 +28,7 @@ func FuncMap() template.FuncMap { "hasRequiredCookieParams": hasRequiredCookieParams, "paramType": paramType, "hasUnions": hasUnions, + "hasUntypedVariant": hasUntypedVariant, "catchAllField": catchAllField, "catchAllValueType": catchAllValueType, "declaredJSONNames": declaredJSONNames, @@ -226,6 +227,8 @@ func catchAllValueType(f *ir.Field) string { // fields, an embedded type's included: those are promoted onto the struct, so a // catch-all that re-collected them would emit each one twice. func declaredJSONNames(pkg *ir.Package, td *ir.TypeDef) []string { + byName := ir.TypesByName(pkg.Types) + var names []string visited := make(map[string]bool) @@ -239,7 +242,9 @@ func declaredJSONNames(pkg *ir.Package, td *ir.TypeDef) []string { switch { case f.CatchAll: case f.Embedded: - walk(structByName(pkg, f.Type)) + // The type may be written as a pointer where an indirection broke a + // reference cycle, and may be an alias standing for the struct. + walk(ir.StructNamed(byName, strings.TrimPrefix(f.Type, "*"))) case f.JSONName == "" || f.JSONName == "-": default: names = append(names, f.JSONName) @@ -250,18 +255,6 @@ func declaredJSONNames(pkg *ir.Package, td *ir.TypeDef) []string { return names } -// structByName returns the generated struct of the given Go type, which may be -// written as a pointer where an indirection broke a reference cycle. -func structByName(pkg *ir.Package, goType string) *ir.TypeDef { - goType = strings.TrimPrefix(goType, "*") - for _, td := range pkg.Types { - if td.Name == goType && td.Kind == ir.TypeKindStruct { - return td - } - } - return nil -} - func hasCatchAllTypes(types []*ir.TypeDef) bool { return slices.ContainsFunc(types, func(td *ir.TypeDef) bool { return catchAllField(td) != nil @@ -321,6 +314,12 @@ func hasUnions(types []*ir.TypeDef) bool { return false } +// hasUntypedVariant reports whether a union has a variant no Go type could be +// derived for, whose payloads nothing but an any decode accepts. +func hasUntypedVariant(td *ir.TypeDef) bool { + return slices.ContainsFunc(td.UnionTypes, func(v *ir.UnionVariant) bool { return v.TypeName == "any" }) +} + // discriminatorFieldName converts a JSON property name to a Go field name // for use in the discriminator struct in UnmarshalJSON. func discriminatorFieldName(propertyName string) string { diff --git a/internal/ir/types.go b/internal/ir/types.go index 58bbdbf..564b6c0 100644 --- a/internal/ir/types.go +++ b/internal/ir/types.go @@ -1,5 +1,48 @@ package ir +import "strings" + +// TypesByName indexes type definitions by the Go name they declare. +func TypesByName(types []*TypeDef) map[string]*TypeDef { + byName := make(map[string]*TypeDef, len(types)) + for _, td := range types { + if td != nil { + byName[td.Name] = td + } + } + return byName +} + +// NamedType returns goType when it is a bare type name rather than a builtin or a +// composite with no single referent. +func NamedType(goType string) string { + if goType == "" || goType == "any" || strings.ContainsAny(goType, ".[]*{} ") { + return "" + } + return goType +} + +// StructNamed returns the struct goType ultimately denotes, following the aliases +// that may stand between the two. It returns nil for anything that does not end at +// a generated struct. +func StructNamed(byName map[string]*TypeDef, goType string) *TypeDef { + for range len(byName) + 1 { + td := byName[NamedType(goType)] + if td == nil { + return nil + } + switch td.Kind { + case TypeKindStruct: + return td + case TypeKindAlias: + goType = td.GoType + default: + return nil + } + } + return nil +} + // TypeKind represents the kind of Go type to generate. type TypeKind int diff --git a/internal/templates/helpers.go.tmpl b/internal/templates/helpers.go.tmpl index eeacc08..cefe2a4 100644 --- a/internal/templates/helpers.go.tmpl +++ b/internal/templates/helpers.go.tmpl @@ -527,12 +527,12 @@ func writeMultipartField(w *multipart.Writer, name string, rv reflect.Value) err switch { case isByteSlice(rv): - // A nil slice is an unset optional property; an empty non-nil one is an - // empty file the caller asked for. + // Files arrive as a FormFile, so a byte slice here is OpenAPI's + // format: byte — base64 text, not an upload. A nil one is unset. if rv.IsNil() { return nil } - return writeFilePart(w, name, FormFile{Content: rv.Bytes()}) + return w.WriteField(name, formatScalar(rv)) case isSlice(rv): for i := 0; i < rv.Len(); i++ { if err := writeMultipartField(w, name, rv.Index(i)); err != nil { diff --git a/internal/templates/reserved.go b/internal/templates/reserved.go index 7d7bd14..db273b7 100644 --- a/internal/templates/reserved.go +++ b/internal/templates/reserved.go @@ -15,6 +15,16 @@ var ReservedIdentifiers = []string{ "Client", "ClientOption", "DefaultRetryConfig", + "ErrBadGateway", + "ErrBadRequest", + "ErrConflict", + "ErrForbidden", + "ErrGatewayTimeout", + "ErrInternalServerError", + "ErrNotFound", + "ErrServiceUnavailable", + "ErrTooManyRequests", + "ErrUnauthorized", "FormFile", "Middleware", "NewClient", diff --git a/internal/templates/types.go.tmpl b/internal/templates/types.go.tmpl index 198b77d..a9f22fe 100644 --- a/internal/templates/types.go.tmpl +++ b/internal/templates/types.go.tmpl @@ -210,6 +210,17 @@ func (u *{{ $typeName }}) UnmarshalJSON(data []byte) error { errors = append(errors, err) } {{- end }} +{{- end }} +{{- if hasUntypedVariant . }} + // A variant with no Go type of its own still covers payloads the spec says are + // valid, so they decode into any rather than failing as an unmatched variant. + var untyped any + if err := json.Unmarshal(data, &untyped); err == nil { + u.Value = untyped + return nil + } else { + errors = append(errors, err) + } {{- end }} return fmt.Errorf("data did not match any variant of {{ $typeName }}: %v", errors) {{- end }}