-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
feat: enable WrappedContext for router specifics access points in Adapters #699
base: main
Are you sure you want to change the base?
Conversation
@@ -61,6 +61,10 @@ func (c *fiberCtx) Matched() string { | |||
return c.orig.Route().Path | |||
} | |||
|
|||
func (c *fiberCtx) WrappedContext() interface{} { |
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.
For fiber this is unsafe.
Please take a look to this comment - #686 (comment)
and this PR - #693
/*
* Web framework "fiber" https://gofiber.io/ uses high-performance zero-allocation "fasthttp" server https://github.com/valyala/fasthttp
*
* The underlying fasthttp server prohibits to use or refer to `*fasthttp.RequestCtx` outside handler
* The quote from documentation to fasthttp https://github.com/valyala/fasthttp/blob/master/README.md
*
* > VERY IMPORTANT! Fasthttp disallows holding references to RequestCtx or to its' members after returning from RequestHandler. Otherwise data races are inevitable. Carefully inspect all the net/http request handlers converted to fasthttp whether they retain references to RequestCtx or to its' members after returning
*
* As the result "fiber" prohibits to use or refer to `*fiber.Ctx` outside handler
* The quote from documentation to fiber https://docs.gofiber.io/#zero-allocation
*
* > Because fiber is optimized for high-performance, values returned from fiber.Ctx are not immutable by default and will be re-used across requests. As a rule of thumb, you must only use context values within the handler, and you must not keep any references. As soon as you return from the handler, any values you have obtained from the context will be re-used in future requests and will change below your feet
*
* To deal with these limitations, the contributor of to this adapter @excavador (Oleg Tsarev, email: [email protected], telegram: @oleg_tsarev) is clear variable explicitly in the end of huma.Adapter methods Handle and ServeHTTP
*
* You must NOT use member `unsafeFiberCtx` directly in adapter, but instead use `orig()` private method
*/
unsafeFiberCtx *fiber.Ctx
unsafeGolangCtx context.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.
Would using orig()
here help? @genslein do you need Fiber support?
@genslein I wonder if it makes sense for your use-case to just copy the adapter code and provide whatever strongly typed additional methods you need. Adapters aren't much code, don't change much, and then you can add any extra utilities your want and migrate on your own schedule without needing anything to be updated in the Huma core. Mainly I'm pushing back on expanding the |
One of the possible options:
@danielgtaylor for instance, in case of fiber interface WrappedContext interface {
WrappedContext() *fiber.Ctx
}
func (c *fiberCtx) WrappedContext() *fiber.Ctx {
return c.orig()
} Usage (types for clarity) func Handler(c huma.Context) error {
var wcg humafiber.WrapperContext
var ok bool
if wcg, ok = c.(humafiber.WrappedContext); ok {
var fc *fiber.Ctx = wcg.WrappedContext()
...
}
} |
@danielgtaylor also alternative interface WrappedContext[T any] interface {
WrappedContext() T
}
func (c *fiberCtx) WrappedContext() *fiber.Ctx {
return c.orig()
} func Handler(c huma.Context) error {
var wcg huma.WrapperContext[*fiber.Ctx]
var ok bool
if wcg, ok = c.(huma.WrapperContext[*fiber.Ctx]); ok {
var fc *fiber.Ctx = wcg.WrappedContext()
...
}
} |
@excavador yes that example could work. I still am not sure we need this in the core Huma code since it's easy to copy and modify an adapter as needed to give you access to anything else you need for your specific migration project. The adapters are designed to be lightweight and easy to copy/modify, and they change rarely so getting out of sync is not a major concern. |
Hey all, sorry for the delayed response. That makes sense for the adapter code, I'll see if I can unwind the base |
Hey all,
During my time trying to transition my
Bring-your-own-Router
work, many of my handlers are specific to the frameworks we use in each project. For example,func MyHandler(c echo.Context) error
.This would normally not be an issue, but the transition for the number of endpoints we have is difficult enough as is. Rather than doing them all up front, I'd rather like to have access to the underlying framework to reuse the existing code.
I'm not tied to the naming but this was easier than rewriting all of my handlers. Please comment and let me know what I can do to help out here.