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

Strict server generation #499

Merged
merged 24 commits into from Jun 29, 2022
Merged

Strict server generation #499

merged 24 commits into from Jun 29, 2022

Conversation

Warboss-rus
Copy link
Contributor

@Warboss-rus Warboss-rus commented Dec 14, 2021

Allows generating code that will automatically parse the request and response bodies. The advantages are:

  • Less code needs to be written manually, service code feels more like RPC
  • Service code is forced to comply with schema, anything that is not in schema cannot be extracted or written to response (hence the name "strict")
  • Powerful middlewares that get access to raw request and response data as well as parsed structures. For example, we use 3 middlewares: authorization (parse token and put it into context), logging (logs request data, operation id, processing time, etc.) and errors middleware (turns go errors into response structures)
  • Abstracts service code from server (you can switch from echo to gin to chi without touching service code, just middlewares)
  • It is optional, so if you are not interesting in request and response bodies (for example, when proxying) you just use existing servers

How it works and how to use it:

  • Add strict-server to generate parameter among one of existing servers (echo, gin and chi are supported)
  • This will generate its own interface called StrictServerInterface and server-specific strictHandler, that implements vanilla ServerInterface. That way we reuse all of the code of existing services without need of duplication
  • StrictServerInterface uses request and response structures. This makes it easy to pass them to middlewares, log them, etc. A structure is generated for each request, that contains all params (including path params), content type (if it is ambiguous) and all the bodies (if request supports multiple content types only one body field will be initialized). Structures are also generated for all response body combinations, so for example, returning AddPet200JSONResponse sets the content type to "application/json", status code to 200 and marshals the body structure. Response headers, custom content types (if not fixed), custom response codes (if not fixed) are also supported.
  • StrictServerInterface methods return interface{} that can be one of response structures, nil or error. Unfortunately go doesn't support union types, so we can't clearly declare what can be returned in the interface itself
  • application/json, text/plain, multipart/form-data and application/x-www-form-urlencoded content types are supported. Anything else is passed as an io.Reader. multipart/form-data and application/x-www-form-urlencoded require additional data from request, so we have to support them. application/x-www-form-urlencoded are now can be marhsalled to a json -tagged struct, while multipart/form-data is being passed as a multipart.Reader (because you may want to go through parts manually for perfomance reasons. You can still call a binder from runtime to bind it to the struct in your code)
  • type: string, format: binary now transforms into *multipart.FileHeader. Required for formdata binding.

Why is this a draft and what is not complete:

  • Finalize the terminology. Strict is the best I came up with, but would love to hear suggestions.
  • Proper formdata binding. Gin supports full multipart binding, Echo supports fields, but not files and Chi supports nothing. I've implemented a custom binder that allows binding url.Values to json tags.
  • Formdata support by client. If we support formdata in server we have to support it in client as well, so reverse binding is also required. There is a problem though as multipart.FileHeader cannot be constructed, so I wrote a File wrapper. application/x-www-form-urlencoded is working fine now, and I decided to not add multipart support to client for now, since server doesn't automatically bind multipart values, so client doesn't have to support it. I will probably do it in a separate PR later.
  • Tests. An example is added with chi as a server, but code has a lot of tricky parts that could be covered.
  • Reusable responses. It is very convinient to reuse error responses and return them from middlewares. Generator will try to use aliases whenether possible so you can return common responses from middlewares and it will work.
  • Support additional properties in formdata binding. Does not work for now.
  • Add documentation to README

Small example of a single method from petstore:

type AddPetRequestObject struct {
	Body *AddPetJSONRequestBody
}

type AddPet200JSONResponse = Pet

type AddPetdefaultJSONResponse struct {
	Body       Error
	StatusCode int
}

func (t AddPetdefaultJSONResponse) MarshalJSON() ([]byte, error) {
	return json.Marshal(t.Body)
}

// StrictServerInterface represents all server handlers.
type StrictServerInterface interface {
	// Creates a new pet
	// (POST /pets)
	AddPet(ctx context.Context, request AddPetRequestObject) interface{}
}

Thats all, would love to hear your thoughts and suggestions on this. Is anyone interested?

@deepmap-marcinr
Copy link
Contributor

Oh wow, this is a large, impressive PR. I'm slowly going through it.

I think that "Strict" is a good word for it. Our code in here is intentionally more permissive, since at the beginning, we had many more limitations and wanted to fail open and leave more difficult validation to the handler code itself. I do like the idea of putting it behind a flag, however, I have to dig through more and understand what effect this might have on current schemas. It looks like it won't be breaking anything, which is good.

I wish I had spent more time on conformance tests of some kind!

ilya.bogdanov added 3 commits May 22, 2022 18:34
# Conflicts:
#	cmd/oapi-codegen/oapi-codegen.go
#	examples/authenticated-api/echo/api/api.gen.go
#	examples/petstore-expanded/chi/api/petstore.gen.go
#	examples/petstore-expanded/echo/api/petstore-types.gen.go
#	examples/petstore-expanded/petstore-client.gen.go
#	internal/test/client/client.gen.go
#	internal/test/components/components.gen.go
#	internal/test/issues/issue-312/issue.gen.go
#	internal/test/schemas/schemas.gen.go
#	internal/test/server/server.gen.go
#	pkg/codegen/codegen.go
#	pkg/codegen/templates/request-bodies.tmpl
@Warboss-rus
Copy link
Contributor Author

Warboss-rus commented May 22, 2022

An update on the state of this PR. Most of the work is done,I just need to deal with some bugs with and additional properties, complete client implementation and write some documentation.
As for the effect on existing code - minimal impact on server side (type: string, format: binary will be converted to a new File type and a fix to request body referencing), expanded client with support for text/plain and application/x-www-form-urlencoded (can remove it or move into a separate PR if it is too much changes)
I've also updated the first message with additional information and an example

