Bind{Query,Path}Params panic on non pointer value

[aria3ppp]

Here #1446 trying to address this issue but it never came into reality for no reason! BTW it's a bug as framework should not panic instead it should handle this situation gracefully!

UPDATE: Should i pr the changes needed to remedy the situation as it seems pretty straightforward to me?

[aldas]

This is not some dynamic situation where Bind sometimes receives pointer and sometime do not. It more developer problem that this part is not tested and panic for this case is maybe not 100% mos eloquent solution but still fine. I think we use panic when creating middlewares but this could be because that interface does not have error as return.

Something similar - what does standard library sql/row.scan does?

[aria3ppp]

This is not some dynamic situation where Bind sometimes receives pointer and sometime do not. It more developer problem that this part is not tested and panic for this case is maybe not 100% mos eloquent solution but still fine. I think we use panic when creating middlewares but this could be because that interface does not have error as return.

Something similar - what does standard library sql/row.scan does?

Correct me if i'm wrong: all echo binding helper methods must always get a pointer to be able to fill the data back; then it should check the input for the wrong values! Panicing in the framework is a total mess as it tries to recover and then panic again: it took me quite some time to figure it out what caused the problem :((

UPDATE: Do you mean programmer mistakes should not incur a binding error as it should only catch request errors? Then there should be an internal error type that framework can handle and return http.StatusInternalServerError HTTPError for that situation!

UPDATE2: Something like this i mean: My echo fork suggestions

[aldas]

What I mean is that this is programmer error as there is no way to clients Request to change that struct to pointer or vice versa. This means that you should have seen this if you tested it as it happens always and never-ever in production. These are cases where usually "Must*" prefix is used.

Anyway, seems that binddata has already similar checks there https://github.com/labstack/echo/blob/abecadcbdc38dfc5e0d7666243d533c85cca7597/bind.go#L148 so just add pointer check some there.

Castchecking internal error up there is without refacoring other errors unnecessary - just check for pointer and return ordinary error.

[aria3ppp]

What I mean is that this is programmer error as there is no way to clients Request to change that struct to pointer or vice versa. This means that you should have seen this if you tested it as it happens always and never-ever in production. These are cases where usually "Must*" prefix is used.

Anyway, seems that binddata has already similar checks there

https://github.com/labstack/echo/blob/abecadcbdc38dfc5e0d7666243d533c85cca7597/bind.go#L148

so just add pointer check some there. Castchecking internal error up there is without refacoring other errors unnecessary - just check for pointer and return ordinary error.

It won't reach there as it panics earlier: https://github.com/labstack/echo/blob/abecadcbdc38dfc5e0d7666243d533c85cca7597/bind.go#L131

so i think it should be consider for v5 to improve on this case if there's no backward compatible change to remedy this issue atm

[aldas]

Hi, this reply is a bit late. I think your example fix is OK for v4 but leave this error conversion part out and return plain error. If you have time, please create PR with at least 1 test checking that conditions - and we are good to go.

I'll add note in my todo list to refactor a little bit how Bind errors are handled.

[aria3ppp]

Hi, this reply is a bit late. I think your example fix is OK for v4 but leave this error conversion part out and return plain error. If you have time, please create PR with at least 1 test checking that conditions - and we are good to go.

I'll add note in my todo list to refactor a little bit how Bind errors are handled.

Hello there I'm super late here

In my opinion the panicing here is ok as they are caught by using a recover middleware to return a 500 status code. The problem stem from inconsistencies between BindBody and other bind methods (BindPathParams, BindQueryParams, BindHeaders) and even in BindBody itself as with requests content type json/xml the non-pointer destination value errors are caught by json/xml deserialisation functions but forms/paths/queries/headers are parsed by bindData will panic if the destination is non-pointer.

So i think there is two options here: One is to catch non-pointer destination value on json/xml part and then panic on them and leave bindData function as it is. Or catch the non-poninter destination value on bindData and blame request instead of the server by returning an 400 status code just to conform to json/xml binding code.

Maybe the third option is just to leave the whole thing as it is and accept the inconsistency?