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

Detect content type correctly if raised any low level errors #1427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hamedrnik
Copy link

@hamedrnik hamedrnik commented Jun 15, 2016

If Rack level errors raise, content type of the request couldn't be detected correctly. Basically there are two types of errors might happen in Rack level, Rack::Utils::ParameterTypeError and Rack::Utils::InvalidParameterError.
Passing query parameters like x[y]=1&x[y]z=2 and foo%81E=1 will raise the Rack level errors and the content type couldn't be detected correctly.

PS: appraisal rake spec raises two errors which are related to autoloading in Rails. How can I solve this problem?

@dblock
Copy link
Member

dblock commented Jun 15, 2016

Why would we want to swallow this error here? I would think it should just go upstream and be handled by whatever rescue_from in the Grape API declaration.

@hamedrnik
Copy link
Author

hamedrnik commented Jun 15, 2016

@dblock That's exactly what I'm doing. I have resuce_from in my app and resuce these rack level errors but the problem is content type couldn't be detected correctly and I should hard code the content type. e.g.

rescue_from Rack::Utils::ParameterTypeError do |e|
  error_model = Models::Error.new(e)

  error!({
    errors: Serializers::Errors.new.serialize([error_model])
  }, error_model.status_code, 'Content-Type' => 'application/json')
end

This patch is only for detecting the response content type correctly. That's not for handling Rack level errors.

Thanks

@dblock
Copy link
Member

dblock commented Jun 15, 2016

I get it, I'm cool with merging it on green.

@@ -11,6 +11,7 @@

#### Fixes

* [#1427](https://github.com/ruby-grape/grape/pull/1427): Detect content type in case of raising Rack level errors - [@iCEAGE](https://github.com/iCEAGE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a period at the end.

I think the description is a bit misleading. Your problem is content type, but what is being fixed is about handling an invalid query string when trying to detect the response format, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right. I updated the description. Thanks

If Rack level errors raise, content type of the request couldn't be detected correctly. Basically there are two types of errors might happen in Rack level, Rack::Utils::ParameterTypeError and Rack::Utils::InvalidParameterError.\n
Passing query parameters like `x[y]=1&x[y]z=2` and `foo%81E=1` will raise the Rack level errors and the content type couldn't be detected correctly.
@hamedrnik
Copy link
Author

hamedrnik commented Jun 15, 2016

I get it, I'm cool with merging it on green.

As I said earlier in the patch description appraisal rake spec raises uninitialized constant Rack::Utils::ParameterTypeError error. Do you know how I can solve it? I guess I should add an autoload to lib/grape.rb, shouldn't I?

@dblock
Copy link
Member

dblock commented Jun 15, 2016

If you're missing a require then obviously yes. Why doesn't it fail on Ruby 2.3.0?

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