🐛 Fix routing with mount and static #3454
Conversation
|
""" WalkthroughThis change removes route position and route count tracking from the application core and its mounting logic. Associated atomic operations and tests relying on these fields are deleted. Route grouping and static registration are refactored, and a new test is added to verify static file serving works when mounting sub-apps. Changes
Sequence Diagram(s)sequenceDiagram
participant MainApp
participant SubApp
participant StaticServer
participant Client
Client->>MainApp: GET /subApp/public/test.txt
MainApp->>SubApp: Forward request
SubApp->>StaticServer: Serve static file /public/test.txt
StaticServer-->>SubApp: File content or error
SubApp-->>MainApp: Response
MainApp-->>Client: Response
Assessment against linked issues
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
| // merge global tree routes in current tree stack | ||
| tsMap[treePart] = uniqueRouteStack(append(tsMap[treePart], tsMap[""]...)) | ||
| // if it's a global route, assign to every bucket | ||
| if routePath == "" { |
There was a problem hiding this comment.
we can also combine this IF statement, I only split it up for readability
![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 (4)
mount.go (1)
177-178: Remove/format leftover commented code to satisfy linter and avoid noiseThe commented‐out declaration
//var routePos uint32has no functional purpose after the route-position logic was removed and is currently triggeringgocritic(commentFormatting: put a space between // and comment text).
Either delete the line or keep it properly formatted with a leading space and a short explanation so it does not look like forgotten code.- //var routePos uint32 + // var routePos uint32 // kept for historical reference – delete once route-position tracking is fully removed🧰 Tools
🪛 GitHub Check: lint
[failure] 177-177:
commentFormatting: put a space between//and comment text (gocritic)router.go (2)
49-57: Comment style & deadposfield create maintenance debt
- Comment starts with triple
###which violates Go comment conventions and will be flagged by linters (gocritic,golint).- The
posfield is now unused since sorting and assignment were deleted. Keeping an unused 8-byte field in everyRouteinstance wastes memory (~0.8 MB per 100 k routes) and confuses future readers.Consider deleting the field completely (plus all mentions) or at least marking it
// deprecateduntil it is removed.- // ### important: always keep in sync with the copy method "app.copyRoute" and all creations of Route struct### - pos uint32 // Position in stack -> important for the sort of the matched routes + // NOTE: keep this struct in sync with app.copyRoute and every direct Route initialisation. + // Deprecated – `pos` is no longer used after removal of route-sorting logic. + // pos uint32 // deprecated🧰 Tools
🪛 GitHub Check: lint
[failure] 51-51:
fieldposis unused (unused)
451-466: Duplicate-route merge only checks the last entry – edge cases slip through
addRoutemerges handlers when the immediately preceding route matches (app.stack[m][l-1]).
If the same route/path was registered earlier but not in the last position, duplication is silently allowed and the stack grows unnecessarily.Consider using a map keyed by
(path,use,mount)per method or scanning the slice in reverse until the first non-middleware mismatch:- l := len(app.stack[m]) - if l > 0 && app.stack[m][l-1].Path == route.Path && route.use == app.stack[m][l-1].use && !route.mount && !app.stack[m][l-1].mount { + for i := len(app.stack[m]) - 1; i >= 0; i-- { + prev := app.stack[m][i] + if prev.Path == route.Path && prev.use == route.use && !prev.mount && !route.mount { + prev.Handlers = append(prev.Handlers, route.Handlers...) + return + } + }This prevents silent duplication and keeps the stack compact.
router_test.go (1)
514-538: Strengthen new integration test with content assertion
Test_Router_Mount_n_Staticonly checks the status code.
Adding a quick body assertion ensures we’re not getting a 200 with an unexpected body (e.g., the catch-all middleware).resp, err := app.Test(httptest.NewRequest(MethodGet, "/static/style.css", nil)) utils.AssertEqual(t, nil, err, "app.Test(req)") utils.AssertEqual(t, 200, resp.StatusCode, "Status code") + +body, err := io.ReadAll(resp.Body) +utils.AssertEqual(t, nil, err, "read body") +utils.AssertEqual(t, true, strings.Contains(string(body), "color"), "static file contents")While at it, you might also assert that
/mount/testreturns 200 to cover the mounted sub-app.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app.go(0 hunks)mount.go(1 hunks)mount_test.go(0 hunks)router.go(6 hunks)router_test.go(3 hunks)
💤 Files with no reviewable changes (2)
- app.go
- mount_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
router.go (2)
helpers.go (1)
MethodGet(819-819)app.go (1)
App(88-118)
router_test.go (3)
app.go (2)
New(497-590)Static(401-444)helpers.go (2)
StatusNotFound(890-890)MethodGet(819-819)utils/assertions.go (1)
AssertEqual(19-68)
🪛 GitHub Check: lint
mount.go
[failure] 177-177:
commentFormatting: put a space between // and comment text (gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Analyse
- GitHub Check: Build (1.20.x, macos-latest)
- GitHub Check: Build (1.19.x, macos-latest)
- GitHub Check: Build (1.21.x, windows-latest)
- GitHub Check: Build (1.21.x, macos-latest)
- GitHub Check: Build (1.23.x, ubuntu-latest)
- GitHub Check: Build (1.18.x, windows-latest)
- GitHub Check: Build (1.22.x, windows-latest)
- GitHub Check: Build (1.20.x, windows-latest)
- GitHub Check: Build (1.23.x, windows-latest)
- GitHub Check: Build (1.18.x, macos-latest)
- GitHub Check: Compare
🔇 Additional comments (2)
router.go (1)
485-531: Nested loops in newbuildTree()may regress startup time for large route setsThe second pass iterates every discovered 3-char bucket (
treePath) × every route, resulting inO(buckets × routes)complexity (potentially millions of iterations for 10 k+ routes).A lighter approach:
- Compute each route’s bucket once and append directly (
map[bucket][]*Route).- After population, copy global routes into every bucket with a single loop.
This keeps the algorithm
O(routes + buckets + routes_global × buckets).No immediate change required, but worth profiling when apps register many routes.
router_test.go (1)
477-478: Nice! Tests run in parallelAdding
t.Parallel()improves overall test-suite throughput.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues where static routes become inaccessible when mounting sub-apps by refactoring route registration, removing legacy position-based tracking, and adding a dedicated test for the mount+static scenario.
- Extracted
cssDirconstant and updated static tests to use it - Removed
routesCount/posfields and adjustedaddRoute/buildTreeto rely on prefix buckets - Added
Test_Router_Mount_n_Staticto cover combined mount and static routing
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| router_test.go | Introduced cssDir, cleaned up static tests, and added new mount+static test |
| router.go | Removed position tracking (pos, routesCount), refactored addRoute signature and buildTree logic |
| mount_test.go | Removed outdated assertions for routesCount and pos |
| mount.go | Dropped legacy routesCount/pos adjustments in sub-app processing |
| app.go | Removed routesCount field |
Comments suppressed due to low confidence (4)
router_test.go:537
- Consider adding assertions to verify the response body/content of the static file to ensure the correct file is served.
utils.AssertEqual(t, 200, resp.StatusCode, "Status code")
router_test.go:515
- This test sets up a mounted subapp but doesn't verify requests to the mounted route (
/mount/test). Consider adding a request and assertion for the mount endpoint to cover that behavior.
func Test_Router_Mount_n_Static(t *testing.T) {
router.go:450
- The
addRoutemethod signature no longer accepts theisMountedflag, which may cause mounted routes to be treated as non-mounted. Consider restoring the flag or leveragingroute.mountto preserve mount semantics.
func (app *App) addRoute(method string, route *Route) {
router.go:435
- [nitpick] Explicitly assigning
group: nilis redundant since the zero value is alreadynil. You can remove this line to reduce noise.
group: nil,
Fixes #3104 #3442
[Bug]: Static server in sub app does not work #3104
[Bug]: When mounting a subapp with mount, the static route is inaccessible. #3442