fix: handle http.FS() and import naming conflicts in filesystem migration#268
Conversation
CybotTM
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @CybotTM, 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 addresses two critical bugs in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![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 addresses two bugs in the filesystem middleware migration: incorrect handling of http.FS() and import naming conflicts with user packages named static. The introduction of new regexes and logic to handle these cases, along with comprehensive new tests, significantly improves the robustness of the migration script. I have a few suggestions to further improve the robustness by handling an edge case with dot imports and ensuring consistency in the import path replacement logic.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds dynamic import-alias detection and transforms for migrating v2 filesystem middleware to v3 static middleware, converting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/internal/migrations/v3/filesystem_middleware.go`:
- Around line 21-37: The static import conflict detection in staticMiddlewarePkg
currently treats any import path ending with /static as conflicting (using
reStaticImport), which incorrectly flags Fiber's own middleware (e.g.,
github.com/gofiber/fiber/.../middleware/static) and can force a new alias;
update staticMiddlewarePkg to ignore Fiber static middleware paths by refining
the regex or adding a path check against known Fiber import prefixes (e.g.,
"github.com/gofiber/fiber") and, when an alias is present in the match (m[1]),
return that alias instead of forcing "staticmw" so existing aliases are reused;
ensure reStaticImport or the post-match logic excludes Fiber middleware and
prefers the detected alias when available.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/filesystem_middleware.gocmd/internal/migrations/v3/filesystem_middleware_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/filesystem_middleware.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/filesystem_middleware_test.go (1)
cmd/internal/migrations/v3/filesystem_middleware.go (1)
MigrateFilesystemMiddleware(42-98)
🔇 Additional comments (2)
cmd/internal/migrations/v3/filesystem_middleware.go (1)
44-75: Migration rewrite flow looks consistent.Alias-aware import swaps combined with the http.Dir/http.FS Root conversions are well-aligned with the new v3 static middleware shape.
cmd/internal/migrations/v3/filesystem_middleware_test.go (1)
121-302: Test coverage for issue#267scenarios is solid.The new cases exercise both the http.FS unwrapping and import conflict aliasing paths, including the combined scenario.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…tion Fixes two bugs in the filesystem middleware migration to static middleware: Bug 1: http.FS() not handled correctly - Added regex to match Root: http.FS(...) patterns - Extract inner value directly since embed.FS implements fs.FS - Prevents invalid code like os.DirFS(http.FS(embedFS)) Bug 2: Import naming conflict not handled - Detect existing imports ending with /static - Exclude Fiber's own middleware paths from conflict detection - Handle dot imports (import . "path/to/static") - Alias middleware import to staticmw when conflict exists - Use quotes in import replacement for safety Added tests: - Test_MigrateFilesystemMiddleware_WithHTTPFS - Test_MigrateFilesystemMiddleware_WithStaticPackageConflict - Test_MigrateFilesystemMiddleware_WithAliasedStaticImport - Test_MigrateFilesystemMiddleware_FiberStaticNotConflict - Test_MigrateFilesystemMiddleware_IssueExample Fixes gofiber#267
CybotTM
force-pushed
the
fix/filesystem-migration-bugs
branch
from
a3d75e2 to
aefabda
Compare
|
All review feedback has been addressed and commits have been squashed into a single commit. Ready for review ✅ Changes:
All 8 filesystem migration tests pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/internal/migrations/v3/filesystem_middleware.go`:
- Around line 16-18: The current regexes reFilesystemRootHTTPDir and
reFilesystemRootHTTPFS fail on nested parentheses (they use [^)]+); replace the
regex-based extraction for http.Dir(...) and http.FS(...) with a
parenthesis-matching extractor: find the index of "http.Dir(" or "http.FS(" in
the input, then iterate characters from the opening '(' keeping a depth counter
to locate the matching closing ')' (handling nested parens) and return the full
inner argument; update the code paths that previously used
reFilesystemRootHTTPDir/reFilesystemRootHTTPFS to call this new helper (keep
reFilesystemRoot for simpler non-paren cases) and ensure trimming/whitespace
handling matches existing behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/filesystem_middleware.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/filesystem_middleware.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
thx for the PR @CybotTM |
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
## Summary Upgrades the web framework from Fiber v2 to Fiber v3. ## Changes - Update imports from `fiber/v2` to `fiber/v3` - Change handler signatures from `*fiber.Ctx` to `fiber.Ctx` (interface, not pointer) - Update body parser: `c.BodyParser()` → `c.Bind().Body()` - Migrate `filesystem` middleware to `static` middleware - Use helmet from `fiber/v3/middleware/helmet` (now bundled) - Alias internal static package as `webstatic` to avoid naming conflict ## Breaking Changes Handled | v2 | v3 | |----|----| | `*fiber.Ctx` | `fiber.Ctx` (interface) | | `c.BodyParser(&body)` | `c.Bind().Body(&body)` | | `filesystem.New()` | `static.New()` | | `helmet/v2` | `fiber/v3/middleware/helmet` | ## Test Plan - [x] All existing tests pass - [x] Build succeeds - [ ] Manual testing of password change flow - [ ] Manual testing of password reset flow (if enabled) ## Related - [Fiber v3 Migration Guide](https://docs.gofiber.io/next/whats_new) - Found and reported bugs in `fiber migrate` tool: gofiber/cli#267 - Submitted fix PR: gofiber/cli#268
2 participants
Summary
Fixes two bugs in the filesystem middleware migration to static middleware:
Bug 1:
http.FS()not handled correctly - WhenRoot: http.FS(embedFS)was used, it fell through to the genericRoot:pattern and got wrapped withos.DirFS(), producing invalid code likeos.DirFS(http.FS(embedFS)). Nowhttp.FS()is recognized and the inner value is extracted directly sinceembed.FSalready implementsfs.FS.Bug 2: Import naming conflict - When users had their own package named "static" (e.g.,
internal/web/static), the migration would add the middleware import without aliasing, causing a naming conflict. Now the migration detects existing "static" imports and aliases the middleware import tostaticmwwhen needed.Fixes #267
Test plan
Test_MigrateFilesystemMiddleware_WithHTTPFS- verifieshttp.FS(embedFS)producesFS: embedFSTest_MigrateFilesystemMiddleware_WithStaticPackageConflict- verifies middleware import is aliased when there's a conflictingstaticpackageTest_MigrateFilesystemMiddleware_WithAliasedStaticImport- verifies no aliasing needed when user's import is already aliasedTest_MigrateFilesystemMiddleware_IssueExample- tests the exact scenario from the issueSummary by CodeRabbit
Bug Fixes
Tests