Update the code logic for latestNode in tree.go

[zhuxi0511]

This PR is a new code solution for #2820 and it also solve the issues #2894 #2878

the old latestNode logic should be modified to save a list of node. Then the error match for fixed path can be roll back to the right param node and right path.

the benchmark result:

master:https://app.travis-ci.com/github/zhuxi0511/go-http-routing-benchmark/builds/239484250
this PR:https://app.travis-ci.com/github/zhuxi0511/go-http-routing-benchmark/builds/239484068

name              old time/op    new time/op    delta
Gin_Param           62.8ns ± 1%    64.8ns ± 3%  +3.13%  (p=0.004 n=10+10)
Gin_Param5           118ns ± 3%     115ns ± 3%  -2.96%  (p=0.000 n=10+10)
Gin_Param20          310ns ± 0%     294ns ± 1%  -5.06%  (p=0.000 n=10+9)
Gin_ParamWrite       118ns ± 1%     122ns ± 3%  +3.68%  (p=0.000 n=8+9)
Gin_GithubStatic    75.2ns ± 1%    76.4ns ± 3%    ~     (p=1.000 n=8+10)
Gin_GithubParam      134ns ± 1%     133ns ± 2%    ~     (p=0.824 n=9+10)
Gin_GithubAll       26.0µs ± 0%    26.8µs ± 1%  +2.94%  (p=0.000 n=9+10)
Gin_GPlusStatic     59.0ns ± 0%    59.0ns ± 0%    ~     (p=0.445 n=10+10)
Gin_GPlusParam      84.4ns ± 3%    80.1ns ± 0%  -5.07%  (p=0.000 n=10+10)
Gin_GPlus2Params     109ns ± 2%     102ns ± 1%  -6.13%  (p=0.000 n=10+10)
Gin_GPlusAll        1.14µs ± 0%    1.12µs ± 0%  -1.87%  (p=0.000 n=8+10)
Gin_ParseStatic     59.8ns ± 6%    63.2ns ± 3%  +5.71%  (p=0.000 n=10+10)
Gin_ParseParam      68.9ns ± 2%    71.9ns ± 1%  +4.30%  (p=0.000 n=9+10)
Gin_Parse2Params    85.1ns ± 0%    85.1ns ± 1%    ~     (p=0.138 n=10+10)
Gin_ParseAll        1.93µs ± 0%    2.00µs ± 1%  +3.40%  (p=0.000 n=10+10)
Gin_StaticAll       18.2µs ± 0%    18.0µs ± 0%  -0.98%  (p=0.000 n=10+9)

I am a new goer, so some code may not good enough. Thank you for your help and review.

This PR has 3 similar code section for roll back skippedNode. I just keep this code for simple logic to get right route guarantee. It should be optimized in next PR, Thanks!

[codecov]

Codecov Report

Merging #2897 (7fc975f) into master (5929d52) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2897   +/-   ##
=======================================
  Coverage   98.72%   98.73%           
=======================================
  Files          41       41           
  Lines        3066     3089   +23     
=======================================
+ Hits         3027     3050   +23     
  Misses         27       27           
  Partials       12       12           

Flag	Coverage Δ
go-1.13	98.73% <100.00%> (+<0.01%)	⬆️
go-1.14	98.57% <100.00%> (+0.01%)	⬆️
go-1.15	98.57% <100.00%> (+0.01%)	⬆️
go-1.16	98.57% <100.00%> (+0.01%)	⬆️
go-1.17	98.47% <100.00%> (+0.01%)	⬆️
macos-latest	98.73% <100.00%> (+<0.01%)	⬆️
nomsgpack	98.71% <100.00%> (+<0.01%)	⬆️
ubuntu-latest	98.73% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
context.go	97.82% <100.00%> (+<0.01%)	⬆️
gin.go	99.11% <100.00%> (+0.01%)	⬆️
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 5929d52...7fc975f. Read the comment docs.

[Bisstocuz]

You need to check out the Codecov and refine the unit tests. 😀

[zhuxi0511]

You need to check out the Codecov and refine the unit tests. 😀

Already added more test for codecov test, THX

[appleboy]

LGTM

[Bisstocuz]

Does this PR can fix #2843 #2847 ? 😁
If can, I think maintainers will published a new update after this PR merged.

[zhuxi0511]

Does this PR can fix #2843 #2847 ? 😁 If can, I think maintainers will published a new update after this PR merged.

I have tried the case in #2843. But regret for that this PR cannot fix this issue. It may need other PR to fix. @Bisstocuz

[appleboy]

#2847 PR fixed the #2843 issue

[appleboy]

@thinkerou need your feedbacks.

[thinkerou]

lgtm

[ibraheemdev]

I think there are still some bugs. The following works:

tree := &node{}
tree.addRoute("/get/:param", fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))

But if you change the param name of one of them, it doesn't

tree := &node{}
tree.addRoute("/get/:a" fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))

panic: ':param' in new path '/get/:param/foo' conflicts with existing wildcard ':a' in existing prefix '/get/:a'

[zhuxi0511]

I think there are still some bugs. The following works:

tree := &node{}
tree.addRoute("/get/:param", fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))

But if you change the param name of one of them, it doesn't

tree := &node{}
tree.addRoute("/get/:a" fakeHandler("1"))
tree.addRoute("/get/:param/foo", fakeHandler("1"))

panic: ':param' in new path '/get/:param/foo' conflicts with existing wildcard ':a' in existing prefix '/get/:a'

@ibraheemdev this is not a bug. It's a restriction for param route in my mind.
And this panic happened in the tree building progress, the earlier version of gin should be same behavior. You can have a try.

[ibraheemdev]

I guess it's not a bug, but it's something I would expect to be supported. Would the current backtracking logic handle this?

[zhuxi0511]

I guess it's not a bug, but it's something I would expect to be supported. Would the current backtracking logic handle this?

The current logic can only process the tree path match function, and this all after tree building progress.
And that panic happened in the tree building progress. They are in two different stages. So you may need a new PR~

[ibraheemdev]

Yes, but if the backtracking logic can handle those routes, then only the insert function has to be changed.