🐛 bug: Execute middleware routes when handling errors#3846
🐛 bug: Execute middleware routes when handling errors#3846ReneWerner87 merged 8 commits intomainfrom
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. WalkthroughAdds a skipNonUseRoutes flag to contexts, uses it during serverErrorHandler to traverse remaining middleware for recognized methods (skipping non-use routes), updates App.next/App.nextCustom to respect the flag, and adds integration tests validating 413 (body-limit) behavior and header preservation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server as serverErrorHandler
participant Router
participant MW as Middleware
participant ErrorH as AppErrorHandler
Client->>Server: Request triggers body-limit error (413)
activate Server
note right of Server `#f9f2d9`: If method recognized\nset skipNonUseRoutes = true
Server->>Server: setSkipNonUseRoutes(true)
Server->>Router: Traverse remaining middleware (app.next / app.nextCustom)
activate Router
note right of Router `#e8f7ef`: Router skips non-use routes\nwhen skip flag is true
Router->>MW: Invoke matching use-route middleware
activate MW
MW->>MW: Update headers/state (CORS, Helmet, X-Request-ID)
deactivate MW
deactivate Router
Server->>Server: setSkipNonUseRoutes(false)
Server->>ErrorH: Call app-level ErrorHandler
activate ErrorH
ErrorH->>Client: Respond (headers from middleware preserved)
deactivate ErrorH
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
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)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (10)📚 Learning: 2024-11-15T07:56:21.623ZApplied to files:
📚 Learning: 2024-11-10T23:44:13.704ZApplied to files:
📚 Learning: 2024-11-08T04:10:42.990ZApplied to files:
📚 Learning: 2024-10-08T19:06:06.583ZApplied to files:
📚 Learning: 2024-10-08T19:06:06.583ZApplied to files:
📚 Learning: 2025-10-16T07:19:52.418ZApplied to files:
📚 Learning: 2024-12-13T08:14:22.851ZApplied to files:
📚 Learning: 2024-10-02T23:03:31.727ZApplied to files:
📚 Learning: 2025-10-16T07:15:26.529ZApplied to files:
📚 Learning: 2024-11-29T12:37:27.581ZApplied to files:
🧬 Code graph analysis (1)app_integration_test.go (3)
⏰ 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)
🔇 Additional comments (6)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3846 +/- ##
==========================================
- Coverage 92.18% 92.14% -0.05%
==========================================
Files 115 115
Lines 9754 9754
==========================================
- Hits 8992 8988 -4
- Misses 484 486 +2
- Partials 278 280 +2
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:
|
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the serverErrorHandler to execute middleware (use routes) when handling errors that occur before normal request processing, such as body size limit violations. This ensures that middleware like CORS can set appropriate headers even when errors occur early in the request lifecycle.
Key changes:
- Added a
skipNonUseRoutesflag to the context that filters routing to only execute middleware (use routes) while skipping regular route handlers - Modified
serverErrorHandlerto re-run the middleware chain with this flag enabled before calling the error handler - Added comprehensive integration tests covering CORS header preservation in error scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ctx.go | Added skipNonUseRoutes field to DefaultCtx and implemented getter/setter methods; reset flag in Reset() and release() |
| ctx_interface.go | Added getSkipNonUseRoutes() and setSkipNonUseRoutes() to CustomCtx interface |
| ctx_interface_gen.go | Updated generated interface with new skip methods |
| router.go | Modified next() and nextCustom() to skip non-use routes when flag is set |
| app.go | Enhanced serverErrorHandler to run middleware chain before error handling |
| app_integration_test.go | Added integration tests verifying middleware execution during error handling with both default and custom contexts |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to ensure middleware, such as for CORS, is executed even when fasthttp encounters a server-level error like the request body being too large. This is achieved by adding a new path in the serverErrorHandler to run the middleware stack before invoking the final error handler. The logic is controlled by a new skipNonUseRoutes flag in the context. New integration tests have been added to verify this behavior, including scenarios with custom contexts and multiple middleware.
My review has two main points:
- In
app.go, potential errors from the middleware stack during this special error handling path are ignored. This could hide bugs in middleware, and I've suggested logging these errors for better observability. - In
ctx.go, there's a minor formatting inconsistency in theDefaultCtxstruct that could be addressed for better readability.
Overall, the changes are well-implemented and the tests are thorough.
|
@codex Review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ctx_interface.go (1)
29-29: Breaking change acceptable for v3.The addition of
getSkipNonUseRoutes()andsetSkipNonUseRoutes(skip bool)to theCustomCtxinterface is a breaking change for external implementations, as noted in the previous review. However, this is acceptable since the PR targets v3. The methods follow the established naming pattern for internal context state management.Based on learnings
Also applies to: 33-33
ctx.go (1)
71-71: LGTM!The field is properly declared with a clear comment. The comment alignment was previously noted as a minor readability concern but doesn't affect functionality.
🧹 Nitpick comments (1)
app.go (1)
1330-1345: Consider using defer for flag restoration.The flag is properly set before middleware traversal and restored afterward. However, if a panic occurs during traversal, the flag won't be restored. While panics during error handling are unlikely, using defer would be more defensive:
if c.getMethodInt() != -1 { c.setSkipNonUseRoutes(true) + defer c.setSkipNonUseRoutes(false) var nextErr error if d, isDefault := c.(*DefaultCtx); isDefault { _, nextErr = app.next(d) } else { _, nextErr = app.nextCustom(c) } if nextErr != nil && !errors.Is(nextErr, ErrNotFound) && !errors.Is(nextErr, ErrMethodNotAllowed) { log.Errorf("serverErrorHandler: middleware traversal failed: %v", nextErr) } - - c.setSkipNonUseRoutes(false) }The error logging properly excludes expected routing errors as confirmed in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app.go(1 hunks)app_integration_test.go(1 hunks)ctx.go(5 hunks)ctx_interface.go(1 hunks)ctx_interface_gen.go(1 hunks)router.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
router.goctx_interface.goapp.goctx_interface_gen.goctx.goapp_integration_test.go
🧠 Learnings (8)
📚 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:
ctx_interface.goapp_integration_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.goapp_integration_test.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.goapp_integration_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.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:
ctx.goapp_integration_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:
ctx.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
app_integration_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_integration_test.go
🧬 Code graph analysis (3)
app.go (3)
ctx.go (5)
DefaultCtx(51-72)DefaultCtx(138-140)DefaultCtx(150-152)DefaultCtx(158-160)DefaultCtx(433-435)constants.go (2)
ErrNotFound(122-122)ErrMethodNotAllowed(123-123)log/fiberlog.go (1)
Errorf(49-51)
ctx.go (1)
router.go (2)
App(376-396)Route(43-64)
app_integration_test.go (5)
ctx.go (5)
DefaultCtx(51-72)DefaultCtx(138-140)DefaultCtx(150-152)DefaultCtx(158-160)DefaultCtx(433-435)app.go (4)
App(68-110)New(521-634)Config(113-424)NewWithCustomCtx(639-643)listen.go (1)
ListenConfig(42-125)constants.go (8)
MethodPost(7-7)HeaderOrigin(206-206)StatusOK(52-52)StatusRequestEntityTooLarge(86-86)HeaderAccessControlAllowOrigin(201-201)HeaderAccessControlAllowCredentials(198-198)HeaderAccessControlExposeHeaders(202-202)HeaderVary(187-187)ctx_interface_gen.go (1)
Ctx(18-430)
⏰ 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: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (11)
ctx_interface_gen.go (1)
163-163: LGTM!The generated interface additions mirror the
CustomCtxchanges and follow the established pattern. No concerns with the generated code.Also applies to: 167-167
router.go (2)
142-144: LGTM!The filtering logic correctly skips non-use routes after a match when the flag is set. This enables middleware-only traversal during error handling, which aligns with the PR objectives.
242-244: LGTM!Consistent with the
App.nextimplementation, properly using the getter method forCustomCtx. The filtering logic is correct.ctx.go (3)
558-558: LGTM!Proper initialization in the
Resetmethod ensures the flag starts false for each new request.
589-589: LGTM!The flag is properly reset in the
releasemethod, ensuring clean state when the context is returned to the pool.
662-664: LGTM!The getter and setter implementations are straightforward and follow the established pattern for internal context state accessors.
Also applies to: 678-680
app_integration_test.go (5)
18-24: LGTM!The custom context type provides appropriate test coverage for the custom context code path. The implementation correctly embeds
DefaultCtxand follows theCustomCtxinterface pattern.
26-84: Well-structured helper function.The
performOversizedRequesthelper properly manages resources and provides a clean testing interface. The in-memory listener setup, server lifecycle management, and response copying are all handled correctly. Therequire.Eventuallypattern prevents flaky tests by waiting for server readiness.The 32-byte request body works with the test configurations (BodyLimit: 8, 16) to trigger oversized request errors.
86-104: LGTM!This test validates the core PR objective: ensuring middleware (CORS) executes during early error handling (body limit violations). The assertions comprehensively verify that CORS headers are preserved in the 413 error response.
106-122: LGTM!This test confirms that multiple middleware handlers execute during error handling, with each able to set response headers. The test validates both custom middleware and default CORS configuration work correctly on the error path.
124-144: LGTM!This test ensures the custom context code path (
app.nextCustom) works correctly during error handling. The type assertion in the middleware confirms the custom context is preserved throughout the traversal, and CORS headers are properly set.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app_integration_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
app_integration_test.go
🧠 Learnings (5)
📚 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_integration_test.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_integration_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_integration_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_integration_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_integration_test.go
🧬 Code graph analysis (1)
app_integration_test.go (4)
ctx.go (5)
DefaultCtx(51-72)DefaultCtx(138-140)DefaultCtx(150-152)DefaultCtx(158-160)DefaultCtx(433-435)app.go (4)
App(68-110)New(521-634)Config(113-424)NewWithCustomCtx(639-643)ctx_interface.go (2)
CustomCtx(15-35)NewDefaultCtx(39-49)constants.go (9)
MethodPost(7-7)HeaderOrigin(206-206)StatusOK(52-52)StatusRequestEntityTooLarge(86-86)HeaderAccessControlAllowOrigin(201-201)HeaderAccessControlAllowCredentials(198-198)HeaderAccessControlExposeHeaders(202-202)HeaderVary(187-187)HeaderXContentTypeOptions(246-246)
⏰ 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: Compare
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: lint
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (4)
app_integration_test.go (4)
27-85: LGTM! Well-structured test helper.The helper function properly manages the lifecycle of the in-memory server, handles resource pooling, and ensures cleanup. Good use of
t.Helper(),t.Cleanup(), andrequire.Eventually()for robust test orchestration.
87-105: LGTM! Test validates CORS header preservation on 413 errors.The test correctly triggers a body-limit violation and verifies that CORS middleware headers are preserved in the error response. Assertions are comprehensive and match the configured CORS policy.
107-121: LGTM! Test validates Helmet header preservation on 413 errors.The test correctly verifies that Helmet middleware security headers are preserved when the body-limit error occurs. Assertions cover the key security headers set by Helmet.
123-139: LGTM! Test validates multiple middleware headers are retained.The test confirms that both custom middleware and subsequent CORS middleware execute during error handling, preserving headers from each. The assertion for the default CORS origin (
"*") is correct.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 810fe69 | Previous: 490d4b0 | Ratio |
|---|---|---|---|
Benchmark_Compress_Parallel/Zstd - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
Summary
Fixes #3803
Fixes #1940
Fixes #2086