separate methods to bind only: query params, route params, request body

[aldas]

To allow user more precise control over where bind takes its source data, bind is refactored to consist of 3 different methods that can be user separately.

1. DefaultBinder.BindPathParams - binds only path params to given interface
2. DefaultBinder.BindQueryParams - binds only query params to given interface
3. DefaultBinder.BindBody - binds request body to given interface

This is not breaking change as Binder interface was not changed.

Example usage would be:

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

Also added bunch of tests for cases when bind target is filled from multiple different sources - to document how binding priority works.

[codecov]

Codecov Report

Merging #1681 (bd5810f) into master (7a90304) will increase coverage by 1.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1681      +/-   ##
==========================================
+ Coverage   84.35%   85.59%   +1.24%     
==========================================
  Files          28       29       +1     
  Lines        1911     1993      +82     
==========================================
+ Hits         1612     1706      +94     
+ Misses        189      181       -8     
+ Partials      110      106       -4     

Impacted Files	Coverage Δ
bind.go	90.00% <100.00%> (+5.03%)	⬆️
response.go	85.18% <0.00%> (ø)
middleware/decompress.go	88.37% <0.00%> (ø)
router.go	96.88% <0.00%> (+0.01%)	⬆️
echo.go	86.39% <0.00%> (+1.63%)	⬆️
middleware/jwt.go	77.89% <0.00%> (+1.75%)	⬆️
middleware/static.go	68.65% <0.00%> (+3.08%)	⬆️
middleware/cors.go	89.15% <0.00%> (+11.21%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a90304...bd5810f. Read the comment docs.

[pafuent]

This is a better and more intuitive approach than the one that I proposed
Just a minor thing, Could you please add in the PR message the issues that this PR fixes?

[aldas]

Workaround for #1670 #1638 #1356 #1565 #1448 #1495 - when query/route params are interfering with body binding (POST/PUT requests mostly)

[aldas]

@vishr @lammel or other Echo official maintainers - I think Echo documentation at https://echo.labstack.com/guide/request needs clear declaration for users that c.bind() uses route+query+body together and if user does not want to mix (possibly) fields from body with route/query params then c.bind() should be avoided. Current implementation has even effect that for following route /api/entity/:id and URL /api/entity/123?id=999 and bind target struct{ ID int } would end up as 999 in id field which is probably not at all intuitive.

to be honest. documentation does not even mention that query and route is binded along with body

To bind request body into a Go type use Context#Bind(i interface{}). The default binder supports decoding application/json, application/xml and application/x-www-form-urlencoded data based on the Content-Type header.
Example below binds the request payload into User struct based on tags:

[iambenkay]

Why isn't this merged yet?

[lammel]

Lets give an update on the delays in merging this PR... but it sure is a definite candidate for merging soon.

We still need to discuss the best way forward. Either use this workaround as is and put a burden on the developer or accept that we need to introduce a breaking change in the Bind interface to remedy the situation.

Feedback is welcome meanwhile.

[aldas]

About this PR. I should have reversed arguments for these new functions. So BindQueryParams(i interface{}, c Context) error would be BindQueryParams(c Context, i interface{}) error instead. As a public API this would make more sense.

Looking at request example https://echo.labstack.com/guide/request I think that original Echo author(s) intentions were that binding for query params should work only then query: tag exists in bind target struct. This would mean that c.Bind() behavior works mostly/only by explicitly written tags - for query params. Maybe current behavior (binding without a tag) is a bug?

I think documentation/cookbook need proper explanation how binding works.

• How route/query/body binding can be done
  • all in one (like current example + route param)
  • separately - sometimes you do not want to mix query with body. For json binging it could be even example one-liner with encoding/json but only query params to struct Echo is currently lacking.
  • clear explanation in which order c.Bind() binding works (route-query-body).
  • clear warning about limitations - route/query params can not be bind to slice etc

Maybe in v4 just provide new methods to do binding separately and make sure that route/query params are bind only then their tag exists. And maybe in v5 consider removing c.Bind() and leave only separate functions as binding from multiple sources is hard to understand as very few people read documentation.

For consideration. These are examples for similar functionality in other popular frameworks

• gin - has binding (separate funcs) https://github.com/gin-gonic/gin#model-binding-and-validation
• beego - has binding https://beego.me/docs/mvc/controller/params.md#data-bind
• chi - has binding (body to struct / custom written binder) https://github.com/go-chi/render/blob/master/render.go#L20 chi documentation seems to be non-existent about binding so i'll just reference to code.

[lammel]

About this PR. I should have reversed arguments for these new functions. So BindQueryParams(i interface{}, c Context) error would be BindQueryParams(c Context, i interface{}) error instead. As a public API this would make more sense.

Yes, that would be more in line with the current interfaces.

Looking at request example https://echo.labstack.com/guide/request I think that original Echo author(s) intentions were that binding for query params should work only then query: tag exists in bind target struct. This would mean that c.Bind() behavior works mostly/only by explicitly written tags - for query params. Maybe current behavior (binding without a tag) is a bug?

We need to look into that, that may be the simplest solution.

I think documentation/cookbook need proper explanation how binding works.

Documentation definitly needs to be updated to describe the correct behaviour then.

Maybe in v4 just provide new methods to do binding separately and make sure that route/query params are bind only then their tag exists. And maybe in v5 consider removing c.Bind() and leave only separate functions as binding from multiple sources is hard to understand as very few people read documentation.

This is what I was thinking about.

For consideration. These are examples for similar functionality in other popular frameworks

• gin - has binding (separate funcs) https://github.com/gin-gonic/gin#model-binding-and-validation
• beego - has binding https://beego.me/docs/mvc/controller/params.md#data-bind
• chi - has binding (body to struct / custom written binder) https://github.com/go-chi/render/blob/master/render.go#L20 chi documentation seems to be non-existent about binding so i'll just reference to code.

Thanks for the list, very helpful.

[pafuent]

echo/bind.go

Lines 43 to 48 in 502cce2

	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)
	}