…irst one will use an alias, all others will generate new structs
@deepmap-marcinr
Copy link
Contributor

Ok, wow, nice changes here.

I originally intended for oapi-codegen to be more like what you have written, however, I decided not to go that route because of lots of edge cases that are difficult to deal with, in terms of interpreting multiple types. To that end, I've built everything in layers, with really flexible, and verbose on the bottom, and nicer to use built on top of that. For instnace, we generate helpers to generate any kind of request, but on top of that, we build a client which makes that easier, etc. You have made a number of simplifying assumptions about encoding responses, which limit what the server can do, in exchange for having a cleaner interface, so if I may make a proposal, call this SimplifiedServer not StrictServer. What do you think?

What your server can not do is handle unsupported content types - where the more generic ServerInterface leaves that to the user. Your auxiliary types,specific to each request or response type make it easy to use type switches, though it is possible to do crazy stuff like have two different application/json types, for example, I can specify a request body for application/json and application/vnd.api+json. Both of these should be handled as JSON and your type generation will break on this.

Because you built this as an adapter around the existing ServerInterface, it's optional to the user, which is great.

I think that the README.md needs some more info on the simplifying assumptions, but then I'm happy to merge this. this is the most complex work anyone has done in oapi-codegen outside of me, thank you.

ilya.bogdanov added 2 commits June 1, 2022 11:08
# Conflicts:
#	cmd/oapi-codegen/oapi-codegen.go
#	pkg/codegen/codegen.go
#	pkg/codegen/configuration.go
#	pkg/codegen/operations.go
@Warboss-rus
Copy link
Contributor Author

Thanks, @deepmap-marcinr, but I have a few corrections:

You have made a number of simplifying assumptions about encoding responses, which limit what the server can do, in exchange for having a cleaner interface, so if I may make a proposal, call this SimplifiedServer not StrictServer. What do you think?

The idea behind strict server is to always give user code an ability to get the required data, if permitted by schema. Thats why I bothered with application/x-www-form-urlencoded and multipart content types, since they require additional data from headers and\or query. Most other unsupported types can be handled manually by user code. So I don't think "simple" is a good word here, its just automation and strictness.

Both of these should be handled as JSON and your type generation will break on this.

It won't break, io.Reader will be generated for all unsupported types, so it can be manually parsed by user code. Returning JSON is little bit trickier (since you have to return io.Reader), but in can be done with something like bytes.Buffer or even io.Pipe. Yes, we lose the strictness, but it will still work. The same problem I had with multipart requests - I could go a strict way, parse the request into a form and bind it to struct, but we would lose flexibility and efficiency of manual part iteration. So I made a choice to provider a reader and tools to bind the data in user code.

@deepmap-marcinr
Copy link
Contributor

Oh, I didn't notice the part about unhandled request types, nice.

There are still other things which are blocked. For example, in echo, you can return a reader to the router when you call ctx.Stream. Streaming responses don't appear possible. We don't need to call is SimplifiedServer.

@Warboss-rus
Copy link
Contributor Author

Streaming responses don't appear possible.

It is possible with all 3 servers - all unsupported content types in responses will generate io.Reader and an optional content length. It will even close the reader, if it implements io.ReadCloser.

@deepmap-marcinr
Copy link
Contributor

Hmm. So, do you consider this ready to go in as-is?

@deepmap-marcinr
Copy link
Contributor

I see it now. Yeah. I was looking through generated code to get an idea of what things work, and it's a lot to sift through.

@Warboss-rus
Copy link
Contributor Author

Hmm. So, do you consider this ready to go in as-is?

It needs documentation and some little touch ups on the binding side. It is something like 98% complete right now. I will try to finish it ASAP, but I don't have a lot of free time, so I would guess it will take me a month to get it to a fully complete state

@Warboss-rus Warboss-rus marked this pull request as ready for review June 29, 2022 09:28
@Warboss-rus
Copy link
Contributor Author

@deepmap-marcinr The PR is ready for review, I've fixed some small errors and added short documentation. I don't have a lot of free time to complete some optional stuff (like multipart binding for client), so it is going to come later, in a separate PR (or maybe someone else can do it).

@deepmap-marcinr deepmap-marcinr merged commit 6f9e7bd into deepmap:master Jun 29, 2022
@Warboss-rus Warboss-rus deleted the strict branch June 29, 2022 16:03
adrianpk pushed a commit to foorester/oapi-codegen that referenced this pull request Jan 16, 2024
* Added strict server generation that does automatic marshaling of request and response bodies, meaning less manual code and forced schema compliance

* Support any content type by using io.Reader

* removed generating marshalling code from client for content types other than JSON

* Multipart and formdata are now passed as forms without any binding

* run make generate to fix the build

* added sorting of request bodies to reduce number of random changes in generated code, fixed content type in RequestBody's comment

* Implemented basic formdata binder (not yet integrated into strict server)

* Added tests for strict generation, support for form marshalling, client improvements

* Fixed incorrect referencing of request bodies, added sorting of response definitions

* Added sorting of content types in responses

* Support multipart responses using callback function, added header example

* Added sorting of headers in response objects

* Added proper testing to strict servers

* Fix after master merge

* Reuse responses defined in components section, moved strict test to tests

* When multiple responses ref to a single reusable response, only the first one will use an alias, all others will generate new structs

* Update generated code after merge

* Some documentation for strict server

* Support for AdditionalProperties when binding forms

Co-authored-by: ilya.bogdanov <ilya.bogdanov@ispringsolutions.com>
Co-authored-by: ilya.bogdanov <ilya.bogdanov@ispring.com>
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

2 participants