gofiber / fiber

⚡️ Express inspired web framework written in Go
https://gofiber.io
MIT License
33.9k stars 1.67k forks source link

🐛 [Bug]: UserContext no more accessible when adapting http.Handler to fiber.Handler #2711

Open tmarwen opened 1 year ago

tmarwen commented 1 year ago

Bug Description

When using the adaptor package to adapt an existing http.Handler to fiber.Handler, the fiber.Ctx#UserContext is no more accessible and all values already stored within are no more accessible as well.

Here down an example illustrating the issue:

package main

import (
    "context"
    "fmt"
    "net/http"

    "github.com/gofiber/fiber/v2"
    "github.com/gofiber/fiber/v2/middleware/adaptor"
)

func main() {
    userNameCtxKey := struct{}{}

    app := fiber.New()

    // Middleware enriching the UserContext
    app.Use(func(c *fiber.Ctx) error {
        userCtx := context.WithValue(c.UserContext(), userNameCtxKey, "go-fiber")
        c.SetUserContext(userCtx)
        return c.Next()
    })

    helloHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "text/plain; charset=utf-8")
        w.WriteHeader(http.StatusOK)
        fmt.Fprintf(w, "Hello %s", r.Context().Value(userNameCtxKey))
    })

    helloFiberHandler := adaptor.HTTPHandler(helloHandler)

    app.Get("/hello", helloFiberHandler)

    helloInternalHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "text/plain; charset=utf-8")
        w.WriteHeader(http.StatusOK)
        userCtx := r.Context().Value("__local_user_context__").(context.Context)
        fmt.Fprintf(w, "Hello %s", userCtx.Value(userNameCtxKey))
    })

    helloInternalFiberHandler := adaptor.HTTPHandler(helloInternalHandler)

    app.Get("/hello", helloFiberHandler)
    app.Get("/hello-internal", helloInternalFiberHandler)

    app.Listen(":8080")
}

As illustrated above, the context value with key userNameCtxKey initially stored in c.UserContext is not accessible through the raw http.Handler http.Request#Context since the latter use *fasthttp.RequestCtx as its Context value which still holds the user request context but hidded under the __local_user_context__ internal key as demonstrated within the second handler adaptor.

The above is for sure a simple illustration of the issue but the implications of this in real world scenarios are indeed impacting as context.Context is the main medium for cross-boundaries values passing. One of the main use cases where this issue surfaced where propagation of incoming OpenTelemetry trace span when using https://github.com/gofiber/contrib/blob/main/otelfiber/:

  1. The incoming trace is properly extracted from header then a new span is created and its context is injected as the UserContext:
spanName := utils.CopyString(c.Path())
        ctx, span := tracer.Start(ctx, spanName, opts...)
        defer span.End()

        // pass the span through userContext
        c.SetUserContext(ctx)
  1. gqlgen HTTP handler is being used for GQL requests processing:
    graphHandler := adaptor.HTTPHandler(gqlServer)
    fiberApp.Post("/gqlapi", graphHandler)
  2. Parent span is not accessible in operation interceptor (and any other function using context.Context as argument)
    
    type tracingOperationInterceptor struct {
    }

