🩹 bug: Fix ErrorHandler invocation for mounted sub-apps #3907
Conversation
|
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. WalkthroughReplaced nondeterministic mount iteration with deterministic iteration over Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MainApp
participant MountResolver as MountResolver (iterates keys)
participant SubApp
participant ErrorHandler
Client->>MainApp: HTTP request (path)
MainApp->>MountResolver: iterate mountKeys, normalize path & prefixes
MountResolver-->>MainApp: select best-matching SubApp (by normalized prefix)
MainApp->>SubApp: forward request
SubApp->>ErrorHandler: on handler error -> invoke SubApp.ErrorHandler (if set) else bubble up
Note right of MainApp: If SubApp has no ErrorHandler,\nMainApp or nearest ancestor ErrorHandler is used
ErrorHandler-->>Client: formatted error response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 addresses a critical bug in the application's error handling mechanism, specifically concerning how error handlers are invoked for mounted sub-applications. By switching from an unordered map iteration to an ordered key list, it ensures that the correct error handler is consistently identified and used, thereby improving the reliability and predictability of error management across the application. 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 addresses an issue of non-deterministic behavior when selecting an ErrorHandler for mounted sub-apps. The change correctly replaces iteration over a map with iteration over a pre-sorted slice of keys, ensuring a consistent order. However, my review identified a remaining logical flaw in how the most specific error handler is chosen, which could lead to an incorrect handler being used in certain scenarios. I've left a detailed comment on this.
| for _, prefix := range app.mountFields.appListKeys { | ||
| subApp := app.mountFields.appList[prefix] | ||
| if prefix != "" && strings.HasPrefix(ctx.path, prefix) { | ||
| parts := len(strings.Split(prefix, "/")) | ||
| if mountedPrefixParts <= parts { |
There was a problem hiding this comment.
While this change correctly addresses the non-deterministic iteration by using a sorted slice of keys, there's a remaining logical issue in how the error handler is selected. The current logic can incorrectly choose an error handler from a less specific mounted app.
For example, if you have apps mounted at /api (with a custom error handler) and /api/v1 (without one), a request to /api/v1/users will incorrectly use the error handler from /api instead of falling back to the root app's handler.
This happens because mountedErrHandler is not reset when a more specific prefix (with no custom error handler) is found. The logic should be adjusted to ensure that only the error handler for the most specific matching prefix is considered. If that most specific prefix doesn't have a custom error handler, mountedErrHandler should be nil so that the root handler is used.
To fix this, the logic should be updated to reset mountedErrHandler when a more specific prefix is found.
There was a problem hiding this comment.
Can anyone check if what gemini-code-assist says is correct? I think that if you have apps mounted at /api (with a custom error handler) and /api/v1 (without one), a request to /api/v1/users should use the error handler from the closest mount-path which would be /api instead of falling back to the root app's handler. Am i right?
There was a problem hiding this comment.
Thank you for raising this important question, @duhnnie. I apologize for the confusion in my previous comment; my analysis of the desired behavior and the current code's outcome after your PR was indeed mistaken.
You are absolutely correct in your understanding:
"if you have apps mounted at
/api(with a custom error handler) and/api/v1(without one), a request to/api/v1/usersshould use the error handler from the closest mount-path which would be/apiinstead of falling back to the root app's handler."
This is the correct and expected hierarchical fallback behavior for mounted applications.
Let me clarify why your PR, with the change to iterate over app.mountFields.appListKeys (which are sorted by key length, typically ascending), actually achieves this desired behavior:
- Iteration Order: By iterating through
appListKeyssorted by length (e.g.,"/","/api","/api/v1"), the loop processes shorter, less specific prefixes first, and then moves to longer, more specific ones. mountedErrHandlerandmountedPrefixParts:- When the loop encounters a matching prefix (e.g.,
"/api"for"/api/v1/users") that has anerrorHandler, it updatesmountedErrHandlerto that handler andmountedPrefixPartsto its length. - Crucially, when it later encounters a more specific matching prefix (e.g.,
"/api/v1") that does not have its ownerrorHandler,mountedErrHandleris not overwritten (becausesubApp.errorHandlerisnil). However,mountedPrefixPartsis updated to the length of this more specific prefix (e.g., 2 for"/api/v1").
- When the loop encounters a matching prefix (e.g.,
- The
mountedPrefixParts <= partsCondition: This condition ensures that once a more specific prefix has been processed (andmountedPrefixPartsupdated to its length), any less specific prefixes (i.e., those withpartsless than the currentmountedPrefixParts) will be skipped, preventing them from overwriting themountedErrHandlerwith a less specific one, unless the currentmountedErrHandlerisniland a shorter prefix has a handler.
Example Trace (with EH_ROOT, EH_A for /api, and nil for /api/v1):
- Initial:
mountedErrHandler = nil,mountedPrefixParts = 0 - Prefix
/: Matches.EH_ROOTis set.mountedPrefixParts = 1. - Prefix
/api: Matches.EH_Ais set (overwritingEH_ROOT).mountedPrefixParts = 1. - Prefix
/api/v1: Matches.subApp.errorHandlerisnil.mountedErrHandlerremainsEH_A.mountedPrefixPartsbecomes2.
Result: After the loop, mountedErrHandler will correctly be EH_A, which is the handler for the closest mounted app (/api) that has one, as you intended.
My apologies again for the misdirection. Your PR correctly implements the desired hierarchical error handler selection. The initial comment was based on an incorrect interpretation of the code's flow with the sorted keys.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where the ErrorHandler selection for mounted sub-apps was non-deterministic due to Go's unordered map iteration. The fix ensures consistent ErrorHandler resolution by iterating over an ordered slice of keys instead of the map directly.
Key Changes:
- Modified the ErrorHandler method to iterate over
app.mountFields.appListKeys(ordered slice) instead of directly iterating theapp.mountFields.appListmap - This guarantees deterministic ErrorHandler selection that matches the longest prefix, resolving issue #3906
gaby
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@duhnnie Thanks for the fix. Do you know if the problem also exists in version 3? Can you add a unit test that would fail in the target branch without your fix and would run after your fix? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
utils/strings.go (1)
7-8: Document the new exported helper (and optionally simplify the check).
AddTrailingSlash’s behavior looks correct, but it’s exported and currently lacks a Go doc comment, unlike the other helpers in this file. Please add a short comment (// AddTrailingSlash ...) so linters and docs stay clean. You could also (optionally) avoid importingstringsby checking the last byte directly:func AddTrailingSlash(s string) string { if len(s) > 0 && s[len(s)-1] == '/' { return s } return s + "/" }Also applies to: 79-85
app_test.go (1)
1981-2053: Align the new test with project conventions and clean up leftovers.The core logic of
TestErrorHandler_PicksRightOnelooks good and matches the intended error‑handler resolution, but a few small cleanups would help:
- Add
t.Parallel()at the start ofTestErrorHandler_PicksRightOneto follow the repo’s convention for new tests.errorHandlerTestCaseis currently unused; either wire it intotestCasesor remove it to avoid dead code.- The comment above the
"/api/v1/use"case still says"/api/v1/users mount has a custom ErrorHandler"; updating it to"/api/v1/use"would avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.go(1 hunks)app_test.go(1 hunks)utils/strings.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
utils/strings.goapp_test.go
**/*_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
🧠 Learnings (4)
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
utils/strings.go
📚 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-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
|
I verified that this issue also occurs in Version 3. You can reproduce it by cloning and running this repository: https://github.com/duhnnie/Fiber-bug-repo/tree/V3 (branch I also identified an additional related issue, which has been fixed as well. Please see the updated PR description above for full details (I’m surprised it went unnoticed until now). A unit test has been added to cover this fix. Let me know if you need anything else to proceed with the merge. Thank you. |
app_test.go
Outdated
| }) | ||
| } | ||
|
|
||
| type errorHandlerTestCase struct { |
|
@duhnnie pls fix the lint errors can you also port this fix to v3 ? |
|
I tested it and you have a valid point. |
duhnnie
force-pushed
the
v2-fix-custom-errorhandler-invokation
branch
2 times, most recently
from
daa8c85 to
4751ea6
Compare
duhnnie
force-pushed
the
v2-fix-custom-errorhandler-invokation
branch
from
4751ea6 to
030a73f
Compare
duhnnie
force-pushed
the
v2-fix-custom-errorhandler-invokation
branch
from
030a73f to
8c6b0c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/strings_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:
utils/strings_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:
utils/strings_test.go
🧬 Code graph analysis (1)
utils/strings_test.go (1)
utils/strings.go (1)
AddTrailingSlash(80-86)
🔇 Additional comments (1)
utils/strings_test.go (1)
262-278: LGTM! Comprehensive benchmark coverage.The benchmark effectively covers various scenarios including edge cases (empty string) and performance edge cases (long strings). The test cases validate both the fast path (already has slash) and slow path (concatenation required).
|
@ReneWerner87 @gaby Ok guys, I think I have this PR ready.
Respecting Version 3, I need this PR to be merged first in order to port this fix to V3: gofiber/utils#162 |
duhnnie
force-pushed
the
v2-fix-custom-errorhandler-invokation
branch
from
3196226 to
2c70b2e
Compare
3 participants
PR Description
This PR resolves two issues related to the
ErrorHandlerassignment.Issue 1
There was a problem with how mounted sub-apps were compared against the current request path to determine the appropriate
ErrorHandler. Previously, the code iterated directly overapp.mountFields.appList(a map). Since Go does not guarantee map iteration order, this resulted in inconsistent behavior: the selectedErrorHandlercould differ between runs.The fix ensures iteration uses
app.mountFields.appListKey, an array that contains the mount keys in a deterministic order (sorted by key length for Render). This guarantees consistent iteration and ensures the correct access order intoapp.mountFields.appList.Example
Consider the following mounts (sub-apps):
[empty]: the root path, with no customErrorHandler/api/v1: with customErrorHandler/api/v1/users: with no customErrorHandler— it should fall back to the nearest upper mount (/api/v1)The issue arose from iterating over a map containing the path mounts as keys. In Go, map iteration order is nondeterministic.
Suppose a request to
/api/v1/usersfails, and Fiber attempts to determine the correctErrorHandlerby invokingErrorHandler(). The map might be iterated in the following order:[empty]/api/v1/users/api/v1(which has a customErrorHandler)The algorithm contains a condition to select a custom
ErrorHandlerfrom the current iteration if applicable. It also checks that the number of path segments is greater than or equal to the previous iteration. In this scenario, the loop stops after the second iteration (/api/v1/users). Because no customErrorHandlerwas found up to that point, Fiber incorrectly falls back to theDefaultErrorHandler, instead of the one in/api/v1.About the Fix
The fix replaces map iteration with iteration over an existing ordered (by length) array of mount paths.
Important: this bug occurred randomly due to nondeterministic map iteration.
Issue 2
This issue was similar, also related to incorrect path matching. Consider the following mounts:
[empty]: no customErrorHandler/api/v1: with customErrorHandler/api/v1/users: no customErrorHandler/api/v1/use: with customErrorHandlerGiven a failing request to
/api/v1/users, and assuming Issue 1 is fixed, theErrorHandlerresolution iterates over the mount paths in this order:[empty]: empty path, so continue/api/v1: matches as a prefix and has a customErrorHandler, so it is selected/api/v1/use: incorrectly matches as a prefix and also has a customErrorHandler, so it overrides the previous selection/api/v1/users: matches as a prefix but has no customErrorHandler, so it keeps the previous selectionAs a result,
/api/v1/usersends up using theErrorHandlerfrom/api/v1/use, which is incorrect. It should use the one from/api/v1.This happens because a comparison like:
is logically incorrect for this use case.
About the Fix
The fix applies normalization to paths only during comparison. The normalization consists of adding a trailing slash to both paths, ensuring comparisons behave correctly:
This guarantees proper matching without affecting the original mounts.
Fixes #3906