Rack::JSONBodyParser: support passing the parser error class

[Juanmcuello]

Allow to pass the error class to be rescued when there is a json parse error. This is specially needed when using a parser other than JSON. Otherwise we need to rescue the corresponding exception and raise JSON:ParseError inside the block.

[mpalmer]

Yes, that does seem like a useful addition. If you can add a test demonstrating the use of an alternate error class (it can be a trivial custom parser that just raises) that'd be neat-o, and it should then be good to merge.

[ioquatix]

I'm not against this change but:

• Why rescue errors here at all? Alternatively, why not rescue ALL errors around JSON.parse?
• Why assume a JSON response is the expected one? (shouldn't we check the request accept header?)
• Why not let some more general exception handling middleware deal with it? (i.e. further up the middleware chain).

[Juanmcuello]

@mpalmer , I added a new commit that uses a custom exception in the test case. I can squash the two commits if you are OK to merge it.

@ioquatix , all valid points, but it seems that going in that direction will introduce breaking changes, so not sure if this MR is the right place to do it. Meanwhile, I think you still can handle the parsing in a passed block and raise other exceptions to be rescued further up in the middleware chain if you need it.

Another point to consider in case of a refactor, could be not to depend on the JSON gem. I don't use it for example, but by requiring this middleware the json gem is required and I end up with the ruby core objects extended with the .to_json method that I would like to avoid.

Anyway, let me know what you think about this MR. No problem to close it, I can continue handling the exception in the block or create a custom middleware that suits my needs in that case.

[ioquatix]

Another point to consider in case of a refactor, could be not to depend on the JSON gem.

I also agree with this.

Would it be weird to do require 'json' unless defined?(JSON) or something?

@jeremyevans wdyt about this implementation and PR?

[jeremyevans]

Seems fine to me.

[ioquatix]

@Juanmcuello We can merge this PR, or we can step back and look at the bigger picture w.r.t my thoughts above. I don't really have a strong opinion either way, but what I will say, is merging this PR will make it harder to step back and perhaps address the bigger issues around error handling that this middleware has. LMK what you'd like to do.

[Juanmcuello]

@ioquatix , maybe we can rescue StandardError very close to the parsing block and raise a custom JSONBodyParser::ParserError:

class JSONBodyParser
+    class ParserError < StandardError; end
+
     CONTENT_TYPE_MATCHERS = {
       String => lambda { |option, header|
         Rack::MediaType.type(header) == option
@@ -61,7 +63,7 @@ module Rack
 
           update_form_hash_with_json_body(env)
         end
-      rescue JSON::ParserError
+      rescue ParserError
         body = { error: 'Failed to parse body as JSON' }.to_json
         header = { 'Content-Type' => 'application/json' }
         return Rack::Response.new(body, 400, header).finish
@@ -76,8 +78,15 @@ module Rack
       return unless (body_content = body.read) && !body_content.empty?
 
       body.rewind # somebody might try to read this stream
+
+      begin
+        parsed_body = @parser.call(body_content)
+      rescue StandardError
+        raise ParserError
+      end
+
       env.update(
-        Rack::RACK_REQUEST_FORM_HASH => @parser.call(body_content),
+        Rack::RACK_REQUEST_FORM_HASH => parsed_body,
         Rack::RACK_REQUEST_FORM_INPUT => body
       )

I think most of the parsing libraries will raise a subclass of StandardError so this way we can avoid passing the parser_error_class.

I understand we don't address all the points you mentioned, but I don't think I'll be able to do a bigger refactor in the short term.

[ioquatix]

That seems reasonable to me, I'd only suggest making ParserError a private constant.

[Juanmcuello]

@ioquatix , I pushed the changes. Let me know what you think.

[Juanmcuello]

@ioquatix , just to know if you had the chance to review the changes.

[Juanmcuello]

@ioquatix , should I close this? Or maybe go with the first proposal that was simpler? I'd rather like not having PR hanging around.

[ioquatix]

Sorry, I missed your comments for some reason.