Repository: gin-gonic/gin
Pull request #3502 (2023-02-16 13:30:23)

fix(tree): correctly expand the capacity of params
Even though #2690 was fixed in #2755, the solution fixes the panic but doesn't actually expand the capacity of the params slice which means that the handler never gets those parameters in `context.Params` (as pointed out in https://github.com/gin-gonic/gin/issues/2690#issuecomment-870207672). I have also added a router test to illustrate this - without the changes in `tree.go` it fails:

```
=== RUN   TestRouteParamsNotEmpty
    routes_test.go:326:
                Error Trace:    /repos/gin/routes_test.go:326
                Error:          Not equal:
                                expected: "john"
                                actual  : ""

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -john
                                +
                Test:           TestRouteParamsNotEmpty
    routes_test.go:327:
                Error Trace:    /repos/gin/routes_test.go:327
                Error:          Not equal:
                                expected: "smith"
                                actual  : ""

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -smith
                                +
                Test:           TestRouteParamsNotEmpty
--- FAIL: TestRouteParamsNotEmpty (0.00s)
```


codecov[bot]: # [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/3502?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) Report
> Merging [#3502](https://codecov.io/gh/gin-gonic/gin/pull/3502?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) (f949602) into [master](https://codecov.io/gh/gin-gonic/gin/commit/d07db174acf44bfaf191ca2f6d7beafa2ff946da?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) (d07db17) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##           master    #3502   +/-   ##
=======================================
  Coverage   98.63%   98.64%           
=======================================
  Files          42       42           
  Lines        3148     3163   +15     
=======================================
+ Hits         3105     3120   +15     
  Misses         29       29           
  Partials       14       14           
```

| Flag | Coverage Δ | |
|---|---|---|
|  | `98.64% <100.00%> (+<0.01%)` | :arrow_up: |
| go-1.16 | `?` | |
| go-1.17 | `98.54% <100.00%> (+<0.01%)` | :arrow_up: |
| go-1.18 | `98.54% <100.00%> (+<0.01%)` | :arrow_up: |
| go-1.19 | `98.64% <100.00%> (+<0.01%)` | :arrow_up: |
| go-1.20 | `?` | |
| macos-latest | `?` | |
| ubuntu-latest | `98.64% <100.00%> (+<0.01%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/gin-gonic/gin/pull/3502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic) | Coverage Δ | |
|---|---|---|
| [tree.go](https://codecov.io/gh/gin-gonic/gin/pull/3502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#diff-dHJlZS5nbw==) | `100.00% <100.00%> (ø)` | |
| [gin.go](https://codecov.io/gh/gin-gonic/gin/pull/3502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic#diff-Z2luLmdv) | `99.19% <0.00%> (+<0.01%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gin-gonic)

cyryl-form3: Hello dear folks, hope you're having a great day ! 
Is this PR something you would be willing to consider including/merging ? :) 
appleboy: @georgijd-form3 Can you add a performance report?

Patch:
diff --git a/tree.go b/tree.go
index dda8f4f..7b1e008 100644
--- a/tree.go
+++ b/tree.go
@@ -497,7 +497,14 @@ walk: // Outer loop for walking the tree
 					}
 
 					// Save param value
-					if params != nil && cap(*params) > 0 {
+					if params != nil {
+						// Preallocate capacity if necessary
+						if cap(*params) < int(globalParamsCount) {
+							newParams := make(Params, len(*params), globalParamsCount)
+							copy(newParams, *params)
+							*params = newParams
+						}
+
 						if value.params == nil {
 							value.params = params
 						}
@@ -544,6 +551,13 @@ walk: // Outer loop for walking the tree
 				case catchAll:
 					// Save param value
 					if params != nil {
+						// Preallocate capacity if necessary
+						if cap(*params) < int(globalParamsCount) {
+							newParams := make(Params, len(*params), globalParamsCount)
+							copy(newParams, *params)
+							*params = newParams
+						}
+
 						if value.params == nil {
 							value.params = params
 						}


Test patch:
diff --git a/routes_test.go b/routes_test.go
index 633c0ab..7a51f81 100644
--- a/routes_test.go
+++ b/routes_test.go
@@ -337,6 +337,45 @@ func TestRouteParamsByNameWithExtraSlash(t *testing.T) {
 	assert.Equal(t, "/is/super/great", wild)
 }
 
+// TestRouteParamsNotEmpty tests that context parameters will be set
+// even if a route with params/wildcards is registered after the context
+// initialisation (which happened in a previous requets).
+func TestRouteParamsNotEmpty(t *testing.T) {
+	name := ""
+	lastName := ""
+	wild := ""
+	router := New()
+
+	w := PerformRequest(router, http.MethodGet, "/test/john/smith/is/super/great")
+
+	assert.Equal(t, http.StatusNotFound, w.Code)
+
+	router.GET("/test/:name/:last_name/*wild", func(c *Context) {
+		name = c.Params.ByName("name")
+		lastName = c.Params.ByName("last_name")
+		var ok bool
+		wild, ok = c.Params.Get("wild")
+
+		assert.True(t, ok)
+		assert.Equal(t, name, c.Param("name"))
+		assert.Equal(t, lastName, c.Param("last_name"))
+
+		assert.Empty(t, c.Param("wtf"))
+		assert.Empty(t, c.Params.ByName("wtf"))
+
+		wtf, ok := c.Params.Get("wtf")
+		assert.Empty(t, wtf)
+		assert.False(t, ok)
+	})
+
+	w = PerformRequest(router, http.MethodGet, "/test/john/smith/is/super/great")
+
+	assert.Equal(t, http.StatusOK, w.Code)
+	assert.Equal(t, "john", name)
+	assert.Equal(t, "smith", lastName)
+	assert.Equal(t, "/is/super/great", wild)
+}
+
 // TestHandleStaticFile - ensure the static file handles properly
 func TestRouteStaticFile(t *testing.T) {
 	// SETUP file
diff --git a/tree_test.go b/tree_test.go
index 2005738..aacc914 100644
--- a/tree_test.go
+++ b/tree_test.go
@@ -893,9 +893,9 @@ func TestTreeInvalidNodeType(t *testing.T) {
 
 func TestTreeInvalidParamsType(t *testing.T) {
 	tree := &node{}
-	tree.wildChild = true
-	tree.children = append(tree.children, &node{})
-	tree.children[0].nType = 2
+	// add a child with wildcard
+	route := "/:path"
+	tree.addRoute(route, fakeHandler(route))
 
 	// set invalid Params type
 	params := make(Params, 0)
@@ -904,6 +904,34 @@ func TestTreeInvalidParamsType(t *testing.T) {
 	tree.getValue("/test", &params, getSkippedNodes(), false)
 }
 
+func TestTreeExpandParamsCapacity(t *testing.T) {
+	data := []struct {
+		path string
+	}{
+		{"/:path"},
+		{"/*path"},
+	}
+
+	for _, item := range data {
+		tree := &node{}
+		tree.addRoute(item.path, fakeHandler(item.path))
+		params := make(Params, 0)
+
+		value := tree.getValue("/test", &params, getSkippedNodes(), false)
+
+		if value.params == nil {
+			t.Errorf("Expected %s params to be set, but they weren't", item.path)
+			continue
+		}
+
+		if len(*value.params) != 1 {
+			t.Errorf("Wrong number of %s params: got %d, want %d",
+				item.path, len(*value.params), 1)
+			continue
+		}
+	}
+}
+
 func TestTreeWildcardConflictEx(t *testing.T) {
 	conflicts := [...]struct {
 		route        string