diff --git a/binding/all.go b/binding/all.go index dd2a3ad8..230c5b9d 100644 --- a/binding/all.go +++ b/binding/all.go @@ -28,25 +28,22 @@ func (allBinding) BindMany(req *http.Request, uriParams map[string][]string, obj return err } - // from binding.Query - values := req.URL.Query() - if err := mapForm(obj, values); err != nil { - return err - } - // from context.Bind (for body/post-form/anything else) contentType := req.Header.Get("Content-Type") contentTypeLastIdx := strings.IndexAny(contentType, " ;") // trim "application/json; charset=utf-8" -> "application/json" if contentTypeLastIdx != -1 { contentType = contentType[:contentTypeLastIdx] } + b := Default(req.Method, contentType) - // if no Content-Type, assume request has no body. This avoids binding query params again in binding.Default - if contentType == "" { - return validate(obj) + // Since req.ParseForm() reads body and query, check whether the selected binding includes query parsing before parsing query values explicitly. + if !bindingIncludesQueryParsing(b) { + // from binding.Query + if err := mapForm(obj, req.URL.Query()); err != nil { + return err + } } - b := Default(req.Method, contentType) - // final validation done by whatever binding is selected here + // final validation done by whatever binding was selected by Default return b.Bind(req, obj) } diff --git a/binding/binding.go b/binding/binding.go index 84f4d866..8c7a997a 100644 --- a/binding/binding.go +++ b/binding/binding.go @@ -131,6 +131,16 @@ func Default(method, contentType string) Binding { } } +// bindingIncludesQueryParsing reports whether a binding parses URL query values as part of Bind. +func bindingIncludesQueryParsing(binding Binding) bool { + switch binding { + case Form, FormPost, FormMultipart: + return true + default: + return false + } +} + func validate(obj any) error { if Validator == nil { return nil diff --git a/binding/binding_nomsgpack.go b/binding/binding_nomsgpack.go index 735d0e23..5929b1c8 100644 --- a/binding/binding_nomsgpack.go +++ b/binding/binding_nomsgpack.go @@ -123,6 +123,16 @@ func Default(method, contentType string) Binding { } } +// bindingIncludesQueryParsing reports whether a binding parses URL query values as part of Bind. +func bindingIncludesQueryParsing(binding Binding) bool { + switch binding { + case Form, FormPost, FormMultipart: + return true + default: + return false + } +} + func validate(obj any) error { if Validator == nil { return nil diff --git a/binding/binding_test.go b/binding/binding_test.go index f880cd93..e5d5d7b6 100644 --- a/binding/binding_test.go +++ b/binding/binding_test.go @@ -154,6 +154,10 @@ type FooStructForMapPtrType struct { PtrBar *map[string]any `form:"ptr_bar"` } +type FooStructForAllFormPrecedence struct { + Count int `form:"count" binding:"required"` +} + func TestBindingDefault(t *testing.T) { assert.Equal(t, Form, Default(http.MethodGet, "")) assert.Equal(t, Form, Default(http.MethodGet, MIMEJSON)) @@ -1498,6 +1502,13 @@ func TestBindingAllQuery(t *testing.T) { req = requestWithBody(http.MethodGet, "/?bool_foo=fasl", "") err = b.BindMany(req, nil, &obj2) require.Error(t, err) + + // fail case 2 + obj3 := FooStructForBoolType{} + req = requestWithBody(http.MethodPost, "/?bool_foo=fasl", "{}") + req.Header.Set("Content-Type", MIMEJSON) + err = b.BindMany(req, nil, &obj3) + require.Error(t, err) } func TestBindingAllBody(t *testing.T) { @@ -1518,6 +1529,19 @@ func TestBindingAllBody(t *testing.T) { require.Error(t, err) } +// TestBindingAllFormBodyOverridesInvalidQuery. Since req.ParseForm() reads body and query, invalid queries are replaced +// by valid values in body +func TestBindingAllFormBodyOverridesInvalidQuery(t *testing.T) { + b := All + + obj := FooStructForAllFormPrecedence{} + req := requestWithBody(http.MethodPost, "/?count=invalid", "count=7") + req.Header.Set("Content-Type", MIMEPOSTForm) + err := b.BindMany(req, nil, &obj) + require.NoError(t, err) + assert.Equal(t, 7, obj.Count) +} + func TestBindingAllHeaderUriQueryBody(t *testing.T) { b := All diff --git a/context.go b/context.go index 0106504c..b4b99ca9 100644 --- a/context.go +++ b/context.go @@ -809,9 +809,9 @@ func (c *Context) BindUri(obj any) error { // // Note: // - Caller must tag struct fields appropriately for the desired binding (eg `header:"xxx"` vs `uri:"xxx"`) -// - Caller must ensure no duplication between field names (else use separate binding engines instead) +// - Caller must ensure no duplication between field names. Duplication may result in undefined behavior. (use separate binding engines instead) // - Caller must provide Content-Type header to select the correct body binding (eg "application/json" for JSON binding). -// A request without Content-Type will result in no body binding +// A request without Content-Type will fallback to gin's default behavior for body binding (see ShouldBind) // - Binding validation tags are verified after all request parts have been bound func (c *Context) BindAll(obj any) error { err := c.ShouldBindAll(obj) @@ -947,9 +947,9 @@ func (c *Context) ShouldBindUri(obj any) error { // // Note: // - Caller must tag struct fields appropriately for the desired binding (eg `header:"xxx"` vs `uri:"xxx"`) -// - Caller must ensure no duplication between field names (else use separate binding engines instead) +// - Caller must ensure no duplication between field names. Duplicate fields may result in undefined behavior. (use separate binding engines instead) // - Caller must provide Content-Type header to select the correct body binding (eg "application/json" for JSON binding). -// A request without Content-Type will result in no body binding +// A request without Content-Type will fallback to gin's default behavior for body binding (see ShouldBind) // - Binding validation tags are verified after all request parts have been bound func (c *Context) ShouldBindAll(obj any) error { uriParams := make(map[string][]string, len(c.Params))