Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix conflict between param and exact path #2706

Merged
merged 7 commits into from
May 19, 2021
Merged

Fix conflict between param and exact path #2706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
801d7b3
Fix conflict between param and exact path
g1eny0ung Apr 25, 2021
39fa5a3
Add test
g1eny0ung Apr 25, 2021
7c9e3b0
Fix prefix conflict in exact paths
g1eny0ung Apr 28, 2021
3bb5308
Use backtracking
g1eny0ung Apr 29, 2021
e75fe4a
Fix panic
g1eny0ung Apr 29, 2021
47b1ea4
Merge branch 'master' into fix/issue#2682
g1eny0ung Apr 29, 2021
808f903
Merge branch 'master' into fix/issue#2682
g1eny0ung May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ type node struct {
fullPath string
}

type skip struct {
path string
paramNode *node
}

// Increments priority of the given child and reorders if necessary
func (n *node) incrementChildPrio(pos int) int {
cs := n.children
Expand Down Expand Up @@ -400,6 +405,8 @@ type nodeValue struct {
// made if a handle exists with an extra (without the) trailing slash for the
// given path.
func (n *node) getValue(path string, params *Params, unescape bool) (value nodeValue) {
var skipped *skip

walk: // Outer loop for walking the tree
for {
prefix := n.path
Expand All @@ -411,6 +418,21 @@ walk: // Outer loop for walking the tree
idxc := path[0]
for i, c := range []byte(n.indices) {
if c == idxc {
if strings.HasPrefix(n.children[len(n.children)-1].path, ":") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to check if the last child is of type Param, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then when you need to backtrack, then you can go to the Param child directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think is needed to save the Param child to a place? Otherwise, I will not be able to find it because the walk loop will replace the previous node with the current.

skipped = &skip{
path: prefix + path,
paramNode: &node{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just make this data structure take n?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this operation removes indices to prevent prefix checking.

path: n.path,
wildChild: n.wildChild,
nType: n.nType,
priority: n.priority,
children: n.children,
handlers: n.handlers,
fullPath: n.fullPath,
},
g1eny0ung marked this conversation as resolved.
Show resolved Hide resolved
}
}

n = n.children[i]
continue walk
}
Expand Down Expand Up @@ -542,6 +564,13 @@ walk: // Outer loop for walking the tree
return
}

if path != "/" && skipped != nil && strings.HasSuffix(skipped.path, path) {
path = skipped.path
n = skipped.paramNode
skipped = nil
continue walk
}

// Nothing found. We can recommend to redirect to the same URL with an
// extra trailing slash if a leaf exists for that path
value.tsr = (path == "/") ||
Expand Down
18 changes: 15 additions & 3 deletions tree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,16 @@ func TestTreeWildcard(t *testing.T) {

routes := [...]string{
"/",
"/cmd/:tool/:sub",
"/cmd/:tool/",
"/cmd/:tool/:sub",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the updates, I really appreciate it.

I think we could use another test to check edge case to check:

if you have routes like this set up:

  • /cmd/whoami/foo
  • /cmd/:tool/
  • /cmd/:tool/:subtool

What happens if someone hits the route:

  • /cmd/whoami/bar?

It should cause a 404 and not hit /cmd/:tool/:subtool.

Basically, whenever the tree is exhausted or a the end of a path segment is found, the backtrack should be invalidated. Once you hit /cmd/whoami/, it shouldn't be possible to fall back to /cmd/tool/.

Copy link
Contributor Author

@g1eny0ung g1eny0ung Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the problem.

For example, /cmd/who/ will not hit /cmd/:tool/ because it has the same prefix as /cmd/whoami/foo, but it should match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Know what you mean. I will think about this situation.

Copy link
Contributor Author

@g1eny0ung g1eny0ung Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what still let me confused is: with the above routes, whether /cmd/whoami/bar hit or not, seems they are all meaningful?

According to my understanding and my actual use, if I don’t have an exact route definition like /cmd/whoami/foo, then /cmd/whoami/bar needs to be matched to the wildcard route.

"/cmd/whoami",
"/cmd/whoami/root",
"/cmd/whoami/root/",
"/src/*filepath",
"/search/",
"/search/:query",
"/search/gin-gonic",
"/search/google",
"/user_:name",
"/user_:name/about",
"/files/:dir/*filepath",
Expand All @@ -150,6 +153,7 @@ func TestTreeWildcard(t *testing.T) {
"/doc/go1.html",
"/info/:user/public",
"/info/:user/project/:project",
"/info/:user/project/golang",
}
for _, route := range routes {
tree.addRoute(route, fakeHandler(route))
Expand All @@ -159,21 +163,29 @@ func TestTreeWildcard(t *testing.T) {
{"/", false, "/", nil},
{"/cmd/test", true, "/cmd/:tool/", Params{Param{"tool", "test"}}},
{"/cmd/test/", false, "/cmd/:tool/", Params{Param{"tool", "test"}}},
{"/cmd/test/3", false, "/cmd/:tool/:sub", Params{Param{Key: "tool", Value: "test"}, Param{Key: "sub", Value: "3"}}},
{"/cmd/who", true, "/cmd/:tool/", Params{Param{"tool", "who"}}},
{"/cmd/who/", false, "/cmd/:tool/", Params{Param{"tool", "who"}}},
{"/cmd/whoami", false, "/cmd/whoami", nil},
{"/cmd/whoami/", true, "/cmd/whoami", nil},
{"/cmd/whoami/r", false, "/cmd/:tool/:sub", Params{Param{Key: "tool", Value: "whoami"}, Param{Key: "sub", Value: "r"}}},
{"/cmd/whoami/r/", true, "/cmd/:tool/:sub", Params{Param{Key: "tool", Value: "whoami"}, Param{Key: "sub", Value: "r"}}},
{"/cmd/whoami/root", false, "/cmd/whoami/root", nil},
{"/cmd/whoami/root/", false, "/cmd/whoami/root/", nil},
{"/cmd/whoami/root", true, "/cmd/whoami/root/", nil},
{"/cmd/test/3", false, "/cmd/:tool/:sub", Params{Param{Key: "tool", Value: "test"}, Param{Key: "sub", Value: "3"}}},
{"/src/", false, "/src/*filepath", Params{Param{Key: "filepath", Value: "/"}}},
{"/src/some/file.png", false, "/src/*filepath", Params{Param{Key: "filepath", Value: "/some/file.png"}}},
{"/search/", false, "/search/", nil},
{"/search/someth!ng+in+ünìcodé", false, "/search/:query", Params{Param{Key: "query", Value: "someth!ng+in+ünìcodé"}}},
{"/search/someth!ng+in+ünìcodé/", true, "", Params{Param{Key: "query", Value: "someth!ng+in+ünìcodé"}}},
{"/search/gin", false, "/search/:query", Params{Param{"query", "gin"}}},
g1eny0ung marked this conversation as resolved.
Show resolved Hide resolved
{"/search/gin-gonic", false, "/search/gin-gonic", nil},
{"/search/google", false, "/search/google", nil},
{"/user_gopher", false, "/user_:name", Params{Param{Key: "name", Value: "gopher"}}},
{"/user_gopher/about", false, "/user_:name/about", Params{Param{Key: "name", Value: "gopher"}}},
{"/files/js/inc/framework.js", false, "/files/:dir/*filepath", Params{Param{Key: "dir", Value: "js"}, Param{Key: "filepath", Value: "/inc/framework.js"}}},
{"/info/gordon/public", false, "/info/:user/public", Params{Param{Key: "user", Value: "gordon"}}},
{"/info/gordon/project/go", false, "/info/:user/project/:project", Params{Param{Key: "user", Value: "gordon"}, Param{Key: "project", Value: "go"}}},
{"/info/gordon/project/golang", false, "/info/:user/project/golang", Params{Param{Key: "user", Value: "gordon"}}},
})

checkPriorities(t, tree)
Expand Down