deepmap
Required request body must be parsed by the consumer
Consider the following API spec:
paths:
/users:
post:
tags:
- users
summary: Create a new user
operationId: createUser
requestBody:
$ref: '#/components/requestBodies/CreateUserRequest'
responses:
'200':
$ref: '#/components/responses/CreateUserResponse'
...
components:
requestBodies:
CreateUserRequest:
required: true
content:
application/json:
schema:
type: object
properties:
email:
type: string
password:
type: string
format: password
required:
- email
- password
when I run this through oapi-codegen the generated server looks like this:
// ServerInterface represents all server handlers.
type ServerInterface interface {
// Create a new user
// (POST /users)
CreateUser(w http.ResponseWriter, r *http.Request)
}
// CreateUser operation middleware
func (siw *ServerInterfaceWrapper) CreateUser(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
var handler = func(w http.ResponseWriter, r *http.Request) {
siw.Handler.CreateUser(w, r)
}
for _, middleware := range siw.HandlerMiddlewares {
handler = middleware(handler)
}
handler(w, r.WithContext(ctx))
}
In this case when implementing the ServerInterface I need to write the boilerplate that parses the request body like this:
func (s *Server) CreateUser(w http.ResponseWriter, r *http.Request) {
req := new(CreateUserRequest)
if err := json.NewDecoder(r.Body).Decode(req); err != nil {
http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
In this case it looks like that required: true in the request body object does not make any difference (the code generated with and without it is the same). Wouldn't it be better if the generated method accepts the body as third argument and the parsing boilerplate is generated. Something like this:
// generated
// CreateUser operation middleware
func (siw *ServerInterfaceWrapper) CreateUser(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
var handler = func(w http.ResponseWriter, r *http.Request) {
req := new(CreateUserRequest)
if err := json.NewDecoder(r.Body).Decode(req); err != nil {
http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
siw.Handler.CreateUser(w, r, req)
}
for _, middleware := range siw.HandlerMiddlewares {
handler = middleware(handler)
}
handler(w, r.WithContext(ctx))
}
// ServerInterface represents all server handlers.
type ServerInterface interface {
// Create a new user
// (POST /users)
CreateUser(w http.ResponseWriter, r *http.Request, req CreateUserRequest)
}
What I suggest is similar to what the tool does when there are required parameters. The parameters are collected from the headers/query into a struct, and if something is missing it returns bad request.
One downside of this is that the implementor has less control over the response when the request body fails to parse, but at least in my experience this is something I rarely cared about and almost always copy-pasted the logic with json.NewDecoder and Bad Request with generic msg on error. To mitigate this we could put this feature under a flag, and control it via the cmd invocation on oapi-codegen.
NOTE: If you agree with me that this is a useful feature, I can work on it and contribute a PR to the library.
I'm planning on having a V2 version of this library, which allows users to register Content-Type keyed decoders/encoders and this code will change quite drastically.
I agree that it would be very handy to decode the body without the boilerplate. I was unable to do that because of the presence of content types which we don't have code to handle.
So, your proposal works great for things we are capable of unmarshalling right now, but not so well for content types which we don't directly support, or requests which may have multiple content types.
@deepmap-marcinr thanks for the response. I am so used to JSON APIs, I tend to forget there are multiple types of Content Type one can use for APIs.
I think that what you have said definitely makes sense and just putting a JSON decoder inside the generated code won't work for everyone.
If you are going to develop v2 in the open (which I suppose you would do) I can try to help with some of the work.
I suppose that it would be something like this:
// generated
// CreateUser operation middleware
func (siw *ServerInterfaceWrapper) CreateUser(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
var handler = func(w http.ResponseWriter, r *http.Request) {
req := new(CreateUserRequest)
decoder, err := siw.GetDecoder(r)
if err != nil {
http.Error(w, "No decoder for that content-type", http.StatusBadRequest)
return
}
if err := decoder.Decode(req); err != nil {
http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
siw.Handler.CreateUser(w, r, req)
}
for _, middleware := range siw.HandlerMiddlewares {
handler = middleware(handler)
}
handler(w, r.WithContext(ctx))
}
where GetDecoder would look at the content-type header of the passed request and return a decoder based on that (or error if no decoder).
And the consumer could register decoders where the ServerInterfaceWrapper is instantiated.
Any news here? This would really enhance this library. A second step here could be to somehow run a validation hook aswell before passing it the the controller. I.e building on your example @asankov (pseudocode-ish):
....
if err := decoder.Decode(req); err != nil {
http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
validator := siw.GetValidator() //Struct tag based validator, ex https://github.com/go-playground/validator
if validator != nil{
if err := validator.Validate(req); err != nil{
http.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest)
return
}
}
siw.Handler.CreateUser(w, r, req)
}
....
