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.

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

Fix conflict between param and exact path #2706

Merged
merged 7 commits into from May 19, 2021

Conversation

g1eny0ung
Copy link
Contributor

@g1eny0ung g1eny0ung commented Apr 25, 2021

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).

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@codecov
Copy link

codecov bot commented Apr 25, 2021

Codecov Report

Merging #2706 (808f903) into master (4fe5f3e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
tree.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fe5f3e...808f903. Read the comment docs.

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung
Copy link
Contributor Author

g1eny0ung commented Apr 28, 2021

@thinkerou Would you please take a look when you are free? Very thx. 😃

@appleboy
Copy link
Member

@g1eny0ung Can you also send the PR to https://github.com/julienschmidt/httprouter?

@g1eny0ung
Copy link
Contributor Author

g1eny0ung commented Apr 28, 2021

@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.

tree.go Outdated Show resolved Hide resolved
tree_test.go Show resolved Hide resolved
@appleboy
Copy link
Member

@g1eny0ung Please provide the benchmark result like #2663 (comment) and #2663 (comment)

@g1eny0ung
Copy link
Contributor Author

@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.

g1eny0ung added 3 commits Apr 28, 2021
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
Signed-off-by: Yue Yang <g1enyy0ung@gmail.com>
@g1eny0ung
Copy link
Contributor Author

@appleboy After @rw-access noted (backtracking) and some thoughts 🤔, I update the code to save a temporary skipped value when the children have a param node at the last.

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 skipped value is not nil to perform a backtracking search. The param node in skipped will be set to the current node. Because it has no indices, it will jump the non-wildcard children check, go directly to the place of param judgment.

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
this PR - https://travis-ci.com/github/justttype/go-http-routing-benchmark/builds/224455608

The benchstat report:

name              old time/op    new time/op    delta
Gin_Param           66.6ns ± 0%    67.6ns ± 0%   ~     (p=1.000 n=1+1)
Gin_Param5           117ns ± 0%     112ns ± 0%   ~     (p=1.000 n=1+1)
Gin_Param20          303ns ± 0%     298ns ± 0%   ~     (p=1.000 n=1+1)
Gin_ParamWrite       123ns ± 0%     119ns ± 0%   ~     (p=1.000 n=1+1)
Gin_GithubStatic    77.4ns ± 0%    90.0ns ± 0%   ~     (p=1.000 n=1+1)
Gin_GithubParam      133ns ± 0%     156ns ± 0%   ~     (p=1.000 n=1+1)
Gin_GithubAll       26.8µs ± 0%    31.1µs ± 0%   ~     (p=1.000 n=1+1)
Gin_GPlusStatic     61.7ns ± 0%    66.8ns ± 0%   ~     (p=1.000 n=1+1)
Gin_GPlusParam      82.5ns ± 0%    92.2ns ± 0%   ~     (p=1.000 n=1+1)
Gin_GPlus2Params     103ns ± 0%     116ns ± 0%   ~     (p=1.000 n=1+1)
Gin_GPlusAll        1.13µs ± 0%    1.29µs ± 0%   ~     (p=1.000 n=1+1)
Gin_ParseStatic     62.3ns ± 0%    65.5ns ± 0%   ~     (p=1.000 n=1+1)
Gin_ParseParam      71.4ns ± 0%    74.3ns ± 0%   ~     (p=1.000 n=1+1)
Gin_Parse2Params    84.4ns ± 0%    84.7ns ± 0%   ~     (p=1.000 n=1+1)
Gin_ParseAll        2.01µs ± 0%    2.16µs ± 0%   ~     (p=1.000 n=1+1)
Gin_StaticAll       18.5µs ± 0%    23.7µs ± 0%   ~     (p=1.000 n=1+1)

