RFC: Return an instance of *fiber.Error when no handler found

[codemicro]

What does this change?

This pull request alters the routing behaviour of Fiber to return an instance of *fiber.Error when a handler cannot be found to service a given request. Prior to this commit, Fiber formed a plain-text response on-the-fly that cannot be modified by users of the framework.

This change means that errors resulting from no matching handlers being found would be routed via fiber.App.config.ErrorHandler, just like other errors are within Fiber.

Why might we want this change?

This change allows users to customise the look of their 404 page, for example, to serve a proper HTML page instead of serving plain-text.

Additionally, this would remove the need to add a custom 404 handler to the end of the call stack as documented here, which prevents 405 Method Not Allowed responses from here being correctly served. This is due to the 404 handler matching every request that otherwise exhausts the list of currently registered handlers. By switching to returning an error instead of making an on-the-fly response, we remove the need for the catch-all 404 handler to exist, hence meaning 405 Method Not Allowed responses will be dealt with correctly.

Precedent

Returning *fiber.Error types to form an appropriate error response is already done in the codebase, for example when serving 405 Method Not Allowed responses.

Considerations

1. Could this count as a breaking change? Theorietically, any user using the framework should already have an adequate error handler in place. If they chose to continue using the default error handler, nothing would appear to change - responses generated by this change and by the previous code are identical when using the default error handler.

   It is not impossible to think that this could be a breaking change, as the fiber.App.config.ErrorHandler function is now being called with errors that it was not once called with. However, I believe that this change should not cause an issue as it is assumed that any user of the framework has a functional error handler already impemented, be it the default one or a custom one.

2. This error cannot be checked for using errors.Is. Instead, you'd have to do something like this:

   if e, ok := err.(*fiber.Error); ok && e.Code == fiber.StatusNotFound {
       // ...
   }

---

I'd be interested to hear everyone's opinions and feedback on this proposal.

:)

[codemicro]

It appears that the Test_App_Next_Method test is failing as a result of a poorly written test.

--- FAIL: Test_App_Next_Method (0.00s)
    app_test.go:1099: 
        Test:            Test_App_Next_Method
        Trace:           app_test.go:1099
        Description:     Status code
        Expect:          404     (int)
        Result:          200     (int)

// what's in the test
app.Use(func(c *Ctx) error {
    utils.AssertEqual(t, MethodGet, c.Method())
    c.Next()
    utils.AssertEqual(t, MethodGet, c.Method())
    return
})
// changing the test to this makes the test pass
app.Use(func(c *Ctx) error {
    utils.AssertEqual(t, MethodGet, c.Method())
    err := c.Next()
    utils.AssertEqual(t, MethodGet, c.Method())
    return err
})

---

The Test_Logger_All was failing as it expected there to be an output to contain a - to signify a nil error, when in fact it did not get a nil error, but a Cannot GET /.

--- FAIL: Test_Logger_All (0.00s)
    logger_test.go:152: 
        Test:       Test_Logger_All
        Trace:      logger_test.go:152
        Expect:     3406Host=example.comhttp0.0.0.0example.com/?foo=bar/-                (string)
        Result:     3406Host=example.comhttp0.0.0.0example.com/?foo=bar/Cannot GET /     (string)

// what was in the test:
expected := fmt.Sprintf("%dHost=example.comhttp0.0.0.0example.com/?foo=bar/%s%s%s%s%s%s%s%s%s-", os.Getpid(), cBlack, cRed, cGreen, cYellow, cBlue, cMagenta, cCyan, cWhite, cReset)
utils.AssertEqual(t, expected, buf.String())
// changing to this makes the test pass
expected := fmt.Sprintf("%dHost=example.comhttp0.0.0.0example.com/?foo=bar/%s%s%s%s%s%s%s%s%sCannot GET /", os.Getpid(), cBlack, cRed, cGreen, cYellow, cBlue, cMagenta, cCyan, cWhite, cReset)
utils.AssertEqual(t, expected, buf.String())

[codemicro]

I am unsure as to what to do with the failing Test_Cache_WithHeadThenGet test. Since c.Next() returns an error on a route that's not found, the cache middleware takes this as a reason to quit what it's doing, hence none of the cache headers are being applied to the response, which the test expects.

[codemicro]

As far as I can tell, Test_Cache_WithHeadThenGet is supposed to check that once a HEAD request to a specific endpoint is cached, a GET request to the same endpoint doesn't return the HEAD request's cached content. I've altered it to fit that bill, as before it was making requests to different endpoints, one of which just didn't exist.

If this assumption is incorrect, please let me know.

[codemicro]

@ReneWerner87 If you're happy with this, I'm ok for it to be merged.

[ReneWerner87]

For me it would be okay, just want to ask the other maintainers for their opinion

In the next release it will be then in any case.