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

Errors caused by invalid body content are not processed by rescue handlers #904

Closed
croeck opened this issue Jan 20, 2015 · 4 comments
Closed

Comments

@croeck
Copy link
Contributor

croeck commented Jan 20, 2015

Hi all,

I ran into a more or less severe issue today. I am running grape directly behind rack (NO rails) and found an issue that has also been described in issue #389, which however was run behind rails.

Here is the problem:
If I fire a request against my API with the body set to an random non-JSON value, for instance simply test, the MultiJson parser returns an MultiJson::ParserError error with the message 795: unexpected token at 'test' that occurs in the Grape::Middleware::Formatter and is then thrown as 400 response message.

What I would expect is that my rescue_all handler gets notified of the error and can handle the situation. Instead, Grape::Middleware::Error.call catches the thrown error, which then gets processed in the Grape::ErrorFormatter::Base.

In there, two lines (actual comments) perfectly explain the next issue:

 # env['api.endpoint'].route does not work when the error occurs within a middleware
 # the Endpoint does not have a valid env at this moment

Ok, so there the call crashes again and raises a NoMethodError. This error then finally gets processed by my rescue_all handler. Nevertheless I receive a response with the status 500 instead of the actual expected 400.

Even if the exception in the Grape::ErrorFormatter::Base would be prevented, the call would not reach the rescue handlers.

In my opinion the rescue handlers should get a chance to handle the error since it is directly related to the request body.

If a have some spare time I'll try to write a spec for the issue in the next couple of days.

croeck added a commit to croeck/grape that referenced this issue Jan 21, 2015
@croeck croeck changed the title MultiJson::ParseError caused by invalid body content is not processed by rescue handlers Errors caused by invalid body content are not processed by rescue handlers Jan 21, 2015
@croeck
Copy link
Contributor Author

croeck commented Jan 21, 2015

Ok, I wrote a small spec and found out that the issue is related to the content type definition that is sent by the user. When specifying application/json, the request fails. But when no content type is specified, the request is processed and fails (as expected) because of the missing param.

@dblock
Copy link
Member

dblock commented Jan 21, 2015

It's definitely a problem, would love it to see this fixed.

croeck added a commit to croeck/grape that referenced this issue Jan 28, 2015
… error that translates into a customized bad request response (+spec)
croeck added a commit to croeck/grape that referenced this issue Jan 28, 2015
… error that translates into a customized bad request response (+spec)
croeck added a commit to croeck/grape that referenced this issue Jan 28, 2015
… error that translates into a customized bad request response (+spec)
@croeck
Copy link
Contributor Author

croeck commented Jan 28, 2015

I updated the pull-request #906 and added a possible solution how to handle the parsing errors. It shows that I catch the error and raise a new InvalidMessageBody, which by default translates into a 400 and can be processed by any rescue handler (see included spec).

croeck added a commit to croeck/grape that referenced this issue Jan 30, 2015
… error that translates into a customized bad request response (+spec)
croeck added a commit to croeck/grape that referenced this issue Jan 30, 2015
… error that translates into a customized bad request response (+spec)
croeck added a commit to croeck/grape that referenced this issue Jan 30, 2015
… error that translates into a customized bad request response (+spec)
croeck added a commit to croeck/grape that referenced this issue Jan 30, 2015
… error that translates into a customized bad request response (+spec)

Conflicts:
	CHANGELOG.md
	lib/grape.rb
	lib/grape/locale/en.yml
croeck added a commit to croeck/grape that referenced this issue Jan 30, 2015
… error that translates into a customized bad request response (+spec)
@dm1try
Copy link
Member

dm1try commented Aug 20, 2015

Fixed in #906

@dm1try dm1try closed this as completed Aug 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants