🐛 bug: Fix handler order in routing #3321
Conversation
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
WalkthroughThis pull request updates the method signatures across several files to replace the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3321 +/- ##
==========================================
- Coverage 84.22% 84.12% -0.10%
==========================================
Files 116 116
Lines 11560 11561 +1
==========================================
- Hits 9736 9726 -10
- Misses 1395 1403 +8
- Partials 429 432 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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)
app.go (1)
753-753: Consider documenting when group is nil.You're passing
nilfor the group parameter inapp.register. While this looks intentional, adding a brief comment clarifying thatnilindicates no group context could improve readability and maintenance.register.go (1)
48-49: New code for the All method - add test coverage.Lines 48-49 are newly introduced but flagged by static analysis as uncovered. Consider adding specific test scenarios for the
Allroute usage to ensure coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-49: register.go#L48-L49
Added lines #L48 - L49 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.go(2 hunks)docs/api/app.md(1 hunks)docs/partials/routing/handler.md(2 hunks)register.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
register.go
[warning] 48-49: register.go#L48-L49
Added lines #L48 - L49 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Analyse
- GitHub Check: Compare
🔇 Additional comments (28)
app.go (11)
759-763: Signature update for Get.Switching from
middleware ...Handlertohandlers ...Handlerprovides a more generic naming scheme. Be sure all references across the codebase and docs are updated accordingly.
767-770: Signature update for Head.This change aligns with the new
handlers ...Handlerconvention. The method structure appears consistent with the rest of the code.
773-776: Signature update for Post.Adopting the
handlersnaming is clear and consistent. No issues found.
779-782: Signature update for Put.This looks good. The method signature is more descriptive of the parameters involved.
784-787: Signature update for Delete.No concerns. The parameter change follows the new convention.
788-791: Signature update for Connect.Consistent naming convention adopted. Code remains clear and straightforward.
794-797: Signature update for Options.Looks consistent with the updated pattern. No issues spotted.
800-803: Signature update for Trace.Switching to
handlers ...Handlerfosters alignment and consistency here as well.
806-809: Signature update for Patch.Renaming
middlewaretohandlersmaintains the uniform naming convention across all methods.
812-815: Signature update for Add.The revised variadic parameter naming is consistent with the other methods. No additional concerns.
819-821: Signature update for All.Renaming the parameter for the
Allmethod improves clarity and unifies its behavior with other route methods.register.go (12)
9-19: Renamed interface methods to use 'handlers'.The interface methods now consistently use
handlers ...Handlerrather thanmiddleware ...Handler. This uniform terminology reduces confusion about method parameters.
20-20: Signature update for Add in Register interface.Changing from
middleware ...Handlertohandlers ...Handlercompletes the consistency across the interface's methods.
55-56: Signature update for Get.Renaming to
handlersis coherent with the rest of the code. No issues identified.
63-64: Signature update for Head.All references to
middlewarereplaced byhandlers. The code is succinct and consistent.
68-69: Signature update for Post.Clean transition to the new naming pattern. Looks good.
74-75: Signature update for Put.No problem spotted. The approach remains consistent.
79-80: Signature update for Delete.No issues identified. This change aligns with the overall rename effort.
85-86: Signature update for Connect.Straightforward parameter renaming. The method logic appears unaffected.
91-92: Signature update for Options.Continues the naming consistency. Good to see the uniform approach maintained.
97-98: Signature update for Trace.Use of
handlersclarifies the function intent. No additional concerns.
103-104: Signature update for Patch.No issues. This renaming step is consistent with the others.
108-109: Add method usage changed.Applying
handlers ...Handlerensures the function can handle multiple chained handlers seamlessly.docs/partials/routing/handler.md (4)
12-20: Updated HTTP method signatures to use 'handlers'.These doc changes accurately reflect the new
handlers ...Handlerparameter introduced in the code. Documentation remains clear and instructive.
23-23: Add method doc updated.Renaming the variadic parameter to
handlerskeeps the documentation consistent with the implementation code.
27-27: All method doc updated.The revised signature highlights that multiple handlers can be chained, which aligns with the rest of the doc changes.
50-52: Use method doc updated.Switching from
middlewares ...Handlertohandlers ...Handlerclarifies that any function can be included, whether it's a middleware or a final handler.docs/api/app.md (1)
137-150: Update variadic parameter naming in Register interface.
The updated documentation correctly reflects the API change where the parameter formerly namedmiddleware ...Handlerhas been renamed tohandlers ...Handler. This change enhances clarity and maintains consistency with the updated routing handler registration logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router.go (1)
321-326: Improve the panic message for better debugging.While the validation for missing handlers is good, the panic message could be more descriptive.
- panic(fmt.Sprintf("missing handler/middleware in route: %s\n", pathRaw)) + panic(fmt.Sprintf("missing handler/middleware in route: %s (method: %v)\n", pathRaw, methods))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 325-325: router.go#L325
Added line #L325 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
router.go
[warning] 325-325: router.go#L325
Added line #L325 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
🔇 Additional comments (1)
router.go (1)
23-34: LGTM! Consistent parameter naming across Router interface methods.The change from
middleware ...Handlertohandlers ...Handlerimproves code consistency and better reflects that these parameters can be both middleware and regular handlers.
router.go
Outdated
| if len(handlers) == 0 && !isMount { | ||
| panic(fmt.Sprintf("missing handler/middleware in route: %s\n", pathRaw)) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for the missing handler validation.
The static analysis indicates that the validation code is not covered by tests. Please add test cases to verify this validation.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any existing tests for missing handler validation
echo "Searching for existing tests..."
rg -l 'missing handler/middleware in route' test_*.go || echo "No existing tests found"
echo -e "\nSearching for similar panic tests..."
ast-grep --pattern 'func Test$_($_ *testing.T) {
$$$
panic($$$)
$$$
}'Length of output: 405
Action Required: Add Tests for Missing Handler Validation
The current code in router.go (lines 324-326) triggers a panic when no handler/middleware is provided:
if len(handlers) == 0 && !isMount {
panic(fmt.Sprintf("missing handler/middleware in route: %s\n", pathRaw))
}However, our search confirms that there are no tests covering this scenario. Please add test cases that explicitly:
- Invoke the route setup with an empty handler list.
- Assert that the function panics with the expected error message.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 325-325: router.go#L325
Added line #L325 was not covered by tests
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: acc80f8 | Previous: 5b0a96e | Ratio |
|---|---|---|---|
BenchmarkAppendMsgitem-4_middleware_csrf |
0.6236 ns/op 1603.55 MB/s 0 B/op 0 allocs/op |
0.3184 ns/op 3140.87 MB/s 0 B/op 0 allocs/op |
1.96 |
BenchmarkAppendMsgitem-4_middleware_csrf - ns/op |
0.6236 ns/op |
0.3184 ns/op |
1.96 |
BenchmarkAppendMsgstorageManager |
0.6225 ns/op 1606.46 MB/s 0 B/op 0 allocs/op |
0.3127 ns/op 3197.96 MB/s 0 B/op 0 allocs/op |
1.99 |
BenchmarkAppendMsgstorageManager - ns/op |
0.6225 ns/op |
0.3127 ns/op |
1.99 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router.go (1)
324-324: Consider using string concatenation for better performance.Replace
fmt.Sprintfwith string concatenation for better performance in error messages.-panic(fmt.Sprintf("missing handler/middleware in route: %s", pathRaw)) +panic("missing handler/middleware in route: " + pathRaw) -panic(fmt.Sprintf("nil handler in route: %s", pathRaw)) +panic("nil handler in route: " + pathRaw)Also applies to: 329-329
🧰 Tools
🪛 GitHub Check: lint
[failure] 324-324:
fmt.Sprintf can be replaced with string concatenation (perfsprint)🪛 GitHub Actions: golangci-lint
[warning] 324-324: fmt.Sprintf can be replaced with string concatenation (perfsprint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app_test.go(3 hunks)hooks_test.go(2 hunks)mount.go(2 hunks)router.go(3 hunks)router_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
router.go
[failure] 324-324:
fmt.Sprintf can be replaced with string concatenation (perfsprint)
[failure] 329-329:
fmt.Sprintf can be replaced with string concatenation (perfsprint)
🪛 GitHub Actions: golangci-lint
router.go
[warning] 324-324: fmt.Sprintf can be replaced with string concatenation (perfsprint)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (9)
hooks_test.go (1)
89-91: LGTM! Enhanced test robustness.The addition of explicit failure checks when expected panics don't occur improves test reliability by preventing false positives.
Also applies to: 175-177
mount.go (1)
58-58: LGTM! Simplified handler registration.The removal of the
nilargument aligns with the updated handler registration pattern across the codebase.Also applies to: 88-88
router.go (2)
23-34: LGTM! Consistent handler parameter naming.The standardization of
handlers ...Handleracross all router methods improves API consistency.
323-331: LGTM! Added robust handler validation.The new validation ensures that routes are properly configured with valid handlers, preventing runtime issues.
🧰 Tools
🪛 GitHub Check: lint
[failure] 324-324:
fmt.Sprintf can be replaced with string concatenation (perfsprint)
[failure] 329-329:
fmt.Sprintf can be replaced with string concatenation (perfsprint)🪛 GitHub Actions: golangci-lint
[warning] 324-324: fmt.Sprintf can be replaced with string concatenation (perfsprint)
router_test.go (2)
34-65: LGTM! Added comprehensive handler order test.The new test effectively verifies that multiple handlers are executed in the correct sequence.
331-352: LGTM! Improved test organization.The restructuring into sub-tests for "No Handler" and "Nil Handler" scenarios improves test clarity and maintainability.
app_test.go (3)
485-486: LGTM! Improved test reliability by handling missing panic case.The addition of the else clause ensures the test fails explicitly when an expected panic doesn't occur, making the test more reliable and easier to debug.
1063-1064: LGTM! Consistent error handling for panic recovery.The added else clause follows the same pattern as other panic tests, maintaining consistency throughout the test suite.
1295-1296: LGTM! Completed panic recovery with proper error handling.The else clause addition completes the panic recovery pattern, ensuring test failures are explicit when expected panics don't occur.
gaby
changed the title
|
@ReneWerner87 I think docs need to be updated: https://docs.gofiber.io/next/whats_new#http-method-registration At present state, what is the current "request-flow" for the handlers in the current signature: |
the documentation is already correct, the only change compared to v3 is that the signature now expects at least one handler Previously, the remaining parameters made it possible not to inject a handler and the code threw an error at runtime and not at compile time |
|
In the new docs, it says the end handler goes second and the "middleware" goes next. In your PR there is no distinction made for middleware. |
okay i didn't know, can you show me which specific line you mean |
|
|
|
So where does the end-handler go? At the end like in V2? |
yes right at the end like in v2, that's why we didn't put any additional description there |
|
@coderabbitai change the pr summary and update with all changes from this pr |
|
Tip For best results, initiate chat on the files or code changes. @ReneWerner87, I'll update the PR summary to comprehensively cover all the changes in this pull request. Updated PR SummaryThis pull request addresses issue #3312 by fixing handler order in routing and standardizing method signatures across the Fiber framework. The changes ensure proper handler execution order while improving the consistency and validation of route registration methods. 🔧 Core ChangesMethod Signature Standardization:
Handler Registration Improvements:
Internal Logic Updates:
📁 Files ModifiedCore Framework Files:
Testing & Validation:
Documentation:
🎯 Key Benefits
🔄 Handler Execution FlowThe handler execution order remains the same as v2:
This change ensures that |
Fixes #3312