-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
chore: update gin-compat example to use engine.OutputOpenAPISpec #337
base: main
Are you sure you want to change the base?
Conversation
ginRouter.GET("/openapi.json", serveOpenApiJSONDescription(engine.OpenAPI)) | ||
ginRouter.GET("/openapi.json", serveOpenAPISpec(engine.OutputOpenAPISpec())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree that's more universal, I don't like the fact that engine.OutputOpenAPISpec()
here creates a file AND returns a slice of bytes.
- Manipulating bytes of JSON instead of a struct is not cool
- There's a big side effect (checking the spec and creating a file) that is a bit hidden.
Don't get me wrong, I like the fact that is has been moved to the Engine
. I'm just not sure the API we're proposing is straightforward for users. What do you think?
Ideas that come to my mind: should add an engine option to allow user to serve their spec from an option we specify on the beginning? Or is it fine like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't get me wrong, I like the fact that is has been moved to the Engine. I'm just not sure the API we're proposing is straightforward for users. What do you think?
I agree
Manipulating bytes of JSON instead of a struct is not cool
We do this in the Server.registerOpenAPIRoutes
already. Should we change ours?
// Registers the routes to serve the OpenAPI spec and Swagger UI.
func (s *Server) registerOpenAPIRoutes(jsonSpec []byte) {
GetStd(s, s.OpenAPIServerConfig.SpecURL, func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write(jsonSpec)
})
gin Data Renderer
// Render (Data) writes data with custom ContentType.
func (r Data) Render(w http.ResponseWriter) (err error) {
r.WriteContentType(w)
_, err = w.Write(r.Data)
return
}
There's a big side effect (checking the spec and creating a file) that is a bit hidden.
I can disable these if it's desired.
Ideas that come to my mind: should add an engine option to allow user to serve their spec from an option we specify on the beginning?
So this has been what I've been striving for/staring at. I'm not quite sure there is a nice way of doing this unless we wrap the gin.Engine and our Engine together in another struct and have it adhere to an interface called like OpenAPIHostable
(terrible name atm, but hopefully it shows my point)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure there is a nice way of doing this unless we wrap the gin.Engine and our Engine together in another struct and have it adhere to an interface called like OpenAPIHostable (terrible name atm, but hopefully it shows my point)
Yeah exactly what I was thinking about. Maybe
type OpenAPIServable interface {
Serve(openapi *fuego.OpenAPI)
}
So the user can dispose of the OpenAPI struct and its own dependencies in his struct.
We would propose a default struct for each engine, but the user would still have the possibility to register its own (to add its middleware etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would propose a default struct for each engine, but the user would still have the possibility to register its own (to add its middleware etc).
Okay. I'm sold. My main concern was dealing with all the different startup varieties for each framework. But if we say the user can just do it themselves if they need more control then that's great.
I'd like to take this on as well. I'm. pretty confident I can get rid of our Server.OpenAPIServerConfig as believe everything can get into the Engine somehow (this is just a bonus ofcourse).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you manage to get rid of Server.OpenAPIServerConfig
I'll be extremely happy.
On another subject, maybe another way is to register the route is with a more traditional approach. Maybe that the user just want to register the route using traditional controller, not by implementing an interface...
I kind of liked the simplicity of
func serveOpenApiJSONDescription(s *fuego.OpenAPI) func(ctx *gin.Context) {
return func(ctx *gin.Context) {
ctx.JSON(http.StatusOK, s.Description())
}
}
Arf, I cannot decide...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we'll see by iterating on it. All propositions have great value, even the ones not implemented :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also really like the OpenAPIServable
as it aligns so heavily with our Server. I will say that's what hooked me on this project. Was I ran Run
and I had my spec as a file., it was served for me. It would also make getting started with using and adaptor even more straightforward IMO.
I'll see what I can do.
No description provided.