From 3696cf264fdf3f8e0057413da8c263ae35e382e6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Nov 2025 14:53:50 +0000 Subject: [PATCH 1/3] Extend filesystem to static migration for NotFoundFile Extend the filesystem middleware migration to handle NotFoundFile: - Comment out NotFoundFile configuration (deprecated) - Add TODO comment referencing NotFoundHandler (fiber.Handler) - Ensure idempotent migration (no duplicate comments on re-run) Changes: - Add regex pattern to detect and comment NotFoundFile - Only apply migration if TODO comment not already present - Add test for NotFoundFile migration - Add test for idempotency to prevent duplicate migrations Migration transforms: NotFoundFile: "index.html", To: // TODO: Migrate to NotFoundHandler (fiber.Handler) - NotFoundFile is deprecated // NotFoundFile: "index.html", --- .../migrations/v3/filesystem_middleware.go | 9 ++ .../v3/filesystem_middleware_test.go | 87 +++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/cmd/internal/migrations/v3/filesystem_middleware.go b/cmd/internal/migrations/v3/filesystem_middleware.go index a8c502f..9e2abb8 100644 --- a/cmd/internal/migrations/v3/filesystem_middleware.go +++ b/cmd/internal/migrations/v3/filesystem_middleware.go @@ -34,6 +34,15 @@ func MigrateFilesystemMiddleware(cmd *cobra.Command, cwd string, _, _ *semver.Ve reIndex := regexp.MustCompile(`Index:\s*([^,\n]+)`) content = reIndex.ReplaceAllString(content, `IndexNames: []string{$1}`) + // Handle NotFoundFile migration - comment it out and add TODO for NotFoundHandler + // Only migrate if not already migrated (check for TODO comment) + if !strings.Contains(content, "TODO: Migrate to NotFoundHandler") { + reNotFoundFile := regexp.MustCompile(`(?m)^(\s*)(NotFoundFile:\s*[^,\n]+)(,?)`) + content = reNotFoundFile.ReplaceAllString(content, + `$1// TODO: Migrate to NotFoundHandler (fiber.Handler) - NotFoundFile is deprecated +$1// $2$3`) + } + return content }) if err != nil { diff --git a/cmd/internal/migrations/v3/filesystem_middleware_test.go b/cmd/internal/migrations/v3/filesystem_middleware_test.go index 9a47c38..5427703 100644 --- a/cmd/internal/migrations/v3/filesystem_middleware_test.go +++ b/cmd/internal/migrations/v3/filesystem_middleware_test.go @@ -40,3 +40,90 @@ func main() { assert.Contains(t, content, `IndexNames: []string{"index.html"}`) assert.Contains(t, buf.String(), "Migrating filesystem middleware") } + +func Test_MigrateFilesystemMiddleware_WithNotFoundFile(t *testing.T) { + t.Parallel() + + dir, err := os.MkdirTemp("", "mfs-notfound") + require.NoError(t, err) + defer func() { require.NoError(t, os.RemoveAll(dir)) }() + + file := writeTempFile(t, dir, `package main +import ( + "github.com/gofiber/fiber/v2/middleware/filesystem" + "net/http" +) +func main() { + app.All("/*", filesystem.New(filesystem.Config{ + Root: http.Dir("./dist"), + NotFoundFile: "index.html", + Index: "index.html", + })) +}`) + + var buf bytes.Buffer + cmd := newCmd(&buf) + require.NoError(t, v3.MigrateFilesystemMiddleware(cmd, dir, nil, nil)) + + content := readFile(t, file) + // Check that NotFoundFile is commented out + assert.Contains(t, content, "// NotFoundFile: \"index.html\"") + // Check that TODO comment is added + assert.Contains(t, content, "// TODO: Migrate to NotFoundHandler (fiber.Handler) - NotFoundFile is deprecated") + // Check that static migration happened + assert.Contains(t, content, `static.New("", static.Config{`) + assert.Contains(t, content, "FS: os.DirFS(\"./dist\")") + assert.Contains(t, content, `IndexNames: []string{"index.html"}`) +} + +func Test_MigrateFilesystemMiddleware_NotFoundFileIdempotent(t *testing.T) { + t.Parallel() + + dir, err := os.MkdirTemp("", "mfs-idempotent") + require.NoError(t, err) + defer func() { require.NoError(t, os.RemoveAll(dir)) }() + + file := writeTempFile(t, dir, `package main +import ( + "github.com/gofiber/fiber/v2/middleware/filesystem" + "net/http" +) +func main() { + app.All("/*", filesystem.New(filesystem.Config{ + Root: http.Dir("./dist"), + NotFoundFile: "index.html", + Index: "index.html", + })) +}`) + + var buf bytes.Buffer + cmd := newCmd(&buf) + + // First migration + require.NoError(t, v3.MigrateFilesystemMiddleware(cmd, dir, nil, nil)) + firstContent := readFile(t, file) + + // Second migration - should be idempotent + buf.Reset() + require.NoError(t, v3.MigrateFilesystemMiddleware(cmd, dir, nil, nil)) + secondContent := readFile(t, file) + + // Content should be exactly the same after second migration + assert.Equal(t, firstContent, secondContent, "Migration should be idempotent") + + // Verify the TODO comment is only present once + assert.Equal(t, 1, countOccurrences(secondContent, "TODO: Migrate to NotFoundHandler")) + // Verify the NotFoundFile comment is only present once + assert.Equal(t, 1, countOccurrences(secondContent, "// NotFoundFile:")) +} + +// Helper function to count occurrences of a substring +func countOccurrences(str, substr string) int { + count := 0 + for i := 0; i <= len(str)-len(substr); i++ { + if str[i:i+len(substr)] == substr { + count++ + } + } + return count +} From 123ef46229b1778a36c73a992e6f43679e99b363 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Nov 2025 16:00:13 +0000 Subject: [PATCH 2/3] Fix test assertion for formatted output Remove extra whitespace from assertion to match goimports formatted output --- cmd/internal/migrations/v3/filesystem_middleware_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/internal/migrations/v3/filesystem_middleware_test.go b/cmd/internal/migrations/v3/filesystem_middleware_test.go index 5427703..f5ddd26 100644 --- a/cmd/internal/migrations/v3/filesystem_middleware_test.go +++ b/cmd/internal/migrations/v3/filesystem_middleware_test.go @@ -72,7 +72,7 @@ func main() { assert.Contains(t, content, "// TODO: Migrate to NotFoundHandler (fiber.Handler) - NotFoundFile is deprecated") // Check that static migration happened assert.Contains(t, content, `static.New("", static.Config{`) - assert.Contains(t, content, "FS: os.DirFS(\"./dist\")") + assert.Contains(t, content, `FS: os.DirFS("./dist")`) assert.Contains(t, content, `IndexNames: []string{"index.html"}`) } From 51bdb84dfdf5d33f95fd7959f88e19fd058c57ea Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Nov 2025 16:03:12 +0000 Subject: [PATCH 3/3] Refactor: move regex compilation to package level and use strings.Count Performance improvements: - Move all regex compilation from closure to package-level variables - Avoids repeated compilation overhead when processing multiple files - Compiled regexes are now reused across all file migrations Code quality improvements: - Replace custom countOccurrences helper with strings.Count - More idiomatic and efficient standard library usage - Remove unnecessary helper function This addresses code review feedback for better performance and idiomatic Go. --- .../migrations/v3/filesystem_middleware.go | 23 +++++++++++-------- .../v3/filesystem_middleware_test.go | 16 +++---------- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/cmd/internal/migrations/v3/filesystem_middleware.go b/cmd/internal/migrations/v3/filesystem_middleware.go index 9e2abb8..79e7373 100644 --- a/cmd/internal/migrations/v3/filesystem_middleware.go +++ b/cmd/internal/migrations/v3/filesystem_middleware.go @@ -11,6 +11,14 @@ import ( "github.com/gofiber/cli/cmd/internal" ) +var ( + reFilesystemNew = regexp.MustCompile(`filesystem\.New\s*\(`) + reFilesystemRootHTTP = regexp.MustCompile(`Root:\s*http.Dir\(([^)]+)\)`) + reFilesystemRoot = regexp.MustCompile(`Root:\s*([^,\n]+)`) + reFilesystemIndex = regexp.MustCompile(`Index:\s*([^,\n]+)`) + reFilesystemNotFoundFile = regexp.MustCompile(`(?m)^(\s*)(NotFoundFile:\s*[^,\n]+)(,?)`) +) + func MigrateFilesystemMiddleware(cmd *cobra.Command, cwd string, _, _ *semver.Version) error { changed, err := internal.ChangeFileContent(cwd, func(content string) string { content = strings.ReplaceAll(content, @@ -20,25 +28,20 @@ func MigrateFilesystemMiddleware(cmd *cobra.Command, cwd string, _, _ *semver.Ve "github.com/gofiber/fiber/v3/middleware/filesystem", "github.com/gofiber/fiber/v3/middleware/static") - reNew := regexp.MustCompile(`filesystem\.New\s*\(`) - content = reNew.ReplaceAllString(content, `static.New("", `) + content = reFilesystemNew.ReplaceAllString(content, `static.New("", `) content = strings.ReplaceAll(content, "filesystem.Config{", "static.Config{") - reRootHTTP := regexp.MustCompile(`Root:\s*http.Dir\(([^)]+)\)`) - content = reRootHTTP.ReplaceAllString(content, `FS: os.DirFS($1)`) + content = reFilesystemRootHTTP.ReplaceAllString(content, `FS: os.DirFS($1)`) - reRoot := regexp.MustCompile(`Root:\s*([^,\n]+)`) - content = reRoot.ReplaceAllString(content, `FS: os.DirFS($1)`) + content = reFilesystemRoot.ReplaceAllString(content, `FS: os.DirFS($1)`) - reIndex := regexp.MustCompile(`Index:\s*([^,\n]+)`) - content = reIndex.ReplaceAllString(content, `IndexNames: []string{$1}`) + content = reFilesystemIndex.ReplaceAllString(content, `IndexNames: []string{$1}`) // Handle NotFoundFile migration - comment it out and add TODO for NotFoundHandler // Only migrate if not already migrated (check for TODO comment) if !strings.Contains(content, "TODO: Migrate to NotFoundHandler") { - reNotFoundFile := regexp.MustCompile(`(?m)^(\s*)(NotFoundFile:\s*[^,\n]+)(,?)`) - content = reNotFoundFile.ReplaceAllString(content, + content = reFilesystemNotFoundFile.ReplaceAllString(content, `$1// TODO: Migrate to NotFoundHandler (fiber.Handler) - NotFoundFile is deprecated $1// $2$3`) } diff --git a/cmd/internal/migrations/v3/filesystem_middleware_test.go b/cmd/internal/migrations/v3/filesystem_middleware_test.go index f5ddd26..6b649db 100644 --- a/cmd/internal/migrations/v3/filesystem_middleware_test.go +++ b/cmd/internal/migrations/v3/filesystem_middleware_test.go @@ -3,6 +3,7 @@ package v3_test import ( "bytes" "os" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -112,18 +113,7 @@ func main() { assert.Equal(t, firstContent, secondContent, "Migration should be idempotent") // Verify the TODO comment is only present once - assert.Equal(t, 1, countOccurrences(secondContent, "TODO: Migrate to NotFoundHandler")) + assert.Equal(t, 1, strings.Count(secondContent, "TODO: Migrate to NotFoundHandler")) // Verify the NotFoundFile comment is only present once - assert.Equal(t, 1, countOccurrences(secondContent, "// NotFoundFile:")) -} - -// Helper function to count occurrences of a substring -func countOccurrences(str, substr string) int { - count := 0 - for i := 0; i <= len(str)-len(substr); i++ { - if str[i:i+len(substr)] == substr { - count++ - } - } - return count + assert.Equal(t, 1, strings.Count(secondContent, "// NotFoundFile:")) }