From 9902cdbcce65b75158fd451db9eec38a49832bc9 Mon Sep 17 00:00:00 2001 From: Stephano George Date: Mon, 8 Aug 2022 17:41:09 +0800 Subject: [PATCH] catch-all: Fix wrong conflict with param. Remove unnecessary check when inserting catch-all node. Add tests. --- gin_integration_test.go | 12 +++++++++++- tree.go | 11 +---------- tree_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/gin_integration_test.go b/gin_integration_test.go index ae975b3d9a..e87846d699 100644 --- a/gin_integration_test.go +++ b/gin_integration_test.go @@ -414,11 +414,14 @@ func testGetRequestHandler(t *testing.T, h http.Handler, url string) { func TestTreeRunDynamicRouting(t *testing.T) { router := New() + router.GET("/", func(c *Context) { c.String(http.StatusOK, "home") }) router.GET("/aa/*xx", func(c *Context) { c.String(http.StatusOK, "/aa/*xx") }) router.GET("/ab/aa", func(c *Context) { c.String(http.StatusOK, "/ab/aa") }) router.GET("/ab/*xx", func(c *Context) { c.String(http.StatusOK, "/ab/*xx") }) router.GET("/ab/zz", func(c *Context) { c.String(http.StatusOK, "/ab/zz") }) - router.GET("/", func(c *Context) { c.String(http.StatusOK, "home") }) + router.GET("/abc/def", func(c *Context) { c.String(http.StatusOK, "/abc/def") }) + router.GET("/abc/d/:ee/*all", func(c *Context) { c.String(http.StatusOK, "/abc/d/:ee/*all") }) + router.GET("/abc/de/*all", func(c *Context) { c.String(http.StatusOK, "/abc/de/*all") }) router.GET("/:cc", func(c *Context) { c.String(http.StatusOK, "/:cc") }) router.GET("/c1/:dd/e", func(c *Context) { c.String(http.StatusOK, "/c1/:dd/e") }) router.GET("/c1/:dd/e1", func(c *Context) { c.String(http.StatusOK, "/c1/:dd/e1") }) @@ -465,6 +468,8 @@ func TestTreeRunDynamicRouting(t *testing.T) { testRequest(t, ts.URL+"/ab/ab", "", "/ab/*xx") testRequest(t, ts.URL+"/ab/aab", "", "/ab/*xx") testRequest(t, ts.URL+"/ab/", "", "/ab/*xx") + testRequest(t, ts.URL+"/abc/d/e", "", "/abc/d/:ee/*all") + testRequest(t, ts.URL+"/abc/d//", "", "/abc/d/:ee/*all") testRequest(t, ts.URL+"/all", "", "/:cc") testRequest(t, ts.URL+"/all/cc", "", "/:cc/cc") testRequest(t, ts.URL+"/a/cc", "", "/:cc/cc") @@ -504,7 +509,9 @@ func TestTreeRunDynamicRouting(t *testing.T) { testRequest(t, ts.URL+"/get/aa/abc/", "", "/get/:param/abc/") testRequest(t, ts.URL+"/get/abas/abc/", "", "/get/:param/abc/") testRequest(t, ts.URL+"/get/testt/abc", "", "/get/:param/abc/") + testRequest(t, ts.URL+"/get/test/", "", "/get/:param") testRequest(t, ts.URL+"/get/test/abcde/", "", "/get/:param/*all") + testRequest(t, ts.URL+"/get/test//abc", "", "/get/:param/*all") testRequest(t, ts.URL+"/something/secondthing/test", "", "/something/secondthing/test") testRequest(t, ts.URL+"/something/secondthingaaaa/thirdthing", "", "/something/:paramname/thirdthing") testRequest(t, ts.URL+"/something/abcdad/thirdthing", "", "/something/:paramname/thirdthing") @@ -556,6 +563,9 @@ func TestTreeRunDynamicRouting(t *testing.T) { testRequest(t, ts.URL+"/get/abc/123abf/testss", "", "/get/abc/123abf/:param") testRequest(t, ts.URL+"/get/abc/123abfff/te", "", "/get/abc/123abfff/:param") // 404 not found + testRequest(t, ts.URL+"/abc/deX", "404 Not Found") + testRequest(t, ts.URL+"/abc/defX", "404 Not Found") + testRequest(t, ts.URL+"/abc/d/", "404 Not Found") testRequest(t, ts.URL+"/c/d/e", "404 Not Found") testRequest(t, ts.URL+"/c/d/e1", "404 Not Found") testRequest(t, ts.URL+"/c/d/eee", "404 Not Found") diff --git a/tree.go b/tree.go index a2e30b72bb..49e2d2624c 100644 --- a/tree.go +++ b/tree.go @@ -225,7 +225,7 @@ walk: if child.nType == catchAll && (isCatchAll || strings.HasPrefix(path, "/:")) { n.panicWildcardConflict(fullPath, path) } - if isCatchAll && child.wildChild && child.children[len(child.children)-1].nType == param { + if isCatchAll && child.path == "/" && child.wildChild && child.children[len(child.children)-1].nType == param { n.panicWildcardConflict(fullPath, path) } } @@ -352,15 +352,6 @@ func (n *node) insertChild(path string, fullPath string, handlers HandlersChain) panic("catch-all routes are only allowed at the end of the path in path '" + fullPath + "'") } - if len(n.path) > 0 && n.path[len(n.path)-1] == '/' { - pathSeg := strings.SplitN(n.children[0].path, "/", 2)[0] - panic("catch-all wildcard '" + path + - "' in new path '" + fullPath + - "' conflicts with existing path segment '" + pathSeg + - "' in existing prefix '" + n.path + pathSeg + - "'") - } - // currently fixed width 1 for '/' i-- if path[i] != '/' { diff --git a/tree_test.go b/tree_test.go index 9ff1bb0d00..5de0298f9d 100644 --- a/tree_test.go +++ b/tree_test.go @@ -163,6 +163,9 @@ func TestTreeWildcard(t *testing.T) { "/ab/aa", "/ab/*xx", "/ab/zz", + "/abc/def", + "/abc/d/:ee/*all", + "/abc/de/*all", "/:cc", "/c1/:dd/e", "/c1/:dd/e1", @@ -233,6 +236,11 @@ func TestTreeWildcard(t *testing.T) { {"/ab/ab", false, "/ab/*xx", Params{Param{Key: "xx", Value: "/ab"}}}, {"/ab/aab", false, "/ab/*xx", Params{Param{Key: "xx", Value: "/aab"}}}, {"/ab/", false, "/ab/*xx", Params{Param{Key: "xx", Value: "/"}}}, + {"/abc/d/e", true, "/abc/d/:ee/*all", Params{Param{Key: "ee", Value: "e"}}}, + {"/abc/d//", false, "/abc/d/:ee/*all", Params{Param{Key: "ee", Value: ""}, Param{Key: "all", Value: "/"}}}, + {"/abc/deX", true, "", Params{Param{Key: "cc", Value: "abc"}, Param{Key: "dd", Value: "deX"}}}, + {"/abc/defX", true, "", Params{Param{Key: "cc", Value: "abc"}, Param{Key: "dd", Value: "defX"}}}, + {"/abc/d/", true, "", Params{Param{Key: "cc", Value: "abc"}, Param{Key: "dd", Value: "d"}}}, {"/a", false, "/:cc", Params{Param{Key: "cc", Value: "a"}}}, // * Error with argument being intercepted // new PR handle (/all /all/cc /a/cc) @@ -269,7 +277,9 @@ func TestTreeWildcard(t *testing.T) { {"/get/aa/abc/", false, "/get/:param/abc/", Params{Param{Key: "param", Value: "aa"}}}, {"/get/abas/abc/", false, "/get/:param/abc/", Params{Param{Key: "param", Value: "abas"}}}, {"/get/testt/abc", true, "/get/:param/abc/", Params{Param{Key: "param", Value: "testt"}}}, + {"/get/test/", true, "/get/:param", Params{Param{Key: "param", Value: "test"}}}, {"/get/test/abcde/", false, "/get/:param/*all", Params{Param{Key: "param", Value: "test"}, Param{Key: "all", Value: "/abcde/"}}}, + {"/get/test//abc", false, "/get/:param/*all", Params{Param{Key: "param", Value: "test"}, Param{Key: "all", Value: "//abc"}}}, {"/something/secondthing/test", false, "/something/secondthing/test", nil}, {"/something/abcdad/thirdthing", false, "/something/:paramname/thirdthing", Params{Param{Key: "paramname", Value: "abcdad"}}}, {"/something/secondthingaaaa/thirdthing", false, "/something/:paramname/thirdthing", Params{Param{Key: "paramname", Value: "secondthingaaaa"}}}, @@ -556,6 +566,23 @@ func TestTreeCatchAllRoot(t *testing.T) { }, true) } +func TestCatchAllPathNoLeadingSlash(t *testing.T) { + tree := &node{} + + routes := [...]string{ + "/abc/defg/:param", + "/abc/def/*all", + } + for _, route := range routes { + tree.addRoute(route, fakeHandler(route)) + } + + checkRequests(t, tree, testRequests{ + {"/abc/defX", true, "", nil}, + {"/abc/def/X", false, "/abc/def/*all", Params{Param{"all", "/X"}}}, + }) +} + func TestTreeCatchMaxParams(t *testing.T) { tree := &node{} var route = "/cmd/*filepath"