I think that the error here is to assume that because bindData fails, Bind should fail. The calls for bindData are kind of optional or "call it just because this could be a param/query binding". For this reason, bindData should only fail when the intention of the Echo user is to consume that kind of binding and there is an error during that binding. As an example, bindData should fail on line 46 if the type to bind is an struct, contains query tags and the bind of a particular struct field fails.
To recap, bindData could be modified to not return an error if the type to bind is not an struct (the check must be kept to not panic when NumField() is called). Also, this change could be beneficial for #1356. This shouldn't be an issue for the Echo users that relies on "param/query" binding, because they are already using structs on their calls to Echo#Bind, so they could safely update to a possible newer version that doesn't fail if the type to bind isn't a struct.

[aldas]

I agree that sometimes it should not error out or at least let user to have choice. But this is current API behavior/implementation not a bug. It makes sense to return early when some field binding fails due data type problems. This would flush out problems faster for your API clients that have incorrect implementation. If you have worked on rewriting legacy APIs then it is absolute nightmare when percentage of clients are sending some fields as strings instead of number etc and legacy app was not strict about that (ala js/php are pretty relaxed about types).

I'll add here that order of binding -> route params before query does not make sense for me. Example /api/users/:id/roles?id=1 would bind query param last.

so we have these questions/problems for bind:

• what parts of binding is optional / when to return?
• in what order should binding done?
• should only tagged fields be binded for route/query?
• should user have choice what is what is not binded (route/query/body)?
• documentation is out of date. Bind does magick - docs is missing detailed explanation how binding works, how and when to use it and what to look out.

Bind does too much and in unclear way. Is there a reason why must bind do 3 things (binding route/query/body) by default in one call?

[iambenkay]

Tiny suggestion that can be added to the PR. Usually when working with echo I have to manually process my comma separated query values. Would be great to abstract that into the bindQueryParams method.

basically
/sort?by=name,email should resolve the by param to a slice [name, email]

In order to optionally parse the comma separated strings, we could only process them when a struct is passed into bindData and the concerned field is expecting a slice.

any thoughts?

[pafuent]

