🔥 feat: Add Support for Removing Routes #3230
Conversation
ckoch786
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe pull request introduces significant enhancements to route management in the Fiber web framework. The changes focus on providing methods to remove routes dynamically, including Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
ckoch786
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
router.go (2)
310-310: Add a space after '//' in the comment for proper formattingThe comment at line 310 should have a space after the
//for better readability and to adhere to Go's style guidelines.Apply this diff to fix the comment formatting:
-//Decrement global handler count +// Decrement global handler count🧰 Tools
🪛 golangci-lint (1.62.2)
310-310: File is not
gofumpt-ed with-extra(gofumpt)
310-310: commentFormatting: put a space between
//and comment text(gocritic)
392-395: Simplify the return statement by directly returning the boolean expressionYou can simplify the code by returning the result of the condition directly, which improves readability.
Apply this diff to simplify the code:
if r.path == pathRaw { return true } return false +// Simplify the return statement +return r.path == pathRaw🧰 Tools
🪛 golangci-lint (1.62.2)
392-392: S1008: should use 'return r.path == pathRaw' instead of 'if r.path == pathRaw { return true }; return false'
(gosimple)
router_test.go (5)
403-403: Remove unnecessary blank line at the start of the blockThere is an extra empty line at the start of the block, which is unnecessary and can be removed for better code readability.
Apply this diff to remove the unnecessary line:
- app.Get("/test", func(c Ctx) error {🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 403-403: empty-lines: extra empty line at the start of a block
(revive)
403-403: unnecessary leading newline
(whitespace)
414-414: Remove unnecessary blank line at the start of the blockThere is an extra empty line at the start of the second route registration block, which can be removed for consistency and readability.
Apply this diff:
- app.Get("/test", func(c Ctx) error {🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 414-414: empty-lines: extra empty line at the start of a block
(revive)
414-414: unnecessary leading newline
(whitespace)
471-471: Remove unnecessary blank line at the start of the blockAn extra empty line is present at the start of the
Test_App_Remove_Route_With_Middlewarefunction block. Removing it improves code consistency.Apply this diff:
- func Test_App_Remove_Route_With_Middleware(t *testing.T) {
479-479: Remove unnecessary blank line inside the functionThere's an extra blank line inside the function which can be removed to improve code readability.
Apply this diff:
- app.Get("/test", func(c Ctx) error {🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 479-479: empty-lines: extra empty line at the start of a block
(revive)
479-479: unnecessary leading newline
(whitespace)
490-490: Remove unnecessary blank line at the start of the blockAnother extra empty line is present before the second route registration. Removing it maintains consistency.
Apply this diff:
- app.Get("/test", func(c Ctx) error {🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 490-490: empty-lines: extra empty line at the start of a block
(revive)
490-490: unnecessary leading newline
(whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
router.go(5 hunks)router_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
router_test.go
[warning] 403-403: empty-lines: extra empty line at the start of a block
(revive)
403-403: unnecessary leading newline
(whitespace)
404-404: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 414-414: empty-lines: extra empty line at the start of a block
(revive)
414-414: unnecessary leading newline
(whitespace)
415-415: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 479-479: empty-lines: extra empty line at the start of a block
(revive)
479-479: unnecessary leading newline
(whitespace)
480-480: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 490-490: empty-lines: extra empty line at the start of a block
(revive)
490-490: unnecessary leading newline
(whitespace)
491-491: File is not gofumpt-ed with -extra
(gofumpt)
router.go
310-310: File is not gofumpt-ed with -extra
(gofumpt)
310-310: commentFormatting: put a space between // and comment text
(gocritic)
390-390: File is not gofumpt-ed with -extra
(gofumpt)
392-392: S1008: should use 'return r.path == pathRaw' instead of 'if r.path == pathRaw { return true }; return false'
(gosimple)
[warning] 408-408: confusing-naming: Method 'removeRoute' differs only by capitalization to method 'RemoveRoute' in the same source file
(revive)
🔇 Additional comments (1)
router_test.go (1)
403-416: Verify duplicate route registration in 'Test_App_Remove_Route'
In Test_App_Remove_Route, the route /test is registered twice with different handlers. This may lead to unintended behavior, as the second registration may overwrite the first. Please confirm if this is intentional or adjust the test accordingly.
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 403-403: empty-lines: extra empty line at the start of a block
(revive)
403-403: unnecessary leading newline
(whitespace)
404-404: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 414-414: empty-lines: extra empty line at the start of a block
(revive)
414-414: unnecessary leading newline
(whitespace)
415-415: File is not gofumpt-ed with -extra
(gofumpt)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
router.go (3)
310-310: Add space after // in commentFix the comment formatting to follow Go conventions.
- //Decrement global handler count + // Decrement global handler count🧰 Tools
🪛 golangci-lint (1.62.2)
310-310: commentFormatting: put a space between
//and comment text(gocritic)
310-310: File is not
gofumpt-ed with-extra(gofumpt)
392-399: Simplify the return statementThe if-return block can be simplified to a direct return statement.
func (app *App) routeExists(method string, pathRaw string) bool { return slices.ContainsFunc(app.stack[app.methodInt(method)], func(r *Route) bool { - if r.path == pathRaw { - return true - } - return false + return r.path == pathRaw }) }🧰 Tools
🪛 golangci-lint (1.62.2)
392-392: File is not
gofumpt-ed with-extra(gofumpt)
394-394: S1008: should use 'return r.path == pathRaw' instead of 'if r.path == pathRaw { return true }; return false'
(gosimple)
402-403: Remove or address TODO comments in documentationThe TODO comments in the documentation raise important questions about the intended usage of
RemoveRoute. These should be either:
- Removed if they're implementation notes
- Converted to proper documentation if they're usage constraints
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router.go(5 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
router.go
310-310: commentFormatting: put a space between // and comment text
(gocritic)
310-310: File is not gofumpt-ed with -extra
(gofumpt)
392-392: File is not gofumpt-ed with -extra
(gofumpt)
394-394: S1008: should use 'return r.path == pathRaw' instead of 'if r.path == pathRaw { return true }; return false'
(gosimple)
[warning] 410-410: confusing-naming: Method 'removeRoute' differs only by capitalization to method 'RemoveRoute' in the same source file
(revive)
🔇 Additional comments (4)
router.go (4)
11-11: LGTM: Good choice of using the standard slices package
The addition of the slices package is appropriate for the new route management functionality.
307-316: LGTM: Well-implemented duplicate route handling
The implementation is thread-safe and properly manages route and handler counts using atomic operations.
🧰 Tools
🪛 golangci-lint (1.62.2)
310-310: commentFormatting: put a space between // and comment text
(gocritic)
310-310: File is not gofumpt-ed with -extra
(gofumpt)
401-408: Rename internal method to avoid confusion
Having RemoveRoute and removeRoute methods that differ only by capitalization can lead to confusion.
410-431: Verify route removal edge cases
Let's verify the handling of several edge cases in the route removal implementation:
- Removal of routes with mounted apps
- Impact on middleware chains
- Memory management of removed routes
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Route removal implementation is safe and well-handled
The codebase analysis shows that route removal is properly implemented with the following safety measures:
- Mutex locking is used to prevent race conditions during route removal
- Atomic operations handle global counters (handlers and routes)
- Mounted apps are properly tracked via
mountFieldsandhasMountedApps() - Memory management is handled through Go's garbage collection as removed routes are simply unlinked from the stack slice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for mounted routes and middleware chains
ast-grep --pattern 'mount := true' -l go
rg -A 5 'func.*Mount'
# Check for potential memory leaks
rg -g '*.go' 'removeRoute|RemoveRoute' -A 5 -B 5
Length of output: 7958
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 410-410: confusing-naming: Method 'removeRoute' differs only by capitalization to method 'RemoveRoute' in the same source file
(revive)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
router_test.go (1)
457-514: Enhance middleware testing coverageThe test could be improved by:
- Verifying middleware execution order
- Testing multiple middleware functions
- Testing middleware state preservation
func Test_App_Remove_Route_With_Middleware(t *testing.T) { t.Parallel() app := New() + executionOrder := []string{} middleware1 := func(c Ctx) error { + executionOrder = append(executionOrder, "middleware1") return c.Next() } + middleware2 := func(c Ctx) error { + executionOrder = append(executionOrder, "middleware2") + return c.Next() + } - app.Get("/test", func(c Ctx) error { - // ... existing code ... - }, middleware1) + app.Get("/test", func(c Ctx) error { + executionOrder = append(executionOrder, "handler") + return c.SendStatus(http.StatusOK) + }, middleware1, middleware2) // ... rest of the test ... + require.Equal(t, []string{"middleware1", "middleware2", "handler"}, executionOrder)🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 465-465: empty-lines: extra empty line at the start of a block
(revive)
465-465: unnecessary leading newline
(whitespace)
466-466: File is not
gofumpt-ed with-extra(gofumpt)
[warning] 476-476: empty-lines: extra empty line at the start of a block
(revive)
476-476: unnecessary leading newline
(whitespace)
477-477: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
router_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
router_test.go
[warning] 406-406: empty-lines: extra empty line at the start of a block
(revive)
406-406: unnecessary leading newline
(whitespace)
407-407: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 417-417: empty-lines: extra empty line at the start of a block
(revive)
417-417: unnecessary leading newline
(whitespace)
418-418: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 465-465: empty-lines: extra empty line at the start of a block
(revive)
465-465: unnecessary leading newline
(whitespace)
466-466: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 476-476: empty-lines: extra empty line at the start of a block
(revive)
476-476: unnecessary leading newline
(whitespace)
477-477: File is not gofumpt-ed with -extra
(gofumpt)
517-517: parameter testing.TB should be the first or after context.Context
(thelper)
517-517: parameter testing.TB should have name tb
(thelper)
517-517: test helper function should start from tb.Helper()
(thelper)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
router.go (2)
306-318: Clarify TODO comment and document handler count adjustmentThe TODO comment about
RemoveRouteneeds clarification. Additionally, the handler count adjustment logic using bitwise complement (^) is not immediately obvious.
- Remove or clarify the TODO comment
- Add a comment explaining the handler count adjustment:
- //Decrement global handler count + // Decrement global handler count using bitwise complement + // ^uint32(len(handlers)-1) is equivalent to -uint32(len(handlers)-1) atomic.AddUint32(&app.handlersCount, ^uint32(len(handlers)-1))🧰 Tools
🪛 golangci-lint (1.62.2)
311-311: commentFormatting: put a space between
//and comment text(gocritic)
311-311: File is not
gofumpt-ed with-extra(gofumpt)
408-432: Improve error handling for invalid methodsThe method silently skips invalid HTTP methods. Consider providing feedback to callers about invalid methods.
func (app *App) deleteRoute(path string, methods []string) { + var invalidMethods []string for _, method := range methods { method = utils.ToUpper(method) m := app.methodInt(method) if m == -1 { + invalidMethods = append(invalidMethods, method) continue } index := slices.IndexFunc(app.stack[m], func(r *Route) bool { return r.Path == path }) if index == -1 { continue } app.stack[m] = slices.Delete(app.stack[m], index, index+1) } + if len(invalidMethods) > 0 { + app.config.ErrorHandler(nil, fmt.Errorf("invalid HTTP methods: %v", invalidMethods)) + } app.routesRefreshed = true }router_test.go (2)
465-489: Enhance error messages in route handler verificationWhile the verification is thorough, the error message could be more descriptive.
for _, route := range routes { - require.Equal(tb, 1, strings.Count(route.handlers, " ")) + require.Equal(tb, 1, strings.Count(route.handlers, " "), + "route %s %s should have exactly one handler, got: %s", + route.method, route.path, route.handlers) }🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 470-470: empty-lines: extra empty line at the start of a block
(revive)
470-470: unnecessary leading newline
(whitespace)
471-471: File is not
gofumpt-ed with-extra(gofumpt)
386-410: Add test cases for edge scenariosWhile the basic functionality is well-tested, consider adding test cases for:
- Removing non-existent routes
- Removing routes with invalid methods
- Concurrent route operations
Example test case:
func Test_App_Remove_NonExistent_Route(t *testing.T) { t.Parallel() app := New() app.RemoveRoute("/non-existent", "GET") require.Equal(t, uint32(0), app.handlersCount) require.Equal(t, uint32(0), app.routesCount) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
router.go(5 hunks)router_test.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
router_test.go
[warning] 442-442: empty-lines: extra empty line at the end of a block
(revive)
452-452: File is not gofumpt-ed with -extra
(gofumpt)
453-453: unnecessary trailing newline
(whitespace)
[warning] 470-470: empty-lines: extra empty line at the start of a block
(revive)
470-470: unnecessary leading newline
(whitespace)
471-471: File is not gofumpt-ed with -extra
(gofumpt)
router.go
311-311: commentFormatting: put a space between // and comment text
(gocritic)
311-311: File is not gofumpt-ed with -extra
(gofumpt)
393-393: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (3)
router.go (1)
474-477: Well-documented performance considerations
The comment clearly explains the performance implications and design decisions regarding thread safety.
router_test.go (2)
455-463: Well-structured test helper function
The verifyRequest helper is well-implemented with proper use of testing.TB and t.Helper().
412-440: Well-implemented middleware test case
The test thoroughly verifies route removal behavior with middleware, including proper handler count verification.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
router.go (4)
392-396: Improve RemoveRoute documentationThe method documentation should:
- Explain the purpose of the middlewareCount parameter
- Document the behavior when methods array is empty
- Clarify when RebuildTree is required to be called
-// RemoveRoute is used to remove a route from the stack -// You should call RebuildTree after using this to ensure consistency of the tree +// RemoveRoute removes a route from the stack for the specified HTTP methods. +// Parameters: +// - path: The route path to remove +// - middlewareCount: Number of middleware handlers (use 0 for routes without middleware) +// - methods: HTTP methods (GET, POST, etc.). If empty, the route is not removed +// +// Note: Call RebuildTree() after removing routes to ensure routing tree consistency
402-404: Clarify middleware count increment logicThe increment of middlewareCount when it's 0 is not clearly explained and could be confusing.
- if middlewareCount == 0 { - middlewareCount++ - } + // Ensure at least one handler is counted (the route handler itself) + if middlewareCount == 0 { + middlewareCount = 1 // Count the route handler + }
406-407: Simplify atomic decrement operationThe current atomic decrement using bitwise complement is hard to understand.
- atomic.AddUint32(&app.handlersCount, ^uint32(middlewareCount-1)) + atomic.AddUint32(&app.handlersCount, ^uint32(middlewareCount-1)) // Decrement by middlewareCount🧰 Tools
🪛 golangci-lint (1.62.2)
406-406: commentFormatting: put a space between
//and comment text(gocritic)
406-406: File is not
gofumpt-ed with-extra(gofumpt)
416-418: Log invalid HTTP methodsThe method silently skips invalid HTTP methods, which could hide configuration errors.
if m == -1 { + app.config.Logger.Printf("Invalid HTTP method %s when removing route %s", method, path) continue // Skip invalid HTTP methods }router_test.go (2)
403-457: Consider using table-driven testsThe duplicate route registration tests have similar structure and could be refactored into a table-driven test to improve maintainability.
func Test_App_Route_Registration_Prevent_Duplicate(t *testing.T) { tests := []struct { name string middleware []func(Ctx) error expectedHandlers uint32 }{ { name: "without middleware", middleware: nil, expectedHandlers: 2, }, { name: "with middleware", middleware: []func(Ctx) error{func(c Ctx) error { return c.Next() }}, expectedHandlers: 3, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { app := New() registerTreeManipulationRoutes(app, tt.middleware...) registerTreeManipulationRoutes(app) verifyRequest(t, app, "/test", http.StatusOK) require.Equal(t, tt.expectedHandlers, app.handlersCount) }) } }
Line range hint
459-471: Remove unnecessary empty linesThe function has unnecessary empty lines at the start and end.
func registerTreeManipulationRoutes(app *App, middleware ...func(Ctx) error) { - app.Get("/test", func(c Ctx) error { app.Get("/dynamically-defined", func(c Ctx) error { return c.SendStatus(http.StatusOK) }) app.RebuildTree() return c.SendStatus(http.StatusOK) }, middleware...) - }🧰 Tools
🪛 golangci-lint (1.62.2)
470-470: File is not
gofumpt-ed with-extra(gofumpt)
471-471: unnecessary trailing newline
(whitespace)
[warning] 488-488: empty-lines: extra empty line at the start of a block
(revive)
488-488: unnecessary leading newline
(whitespace)
489-489: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
router.go(5 hunks)router_test.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
router.go
386-386: File is not gofumpt-ed with -extra
(gofumpt)
406-406: commentFormatting: put a space between // and comment text
(gocritic)
406-406: File is not gofumpt-ed with -extra
(gofumpt)
router_test.go
[warning] 459-459: empty-lines: extra empty line at the start of a block
(revive)
[warning] 459-459: empty-lines: extra empty line at the end of a block
(revive)
459-459: unnecessary leading newline
(whitespace)
460-460: File is not gofumpt-ed with -extra
(gofumpt)
470-470: File is not gofumpt-ed with -extra
(gofumpt)
471-471: unnecessary trailing newline
(whitespace)
[warning] 488-488: empty-lines: extra empty line at the start of a block
(revive)
488-488: unnecessary leading newline
(whitespace)
489-489: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (4)
router.go (3)
386-390: Case sensitivity must be respected in route existence check
The route path comparison should follow the app's case sensitivity configuration.
🧰 Tools
🪛 golangci-lint (1.62.2)
386-386: File is not gofumpt-ed with -extra
(gofumpt)
306-310: LGTM: Duplicate route handling implementation
The implementation correctly prevents duplicate routes by removing existing routes before adding new ones.
Line range hint 475-479: LGTM: Clear documentation about performance implications
The documentation effectively communicates the performance impact and thread-safety considerations of rebuilding the tree.
router_test.go (1)
473-481: LGTM: Well-implemented test helpers
The helper functions are well-structured with proper test helper marking, clear error messages, and good parameter naming.
Also applies to: 484-507
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
router_test.go (3)
380-380: Remove unnecessary empty lines inregisterTreeManipulationRoutesThere are extra empty lines at the start and end of the
registerTreeManipulationRoutesfunction, as flagged by the static analysis tool.Apply this diff to remove the unnecessary empty lines:
-func registerTreeManipulationRoutes(app *App, middleware ...func(Ctx) error) { +func registerTreeManipulationRoutes(app *App, middleware ...func(Ctx) error) { app.Get("/test", func(c Ctx) error { app.Get("/dynamically-defined", sendStatusOK) app.RebuildTree() return c.SendStatus(http.StatusOK) }, middleware...) - }Also applies to: 389-390
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
407-408: Remove extra empty line inverifyRouteHandlerCountsAn unnecessary empty line is present at the start of the
forloop in theverifyRouteHandlerCountsfunction.Apply this diff to remove the empty line:
func verifyRouteHandlerCounts(tb testing.TB, app *App, expectedRoutesCount int) { tb.Helper() - // this is taken from listen.go's printRoutesMessage app method + // this is taken from listen.go's printRoutesMessage app method var routes []RouteMessage for _, routeStack := range app.stack { for _, route := range routeStack { // ...🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not
gofumpt-ed with-extra(gofumpt)
483-483: Remove unnecessary empty line at the end of functionThere's an extra empty line at the end of
Test_App_Remove_Route_Parameterized, which is unnecessary.Apply this diff to remove the empty line:
func Test_App_Remove_Route_Parameterized(t *testing.T) { t.Parallel() app := New() app.Get("/test/:id", sendStatusOK) app.RemoveRoute("/test/:id", http.MethodGet) verifyThereAreNoRoutes(t, app) -} +}🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 483-483: empty-lines: extra empty line at the end of a block
(revive)
router.go (3)
441-441: Add space after//in commentThe comment
//Decrement global handler countlacks a space after//. Proper comment formatting improves code readability.Apply this diff to fix the comment:
-//Decrement global handler count +// Decrement global handler count atomic.AddUint32(&app.handlersCount, ^uint32(handlerCount-1)) //nolint:gosec // Not a concern🧰 Tools
🪛 golangci-lint (1.62.2)
441-441: File is not
gofumpt-ed with-extra(gofumpt)
441-441: commentFormatting: put a space between
//and comment text(gocritic)
442-442: Correct thenolintdirective formattingThe
nolintdirective should immediately follow//without a leading space to be properly recognized by linters.Apply this diff to correct the formatting:
atomic.AddUint32(&app.handlersCount, ^uint32(handlerCount-1)) // nolint:gosec // Not a concern -atomic.AddUint32(&app.handlersCount, ^uint32(handlerCount-1)) // nolint:gosec // Not a concern +atomic.AddUint32(&app.handlersCount, ^uint32(handlerCount-1)) //nolint:gosec // Not a concern🧰 Tools
🪛 golangci-lint (1.62.2)
442-442: directive
// nolint:gosec // Not a concernshould be written without leading space as//nolint:gosec // Not a concern(nolintlint)
386-400: Addressgofumptformatting issues inrouteExistsThe
routeExistsfunction is not formatted according togofumpt -extraguidelines, as indicated by the static analysis tool. Consistent formatting improves code readability and maintainability.Consider running
gofumpt -extraon the file to apply standard formatting.🧰 Tools
🪛 golangci-lint (1.62.2)
386-386: File is not
gofumpt-ed with-extra(gofumpt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
router.go(4 hunks)router_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
router.go
386-386: File is not gofumpt-ed with -extra
(gofumpt)
441-441: File is not gofumpt-ed with -extra
(gofumpt)
441-441: commentFormatting: put a space between // and comment text
(gocritic)
442-442: directive // nolint:gosec // Not a concern should be written without leading space as //nolint:gosec // Not a concern
(nolintlint)
router_test.go
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
381-381: File is not gofumpt-ed with -extra
(gofumpt)
389-389: File is not gofumpt-ed with -extra
(gofumpt)
390-390: unnecessary trailing newline
(whitespace)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not gofumpt-ed with -extra
(gofumpt)
429-429: test helper function should start from tb.Helper()
(thelper)
[warning] 483-483: empty-lines: extra empty line at the end of a block
(revive)
491-491: File is not gofumpt-ed with -extra
(gofumpt)
492-492: unnecessary trailing newline
(whitespace)
🔇 Additional comments (2)
router_test.go (1)
446-594: Excellent test coverage for route removal scenarios
The added test functions comprehensively cover various scenarios for route removal, including:
- Removing routes by name (
Test_App_Remove_Route_By_Name) - Handling non-existing routes (
Test_App_Remove_Route_By_Name_Non_Existing_Route,Test_App_Remove_Route_Non_Existing_Route) - Removing nested routes (
Test_App_Remove_Route_Nested) - Removing parameterized routes (
Test_App_Remove_Route_Parameterized) - Concurrent route removal and addition (
Test_App_Remove_Route_Concurrent)
These tests enhance the robustness of the route management functionality.
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 483-483: empty-lines: extra empty line at the end of a block
(revive)
491-491: File is not gofumpt-ed with -extra
(gofumpt)
492-492: unnecessary trailing newline
(whitespace)
router.go (1)
11-11: Ensure compatibility with Go version for slices package
The slices package from the standard library is imported, which requires Go 1.21 or higher. Ensure that the project's minimum Go version is updated accordingly to prevent build issues.
Run the following script to check the Go version specified in go.mod:
✅ Verification successful
Go version is compatible for using the slices package
The project uses Go 1.23 which is higher than the required version 1.21 for the slices package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Go version is 1.21 or higher
# Extract the Go version from go.mod
go_version=$(grep '^go ' go.mod | awk '{print $2}')
# Compare versions
required_version="1.21"
if [[ "$(printf '%s\n' "$required_version" "$go_version" | sort -V | head -n1)" == "$required_version" ]]; then
echo "Go version ($go_version) is compatible."
else
echo "Go version ($go_version) is not compatible. Please update to Go 1.21 or higher."
fi
Length of output: 233
|
@ckoch786 In the root of the repo we have a You can run the tests locally and also the lint tests. |
gaby
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
router.go (3)
306-312: Consider adding debug logging for route replacementThe duplicate route handling logic is correct, but it would be helpful to log when a route is being replaced to aid in debugging and monitoring.
// Duplicate Route Handling if app.routeExists(method, pathRaw) { + if app.config.EnableDebugLog { + app.debug("[Router] Replacing existing route: %s %s", method, pathRaw) + } matchPathFunc := func(r *Route) bool { return r.Path == pathRaw } app.deleteRoute([]string{method}, matchPathFunc) }
402-408: Clarify documentation about duplicate preventionThe comment "This only needs to be called to remove a route, route registration prevents duplicate routes" might be confusing. Consider clarifying that while registration prevents duplicates by replacing existing routes, this method is needed for explicit route removal.
-// This only needs to be called to remove a route, route registration prevents duplicate routes. +// This method is used to explicitly remove a route. Note that registering a route with an existing path +// will automatically replace the old route, but this method allows removing routes without replacement.
418-450: Consider documenting atomic operationsThe atomic operations for decrementing counters use bitwise complement, which might not be immediately obvious to all developers.
// Decrement global handler count -atomic.AddUint32(&app.handlersCount, ^uint32(len(route.Handlers)-1)) // nolint:gosec // Not a concern +// Use bitwise complement (^) with AddUint32 to perform atomic decrement +atomic.AddUint32(&app.handlersCount, ^uint32(len(route.Handlers)-1)) // Decrement global route position -atomic.AddUint32(&app.routesCount, ^uint32(0)) +// Decrement route count by 1 using bitwise complement of 0 +atomic.AddUint32(&app.routesCount, ^uint32(0))🧰 Tools
🪛 golangci-lint (1.62.2)
441-441: directive
// nolint:gosec // Not a concernshould be written without leading space as//nolint:gosec // Not a concern(nolintlint)
router_test.go (1)
520-597: Consider adding more concurrent test scenariosWhile the concurrent test is good, consider adding tests for:
- Concurrent removal of different routes
- Concurrent operations on routes with middleware
- Concurrent operations with route groups
Example additional test:
func Test_App_Remove_Route_Concurrent_Different_Routes(t *testing.T) { t.Parallel() app := New() // Add test routes app.Get("/test1", sendStatusOK) app.Get("/test2", sendStatusOK) // Concurrently remove different routes var wg sync.WaitGroup wg.Add(2) go func() { defer wg.Done() app.RemoveRoute("/test1", http.MethodGet) }() go func() { defer wg.Done() app.RemoveRoute("/test2", http.MethodGet) }() wg.Wait() app.RebuildTree() verifyThereAreNoRoutes(t, app) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router.go(4 hunks)router_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
router.go
386-386: File is not gofumpt-ed with -extra
(gofumpt)
441-441: directive // nolint:gosec // Not a concern should be written without leading space as //nolint:gosec // Not a concern
(nolintlint)
router_test.go
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
381-381: File is not gofumpt-ed with -extra
(gofumpt)
389-389: File is not gofumpt-ed with -extra
(gofumpt)
390-390: unnecessary trailing newline
(whitespace)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not gofumpt-ed with -extra
(gofumpt)
[warning] 486-486: empty-lines: extra empty line at the end of a block
(revive)
495-495: File is not gofumpt-ed with -extra
(gofumpt)
496-496: unnecessary trailing newline
(whitespace)
🔇 Additional comments (4)
router.go (2)
386-400: LGTM! Clean implementation of route existence check
The implementation correctly handles case sensitivity and follows the codebase's patterns.
🧰 Tools
🪛 golangci-lint (1.62.2)
386-386: File is not gofumpt-ed with -extra
(gofumpt)
410-416: LGTM! Consistent implementation for name-based route removal
The implementation follows the same pattern as RemoveRoute and reuses the deleteRoute logic effectively.
router_test.go (2)
376-435: LGTM! Well-structured test helpers
The helper functions are well-designed, properly use tb.Helper(), and provide good abstractions for common test operations.
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
381-381: File is not gofumpt-ed with -extra
(gofumpt)
389-389: File is not gofumpt-ed with -extra
(gofumpt)
390-390: unnecessary trailing newline
(whitespace)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not gofumpt-ed with -extra
(gofumpt)
437-518: LGTM! Comprehensive test coverage for route removal
The tests cover all important scenarios including basic removal, removal by name, nested routes, parameterized routes, and edge cases.
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 486-486: empty-lines: extra empty line at the end of a block
(revive)
495-495: File is not gofumpt-ed with -extra
(gofumpt)
496-496: unnecessary trailing newline
(whitespace)
* Add new methods named RemoveRoute and RemoveRouteByName. * Update register method to prevent duplicate routes. * Clean up tests * Update docs * Add Dockerfile
ckoch786
force-pushed
the
feature/3098-allow-removing-registered-route
branch
from
7b0a410 to
6909630
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
Dockerfile (1)
1-19: Add final stage for smaller runtime image.While the builder stage is well-configured, consider adding a final stage for a smaller runtime image.
Add this after the current content:
+# Create small runtime image +FROM scratch + +COPY --from=builder /app/server /server + +EXPOSE 3000 +ENTRYPOINT ["/server"]router_test.go (2)
380-390: Fix formatting issues.The function is well-structured but has unnecessary empty lines.
Apply this diff to fix formatting:
func registerTreeManipulationRoutes(app *App, middleware ...func(Ctx) error) { - app.Get("/test", func(c Ctx) error { app.Get("/dynamically-defined", sendStatusOK) - app.RebuildTree() - return c.SendStatus(http.StatusOK) }, middleware...) - }🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
381-381: File is not
gofumpt-ed with-extra(gofumpt)
389-389: File is not
gofumpt-ed with-extra(gofumpt)
390-390: unnecessary trailing newline
(whitespace)
402-427: Consider simplifying route message collection.While the function works correctly, it could be simplified for better maintainability.
Consider this refactoring:
func verifyRouteHandlerCounts(tb testing.TB, app *App, expectedRoutesCount int) { tb.Helper() - var routes []RouteMessage + routes := make(map[string]RouteMessage) for _, routeStack := range app.stack { for _, route := range routeStack { routeMsg := RouteMessage{ name: route.Name, method: route.Method, path: route.Path, } for _, handler := range route.Handlers { routeMsg.handlers += runtime.FuncForPC(reflect.ValueOf(handler).Pointer()).Name() + " " } - routes = append(routes, routeMsg) + key := route.Method + route.Path + routes[key] = routeMsg } } - for _, route := range routes { + for _, route := range routes { require.Equal(tb, expectedRoutesCount, strings.Count(route.handlers, " ")) } }🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not
gofumpt-ed with-extra(gofumpt)
docs/whats_new.md (1)
944-951: Add usage examples for route removal features.The documentation would benefit from examples showing:
- Basic route removal
- Route removal by name
- Rebuilding the tree after removal
Consider adding examples like:
// Remove route by path app.RemoveRoute("/api/users", MethodGet) app.RebuildTree() // Remove route by name app.Get("/api/users", handler).Name("users") app.RemoveRouteByName("users", MethodGet) app.RebuildTree()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile(1 hunks)docs/api/app.md(1 hunks)docs/whats_new.md(1 hunks)router.go(4 hunks)router_test.go(2 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/api/app.md
765-765: Expected: 1; Actual: 2
Multiple consecutive blank lines
(MD012, no-multiple-blanks)
775-775: null
Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 golangci-lint (1.62.2)
router.go
386-386: File is not gofumpt-ed with -extra
(gofumpt)
441-441: directive // nolint:gosec // Not a concern should be written without leading space as //nolint:gosec // Not a concern
(nolintlint)
router_test.go
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
381-381: File is not gofumpt-ed with -extra
(gofumpt)
389-389: File is not gofumpt-ed with -extra
(gofumpt)
390-390: unnecessary trailing newline
(whitespace)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not gofumpt-ed with -extra
(gofumpt)
460-460: commentedOutCode: may want to remove commented-out code
(gocritic)
460-460: File is not gofumpt-ed with -extra
(gofumpt)
460-460: commentFormatting: put a space between // and comment text
(gocritic)
471-471: File is not gofumpt-ed with -extra
(gofumpt)
471-471: commentedOutCode: may want to remove commented-out code
(gocritic)
471-471: commentFormatting: put a space between // and comment text
(gocritic)
479-479: SA4006: this value of err is never used
(staticcheck)
479-479: ineffectual assignment to err
(ineffassign)
[warning] 525-525: empty-lines: extra empty line at the end of a block
(revive)
534-534: File is not gofumpt-ed with -extra
(gofumpt)
535-535: unnecessary trailing newline
(whitespace)
🔇 Additional comments (8)
router.go (4)
386-400: LGTM! Well-implemented route existence check.
The implementation correctly handles case sensitivity and efficiently searches for existing routes.
🧰 Tools
🪛 golangci-lint (1.62.2)
386-386: File is not gofumpt-ed with -extra
(gofumpt)
402-416: LGTM! Clean and well-documented route removal methods.
Both methods are well-designed with clear documentation about the RebuildTree requirement.
492-492: LGTM! Excellent documentation update.
The documentation clearly explains the performance implications with benchmark results.
418-450: Verify concurrent route removal safety.
While the implementation handles thread safety for route removal, we should verify the behavior when removing routes during active requests.
✅ Verification successful
Let me gather more information about the route handling and tree structure to better understand the concurrent safety.
Let me check one more aspect about the request handling to see if there's any read locking during actual request processing.
Based on the gathered information, I can now provide the final response:
Route removal implementation is concurrently safe
The implementation correctly handles concurrent safety through multiple mechanisms:
- Mutex protection during route modifications (add/delete/rebuild)
- Atomic counter updates for handlers and routes
- Tree structure is rebuilt atomically with mutex protection
- Request handling uses a separate tree stack that's only updated after modifications are complete
The routesRefreshed flag ensures the router tree is rebuilt before processing new requests, preventing any race conditions during active requests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for request handling code that might be affected by concurrent route removal
rg -A 5 "func.*next.*" router.go
Length of output: 472
Script:
#!/bin/bash
# Check how treeStack is populated and if there's any synchronization
rg -A 10 "treeStack.*=" router.go
# Look for tree building/refreshing logic
ast-grep --pattern 'func (app *App) buildTree() {
$$$
}'
# Check if there's any read locking during request handling
rg -B 2 -A 5 "mutex\.(RLock|Lock)" router.go
Length of output: 1112
Script:
#!/bin/bash
# Check the main request handling logic
rg -B 5 -A 10 "func.*Handler|func.*handle" router.go
# Look for any tree access synchronization in the context
ast-grep --pattern 'func (c *$_) $_($$$) {
$$$
app.treeStack
$$$
}'
Length of output: 992
🧰 Tools
🪛 golangci-lint (1.62.2)
441-441: directive // nolint:gosec // Not a concern should be written without leading space as //nolint:gosec // Not a concern
(nolintlint)
router_test.go (4)
376-378: LGTM! Well-structured helper function.
The function is concise, follows single responsibility principle, and is reused effectively across test cases.
392-400: LGTM! Well-structured test helper.
The function follows testing best practices:
- Includes
tb.Helper()call - Clear error messages
- Proper status code verification
429-435: Add tb.Helper() call to test helper function.
The function is missing a tb.Helper() call which is required for proper test failure reporting.
487-607: LGTM! Comprehensive test coverage.
The test suite effectively covers various route removal scenarios:
- Route removal by name
- Non-existing routes
- Nested routes
- Parameterized routes
- Concurrent operations
- Duplicate route prevention
The concurrent test is particularly valuable for verifying thread safety.
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 525-525: empty-lines: extra empty line at the end of a block
(revive)
534-534: File is not gofumpt-ed with -extra
(gofumpt)
535-535: unnecessary trailing newline
(whitespace)
docs/api/app.md
Outdated
| app.Get("/api/feature-a", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-a") | ||
| }) | ||
|
|
||
| app.RebuildTree() | ||
| return c.SendStatus(http.StatusOK) | ||
| }) |
There was a problem hiding this comment.
Fix inconsistencies in example code.
The example code has several issues:
- Uses
Ctxinstead offiber.Ctx - Uses undefined
MethodGetconstant - Uses undefined
http.StatusOKconstant
Apply this diff to fix the issues:
- app.Get("/api/feature-a", func(c Ctx) error {
- app.RemoveRoute("/api/feature", MethodGet)
+ app.Get("/api/feature-a", func(c fiber.Ctx) error {
+ app.RemoveRoute("/api/feature", fiber.MethodGet)
app.RebuildTree()
// Redefine route
- app.Get("/api/feature", func(c Ctx) error {
+ app.Get("/api/feature", func(c fiber.Ctx) error {
return c.SendString("Testing feature-a")
})
app.RebuildTree()
- return c.SendStatus(http.StatusOK)
+ return c.SendStatus(fiber.StatusOK)
})Also applies to: 802-812
There was a problem hiding this comment.
@gaby, please apply the suggested diff above to fix the inconsistencies in the example code.
There was a problem hiding this comment.
@gaby @coderabbitai the router_test.go file is already part of the fiber package so the Ctx is already the fiber Ctx. Also MethodGet is also in the fiber package and that is what is being used. In addition MethodGet and StatusOK are both defined in the fiber package source file constants.go. So I did update my tests to use the fiber constant StatusOK instead of the http.StatusOK constant.
When I made the update it importing github.com/gofiber/fiber after doing so and running make tidy and then make test I get the following errors:
github.com/gofiber/fiber/v3 [github.com/gofiber/fiber/v3.test]
./router_test.go:378:9: c.SendStatus(http.StatusOK) (no value) used as value
./router_test.go:382:19: cannot use func(c *fiber.Ctx) error {…} (value of type func(c *"github.com/gofiber/fiber".Ctx) error) as Handler value in argument to app.Get
./router_test.go:383:35: cannot use sendStatusOK (value of type func(c *"github.com/gofiber/fiber".Ctx) error) as Handler value in argument to app.Get
./router_test.go:387:10: c.SendStatus(http.StatusOK) (no value) used as value
./router_test.go:450:28: cannot use func(c *fiber.Ctx) error {…} (value of type func(c *"github.com/gofiber/fiber".Ctx) error) as Handler value in argument to app.Get
./router_test.go:454:27: cannot use func(c *fiber.Ctx) error {…} (value of type func(c *"github.com/gofiber/fiber".Ctx) error) as Handler value in argument to app.Get
./router_test.go:455:11: c.SendString("Testing feature-a") (no value) used as value
./router_test.go:459:10: c.SendStatus(fiber.StatusOK) (no value) used as value
./router_test.go:461:28: cannot use func(c *fiber.Ctx) error {…} (value of type func(c *"github.com/gofiber/fiber".Ctx) error) as Handler value in argument to app.Get
./router_test.go:465:27: cannot use func(c *fiber.Ctx) error {…} (value of type func(c *"github.com/gofiber/fiber".Ctx) error) as Handler value in argument to app.Get
./router_test.go:465:27: too many errors
FAIL github.com/gofiber/fiber/v3 [build failed]
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
docs/api/app.md
Outdated
| ```go title="Signature" | ||
| func (app *App) RemoveRoute() | ||
| ``` |
There was a problem hiding this comment.
Update method signature to include parameters.
The method signature should be:
-func (app *App) RemoveRoute()
+func (app *App) RemoveRoute(path string, methods ...string)There was a problem hiding this comment.
Also remove the extra space at the beginning of the 2nd sentence.
There was a problem hiding this comment.
@gaby, understood. The extra space at the beginning of the second sentence should be removed.
There was a problem hiding this comment.
@gaby, understood. Here are the updated method signatures:
func (app *App) RemoveRoute(path string, methods ...string)func (app *App) RemoveRouteByName(name string, methods ...string)I've also removed the extra space at the beginning of the second sentence:
"You must call the RebuildTree() method after the remove to ensure the route is removed."
router_test.go
Outdated
| func Test_App_Romove_Route_A_B_Feature_Testing(t *testing.T) { | ||
| t.Parallel() | ||
| app := New() | ||
|
|
||
| app.Get("/api/feature-a", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-a") | ||
| }) | ||
|
|
||
| //app.RebuildTree() | ||
| return c.SendStatus(http.StatusOK) | ||
| }) | ||
| app.Get("/api/feature-b", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-b") | ||
| }) | ||
|
|
||
| //app.RebuildTree() | ||
| return c.SendStatus(http.StatusOK) | ||
| }) | ||
|
|
||
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | ||
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature-a", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusNotFound, resp.StatusCode, "Status code") | ||
| require.Equal(t, http.StatusOK, resp, "Status Code") | ||
|
|
||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/test", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | ||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil)) | ||
| require.Equal(t, "Testing feature-a", resp, "Response Message") | ||
|
|
||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | ||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "api/feature-b", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | ||
| require.Equal(t, "Testing feature-b", resp, "Response Message") | ||
| } |
There was a problem hiding this comment.
Fix multiple issues in test function.
- Function name has a typo ("Romove" should be "Remove")
- Contains commented out code
- Ineffectual assignment to
err - Test logic could be simplified using helper functions
Apply these changes:
-func Test_App_Romove_Route_A_B_Feature_Testing(t *testing.T) {
+func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) {
t.Parallel()
app := New()
app.Get("/api/feature-a", func(c Ctx) error {
app.RemoveRoute("/api/feature", MethodGet)
app.RebuildTree()
- // Redefine route
app.Get("/api/feature", func(c Ctx) error {
return c.SendString("Testing feature-a")
})
- //app.RebuildTree()
return c.SendStatus(http.StatusOK)
})
app.Get("/api/feature-b", func(c Ctx) error {
app.RemoveRoute("/api/feature", MethodGet)
app.RebuildTree()
- // Redefine route
app.Get("/api/feature", func(c Ctx) error {
return c.SendString("Testing feature-b")
})
- //app.RebuildTree()
return c.SendStatus(http.StatusOK)
})
- resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature-a", nil))
- require.NoError(t, err, "app.Test(req)")
- require.Equal(t, http.StatusOK, resp, "Status Code")
+ verifyRequest(t, app, "/api/feature-a", http.StatusOK)
- resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil))
- require.Equal(t, "Testing feature-a", resp, "Response Message")
+ resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil))
+ require.NoError(t, err)
+ body, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Equal(t, "Testing feature-a", string(body), "Response Message")
- resp, err = app.Test(httptest.NewRequest(MethodGet, "api/feature-b", nil))
- require.NoError(t, err, "app.Test(req)")
- require.Equal(t, "Testing feature-b", resp, "Response Message")
+ verifyRequest(t, app, "/api/feature-b", http.StatusOK)
+
+ resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil))
+ require.NoError(t, err)
+ body, err = io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Equal(t, "Testing feature-b", string(body), "Response Message")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Test_App_Romove_Route_A_B_Feature_Testing(t *testing.T) { | |
| t.Parallel() | |
| app := New() | |
| app.Get("/api/feature-a", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| // Redefine route | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-a") | |
| }) | |
| //app.RebuildTree() | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| app.Get("/api/feature-b", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| // Redefine route | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-b") | |
| }) | |
| //app.RebuildTree() | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | |
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature-a", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| require.Equal(t, http.StatusNotFound, resp.StatusCode, "Status code") | |
| require.Equal(t, http.StatusOK, resp, "Status Code") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/test", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil)) | |
| require.Equal(t, "Testing feature-a", resp, "Response Message") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "api/feature-b", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | |
| require.Equal(t, "Testing feature-b", resp, "Response Message") | |
| } | |
| func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) { | |
| t.Parallel() | |
| app := New() | |
| app.Get("/api/feature-a", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-a") | |
| }) | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| app.Get("/api/feature-b", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-b") | |
| }) | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| verifyRequest(t, app, "/api/feature-a", http.StatusOK) | |
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil)) | |
| require.NoError(t, err) | |
| body, err := io.ReadAll(resp.Body) | |
| require.NoError(t, err) | |
| require.Equal(t, "Testing feature-a", string(body), "Response Message") | |
| verifyRequest(t, app, "/api/feature-b", http.StatusOK) | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil)) | |
| require.NoError(t, err) | |
| body, err = io.ReadAll(resp.Body) | |
| require.NoError(t, err) | |
| require.Equal(t, "Testing feature-b", string(body), "Response Message") | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
460-460: commentedOutCode: may want to remove commented-out code
(gocritic)
460-460: File is not gofumpt-ed with -extra
(gofumpt)
460-460: commentFormatting: put a space between // and comment text
(gocritic)
471-471: File is not gofumpt-ed with -extra
(gofumpt)
471-471: commentedOutCode: may want to remove commented-out code
(gocritic)
471-471: commentFormatting: put a space between // and comment text
(gocritic)
479-479: SA4006: this value of err is never used
(staticcheck)
479-479: ineffectual assignment to err
(ineffassign)
|
@gaby I cleaned up my branch and just pushed the docs changes. |
* Add new methods named RemoveRoute and RemoveRouteByName. * Update register method to prevent duplicate routes. * Clean up tests * Update docs * Add Dockerfile
ckoch786
force-pushed
the
feature/3098-allow-removing-registered-route
branch
from
82142d1 to
8df76b9
Compare
Dockerfile
Outdated
| @@ -0,0 +1,25 @@ | |||
| # Usage: | |||
There was a problem hiding this comment.
I am stuck in Windows currently so I created the Dockerfile so that I could get all the dependencies for running make to install properly. I have more updates that make it more useful that I will be pushing soon that utilizes the build caching and I updated the comments to include how to utilize volumes and bind mounts with run to keep preserve all the downloaded packages between runs and to allow the linter to update the local code that is outside the docker container.
However if you do not want me to add it I can just remove it.
What is nice about using docker for running the make actions is that if it is used it gives you an isolated container from the rest of your machine so that you can get consistent results. Also using docker makes it easier for others to work with the Makefile as all they need to do is install Docker and the rest will just work.
There was a problem hiding this comment.
You cna remove it, this could be added to the docs though. Or one of the other fiber repos
There was a problem hiding this comment.
Also if you choose to keep it I can create helper scripts for running docker build and docker run to make it easier to use .
docs/api/app.md
Outdated
| ```go title="Signature" | ||
| func (app *App) RemoveRoute() | ||
| ``` |
docs/api/app.md
Outdated
| ```go title="Signature" | ||
| func (app *App) RemoveRoute() | ||
| ``` |
There was a problem hiding this comment.
Also remove the extra space at the beginning of the 2nd sentence.
docs/api/app.md
Outdated
| app.Get("/api/feature-a", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-a") | ||
| }) | ||
|
|
||
| app.RebuildTree() | ||
| return c.SendStatus(http.StatusOK) | ||
| }) |
router_test.go
Outdated
| @@ -369,31 +373,267 @@ func Test_Router_NotFound_HTML_Inject(t *testing.T) { | |||
| require.Equal(t, "Cannot DELETE /does/not/exist<script>alert('foo');</script>", string(c.Response.Body())) | |||
| } | |||
|
|
|||
| func sendStatusOK(c Ctx) error { | |||
There was a problem hiding this comment.
Remove this helper, do this call in the test
|
@ckoch786 Test and lint workflows are failing. You can test them locally using the |
* Add new methods named RemoveRoute and RemoveRouteByName. * Update register method to prevent duplicate routes. * Clean up tests * Update docs * Add Dockerfile
ckoch786
force-pushed
the
feature/3098-allow-removing-registered-route
branch
from
6b4e2d5 to
cf59d6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
Dockerfile (2)
1-20: Consider moving Dockerfile to documentation.Based on the previous discussion, this Dockerfile was created for local development on Windows. Since it's a development tool, it would be more appropriate to move it to the documentation or another Fiber repository.
45-46: Fix formatting in RUN command.Add whitespace after the RUN command for better readability.
-RUN --mount=type=cache,target=/go/pkg/mod/cache - go mod download -x +RUN --mount=type=cache,target=/go/pkg/mod/cache \ + go mod download -x🧰 Tools
🪛 Hadolint (2.12.0)
[error] 45-45: missing whitespace
(DL1000)
docs/api/app.md (1)
768-768: Fix spacing in documentation.Remove the extra space at the beginning of the sentence.
-This method removes a route by path. You must call the `RebuildTree()` method after the remove in to ensure the route is removed. +This method removes a route by path. You must call the `RebuildTree()` method after the remove in to ensure the route is removed.router_test.go (3)
376-390: Fix formatting issues.The code has unnecessary empty lines and formatting issues.
Apply this diff to fix the formatting:
func sendStatusOK(c Ctx) error { return c.SendStatus(http.StatusOK) } - func registerTreeManipulationRoutes(app *App, middleware ...func(Ctx) error) { - app.Get("/test", func(c Ctx) error { app.Get("/dynamically-defined", sendStatusOK) - app.RebuildTree() - return c.SendStatus(http.StatusOK) }, middleware...) - }🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
381-381: File is not
gofumpt-ed with-extra(gofumpt)
389-389: File is not
gofumpt-ed with-extra(gofumpt)
390-390: unnecessary trailing newline
(whitespace)
525-535: Fix formatting issues.The test has unnecessary empty lines.
Apply this diff to fix the formatting:
func Test_App_Remove_Route_Parameterized(t *testing.T) { t.Parallel() app := New() app.Get("/test/:id", sendStatusOK) verifyRequest(t, app, "/test/:id", http.StatusOK) app.RemoveRoute("/test/:id", MethodGet) verifyThereAreNoRoutes(t, app) - }🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 525-525: empty-lines: extra empty line at the end of a block
(revive)
534-534: File is not
gofumpt-ed with-extra(gofumpt)
535-535: unnecessary trailing newline
(whitespace)
559-581: Consider enhancing concurrent test verification.While the test verifies basic concurrent operations, consider adding:
- Verification of intermediate states
- Checking for race conditions using the race detector
- Verifying handler counts remain consistent
Apply this diff to enhance the test:
func Test_App_Remove_Route_Concurrent(t *testing.T) { t.Parallel() app := New() // Add test route app.Get("/test", sendStatusOK) + initialHandlersCount := app.handlersCount // Concurrently remove and add routes var wg sync.WaitGroup for i := 0; i < 10; i++ { wg.Add(1) go func() { defer wg.Done() app.RemoveRoute("/test", MethodGet) app.Get("/test", sendStatusOK) }() } wg.Wait() // Verify final state app.RebuildTree() verifyRequest(t, app, "/test", http.StatusOK) + require.Equal(t, initialHandlersCount, app.handlersCount, "Handlers count should remain consistent") + verifyRouteHandlerCounts(t, app, 1) }To run with race detector:
go test -race ./... -run=Test_App_Remove_Route_Concurrent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile(1 hunks)docs/api/app.md(1 hunks)docs/whats_new.md(1 hunks)router.go(4 hunks)router_test.go(2 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/api/app.md
765-765: Expected: 1; Actual: 2
Multiple consecutive blank lines
(MD012, no-multiple-blanks)
775-775: null
Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 golangci-lint (1.62.2)
router.go
386-386: File is not gofumpt-ed with -extra
(gofumpt)
441-441: directive // nolint:gosec // Not a concern should be written without leading space as //nolint:gosec // Not a concern
(nolintlint)
router_test.go
[warning] 380-380: empty-lines: extra empty line at the end of a block
(revive)
[warning] 380-380: empty-lines: extra empty line at the start of a block
(revive)
380-380: unnecessary leading newline
(whitespace)
381-381: File is not gofumpt-ed with -extra
(gofumpt)
389-389: File is not gofumpt-ed with -extra
(gofumpt)
390-390: unnecessary trailing newline
(whitespace)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not gofumpt-ed with -extra
(gofumpt)
479-479: SA4006: this value of err is never used
(staticcheck)
479-479: ineffectual assignment to err
(ineffassign)
[warning] 525-525: empty-lines: extra empty line at the end of a block
(revive)
534-534: File is not gofumpt-ed with -extra
(gofumpt)
535-535: unnecessary trailing newline
(whitespace)
🪛 Hadolint (2.12.0)
Dockerfile
[error] 45-45: missing whitespace
(DL1000)
🔇 Additional comments (11)
router.go (4)
386-400: LGTM: Case-sensitive route existence check.
The implementation correctly handles case sensitivity based on app configuration.
🧰 Tools
🪛 golangci-lint (1.62.2)
386-386: File is not gofumpt-ed with -extra
(gofumpt)
402-408: LGTM: Clean implementation of RemoveRoute.
The method is well-documented and follows the single responsibility principle.
410-416: LGTM: Clean implementation of RemoveRouteByName.
The method is well-documented and follows the same pattern as RemoveRoute.
418-450: Consider adding error handling for concurrent modifications.
The deleteRoute method handles concurrent access with mutex, but consider adding error handling for cases where the route is modified between finding and deleting it.
Consider adding error handling:
func (app *App) deleteRoute(methods []string, matchFunc func(r *Route) bool) {
app.mutex.Lock()
defer app.mutex.Unlock()
+ var deletedCount int
for _, method := range methods {
method = utils.ToUpper(method)
m := app.methodInt(method)
if m == -1 {
continue
}
index := slices.IndexFunc(app.stack[m], matchFunc)
if index == -1 {
continue
}
route := app.stack[m][index]
atomic.AddUint32(&app.handlersCount, ^uint32(len(route.Handlers)-1))
atomic.AddUint32(&app.routesCount, ^uint32(0))
app.stack[m] = slices.Delete(app.stack[m], index, index+1)
+ deletedCount++
}
+ if deletedCount == 0 {
+ return fmt.Errorf("no routes were deleted")
+ }
app.routesRefreshed = true
+ return nil
}🧰 Tools
🪛 golangci-lint (1.62.2)
441-441: directive // nolint:gosec // Not a concern should be written without leading space as //nolint:gosec // Not a concern
(nolintlint)
docs/api/app.md (2)
770-777:
Update method signatures in documentation.
The method signatures should include their parameters.
-func (app *App) RemoveRoute()
+func (app *App) RemoveRoute(path string, methods ...string)
-func (app *App) RemoveRouteByName()
+func (app *App) RemoveRouteByName(name string, methods ...string)Likely invalid or redundant comment.
🧰 Tools
🪛 Markdownlint (0.37.0)
775-775: null
Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
791-812:
Fix example code to use proper package references.
The example code should use proper package references and constants.
- app.Get("/api/feature-a", func(c Ctx) error {
- app.RemoveRoute("/api/feature", MethodGet)
+ app.Get("/api/feature-a", func(c fiber.Ctx) error {
+ app.RemoveRoute("/api/feature", fiber.MethodGet)
app.RebuildTree()
// Redefine route
- app.Get("/api/feature", func(c Ctx) error {
+ app.Get("/api/feature", func(c fiber.Ctx) error {
return c.SendString("Testing feature-a")
})
app.RebuildTree()
- return c.SendStatus(http.StatusOK)
+ return c.SendStatus(fiber.StatusOK)
})Likely invalid or redundant comment.
router_test.go (5)
487-498: LGTM!
The test is well-structured and effectively verifies route removal by name functionality.
500-508: LGTM!
The test effectively verifies that removing a non-existent route doesn't affect the application state.
510-523: LGTM!
The test effectively verifies the removal of nested routes within groups.
537-557: LGTM!
Both tests effectively verify:
- Basic route removal functionality
- Handling of non-existent route removal
402-427: 🛠️ Refactor suggestion
Add tb.Helper() call to verifyRouteHandlerCounts.
The function is a test helper but is missing the tb.Helper() call, which would improve test failure reporting by pointing to the correct line in the calling test.
Apply this diff to add the helper call:
func verifyRouteHandlerCounts(tb testing.TB, app *App, expectedRoutesCount int) {
+ tb.Helper()
var routes []RouteMessage
for _, routeStack := range app.stack {
for _, route := range routeStack {Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 407-407: empty-lines: extra empty line at the start of a block
(revive)
407-407: unnecessary leading newline
(whitespace)
408-408: File is not gofumpt-ed with -extra
(gofumpt)
router_test.go
Outdated
| func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) { | ||
| t.Parallel() | ||
| app := New() | ||
|
|
||
| app.Get("/api/feature-a", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-a") | ||
| }) | ||
|
|
||
| app.RebuildTree() | ||
| return c.SendStatus(http.StatusOK) | ||
| }) | ||
| app.Get("/api/feature-b", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-b") | ||
| }) | ||
|
|
||
| app.RebuildTree() | ||
| return c.SendStatus(http.StatusOK) | ||
| }) | ||
|
|
||
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | ||
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature-a", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusNotFound, resp.StatusCode, "Status code") | ||
| require.Equal(t, http.StatusOK, resp, "Status Code") | ||
|
|
||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/test", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | ||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil)) | ||
| require.Equal(t, "Testing feature-a", resp, "Response Message") | ||
|
|
||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | ||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "api/feature-b", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | ||
| require.Equal(t, "Testing feature-b", resp, "Response Message") | ||
| } |
There was a problem hiding this comment.
Fix response handling and cleanup code.
The test has several issues:
- Incorrect response handling - comparing response object with string
- Commented code that should be removed
- Missing leading slash in path
Apply this diff to fix the issues:
func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) {
t.Parallel()
app := New()
app.Get("/api/feature-a", func(c Ctx) error {
app.RemoveRoute("/api/feature", MethodGet)
app.RebuildTree()
- // Redefine route
app.Get("/api/feature", func(c Ctx) error {
return c.SendString("Testing feature-a")
})
- app.RebuildTree()
return c.SendStatus(http.StatusOK)
})
app.Get("/api/feature-b", func(c Ctx) error {
app.RemoveRoute("/api/feature", MethodGet)
app.RebuildTree()
- // Redefine route
app.Get("/api/feature", func(c Ctx) error {
return c.SendString("Testing feature-b")
})
- app.RebuildTree()
return c.SendStatus(http.StatusOK)
})
resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature-a", nil))
require.NoError(t, err, "app.Test(req)")
- require.Equal(t, http.StatusOK, resp, "Status Code")
+ require.Equal(t, http.StatusOK, resp.StatusCode, "Status Code")
resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil))
- require.Equal(t, "Testing feature-a", resp, "Response Message")
+ body, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Equal(t, "Testing feature-a", string(body), "Response Message")
- resp, err = app.Test(httptest.NewRequest(MethodGet, "api/feature-b", nil))
+ resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature-b", nil))
require.NoError(t, err, "app.Test(req)")
- require.Equal(t, "Testing feature-b", resp, "Response Message")
+ body, err = io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Equal(t, "Testing feature-b", string(body), "Response Message")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) { | |
| t.Parallel() | |
| app := New() | |
| app.Get("/api/feature-a", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| // Redefine route | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-a") | |
| }) | |
| app.RebuildTree() | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| app.Get("/api/feature-b", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| // Redefine route | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-b") | |
| }) | |
| app.RebuildTree() | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | |
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature-a", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| require.Equal(t, http.StatusNotFound, resp.StatusCode, "Status code") | |
| require.Equal(t, http.StatusOK, resp, "Status Code") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/test", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil)) | |
| require.Equal(t, "Testing feature-a", resp, "Response Message") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "api/feature-b", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | |
| require.Equal(t, "Testing feature-b", resp, "Response Message") | |
| } | |
| func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) { | |
| t.Parallel() | |
| app := New() | |
| app.Get("/api/feature-a", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-a") | |
| }) | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| app.Get("/api/feature-b", func(c Ctx) error { | |
| app.RemoveRoute("/api/feature", MethodGet) | |
| app.RebuildTree() | |
| app.Get("/api/feature", func(c Ctx) error { | |
| return c.SendString("Testing feature-b") | |
| }) | |
| return c.SendStatus(http.StatusOK) | |
| }) | |
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/api/feature-a", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status Code") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature", nil)) | |
| body, err := io.ReadAll(resp.Body) | |
| require.NoError(t, err) | |
| require.Equal(t, "Testing feature-a", string(body), "Response Message") | |
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/api/feature-b", nil)) | |
| require.NoError(t, err, "app.Test(req)") | |
| body, err = io.ReadAll(resp.Body) | |
| require.NoError(t, err) | |
| require.Equal(t, "Testing feature-b", string(body), "Response Message") | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
479-479: SA4006: this value of err is never used
(staticcheck)
479-479: ineffectual assignment to err
(ineffassign)
|
@gaby @coderabbitai I am using the the Makefile lint and test. The lint is now passing locally however the tests are really hit or miss for me locally not sure what is going on. Even when running on a fresh checkout of gofiber/fiber on main three of the tests fail for me. Also I did get to the point where all the test passed after running a few with |
* Add new methods named RemoveRoute and RemoveRouteByName. * Update register method to prevent duplicate routes. * Clean up tests * Update docs
ckoch786
force-pushed
the
feature/3098-allow-removing-registered-route
branch
from
cf59d6d to
01a58ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
router_test.go (1)
443-477:⚠️ Potential issueFix response handling and remove commented code
The test function
Test_App_Remove_Route_A_B_Feature_Testinghas several issues:
- The
require.Equalassertions incorrectly compare the*http.Responseobject to a string. You should read the response body and compare it to the expected string.- Commented-out code should be removed for code cleanliness.
- Missing leading slashes in the request paths.
Apply this diff to fix the issues:
func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) { t.Parallel() app := New() app.Get("/api/feature-a", func(c Ctx) error { app.RemoveRoute("/api/feature", MethodGet) app.RebuildTree() - // Redefine route app.Get("/api/feature", func(c Ctx) error { return c.SendString("Testing feature-a") }) - app.RebuildTree() return c.SendStatus(StatusOK) }) app.Get("/api/feature-b", func(c Ctx) error { app.RemoveRoute("/api/feature", MethodGet) app.RebuildTree() - // Redefine route app.Get("/api/feature", func(c Ctx) error { return c.SendString("Testing feature-b") }) - app.RebuildTree() return c.SendStatus(StatusOK) }) verifyRequest(t, app, "/api/feature-a", StatusOK) resp := verifyRequest(t, app, "/api/feature", StatusOK) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "Testing feature-a", string(body), "Response Message") - resp = verifyRequest(t, app, "api/feature-b", StatusOK) + resp = verifyRequest(t, app, "/api/feature-b", StatusOK) require.NoError(t, err) - require.Equal(t, "Testing feature-b", resp, "Response Message") + body, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "Testing feature-b", string(body), "Response Message") }docs/api/app.md (1)
768-816:⚠️ Potential issueFix inconsistencies in example code
The example code has several issues:
- Uses
Ctxinstead offiber.Ctx.- Uses undefined
MethodGetconstant; it should befiber.MethodGet.- Uses undefined
http.StatusOKconstant; it should befiber.StatusOK.- Missing blank lines around fenced code blocks as per Markdown conventions.
Apply this diff to fix the issues:
This method removes a route by path. You must call the `RebuildTree()` method after the remove to ensure the route is removed. ```go title="Signature" -func (app *App) RemoveRoute(path string, methods ...string) +func (app *App) RemoveRoute(path string, methods ...string)This method removes a route by name.
-func (app *App) RemoveRouteByName(name string, methods ...string) +func (app *App) RemoveRouteByName(name string, methods ...string)package main import ( "log" "github.com/gofiber/fiber/v3" ) func main() { app := fiber.New() app.Get("/api/feature-a", func(c fiber.Ctx) error { - app.RemoveRoute("/api/feature", MethodGet) + app.RemoveRoute("/api/feature", fiber.MethodGet) app.RebuildTree() // Redefine route - app.Get("/api/feature", func(c Ctx) error { + app.Get("/api/feature", func(c fiber.Ctx) error { return c.SendString("Testing feature-a") }) app.RebuildTree() - return c.SendStatus(http.StatusOK) + return c.SendStatus(fiber.StatusOK) }) app.Get("/api/feature-b", func(c fiber.Ctx) error { - app.RemoveRoute("/api/feature", MethodGet) + app.RemoveRoute("/api/feature", fiber.MethodGet) app.RebuildTree() // Redefine route - app.Get("/api/feature", func(c Ctx) error { + app.Get("/api/feature", func(c fiber.Ctx) error { return c.SendString("Testing feature-b") }) app.RebuildTree() - return c.SendStatus(http.StatusOK) + return c.SendStatus(fiber.StatusOK) }) log.Fatal(app.Listen(":3000")) }🧰 Tools
🪛 Markdownlint (0.37.0)
775-775: null
Fenced code blocks should be surrounded by blank lines(MD031, blanks-around-fences)
🧹 Nitpick comments (2)
docs/api/app.md (2)
765-765: Remove multiple consecutive blank linesThere are multiple consecutive blank lines, which is against Markdown conventions. Please remove extra blank lines to maintain consistency.
Apply this change:
-765~ -🧰 Tools
🪛 Markdownlint (0.37.0)
765-765: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
775-775: Ensure blank lines around fenced code blocksAccording to Markdown conventions, fenced code blocks should be surrounded by blank lines. Please add a blank line before the fenced code block.
Apply this change:
-This method removes a route by name +This method removes a route by name<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 775-775: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cf59d6dbfa9da2b59b46fe52f984bcf45dec93f1 and 01a58abcff9b01434b57af8f77cea337e86d3947. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `docs/api/app.md` (1 hunks) * `docs/whats_new.md` (1 hunks) * `router.go` (4 hunks) * `router_test.go` (2 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * docs/whats_new.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Learnings (1)</summary> <details> <summary>docs/api/app.md (1)</summary>Learnt from: ckoch786
PR: #3230
File: docs/whats_new.md:944-951
Timestamp: 2024-12-15T19:56:45.935Z
Learning: Detailed usage examples and explanations for new methods likeRemoveRouteandRemoveRouteByNameare documented indocs/api/app.md, so it's unnecessary to duplicate them indocs/whats_new.md.</details> </details><details> <summary>🪛 Markdownlint (0.37.0)</summary> <details> <summary>docs/api/app.md</summary> 765-765: Expected: 1; Actual: 2 Multiple consecutive blank lines (MD012, no-multiple-blanks) --- 775-775: null Fenced code blocks should be surrounded by blank lines (MD031, blanks-around-fences) </details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary> <details> <summary>router.go (1)</summary> `386-400`: **Ensure consistent handling of case sensitivity in `routeExists`** The method `routeExists` correctly accounts for case sensitivity in path comparison. However, to ensure consistency across the codebase, consider using `r.Path` instead of `r.path` when comparing routes, as `r.Path` contains the original registered route path. Run the following script to check for inconsistencies in route path comparisons: </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Tip For best results, initiate chat on the files or code changes. @ckoch786, sorry to hear you're experiencing issues running the tests locally. Here are some steps that might help:
If the problems persist, please share the specific tests failing and any error messages you receive. This information can help diagnose the issue more effectively. |
* Add new methods named RemoveRoute and RemoveRouteByName. * Update register method to prevent duplicate routes. * Clean up tests * Update docs
ckoch786
force-pushed
the
feature/3098-allow-removing-registered-route
branch
from
01a58ab to
0cab312
Compare
|
@gaby I am looking at the failing tests from the actions that ran against my branch. I think I found the the changes that I made that is probably causing them to fail. Though it is weird that they do not always fail for me locally, not sure what that is about yet. |
Are you running just the new tests or the full suite? Use this |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
router.go (3)
386-400: Code formatting could be improved.Consider removing the extra blank lines between the case sensitivity check and the return statement to maintain consistent spacing.
func (app *App) routeExists(method, pathRaw string) bool { pathToCheck := pathRaw if !app.config.CaseSensitive { pathToCheck = utils.ToLower(pathToCheck) } - return slices.ContainsFunc(app.stack[app.methodInt(method)], func(r *Route) bool { routePath := r.path if !app.config.CaseSensitive { routePath = utils.ToLower(routePath) } - return routePath == pathToCheck }) }
402-416: Consider adding error return values.The methods could benefit from returning errors to indicate whether the route was found and removed successfully. This would help users handle cases where routes don't exist.
-func (app *App) RemoveRoute(path string, methods ...string) { +func (app *App) RemoveRoute(path string, methods ...string) error { pathMatchFunc := func(r *Route) bool { return r.Path == path } - app.deleteRoute(methods, pathMatchFunc) + return app.deleteRoute(methods, pathMatchFunc) } -func (app *App) RemoveRouteByName(name string, methods ...string) { +func (app *App) RemoveRouteByName(name string, methods ...string) error { matchFunc := func(r *Route) bool { return r.Name == name } - app.deleteRoute(methods, matchFunc) + return app.deleteRoute(methods, matchFunc) }
418-450: Consider adding validation for methods parameter.The deleteRoute method could benefit from validating that at least one method is provided to prevent unnecessary locking.
func (app *App) deleteRoute(methods []string, matchFunc func(r *Route) bool) { + if len(methods) == 0 { + return + } app.mutex.Lock() defer app.mutex.Unlock()docs/api/app.md (1)
764-772: Fix typo in method description.There's a grammatical error in the description.
-This method removes a route by path. You must call the `RebuildTree()` method after the remove in to ensure the route is removed. +This method removes a route by path. You must call the `RebuildTree()` method after the remove to ensure the route is removed.🧰 Tools
🪛 Markdownlint (0.37.0)
765-765: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
router_test.go (1)
558-584: Consider increasing concurrent test coverage.While the concurrent test is well-implemented, consider:
- Increasing the number of iterations for better race condition detection
- Adding random delays to increase contention
- Testing with multiple different routes
- for i := 0; i < 10; i++ { + for i := 0; i < 100; i++ { wg.Add(1) go func() { defer wg.Done() + time.Sleep(time.Duration(rand.Intn(100)) * time.Microsecond) app.RemoveRoute("/test", MethodGet) app.Get("/test", func(c Ctx) error { return c.SendStatus(StatusOK) }) }() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/api/app.md(1 hunks)docs/whats_new.md(1 hunks)router.go(4 hunks)router_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/whats_new.md
🧰 Additional context used
📓 Learnings (1)
docs/api/app.md (1)
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`.
🪛 Markdownlint (0.37.0)
docs/api/app.md
765-765: Expected: 1; Actual: 2
Multiple consecutive blank lines
(MD012, no-multiple-blanks)
775-775: null
Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (4)
router.go (1)
306-312: LGTM! Well-implemented duplicate route handling.
The implementation correctly prevents duplicate routes by checking existence and removing the old route before adding a new one. This maintains routing consistency and prevents potential conflicts.
docs/api/app.md (1)
779-816: LGTM! Well-documented example.
The example effectively demonstrates the usage of RemoveRoute, including proper error handling and the necessary RebuildTree calls.
router_test.go (2)
376-430: LGTM! Well-implemented test helpers.
The helper functions follow testing best practices, including proper tb.Helper() calls and clear documentation.
494-556: LGTM! Comprehensive edge case coverage.
The tests effectively cover important edge cases including non-existing routes, nested routes, and parameterized routes.
| func Test_App_Remove_Route_A_B_Feature_Testing(t *testing.T) { | ||
| t.Parallel() | ||
| app := New() | ||
|
|
||
| app.Get("/api/feature-a", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-a") | ||
| }) | ||
|
|
||
| app.RebuildTree() | ||
| return c.SendStatus(StatusOK) | ||
| }) | ||
| app.Get("/api/feature-b", func(c Ctx) error { | ||
| app.RemoveRoute("/api/feature", MethodGet) | ||
| app.RebuildTree() | ||
| // Redefine route | ||
| app.Get("/api/feature", func(c Ctx) error { | ||
| return c.SendString("Testing feature-b") | ||
| }) | ||
|
|
||
| return c.SendStatus(http.StatusOK) | ||
| app.RebuildTree() | ||
| return c.SendStatus(StatusOK) | ||
| }) | ||
|
|
||
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusNotFound, resp.StatusCode, "Status code") | ||
| verifyRequest(t, app, "/api/feature-a", StatusOK) | ||
|
|
||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/test", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | ||
| resp := verifyRequest(t, app, "/api/feature", StatusOK) | ||
| require.Equal(t, "Testing feature-a", resp, "Response Message") | ||
|
|
||
| resp, err = app.Test(httptest.NewRequest(MethodGet, "/dynamically-defined", nil)) | ||
| require.NoError(t, err, "app.Test(req)") | ||
| require.Equal(t, http.StatusOK, resp.StatusCode, "Status code") | ||
| resp = verifyRequest(t, app, "/api/feature-b", StatusOK) | ||
| require.Equal(t, "Testing feature-b", resp, "Response Message") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix test implementation issues.
The test has several issues:
- Contains unnecessary comments
- Incorrect response comparison
- Missing proper response body handling
- // Redefine route
app.Get("/api/feature", func(c Ctx) error {
return c.SendString("Testing feature-a")
})
- app.RebuildTree()
resp := verifyRequest(t, app, "/api/feature", StatusOK)
- require.Equal(t, "Testing feature-a", resp, "Response Message")
+ body, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Equal(t, "Testing feature-a", string(body), "Response Message")Committable suggestion skipped: line range outside the PR's diff.
Description
Fixes #3098
Changes introduced
Type of change