Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handles whether a request body is set as required or not in the specification. #1608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emilien-puget
Copy link

@emilien-puget emilien-puget commented May 11, 2024

As briefly discussed there

The current behavior of the code generation is a mix between always required and always optional bodies.
However the open api specification is pretty clear about that requirement of body
see https://spec.openapis.org/oas/v3.1.0#fixed-fields-10 or https://swagger.io/docs/specification/describing-request-body/

Request bodies are optional by default.

this PR aims to bring the code generation closer to the intent of the open api specification.

using this spec as an example

  /requiredrequests/:
    post:
      requestBody:
        required: true
        content:
....
  /requests/:
    post:
      requestBody:
        required: false

type declaration

The issue here is that a body is always optional, using a pointer to convey this intent.

current behavior:

type PostRequiredrequestsRequestObject struct {
	Body *PostRequiredrequestsJSONRequestBody
}

a required type will always be a pointer.

suggested behavior:

type PostRequiredrequestsRequestObject struct {
	Body PostRequiredrequestsJSONRequestBody
}

we drop the ptr since the body was marqued as required in the open api specification.

as seen here, the type generation is considering a request body as being always optional.

strict function handler

The issue here is that a body is always required.

current behavior

var request PostRequestsRequestObject

var body PostRequestsJSONRequestBody
if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
	sh.options.RequestErrorHandlerFunc(w, r, fmt.Errorf("can't decode JSON body: %w", err))
	return
}
request.Body = &body

suggested behavior:

var request PostRequestsRequestObject

if strings.HasPrefix(r.Header.Get("Content-Type"), "application/json") {

	var body PostRequestsJSONRequestBody
	if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
		sh.options.RequestErrorHandlerFunc(w, r, fmt.Errorf("can't decode JSON body: %w", err))
		return
	}
	request.JSONBody = &body
}

since the api specification states that the body is not required, which is the default value for the required attribute, we need to check that the body is given to us before trying to decode it.

this may not be a great way to check that the body is there, since a user of the api could set the content type header and not setting a body.

I think this proposed change should be at one point the default behavior of the code generation as it is the default behavior of the open api specification

wdyt ?

  • implements the same behavior for all the strict server
  • body check detection using content type header may be a bit si bit simplistic

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.

None yet

1 participant