-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
"binding element must be a struct" introduced with path params binding #1356
Comments
Also, binding to slices is returning the same error. var items []uuid.UUID
c.Bind(&items) |
For me this causes to all my inner structures in the field example: package main
import (
"github.com/labstack/echo/v4"
"github.com/labstack/echo/v4/middleware"
"github.com/labstack/gommon/log"
)
type user struct {
ID int `json:"id" param:"id"`
Name string `json:"name"`
BranchID uint
Branch Branch
}
type branch struct {
ID int
Name string
}
func main() {
s := echo.New()
s.Use(middleware.Logger())
s.PUT("/users/:id", func(c echo.Context) error {
u := &user{}
if err := c.Bind(&u); err != nil {
log.Fatal(err)
}
log.Print(u)
return nil
})
s.Start(":7070")
} This set the |
To me, as a quick fix, just change the name of the parameter to I hope this will fix in a couple of days this will helpfull for me. |
Breakage on existing feature really hurts, if couldn't be fixed quickly, maybe a temporary revert and a new version is a good idea? |
+1 On this issue. This is a bug that breaks existing functionality and has been an issue since early August. A patch release would be appreciated, so that I don't have to pin at v4.1.6 indefinitely. Thanks! |
This bug exists in 4.1.11 which is the latest release. package main
import (
"github.com/labstack/echo/v4"
"github.com/labstack/echo/v4/middleware"
"github.com/labstack/gommon/log"
)
func main() {
s := echo.New()
s.Use(middleware.Logger())
s.PUT("/users/:id", func(c echo.Context) error {
var data interface{}
if err := c.Bind(&data); err != nil {
log.Fatal(err)
}
log.Print(data)
return nil
})
s.Start(":8811")
} request: curl -X PUT -H "Content-Type: application/json" -d '{"name":"John"}' localhost:8811/users/1 I have read the // Bind implements the `Binder#Bind` function.
func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
req := c.Request()
names := c.ParamNames()
values := c.ParamValues()
params := map[string][]string{}
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 err = b.bindData(i, c.QueryParams(), "query"); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
if req.ContentLength == 0 {
return
}
ctype := req.Header.Get(HeaderContentType)
switch {
case strings.HasPrefix(ctype, MIMEApplicationJSON):
if err = json.NewDecoder(req.Body).Decode(i); err != nil {
if ute, ok := err.(*json.UnmarshalTypeError); ok {
return NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Unmarshal type error: expected=%v, got=%v, field=%v, offset=%v", ute.Type, ute.Value, ute.Field, ute.Offset)).SetInternal(err)
} else if se, ok := err.(*json.SyntaxError); ok {
return NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Syntax error: offset=%v, error=%v", se.Offset, se.Error())).SetInternal(err)
}
return NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
.... if the request has url param and content both, it must be got error because it binds param first. |
@vishr, param binding is a feature that I wanted, but I think it's better if it gets reverted to fix breaking change it introduced. And then planned again for another release. |
Is it possible that add a flag to enable or disable param binding feature and disable by default? |
I did some tricks to temporary solve the problem by adding |
Any updates on this? I see that @vishr is assigned on this issue? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Please don't close this! I'm stuck back at version 4.1.6, which was the last release without this bug. |
Do not throw an error when binding to a slice, if binding occurs on path- or query-parameters => data is very likely to be found in the body, which will be bound later Should fix labstack#1356, labstack#1448, labstack#1495 (second case), labstack#1565
This workaround is causing interesting problems! If the c.bind() afterwards fails, other routes using params will suddenly fail with "Not found". Only restarting the echo service resolves this. I am now using ioutil.ReadAll(c.Request().Body) and Unmarshal it later into the struct. |
Composable Bind implementations would be nice, in the meantime, roll your own Binder I think. I did not want Bind to involve query or path params. package util
import (
"encoding"
"encoding/json"
"encoding/xml"
"errors"
"fmt"
"net/http"
"reflect"
"strconv"
"strings"
"github.com/labstack/echo/v4"
)
type (
// Binder is the interface that wraps the Bind method.
Binder interface {
Bind(i interface{}, c echo.Context) error
}
// DefaultBinder is the default implementation of the Binder interface.
DefaultBinder struct{}
// BindUnmarshaler is the interface used to wrap the UnmarshalParam method.
// Types that don't implement this, but do implement encoding.TextUnmarshaler
// will use that interface instead.
BindUnmarshaler interface {
// UnmarshalParam decodes and assigns a value from an form or query param.
UnmarshalParam(param string) error
}
)
// Bind implements the `Binder#Bind` function.
func (b *DefaultBinder) Bind(i interface{}, c echo.Context) (err error) {
req := c.Request()
ctype := req.Header.Get(echo.HeaderContentType)
switch {
case strings.HasPrefix(ctype, echo.MIMEApplicationJSON):
if err = json.NewDecoder(req.Body).Decode(i); err != nil {
if ute, ok := err.(*json.UnmarshalTypeError); ok {
return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Unmarshal type error: expected=%v, got=%v, field=%v, offset=%v", ute.Type, ute.Value, ute.Field, ute.Offset)).SetInternal(err)
} else if se, ok := err.(*json.SyntaxError); ok {
return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Syntax error: offset=%v, error=%v", se.Offset, se.Error())).SetInternal(err)
}
return echo.NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
case strings.HasPrefix(ctype, echo.MIMEApplicationXML), strings.HasPrefix(ctype, echo.MIMETextXML):
if err = xml.NewDecoder(req.Body).Decode(i); err != nil {
if ute, ok := err.(*xml.UnsupportedTypeError); ok {
return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Unsupported type error: type=%v, error=%v", ute.Type, ute.Error())).SetInternal(err)
} else if se, ok := err.(*xml.SyntaxError); ok {
return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Syntax error: line=%v, error=%v", se.Line, se.Error())).SetInternal(err)
}
return echo.NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
case strings.HasPrefix(ctype, echo.MIMEApplicationForm), strings.HasPrefix(ctype, echo.MIMEMultipartForm):
params, err := c.FormParams()
if err != nil {
return echo.NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
if err = b.bindData(i, params, "form"); err != nil {
return echo.NewHTTPError(http.StatusBadRequest, err.Error()).SetInternal(err)
}
default:
return echo.ErrUnsupportedMediaType
}
return
}
func (b *DefaultBinder) bindData(ptr interface{}, data map[string][]string, tag string) error {
if ptr == nil || len(data) == 0 {
return nil
}
typ := reflect.TypeOf(ptr).Elem()
val := reflect.ValueOf(ptr).Elem()
// Map
if typ.Kind() == reflect.Map {
for k, v := range data {
val.SetMapIndex(reflect.ValueOf(k), reflect.ValueOf(v[0]))
}
return nil
}
// !struct
if typ.Kind() != reflect.Struct {
return errors.New("binding element must be a struct")
}
for i := 0; i < typ.NumField(); i++ {
typeField := typ.Field(i)
structField := val.Field(i)
if !structField.CanSet() {
continue
}
structFieldKind := structField.Kind()
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
}
}
inputValue, exists := data[inputFieldName]
if !exists {
// Go json.Unmarshal supports case insensitive binding. However the
// url params are bound case sensitive which is inconsistent. To
// fix this we must check all of the map values in a
// case-insensitive search.
for k, v := range data {
if strings.EqualFold(k, inputFieldName) {
inputValue = v
exists = true
break
}
}
}
if !exists {
continue
}
// Call this first, in case we're dealing with an alias to an array type
if ok, err := unmarshalField(typeField.Type.Kind(), inputValue[0], structField); ok {
if err != nil {
return err
}
continue
}
numElems := len(inputValue)
if structFieldKind == reflect.Slice && numElems > 0 {
sliceOf := structField.Type().Elem().Kind()
slice := reflect.MakeSlice(structField.Type(), numElems, numElems)
for j := 0; j < numElems; j++ {
if err := setWithProperType(sliceOf, inputValue[j], slice.Index(j)); err != nil {
return err
}
}
val.Field(i).Set(slice)
} else if err := setWithProperType(typeField.Type.Kind(), inputValue[0], structField); err != nil {
return err
}
}
return nil
}
func setWithProperType(valueKind reflect.Kind, val string, structField reflect.Value) error {
// But also call it here, in case we're dealing with an array of BindUnmarshalers
if ok, err := unmarshalField(valueKind, val, structField); ok {
return err
}
switch valueKind {
case reflect.Ptr:
return setWithProperType(structField.Elem().Kind(), val, structField.Elem())
case reflect.Int:
return setIntField(val, 0, structField)
case reflect.Int8:
return setIntField(val, 8, structField)
case reflect.Int16:
return setIntField(val, 16, structField)
case reflect.Int32:
return setIntField(val, 32, structField)
case reflect.Int64:
return setIntField(val, 64, structField)
case reflect.Uint:
return setUintField(val, 0, structField)
case reflect.Uint8:
return setUintField(val, 8, structField)
case reflect.Uint16:
return setUintField(val, 16, structField)
case reflect.Uint32:
return setUintField(val, 32, structField)
case reflect.Uint64:
return setUintField(val, 64, structField)
case reflect.Bool:
return setBoolField(val, structField)
case reflect.Float32:
return setFloatField(val, 32, structField)
case reflect.Float64:
return setFloatField(val, 64, structField)
case reflect.String:
structField.SetString(val)
default:
return errors.New("unknown type")
}
return nil
}
func unmarshalField(valueKind reflect.Kind, val string, field reflect.Value) (bool, error) {
switch valueKind {
case reflect.Ptr:
return unmarshalFieldPtr(val, field)
default:
return unmarshalFieldNonPtr(val, field)
}
}
func unmarshalFieldNonPtr(value string, field reflect.Value) (bool, error) {
fieldIValue := field.Addr().Interface()
if unmarshaler, ok := fieldIValue.(BindUnmarshaler); ok {
return true, unmarshaler.UnmarshalParam(value)
}
if unmarshaler, ok := fieldIValue.(encoding.TextUnmarshaler); ok {
return true, unmarshaler.UnmarshalText([]byte(value))
}
return false, nil
}
func unmarshalFieldPtr(value string, field reflect.Value) (bool, error) {
if field.IsNil() {
// Initialize the pointer to a nil value
field.Set(reflect.New(field.Type().Elem()))
}
return unmarshalFieldNonPtr(value, field.Elem())
}
func setIntField(value string, bitSize int, field reflect.Value) error {
if value == "" {
value = "0"
}
intVal, err := strconv.ParseInt(value, 10, bitSize)
if err == nil {
field.SetInt(intVal)
}
return err
}
func setUintField(value string, bitSize int, field reflect.Value) error {
if value == "" {
value = "0"
}
uintVal, err := strconv.ParseUint(value, 10, bitSize)
if err == nil {
field.SetUint(uintVal)
}
return err
}
func setBoolField(value string, field reflect.Value) error {
if value == "" {
value = "false"
}
boolVal, err := strconv.ParseBool(value)
if err == nil {
field.SetBool(boolVal)
}
return err
}
func setFloatField(value string, bitSize int, field reflect.Value) error {
if value == "" {
value = "0.0"
}
floatVal, err := strconv.ParseFloat(value, bitSize)
if err == nil {
field.SetFloat(floatVal)
}
return err
} |
I'm confused about this ticket. The documentation doesn't say that the binding element must be a struct. On v4.1.6 and before, this worked fine. A change was introduced post-v4.1.6 that broke existing functionality. If this was intentional, then shouldn't the next release have been 5.0.0, following semver? If it wasn't intentional, then shouldn't the change be reverted? Beyond that, I'm having trouble figuring out why this change happened in the first place. Looking at the v4.1.7 release, I think issue #988 is the only one relevant here, but that ticket also has the binding element issue |
The bug comes from this commit (from this PR), to solve this proposal. What is done is done, but I agree with you, breaking compatibility on such critical functions is not something we expect… unit tests were not complete enough. PR #1574 includes a quick fix which works in most, common, cases (by ignoring the binding error on the parameter). Let's hope it gets merged soon… If you have any idea or comments to improve it, feel free to tell me about it. |
Thanks for the reply! Looking at the Bind() code, it seems to:
And an error with any of these exits early, correct? That's a lot, and I think the function is too complicated for its own good. What we're seeing here is an issue with 1 breaking 3, even though param binding wasn't intended for the situation this ticket. For a 4.1.x/4.2.0 fix, how do you feel about adding BindBody(), BindParams(), and BindQueryParams() methods, so that developers can explicitly state what they're trying to bind? (I also can't imagine a case where you wouldn't know what you're trying to bind.) That doesn't fix the issue with Bind() overall, but it's a workaround that will only require minor code changes. I'm not sure what the best way to unravel Bind() would be. To be a 4.x.x release, I think it would have to:
I'm not sure all 3 are doable. Maybe the function parameters should change in 5.0.0? |
I totally agree, and also think your suggestion of separate This has already been suggested somewhere (another issue maybe?), but not implemented yet, although it would not be too hard to code… maybe the maintainer could tell us about it, as for me, I don't know what the plans are for the next versions and I don't want to waste time on something which has no chance of getting merged (my own PR is enough for me right now). |
The description of bind() clearly states:
Why is it also parsing Param and QueryParam which are part of the header? Adding the Param and QueryParam parser into the binder was a breaking change and should not have happened. Developers like me expected this function only parsing the body as the description states. I'm with @cdukes and his suggestion of adding BindParams() and BindQueryParams() but the Bind() should only parse the body like it always did (if added to 4.x). |
Until, the new behaviour is fixed or reverted, I agree that this is the easiest workaround when you only need to read a JSON Body with a non-struct like an array. A more verbose code example: b, err := ioutil.ReadAll(c.Request().Body)
if err != nil {
return c.NoContent(http.StatusNotAcceptable)
}
var req []MyArray // the non-struct body
if b != nil {
err := json.Unmarshal(b, &req)
if err != nil {
return c.NoContent(http.StatusNotAcceptable)
}
} else {
return c.NoContent(http.StatusNotAcceptable)
}
// Do something with req |
Do not throw an error when binding to a slice, if binding occurs on path- or query-parameters => data is very likely to be found in the body, which will be bound later Should fix labstack#1356, labstack#1448, labstack#1495 (second case), labstack#1565
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I wouldn't like to see this issue automatically close. It's a revision version that broke compatibility. |
Same - I upgraded to v4 yesterday and this broke (Bind on a json array now returning an error). |
This is also biting my team. |
I'm not sure if this is the bug I ran into as well, or it's something similar, but we had unit tests that were complaining about suddenly receiving the wrong status code and apparently reverting to 4.1.6 fixed the issue. It happened mostly on routes that have a path param and also receive a JSON body. Is there anything we can help with regarding this ? Quite some time seems to have passed without any update on this. |
Hi all, My PR was closed due to inactivity, but if you really need this to work, apply this quick patch and things should work as expected. |
@benoitmasson yep, I saw it, thanks! At this point it seems as if it would be better to fork the repo and try to keep an up to date version based on this one. |
This is almost a year and half old and it hasn't been fixed? Why would anyone want to rely on a framework where these basic functionality type of issues aren't resolved? I also want to note that even using a struct with a PATCH method seems broken. |
Hi, is there a timeline to get the different solutions offered in different merge requests actually on master? This is currently quite breaking for us, and many others. Can this be picked up again? Thanks! |
Will try to look into this in the next days. Never touched the bind code before though. |
@lammel there was a previous PR by @benoitmasson that was closed due to inactivity, maybe that one fixes the issue ? |
Same problem here.
Can be used to bypass all other fancy logic regarding form binding and such ... |
Hi, an alternative to this problem would be this: https://www.buggger.com/en/rqt/5ecb9ceacb08b |
Issues with binding should be resolved with echo v4.2 (see release notes) with various PRs merged that are referenced here. So feedback is welcome, if there are open issues left... Please let us know. |
This issue seems fixed by #1835 and was released in v4.2.2. |
Can confirm this is now working for me in v4.2.2. |
@lammel Sorry - I think this may have to be re-opened. I noticed this works fine when using a struct, but if you bind to a map EDIT: if err = (&echo.DefaultBinder{}).BindBody(c, &tagValue); err != nil {
return fmt.Errorf("could not parse update: %v", err)
} This appears do what I expected. |
Issue Description
This is simillar to #988 and introduced by 858270f in a specific situation: taking the address of your struct twice and sending it to bind.
Because of the newly introduced params binding, when the struct's type is checked, it fails here:
Before the params binding was introduced,
bindData
was not called for POST/PUT/PATCH, because for these methods you directly decode the body into the given struct.And a mention: If you have
id
both as path param and json property in the request body, the value from the body will overwrite the one from the path. This could lead to confusion and bugs in projects.Checklist
Expected behaviour
If this is known and planned for next major version, than it's OK, else compatibility should not be broken.
Actual behaviour
See
Expected behaviour
Steps to reproduce
Take the address of the struct sent at bind twice and make a request to a server (see
Working code to debug
).Working code to debug
Version/commit
858270f
The text was updated successfully, but these errors were encountered: