Skip to content

Commit

Permalink
catch-all: Fix wrong conflict with param. Remove unnecessary check wh…
Browse files Browse the repository at this point in the history 
…en inserting catch-all node. Add tests.
  • Loading branch information
@StephanoGeorge
StephanoGeorge committed Aug 8, 2022
1 parent 8f2fd7a commit 9902cdb
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 11 deletions.
12 changes: 11 additions & 1 deletion gin_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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") })
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
11 changes: 1 addition & 10 deletions tree.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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] != '/' {
Expand Down
27 changes: 27 additions & 0 deletions tree_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"}}},
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit 9902cdb

Please sign in to comment.