Use default_rescue_handler to handle invalid response

ruby-grape · darren987469 · Aug 5, 2018 · Aug 7, 2018 · Aug 9, 2018 · Aug 10, 2018
commit d2bc1b67e049c80a1f70d73f7fba3b5c13797852
diff --git a/README.md b/README.md
@@ -2254,16 +2254,7 @@ class Twitter::API < Grape::API
 end
 ```
 
-By default, object returned by `rescue_from` would be taken as response message. It is suggested to call `error!` in the last line of `rescue_from` block. But you could return `String`, `Hash`, `Array`, `Rack::Response` or even `nil` as well.
-
-```ruby
-rescue_from(:all) { error!('error') }
-rescue_from(:all) { 'error' }
-rescue_from(:all) { { message: 'error' } }
-rescue_from(:all) { [500, 'error', { 'Content-Type' => 'text/error' }] }
-rescue_from(:all) { Rack::Response.new('error', 500) }
-rescue_from(:all) { nil } # response default_message and default_status
-```
+The `rescue_from` block must return a `Rack::Response` object, or call `error!`.
 
 The `with` keyword is available as `rescue_from` options, it can be passed method name or Proc object.
 

diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb
@@ -128,18 +128,11 @@ def run_rescue_handler(handler, error)
         end
 
         response = handler.arity.zero? ? instance_exec(&handler) : instance_exec(error, &handler)
-        return response if response.is_a?(Rack::Response)
 
-        # take object returned from handler as [message, status, headers]
-        # assign default value if they are nil
-        message, status, headers = response
-        message ||= default_options[:default_message]
-        status ||= default_options[:default_status]
-
-        if headers.present? && headers.is_a?(Hash)
-          error!(message, status, headers)
+        if response.is_a?(Rack::Response)
+          response
         else
-          error!(message, status)
+          run_rescue_handler(:default_rescue_handler, Grape::Exceptions::InvalidResponse.new)
         end
       end
     end

diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb
@@ -1724,52 +1724,14 @@ class CustomError < Grape::Exceptions::Base; end
       expect(last_response.body).to eq('Formatter Error')
     end
 
-    context 'takes object returned by rescue_from as error message' do
-      it 'accepts nil and returns default message and status' do
-        subject.rescue_from(:all) { nil }
-        subject.get('/') { raise }
+    it 'uses default_rescue_handler to handle invalid response from rescue_from' do
+      subject.rescue_from(:all) { 'error' }
+      subject.get('/') { raise }
 
-        get '/'
-        expect(last_response.status).to eql 500
-        expect(last_response.body).to eql ''
-      end
-
-      it 'accepts string' do
-        subject.rescue_from(:all) { 'whatever' }
-        subject.get('/') { raise }
-
-        get '/'
-        expect(last_response.status).to eql 500
-        expect(last_response.body).to eql 'whatever'
-      end
-
-      it 'accepts hash' do
-        hash = { message: 'error' }
-        subject.rescue_from(:all) { hash }
-        subject.get('/') { raise }
-
-        get '/'
-        expect(last_response.status).to eql 500
-        expect(last_response.body).to eql hash.to_json
-      end
-
-      it 'accepts Array' do
-        subject.rescue_from(:all) { ['error', 400] }
-        subject.get('/') { raise }
-
-        get '/'
-        expect(last_response.status).to eql 400
-        expect(last_response.body).to eql 'error'
-      end
-
-      it 'accepts Rack::Response' do
-        subject.rescue_from(:all) { Rack::Response.new('error', 400) }
-        subject.get('/') { raise }
-
-        get '/'
-        expect(last_response.status).to eql 400
-        expect(last_response.body).to eql 'error'
-      end
+      expect_any_instance_of(Grape::Middleware::Error).to receive(:default_rescue_handler).and_call_original
+      get '/'
+      expect(last_response.status).to eql 500
+      expect(last_response.body).to eql 'Invalid response'
     end
   end
 

diff --git a/spec/grape/middleware/exception_spec.rb b/spec/grape/middleware/exception_spec.rb
@@ -128,7 +128,7 @@ def app
       subject do
         Rack::Builder.app do
           use Spec::Support::EndpointFaker
-          use Grape::Middleware::Error, rescue_handlers: { NotImplementedError => -> { ['rescued', 200, {}] } }
+          use Grape::Middleware::Error, rescue_handlers: { NotImplementedError => -> { Rack::Response.new('rescued', 200, {}) } }
           run ExceptionSpec::OtherExceptionApp
         end
       end