Merge pull request #906 from croeck/invalid-body-parse-error-not-rescued-by-handlers

Invalid body parse error not rescued by handlers

Author: dblock

CHANGELOG.md

@@ -9,6 +9,7 @@ Next Release
 * [#901](https://github.com/intridea/grape/pull/901): Fix: callbacks defined in a version block are only called for the routes defined in that block - [@kushkella](https://github.com/kushkella).
 * [#886](https://github.com/intridea/grape/pull/886): Group of parameters made to require an explicit type of Hash or Array - [@jrichter1](https://github.com/jrichter1).
 * [#912](https://github.com/intridea/grape/pull/912): Extended the `:using` feature for param documentation to `optional` fields - [@croeck](https://github.com/croeck).
+* [#906](https://github.com/intridea/grape/pull/906): Fix: invalid body parse errors are not rescued by handlers - [@croeck](https://github.com/croeck).
 * Your contribution here.
 
 0.10.1 (12/28/2014)

lib/grape.rb

@@ -57,6 +57,7 @@ module Exceptions
     autoload :IncompatibleOptionValues,       'grape/exceptions/incompatible_option_values'
     autoload :MissingGroupTypeError,          'grape/exceptions/missing_group_type'
     autoload :UnsupportedGroupTypeError,      'grape/exceptions/unsupported_group_type'
+    autoload :InvalidMessageBody,             'grape/exceptions/invalid_message_body'
   end
 
   module ErrorFormatter

lib/grape/exceptions/invalid_message_body.rb

@@ -0,0 +1,10 @@
+# encoding: utf-8
+module Grape
+  module Exceptions
+    class InvalidMessageBody < Base
+      def initialize(body_format)
+        super(message: compose_message('invalid_message_body', body_format: body_format), status: 400)
+      end
+    end
+  end
+end

lib/grape/locale/en.yml

@@ -35,4 +35,10 @@ en:
         all_or_none: 'provide all or none of parameters'
         missing_group_type: 'group type is required'
         unsupported_group_type: 'group type must be Array or Hash'
+        invalid_message_body:
+          problem: "message body does not match declared format"
+          resolution:
+            "when specifying %{body_format} as content-type, you must pass valid
+            %{body_format} in the request's 'body'
+            "
 

lib/grape/middleware/formatter.rb

@@ -81,6 +81,8 @@ def read_rack_input(body)
                 end
                 env['rack.request.form_input'] = env['rack.input']
               end
+            rescue Grape::Exceptions::Base => e
+              raise e
             rescue StandardError => e
               throw :error, status: 400, message: e.message
             end

lib/grape/parser/json.rb

@@ -4,6 +4,9 @@ module Json
       class << self
         def call(object, env)
           MultiJson.load(object)
+        rescue MultiJson::ParseError
+          # handle JSON parsing errors via the rescue handlers or provide error message
+          raise Grape::Exceptions::InvalidMessageBody, 'application/json'
         end
       end
     end

lib/grape/parser/xml.rb

@@ -4,6 +4,9 @@ module Xml
       class << self
         def call(object, env)
           MultiXml.parse(object)
+        rescue MultiXml::ParseError
+          # handle XML parsing errors via the rescue handlers or provide error message
+          raise Grape::Exceptions::InvalidMessageBody, 'application/xml'
         end
       end
     end

spec/grape/exceptions/body_parse_errors_spec.rb

@@ -0,0 +1,105 @@
+require 'spec_helper'
+
+describe Grape::Exceptions::ValidationErrors do
+  context 'api with rescue_from :all handler' do
+    subject { Class.new(Grape::API) }
+    before {
+      subject.rescue_from :all do |e|
+        rack_response 'message was processed', 400
+      end
+      subject.params do
+        requires :beer
+      end
+      subject.post '/beer' do
+        'beer received'
+      end
+    }
+
+    def app
+      subject
+    end
+
+    context 'with content_type json' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', 'CONTENT_TYPE' => 'application/json'
+        expect(last_response.status).to eq 400
+        expect(last_response.body).to eq('message was processed')
+      end
+    end
+
+    context 'with content_type xml' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', 'CONTENT_TYPE' => 'application/xml'
+        expect(last_response.status).to eq 400
+        expect(last_response.body).to eq('message was processed')
+      end
+    end
+
+    context 'with content_type text' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', 'CONTENT_TYPE' => 'text/plain'
+        expect(last_response.status).to eq 400
+        expect(last_response.body).to eq('message was processed')
+      end
+    end
+
+    context 'with no specific content_type' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', {}
+        expect(last_response.status).to eq 400
+        expect(last_response.body).to eq('message was processed')
+      end
+    end
+  end
+
+  context 'api without a rescue handler' do
+    subject { Class.new(Grape::API) }
+    before {
+      subject.params do
+        requires :beer
+      end
+      subject.post '/beer' do
+        'beer received'
+      end
+    }
+
+    def app
+      subject
+    end
+
+    context 'and with content_type json' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', 'CONTENT_TYPE' => 'application/json'
+        expect(last_response.status).to eq 400
+        expect(last_response.body).to include('message body does not match declared format')
+        expect(last_response.body).to include('application/json')
+      end
+    end
+
+    context 'with content_type xml' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', 'CONTENT_TYPE' => 'application/xml'
+        expect(last_response.status).to eq 400
+        expect(last_response.body).to include('message body does not match declared format')
+        expect(last_response.body).to include('application/xml')
+      end
+    end
+
+    context 'with content_type text' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', 'CONTENT_TYPE' => 'text/plain'
+        expect(last_response.status).to eq 400
+        expect(last_response.body).to eq('beer is missing')
+      end
+    end
+
+    context 'and with no specific content_type' do
+      it 'can recover from failed body parsing' do
+        post '/beer', 'test', {}
+        expect(last_response.status).to eq 400
+        # plain response with text/html
+        expect(last_response.body).to eq('beer is missing')
+      end
+    end
+  end
+end