Fix router not matching param route end slash and implement matching by path+method match

[aldas]

Fixes:

• when url ends with slash first param route is the match (fix timeout middleware returns wrong HTTP status code #1804)
• router should check if node is suitable as matching route by path+method and if not then continue search in tree (fix The proxy middleware is not forwarding requests #1808)

[codecov]

Codecov Report

Merging #1812 (546a1e5) into master (dec96f0) will increase coverage by 0.00%.
The diff coverage is 95.08%.

@@           Coverage Diff           @@
##           master    #1812   +/-   ##
=======================================
  Coverage   89.49%   89.50%           
=======================================
  Files          32       32           
  Lines        2685     2734   +49     
=======================================
+ Hits         2403     2447   +44     
- Misses        181      185    +4     
- Partials      101      102    +1     

Impacted Files	Coverage Δ
router.go	95.03% <95.08%> (-1.02%)	⬇️

---

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 dec96f0...546a1e5. Read the comment docs.

[aldas]

benchmarks for

go test -run="-" -bench="BenchmarkEcho.*" -count 10
go test -run="-" -bench="BenchmarkRouter.*" -count 10

Results:

x@x:~/code/echo$ benchstat before_fix.out after_fix.out 
name                      old time/op    new time/op    delta
EchoStaticRoutes-6          17.0µs ± 2%    18.4µs ± 1%  +7.69%  (p=0.000 n=9+10)
EchoStaticRoutesMisses-6    17.3µs ± 3%    18.4µs ± 1%  +6.47%  (p=0.000 n=10+10)
EchoGitHubAPI-6             32.5µs ± 3%    34.0µs ± 1%  +4.58%  (p=0.000 n=8+9)
EchoGitHubAPIMisses-6       32.3µs ± 3%    34.3µs ± 1%  +6.46%  (p=0.000 n=10+10)
EchoParseAPI-6              2.04µs ± 2%    2.10µs ± 3%  +2.90%  (p=0.000 n=10+10)
 
name                         old time/op    new time/op    delta
RouterStaticRoutes-6           13.6µs ± 2%    14.8µs ± 1%   +8.51%  (p=0.000 n=10+10)
RouterStaticRoutesMisses-6      525ns ± 2%     586ns ± 3%  +11.71%  (p=0.000 n=10+10)
RouterGitHubAPI-6              25.9µs ± 2%    28.2µs ± 1%   +9.05%  (p=0.000 n=10+10)
RouterGitHubAPIMisses-6         616ns ± 1%     704ns ± 1%  +14.38%  (p=0.000 n=10+8)
RouterParseAPI-6               1.37µs ± 1%    1.46µs ± 2%   +6.86%  (p=0.000 n=10+10)
RouterParseAPIMisses-6          358ns ± 2%     361ns ± 1%   +0.87%  (p=0.004 n=9+10)
RouterGooglePlusAPI-6           882ns ± 2%     955ns ± 3%   +8.35%  (p=0.000 n=10+10)
RouterGooglePlusAPIMisses-6     493ns ± 0%     546ns ± 1%  +10.76%  (p=0.000 n=7+10)
RouterParamsAndAnyAPI-6        2.07µs ± 1%    2.35µs ± 0%  +13.58%  (p=0.000 n=10+8)

so most affected are misses - these are cases when backtracking is done.

[lammel]

Sounds like we already have a plan on how paths are resolved for specific HTTP methods.
I need to check on the tests first.
This is definitely something the must be documented in the release notes and docs.

What are the opinions on changing this behaviour for v4.3 vs v5?

[aldas]

Sounds like we already have a plan on how paths are resolved for specific HTTP methods.
I need to check on the tests first.
This is definitely something the must be documented in the release notes and docs.

What are the opinions on changing this behaviour for v4.3 vs v5?

nah, I am not that sure about it. I just wanted to have a note there. Thing is that Echo has gone so far without having to have more extensive error handling for app "setup" therefore question is if it is necessary at all? At the same time it could be that we have pushed that burden to developer to debug their code and see why some things do not work.

For route building problems I see 3 options:

1. return error immediately (breaking change)
2. store error (like binder) and return on startup (there we are already returning errors) (probably breaking)
3. continue as today. maybe improve by logging errors/warnings

For example if you look what Gin or Chi does - none of them actually return error when adding a route. Even if returning error would be most idiomatic Go. There is always but - but but framework intention is to ease/simplify developer life and we all know that dealing with errors is painful, takes time and consideration which is opposite of simple.

[lammel]

For that we could split it up into 2 stops.

Provide route resolving based on method but

1. for v4.3 silently ignore errors on route building
2. for v5 change adding route to return errors

The changed behaviour is a correction in route handlin (think bugfix) and can be landed in v4.3 with an important note IMHO.

[aldas]

mmm, things in this PR actually does not need to be 4.3 as they are just bugfixes. error handling for router is separate things and is not includeded here and can/should be implemented in a separate PR.

[aldas]

bing @lammel

[lammel]

Changes look good. Haven't done any debug testing, but tests are there and document the changed behavior pretty well.

We have some "breaking" changes actually:

• Correctly match routes with trailing backslash
• Correctly match routes with param node as leaf
• Routes with param node as leaf will capture remaining path into the last param node
• Considering handlers when resolving routes

While the first is a correction and second can be considered fixes to existing bugs in routing, the 3rd point is change in behavior.
As the current misbehavior will not find a matching route, I think it is enough to add that to the release notes.

Did you do any performance tests for the changes? Benchstat tells us of a small drop for some scenarios.

[aldas]

I posted performance stats #1812 (comment) up in this thread. It is below 10% increase but it is expected as previously method match was checked once at the end and now is checked when path is a match and does backtracking when it is but method for that matched route does not exist. This is inherently more work.

I would not say to these are huge breaking changes. more like corner cases. Also Routes with param node as leaf will capture remaining path into the last param node is not new feature - this is how current version acts (on some cases).

I'll add those points as note about routing in changelog