Skip to content

Commit

Permalink
RFC: Return an instance of *fiber.Error when no handler found (#1847)
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
codemicro committed Apr 5, 2022
1 parent 16b8717 commit e974c67
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 10 deletions.
4 changes: 2 additions & 2 deletions app_test.go
Expand Up @@ -1089,9 +1089,9 @@ func Test_App_Next_Method(t *testing.T) {

app.Use(func(c *Ctx) error {
utils.AssertEqual(t, MethodGet, c.Method())
c.Next()
err := c.Next()
utils.AssertEqual(t, MethodGet, c.Method())
return nil
return err
})

resp, err := app.Test(httptest.NewRequest(MethodGet, "/", nil))
Expand Down
10 changes: 5 additions & 5 deletions middleware/cache/cache_test.go
Expand Up @@ -391,32 +391,32 @@ func Test_Cache_WithHead(t *testing.T) {
func Test_Cache_WithHeadThenGet(t *testing.T) {
app := fiber.New()
app.Use(New())
app.Get("/get", func(c *fiber.Ctx) error {
app.Get("/", func(c *fiber.Ctx) error {
return c.SendString(c.Query("cache"))
})

headResp, err := app.Test(httptest.NewRequest("HEAD", "/head?cache=123", nil))
headResp, err := app.Test(httptest.NewRequest("HEAD", "/?cache=123", nil))
utils.AssertEqual(t, nil, err)
headBody, err := ioutil.ReadAll(headResp.Body)
utils.AssertEqual(t, nil, err)
utils.AssertEqual(t, "", string(headBody))
utils.AssertEqual(t, cacheMiss, headResp.Header.Get("X-Cache"))

headResp, err = app.Test(httptest.NewRequest("HEAD", "/head?cache=123", nil))
headResp, err = app.Test(httptest.NewRequest("HEAD", "/?cache=123", nil))
utils.AssertEqual(t, nil, err)
headBody, err = ioutil.ReadAll(headResp.Body)
utils.AssertEqual(t, nil, err)
utils.AssertEqual(t, "", string(headBody))
utils.AssertEqual(t, cacheHit, headResp.Header.Get("X-Cache"))

getResp, err := app.Test(httptest.NewRequest("GET", "/get?cache=123", nil))
getResp, err := app.Test(httptest.NewRequest("GET", "/?cache=123", nil))
utils.AssertEqual(t, nil, err)
getBody, err := ioutil.ReadAll(getResp.Body)
utils.AssertEqual(t, nil, err)
utils.AssertEqual(t, "123", string(getBody))
utils.AssertEqual(t, cacheMiss, getResp.Header.Get("X-Cache"))

getResp, err = app.Test(httptest.NewRequest("GET", "/get?cache=123", nil))
getResp, err = app.Test(httptest.NewRequest("GET", "/?cache=123", nil))
utils.AssertEqual(t, nil, err)
getBody, err = ioutil.ReadAll(getResp.Body)
utils.AssertEqual(t, nil, err)
Expand Down
2 changes: 1 addition & 1 deletion middleware/logger/logger_test.go
Expand Up @@ -148,7 +148,7 @@ func Test_Logger_All(t *testing.T) {
utils.AssertEqual(t, nil, err)
utils.AssertEqual(t, fiber.StatusNotFound, resp.StatusCode)

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)
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())
}

Expand Down
3 changes: 1 addition & 2 deletions router.go
Expand Up @@ -134,8 +134,7 @@ func (app *App) next(c *Ctx) (match bool, err error) {
}

// If c.Next() does not match, return 404
_ = c.SendStatus(StatusNotFound)
_ = c.SendString("Cannot " + c.method + " " + c.pathOriginal)
err = NewError(StatusNotFound, "Cannot " + c.method + " " + c.pathOriginal)

// If no match, scan stack again if other methods match the request
// Moved from app.handler because middleware may break the route chain
Expand Down

1 comment on commit e974c67

@Fenny
Copy link
Member

@Fenny Fenny commented on e974c67 Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: e974c67 Previous: 220af53 Ratio
Benchmark_Router_Chain 471.9 ns/op 40 B/op 2 allocs/op 202 ns/op 0 B/op 0 allocs/op 2.34
Benchmark_Router_WithCompression 469.4 ns/op 40 B/op 2 allocs/op 200.3 ns/op 0 B/op 0 allocs/op 2.34

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.