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

Add mixed param and non-param paths (port of httprouter#329) #2663

Merged
merged 3 commits into from
Apr 6, 2021

Conversation

rw-access
Copy link
Contributor

This ports a PR I made to httprouter, julienschmidt/httprouter#329.

It adds the ability to have route schemas like /users/:name and /users/groups without running into conflicts.

From julienschmidt/httprouter#329:

I updated the search tree, so that non-wildcard paths are resolved before wildcard paths, but both can be valid children. For instance, now you can finally have /users/roles/:role and /users/:id at the same time without issue.

First, non-wildcard children are checked for matches. If those fail to match, then it tries the wildcard children routes. For existing compatible route configurations, there should be no (or negligible) performance impact. Many people prefer compact routes and expect a sort of precedence with httprouter, that mirrors this behavior.

This only works for param wildcards of the form :name and doesn't change the existing *

One thing to note, because of the prefix tree structure, if the path /books/fantasy/2007 would matched a /books/fantasy/... route because it takes priority over a /books/: category/:year template route, because exact prefixes take priority over wildcard prefixes. I think this is okay and is still a net positive. This can be merely a documentation issue.

Overall, I think the changes capture a pretty intuitive behavior, has little to no performance impact and is something many people expect from a router. Personally, I use gin and fizz and would love to have this behavior. So I played around with it and want to share and gather your thoughts @julienschmidt.

Related issues
Looks like there's a lot of issues in this repo, but mostly in Gin. I did my best to capture them, but I'm sure that I'm missing some.

Partially resolves julienschmidt/httprouter#73 (doesn't work for catch-all wildcards)
Resolves julienschmidt/httprouter#91
Resolves julienschmidt/httprouter#183
Resolves julienschmidt/httprouter#202
Resolves julienschmidt/httprouter#203

Resolves #136
Resolves #205
Resolves #360
Resolves #388
Resolves #574
Resolves #957
Resolves #1148
Resolves #1065
Resolves #1132
Resolves #1301
Resolves #1681
Resolves #1730
Resolves #1756
Resolves #1990
Resolves #1993
Resolves #2005
Resolves #2016
Resolves #2062
Resolves #2195
Resolves #2289
Resolves #2416
Resolves #2465
Resolves #2537

@rw-access rw-access changed the title Port changes from httprouter#329 to gin Add mixed param and non-param paths (port of httprouter#329) Mar 26, 2021
shyaminayesh
shyaminayesh previously approved these changes Mar 26, 2021
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 27, 2021

Codecov Report

Merging #2663 (5134779) into master (a331dc6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5134779 differs from pull request most recent head 32b2982. Consider uploading reports for the commit 32b2982 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2663   +/-   ##
=======================================
  Coverage   98.64%   98.64%           
=======================================
  Files          41       41           
  Lines        1989     1991    +2     
=======================================
+ Hits         1962     1964    +2     
  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 a331dc6...32b2982. Read the comment docs.

@thinkerou
Copy link
Member

we also should have benchmark test result

@rw-access
Copy link
Contributor Author

@thinkerou do you want me to run those benchmarks or is that something you or another maintainer would do?
I'm assuming that it means I would want to fork go-http-routing-benchmark and point it to this branch

@rw-access rw-access requested review from appleboy and removed request for a team March 30, 2021 09:48
@appleboy
Copy link
Member

@rw-access Yes, please fork the repo go-http-routing-benchmark and post the final benchmark result (before and after).

@rw-access
Copy link
Contributor Author

@appleboy
Copy link
Member

appleboy commented Apr 4, 2021

@rw-access Thanks for your benchmark report. I make a new report using benchstat command.

$ benchstat router01.txt router02.txt router03.txt 
name \ time/op    router01.txt  router02.txt  router03.txt
Gin_Param          61.7ns ± 0%   66.9ns ± 0%   66.2ns ± 0%
Gin_Param5          108ns ± 0%    112ns ± 0%    117ns ± 0%
Gin_Param20         291ns ± 0%    287ns ± 0%    307ns ± 0%
Gin_ParamWrite      122ns ± 0%    122ns ± 0%    119ns ± 0%
Gin_GithubStatic   75.7ns ± 0%   81.8ns ± 0%   81.2ns ± 0%
Gin_GithubParam     124ns ± 0%    127ns ± 0%    126ns ± 0%
Gin_GithubAll      27.6µs ± 0%   26.3µs ± 0%   26.8µs ± 0%
Gin_GPlusStatic    60.0ns ± 0%   62.8ns ± 0%   65.8ns ± 0%
Gin_GPlusParam     78.1ns ± 0%   84.2ns ± 0%   83.8ns ± 0%
Gin_GPlus2Params    101ns ± 0%    113ns ± 0%    105ns ± 0%
Gin_GPlusAll       1.11µs ± 0%   1.25µs ± 0%   1.14µs ± 0%
Gin_ParseStatic    63.6ns ± 0%   65.3ns ± 0%   63.5ns ± 0%
Gin_ParseParam     73.8ns ± 0%   71.2ns ± 0%   70.5ns ± 0%
Gin_Parse2Params   83.7ns ± 0%   83.7ns ± 0%   84.3ns ± 0%
Gin_ParseAll       1.94µs ± 0%   1.99µs ± 0%   1.98µs ± 0%
Gin_StaticAll      19.1µs ± 0%   19.5µs ± 0%   18.4µs ± 0%

name \ alloc/op   router01.txt  router02.txt  router03.txt
Gin_Param           0.00B         0.00B         0.00B     
Gin_Param5          0.00B         0.00B         0.00B     
Gin_Param20         0.00B         0.00B         0.00B     
Gin_ParamWrite      0.00B         0.00B         0.00B     
Gin_GithubStatic    0.00B         0.00B         0.00B     
Gin_GithubParam     0.00B         0.00B         0.00B     
Gin_GithubAll       0.00B         0.00B         0.00B     
Gin_GPlusStatic     0.00B         0.00B         0.00B     
Gin_GPlusParam      0.00B         0.00B         0.00B     
Gin_GPlus2Params    0.00B         0.00B         0.00B     
Gin_GPlusAll        0.00B         0.00B         0.00B     
Gin_ParseStatic     0.00B         0.00B         0.00B     
Gin_ParseParam      0.00B         0.00B         0.00B     
Gin_Parse2Params    0.00B         0.00B         0.00B     
Gin_ParseAll        0.00B         0.00B         0.00B     
Gin_StaticAll       0.00B         0.00B         0.00B     

name \ allocs/op  router01.txt  router02.txt  router03.txt
Gin_Param            0.00          0.00          0.00     
Gin_Param5           0.00          0.00          0.00     
Gin_Param20          0.00          0.00          0.00     
Gin_ParamWrite       0.00          0.00          0.00     
Gin_GithubStatic     0.00          0.00          0.00     
Gin_GithubParam      0.00          0.00          0.00     
Gin_GithubAll        0.00          0.00          0.00     
Gin_GPlusStatic      0.00          0.00          0.00     
Gin_GPlusParam       0.00          0.00          0.00     
Gin_GPlus2Params     0.00          0.00          0.00     
Gin_GPlusAll         0.00          0.00          0.00     
Gin_ParseStatic      0.00          0.00          0.00     
Gin_ParseParam       0.00          0.00          0.00     
Gin_Parse2Params     0.00          0.00          0.00     
Gin_ParseAll         0.00          0.00          0.00     
Gin_StaticAll        0.00          0.00          0.00
  • router01: for v1.6.3
  • router02: for master using commit a331dc6
  • router03: Benchmarks for rw-access:port-httprouter-329 using commit 32b2982

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, thanks!

@appleboy appleboy merged commit f3de813 into gin-gonic:master Apr 6, 2021
@Tevic
Copy link
Contributor

Tevic commented Apr 7, 2021

Router1 "/get/test/abc/"
Router2 "/get/:param/abc/"

I tests these two routers with some requests:
/get/test/abc/ -> Router1
/get/xyz/abc/ -> Router2
/get/tt/abc/ -> 404(Expect Router2)
So, if there are some param starts with the prefix 't' in this example, it will not get the right router.

@rw-access
Copy link
Contributor Author

ah, thanks @Tevic. I think it has to do with node.indices. I have a guess and will take a look and add some tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment