Skip to content

Commit

Permalink
Merge pull request #1681 from aldas/different_bind_methods
Browse files Browse the repository at this point in the history 
Improve default binder with separate methods for binding to query params, route params, request body
  • Loading branch information
@lammel
lammel committed Dec 16, 2020
2 parents 03ce9b2 + bd5810f commit ea31edb
Show file tree
Hide file tree
Showing 2 changed files with 333 additions and 6 deletions.
37 changes: 31 additions & 6 deletions bind.go
View file
Expand Up @@ -30,10 +30,8 @@ type (
}
)

// Bind implements the `Binder#Bind` function.
func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
req := c.Request()

// BindPathParams binds path params to bindable object
func (b *DefaultBinder) BindPathParams(c Context, i interface{}) error {
names := c.ParamNames()
values := c.ParamValues()
params := map[string][]string{}
Expand All @@ -43,12 +41,28 @@ func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
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 nil
}

// BindQueryParams binds query params to bindable object
func (b *DefaultBinder) BindQueryParams(c Context, i interface{}) error {
if err := b.bindData(i, c.QueryParams(), "query"); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
return nil
}

// BindBody binds request body contents to bindable object
// NB: then binding forms take note that this implementation uses standard library form parsing
// which parses form data from BOTH URL and BODY if content type is not MIMEMultipartForm
// See non-MIMEMultipartForm: https://golang.org/pkg/net/http/#Request.ParseForm
// See MIMEMultipartForm: https://golang.org/pkg/net/http/#Request.ParseMultipartForm
func (b *DefaultBinder) BindBody(c Context, i interface{}) (err error) {
req := c.Request()
if req.ContentLength == 0 {
return
}

ctype := req.Header.Get(HeaderContentType)
switch {
case strings.HasPrefix(ctype, MIMEApplicationJSON):
Expand Down Expand Up @@ -80,7 +94,18 @@ func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
default:
return ErrUnsupportedMediaType
}
return
return nil
}

// Bind implements the `Binder#Bind` function.
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
}
return b.BindBody(c, i)
}

func (b *DefaultBinder) bindData(ptr interface{}, data map[string][]string, tag string) error {
Expand Down
302 changes: 302 additions & 0 deletions bind_test.go
View file
Expand Up @@ -553,3 +553,305 @@ func testBindError(assert *assert.Assertions, r io.Reader, ctype string, expecte
}
}
}

func TestDefaultBinder_BindToStructFromMixedSources(t *testing.T) {
// tests to check binding behaviour when multiple sources path params, query params and request body are in use
// 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 {
ID int `json:"id"`
Node string `json:"node"`
}

var testCases = []struct {
name string
givenURL string
givenContent io.Reader
givenMethod string
whenBindTarget interface{}
whenNoPathParams bool
expect interface{}
expectError string
}{
{
name: "ok, POST bind to struct with: path param + query param + empty 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
},
{
name: "ok, POST bind to struct with: path param + empty body",
givenMethod: http.MethodPost,
givenURL: "/api/real_node/endpoint",
givenContent: strings.NewReader(`{"id": 1}`),
expect: &Node{ID: 1, Node: "real_node"},
},
{
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
},
{
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
expectError: "code=400, message=unexpected EOF, internal=unexpected EOF",
},
{
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",
},
{ // 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{},
expectError: "code=400, message=binding element must be a struct, internal=binding element must be a struct",
},
{ // 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{},
expectError: "code=400, message=binding element must be a struct, internal=binding element must be a struct",
},
{
name: "ok, GET body bind json array to slice",
givenMethod: http.MethodGet,
givenURL: "/api/real_node/endpoint",
givenContent: strings.NewReader(`[{"id": 1}]`),
whenNoPathParams: true,
whenBindTarget: &[]Node{},
expect: &[]Node{{ID: 1, Node: ""}},
expectError: "",
},
}

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

if !tc.whenNoPathParams {
c.SetParamNames("node")
c.SetParamValues("real_node")
}

var bindTarget interface{}
if tc.whenBindTarget != nil {
bindTarget = tc.whenBindTarget
} else {
bindTarget = &Node{}
}
b := new(DefaultBinder)

err := b.Bind(bindTarget, c)
if tc.expectError != "" {
assert.EqualError(t, err, tc.expectError)
} else {
assert.NoError(t, err)
}
assert.Equal(t, tc.expect, bindTarget)
})
}
}

func TestDefaultBinder_BindBody(t *testing.T) {
// tests to check binding behaviour when multiple sources path params, query params and request body are in use
// generally when binding from request body - URL and path params are ignored - unless form is being binded.
// 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"`
}
type Nodes struct {
Nodes []Node `xml:"node" form:"node"`
}