I agree that sometimes it should not error out or at least let user to have choice. But this is current API behavior/implementation not a bug. It makes sense to return early when some field binding fails due data type problems. This would flush out problems faster for your API clients that have incorrect implementation. If you have worked on rewriting legacy APIs then it is absolute nightmare when percentage of clients are sending some fields as strings instead of number etc and legacy app was not strict about that (ala js/php are pretty relaxed about types).

Sorry, it seems that I didn't express my idea properly. The only error that could be removed is the struct type check. The rest of the errors should be kept for the reasons that you are mentioning. For me, that case is not an error because calling bindData() on line 43 and 46 it's a "try to guess if the variable to bind is a struct properly tagged". The only caveat to relaxing this error (not a struct) could be that the binded variable will remain as it was before the call to bindData, but probably that couldn't be a big problem because before its use some kind of validation must be performed on it.

Here are mi opinions about some of your questions:

what parts of binding is optional / when to return?

The answer to this one is kind of tricky, because to know 100% sure if the calls to bindData for param/query are not optional, the type must be validated and then all the fields needs to be check for the presence of the corresponding tag, and if you find that all this is true, you will do it again the same inside bindData.

should only tagged fields be binded for route/query?

I think yes. The autobinding by name could be nice for some users, but in my case I always prefer being explicit. Also I think it could simplify the code, because the intention of the user will be always be clear.

should user have choice what is what is not binded (route/query/body)?

Yes, and could be implemented with the three functions that you are proposing: BindRouteParams/BindQueryParams/BindBody

[pafuent]

@iambenkay I'm don't have a strong position on your suggestion, but maybe what we can discuss if it's worthy to implement ourselves in Echo a more complex DefaultBinder or if we should use a third party Binder as DefaultBinder. Of course, the current implementation of Echo have the mechanisms needed to select/use the Binder that better suit your needs, but it's kind of common that Echo users fills issues because the DefaultBinder doesn't work as they assume/need.

[iambenkay]

@pafuent I think the current echo default binder is just fine. Just so far it has delivered well. Just a few hiccups like binding all the data units in a request at once and the comma separated query string for me. This PR obliterates the first issue.

maybe when this is merged, I could send in a PR that builds on this to implement the comma separated query values.

I understand that there is always the option of using a different binder altogether but if most users are like me, then they don't really like replacing Defaults in frameworks seeing that they are perfectly suited to the framework. Would be great if we could add this feature.

[aldas]

Offtopic: this should be discussed in separate issue

just finished porting older php app to go and there is one nice feature that PHP has - mapping &param[]=1&param[]=2 to array [1,2]. And looking at Gin API there are methods to bind params to native type ala Int64, Float64, Bool - these are also nice things that I always have as similar utility functions in my Echo apps.

ala

func ToValidInt64(input string, defaultValue int64) (int64, bool) {
	if input == "" {
		return defaultValue, true
	}
	n, err := strconv.Atoi(input)
	if err != nil {
		return 0, false
	}
	return int64(n), true
}

and in handlers

	length, ok := sanitizer.ToValidInt64(c.QueryParam("length"), 25)
	if !ok {
		return apperror.NewValidationError("length is not valid int64 value")
	}

Maybe echo could have special func/structs for handling things like that. ala very short API for usage like that.

length := int64(25) // default value
var csv bool
var params []string

b := echo.BindQuery(c) // deals only with query params
if err := b.Int64(&length, "length"); err != nil {
    return errors.wrap(err, "length is not valid int64 value")
}
if err := b.Bool(&csv, "csv"); err != nil {
    return errors.wrap(err, "csv is not valid bool value")
}
// NB: go URL returns query params as list of values `c.Request().URL.Query()` 
// but Echo  `c.QueryParam("params[]")` returns single value  so it is not possible in Echo
if err := b.SliceStrings(&params, "params[]"); err != nil { 
    return errors.wrap(err, "params contains invalid values")
}
// or name like that
if err := b.Int64s(&ids, "ids[]"); err != nil { 
    return errors.wrap(err, "ids contains invalid values")
}

or even this - fluent APIs are not very idiomatic go but for me I usually have bunch of query params that I need to bind and need to check for error (first) and return some meaningful message

 // deals only with query params, returns on first error, error struct contains field name as separate member
