🔥 feat: add SkipUnmatchedRoutes config option#4411
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 Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ChangesSkipUnmatchedRoutes Feature
Sequence DiagramsequenceDiagram
participant Client
participant Handler as defaultRequestHandler/customRequestHandler
participant Config as app.config.SkipUnmatchedRoutes
participant RouteChecker as App.routeExists
participant Middleware as middleware chain
participant Endpoint as matched endpoint route
Client->>Handler: incoming HTTP request
Handler->>Config: read SkipUnmatchedRoutes
alt SkipUnmatchedRoutes == true
Handler->>RouteChecker: routeExists(method, path)
alt routeExists == false
Handler->>Handler: return 404 StatusNotFound
else routeExists == true
Handler->>Middleware: enter middleware chain
Middleware->>Endpoint: execute matched endpoint
end
else SkipUnmatchedRoutes == false
Handler->>Middleware: enter middleware chain
Middleware->>Endpoint: scan & execute matched endpoint
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router_test.go (1)
2570-2665: ⚡ Quick winAdd a custom-context variant for
SkipUnmatchedRoutesto cover both handler paths.These tests validate the default handler path well, but they don’t explicitly assert behavior through
newCustomApp()/custom request handling. Adding one matched + one unmatched subtest there would guard against regressions incustomRequestHandlertoo.Based on learnings from provided context:
SkipUnmatchedRoutesshort-circuit logic is present in both default and custom handlers (router.go:449-502).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router_test.go` around lines 2570 - 2665, Add companion subtests to Test_App_SkipUnmatchedRoutes that exercise the custom-context path by creating an app via newCustomApp() (or the equivalent factory used in custom request handling) and invoking its Test helper with both a matched route ("/users") and an unmatched route ("/notfound" or case/strict variants) while SkipUnmatchedRoutes is true; assert middlewareCalled semantics match the existing default-app cases so the short-circuit logic in customRequestHandler (and related code handling SkipUnmatchedRoutes) is covered and won’t regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@router_test.go`:
- Around line 2570-2665: Add companion subtests to Test_App_SkipUnmatchedRoutes
that exercise the custom-context path by creating an app via newCustomApp() (or
the equivalent factory used in custom request handling) and invoking its Test
helper with both a matched route ("/users") and an unmatched route ("/notfound"
or case/strict variants) while SkipUnmatchedRoutes is true; assert
middlewareCalled semantics match the existing default-app cases so the
short-circuit logic in customRequestHandler (and related code handling
SkipUnmatchedRoutes) is covered and won’t regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f733da5b-bbe5-4e94-bbd5-fc8bf48a7b6b
📒 Files selected for processing (3)
app.gorouter.gorouter_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4411 +/- ##
==========================================
+ Coverage 91.97% 92.00% +0.02%
==========================================
Files 138 138
Lines 13486 13521 +35
==========================================
+ Hits 12404 12440 +36
+ Misses 687 686 -1
Partials 395 395
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@muzzii255 pls check linter errors |
|
@muzzii255 pls add something in the markdown docs for this feature |
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@ReneWerner87 done |
|
@gaby pls check |
gaby
changed the title
gaby
left a comment
gaby
left a comment
There was a problem hiding this comment.
I think we need to do the changes in next(), else we are checking the whole router twice for every requests. That defeats the purpose of this feature.
When |
|
@gaby @ReneWerner87 i tried multiple approach like defering the middlewares when |
|
any approach will add an extra lookup on the tree except rewriting the tree, right now i am using a middleware from the PR #4406 on my codebase, it will be easier for me to just use the fiber's internal tree. |
|
@muzzii255 I think in next() we can check. IsMatched() and if false call c.SendString(not found) |
let me test it |
in next() function this loop executes the middlewares before isMatched() is called, i can try defering the middleware execute by appending the handler to a slice, iterate and execute the middleware after if c.isMatched check |
@gaby Do you mean inside next() do the endpoint scan first (before the main loop runs any middleware), and return ErrNotFound if nothing matches? Because checking IsMatched after the loop is too late — middleware has already executed by then, which defeats the feature. |
|
@gaby Dropped the second scan in SkipUnmatchedRoutes. routeExists -> bool is now firstEndpointIndex -> int — it returns the stack index of the first matching endpoint instead of just whether one exists. next/nextCustom use that index to skip re-matching the endpoints the lookahead already ruled out (if !route.use && indexRoute < firstMatchIndex), so middleware are never skipped and the chain is unchanged. the matched path no longer matches endpoints twice and unmatched/bot requests still 404 before the middleware chain. |
3 participants
Description
Adds a
SkipUnmatchedRoutesconfig option that short-circuits requests to unregistered paths — returning 404 immediately without running through the middleware chain. Useful for cutting unnecessary processing from bots, scanners, and bad URLs.Fixes #4403
Changes introduced
SkipUnmatchedRoutes on app.Config, adds extra lookup costing 150-200ns on matched routes but saving same on unmatched routes
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.