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

Please don’t close this! I’m stuck back at version 4.1.6, which was the last release without this bug.

Also, binding to slices is returning the same error.

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

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

The description of bind() clearly states:

// Bind binds the request body into provided type `i`. The default binder
// does it based on Content-Type header.

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

I wouldn’t like to see this issue automatically close. It’s a revision version that broke compatibility.

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?

Same - I upgraded to v4 yesterday and this broke (Bind on a json array now returning an error).

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

Hi, an alternative to this problem would be this: https://www.buggger.com/en/rqt/5ecb9ceacb08b

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

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.

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

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

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

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!

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.

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

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.

This is also biting my team.

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

Thanks for verifying @jherman , @dahu33 . Closing!

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.

I think the function is too complicated for its own good.

I totally agree, and also think your suggestion of separate BindBody(), … functions is the most appropriate (along with a generic Bind() if needed, but then this one should allow only struct or map[string]string bindings).

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

I’m having trouble figuring out why this change happened in the first place.

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.

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

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.