Fix the path params slice out of range with the wrong latest node #2820
Conversation
getValue didn't update the latest node after walking through the path and may regard the static route as the param type, but the params cap was prealloc and cause out of range. For Example: the route was "/a/b/:c/d" and we send the request path "/a/b/c/d1", the params cap was 1 but the `d` would also be regarded as param then would be out of range.
|
Hi @git-hulk, We encountered the same issue (while trying to upgrade to gin 1.7.3) but I wasn't sure how to tackle it. So thank you for the fix and the explanations! I guess you could add a test case to diff --git a/gin_integration_test.go b/gin_integration_test.go
index a4149de..cb05f3a 100644
--- a/gin_integration_test.go
+++ b/gin_integration_test.go
@@ -463,4 +463,5 @@ func TestTreeRunDynamicRouting(t *testing.T) {
testRequest(t, ts.URL+"/a/dd", "404 Not Found")
testRequest(t, ts.URL+"/addr/dd/aa", "404 Not Found")
testRequest(t, ts.URL+"/something/secondthing/121", "404 Not Found")
+ testRequest(t, ts.URL+"/cc/dd/ee/ff/gg/hh1", "404 Not Found")
}I hope this fix will be merged/released soon so that we'll be able to upgrade gin beyond 1.7.2. |
Codecov Report
@@ Coverage Diff @@
## master #2820 +/- ##
==========================================
+ Coverage 98.71% 98.75% +0.04%
==========================================
Files 41 41
Lines 2100 3055 +955
==========================================
+ Hits 2073 3017 +944
- Misses 15 26 +11
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
git-hulk
changed the title
|
@appleboy anyone can have a look at this PR? |
|
@git-hulk Thanks for your effort. Can you also provide the benchmark result like this comment: #2767 (comment) Hi @qm012 @rw-access, please also take a look if you have free time. |
Thanks I have been working late these two days. I will do a test on the weekend and give you feedback:eyes: |
|
@git-hulk Post the benchmark result? |
|
@appleboy ok, I’ll post the benchmark result later, sorry for missing your previous comment. |
|
The benchmarks: The benchstat report: |
|
benchmark result between master and PR branch: |
|
@appleboy so should I worry about the performance degrade? |
hi @appleboy Migration is done because it affects route matching performance and makes multiple circular calls.If fixing this bug results in such a large performance difference, I think it is not worth the cost, so we can avoid this bug in route definition for the time being until we have a better solution. Test report after migration #2796 (comment) Before the migration |
|
@qm012 We should NOT sacrifice the correctness to promise the performance, right? |
They are compatible, so we need to talk about them. |
There are many ways to solve a problem. |
|
Issue still exists, thus i am frozen at gin 1.7.2... see #2878 |
I'll have to downgrade, too ... weird how this takes 2 months ... :( |
|
IMHO, this PR only intended to fix the bug, the performance data didn't mean this PR cause performance to degrade(I mean the old performance should be wrong since the logic has a bug), so we should fix the bug first then optimize the performance if need. HDYT @appleboy |
|
See solution #2897 already merged in the master branch. |
getValue didn't update the latest node after walking through
the path and may regard the static route as the param type,
but the params cap was preallocated and cause out of range.
For Example:
the route was "/:a/:b/:c/d" and we send the request path "/a/b/c/d1",
the params cap was 3 but the
dwould also be regarded as param thenwould be out of range.