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

Ideas about new functions: chi support, request/response validation #2

Closed
kak-tus opened this issue May 2, 2019 · 10 comments
Closed

Comments

@kak-tus
Copy link
Contributor

kak-tus commented May 2, 2019

I have some ideas to new functions and I am ready to code them.

  1. Add command line option to produce chi compatible server interface and handlers.
  2. Add support for full request validation.
    a) add middleware for chi router with params validation and secure validation;
    b) add secure validation to echo middleware.

For security validation there is needed for "AuthenticationFunc". I prefer to add it not as callback function, but as interface function.

  1. Add support for full response validation. There is two options.

a) validation may be added as on runtime basis as middleware: get generated response and validate it with openapi3filter. It is simple, but adds time to request processing;

b) static validation. We can generate boilerplate, so interface functions will be returns predefined genereated structs from boilerplate, these struct will be self-validated and produce from them json responses. It is more complicated, but will be more user-friendly to developer and gets better perfomance.

Please, say your opinion about this features.

@mromaszewicz
Copy link
Contributor

Yes, I'm planning on response validation in the same way as request validation, it will be very similar. I think it's important for unit tests to validate responses, but in production, we won't.

@kak-tus
Copy link
Contributor Author

kak-tus commented May 3, 2019

About response validation - I mean something like this (in petstore example)

type ServerInterface interface {
	//  (GET /pets)
	FindPets(ctx echo.Context, params FindPetsParams) (Pet, error)
...

...
// Wrapper for FindPets
func (w *ServerInterfaceWrapper) FindPets(ctx echo.Context) error {
...
	// Invoke the callback with all the unmarshalled arguments
	res, err = w.Handler.FindPets(ctx, params)
        if err != nil {
	  return err
        }

        return json.NewEncoder(ctx.Response().Writer).Encode(res)
}

This will get static compile time validation and needed performance with easy usage.

@mromaszewicz
Copy link
Contributor

I'm hesitant to do something like that, because responses may not be JSON. If you look at the petstore example in the code, we do produce the JSON response body, and you just call ctx.JSON() to do what you propose. However, you can have a response body which is a string or a binary payload, and an operation typically has multiple response body types (normal body, error, etc). I don't want to bake the assumption that the response is a JSON response into the handler code, that's up to the application logic.

@kak-tus
Copy link
Contributor Author

kak-tus commented May 3, 2019

Of cause, json encoder in handler code is just an example.
Handler can form response body by content type (media type) of schema.

application/json can produce
FindPets(ctx echo.Context, params FindPetsParams) (Pet, error)

text/html can produce
FindPets(ctx echo.Context, params FindPetsParams) (string, error)

text/csv can produce
FindPets(ctx echo.Context, params FindPetsParams) ([]byte, error)

and so on.

About multiple response. In case of schema with multiple responses we can form struct, that can contain other responses, like this

type PetResponse struct {
  Pet *Pet
  Error *Error
}

Also, this format of output can be selected by command line option.
So, this type of ServerInterface (and handlers) can be produced only in case of this option.

@mromaszewicz
Copy link
Contributor

Yeah, that makes sense. It's a lot of work, and I'll have to do a V2 of the library, since the API would change in an incompatible way. I don't know if you've seen the .tmpl files which generate this stuff, but it's already getting quite complicated. Right now, you have to do a little bit of extra work in sending the payload to the request context. When I do get the time to change the API, I'd also like to make it independent of echo, to support chi, or gin or whatever else.

@dannymidnight
Copy link
Contributor

Add command line option to produce chi compatible server interface and handlers.

👍 I'd be keen to see generated http handlers that aren't tied to echo and happy to help move this along if there's appetite for it.

@aca
Copy link
Contributor

aca commented Jan 29, 2020

@dannymidnight thanks for your hard work. I am using chi server at work and I love it.

sljeff referenced this issue in sljeff/oapi-codegen Mar 25, 2021
Generate all responses with nested structs
@kak-tus
Copy link
Contributor Author

kak-tus commented Aug 14, 2021

It seems, this issue may be closed in near future?

Chi support and request validation works excellent. @dannymidnight and @wangyoucao577 thanks again.

@sljeff merged PR in his repo, that produces types for generated functions.
Your code likes to realise functionality, that I wrote before.
@sljeff, do you plan to add PR to this repo?
Functions with responses is not compatible with old generated interface, but it can be added with new command line option.

@sljeff
Copy link

sljeff commented Aug 15, 2021

It seems, this issue may be closed in near future?

Chi support and request validation works excellent. @dannymidnight and @wangyoucao577 thanks again.

@sljeff merged PR in his repo, that produces types for generated functions.
Your code likes to realise functionality, that I wrote before.
@sljeff, do you plan to add PR to this repo?
Functions with responses is not compatible with old generated interface, but it can be added with new command line option.

Okay, I'll submit it in a few days

@kak-tus
Copy link
Contributor Author

kak-tus commented Jul 3, 2022

Close by #499.

@kak-tus kak-tus closed this as completed Jul 3, 2022
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

No branches or pull requests

5 participants