Merge pull request #1727 from aldas/bind_query_when_get_delete

Bind query params only for HTTP GET/DELETE methods

Author: lammel

Makefile

@@ -1,3 +1,27 @@
+PKG := "github.com/labstack/echo"
+PKG_LIST := $(shell go list ${PKG}/...)
+
 tag:
 	@git tag `grep -P '^\tversion = ' echo.go|cut -f2 -d'"'`
 	@git tag|grep -v ^v
+
+.DEFAULT_GOAL := check
+check: lint vet race ## Check project
+
+init:
+	@go get -u golang.org/x/lint/golint
+
+lint: ## Lint the files
+	@golint -set_exit_status ${PKG_LIST}
+
+vet: ## Vet the files
+	@go vet ${PKG_LIST}
+
+test: ## Run tests
+	@go test -short ${PKG_LIST}
+
+race: ## Run tests with data race detector
+	@go test -race ${PKG_LIST}
+
+help: ## Display this help screen
+	@grep -h -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'

bind.go

@@ -98,12 +98,20 @@ func (b *DefaultBinder) BindBody(c Context, i interface{}) (err error) {
 }
 
 // Bind implements the `Binder#Bind` function.
+// Binding is done in following order: 1) path params; 2) query params; 3) request body. Each step COULD override previous
+// step binded values. For single source binding use their own methods BindBody, BindQueryParams, BindPathParams.
 func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
 	if err := b.BindPathParams(c, i); err != nil {
 		return err
 	}
-	if err = b.BindQueryParams(c, i); err != nil {
-		return err
+	// Issue #1670 - Query params are binded only for GET/DELETE and NOT for usual request with body (POST/PUT/PATCH)
+	// Reasoning here is that parameters in query and bind destination struct could have UNEXPECTED matches and results due that.
+	// i.e. is `&id=1&lang=en` from URL same as `{"id":100,"lang":"de"}` request body and which one should have priority when binding.
+	// This HTTP method check restores pre v4.1.11 behavior and avoids different problems when query is mixed with body
+	if c.Request().Method == http.MethodGet || c.Request().Method == http.MethodDelete {
+		if err = b.BindQueryParams(c, i); err != nil {
+			return err
+		}
 	}
 	return b.BindBody(c, i)
 }

bind_test.go