err := echo.BindQuery(c).
   Int64(&length, "length").
   Bool(&csv, "csv").
   Bind() // and ala BindErrs() that tries to bind all returns all errors
if err != nil {
   return errors.Wrapf(err, "%s can not be bind", (err.(echo.BindError)).field)
}

I have not used any library for this kind of stuff but that later one is probably already implemented in some binding/validation or something-something library

this would be backwards compatible as it would not change existing interfaces (so could be released as 4.x minor version)

[lammel]

After some discussion with @pafuent on the current situation we think the best way forward is a follows:

1. The current DefaultBinder must not consider route or query params unless explicitely specified for the struct (this is what is expected by most users and matches the earlier behaviour. We know that this can be considered breaking, but actually it will restore previour behaviour)
2. Tests will need to be added to assure that binding will not occur for query and route params unless explicitely tagged in the struct
3. The documentation will be corrected to state this change in behaviour
4. An additonal binder AutoBinder (next to DefaultBinder in the core) that will perform binding against all sources (including query and route params) like in the current implementation and add it to the documentation (this should help users relying those that rely on the current behaviour
5. The behaviour of bind failing not non-structs needs to be reviewed and tests with a feasable usecase added (e.g. binding to an array of structs or a map)
6. The current changes using separate functions for BindQueryParam, BindRouteParam, BindBodyParam can be kept, but should get some love in the documentation then.

The changes above will be done for v4 now. Future changes for binding will be discussed in a seperate issue/PR for v5 (where also the interface can change)

We could use this PR to work towards the wanted behaviour above for 1) and 2).
Documentation updates for 3) and 4) should be in seperate PRs in the labstack/echox projekt.
New PR for the autobinder and non-struct binding will need to be opened, I'm not yet sure if 4) the AutoBinder with the current behaviour is really needed.

@aldas: If you are willing to contribute we can keep this PR and adjust to the behaiour and tests.
We can use your newly opened PR labstack/echox#169 for the documentation part.

Feedback very welcome!

[aldas]

NB: I changed initial BindRouteParams name to BindPathParams as is seems that for Echo these are 'path parameters' and I mistakenly used 'route params'

---

For points 1,2 this PR would be useful as base as it separates out path and query binding. Also add bunch of tests for use-cases when all 3 sources exist (these need to changed if only body binding remains for Bind).

Maintainers are allowed to change this PR but maybe its easier to merge it and base next changes on it

---

For 6 I created PR in documentation repo labstack/echox#169

---

As for point 1

The current DefaultBinder must not consider route or query params unless explicitely specified for the struct (this is what is expected by most users and matches the earlier behaviour. We know that this can be considered breaking, but actually it will restore previour behaviour)

it will restore previous behavior is not completely true. For GET/DELETE methods query params have always been binded - even ones that are not explicit tags.

This is first commit of Bind cb75214 (only for binding Form but behavior is similar)
This is commit 0e7a9c1 introduces query and path binding but acts similaraly to previous version

1. line 87 get appropriate tag from reflected field - this is name of query/path variable where data should reside
2. line 90 when tag did not exists then field name in destionation struct is used as backup where data get
3. line 100 gets data from query/path parameters by tag or field name

As I mentioned in #1670

Commit b129098 removed content length and request method check from c.bind() function. This causes many unwanted side effects

this means that b129098 changed how POST/PUT/PATCH interact with query/path parameters.

---

The behaviour of bind failing not non-structs needs to be reviewed and tests with a feasable usecase added (e.g. binding to an array of structs or a map)

If path/query params binding is separated from body binding then most of slice binding problems go away. Binding to slice field is somewhat supported already. See

echo/bind.go

Line 154 in 2b36b3d

