🐛 bug: preserve mounted sub-app regex handler during mount prefixing#4308
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThread mounted apps' ChangesSub-app regex handler preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mount_test.go (1)
38-44: ⚡ Quick winAdd a positive-control assertion to harden this regression test.
Right now, both assertions are negative (
StatusNotFound), so the test can still pass if the route is broken and never matches. Add oneStatusOKcheck forALLOW(direct and/or mounted) before the lowercase checks.Suggested test hardening
func Test_App_Mount_PreservesSubAppRegexHandler(t *testing.T) { t.Parallel() @@ parent.Use("/mounted", sub) + resp, err := sub.Test(httptest.NewRequest(MethodGet, "/resource/ALLOW", http.NoBody)) + require.NoError(t, err) + require.Equal(t, StatusOK, resp.StatusCode) + + resp, err = parent.Test(httptest.NewRequest(MethodGet, "/mounted/resource/ALLOW", http.NoBody)) + require.NoError(t, err) + require.Equal(t, StatusOK, resp.StatusCode) + resp, err := sub.Test(httptest.NewRequest(MethodGet, "/resource/allow", http.NoBody)) require.NoError(t, err) require.Equal(t, StatusNotFound, resp.StatusCode)🤖 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 `@mount_test.go` around lines 38 - 44, Add a positive-control assertion that verifies the ALLOW route actually matches before the lowercase negative checks: call sub.Test and parent.Test with MethodGet for "/resource/allow" (and/or "/mounted/resource/allow") and assert StatusOK before the existing require.Equal(t, StatusNotFound, ...) assertions that check the lowercase variants; update the references to resp, err, httptest.NewRequest, MethodGet, StatusOK and StatusNotFound around the sub.Test and parent.Test calls so the test fails if the route is broken rather than silently passing only on 404s.
🤖 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 `@mount_test.go`:
- Around line 38-44: Add a positive-control assertion that verifies the ALLOW
route actually matches before the lowercase negative checks: call sub.Test and
parent.Test with MethodGet for "/resource/allow" (and/or
"/mounted/resource/allow") and assert StatusOK before the existing
require.Equal(t, StatusNotFound, ...) assertions that check the lowercase
variants; update the references to resp, err, httptest.NewRequest, MethodGet,
StatusOK and StatusNotFound around the sub.Test and parent.Test calls so the
test fails if the route is broken rather than silently passing only on 404s.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8afe111b-bdb9-4e6e-8406-9d0046e3bb54
📒 Files selected for processing (3)
mount.gomount_test.gorouter.go
There was a problem hiding this comment.
Code Review
This pull request updates the route mounting logic to ensure that a sub-app's RegexHandler and customConstraints are preserved during route flattening. Changes include modifying the addPrefixToRoute signature and adding a unit test to verify the preservation of sub-app configurations. Review feedback highlights a regression where parent constraints are replaced instead of merged, potentially breaking prefix parsing. There is also a suggestion to address nested mounting scenarios where handlers might still be lost due to how routes are cloned.
There was a problem hiding this comment.
Pull request overview
This PR fixes a mount-time route reparse issue where cloned routes from a mounted sub-app could be reparsed using the parent app’s RegexHandler, potentially changing (and weakening) regex constraint semantics. The update makes mount prefixing preserve the mounted sub-app’s regex configuration when rebuilding the route parser for the prefixed path.
Changes:
- Updated
addPrefixToRouteto accept an explicitregexHandlerplus variadiccustomConstraints, and use those when reparsing the prefixed route. - Updated mount route expansion to pass the mounted sub-app’s
RegexHandlerandcustomConstraintswhen prefixing/cloning routes. - Added a regression test to ensure mounted routes don’t inherit the parent’s regex compilation semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
router.go |
Extends addPrefixToRoute to reparse with an explicitly provided regex handler + constraints. |
mount.go |
Passes mounted app’s regex handler and constraints into prefixing during route expansion. |
mount_test.go |
Adds regression test for preserving sub-app regex behavior after mounting. |
…ive test assertions Agent-Logs-Url: https://github.com/gofiber/fiber/sessions/2cb83563-ddc3-4552-ad83-2498f3c41c1d Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4308 +/- ##
==========================================
- Coverage 91.33% 91.29% -0.05%
==========================================
Files 132 132
Lines 13087 13089 +2
==========================================
- Hits 11953 11949 -4
- Misses 716 721 +5
- Partials 418 419 +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:
|
Motivation
RegexHandlersemantics are preserved for routes cloned from mounted sub-apps so a sub-app's constraints are not silently recompiled under a different engine.Description
addPrefixToRoutesignature to accept an explicitregexHandlerand variadiccustomConstraintsand use them when reparsing the prefixed path.config.RegexHandlerandcustomConstraintswhen cloning and prefixing sub-app routes so the sub-app's regex configuration is preserved.Test_App_Mount_PreservesSubAppRegexHandlertomount_test.gowhich verifies a mounted route retains the sub-app's regex behavior when the parent uses a different handler.