var testCases = []struct {
name string
givenURL string
givenContent io.Reader
givenMethod string
givenContentType string
whenNoPathParams bool
whenBindTarget interface{}
expect interface{}
expectError string
}{
{
name: "ok, JSON POST bind to struct with: path + query + empty field in body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationJSON,
givenContent: strings.NewReader(`{"id": 1}`),
expect: &Node{ID: 1, Node: ""}, // path params or query params should not interfere with body
},
{
name: "ok, JSON POST bind to struct with: path + query + body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationJSON,
givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
expect: &Node{ID: 1, Node: "zzz"}, // field value from content has higher priority
},
{
name: "ok, JSON POST body bind json array to slice (has matching path/query params)",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationJSON,
givenContent: strings.NewReader(`[{"id": 1}]`),
whenNoPathParams: true,
whenBindTarget: &[]Node{},
expect: &[]Node{{ID: 1, Node: ""}},
expectError: "",
},
{ // rare case as GET is not usually used to send request body
name: "ok, JSON GET bind to struct with: path + query + empty field in body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodGet,
givenContentType: MIMEApplicationJSON,
givenContent: strings.NewReader(`{"id": 1}`),
expect: &Node{ID: 1, Node: ""}, // path params or query params should not interfere with body
},
{ // rare case as GET is not usually used to send request body
name: "ok, JSON GET bind to struct with: path + query + body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodGet,
givenContentType: MIMEApplicationJSON,
givenContent: strings.NewReader(`{"id": 1, "node": "zzz"}`),
expect: &Node{ID: 1, Node: "zzz"}, // field value from content has higher priority
},
{
name: "nok, JSON POST body bind failure",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationJSON,
givenContent: strings.NewReader(`{`),
expect: &Node{ID: 0, Node: ""},
expectError: "code=400, message=unexpected EOF, internal=unexpected EOF",
},
{
name: "ok, XML POST bind to struct with: path + query + empty body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationXML,
givenContent: strings.NewReader(`<node><id>1</id><node>yyy</node></node>`),
expect: &Node{ID: 1, Node: "yyy"},
},
{
name: "ok, XML POST bind array to slice with: path + query + body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationXML,
givenContent: strings.NewReader(`<nodes><node><id>1</id><node>yyy</node></node></nodes>`),
whenBindTarget: &Nodes{},
expect: &Nodes{Nodes: []Node{{ID: 1, Node: "yyy"}}},
},
{
name: "nok, XML POST bind failure",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationXML,
givenContent: strings.NewReader(`<node><`),
expect: &Node{ID: 0, Node: ""},
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",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationForm,
givenContent: strings.NewReader(`id=1&node=yyy`),
expect: &Node{ID: 1, Node: "yyy"},
},
{
// NB: form values are taken from BOTH body and query for POST/PUT/PATCH by standard library implementation
// See: https://golang.org/pkg/net/http/#Request.ParseForm
name: "ok, FORM POST bind to struct with: path + query + empty field in body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMEApplicationForm,
givenContent: strings.NewReader(`id=1`),
expect: &Node{ID: 1, Node: "xxx"},
},
{
// NB: form values are taken from query by standard library implementation
// See: https://golang.org/pkg/net/http/#Request.ParseForm
name: "ok, FORM GET bind to struct with: path + query + empty field in body",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodGet,
givenContentType: MIMEApplicationForm,
givenContent: strings.NewReader(`id=1`),
expect: &Node{ID: 0, Node: "xxx"}, // 'xxx' is taken from URL and body is not used with GET by implementation
},
{
name: "nok, unsupported content type",
givenURL: "/api/real_node/endpoint?node=xxx",
givenMethod: http.MethodPost,
givenContentType: MIMETextPlain,
givenContent: strings.NewReader(`<html></html>`),
expect: &Node{ID: 0, Node: ""},
expectError: "code=415, message=Unsupported Media Type",
},
}

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

if !tc.whenNoPathParams {
c.SetParamNames("node")
c.SetParamValues("real_node")
}

var bindTarget interface{}
if tc.whenBindTarget != nil {
bindTarget = tc.whenBindTarget
} else {
bindTarget = &Node{}
}
b := new(DefaultBinder)

err := b.BindBody(c, bindTarget)
if tc.expectError != "" {
assert.EqualError(t, err, tc.expectError)
} else {
assert.NoError(t, err)
}
assert.Equal(t, tc.expect, bindTarget)
})
}
}

0 comments on commit ea31edb

Please sign in to comment.