From 4b5f5b563e3356ae64f30738890578ddb8922f99 Mon Sep 17 00:00:00 2001 From: Michael McQuade Date: Wed, 19 Aug 2026 19:01:42 -0500 Subject: [PATCH] fix(generator): error body with no type name emits code that is not Go An error response whose schema resolves to a type expression rather than a type name pasted that expression into an identifier position: type map[string]anyResponse struct { func parsemap[string]anyResponse(err error) error { `default: { schema: { type: object } }` and an inline `type: array` both hit it, and the generated package did not compile at all. The wrapper now carries its own name in the IR. A named body still yields Response, so upgrading renames nothing callers match on. A map, a slice, or a builtin takes the name of the operation and status code that produced it, which also fixes a text/plain body landing in an unexported `stringResponse` that errors.As could not name. --- internal/analyzer/operations.go | 22 +++- internal/generator/e2e_error_wrapper_test.go | 121 +++++++++++++++++++ internal/generator/funcmap.go | 24 ++-- internal/ir/operations.go | 13 +- internal/templates/errors.go.tmpl | 16 +-- internal/templates/operations.go.tmpl | 4 +- 6 files changed, 175 insertions(+), 25 deletions(-) create mode 100644 internal/generator/e2e_error_wrapper_test.go diff --git a/internal/analyzer/operations.go b/internal/analyzer/operations.go index 1e887f3..8f49539 100644 --- a/internal/analyzer/operations.go +++ b/internal/analyzer/operations.go @@ -4,6 +4,8 @@ import ( "fmt" "slices" "strings" + "unicode" + "unicode/utf8" highbase "github.com/pb33f/libopenapi/datamodel/high/base" v3high "github.com/pb33f/libopenapi/datamodel/high/v3" @@ -475,6 +477,7 @@ func (a *Analyzer) convertResponses(responses *v3high.Responses, opDef *ir.Opera } } else if isErrorCode(code) { rd.IsError = true + rd.ErrorWrapper = a.errorWrapperName(rd.TypeName, hint) opDef.ErrorResponses = append(opDef.ErrorResponses, rd) } } @@ -482,13 +485,30 @@ func (a *Analyzer) convertResponses(responses *v3high.Responses, opDef *ir.Opera // Handle the default response. if responses.Default != nil { - rd := a.convertSingleResponse("default", responses.Default, opName+"DefaultResponse") + hint := opName + "DefaultResponse" + rd := a.convertSingleResponse("default", responses.Default, hint) rd.IsError = true + rd.ErrorWrapper = a.errorWrapperName(rd.TypeName, hint) opDef.Responses = append(opDef.Responses, rd) opDef.ErrorResponses = append(opDef.ErrorResponses, rd) } } +// errorWrapperName returns the type name for the wrapper that carries an error +// body parsed into Detail. A body whose Go type is a map, a slice, or a builtin +// has no name an identifier can be built from, so the wrapper takes the +// operation's instead of pasting the type expression into the declaration. +func (a *Analyzer) errorWrapperName(typeName, hint string) string { + if typeName == "" { + return "" + } + named := ir.NamedType(typeName) + if r, _ := utf8.DecodeRuneInString(named); unicode.IsUpper(r) { + return named + "Response" + } + return a.namer.Unique(naming.Exported(hint) + "Error") +} + // convertSingleResponse converts one response code/definition to an ir.ResponseDef. func (a *Analyzer) convertSingleResponse(code string, resp *v3high.Response, nameHint string) *ir.ResponseDef { rd := &ir.ResponseDef{ diff --git a/internal/generator/e2e_error_wrapper_test.go b/internal/generator/e2e_error_wrapper_test.go new file mode 100644 index 0000000..e929cb1 --- /dev/null +++ b/internal/generator/e2e_error_wrapper_test.go @@ -0,0 +1,121 @@ +package generator + +import "testing" + +const unnamedErrorBodySpec = `openapi: 3.1.0 +info: { title: errbody, version: "1" } +paths: + /t: + get: + operationId: getT + responses: + "200": { description: ok, content: { application/json: { schema: { type: string } } } } + "404": + description: missing + content: + application/json: + schema: { type: array, items: { type: string } } + default: + description: fallback + content: + application/json: + schema: { type: object } + /u: + get: + operationId: getU + responses: + "200": { description: ok, content: { application/json: { schema: { type: string } } } } + "422": + description: invalid + content: + application/json: + schema: { $ref: "#/components/schemas/ValidationErrors" } + "500": + description: oops + content: + text/plain: + schema: { type: string } +components: + schemas: + ValidationErrors: + type: array + items: { $ref: "#/components/schemas/ValidationError" } + ValidationError: + type: object + properties: + field: { type: string } + message: { type: string } +` + +// TestE2E_UnnamedErrorBodyCompiles covers error bodies whose Go type is an +// expression rather than a name: a map, a slice, and a builtin. Pasting the type +// into the wrapper's declaration put `type map[string]anyResponse` in the output, +// which is not Go. +func TestE2E_UnnamedErrorBodyCompiles(t *testing.T) { + files, _ := generateFromSpec(t, unnamedErrorBodySpec, "errbody") + + runGeneratedWireTest(t, files, "errbody", `package errbody + +import ( + "errors" + "testing" +) + +func TestMapBodyParsesIntoTheWrapper(t *testing.T) { + err := parseGetTDefaultResponseError(&APIError{StatusCode: 500, Status: "500 Internal Server Error", Body: []byte(`+"`"+`{"reason":"upstream"}`+"`"+`)}) + + var wrapped *GetTDefaultResponseError + if !errors.As(err, &wrapped) { + t.Fatalf("errors.As did not match the wrapper: %T", err) + } + if wrapped.Detail["reason"] != "upstream" { + t.Errorf("Detail = %v, want the parsed body", wrapped.Detail) + } + + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != 500 { + t.Error("the wrapper stopped unwrapping to APIError") + } +} + +func TestSliceBodyParsesIntoTheWrapper(t *testing.T) { + err := parseGetTResponse404Error(&APIError{StatusCode: 404, Status: "404 Not Found", Body: []byte(`+"`"+`["gone","really gone"]`+"`"+`)}) + + var wrapped *GetTResponse404Error + if !errors.As(err, &wrapped) { + t.Fatalf("errors.As did not match the wrapper: %T", err) + } + if len(wrapped.Detail) != 2 || wrapped.Detail[0] != "gone" { + t.Errorf("Detail = %v, want the parsed array", wrapped.Detail) + } +} + +// A body whose Go type is a builtin has no name to build an identifier from +// either, and the wrapper it lands in has to stay exported for errors.As. +func TestBuiltinBodyWrapperIsExported(t *testing.T) { + err := parseGetUResponse500Error(&APIError{StatusCode: 500, Status: "500 Internal Server Error", Body: []byte(`+"`"+`"plain text"`+"`"+`)}) + + var wrapped *GetUResponse500Error + if !errors.As(err, &wrapped) { + t.Fatalf("errors.As did not match the wrapper: %T", err) + } + if wrapped.Detail != "plain text" { + t.Errorf("Detail = %q, want the parsed body", wrapped.Detail) + } +} + +// A named body keeps the wrapper name it already had, so upgrading does not +// rename types callers match on. +func TestNamedBodyKeepsItsWrapperName(t *testing.T) { + err := parseValidationErrorsResponse(&APIError{StatusCode: 422, Status: "422 Unprocessable Entity", Body: []byte(`+"`"+`[{"field":"name","message":"required"}]`+"`"+`)}) + + var wrapped *ValidationErrorsResponse + if !errors.As(err, &wrapped) { + t.Fatalf("errors.As did not match the wrapper: %T", err) + } + if len(wrapped.Detail) != 1 || wrapped.Detail[0].Field == nil || *wrapped.Detail[0].Field != "name" { + t.Errorf("Detail = %v, want the parsed body", wrapped.Detail) + } +} +`) +} diff --git a/internal/generator/funcmap.go b/internal/generator/funcmap.go index 949f246..8480cff 100644 --- a/internal/generator/funcmap.go +++ b/internal/generator/funcmap.go @@ -612,18 +612,26 @@ func paginationCursorField(op *ir.OperationDef) string { } // uniqueErrorTypes returns deduplicated error response type names from all operations. -func uniqueErrorTypes(pkg *ir.Package) []string { +func uniqueErrorTypes(pkg *ir.Package) []ErrorWrapper { seen := map[string]bool{} - var types []string + var wrappers []ErrorWrapper for _, op := range pkg.Operations { for _, resp := range op.ErrorResponses { - if resp.TypeName != "" && !seen[resp.TypeName] { - seen[resp.TypeName] = true - types = append(types, resp.TypeName) + if resp.ErrorWrapper == "" || seen[resp.ErrorWrapper] { + continue } + seen[resp.ErrorWrapper] = true + wrappers = append(wrappers, ErrorWrapper{Name: resp.ErrorWrapper, Detail: resp.TypeName}) } } - return types + return wrappers +} + +// ErrorWrapper is one generated error type: the name it declares and the type of +// the body it parses into. +type ErrorWrapper struct { + Name string + Detail string } // errorMessageField returns the error type's string field annotated with @@ -645,8 +653,8 @@ func errorMessageField(pkg *ir.Package, typeName string) *ir.Field { // errorType returns the error response type name for an operation, or "". func errorType(op *ir.OperationDef) string { for _, resp := range op.ErrorResponses { - if resp.TypeName != "" { - return resp.TypeName + if resp.ErrorWrapper != "" { + return resp.ErrorWrapper } } return "" diff --git a/internal/ir/operations.go b/internal/ir/operations.go index 23c86a1..1e25388 100644 --- a/internal/ir/operations.go +++ b/internal/ir/operations.go @@ -45,12 +45,13 @@ type RequestBodyDef struct { // ResponseDef describes one response. type ResponseDef struct { - StatusCode string // "200", "404", "default", etc. - Description string - ContentType string - TypeName string // Go type for the response body (empty if no body) - IsError bool // Whether this is an error response (4xx/5xx) - Headers []*ResponseHeaderDef + StatusCode string // "200", "404", "default", etc. + Description string + ContentType string + TypeName string // Go type for the response body (empty if no body) + ErrorWrapper string // Go type name of the generated wrapper carrying the parsed body + IsError bool // Whether this is an error response (4xx/5xx) + Headers []*ResponseHeaderDef } // ResponseHeaderDef describes a response header. diff --git a/internal/templates/errors.go.tmpl b/internal/templates/errors.go.tmpl index 0140122..8b68002 100644 --- a/internal/templates/errors.go.tmpl +++ b/internal/templates/errors.go.tmpl @@ -63,14 +63,14 @@ func (e *APIError) Is(target error) bool { return e.StatusCode == t.StatusCode } {{ range uniqueErrorTypes . }} -// {{ . }}Response wraps an APIError with a parsed {{ . }} body. -type {{ . }}Response struct { +// {{ .Name }} wraps an APIError with a parsed {{ .Detail }} body. +type {{ .Name }} struct { *APIError - Detail {{ . }} + Detail {{ .Detail }} } -func (e *{{ . }}Response) Error() string { -{{- with errorMessageField $ . }} +func (e *{{ .Name }}) Error() string { +{{- with errorMessageField $ .Detail }} {{- if .IsPointer }} if e.Detail.{{ .Name }} != nil && *e.Detail.{{ .Name }} != "" { return fmt.Sprintf("API error %s: %s", e.statusLabel(), *e.Detail.{{ .Name }}) @@ -84,16 +84,16 @@ func (e *{{ . }}Response) Error() string { return e.APIError.Error() } -func (e *{{ . }}Response) Unwrap() error { +func (e *{{ .Name }}) Unwrap() error { return e.APIError } -func parse{{ . }}Response(err error) error { +func parse{{ .Name }}(err error) error { var apiErr *APIError if !errors.As(err, &apiErr) || len(apiErr.Body) == 0 { return err } - resp := &{{ . }}Response{APIError: apiErr} + resp := &{{ .Name }}{APIError: apiErr} if json.Unmarshal(apiErr.Body, &resp.Detail) == nil { return resp } diff --git a/internal/templates/operations.go.tmpl b/internal/templates/operations.go.tmpl index 58281b2..85cbb84 100644 --- a/internal/templates/operations.go.tmpl +++ b/internal/templates/operations.go.tmpl @@ -46,11 +46,11 @@ type {{ .Name }}Params struct { {{- $errType := errorType . -}} {{ if successType . }} var result {{ successType . }} 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 nil, {{ if $errType }}parse{{ $errType }}(err){{ else }}err{{ end }} } return &result, 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 {{ if $errType }}parse{{ $errType }}(err){{ else }}err{{ end }} } return nil {{ end }}}