func (i *tracingInterceptor) InterceptOperation(ctx context.Context, next graphql.OperationHandler) graphql.ResponseHandler { span := trace.SpanFormContext(ctx) // Span is not properly resolved }


### How to Reproduce

Steps to reproduce the behavior: Run below sample then `cURL` `http://localhost:8080/hello` the `http://localhost:8080/hello-internal`:

```go
package main

import (
    "context"
    "fmt"
    "net/http"

    "github.com/gofiber/fiber/v2"
    "github.com/gofiber/fiber/v2/middleware/adaptor"
)

func main() {
    userNameCtxKey := struct{}{}

    app := fiber.New()

    // Middleware enriching the UserContext
    app.Use(func(c *fiber.Ctx) error {
        userCtx := context.WithValue(c.UserContext(), userNameCtxKey, "go-fiber")
        c.SetUserContext(userCtx)
        return c.Next()
    })

    helloHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "text/plain; charset=utf-8")
        w.WriteHeader(http.StatusOK)
        fmt.Fprintf(w, "Hello %s", r.Context().Value(userNameCtxKey))
    })

    helloFiberHandler := adaptor.HTTPHandler(helloHandler)

    app.Get("/hello", helloFiberHandler)

    helloInternalHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "text/plain; charset=utf-8")
        w.WriteHeader(http.StatusOK)
        userCtx := r.Context().Value("__local_user_context__").(context.Context)
        fmt.Fprintf(w, "Hello %s", userCtx.Value(userNameCtxKey))
    })

    helloInternalFiberHandler := adaptor.HTTPHandler(helloInternalHandler)

    app.Get("/hello", helloFiberHandler)
    app.Get("/hello-internal", helloInternalFiberHandler)

    app.Listen(":8080")
}

Expected Behavior

Fiber should either:

  1. Expose the #UserContext key, being __local_user_context__, publicly so it can be resolved from within any function as long as the incoming c.Context() is accessible
  2. Provide a safe mechanism to copy #UserContext into *fiber.Ctx#Context prior to adapting an http.Handler

Fiber Version

v2.50.0

Code Snippet (optional)

package main

import (
    "context"
    "fmt"
    "net/http"

    "github.com/gofiber/fiber/v2"
    "github.com/gofiber/fiber/v2/middleware/adaptor"
)

func main() {
    userNameCtxKey := struct{}{}

    app := fiber.New()

    // Middleware enriching the UserContext
    app.Use(func(c *fiber.Ctx) error {
        userCtx := context.WithValue(c.UserContext(), userNameCtxKey, "go-fiber")
        c.SetUserContext(userCtx)
        return c.Next()
    })

    helloHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "text/plain; charset=utf-8")
        w.WriteHeader(http.StatusOK)
        fmt.Fprintf(w, "Hello %s", r.Context().Value(userNameCtxKey))
    })

    helloFiberHandler := adaptor.HTTPHandler(helloHandler)

    app.Get("/hello", helloFiberHandler)

    helloInternalHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "text/plain; charset=utf-8")
        w.WriteHeader(http.StatusOK)
        userCtx := r.Context().Value("__local_user_context__").(context.Context)
        fmt.Fprintf(w, "Hello %s", userCtx.Value(userNameCtxKey))
    })

    helloInternalFiberHandler := adaptor.HTTPHandler(helloInternalHandler)

    app.Get("/hello", helloFiberHandler)
    app.Get("/hello-internal", helloInternalFiberHandler)

    app.Listen(":8080")
}

Checklist:

welcome[bot] commented 1 year ago

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

tmarwen commented 1 year ago

@efectn I can provide a PR for this if you can suggest on the preferred approach here.

sergiopastan commented 10 months ago

maybe you have already solved this, but a workaround for your use case (span propagation in gqlgen) could be to wrap the gqlServer in a function like this:

fn := func(w http.ResponseWriter, r *http.Request) {
    userContext := r.Context().Value("__local_user_context__").(context.Context)
    gqlServer.ServeHTTP(w, r.WithContext(userContext))
}
graphHandler := adaptor.HTTPHandlerFunc(fn)
fiberApp.Post("/gqlapi", graphHandler)

but you are right, you have to use the string "__local_user_context__" as key since the UserContext key is not exposed

JonasDoe commented 9 months ago

Hey, what's the status with this issue? I'ld also happily provide a PR, but since @efectn didn't even respond yet, I'm not sure it would even get merged.

efectn commented 9 months ago

Sorry i didn't see it. You can create a PR @JonasDoe @tmarwen

JonasDoe commented 9 months ago

Alright, II underestimated the work for that. The only approach I see is something like

func(ctx *fiber.Ctx) error {
    convertedRequest, err := adaptor.ConvertRequest(ctx, true) // TODO handle err
    // this could happen in adaptor.ConvertRequest either
    ctx.Context().VisitUserValuesAll(func(k interface{}, v interface{}) {
        convertedRequest = convertedRequest.WithContext(context.WithValue(convertedRequest.Context(), ... add all values under v, one after another))
    })

    var response http.ResponseWriter = ... // a custom ResponseWriter implementation for adaptor
    gqlHandler.ServeHTTP(response, convertedRequest)
     // This could be done by the custom ResponseWriter implementation
    ctx.Set(response.SomeStuff)
    ctx.Response().SetBodyRaw(response.SomeOtherStuff)
    ...

    return nil
})...)

Sadly, I don't really have the time to write a new ResponseWriter that allows to pipe all data to the Ctx.fasthttp.Response b/c I figure there are more pitfalls here than I could imagine. Maybe when I've more time to spare, but I don't see that happen soon. But if somebody else has a better idea or more experience - feel free! :)

goyal-aman commented 7 months ago

@JonasDoe / @efectn i'm interested in working on this one, but I have no previous experience on how to work on open projects.

JonasDoe commented 7 months ago

@goyal-aman Sounds great! Normally you fork the project, apply your changes and then create a PR from your forks by clicking on that link in the "New PR" menu: image Idk if that helps. :D If there something else I can do to support you, I'll gladly do that. That said, for myself I've decided to drop fiber/fasthttp in my gqlgen-based project b/c I figure the adaptor mitigates all performance gains from fasthttp anyway. But it might still be useful in some cases, e.g. if you want to use a fiber-based utility library.

goyal-aman commented 7 months ago

@JonasDoe - I feel lost. All I understand right now, before handler(c.Context()) is called, something has to be done so values in __local_user_context__ are available directly in c.Context() is that correct?

func HTTPHandler(h http.Handler) fiber.Handler {
    return func(c *fiber.Ctx) error {

        handler := fasthttpadaptor.NewFastHTTPHandler(h)
                // do something here with context?
        handler(c.Context())
        return nil
    }
}
JonasDoe commented 7 months ago

@goyal-aman I think you're setting up the handler the wrong way - it's fiber.Handler to http.Handler, not the other way around. We have a fiber app running, but need to pass the request to a http.Handler with the help of the adaptor and this new feature.

My idea was that we have to create a new context.Context based on the old one, plus all the values from "__local_user_context__", as I sketched above. The updated context will be stored the convertedRequest in my sketch, and this convertedRequest with the updated context will be handled afterward in the http.Handler implementation of gqlgen. I've also replied to a similar question at stackoverflow - maybe this helps to clarify it a bit more thanks to the screenshot there: https://stackoverflow.com/a/77964925/5767484.