Adding a new config to DefaultBinder #1675
Conversation
Now the DefaultBinder could be configured to avoid binding struct fields by name. This is particularly useful when the user don't want to bind certain struct fields (with this config in true, only the tagged fields will be binded) Fixes labstack#1620, fixes labstack#1631, partially fixes labstack#1670
Codecov Report
@@ Coverage Diff @@
## master #1675 +/- ##
==========================================
+ Coverage 84.35% 84.37% +0.02%
==========================================
Files 28 28
Lines 1911 1914 +3
==========================================
+ Hits 1612 1615 +3
Misses 189 189
Partials 110 110
Continue to review full report at Codecov.
|
| 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 | ||
| AvoidBindByFieldName bool |
There was a problem hiding this comment.
@pafuent please add tests with http.Request objects also. For example POST body in bind to struct with
Line 55 in ceffc10
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)
}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
@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
Line 52 in 9ba1225
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
}
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
})
}
}There was a problem hiding this comment.
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
}There was a problem hiding this comment.
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.
Adding a JSON Bind new unit test
Adding to the DefaultBinder one flag per binding method to prevent it if the flag is set to true.
|
Closing this PR in favor of #1681 |
Now the DefaultBinder could be configured to avoid binding struct fields by name. This is particularly useful when the user don't want to bind certain struct fields (with this config in true, only the tagged fields will be binded)
Fixes #1620, fixes #1631, partially fixes #1670