-
-
Notifications
You must be signed in to change notification settings - Fork 234
Jwt authorization middleware #132
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
Conversation
|
@danielgtaylor Hi, theoretically, what could this be merged if documentation will be added? Or architecturally, do you want to change something or perhaps generally disagree with this new feature? |
|
@Insei hi, thanks for pinging about this and sorry for the delayed response. I think the generic middleware concept is good and am in favor of adding it so long as it remains optional and you can continue to use router-specific middleware if you want/need to. The one hesitation I have is your proposed interface for it, where each middleware is called one after the other without any ability to modify the context. This makes e.g. compression middlewares which require replacing the reader/writer impossible at the moment. I wanted to put together an alternative for you but haven't had the time, so I'll just type out some thoughts here. I think each middleware should be chained like you see with other Go routers that support middleware (or stuff like Express in Node land). E.g. each middleware could be a type CompressionContext struct {
huma.Context
}
func NewCompressionContext(ctx huma.Context) *CompressionContext {
return &CompressionContext{ctx}
}
func (c *CompressionContext) BodyWriter() io.Writer {
return gzip.NewWriter(c.Context.BodyWriter())
}
func CompressionMiddleware(ctx huma.Context, next func(huma.Context)) {
newCtx := NewCompressionContext(ctx)
next(newCtx)
}I also don't think the API argument should be necessary, as this can be handled at middleware instantiation time, for example: type MyMiddleware struct {
api huma.API
}
func NewMyMiddleware(api huma.API) *MyMiddleware {
return &MyMiddleware{api}
}
func (m *MyMiddleware) Handle(ctx huma.Context, next func(huma.Context)) {
// I don't do anything
next(ctx)
}
// Usage example
api := // ...
m := NewMyMiddleware(api)
huma.UseMiddleware(api, m.Handle)Also on that note, why not register middleware with a specific API like Anyway hopefully this helps, let me know what you think and I'll try to come up with a better example of what I mean when I get a chance. For now you can probably look at a few other Go implementations of middleware to see how they code it. |
|
@danielgtaylor Thanks for response!
You're definitely right, and that's what I'll do. I just wanted to ask in this PR if there are any contradictions in implementing middleware specifically for HUMA.
Theoretically, it is possible to use several instances of the huma.API at the same time, but with diff. set of middlewares; of course, the probability of this is not great, but it does exist. But if we will have api.UseMiddleware, this problem will be resolved.
I was just watching how the Register method was made :) |
Signed-off-by: Dmitrii Aleksandrov <goodmobiledevices@gmail.com>
Signed-off-by: Dmitrii Aleksandrov <goodmobiledevices@gmail.com>
| case "header": | ||
| value = ctx.Header(p.Name) | ||
| } | ||
| api.Middlewares().Handler(HandlerFunc(func(_ API, ctx Context) { |
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.
In handler func, I only add this string, and one more at the end, nothing more :)
| } else { | ||
| ctx.SetStatus(status) | ||
| } | ||
| })).Handle(api, ctx) |
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.
This string was changed, nothing more :)
|
@danielgtaylor I made middleware the same way as in chi =) and replace UseMiddleware to API interface. |
Hello, I decided to work a little with huma. Having seen the high level of abstractions in the request processing chain, I thought that it would not be bad to give the opportunity to embed midlleware directly into request processing on the huma side, because huma gives ability to make universal midllewares that is not depends to a specific adapter implementation. And at same time in middleware, we can access to HumaOperation with OpenApi specs and Context with request data.
This is just an example, which I will of course use anyway, but I would like such an opportunity to be available in huma itself.
Middleware Flow:
I will add tests and descriptions to the readme if it is decided that this PR can be merged into main.