Under Rails ActiveSupport parses the JSON request #340

Closed
23tux opened this Issue Feb 18, 2013 · 20 comments

Projects

None yet

4 participants

@23tux
23tux commented Feb 18, 2013

This was "Allow rescue of 500 - MultiJson::LoadError"

Hi,

I'm using grape along with Rails. When posting data to an endpoint, content-type: application/json, and the json is not valid (sometimes this happens), grape throws an ugly rails HTML page that says

MultiJson::LoadError (399: unexpected token at 'no json]'):

I tried to add a rescue_from :all and tried to implement a custom parser for json, but it seems, that the error occurs before all of them are invoked.

Am I doing sth wrong, or is this a bug?

@dblock
Member
dblock commented Feb 18, 2013

It's a non-feature. The input parsing is happening in a parser before we hit the part that can be caught. This is consistent with parsing multipart data in Rack, which happens before Grape. It's similar to malformed HTTP requests, too that might get trapped in the HTTP server proper. Think about it as a malformed HTTP request in which you say "i am sending JSON data", but the server looks at it and says "no, this is not properly formed JSON data, I am not going to pass it along".

All that said, I think I would like to be able to trap this error as well :)

@23tux
23tux commented Feb 19, 2013

Ok, thats a way one can think of. But it happens sometimes (especially when beginning developing a client for that api) that the client sends invalid JSON data to that endpoint. It would be really nice, if I could send a http 400 - Bad Request back, and not a 500 - Internal Server Error. It's definitely not a 500 error, as long the CLIENT sends bad data. I think an API should be able to handle this.

Is it possible in some way, to catch that error (e.g. monkey patching grape), and throw a proper 400 error as JSON data to the client?

@dblock
Member
dblock commented Feb 19, 2013

Again, I would accept a patch that lets Grape rescue this exception.

@23tux
23tux commented Feb 19, 2013

I had a look at it, but it seems, that the error occurs BEFORE grape calls MultiJson.load. I tried to hook into that point, but it seems that it never gets called. Here is the stacktrace:

Started POST "/api" for 127.0.0.1 at 2013-02-19 14:59:06 +0100
Error occurred while parsing request parameters.
Contents:



MultiJson::LoadError (399: unexpected token at 'no json]'):
  json (1.7.7) lib/json/common.rb:155:in `parse'
  json (1.7.7) lib/json/common.rb:155:in `parse'
  multi_json (1.6.1) lib/multi_json/adapters/json_common.rb:6:in `load'
  multi_json (1.6.1) lib/multi_json.rb:102:in `load'
  activesupport (3.2.12) lib/active_support/json/decoding.rb:15:in `decode'
  actionpack (3.2.12) lib/action_dispatch/middleware/params_parser.rb:47:in `parse_formatted_parameters'
  actionpack (3.2.12) lib/action_dispatch/middleware/params_parser.rb:17:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/flash.rb:242:in `call'
  rack (1.4.5) lib/rack/session/abstract/id.rb:210:in `context'
  rack (1.4.5) lib/rack/session/abstract/id.rb:205:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/cookies.rb:341:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/callbacks.rb:28:in `block in call'
  activesupport (3.2.12) lib/active_support/callbacks.rb:405:in `_run__249871641070825127__call__1870565671388814758__callbacks'
  activesupport (3.2.12) lib/active_support/callbacks.rb:405:in `__run_callback'
  activesupport (3.2.12) lib/active_support/callbacks.rb:385:in `_run_call_callbacks'
  activesupport (3.2.12) lib/active_support/callbacks.rb:81:in `run_callbacks'
  actionpack (3.2.12) lib/action_dispatch/middleware/callbacks.rb:27:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/reloader.rb:65:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/remote_ip.rb:31:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/debug_exceptions.rb:16:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/show_exceptions.rb:56:in `call'
  railties (3.2.12) lib/rails/rack/logger.rb:32:in `call_app'
  railties (3.2.12) lib/rails/rack/logger.rb:16:in `block in call'
  activesupport (3.2.12) lib/active_support/tagged_logging.rb:22:in `tagged'
  railties (3.2.12) lib/rails/rack/logger.rb:16:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/request_id.rb:22:in `call'
  rack (1.4.5) lib/rack/methodoverride.rb:21:in `call'
  rack (1.4.5) lib/rack/runtime.rb:17:in `call'
  activesupport (3.2.12) lib/active_support/cache/strategy/local_cache.rb:72:in `call'
  rack (1.4.5) lib/rack/lock.rb:15:in `call'
  actionpack (3.2.12) lib/action_dispatch/middleware/static.rb:62:in `call'
  railties (3.2.12) lib/rails/engine.rb:479:in `call'
  railties (3.2.12) lib/rails/application.rb:223:in `call'
  rack (1.4.5) lib/rack/content_length.rb:14:in `call'
  railties (3.2.12) lib/rails/rack/log_tailer.rb:17:in `call'
  rack (1.4.5) lib/rack/handler/webrick.rb:59:in `service'
  /usr/local/rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/webrick/httpserver.rb:138:in `service'
  /usr/local/rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/webrick/httpserver.rb:94:in `run'
  /usr/local/rvm/rubies/ruby-1.9.3-p194/lib/ruby/1.9.1/webrick/server.rb:191:in `block in start_thread'


  Rendered /usr/local/rvm/gems/ruby-1.9.3-p194@apono/gems/actionpack-3.2.12/lib/action_dispatch/middleware/templates/rescues/_trace.erb (1.9ms)
  Rendered /usr/local/rvm/gems/ruby-1.9.3-p194@apono/gems/actionpack-3.2.12/lib/action_dispatch/middleware/templates/rescues/_request_and_response.erb (3.9ms)
  Rendered /usr/local/rvm/gems/ruby-1.9.3-p194@apono/gems/actionpack-3.2.12/lib/action_dispatch/middleware/templates/rescues/diagnostics.erb within rescues/layout (242.9ms)

As I can see, the error is thrown from activesupport (am I right?). I'm not deeply into grape (yet), but I thought, that grape does it's own parsing of data, and don't invoke Rails specific gems?!

Or maybe sth with my setup is wrong. All the api stuff is into app/api, and inside my routes.rb I've mounted grape with a one liner mount API => '/'.

Any ideas?

@dblock
Member
dblock commented Feb 19, 2013

I think your setup is correct.

It looks like Rails is parsing the request first and chokes on this, even before reaching Grape. So the theory about Grape parsing not being rescued is incorrect. So when mounted under Rails, Grape is probably double-parsing the input, not a big deal, but certainly not ideal performance-wise and should be fixed.

Can you insert Grape earlier in the stack? Probably by inserting Grape middleware in application or environment.rb via a use instead of mount.

@dieb
Contributor
dieb commented Feb 19, 2013

I'm also with a similar issue. I'm issuing a request with application/xml content type but my grape API only supports JSON. ActiveSupport is prematurely trying to parse the xml:

REXML::ParseException (The document <clipped text> does not have a valid root):

I'm thinking @dblock is correct and this is not a grape issue. There should be a way to disable AS or maybe we shouldn't be mounting the API inside rails.

@23tux
23tux commented Feb 19, 2013

I don't know exactly how to put Grape in the stack earlier. When I just try to
config.middleware.use ::API
inside the application.rb (or development.rb), I get an error:
lib/grape/api.rb:450:ininitialize': wrong number of arguments (1 for 0) (ArgumentError)`

