"binding element must be a struct" introduced with path params binding

[andreiavrammsd]

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.

u := &user{}
c.Bind(&u)

Because of the newly introduced params binding, when the struct's type is checked, it fails here:

func (b *DefaultBinder) bindData(ptr interface{}, data map[string][]string, tag string) error {
	typ := reflect.TypeOf(ptr).Elem()
	val := reflect.ValueOf(ptr).Elem()

	if typ.Kind() != reflect.Struct {
		return errors.New("binding element must be a struct")
	}

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

• Dependencies installed
• No typos
• Searched existing issues and docs

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

curl -X PUT -H "Content-Type: application/json" -d '{"name":"John"}' localhost:8811/users/1

Working code to debug

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

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(":8811")
}

Version/commit

858270f

[andreiavrammsd]

Also, binding to slices is returning the same error.

var items []uuid.UUID
c.Bind(&items)

[xXLokerXx]

For me this causes to all my inner structures in the field id change to that value and this really made a mess in my projects because this updates all inner structures to zero values (i'm working with gorm and i'm using the save function)

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 ID in branch relation equals to the ID of user

[xXLokerXx]

To me, as a quick fix, just change the name of the parameter to pk insted of id.

I hope this will fix in a couple of days this will helpfull for me.

[tori3ama]

Also, binding to slices is returning the same error.

var items []uuid.UUID
c.Bind(&items)

Breakage on existing feature really hurts, if couldn't be fixed quickly, maybe a temporary revert and a new version is a good idea?

[cdukes]

+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!

[ryanking8215]

This bug exists in 4.1.11 which is the latest release.
And it really stops the general restful api usage, i.e.

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 source code.

// 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.

[andreiavrammsd]

@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.

[ryanking8215]

Is it possible that add a flag to enable or disable param binding feature and disable by default?

[EndlessCheng]

I did some tricks to temporary solve the problem by adding c.SetParamNames() before bind.