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

OpenAPI Collector not traversing child nodes, or parsing their mountpoints #84

Closed
shairozan opened this issue Jun 20, 2022 · 2 comments · Fixed by #86
Closed

OpenAPI Collector not traversing child nodes, or parsing their mountpoints #84

shairozan opened this issue Jun 20, 2022 · 2 comments · Fixed by #86

Comments

@shairozan
Copy link

I'm using this framework in some projects at work and really love it, but the rough edge I've run up could be me missing something.

In the go-chi world, it's common to have different routers mounted to parent routers. This is a way of separating out / differentiating the middlewares as part of the radix tree. Thus your middlewares can be applied closer to your domain objects in a way that makes sense.

With this library, I've struggled with this, specifically because of the documentation side. If I create a default router:

baseRouter := web.DefaultService()

But then try to add a sub router to it (for endpoints that require authentication via JWT for example)

baseRouter := web.DefaultService()
authenticated := web.DefaultService()

baseRouter.Mount("/authenticated",authenticated)

Will work but without documentation. If I force the sub router to have the same collector as the parent during construction the docs are present but missing the /authenticated prefix from the mount.

I wrote Usecase as a workaround by basically allowing this kind of setup as the UseCase implementation layer as well to allow for HTTP level globals and the further down the stack to a more implementation specific one.

Is there something I'm just missing about how the collector is supposed to be traversing nodes to generate the docs?

@vearutop
Copy link
Member

Hi, thank you for raising this concern. Mount indeed works counter intuitively.

I've experimented a bit with introspection and wrapping of handlers in #86, but I'm not sure if this is a good change.

The problem is that mounted handler/router has made impact on docs collection and handlers setup. Merging the product into another docs/router seems like a less reliable process compared to linear flow when a handler is added into its final place initially.

(For example this prototype implementation would apply security documentation to POST /sum2 and POST /mul2 if they were added with service.Post(...) because security middleware of sub declares openapi annotation for these routes.)

Maybe this implementation will be good enough for the edge case, given it only opens a can of worms for Mount users and would not affect other cases.

As a workaround I can suggest using Route instead:

	// Endpoints with admin access.
	s.Route("/admin", func(r chi.Router) {
		r.Group(func(r chi.Router) {
			r.Use(nethttp.AnnotateOpenAPI(s.OpenAPICollector, func(op *openapi3.Operation) error {
				op.Tags = []string{"Admin Mode"}

				return nil
			}))
			r.Use(adminAuth, nethttp.HTTPBasicSecurityMiddleware(s.OpenAPICollector, "Admin", "Admin access"))
			r.Method(http.MethodPut, "/tasks/{id}", nethttp.NewHandler(usecase.UpdateTask(locator), ff))
		})
	})

	// Endpoints with user access.
	s.Route("/user", func(r chi.Router) {
		r.Group(func(r chi.Router) {
			r.Use(userAuth, nethttp.HTTPBasicSecurityMiddleware(s.OpenAPICollector, "User", "User access"))
			r.Method(http.MethodPost, "/tasks", nethttp.NewHandler(usecase.CreateTask(locator),
				nethttp.SuccessStatus(http.StatusCreated)))
		})
	})

https://github.com/swaggest/rest/blob/v0.2.29/_examples/task-api/internal/infra/nethttp/router.go#L54-L74

@shairozan
Copy link
Author

I think mount may be an ugly route to go. The Route workaround works for me where the Method wrapper takes a handler. It's easy enough to provide a Handler method to the Usecase on top of Interactor so that nothing changes about how the core business logic is written. I'd say I'm good with the workaround with Route since the docs are working. Wouldn't worry about messing the code base up too much :)

Thanks much for the help!

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 a pull request may close this issue.

2 participants