Skip to content

feat(output-options): add default-response-error for unmatched responses - #2561

Draft
DrMkdaddy wants to merge 1 commit into
oapi-codegen:mainfrom
DrMkdaddy:noor/default-response-error
Draft

DrMkdaddy wants to merge 1 commit into
oapi-codegen:mainfrom
DrMkdaddy:noor/default-response-error

Conversation

@DrMkdaddy

Copy link
Copy Markdown

Fixes #1923.

Problem

A response parsing function generated by ClientWithResponses silently accepts a response that matches no declared case:

switch {
case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 200:
	...
case strings.Contains(rsp.Header.Get("Content-Type"), "json") && rsp.StatusCode == 404:
	...
}

return response, nil

If the server returns 500 and the spec declares only 200 and 404, no case matches. The wrapper is returned with err == nil and its typed fields left at their zero value, so a caller cannot distinguish "unrecognised response" from "recognised response with an empty body". As raised in #1923, that makes an unexpected response easy to miss.

Change

New opt-in output option default-response-error, defaulting to false so existing generated output is unchanged. When enabled, the switch gains:

default:
	return nil, fmt.Errorf("unexpected response status: %d", rsp.StatusCode)
output-options:
  default-response-error: true

The error also propagates through <Op>WithResponse, which delegates to the parse function.

Interaction with a spec-declared default response

A spec that declares a default response still takes precedence: its case clause is emitted before the generated default: and is evaluated first. The new case only fires for responses the spec does not describe at all.

On the error value

It is a plain fmt.Errorf carrying the status code rather than a sentinel, to keep the change small and avoid emitting a new exported symbol into every generated package. If you would prefer an exported sentinel (for example ErrUnexpectedResponse) that callers can match with errors.Is, I am happy to switch to that.

Tests

New fixture internal/test/options/default_response_error/, following the existing response_getters layout: one spec declaring 200 + 404, generated twice into separate packages.

  • enabled/: a 200 still parses normally, and a 500 returns an error naming the status code.
  • default/: a 500 still returns the wrapper with no error, pinning the unchanged default behaviour.

make tidy && make test && make generate && make lint all pass locally, and make generate produces no changes outside the new fixture.

AI assistance was used to develop this change. I have reviewed it, tested it, and can explain every line.

Response parsing functions generated by ClientWithResponses silently return the response wrapper when a response's status code and content type match no declared case: the wrapper is left at its zero value and err is nil, so callers cannot distinguish an unrecognised response from a recognised response with an empty body.

Add an opt-in output-options.default-response-error that emits a default: case returning an error for such responses. It defaults to false, so existing generated output is unchanged.

A spec-declared default response still takes precedence: its case clause is emitted before the generated default: and is evaluated first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add an output option for generating a default case in response parsing function

1 participant