Update RouteChain function #3761
Conversation
|
Caution Review failedThe pull request is closed. 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 chainable RouteChain(path string) Register and a callback-style Route(prefix string, fn func(router Router), name ...string) Router across App, Group, Router, and Register types; updates implementations, tests, and documentation to use RouteChain and to support prefix-based sub-router callbacks and optional naming. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant App as App
participant Group as Group/Sub-router
participant Reg as Registering
rect rgb(235,245,255)
Dev->>App: Route(prefix, fn, name?)
App->>Group: Group(prefix)
alt name provided
App->>Group: Name(name...)
end
App->>Group: Invoke fn(Group)
Note right of Group: fn declares routes/subgroups on Group\n(callback-style grouping)
end
sequenceDiagram
autonumber
participant Dev as Developer
participant App as App
participant Reg as Registering
rect rgb(245,255,235)
Dev->>App: RouteChain(path)
App->>Reg: return Registering{app, group, path}
Dev->>Reg: .Get/.Post/.Head/... (chained)
Note right of Reg: Chained calls build nested route paths\nand register handlers fluently
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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 @ReneWerner87, 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 refactors the routing API to provide clearer separation of concerns. It introduces 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
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3761 +/- ##
==========================================
+ Coverage 91.44% 91.48% +0.04%
==========================================
Files 113 113
Lines 11824 11896 +72
==========================================
+ Hits 10812 10883 +71
Misses 745 745
- Partials 267 268 +1
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-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 effectively clarifies the distinction between RouteChain for method chaining and Route for route grouping, which was a point of confusion. The changes are consistent across the codebase, tests, and documentation. I've identified a couple of areas for improvement: adding nil checks in the new Route methods for robustness and updating external documentation links to be language-neutral. Overall, a great clarification.
|
@hiifong what do you thing about this change? is the name okay for the new feature |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
register.go (1)
114-121: Group context is lost when chaining via RouteChain; carry Group through RegisteringRegistering currently doesn’t retain group, so routes created via grp.RouteChain(...) won’t have route.group set and won’t flip grp.anyRouteDefined. This breaks group name prefixing and related hooks/semantics.
Proposed changes (outside this hunk):
// register.go // Extend Registering to carry group context type Registering struct { app *App group *Group path string } // Ensure All/Add pass the group and set anyRouteDefined func (r *Registering) All(handler Handler, handlers ...Handler) Register { r.app.register([]string{methodUse}, r.path, r.group, append([]Handler{handler}, handlers...)...) if r.group != nil && !r.group.anyRouteDefined { r.group.anyRouteDefined = true } return r } func (r *Registering) Add(methods []string, handler Handler, handlers ...Handler) Register { r.app.register(methods, r.path, r.group, append([]Handler{handler}, handlers...)...) if r.group != nil && !r.group.anyRouteDefined { r.group.anyRouteDefined = true } return r } // Use r.Add for GET to preserve group semantics (was calling app.Add directly) func (r *Registering) Get(handler Handler, handlers ...Handler) Register { return r.Add([]string{MethodGet}, handler, handlers...) }And in group.go (within changed hunk below), initialize Registering with group set.
🧹 Nitpick comments (5)
register.go (1)
7-7: Nit: fix doc grammar“generate by RouteChain()” → “generated by RouteChain()”.
Apply this diff:
-// Register defines all router handle interface generate by RouteChain(). +// Register defines the route registration interface generated by RouteChain().group.go (1)
208-223: Optional: guard against nil callbackAvoid a panic if fn is nil.
Apply this diff:
func (grp *Group) Route(prefix string, fn func(router Router), name ...string) Router { + if fn == nil { + return grp + } // Create new group group := grp.Group(prefix) if len(name) > 0 { group.Name(name[0]) } // Define routes fn(group) return group }app.go (1)
918-933: Route callback helper: LGTM; consider nil guard for parity with GroupMatches Router interface. Optional defensive nil check mirrors the suggestion in Group.
Apply this diff:
func (app *App) Route(prefix string, fn func(router Router), name ...string) Router { + if fn == nil { + return app.Group(prefix) + } // Create new group group := app.Group(prefix) if len(name) > 0 { group.Name(name[0]) } // Define routes fn(group) return group }docs/whats_new.md (2)
331-336: Clarify the diff to show Route(prefix, fn, name...) was added, not removed.The prose says Route remains available, but the diff block alone still reads like a removal. Show both additions to avoid misinterpretation.
Apply this diff to the code block:
- Route(path string) Register + RouteChain(path string) Register + Route(prefix string, fn func(router Router), name ...string) Router
1678-1687: Remove trailing semicolon in Go example.Go examples should avoid explicit semicolons for style consistency.
Apply this diff:
- }); + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app.go(1 hunks)app_test.go(4 hunks)docs/api/app.md(5 hunks)docs/whats_new.md(4 hunks)group.go(1 hunks)middleware/cache/cache_test.go(2 hunks)register.go(4 hunks)router.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
group.gomiddleware/cache/cache_test.goapp.goregister.gorouter.goapp_test.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.mddocs/api/app.md
🧠 Learnings (1)
📚 Learning: 2024-12-15T19:56:45.935Z
Learnt from: ckoch786
PR: gofiber/fiber#3230
File: docs/whats_new.md:944-951
Timestamp: 2024-12-15T19:56:45.935Z
Learning: Detailed usage examples and explanations for new methods like `RemoveRoute` and `RemoveRouteByName` are documented in `docs/api/app.md`, so it's unnecessary to duplicate them in `docs/whats_new.md`.
Applied to files:
docs/whats_new.mddocs/api/app.md
🧬 Code graph analysis (4)
group.go (2)
register.go (2)
Register(8-23)Registering(29-33)router.go (2)
Route(43-62)Router(18-40)
app.go (3)
router.go (3)
App(366-385)Route(43-62)Router(18-40)register.go (2)
Register(8-23)Registering(29-33)group.go (1)
Group(14-21)
router.go (1)
register.go (1)
Register(8-23)
app_test.go (1)
router.go (2)
Route(43-62)Router(18-40)
⏰ 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 (18)
router.go (1)
36-37: Interface additions LGTM; confirm all implementers updatedApp and Group implement both methods. If there are other Router implementers (mocks, adapters), ensure they now satisfy the interface.
register.go (2)
22-22: API surface matches Router; goodRouteChain on Register aligns with Router interface.
38-47: Docs updated to RouteChain examples look goodExamples reflect the new helper correctly.
middleware/cache/cache_test.go (2)
878-878: Test update to RouteChain is correctChained Get/Head registration via RouteChain matches the new API.
907-907: LGTM: RouteChain usageThe replacement maintains behavior for HEAD→GET flow.
app.go (1)
908-916: RouteChain on App: LGTMConstructor and scoping are correct; normalization happens in register().
docs/whats_new.md (3)
342-359: LGTM: concise RouteChain example.Clear demonstration of chained registration with optional segments.
363-364: LGTM: Cross-links to API anchors.Anchors align with the new sections in docs/api/app.md.
1658-1659: LGTM: Migration text correctly differentiates RouteChain vs Route.Good clarification that Route preserves v2’s prefix behavior.
app_test.go (4)
1363-1379: LGTM: RouteChain basic chaining test covers all verbs.Exercising all methods on the same path validates the new Register API well.
1390-1399: LGTM: Nested RouteChain("/v1") coverage.Confirms nested chains work and handlers are reachable.
1400-1409: LGTM: Deeply nested RouteChain works as expected.Trace and Get validations on v1/v2/v3 are solid.
1411-1432: LGTM: New Route(prefix, fn, name...) test proves preservation of v2-style grouping.Nice touch verifying named routes resolve to expected paths.
docs/api/app.md (5)
141-149: LGTM: RouteChain signature is explicit and matches implementation.Returning Register is clear and consistent with Router’s surface.
169-171: LGTM: Register interface includes RouteChain for nested chaining.API surface is cohesive.
187-206: LGTM: Examples illustrate All + verb handlers and nested RouteChain.Good parity with Express-style route stacks.
210-215: LGTM: Multi-method usage reads cleanly.Matches the intended chainable ergonomics.
220-234: LGTM: Route(prefix, fn, name...) docs complete the picture.Clear signature and example; aligns with Router interface and tests.
There was a problem hiding this comment.
Pull Request Overview
This PR clarifies documentation for the v2→v3 router migration by properly distinguishing between the new RouteChain helper and the existing Route function. The changes correct the documentation to show that Route was not removed but instead serves a different purpose, while RouteChain is the new helper for method chaining.
- Renames
Routemethod toRouteChainin the Router interface and implementations - Restores the original
Routemethod for prefix encapsulation functionality - Updates documentation and examples to clearly differentiate the two helpers
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| router.go | Updates Router interface to include both RouteChain and Route methods |
| register.go | Renames Route to RouteChain in Register interface and implementation |
| group.go | Implements both RouteChain and Route methods with distinct purposes |
| app.go | Implements both RouteChain and Route methods in the main App struct |
| docs/whats_new.md | Clarifies migration documentation to show RouteChain as new addition |
| docs/api/app.md | Updates API documentation with separate sections for RouteChain and Route |
| app_test.go | Updates tests and adds separate test coverage for both methods |
| middleware/cache/cache_test.go | Updates test calls to use RouteChain |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
group.go (2)
202-203: Fix misleading commentThis creates a Registering, not a “group”.
- // Create new group + // Create new registering helper
211-222: Guard against nil callback in RouteA nil fn will panic at call site; fail fast with a clear message.
func (grp *Group) Route(prefix string, fn func(router Router), name ...string) Router { + if fn == nil { + panic("route handler 'fn' cannot be nil") + } // Create new group group := grp.Group(prefix) if len(name) > 0 { group.Name(name[0]) } // Define routes fn(group) return group }docs/whats_new.md (2)
341-359: Example is clear; small style nitGo doesn’t need a trailing semicolon at the end of the chain; drop it for consistency.
- }) - .Post(func(c fiber.Ctx) error { + }). + Post(func(c fiber.Ctx) error { // Create user return c.JSON(fiber.Map{"message": "User created"}) - }) + })
1657-1659: RouteChain migration example: drop trailing semicolonMinor style consistency with Go examples.
- }); + })Also applies to: 1678-1687
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.go(1 hunks)docs/api/app.md(5 hunks)docs/whats_new.md(4 hunks)group.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
group.goapp.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/app.mddocs/whats_new.md
🧠 Learnings (1)
📚 Learning: 2024-12-15T19:56:45.935Z
Learnt from: ckoch786
PR: gofiber/fiber#3230
File: docs/whats_new.md:944-951
Timestamp: 2024-12-15T19:56:45.935Z
Learning: Detailed usage examples and explanations for new methods like `RemoveRoute` and `RemoveRouteByName` are documented in `docs/api/app.md`, so it's unnecessary to duplicate them in `docs/whats_new.md`.
Applied to files:
docs/api/app.mddocs/whats_new.md
🧬 Code graph analysis (2)
group.go (2)
register.go (2)
Register(8-23)Registering(29-33)router.go (2)
Route(43-62)Router(18-40)
app.go (3)
router.go (3)
App(366-385)Route(43-62)Router(18-40)register.go (2)
Register(8-23)Registering(29-33)group.go (1)
Group(14-21)
⏰ 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 (9)
app.go (2)
908-916: LGTM: App.RouteChain implementationStraightforward constructor; aligns with Register API.
918-932: Add nil check for Route callbackPrevent accidental nil dereference; match Group.Route handling.
func (app *App) Route(prefix string, fn func(router Router), name ...string) Router { + if fn == nil { + panic("route handler 'fn' cannot be nil") + } // Create new group group := app.Group(prefix) if len(name) > 0 { group.Name(name[0]) } // Define routes fn(group) return group }docs/whats_new.md (3)
331-336: Good clarification and link fixClear diff shows RouteChain addition without implying Route removal; Express link uses /en.
363-364: Nice cross-links to API referenceAnchors match the headings in docs/api/app.md.
If you want to double-check anchors resolve post-build, run the docs site locally and test these links.
367-373: Middleware alignment note reads well; Express link OKNo changes needed.
docs/api/app.md (4)
141-149: RouteChain section looks good; Express link correctedAPI and signature align with code; link uses /en.
169-169: Register interface updated consistentlyIncludes RouteChain method; matches register.go.
187-215: Examples read well; consistent with chainable APINo issues; nice emphasis on “route-specific middleware”.
220-233: Route (prefix) docs: concise and accurateSignature and naming example match implementation and group name semantics.
ReneWerner87
changed the title
It's perfect. Thank you so much for your work. |
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai What are your thoughts on the name "RouteChain"? Should we keep it, if not provide suggestions (and why). |
|
Tip For best results, initiate chat on the files or code changes. @ReneWerner87 I think "RouteChain" is actually a well-chosen name for several reasons: Why "RouteChain" works well:1. Semantic Clarity: The name clearly communicates the method's primary purpose - enabling method chaining for route registration. When developers see 2. Clear Differentiation: It effectively distinguishes itself from the new 3. Descriptive of Behavior: The "Chain" suffix accurately describes what this method enables - chaining multiple HTTP method registrations in a fluent interface style. 4. Consistency with Intent: Looking at the usage patterns in the PR: // Old: app.Route("/test").Get(...).Head(...).Post(...)
// New: app.RouteChain("/test").Get(...).Head(...).Post(...)The name aligns perfectly with the chaining behavior. Alternative considerations:If we were to consider alternatives, options might include:
However, these alternatives have drawbacks:
Recommendation:Keep "RouteChain" - it's intuitive, descriptive, and clearly differentiates from the callback-based The naming follows good API design principles: it's self-documenting and reduces cognitive load for developers transitioning from v2 to v3. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
register.go (2)
7-8: Doc grammar nit in Register interfaceTighten wording for clarity.
-// Register defines all router handle interface generate by RouteChain(). +// Register defines the route handler interface generated by RouteChain().Also applies to: 22-23
39-47: Example consistency: prefer Ctx within package examplesInside package fiber examples, using Ctx instead of fiber.Ctx avoids confusion for readers copying snippets into this package.
-// app.RouteChain("/").All(func(c fiber.Ctx) error { +// app.RouteChain("/").All(func(c Ctx) error { // return c.Next() // }) -// app.RouteChain("/api").All(func(c fiber.Ctx) error { +// app.RouteChain("/api").All(func(c Ctx) error { // return c.Next() // }) -// app.RouteChain("/api").All(handler, func(c fiber.Ctx) error { +// app.RouteChain("/api").All(handler, func(c Ctx) error { // return c.Next() // })app.go (1)
908-916: App.RouteChain builder (no group) — LGTMCorrect to omit group here; callers get a path-scoped Register at app level.
After merge, remember to update the migrator as noted in PR comments so v2→v3 guidance reflects RouteChain/Route changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.go(1 hunks)group.go(1 hunks)register.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
app.goregister.gogroup.go
🧬 Code graph analysis (3)
app.go (3)
router.go (3)
App(366-385)Route(43-62)Router(18-40)register.go (2)
Register(8-23)Registering(29-34)group.go (1)
Group(14-21)
register.go (3)
app.go (2)
App(68-110)Handler(40-40)group.go (1)
Group(14-21)constants.go (1)
MethodGet(5-5)
group.go (2)
register.go (2)
Register(8-23)Registering(29-34)router.go (2)
Route(43-62)Router(18-40)
⏰ 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). (4)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
group.go (2)
199-206: Group is now propagated in RouteChain — LGTMThe Registering is correctly scoped with group: grp, so chained routes inherit group name/hooks. This addresses prior feedback.
Run to double-check end-to-end propagation and detect any lingering old-style Route(...) chaining:
#!/bin/bash set -euo pipefail echo "== Ensure Group.RouteChain sets group ==" rg -nP 'func\s+\(grp\s+\*Group\)\s+RouteChain' group.go -n -C2 rg -nP 'Registering\{[^}]*group:\s*grp' group.go -n -C2 echo echo "== Ensure Registering struct has group and all app.register calls use r.group ==" rg -nP 'type\s+Registering\s+struct\s*\{' register.go -n -C2 total=$(rg -n 'app\.register\(' register.go | wc -l | tr -d ' ') with_group=$(rg -nP 'app\.register\([^)]*r\.group' register.go | wc -l | tr -d ' ') echo "app.register calls: $total, with r.group: $with_group" test "$total" = "$with_group" || { echo "Mismatch: some registrations may not pass r.group"; exit 1; } echo echo "== Find any old chaining via .Route(\"...\").Get/... ==" rg -nP '\.Route\(".*"\)\s*\.(All|Get|Head|Post|Put|Delete|Connect|Options|Trace|Patch)\(' -g '!**/vendor/**' -S || true echo echo "Done."
208-225: Route(prefix, fn, name...) adds nil-check and preserves naming — LGTMMatches App.Route semantics and prevents accidental nil call.
register.go (1)
30-33: Group context propagation through Registering — LGTMgroup *Group on Registering and forwarding r.group in All/Add/RouteChain are correct; ensures hooks/names/prefixing apply consistently to chained routes.
If more Registering helpers are added later, ensure they also pass r.group into app.register.
Also applies to: 51-53, 110-112, 114-121
app.go (1)
918-935: Route(prefix, fn, name...) with nil-check — LGTMMatches Group.Route; panic message is consistent; optional name applied before fn.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
* docs: clarify route helper diff * Adjust Route docs to drop v2 mention and update Express links * Harden route helpers and keep group context * Clarify RouteChain diff in what's new guide * docs: show RouteChain signature in migration notes * Add coverage for Route helpers * Clarify route chaining release wording
Summary
RouteChainhelper without implyingRoutewas removedRouteChainandRouteAPI reference anchors for additional detailTesting
GOVERSION=go1.25.1 make audit(fails: govulncheck forbidden to reach vuln.go.dev)GOVERSION=go1.25.1 make generateGOVERSION=go1.25.1 make betteralignGOVERSION=go1.25.1 make modernizeGOVERSION=go1.25.1 make formatGOVERSION=go1.25.1 make testhttps://chatgpt.com/codex/tasks/task_e_68d24f0a24c083269b2ba0c789065c70