Fix conflict between param and exact path #2706
Conversation
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2706 +/- ##
==========================================
+ Coverage 98.67% 98.68% +0.01%
==========================================
Files 41 41
Lines 2036 2054 +18
==========================================
+ Hits 2009 2027 +18
Misses 15 15
Partials 12 12
Continue to review full report at Codecov.
|
|
@thinkerou Would you please take a look when you are free? Very thx. 😃 |
|
@g1eny0ung Can you also send the PR to https://github.com/julienschmidt/httprouter? |
Because the PR julienschmidt/httprouter#329 still not be merged, so it's needed to wait for it or let @rw-access help to update the related code. |
|
@g1eny0ung Please provide the benchmark result like #2663 (comment) and #2663 (comment) |
|
@rw-access @appleboy Thanks for the notes mentioned. I will try to add more tests and the benchmark later to make sure the change code is stable. |
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
|
@appleboy After @rw-access noted (backtracking) and some thoughts 🤔, I update the code to save a temporary The skipped contains the current node path and a param node (same as the current node except no indices). When a param path has the same prefix with the exact path, the prefix tree will work as before, keep searching downwards, but it will fail. Then the follow-up logic will check the Compared to before, I have added more tests, if there are omissions, please help me to add. 😆 The benchmarks: master - https://travis-ci.com/github/justttype/go-http-routing-benchmark/builds/224357913 The Thank you all for your reviews. |
| "/cmd/:tool/", | ||
| "/cmd/:tool/:sub", |
There was a problem hiding this comment.
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/.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Know what you mean. I will think about this situation.
There was a problem hiding this comment.
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.
| @@ -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, ":") { | |||
There was a problem hiding this comment.
I think you want to check if the last child is of type Param, right?
There was a problem hiding this comment.
and then when you need to backtrack, then you can go to the Param child directly?
There was a problem hiding this comment.
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.
|
@thinkerou @appleboy What do you think of these changes? Should it be merged? |
|
@rw-access needs your feedback. |
|
It's very important fix for everybody! |
|
nice,great job!
…------------------ Original ------------------
From: Denis Saponenko ***@***.***>
Date: Wed,May 12,2021 7:13 PM
To: gin-gonic/gin ***@***.***>
Cc: heige ***@***.***>, Comment ***@***.***>
Subject: Re: [gin-gonic/gin] Fix conflict between param and exact path (#2706)
It's very important fix for everybody!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
| if strings.HasPrefix(n.children[len(n.children)-1].path, ":") { | ||
| skipped = &skip{ | ||
| path: prefix + path, | ||
| paramNode: &node{ |
There was a problem hiding this comment.
why not just make this data structure take n?
There was a problem hiding this comment.
Because this operation removes indices to prevent prefix checking.
|
I will bump the new version v1.7.2 if the PR has been merge. Waiting for @thinkerou approval. |
|
Hi All, We already released https://github.com/gin-gonic/gin/releases/tag/v1.7.2 version, just include the changes. |
* Fix conflict between param and exact path Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Add test Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Fix prefix conflict in exact paths Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Use backtracking Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Fix panic Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
* Fix conflict between param and exact path Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Add test Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Fix prefix conflict in exact paths Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Use backtracking Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Fix panic Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> (cherry picked from commit 2921582)
* Fix conflict between param and exact path Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Add test Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Fix prefix conflict in exact paths Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Use backtracking Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> * Fix panic Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang g1enyy0ung@gmail.com
Close #2682.
Close #2696.
This PR solves the above issues.
For a detailed description, see #2682 (comment).