wI2L / fizz

:lemon: Gin wrapper with OpenAPI 3 spec generation
https://pkg.go.dev/github.com/wI2L/fizz
MIT License
214 stars 52 forks source link

Make tonic wrapping optional #52

Closed gablemire closed 3 years ago

gablemire commented 3 years ago

Hello.

I'm in the middle of implementing Fizz for an API. I saw there is a mandatory dependency on tonic and I was wondering why, and if this can be made optional. The reason I believe it should be optional is mainly because tonic seems to be a small project with not a lot of traction. It also seems to be a bundle of utils and in most cases, not all dependencies are required.

My biggest concern right now is about a dependency mismatch between Gin and tonic on the validator package. I'm aware that not using tonic would require me to define more Operation information manually, but I'm absolutely fine with it.

I was wondering if you could consider breaking the dependency and making it optional to use tonic on the handler.

Thanks

wI2L commented 3 years ago

Hello @gablemire

gadgeto/tonic generates wrapping gin-compatible handlers that automatically bind request informations to a specified input type. It uses the reflect package to dynamically inspect the input and output types of the handler. fizz relies on that behavior and fetches the informations it needs from tonic for each handler, as shown here: https://github.com/wI2L/fizz/blob/master/fizz.go#L176

fizz could in theory extract the reflect logic that inspect the handler's input/output types and provides its own fizz.Handler function to wrap Gin handlers. However, you would still need to specify struct tags to differentiate query-params/headers/body fields in the input type of your handler.

Could you elaborate about your use-case? Aside from the potential dependencies diff (that could be fixed easily since both code owners are from the same company), do you have others reasons not to use tonic ? The obvious one is about perfs, since tonic makes heavy use on reflect primitive in the got path of each request, whereas fizz only instantiate once when the router is created.

gablemire commented 3 years ago

Hello @wI2L

The main reason I have is for performance, indeed. You should use reflection to extract the primitives on boot and be done with it afterwards in my opinion. But it's more than that, really. I believe this design choice makes it very difficult for somebody who wishes to use fizz after they developed their API to onboard with fizz. Because it relies on tonic, this means one would have to rewrite all their handlers in order to get auto-generation of the OpenAPI.

Without tonic, fizz could still extract a lot of information (routes, parameters, etc) and would require the user to only document the input / output further. I feel this should be an option. If tonic handlers are used, it should be able to extract the information. It means less performance, but more usability. Otherwise, we should be able to manually define the information and fizz should be able to still process the endpoint with the original Gin handler.

What do you think?

wI2L commented 3 years ago

There's still one downside/issue with that approach. The tonic-wrapped handler signature must be func(*gin.Context, [input]) ([output], error). The input and output types are optional. If you were to ignore the tonic wrapping part, that wouldn't make sense to use that handler signature, because you would extract the request informations from the Gin context manually, and thus, doens't need the input/output types that are still required to describe a route in the OpenAPI spec. Adding more functions to fizz to describe all the parameters of a route instead of getting that informations from those types would clutter the router definition.