@@ -559,7 +559,7 @@ func TestDefaultBinder_BindToStructFromMixedSources(t *testing.T) {
 	// binding is done in steps and one source could overwrite previous source binded data
 	// these tests are to document this behaviour and detect further possible regressions when bind implementation is changed
 
-	type Node struct {
+	type Opts struct {
 		ID   int    `json:"id"`
 		Node string `json:"node"`
 	}
@@ -575,59 +575,105 @@ func TestDefaultBinder_BindToStructFromMixedSources(t *testing.T) {
 		expectError      string
 	}{
 		{
-			name:         "ok, POST bind to struct with: path param + query param + empty body",
+			name:         "ok, POST bind to struct with: path param + query param + body",
 			givenMethod:  http.MethodPost,
 			givenURL:     "/api/real_node/endpoint?node=xxx",
 			givenContent: strings.NewReader(`{"id": 1}`),
-			expect:       &Node{ID: 1, Node: "xxx"}, // in current implementation query params has higher priority than path params
+			expect:       &Opts{ID: 1, Node: "node_from_path"}, // query params are not used, node is filled from path
+		},
+		{
+			name:         "ok, PUT bind to struct with: path param + query param + body",
+			givenMethod:  http.MethodPut,
+			givenURL:     "/api/real_node/endpoint?node=xxx",
+			givenContent: strings.NewReader(`{"id": 1}`),
+			expect:       &Opts{ID: 1, Node: "node_from_path"}, // query params are not used
+		},
+		{
+			name:         "ok, GET bind to struct with: path param + query param + body",
+			givenMethod:  http.MethodGet,
+			givenURL:     "/api/real_node/endpoint?node=xxx",
+			givenContent: strings.NewReader(`{"id": 1}`),
+			expect:       &Opts{ID: 1, Node: "xxx"}, // query overwrites previous path value
+		},
+		{
+			name:         "ok, GET bind to struct with: path param + query param + body",
+			givenMethod:  http.MethodGet,
+			givenURL:     "/api/real_node/endpoint?node=xxx",
+			givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
+			expect:       &Opts{ID: 1, Node: "zzz"}, // body is binded last and overwrites previous (path,query) values
+		},
+		{
+			name:         "ok, DELETE bind to struct with: path param + query param + body",
+			givenMethod:  http.MethodDelete,
+			givenURL:     "/api/real_node/endpoint?node=xxx",
+			givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
+			expect:       &Opts{ID: 1, Node: "zzz"}, // for DELETE body is binded after query params
 		},
 		{
-			name:         "ok, POST bind to struct with: path param + empty body",
+			name:         "ok, POST bind to struct with: path param + body",
 			givenMethod:  http.MethodPost,
 			givenURL:     "/api/real_node/endpoint",
 			givenContent: strings.NewReader(`{"id": 1}`),
-			expect:       &Node{ID: 1, Node: "real_node"},
+			expect:       &Opts{ID: 1, Node: "node_from_path"},
 		},
 		{
 			name:         "ok, POST bind to struct with path + query + body = body has priority",
 			givenMethod:  http.MethodPost,
 			givenURL:     "/api/real_node/endpoint?node=xxx",
 			givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
-			expect:       &Node{ID: 1, Node: "zzz"}, // field value from content has higher priority
+			expect:       &Opts{ID: 1, Node: "zzz"}, // field value from content has higher priority
 		},
 		{
 			name:         "nok, POST body bind failure",
 			givenMethod:  http.MethodPost,
 			givenURL:     "/api/real_node/endpoint?node=xxx",
 			givenContent: strings.NewReader(`{`),
-			expect:       &Node{ID: 0, Node: "xxx"}, // query binding has already modified bind target
+			expect:       &Opts{ID: 0, Node: "node_from_path"}, // query binding has already modified bind target
 			expectError:  "code=400, message=unexpected EOF, internal=unexpected EOF",
 		},
+		{
+			name:         "nok, GET with body bind failure when types are not convertible",
+			givenMethod:  http.MethodGet,
+			givenURL:     "/api/real_node/endpoint?id=nope",
+			givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
+			expect:       &Opts{ID: 0, Node: "node_from_path"}, // path params binding has already modified bind target
+			expectError:  "code=400, message=strconv.ParseInt: parsing \"nope\": invalid syntax, internal=strconv.ParseInt: parsing \"nope\": invalid syntax",
+		},
 		{
 			name:         "nok, GET body bind failure - trying to bind json array to struct",
 			givenMethod:  http.MethodGet,
 			givenURL:     "/api/real_node/endpoint?node=xxx",
 			givenContent: strings.NewReader(`[{"id": 1}]`),
-			expect:       &Node{ID: 0, Node: "xxx"}, // query binding has already modified bind target
-			expectError:  "code=400, message=Unmarshal type error: expected=echo.Node, got=array, field=, offset=1, internal=json: cannot unmarshal array into Go value of type echo.Node",
+			expect:       &Opts{ID: 0, Node: "xxx"}, // query binding has already modified bind target
+			expectError:  "code=400, message=Unmarshal type error: expected=echo.Opts, got=array, field=, offset=1, internal=json: cannot unmarshal array into Go value of type echo.Opts",
 		},
 		{ // binding query params interferes with body. b.BindBody() should be used to bind only body to slice
 			name:             "nok, GET query params bind failure - trying to bind json array to slice",
 			givenMethod:      http.MethodGet,
 			givenURL:         "/api/real_node/endpoint?node=xxx",
 			givenContent:     strings.NewReader(`[{"id": 1}]`),
 			whenNoPathParams: true,
-			whenBindTarget:   &[]Node{},
-			expect:           &[]Node{},
+			whenBindTarget:   &[]Opts{},
+			expect:           &[]Opts{},
 			expectError:      "code=400, message=binding element must be a struct, internal=binding element must be a struct",
 		},
+		{ // binding query params interferes with body. b.BindBody() should be used to bind only body to slice
+			name:             "ok, POST binding to slice should not be affected query params types",
+			givenMethod:      http.MethodPost,
+			givenURL:         "/api/real_node/endpoint?id=nope&node=xxx",
+			givenContent:     strings.NewReader(`[{"id": 1}]`),
+			whenNoPathParams: true,
+			whenBindTarget:   &[]Opts{},
+			expect:           &[]Opts{{ID: 1}},
+			expectError:      "",
+		},
 		{ // binding path params interferes with body. b.BindBody() should be used to bind only body to slice
 			name:           "nok, GET path params bind failure - trying to bind json array to slice",
 			givenMethod:    http.MethodGet,
 			givenURL:       "/api/real_node/endpoint?node=xxx",
 			givenContent:   strings.NewReader(`[{"id": 1}]`),
-			whenBindTarget: &[]Node{},
-			expect:         &[]Node{},
+			whenBindTarget: &[]Opts{},
+			expect:         &[]Opts{},
 			expectError:    "code=400, message=binding element must be a struct, internal=binding element must be a struct",
 		},
 		{
@@ -636,8 +682,8 @@ func TestDefaultBinder_BindToStructFromMixedSources(t *testing.T) {
 			givenURL:         "/api/real_node/endpoint",
 			givenContent:     strings.NewReader(`[{"id": 1}]`),
 			whenNoPathParams: true,
-			whenBindTarget:   &[]Node{},
-			expect:           &[]Node{{ID: 1, Node: ""}},
+			whenBindTarget:   &[]Opts{},
+			expect:           &[]Opts{{ID: 1, Node: ""}},
 			expectError:      "",
 		},
 	}
@@ -653,14 +699,14 @@ func TestDefaultBinder_BindToStructFromMixedSources(t *testing.T) {
 
 			if !tc.whenNoPathParams {
 				c.SetParamNames("node")
-				c.SetParamValues("real_node")
+				c.SetParamValues("node_from_path")
 			}
 
 			var bindTarget interface{}
 			if tc.whenBindTarget != nil {
 				bindTarget = tc.whenBindTarget
 			} else {
-				bindTarget = &Node{}
+				bindTarget = &Opts{}
 			}
 			b := new(DefaultBinder)