Another thing is, that at this point, when you use the autoload path from rails to load your api files, the files aren't loaded yet. You have to manually do the requires on top of the application.rb.

I'm sorry, but I have no idea how to do this, but would love to see it ;)

@dieb
Contributor
dieb commented Feb 19, 2013

I just found a quick/dirty solution to this. I wrote the following middleware:

# File: lib/params_parser_with_ignore.rb
module MyApp
  class ParamsParser < ActionDispatch::ParamsParser
    def initialize(app, opts = {})
      @app = app
      @opts = opts
      super
    end

    def call(env)
      if @opts[:ignore_prefix].nil? or !env['PATH_INFO'].start_with?(@opts[:ignore_prefix])
        super(env)
      else
        @app.call(env)
      end
    end
  end
end

And added the following rails initializer:

require File.dirname(__FILE__) + '/../../lib/params_parser_with_ignore.rb'

MyApp::Application.config.middleware.swap ActionDispatch::ParamsParser, MyApp::ParamsParser, :ignore_prefix => '/api'

What this does is basically avoid ActionDispatch::ParamsParser to parse input parameters whenever the request is on that specific prefix path. In my case, my api is mounted on '/api'.

What do you think?

@23tux
23tux commented Feb 19, 2013

I think this would do it for now (in my case), but a build-in solution inside grape would be great. Either we could insert Grape earlier in the middleware stack (as I wrote, I don't know how), or monkey patch ActionDistpach::ParamsParser out of Grape (but I think, this is more worse than your solution ;) ).

Any other ideas?

@23tux
23tux commented Feb 19, 2013

Another thing, maybe just for beauty: When accessing a route inside the api namespace, e.g. /api/no_route, that doesn't exist, the grape docu says, that a 404 Not Found is thrown. This error is indeed thrown, but with a HTML page from rails and a Content-Type: text/html; charset=utf-8 and not application/json. Maybe when we can fix the earlier issue, we could also hook into that 404 error to return a proper json with Content-Type:application/json

@dieb
Contributor
dieb commented Feb 19, 2013

@23tux: that's a similar problem to this one, though a completely different one. It might require a similar solution. But as you said the source of the problem is mounting grape within rails, so we're getting middlewares and thus functionalities that don't work well with the API.

@dblock
Member
dblock commented Feb 19, 2013

I think we should find and document a way to mount Grape truly "alongside" Rails, where they don't interact.

@dieb
Contributor
dieb commented Feb 19, 2013

@dblock depending on the complexity of the clean unobtrusive mount, I'd suggest a gem (grape-rails maybe?).

@dieb
Contributor
dieb commented Feb 19, 2013

@23tux I just debugged the 404 and it seems the reason is here:

https://github.com/intridea/grape/blob/master/lib/grape/api.rb#L460

Which leads here:

https://github.com/josh/rack-mount/blob/master/lib/rack/mount/route_set.rb#L161

That line passes a X-Cascade: pass, which then leads here:

https://github.com/rails/rails/blob/v3.2.11/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L18

effectively raising the "infamous" RoutingError. I'll try to write a solution to this :)

@23tux
23tux commented Feb 19, 2013

@dieb great, thanks!

@dieb
Contributor
dieb commented Feb 20, 2013

Guys @23tux @dblock,

Unrelated to the actual issue here, I just created a PR addressing the 404 issue we briefly discussed here: #342

As for the actual issue (JSON pre-parsing), I'd love to hear what you guys think about creating a new modular gem 'grape-rails' which includes the appropriate settings for a clean mount of grape inside rails.

@dblock
Member
dblock commented Feb 20, 2013

I like it. I also think that some documentation regarding Rails specifics could be moved to a gem like that.

@dblock
Member
dblock commented Apr 14, 2013

I am going to close this. The part where Rails parses data before it reaches Grape is out of our hands, I don't think there's a solution. All the other issues have been discussed and resolved. Please (re)open if you think otherwise.

@dblock dblock closed this Apr 14, 2013
@bryanrite

Ran into this thread when trying to solve the same problem. I found this which solved my problems:

https://github.com/kares/request_exception_handler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment