Echo binder body deserialization blanket replaces all data

[pcfreak30]

If you have a path or query param say in a basic CRUD setup, and do a POST/PUT/PATCH for updating an entity, the body data will nuke prior parsing of those data sources forcing you to manually parse the id from c.Param

Echo should decode to a temp struct then selectively update the fields to not nuke existing struct data. There may be a way to have knobs on this as well.

[aldas]

Could you provide an example? If we are talking about JSON body, then the "binding" is done by standard library JSON package and there is very few chances that we can do anything about it.

These "features" are know and documented here and there

echo/bind.go

Lines 135 to 151 in 321530d

	// Bind implements the `Binder#Bind` function.
	// Binding is done in following order: 1) path params; 2) query params; 3) request body. Each step COULD override previous
	// step binded values. For single source binding use their own methods BindBody, BindQueryParams, BindPathParams.
	func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
	if err := b.BindPathParams(c, i); err != nil {
	return err
	}
	// Only bind query parameters for GET/DELETE/HEAD to avoid unexpected behavior with destination struct binding from body.
	// For example a request URL `&id=1&lang=en` with body `{"id":100,"lang":"de"}` would lead to precedence issues.
	// The HTTP method check restores pre-v4.1.11 behavior to avoid these problems (see issue #1670)
	method := c.Request().Method
	if method == http.MethodGet || method == http.MethodDelete || method == http.MethodHead {
	if err = b.BindQueryParams(c, i); err != nil {
	return err
	}
	}
	return b.BindBody(c, i)

[pcfreak30]

Could you provide an example? If we are talking about JSON body, then the "binding" is done by standard library JSON package and there is very few chances that we can do anything about it.

These "features" are know and documented here and there

echo/bind.go

Lines 135 to 151 in 321530d

// Bind implements the Binder#Bind function.
// Binding is done in following order: 1) path params; 2) query params; 3) request body. Each step COULD override previous
// step binded values. For single source binding use their own methods BindBody, BindQueryParams, BindPathParams.
func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
if err := b.BindPathParams(c, i); err != nil {
return err
}
// Only bind query parameters for GET/DELETE/HEAD to avoid unexpected behavior with destination struct binding from body.
// For example a request URL &id=1&lang=en with body {"id":100,"lang":"de"} would lead to precedence issues.
// The HTTP method check restores pre-v4.1.11 behavior to avoid these problems (see issue #1670)
method := c.Request().Method
if method == http.MethodGet || method == http.MethodDelete || method == http.MethodHead {
if err = b.BindQueryParams(c, i); err != nil {
return err
}
}
return b.BindBody(c, i)

Yes im aware. But you could decode to a dedicated secondary struct by reflection + cloning, or even a map, and then selectively update so you don't clobber whats already been processed.

PUT /api/item/1 {"name":"hello world"}. if you have a request/update struct for incoming data, with an ID field, the ID gets zeroed out because the JSON decode clobbers, forcing duplicate parsing logic.

[aldas]

when it comes to JSON and structs with non-pointer fields - you can not distinguish if value was empty or it was not set. It is not that simple as it sounds. If you disagree I would gladly review PR for it but I can not guarantee it will be accepted.

[pcfreak30]

when it comes to JSON and structs with non-pointer fields - you can not distinguish if value was empty or it was not set. It is not that simple as it sounds. If you disagree I would gladly review PR for it but I can not guarantee it will be accepted.

I have had to deal with this recently myself so I do understand that... My suggestion would be to use an "dirty" state approach such you track what data was already decoded by the prior sources and don't clobber them.

[aldas]

another note here - if your API accepts values from different sources. For example ID from query and json body then you probably would want to validate that these values are actually same. This is another aspect then we are talking about "something overwrites value to empty". Which means you probably do want to bind/get them to different variables and compare them - for incorrect client implementations or someone trying something malicious

[pcfreak30]

another note here - if your API accepts values from different sources. For example ID from query and json body then you probably would want to validate that these values are actually same. This another aspect then we are talking about "something overwrites value to empty".

In this case the body isnt having the ID or it might not be an issue. Its if your posting to a rest endpoint with the record ID in PATH and the json data effectively zeros that out, as a update action.

[aldas]

Its if your posting to a rest endpoint with the record ID in PATH and the json data effectively zeros that out, as a update action.

but is not checking it correct solution?

well might not be an issue from Echo side at the moment - current behavior is documented "feature" and not a big issue. Unless there is a not overly complex/costly solution to avoid JSON unmarshaller to overwrite field not existing in JSON payload to empty.

As I said - I would gladly review PR or proof-of-concept for this feature.

[pcfreak30]

but is not checking it correct solution?

Please clarify? im decoding the ID and record data into a single struct intended to be an update action.

[aldas]

Example:

curl -X PUT http://server/users/1 \
  -H "Content-Type: application/json" \
  -d '{"id": 123, "name": "Example Name"}'

so we have 1 as ID from path and 123 as body. Would it not be more correct to validate that these 1==123 are the same?

I would say - do not mix path/query with json body. Probably path/query and form is ok.

[pcfreak30]

Example:

curl -X PUT http://server/users/1
-H "Content-Type: application/json"
-d '{"id": 123, "name": "Example Name"}'
so we have 1 as ID from path and 123 as body. Would it not be more correct to validate that these 1==123 are the same?

I would say - do not mix path/query with json body. Probably path/query and form is ok.

remove id from the body and you get what I consider a bug. If your using a unified struct that uses both body and args for data, there is nothing extra to verify.

[aldas]

This is documented feature, and caused by how standard library JSON unmarshalling works.
I would not recommend creating tags like that - it seems that you are probably reusing same struct for POST and PUT handlers.

		var cmd struct {
			ID   int   `path:"id" json:"id"`
			Name string `json:"name"`
		}

As I said - I would gladly review PR or proof-of-concept for fixing this feature.

[aldas]

Related #1578