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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

chore: update gin-compat example to use engine.OutputOpenAPISpec #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions examples/gin-compat/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ func fuegoControllerPost(c fuego.ContextWithBody[HelloRequest]) (*HelloResponse,
}, nil
}

func serveOpenApiJSONDescription(s *fuego.OpenAPI) func(ctx *gin.Context) {
func serveOpenAPISpec(openAPISpec []byte) func(ctx *gin.Context) {
return func(ctx *gin.Context) {
ctx.JSON(http.StatusOK, s.Description())
ctx.Data(http.StatusOK, "application/json", openAPISpec)
}
}

func DefaultOpenAPIHandler(specURL string) gin.HandlerFunc {
return func(ctx *gin.Context) {
ctx.Header("Content-Type", "text/html; charset=utf-8")
ctx.String(200, fuego.DefaultOpenAPIHTML(specURL))
ctx.String(http.StatusOK, fuego.DefaultOpenAPIHTML(specURL))
}
}
2 changes: 1 addition & 1 deletion examples/gin-compat/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func server() (*gin.Engine, *fuego.OpenAPI) {
)

// Serve the OpenAPI spec
ginRouter.GET("/openapi.json", serveOpenApiJSONDescription(engine.OpenAPI))
ginRouter.GET("/openapi.json", serveOpenAPISpec(engine.OutputOpenAPISpec()))
Copy link
Member

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?

Copy link
Collaborator Author

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)

Copy link
Member

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).

Copy link
Collaborator Author

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).

Copy link
Member

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...

Copy link
Member

@EwenQuim EwenQuim Jan 8, 2025 •
edited
Loading

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 :)

Copy link
Collaborator Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow up issue:

ginRouter.GET("/swagger", DefaultOpenAPIHandler("/openapi.json"))

return ginRouter, engine.OpenAPI
Expand Down
Loading