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

WIP: OpenAPIServable #344

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

WIP: OpenAPIServable #344

wants to merge 3 commits into from

Conversation

dylanhitt
Copy link
Collaborator

@dylanhitt dylanhitt commented Jan 11, 2025

Heavy WIP

Working through how this would work.

Just wanted to get a initial outline on how we could do this. Feel free to discuss over top of this. I imagine we reach the point with our adaptors where we have something like fuegogin.Adaptor.Run(), in this case it would engine.OutputOpenAPISpec, engine.RegisterOpenAPIRoutes, and then start the gin server. Again everything is exposed and we can provide options maybe, but users can plug and play as they please.

PS. Feel free to open PR against this or contribute directly 😄

openapi.go Outdated Show resolved Hide resolved
openapi.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
extra/fuegogin/adaptor.go Outdated Show resolved Hide resolved
@dylanhitt dylanhitt changed the title WIP: [skip ci] WIP: OpenAPIServable Jan 11, 2025
@dylanhitt dylanhitt requested review from EwenQuim, ccoVeille and rizerkrof and removed request for EwenQuim and ccoVeille January 14, 2025 03:31
@dylanhitt dylanhitt marked this pull request as ready for review January 14, 2025 03:34
@dylanhitt
Copy link
Collaborator Author

dylanhitt commented Jan 14, 2025

This is ready to review.

I did elect to wrap our engine in the gin Adaptor struct but it's not necessary as you can see. It just felt nicer to me (this piece isn't necessarily done mainly just would like some feedback). We could change route constructors as well it would align with the way our Server looks

func GetGin(a *Adaptor, path string, handler gin.HandlerFunc, options ...func(*fuego.BaseRoute)) *fuego.Route[any, any] {
	return handleGin(a, http.MethodGet, path, handler, options...)
}

We could leave the Engine out but the OpernAPIServable interface turns into this.

type OpenAPIServable interface {
	SpecHandler(e *Engine)
	UIHandler(e *Engine)
}

We could then make RegisterOpenAPIRoutes a method. A little example of what that fully looks like is here: #344 (comment). As I said I did prefer wrapping Engine up in the Adaptor.

In order to really go much further in unifying our server with the adaptors is we're gonna have to get closer to actually running the Server start, and maybe it's not even worth it. Really the only thing we don't provide at this point is OutputOpenAPISpec. Which the user can easily call themselves.


I plan to add comments/test after we decide if this looks goods/is worthwhile.

@dylanhitt dylanhitt force-pushed the OpenAPIServable branch 3 times, most recently from 253ac48 to b6d9ea6 Compare January 14, 2025 04:12
Copy link
Member

@EwenQuim EwenQuim left a comment

Choose a reason for hiding this comment

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

Honestly the best PR of the month. Made my day :)

No big comment on the structure of your PR, it was really nice to see all this!

I'm mostly discussing about the API we will show to our users. This might seem superficial but it is essential when designing a framework, this will impact user adoption a lot.

Feel free to challenge my propositions :)

openapi.go Outdated Show resolved Hide resolved
server.go Show resolved Hide resolved
openapi_handler_test.go Outdated Show resolved Hide resolved
openapi_handler_test.go Outdated Show resolved Hide resolved
openapi.go Outdated Show resolved Hide resolved
engine.go Show resolved Hide resolved
extra/fuegogin/adaptor.go Outdated Show resolved Hide resolved
examples/gin-compat/main.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
@EwenQuim EwenQuim added this to the v0.18 milestone Jan 21, 2025
@dylanhitt
Copy link
Collaborator Author

@EwenQuim @ccoVeille thank you all for the review.

I'll try to get this update with the suggestions before then end of the week. If you need this done sooner please feel free to add additions or open another PR against this.

I am low on time for next week or two.

@dylanhitt
Copy link
Collaborator Author

Oh and please let me know if you have other thoughts. This PR was tough as I believe approaching a more of a decision on how much do we want to fuego to responsible for. To me the real spirit of the PR is just providing some components for serving OpenAPI Routes that's easy (essentially these integrations with other frameworks should provide routes to serve these resources to me it's one of the flagships of the product). The 2nd thing is getting all the config for OpenAPIConfig into Engine. Having to split between Server and Engine feels just wrong.

@dylanhitt dylanhitt force-pushed the OpenAPIServable branch 2 times, most recently from 2d456ed to cbd0157 Compare January 24, 2025 17:24
OpenAPIConfig

[skip ci] fuegogin.NewAdaptor

WIP
dylanhitt and others added 2 commits January 24, 2025 12:33
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
@dylanhitt
Copy link
Collaborator Author

This is ready for another look btw 😄

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.

3 participants