samber / slog-fiber

🚨 Fiber middleware for slog logger
https://pkg.go.dev/github.com/samber/slog-fiber
MIT License
53 stars 9 forks source link

Panic when using the 404 middleware #3

Closed varunshoor closed 11 months ago

varunshoor commented 1 year ago

slog-fiber seems to panic when using 404 middleware and sending requests to non existent routes.

func SetupFiber(client *client.Client, logger *slog.Logger) *fiber.App {
    app := fiber.New()

    app.Use(requestid.New())

    app.Use(slogfiber.New(logger))

    app.Use(func(c *fiber.Ctx) error {
        c.Locals("client", client)
        return c.Next()
    })

    http.SetupRoutes(app)

    app.Use(func(c *fiber.Ctx) error {
        return c.Status(fiber.StatusNotFound).SendString("Not Found")
    })

    return app
}

Steps to reproduce:

Output:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18 pc=0x102d8ae48]

goroutine 12 [running]:
github.com/nt/e/cmd/ed/app.SetupFiber.New.NewWithConfig.func4(0x251f779d11e70701?)
        /Users/varunshoor/go/pkg/mod/github.com/samber/slog-fiber@v1.1.0/middleware.go:79 +0x778
github.com/gofiber/fiber/v2.(*Ctx).Next(0x1400002b380?)
        /Users/varunshoor/go/pkg/mod/github.com/gofiber/fiber/v2@v2.49.1/ctx.go:967 +0x48
github.com/nt/e/cmd/ed/app.SetupFiber.New.func3(0x102ecdd20?)
        /Users/varunshoor/go/pkg/mod/github.com/gofiber/fiber/v2@v2.49.1/middleware/requestid/requestid.go:31 +0xdc
github.com/gofiber/fiber/v2.(*App).next(0x1400015c500, 0x14000146300)
        /Users/varunshoor/go/pkg/mod/github.com/gofiber/fiber/v2@v2.49.1/router.go:145 +0x188
github.com/gofiber/fiber/v2.(*App).handler(0x1400015c500, 0x102c4c248?)
        /Users/varunshoor/go/pkg/mod/github.com/gofiber/fiber/v2@v2.49.1/router.go:172 +0x74
github.com/valyala/fasthttp.(*Server).serveConn(0x14000172000, {0x102f54f20?, 0x1400006e250})
        /Users/varunshoor/go/pkg/mod/github.com/valyala/fasthttp@v1.49.0/server.go:2357 +0xdd0
github.com/valyala/fasthttp.(*workerPool).workerFunc(0x14000116640, 0x1400007b6a0)
        /Users/varunshoor/go/pkg/mod/github.com/valyala/fasthttp@v1.49.0/workerpool.go:224 +0x70
github.com/valyala/fasthttp.(*workerPool).getCh.func1()
        /Users/varunshoor/go/pkg/mod/github.com/valyala/fasthttp@v1.49.0/workerpool.go:196 +0x38
created by github.com/valyala/fasthttp.(*workerPool).getCh in goroutine 1
        /Users/varunshoor/go/pkg/mod/github.com/valyala/fasthttp@v1.49.0/workerpool.go:195 +0x208
exit status 2

I am still learning Go so not sure how to fix this

varunshoor commented 1 year ago

I think its because the code seems to reference config.ClientErrorLevel when it should use config.ServerErrorLevel?

        switch {
        case c.Response().StatusCode() >= http.StatusBadRequest && c.Response().StatusCode() < http.StatusInternalServerError:
            logger.LogAttrs(context.Background(), config.ClientErrorLevel, err.Error(), attributes...)
        case c.Response().StatusCode() >= http.StatusInternalServerError:
            logger.LogAttrs(context.Background(), config.ServerErrorLevel, err.Error(), attributes...)
varunshoor commented 1 year ago

Scratch that, it seems to be because of c.Error(), changing it to a string seems to fix it. Whats the suggested next step? I don't want to send a PR when I don't know what I am doing

samber commented 1 year ago

Ok, thanks for reporting this issue.

I just made a fix -> v1.1.1

varunshoor commented 1 year ago

I am sorry for reopening this but there is a regression. The 400 status code panic seems to have been resolved but now 200 status code requests just return "OK". Is it possible that the call to fiber.NewError() when err == nil resets the output chain?

sandrolain commented 1 year ago

Hi, I encountered the same problem, where I define the response status as error type without returning an error value. In this case I think “err” is nil

sandrolain commented 1 year ago

The fix in the latest version causes an error to be generated even if the route handler did not originally create it. This causes the ErrorHandler (whether default or custom) to also handle the request, which otherwise would not happen. I created a fix in PR-5. This should solve the dual management problem. In the case of responses genuinely managed by the ErrorHandler, however, the logs are created before the response has an error status, therefore they appear with a status of 200