Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a new config to DefaultBinder #1675

Closed
wants to merge 4 commits into from
Closed

Adding a new config to DefaultBinder #1675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e63fa85
Adding a new config to DefaultBinder
pafuent Nov 10, 2020
9ba1225
Rewriting DefaultBinder#AvoidBindByFieldName doc
pafuent Nov 11, 2020
f6bd7bd
Adding DefaultBinder flags to prevent methods of binding
pafuent Nov 13, 2020
eb46e03
Merge branch 'master' into adding_bind_by_id_config_to_default_binder
pafuent Nov 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions bind.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,19 @@ type (
}

// DefaultBinder is the default implementation of the Binder interface.
DefaultBinder struct{}
DefaultBinder struct {
// AvoidBindByFieldName avoid binding struct fields by name automatically. If it's set to true, the binding is
// only performed when a valid tag is pressent.
// This doesn't apply for json/xml encoding. In this case you should use the features provided in the Go stdlib
// to achieve this. e.g. If you want to ignore a struct field during binding, you should add the tag `json:"-"`
AvoidBindByFieldName bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pafuent please add tests with http.Request objects also. For example POST body in bind to struct with

echo/bind.go

Line 55 in ceffc10

if err = json.NewDecoder(req.Body).Decode(i); err != nil {

so it would work only when binding from route params or query params and not from body.

in that sense this comment

		// AvoidBindByFieldName avoid binding struct fields by name automatically. If it's set to true, the binding is
		// only performed when a valid tag is pressent
		AvoidBindByFieldName bool

is not correct, even misleading, because json.NewDecoder(req.Body).Decode(i) would still bind to public field even without tag

test like

func TestBindToStructFromJson(t *testing.T) {
	type User struct {
		ID int `json:"id"`
		IsAdmin bool // field without tag
	}

	e := New()
	req := httptest.NewRequest(http.MethodPost, "/api/endpoint", strings.NewReader(`{"id": 1, "IsAdmin": true}`))
	req.Header.Set(HeaderContentType, MIMEApplicationJSON)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)

	err := func(c Context) error {
		var payload User
		if err := c.Bind(&payload); err != nil {
			return c.JSON(http.StatusBadRequest, Map{"error": err})
		}

		if payload.IsAdmin {
			panic("field is filled by json.decode")
		}

		return c.JSON(http.StatusOK, payload)
	}(c)
	if err != nil {
		t.Fatal(err)
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking a while about this, I couldn't find a way to achieve this desired behavior. There is no way in the Go encoding/json package to ignore fields dynamically, for example using a particular configuration of the Decoder.
Also, I think that the JSON binding should behave as Go encoding/json package defined it, that is what a user of Echo would expect. For that reason I only rewrite the doc of DefaultBinder#AvoidBindByFieldName and I added a new UT that shows how a struct field should be ignored according the Go encoding/json package.
Hope that could satisfy your change request. If you have any idea that I could better adapt to your needs, please don't hesitate and let me know and I'll implement it.

Copy link
Contributor

@aldas aldas Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, I know that json decoder does not allow that wanted to point that out. The commend should probably explicitly say that this flag does not apply to 'request body' (except form). mentioning 'json/xml' is maybe too vague as in domain of HTTP you are primarly dealing with method, urls and bodys - and contents of body is one level 'deeper'.

Copy link
Contributor

@aldas aldas Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pafuent if you already are adding flag to binder why not add flags to disable binding route and query params. This would allow user to customize what is bind

echo/bind.go

Line 52 in 9ba1225

if err = b.bindData(i, c.QueryParams(), "query"); err != nil { 

	if !b.DisableRouteParamsBinding {
		if err := b.bindData(i, params, "param"); err != nil {
			return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
		}
	}
	if !b.DisableQueryParamsBinding {
		if err = b.bindData(i, c.QueryParams(), "query"); err != nil {
			return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
		}
	}
	if b.DisableBodyBinding || req.ContentLength == 0 {
		return
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but from my understanding those flags are not needed, due to the new flag that I added. If you don't have a tag on your struct field and AvoidBindByFieldName set to true, that field won't be bind, unless you are dealing with json/xml (which is what I tried to capture in my comment, if the wording is not accurate, please let me know and I'm glad to change it)
I'm not against adding those flags, it's just seems that their are not needed and making that change will could generate a performance impact for every bind on the server (maybe small, I'm not an expert on the cost of multiple if statements)

Copy link
Contributor

@aldas aldas Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change will could generate a performance impact for every bind on the server

This would allow user to switch off bindData method for them in cases (json POST for example hardly ever needs to bind query params actually). What bindData does is more expensive than one boolean check ever does.

by adding if to bindData for i := 0; i < typ.NumField(); i++ { loop
https://github.com/labstack/echo/pull/1675/files#diff-aade326d3512b5a2ada6faa791ddec468f2a0adedb352339c9e314e74c8949d2L122

} else if b.AvoidBindByFieldName == false {

this means that this if is checked for every (settable) struct field for both route and query params. Assuming struct we are binding to has 10 fields this would mean 10+10 IFs for one request. This is more than 3 IFs to see if bindData is needed at all. In that context these 3 ifs does not matter much

from my understanding those flags are not needed

These flags would allow user precise control over where things for binding are taken by switching of things that should not be used. When your endpoint is json post you most of the time do not expect it to bind anything from query.

Also see this example. AvoidBindByFieldName does not fix the need that sometimes you would like not to bind query params and like to bind route params. In this example first test fails because AvoidBindByFieldName switches off route params binding and second test fails because query params have higher priority than route params.

func TestDefaultBinder_Bind(t *testing.T) {
	type Node struct {
		ID   int    `json:"id"`
		Node string `json:"node"`
	}

	var testCases = []struct {
		name                     string
		givenURL                 string
		givenContent             io.Reader
		givenMethod              string
		whenAvoidBindByFieldName bool
		expectNode               string
	}{
		{
			name:                     "bind to struct with route param + query param and AvoidBindByFieldName=true",
			givenMethod:              http.MethodPost,
			givenURL:                 "/api/real_node/endpoint?node=xxx",
			givenContent:             strings.NewReader(`{"id": 1}`),
			whenAvoidBindByFieldName: true,
			expectNode:               "real_node", // why is route param not bind?
		},
		{
			name:         "bind to struct with route param + query param",
			givenMethod:  http.MethodPost,
			givenURL:     "/api/real_node/endpoint?node=xxx",
			givenContent: strings.NewReader(`{"id": 1}`),
			expectNode:   "real_node", // why is route param overwritten by query param?
		},
		{
			name:         "bind to struct with route + query + body = body has priority, AvoidBindByFieldName=false",
			givenMethod:  http.MethodPost,
			givenURL:     "/api/real_node/endpoint?node=xxx",
			givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
			expectNode:   "zzz",
		},
		{
			name:                     "bind to struct with route + query + body = body has priority, AvoidBindByFieldName=true",
			givenMethod:              http.MethodPost,
			givenURL:                 "/api/real_node/endpoint?node=xxx",
			givenContent:             strings.NewReader(`{"id": 1, "node": "zzz"}`),
			whenAvoidBindByFieldName: true,
			expectNode:               "zzz",
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			e := New()
			// assume route we are testing is "/api/:node/endpoint"
			req := httptest.NewRequest(tc.givenMethod, tc.givenURL, tc.givenContent)
			req.Header.Set(HeaderContentType, MIMEApplicationJSON)
			rec := httptest.NewRecorder()
			c := e.NewContext(req, rec)

			c.SetParamNames("node")
			c.SetParamValues("real_node")

			defaultUser := &Node{}
			b := new(DefaultBinder)
			b.AvoidBindByFieldName = tc.whenAvoidBindByFieldName

			err := b.Bind(defaultUser, c)
			if err != nil {
				t.Fatal(err)
			}
			assert.Equal(t, tc.expectNode, defaultUser.Node)
		})
	}
}

Copy link
Contributor

@aldas aldas Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think root cause for many todays problems is still b129098 which caused bindData to be used for POST/PUT methods. Before that query params was bind only for GET/DELETE. We could say that even this change is slight performance degradation for POST/PUTs as post with query params means that now we are reflecting to see if we could fill struct with query params (before decoding body content to struct)

before:

	if req.ContentLength == 0 {
		if req.Method == http.MethodGet || req.Method == http.MethodDelete {
			if err = b.bindData(i, c.QueryParams(), "query"); err != nil {
				return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
			}
			return
		}
		return NewHTTPError(http.StatusBadRequest, "Request body can't be empty")
	}
	ctype := req.Header.Get(HeaderContentType)

now:

	}
	if err = b.bindData(i, c.QueryParams(), "query"); err != nil {
		return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
	}
	if req.ContentLength == 0 {
		return
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the testing code, It help me a lot to understand better your point.
I'll try to answer your questions:

		{
			name:                     "bind to struct with route param + query param and AvoidBindByFieldName=true",
			givenMethod:              http.MethodPost,
			givenURL:                 "/api/real_node/endpoint?node=xxx",
			givenContent:             strings.NewReader(`{"id": 1}`),
			whenAvoidBindByFieldName: true,
			expectNode:               "real_node", // why is route param not bind?
		},

Why is route param not bind?
That happens because the Node struct didn't say that. You should tag the Node field as json:"node" param:"node"
So in this case, Echo is giving to the developer the knobs that he/she needs to properly configure the binding.
For this is reason is that I believe that those flags are not needed. If you just add tags to your structs that binds for json/xml and don't add tags for param/query and you set AvoidBindByFieldName to true, you will get the behavior that you are requesting. In other words, the binding of route params, query parameters or form data could be configured through the use of the param/query/form tags. If you don't set those tags, the binding won't happen. The thing is that the use of json/xml tags doesn't prevent the binding from route params or query params, which is what I tried to solve with AvoidBindByFieldName.

		{
			name:         "bind to struct with route param + query param",
			givenMethod:  http.MethodPost,
			givenURL:     "/api/real_node/endpoint?node=xxx",
			givenContent: strings.NewReader(`{"id": 1}`),
			expectNode:   "real_node", // why is route param overwritten by query param?
		}

Here you are right, it's because the order in which the code is executed. Sadly I don't know why this was coded in that way, neither which were the design decisions that lead to that order. Maybe someone with more knowledge in the Echo code base could help us.

This would allow user to switch off bindData method for them in cases (json POST for example hardly ever needs to bind query params actually).

That sounds as a nice Use Case for me, I'll add those flags in favor of the performance gain that you are mentioning.


// This flags are aimed to fully disable each one of the three binding methods supported by DefaultBinder
// Setting this flags to true, when you don't need that kind of binding, will result in performace gains.
DisableRouteParamsBinding bool
DisableQueryParamsBinding bool
DisableBodyBinding bool
}

// BindUnmarshaler is the interface used to wrap the UnmarshalParam method.
// Types that don't implement this, but do implement encoding.TextUnmarshaler
Expand All @@ -40,13 +52,17 @@ func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
for i, name := range names {
params[name] = []string{values[i]}
}
if err := b.bindData(i, params, "param"); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
if !b.DisableRouteParamsBinding {
if err := b.bindData(i, params, "param"); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
}
if err = b.bindData(i, c.QueryParams(), "query"); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
if !b.DisableQueryParamsBinding {
if err = b.bindData(i, c.QueryParams(), "query"); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
}
if req.ContentLength == 0 {
if b.DisableBodyBinding || req.ContentLength == 0 {
return
}
ctype := req.Header.Get(HeaderContentType)
Expand Down Expand Up @@ -113,13 +129,14 @@ 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 _, ok := structField.Addr().Interface().(BindUnmarshaler); !ok && structFieldKind == reflect.Struct {
if err := b.bindData(structField.Addr().Interface(), data, tag); err != nil {
return err
}
continue
} else if b.AvoidBindByFieldName == false {
inputFieldName = typeField.Name
}
}

Expand Down
229 changes: 229 additions & 0 deletions bind_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,22 @@ type (
Struct struct {
Foo string
}
jsonBenchmark struct {
One int `json:"One" form:"One" query:"One"`
Two int `json:"Two" form:"Two" query:"Two"`
Three int `json:"Three" form:"Three" query:"Three"`
Four int `json:"Four" form:"Four" query:"Four"`
Five int `json:"Five" form:"Five" query:"Five"`
Six int `json:"Six" form:"Six" query:"Six"`
Seven int `json:"Seven" form:"Seven" query:"Seven"`
Eigth int `json:"Eigth" form:"Eigth" query:"Eigth"`
Nine int `json:"Nine" form:"Nine" query:"Nine"`
Ten int `json:"Ten" form:"Ten" query:"Ten"`
}
)

const (
jsonBenchmarkJSON = `{"One":1,"Two":2,"Three":3,"Four":4,"Five":5,"Six":6,"Seven":7,"Eigth":8,"Nine":9,"Ten":10}`
)

func (t *Timestamp) UnmarshalParam(src string) error {
Expand Down Expand Up @@ -330,6 +346,46 @@ func TestBindbindData(t *testing.T) {
assertBindTestStruct(assert, ts)
}

func TestBindbindDataByTags(t *testing.T) {
assert := assert.New(t)
ts := new(bindTestStructWithTags)
b := new(DefaultBinder)
b.bindData(ts, values, "form")
assertBindTestStruct(assert, (*bindTestStruct)(ts))
}

func TestBindbindDataAvoidBindByFieldName(t *testing.T) {
assert := assert.New(t)
ts := new(bindTestStruct)
b := new(DefaultBinder)
b.AvoidBindByFieldName = true
b.bindData(ts, values, "form")
assertBindTestStructDefaultValues(assert, ts)
}

func TestBindAvoidBindingJsonStructField(t *testing.T) {
type User struct {
ID int `json:"id"`
IsAdmin bool `json:"-"`
}

assert := assert.New(t)

e := New()
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(userAvoidBindJSONField))
req.Header.Set(HeaderContentType, MIMEApplicationJSON)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

defaultUser := &User{}
b := new(DefaultBinder)
b.Bind(defaultUser, c)

assert.Equal(1, defaultUser.ID)
// This should be false because the Zero value of a bool is false and the JSON have it in true
assert.Equal(false, defaultUser.IsAdmin)
}

func TestBindParam(t *testing.T) {
e := New()
req := httptest.NewRequest(GET, "/", nil)
Expand Down Expand Up @@ -470,6 +526,95 @@ func TestBindSetFields(t *testing.T) {
}
}

func TestDefaultBinderConfiguration(t *testing.T) {
type Node struct {
ID int `json:"id"`
Node string `json:"node" param:"node"`
Next string `json:"next" param:"next"`
}

var testCases = []struct {
name string
givenURL string
givenContent io.Reader
givenMethod string
givenBinder DefaultBinder
expectNode string
expectNext string
}{
{
name: "bind to struct with route param + query param and binding by struct field name",
givenMethod: http.MethodPost,
givenURL: "/api/real_node/endpoint?node=xxx&next=real_next",
givenContent: strings.NewReader(`{"id": 1}`),
expectNode: "xxx",
expectNext: "real_next",
},
{
name: "bind to struct with route param + query param and avoid binding by struct field name",
givenMethod: http.MethodPost,
givenURL: "/api/real_node/endpoint?node=xxx",
givenContent: strings.NewReader(`{"id": 1}`),
givenBinder: DefaultBinder{AvoidBindByFieldName: true},
expectNode: "real_node",
},
{
name: "bind to struct with route param + query param and disabling param binding",
givenMethod: http.MethodPost,
givenURL: "/api/real_node/endpoint?node=xxx",
givenContent: strings.NewReader(`{"id": 1}`),
givenBinder: DefaultBinder{DisableRouteParamsBinding: true},
expectNode: "xxx",
},
{
name: "bind to struct with route param + query param and disabling query binding",
givenMethod: http.MethodPost,
givenURL: "/api/real_node/endpoint?node=xxx&next=yyy",
givenContent: strings.NewReader(`{"id": 1}`),
givenBinder: DefaultBinder{DisableQueryParamsBinding: true},
expectNode: "real_node",
expectNext: "", // Node.Next shouldn't be binded
},
{
name: "bind to struct with route param + query param and disabling body binding",
givenMethod: http.MethodPost,
givenURL: "/api/real_node/endpoint",
givenContent: strings.NewReader(`{"id": 1, "node": yyy}`),
givenBinder: DefaultBinder{DisableBodyBinding: true},
expectNode: "real_node",
},
{
name: "bind to struct with route + query + body = body has priority",
givenMethod: http.MethodPost,
givenURL: "/api/real_node/endpoint?node=xxx",
givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
expectNode: "zzz",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
e := New()
// assume route we are testing is "/api/:node/endpoint"
req := httptest.NewRequest(tc.givenMethod, tc.givenURL, tc.givenContent)
req.Header.Set(HeaderContentType, MIMEApplicationJSON)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)

c.SetParamNames("node")
c.SetParamValues("real_node")

defaultUser := &Node{}
err := tc.givenBinder.Bind(defaultUser, c)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, tc.expectNode, defaultUser.Node)
assert.Equal(t, tc.expectNext, defaultUser.Next)
})
}
}

func BenchmarkBindbindData(b *testing.B) {
b.ReportAllocs()
assert := assert.New(b)
Expand Down Expand Up @@ -498,6 +643,72 @@ func BenchmarkBindbindDataWithTags(b *testing.B) {
assertBindTestStruct(assert, (*bindTestStruct)(ts))
}

func BenchmarkBindByJsonBody(b *testing.B) {
b.ReportAllocs()
assert := assert.New(b)

e := New()
var err error
var jb *jsonBenchmark

b.ResetTimer()
for i := 0; i < b.N; i++ {
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(jsonBenchmarkJSON))
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)
req.Header.Set(HeaderContentType, MIMEApplicationJSON)
jb = new(jsonBenchmark)
err = c.Bind(jb)
}
if assert.NoError(err) {
assert.Equal(1, jb.One)
assert.Equal(2, jb.Two)
assert.Equal(3, jb.Three)
assert.Equal(4, jb.Four)
assert.Equal(5, jb.Five)
assert.Equal(6, jb.Six)
assert.Equal(7, jb.Seven)
assert.Equal(8, jb.Eigth)
assert.Equal(9, jb.Nine)
assert.Equal(10, jb.Ten)
}
}

func BenchmarkBindByJsonBodyDisablingRouteParamQueryParamBinding(b *testing.B) {
b.ReportAllocs()
assert := assert.New(b)

e := New()
e.Binder = &DefaultBinder{
DisableQueryParamsBinding: true,
DisableRouteParamsBinding: true,
}
var err error
var jb *jsonBenchmark

b.ResetTimer()
for i := 0; i < b.N; i++ {
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(jsonBenchmarkJSON))
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)
req.Header.Set(HeaderContentType, MIMEApplicationJSON)
jb = new(jsonBenchmark)
err = c.Bind(jb)
}
if assert.NoError(err) {
assert.Equal(1, jb.One)
assert.Equal(2, jb.Two)
assert.Equal(3, jb.Three)
assert.Equal(4, jb.Four)
assert.Equal(5, jb.Five)
assert.Equal(6, jb.Six)
assert.Equal(7, jb.Seven)
assert.Equal(8, jb.Eigth)
assert.Equal(9, jb.Nine)
assert.Equal(10, jb.Ten)
}
}

func assertBindTestStruct(a *assert.Assertions, ts *bindTestStruct) {
a.Equal(0, ts.I)
a.Equal(int8(8), ts.I8)
Expand All @@ -516,6 +727,24 @@ func assertBindTestStruct(a *assert.Assertions, ts *bindTestStruct) {
a.Equal("", ts.GetCantSet())
}

func assertBindTestStructDefaultValues(a *assert.Assertions, ts *bindTestStruct) {
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.GetCantSet())
}

func testBindOkay(assert *assert.Assertions, r io.Reader, ctype string) {
e := New()
req := httptest.NewRequest(http.MethodPost, "/", r)
Expand Down
1 change: 1 addition & 0 deletions echo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const (
userJSONInvalidType = `{"id":"1","name":"Jon Snow"}`
userXMLConvertNumberError = `<user><id>Number one</id><name>Jon Snow</name></user>`
userXMLUnsupportedTypeError = `<user><>Number one</><name>Jon Snow</name></user>`
userAvoidBindJSONField = `{"id": 1, "isAdmin": true}`
)

const userJSONPretty = `{
Expand Down