if structFieldKind == reflect.Slice && numElems > 0 {

when destination field is slice and query/path contains multiple values for that parameters then slice of that native type is created and filled

[aldas]

My suggestion would be:

• only bind query/path params when explicit tag exists in destination struct
• restore how Bind worked before b129098 - not mixing query params with body during binding are sane safe defaults. Why would you want query parameter to end up in struct for json POST - this would be and is very unexpected. Mixing query with body introduces slice binding problems also.
• create new separate binding ways (binders/functions whatever) for struct binding, but do not change existing interfaces
• document behavior in Request recipe/cookbook

[lammel]

it will restore previous behavior is not completely true. For GET/DELETE methods query params have always been binded - even ones that are not explicit tags.

This was probably due to those requests not having a body at all, so only path and query params are available anyway.
Still I find that kind of inconsistent. I usually get around without using any binding at all für GET/DELETE by just accessing route params directly.

For this the "AutoBinder" or whatever we call it, that will bind everything could also be used.

My suggestion would be:

• only bind query/path params when explicit tag exists in destination struct
• restore how Bind worked before b129098 - not mixing query params with body during binding are sane safe defaults. Why would you want query parameter to end up in struct for json POST - this would be and is very unexpected. Mixing query with body introduces slice binding problems also.
• create new separate binding ways (binders/functions whatever) for struct binding, but do not change existing interfaces
• document behavior in Request recipe/cookbook

Acknowledged!

Concerning the new seperate binding ways, just to clarify.
Are you referring to the new functions of the DefaultBinder (BindQueryParam, BindPathParam, BindBody) and an additonal bind everything thingy next to the DefaultBinder (what I called AutoBinder)?

[aldas]

yep, at least these refactored methods. So user can choose from binding: (all), query, path, body - to struct.

This was probably due to those requests not having a body at all, so only path and query params are available anyway.

well main thing was that block

	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")
	}

It was pretty much implementation of requirements:

1. query params should be binded only for (no body) GET/DELETE methods.
2. query params should not be binded for body (POST/PUT/PATCH) methods.
3. POST/PUT/PATCH methods can not have empty body.
4. body is only binded for POST/PUT/PATCH methods.

this guaranteed that for POST/PUT request query param from URL will not have conflicts with bindable struct. ala my second example (binding slice) #1670 (comment)

so if problem was getting status 400 for empty body during post/put this does not mean that all these requirements should be changed. Maybe just relax req 3 and leave 1,2,4 alone. because touching 2 messes up a lot of things

[lammel]

We are getting close! Good work.

It was pretty much implementation of requirements:

1. query params should be binded only for (no body) GET/DELETE methods.
2. query params should not be binded for body (POST/PUT/PATCH) methods.
3. POST/PUT/PATCH methods can not have empty body.
4. body is only binded for POST/PUT/PATCH methods.

this guaranteed that for POST/PUT request query param from URL will not have conflicts with bindable struct. ala my second example (binding slice) #1670 (comment)

so if problem was getting status 400 for empty body during post/put this does not mean that all these requirements should be changed. Maybe just relax req 3 and leave 1,2,4 alone. because touching 2 messes up a lot of things

Reviewing the current changes the rules are different then the 4 stated above.

Ad 2) This is still done currently, mixing query and body params
So do we want to allow mixing params from query and body like in test "ok, POST bind to struct with: path param + query param + empty body".
I'd prefer not to, but that could also be changed for v5 only.

Ad 3) A stated in the HTTP1.0/1.1 RFCs a body MAY be empty (usually only useful for PUT to empty a resource) but still that could be considered a valid request. So not sure we should fail binding in that case or just return an empty struct.

[aldas]

well. my intention was not to change behavior with this PR. idea was to implement changes/"fixes" with further PRs

1. (this PR) provide workaround for binding all + update docs
2. restore behavior that query params are binded only when HTTP method is GET/DELETE (does not depend if body exists or not) + update docs
3. change struct binding work only when tag exists (do not use field name as backup) + update docs
4. ...

[lammel]

Agreed. Might be better to that separate PRs.
Looking forward to more PRs in the bind series then ;-)

So this PR is ready to merge @aldas ?

[aldas]

yep

[arvenil]

Is there a way now to bind query params for POST? Is there a way to restore previous behavior? This change broke a lot of our APIs developed on 4.1.17.

[junedev]

@arvenil Did you check https://echo.labstack.com/guide/binding? That helped me to find the right method to restore the correct behavoir in our case.