name              old alloc/op   new alloc/op   delta
Gin_Param            0.00B          0.00B        ~     (all equal)
Gin_Param5           0.00B          0.00B        ~     (all equal)
Gin_Param20          0.00B          0.00B        ~     (all equal)
Gin_ParamWrite       0.00B          0.00B        ~     (all equal)
Gin_GithubStatic     0.00B          0.00B        ~     (all equal)
Gin_GithubParam      0.00B          0.00B        ~     (all equal)
Gin_GithubAll        0.00B          0.00B        ~     (all equal)
Gin_GPlusStatic      0.00B          0.00B        ~     (all equal)
Gin_GPlusParam       0.00B          0.00B        ~     (all equal)
Gin_GPlus2Params     0.00B          0.00B        ~     (all equal)
Gin_GPlusAll         0.00B          0.00B        ~     (all equal)
Gin_ParseStatic      0.00B          0.00B        ~     (all equal)
Gin_ParseParam       0.00B          0.00B        ~     (all equal)
Gin_Parse2Params     0.00B          0.00B        ~     (all equal)
Gin_ParseAll         0.00B          0.00B        ~     (all equal)
Gin_StaticAll        0.00B          0.00B        ~     (all equal)

name              old allocs/op  new allocs/op  delta
Gin_Param             0.00           0.00        ~     (all equal)
Gin_Param5            0.00           0.00        ~     (all equal)
Gin_Param20           0.00           0.00        ~     (all equal)
Gin_ParamWrite        0.00           0.00        ~     (all equal)
Gin_GithubStatic      0.00           0.00        ~     (all equal)
Gin_GithubParam       0.00           0.00        ~     (all equal)
Gin_GithubAll         0.00           0.00        ~     (all equal)
Gin_GPlusStatic       0.00           0.00        ~     (all equal)
Gin_GPlusParam        0.00           0.00        ~     (all equal)
Gin_GPlus2Params      0.00           0.00        ~     (all equal)
Gin_GPlusAll          0.00           0.00        ~     (all equal)
Gin_ParseStatic       0.00           0.00        ~     (all equal)
Gin_ParseParam        0.00           0.00        ~     (all equal)
Gin_Parse2Params      0.00           0.00        ~     (all equal)
Gin_ParseAll          0.00           0.00        ~     (all equal)
Gin_StaticAll         0.00           0.00        ~     (all equal)

Thank you all for your reviews.

"/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

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

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.

@@ -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.

daheige
daheige approved these changes May 1, 2021
@g1eny0ung
Copy link
Contributor Author

@thinkerou @appleboy What do you think of these changes? Should it be merged?

@appleboy
Copy link
Member

@rw-access needs your feedback.

@vaimr
Copy link

vaimr commented May 12, 2021

It's very important fix for everybody!

@daheige
Copy link
Contributor

daheige commented May 14, 2021 via email

if strings.HasPrefix(n.children[len(n.children)-1].path, ":") {
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.

tree.go Show resolved Hide resolved
Copy link
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fixes

@appleboy
Copy link
Member

I will bump the new version v1.7.2 if the PR has been merge. Waiting for @thinkerou approval.

@appleboy appleboy modified the milestones: v1.8, v1.7.2 May 18, 2021
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm

@appleboy appleboy merged commit 2921582 into gin-gonic:master May 19, 2021
3 checks passed
@appleboy appleboy modified the milestones: v1.7.2, v1.8 May 19, 2021
@g1eny0ung g1eny0ung deleted the fix/issue#2682 branch May 19, 2021
@appleboy
Copy link
Member

Hi All,

We already released https://github.com/gin-gonic/gin/releases/tag/v1.7.2 version, just include the changes.

@nnntlh nnntlh mentioned this pull request Jun 18, 2021
@appleboy appleboy mentioned this pull request Jun 28, 2021
appleboy pushed a commit that referenced this pull request Aug 15, 2021
* 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>
Bisstocuz pushed a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
* 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)
thinkerou pushed a commit that referenced this pull request Nov 23, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matching prefixes among multiple routers causes 404 [Bug] Mixed params and exact paths can cause a 404
6 participants