🧹 chore: Add support for handling unsupported HTTP methods as HTTP 501#3854
🧹 chore: Add support for handling unsupported HTTP methods as HTTP 501#3854ReneWerner87 merged 2 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. WalkthroughA new error case is added to serverErrorHandler to detect "unsupported http request method" errors and map them to HTTP 501 Not Implemented status. Two tests verify the behavior across direct error-handling and full request-path scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3854 +/- ##
=======================================
Coverage 92.10% 92.10%
=======================================
Files 115 115
Lines 9754 9756 +2
=======================================
+ Hits 8984 8986 +2
Misses 490 490
Partials 280 280
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.
Pull Request Overview
This PR adds error handling for unsupported HTTP request methods in the server error handler, mapping them to HTTP 501 Not Implemented status code.
- Adds string-based detection of "unsupported http request method" errors in
serverErrorHandler - Includes comprehensive test coverage for both unit and integration testing scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app.go | Adds case to detect and map unsupported HTTP method errors to ErrNotImplemented |
| app_test.go | Adds two test cases: one for direct error handler testing and one for full request flow testing |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app.go (1)
1324-1325: LGTM - Consider adding explanatory commentThe implementation correctly maps unsupported HTTP method errors to 501 Not Implemented, aligning with RFC 9110 requirements. This follows the same pattern as existing error handling in the function.
However, similar to the concern raised in a previous review about string-based error detection, consider adding a comment explaining why string matching is necessary here. This would improve maintainability if fasthttp's error messages change in the future.
Example:
+ // fasthttp does not provide a dedicated error type for unsupported HTTP methods, + // so we must match the error string. If fasthttp adds such an error type in the future, + // this should be updated to use errors.Is or errors.As instead. case strings.Contains(err.Error(), "unsupported http request method"): err = ErrNotImplemented
📜 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 (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.goapp_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
📚 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_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.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.goapp_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
📚 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_test.go
🧬 Code graph analysis (2)
app.go (1)
constants.go (1)
ErrNotImplemented(149-149)
app_test.go (3)
app.go (1)
New(521-634)ctx.go (5)
DefaultCtx(51-72)DefaultCtx(138-140)DefaultCtx(150-152)DefaultCtx(158-160)DefaultCtx(433-435)constants.go (1)
StatusNotImplemented(104-104)
⏰ 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). (2)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (2)
app_test.go (2)
631-639: LGTM - Well-structured unit testThis test provides focused coverage of the serverErrorHandler's new behavior for unsupported HTTP methods. The test properly verifies both the status code and response body, following established patterns in the test file.
641-684: LGTM - Comprehensive integration testThis integration test provides excellent end-to-end coverage of the unsupported HTTP method handling. The test properly:
- Spins up a real server with proper lifecycle management
- Sends a raw HTTP request with an unsupported method
- Verifies the complete response including status and body
- Cleans up all resources properly
The use of channels for goroutine synchronization and proper connection deadline handling demonstrates good testing practices.
Summary
Fixes #3851