Skip to content

Commit

Permalink
Fix labstack#1729 Binding query/path params and form fields to struct…
Browse files Browse the repository at this point in the history
… only works for explicit tags (labstack#1734)

* Binding query/path params and form fields to struct only works for fields that have explicit TAG defined on struct
* remove unnecessary benchmark after change because it is not valid test anymore
  • Loading branch information
aldas authored Jan 5, 2021
1 parent f718079 commit 02ed3f3
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 35 deletions.
16 changes: 9 additions & 7 deletions bind.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,13 @@ func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
return b.BindBody(c, i)
}

func (b *DefaultBinder) bindData(ptr interface{}, data map[string][]string, tag string) error {
if ptr == nil || len(data) == 0 {
// bindData will bind data ONLY fields in destination struct that have EXPLICIT tag
func (b *DefaultBinder) bindData(destination interface{}, data map[string][]string, tag string) error {
if destination == nil || len(data) == 0 {
return nil
}
typ := reflect.TypeOf(ptr).Elem()
val := reflect.ValueOf(ptr).Elem()
typ := reflect.TypeOf(destination).Elem()
val := reflect.ValueOf(destination).Elem()

// Map
if typ.Kind() == reflect.Map {
Expand All @@ -146,14 +147,15 @@ func (b *DefaultBinder) bindData(ptr interface{}, data map[string][]string, tag
inputFieldName := typeField.Tag.Get(tag)

if inputFieldName == "" {
inputFieldName = typeField.Name
// If tag is nil, we inspect if the field is a struct.
// If tag is nil, we inspect if the field is a not BindUnmarshaler struct and try to bind data into it (might contains fields with tags).
// structs that implement BindUnmarshaler are binded only when they have explicit tag
if _, ok := structField.Addr().Interface().(BindUnmarshaler); !ok && structFieldKind == reflect.Struct {
if err := b.bindData(structField.Addr().Interface(), data, tag); err != nil {
return err
}
continue
}
// does not have explicit tag and is not an ordinary struct - so move to next field
continue
}

inputValue, exists := data[inputFieldName]
Expand Down
88 changes: 60 additions & 28 deletions bind_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,31 @@ var values = map[string][]string{
"ST": {"bar"},
}

func TestToMultipleFields(t *testing.T) {
e := New()
req := httptest.NewRequest(http.MethodGet, "/?id=1&ID=2", nil)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

type Root struct {
ID int64 `query:"id"`
Child2 struct {
ID int64
}
Child1 struct {
ID int64 `query:"id"`
}
}

u := new(Root)
err := c.Bind(u)
if assert.NoError(t, err) {
assert.Equal(t, int64(1), u.ID) // perfectly reasonable
assert.Equal(t, int64(1), u.Child1.ID) // untagged struct containing tagged field gets filled (by tag)
assert.Equal(t, int64(0), u.Child2.ID) // untagged struct containing untagged field should not be bind
}
}

func TestBindJSON(t *testing.T) {
assert := assert.New(t)
testBindOkay(assert, strings.NewReader(userJSON), MIMEApplicationJSON)
Expand Down Expand Up @@ -238,10 +263,13 @@ func TestBindUnmarshalParam(t *testing.T) {
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)
result := struct {
T Timestamp `query:"ts"`
TA []Timestamp `query:"ta"`
SA StringArray `query:"sa"`
ST Struct
T Timestamp `query:"ts"`
TA []Timestamp `query:"ta"`
SA StringArray `query:"sa"`
ST Struct
StWithTag struct {
Foo string `query:"st"`
}
}{}
err := c.Bind(&result)
ts := Timestamp(time.Date(2016, 12, 6, 19, 9, 5, 0, time.UTC))
Expand All @@ -252,7 +280,8 @@ func TestBindUnmarshalParam(t *testing.T) {
assert.Equal(ts, result.T)
assert.Equal(StringArray([]string{"one", "two", "three"}), result.SA)
assert.Equal([]Timestamp{ts, ts}, result.TA)
assert.Equal(Struct{"baz"}, result.ST)
assert.Equal(Struct{""}, result.ST) // child struct does not have a field with matching tag
assert.Equal("baz", result.StWithTag.Foo) // child struct has field with matching tag
}
}

Expand All @@ -274,7 +303,7 @@ func TestBindUnmarshalText(t *testing.T) {
assert.Equal(t, ts, result.T)
assert.Equal(t, StringArray([]string{"one", "two", "three"}), result.SA)
assert.Equal(t, []time.Time{ts, ts}, result.TA)
assert.Equal(t, Struct{"baz"}, result.ST)
assert.Equal(t, Struct{""}, result.ST) // field in child struct does not have tag
}
}

Expand Down Expand Up @@ -323,11 +352,27 @@ func TestBindUnsupportedMediaType(t *testing.T) {
}

func TestBindbindData(t *testing.T) {
assert := assert.New(t)
a := assert.New(t)
ts := new(bindTestStruct)
b := new(DefaultBinder)
b.bindData(ts, values, "form")
assertBindTestStruct(assert, ts)
err := b.bindData(ts, values, "form")
a.NoError(err)

a.Equal(0, ts.I)
a.Equal(int8(0), ts.I8)
a.Equal(int16(0), ts.I16)
a.Equal(int32(0), ts.I32)
a.Equal(int64(0), ts.I64)
a.Equal(uint(0), ts.UI)
a.Equal(uint8(0), ts.UI8)
a.Equal(uint16(0), ts.UI16)
a.Equal(uint32(0), ts.UI32)
a.Equal(uint64(0), ts.UI64)
a.Equal(false, ts.B)
a.Equal(float32(0), ts.F32)
a.Equal(float64(0), ts.F64)
a.Equal("", ts.S)
a.Equal("", ts.cantSet)
}

func TestBindParam(t *testing.T) {
Expand Down Expand Up @@ -470,20 +515,6 @@ func TestBindSetFields(t *testing.T) {
}
}

func BenchmarkBindbindData(b *testing.B) {
b.ReportAllocs()
assert := assert.New(b)
ts := new(bindTestStruct)
binder := new(DefaultBinder)
var err error
b.ResetTimer()
for i := 0; i < b.N; i++ {
err = binder.bindData(ts, values, "form")
}
assert.NoError(err)
assertBindTestStruct(assert, ts)
}

func BenchmarkBindbindDataWithTags(b *testing.B) {
b.ReportAllocs()
assert := assert.New(b)
Expand Down Expand Up @@ -560,8 +591,9 @@ func TestDefaultBinder_BindToStructFromMixedSources(t *testing.T) {
// these tests are to document this behaviour and detect further possible regressions when bind implementation is changed

type Opts struct {
ID int `json:"id"`
Node string `json:"node"`
ID int `json:"id" form:"id" query:"id"`
Node string `json:"node" form:"node" query:"node" param:"node"`
Lang string
}

var testCases = []struct {
Expand Down Expand Up @@ -727,8 +759,8 @@ func TestDefaultBinder_BindBody(t *testing.T) {
// these tests are to document this behaviour and detect further possible regressions when bind implementation is changed

type Node struct {
ID int `json:"id" xml:"id"`
Node string `json:"node" xml:"node"`
ID int `json:"id" xml:"id" form:"id" query:"id"`
Node string `json:"node" xml:"node" form:"node" query:"node" param:"node"`
}
type Nodes struct {
Nodes []Node `xml:"node" form:"node"`
Expand Down Expand Up @@ -824,7 +856,7 @@ func TestDefaultBinder_BindBody(t *testing.T) {
expectError: "code=400, message=Syntax error: line=1, error=XML syntax error on line 1: unexpected EOF, internal=XML syntax error on line 1: unexpected EOF",
},
{
name: "ok, FORM POST bind to struct with: path + query + empty body",
name: "ok, FORM POST bind to struct with: path + query + body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationForm,
Expand Down

0 comments on commit 02ed3f3

Please sign in to comment.