Skip to content
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

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

Merged
merged 4 commits into from Apr 5, 2022
Merged

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

merged 4 commits into from Apr 5, 2022

Conversation

codemicro
Copy link
Contributor

@codemicro codemicro commented Apr 3, 2022

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.

:)

When a handler cannot be found for a given path, previously Fiber
would construct a plaintext response that cannot be modified.

This commit switches to returning a new instance of `*fiber.Error`
with identical error message so that users can customise the look
of their 404 pages.

Signed-off-by: AKP <tom@tdpain.net>
@codemicro
Copy link
Contributor Author

codemicro commented Apr 3, 2022

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 added 2 commits Apr 3, 2022
This test was failing as the error returned by `c.Next()` that's
required to generate the correct 404 status code was not being
passed through the middleware and being silently ignored.

Signed-off-by: AKP <tom@tdpain.net>
Signed-off-by: AKP <tom@tdpain.net>
@codemicro
Copy link
Contributor Author

codemicro commented Apr 3, 2022

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.

As far as I can tell, this test is meant to check that a cached
HEAD request to a given endpoint does not return the cached
content to a GET request to the same endpoint, and the test has
been altered to correctly check for this.

Signed-off-by: AKP <tom@tdpain.net>
@codemicro
Copy link
Contributor Author

codemicro commented Apr 4, 2022

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
Copy link
Contributor Author

codemicro commented Apr 4, 2022

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

@ReneWerner87
Copy link
Member

ReneWerner87 commented Apr 4, 2022

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.

@ReneWerner87 ReneWerner87 merged commit e974c67 into gofiber:master Apr 5, 2022
16 checks passed
ReneWerner87 added a commit to gofiber/docs that referenced this issue Apr 15, 2022
trim21 pushed a commit to trim21/fiber that referenced this issue Aug 15, 2022
…ber#1847)

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

When a handler cannot be found for a given path, previously Fiber
would construct a plaintext response that cannot be modified.

This commit switches to returning a new instance of `*fiber.Error`
with identical error message so that users can customise the look
of their 404 pages.

Signed-off-by: AKP <tom@tdpain.net>

* Fix `Test_App_Next_Method`

This test was failing as the error returned by `c.Next()` that's
required to generate the correct 404 status code was not being
passed through the middleware and being silently ignored.

Signed-off-by: AKP <tom@tdpain.net>

* Fix `Test_Logger_All`

Signed-off-by: AKP <tom@tdpain.net>

* Fix `Test_Cache_WithHeadThenGet` test

As far as I can tell, this test is meant to check that a cached
HEAD request to a given endpoint does not return the cached
content to a GET request to the same endpoint, and the test has
been altered to correctly check for this.

Signed-off-by: AKP <tom@tdpain.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants