Skip to content
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

DefaultHTTPErrorHandler: return error if marshalling JSON fails #2022

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

georgmu
Copy link
Contributor

@georgmu georgmu commented Nov 5, 2021

Before this commit, when passing a message to echo.NewHTTPError() that is not serializable, no error is returned, but a 200 Ok response with no content.
I triggered this behavior by accident when using err.Error instead of err.Error() as message.

I added test cases which trigger the problem for both c.JSON() as well as echo.NewHttpError(). With the fix, both of them now return the same result (500 Internal Server Error).

One thing open is to maybe close the whole connection if even the sending of the new error message fails.

The /bad-json-err testcase fails with current code, it returns 200 Ok instead.
This returns the same response as when calling c.JSON() directly
@aldas
Copy link
Contributor

aldas commented Nov 14, 2021

This is probably not that easy fix. c.JSON can fail because of other reasons other than object not being serializable. ala something wrong has happened to response writer. In that case we would pretty much try now write twice to broken writer and log twice that problem.

Probably DefaultJSONSerializer should check error type and if that error is type of json.UnsupportedTypeError then return some Echo specific error type that global handler knows to be worth handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants