-
-
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
Bind{Query,Path}Params panic on non pointer value #2365
Comments
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! UPDATE: Do you mean programmer mistakes should not incur a binding error as it should only catch request errors? UPDATE2: Something like this i mean: My echo fork suggestions |
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 Line 148 in abecadc
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: Line 131 in abecadc
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 |
Hi, this reply is a bit late. I think your example fix is OK for I'll add note in my todo list to refactor a little bit how Bind errors are handled. |
Hello there In my opinion the panicing here is ok as they are caught by using a recover middleware to return a 500 status code. So i think there is two options here: Maybe the third option is just to leave the whole thing as it is and accept the inconsistency? |
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?
The text was updated successfully, but these errors were encountered: