🐛 bug: V3 fix custom errorhandler invokation #3930
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughErrorHandler selection now iterates deterministically over mounted sub-app keys and compares trailing-slash–normalized request paths and prefixes to choose the deepest matching sub-app's ErrorHandler; a new test verifies correct handler selection across nested mounts and fallbacks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @duhnnie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request ports a critical fix from V2 to V3, addressing an issue where custom error handlers for mounted applications were not always invoked correctly. The changes refine the logic within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ports a fix from V2 to V3, addressing an issue with custom error handler invocation for mounted sub-applications. The changes ensure the correct error handler is selected by normalizing path and prefix strings with a trailing slash and by iterating over mount prefixes in a deterministic, sorted order. A comprehensive test case has been added to verify the fix across various nested mounting scenarios. The implementation appears correct and the new test provides good coverage for the addressed issue.
app_test.go
Outdated
| // /api/v1/users mount has a custom ErrorHandler | ||
| {"/api/v1/use", "/api/v1/use error handler"}, |
There was a problem hiding this comment.
The comment on line 2807 is misleading. It refers to /api/v1/users, but the test case on the following line is for /api/v1/use. Additionally, in this test setup, the /api/v1/users mount does not have a custom error handler, whereas /api/v1/use does. To avoid confusion for future readers, the comment should be updated to accurately describe the test case.
| // /api/v1/users mount has a custom ErrorHandler | |
| {"/api/v1/use", "/api/v1/use error handler"}, | |
| // /api/v1/use mount has a custom ErrorHandler | |
| {"_api/v1/use", "/api/v1/use error handler"}, |
There was a problem hiding this comment.
I applied the suggested comment only, next suggestion is not necessary.
app_test.go
Outdated
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| testCase := testCase // Explicitly capture the current value of `testCase` |
There was a problem hiding this comment.
@gaby @ReneWerner87 This line wasn't included in test for V2, I realized about it at making the port for V3. Should I add it to V2? (even though the test run without failures).
There was a problem hiding this comment.
Nevermind about this, the linter already made me remove it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app.go (1)
1298-1320: Make deepest-prefix selection independent of mount iteration order.The new normalization logic for matching mounted sub‑apps looks good, but the prefix‑depth tracking currently updates
mountedPrefixPartseven for mounts that don’t have a customErrorHandler:if prefix != "" && strings.HasPrefix(normalizedPath, normalizedPrefix) { parts := strings.Count(prefix, "/") + 1 if mountedPrefixParts <= parts { if subApp.configured.ErrorHandler != nil { mountedErrHandler = subApp.config.ErrorHandler } mountedPrefixParts = parts } }This makes the final choice sensitive to the order of
appListKeys: if a deeper mount without a custom handler is processed before its shallower parent with a handler,mountedPrefixPartscan be bumped past the parent’s depth and the parent’s handler will never be considered, even though it should be the nearest applicable handler.To decouple correctness from iteration order and only rank among actual candidates, you can gate the depth update on having a configured handler:
- for _, prefix := range app.mountFields.appListKeys { - subApp := app.mountFields.appList[prefix] - normalizedPrefix := utils.AddTrailingSlashString(prefix) - - if prefix != "" && strings.HasPrefix(normalizedPath, normalizedPrefix) { - // Count slashes instead of splitting - more efficient - parts := strings.Count(prefix, "/") + 1 - if mountedPrefixParts <= parts { - if subApp.configured.ErrorHandler != nil { - mountedErrHandler = subApp.config.ErrorHandler - } - - mountedPrefixParts = parts - } - } - } + for _, prefix := range app.mountFields.appListKeys { + if prefix == "" { + continue + } + + subApp := app.mountFields.appList[prefix] + if subApp.configured.ErrorHandler == nil { + continue // no custom handler → not a candidate + } + + normalizedPrefix := utils.AddTrailingSlashString(prefix) + if strings.HasPrefix(normalizedPath, normalizedPrefix) { + // Count slashes instead of splitting - more efficient + parts := strings.Count(prefix, "/") + 1 + if mountedPrefixParts <= parts { + mountedErrHandler = subApp.config.ErrorHandler + mountedPrefixParts = parts + } + } + }This keeps the “deepest matching prefix wins” behavior while ensuring it only depends on the set of mounts with explicit
ErrorHandlers, not on howappListKeysis ordered internally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app.go(1 hunks)app_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
app_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
app_test.goapp.go
🧠 Learnings (2)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
app_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
app_test.go
🧬 Code graph analysis (1)
app_test.go (1)
app.go (3)
New(522-635)Config(115-426)ErrorHandler(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit (1.25.x, ubuntu-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: lint
duhnnie
force-pushed
the
v3-fix-custom-errorhandler-invokation
branch
2 times, most recently
from
96b3aa1 to
a647a8f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3930 +/- ##
==========================================
- Coverage 91.65% 91.63% -0.02%
==========================================
Files 119 119
Lines 10152 10155 +3
==========================================
+ Hits 9305 9306 +1
- Misses 537 538 +1
- Partials 310 311 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app_test.go (3)
2819-2819: Remove redundant loop variable capture (Go 1.22+).In Go 1.22 and later, loop variables are automatically per-iteration, so the explicit capture
testCase := testCaseis no longer necessary.Apply this diff:
for _, testCase := range testCases { - testCase := testCase // Explicitly capture the current value of `testCase` - t.Run(testCase.path, func(t *testing.T) {
2823-2823: Preferhttp.NoBodyovernilfor request body.Use
http.NoBodyinstead ofnilto clearly indicate an empty request body, consistent with Go conventions.Apply this diff:
- resp, err := app.Test(httptest.NewRequest(MethodGet, testCase.path, nil)) + resp, err := app.Test(httptest.NewRequest(MethodGet, testCase.path, http.NoBody))
2824-2831: Userequire.NoErrorinstead oft.Fatalfor consistency.The test file uses
testify/requirefor assertions (e.g., line 2833). For consistency with project conventions, userequire.NoErrorinstead of manually checking errors witht.Fatal.Apply this diff:
resp, err := app.Test(httptest.NewRequest(MethodGet, testCase.path, http.NoBody)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) body, err := io.ReadAll(resp.Body) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err)Based on learnings, the Fiber project prefers
requiremethods from testify for test assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
app_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
app_test.go
🧠 Learnings (9)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
app_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
app_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
app_test.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
app_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
app_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
app_test.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
app_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
app_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
app_test.go
🪛 GitHub Actions: golangci-lint
app_test.go
[error] 2819-2819: golangci-lint: The copy of the 'for' variable "testCase" can be deleted (Go 1.22+) (copyloopvar)
🪛 GitHub Check: lint
app_test.go
[failure] 2770-2770:
unused-parameter: parameter 'c' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 2823-2823:
httpNoBody: http.NoBody should be preferred to the nil request body (gocritic)
[failure] 2819-2819:
The copy of the 'for' variable "testCase" can be deleted (Go 1.22+) (copyloopvar)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
duhnnie
force-pushed
the
v3-fix-custom-errorhandler-invokation
branch
from
a647a8f to
399266f
Compare
duhnnie
force-pushed
the
v3-fix-custom-errorhandler-invokation
branch
from
399266f to
e151b4b
Compare
duhnnie
changed the title
2 participants
Description
This PR is a V3 port of this fix for this issue originally reported for V2.