Wrap Rack parameter parsing errors in Grape::Exceptions::RequestError

CHANGELOG.md

@@ -10,6 +10,7 @@
 * [#2665](https://github.com/ruby-grape/grape/pull/2665): Pass `attrs` directly to `AttributesIterator` instead of `validator` - [@ericproulx](https://github.com/ericproulx).
 * [#2657](https://github.com/ruby-grape/grape/pull/2657): Instantiate validators at definition time - [@ericproulx](https://github.com/ericproulx).
 * [#2667](https://github.com/ruby-grape/grape/pull/2667): Skip instrumentation in run_validators when no validators present - [@ericproulx](https://github.com/ericproulx).
+* [#2670](https://github.com/ruby-grape/grape/pull/2670): Better handling rack exceptions - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 #### Fixes

UPGRADING.md

@@ -3,6 +3,24 @@ Upgrading Grape
 
 ### Upgrading to >= 3.2
 
+#### Rack parameter parsing errors now raise `Grape::Exceptions::RequestError`
+
+Rack errors raised during parameter parsing (malformed multipart, parameter type conflicts, encoding issues, etc.) are now wrapped in `Grape::Exceptions::RequestError` instead of their previous specific exception classes (`Grape::Exceptions::EmptyMessageBody`, `Grape::Exceptions::TooManyMultipartFiles`, `Grape::Exceptions::TooDeepParameters`, `Grape::Exceptions::ConflictingTypes`, `Grape::Exceptions::InvalidParameters`). Those classes have been removed.
+
+If you rescue any of these specific exceptions, update your rescue clauses to use `Grape::Exceptions::RequestError`:
+
+```ruby
+# Before
+rescue Grape::Exceptions::ConflictingTypes, Grape::Exceptions::TooDeepParameters => e
+  # ...
+
+# After
+rescue Grape::Exceptions::RequestError => e
+  # ...
+```
+
+The error message is now forwarded directly from Rack rather than translated through Grape's locale system. On Rack 3, all Rack bad-request errors share the `Rack::BadRequest` marker module and are covered by a single rescue.
+
 #### `endpoint_run_validators.grape` notification no longer fired when there are no validators
 
 `ActiveSupport::Notifications` subscribers listening to `endpoint_run_validators.grape` will no longer receive an event for endpoints that have no validators. If you rely on this notification to measure every request, subscribe to `endpoint_run.grape` instead, which always fires.

lib/grape.rb

@@ -63,6 +63,24 @@ module Grape
     Rack::OPTIONS
   ].freeze
 
+  # Rack errors that should be rescued and wrapped as Grape::Exceptions::RequestError.
+  # Rack 3 introduced Rack::BadRequest as a marker module included by all bad request
+  # exception classes, allowing a single rescue entry to cover them all.
+  # On Rack 2, these errors are raised as individual exception classes.
+  RACK_ERRORS =
+    if defined?(Rack::BadRequest)
+      [EOFError, Rack::BadRequest]
+    else
+      [
+        EOFError,
+        Rack::Multipart::MultipartPartLimitError,
+        Rack::Multipart::MultipartTotalPartLimitError,
+        Rack::Utils::ParameterTypeError,
+        Rack::Utils::InvalidParameterError,
+        Rack::QueryParser::ParamsTooDeepError
+      ]
+    end.freeze
+
   def self.deprecator
     @deprecator ||= ActiveSupport::Deprecation.new('2.0', 'Grape')
   end

lib/grape/exceptions/request_error.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Grape
+  module Exceptions
+    class RequestError < Base
+      def initialize(status: 400)
+        super(message: $ERROR_INFO&.message, status:)
+      end
+    end
+  end
+end

lib/grape/locale/en.yml

@@ -8,8 +8,6 @@ en:
         at_least_one: 'are missing, at least one parameter must be provided'
         blank: 'is empty'
         coerce: 'is invalid'
-        conflicting_types: 'query params contains conflicting types'
-        empty_message_body: 'empty message body supplied with %{body_format} content-type'
         exactly_one: 'are missing, exactly one parameter must be provided'
         except_values: 'has a value not allowed'
         incompatible_option_values: '%{option1}: %{value1} is incompatible with %{option2}: %{value2}'
@@ -20,7 +18,6 @@ en:
         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'' '
-        invalid_parameters: 'query params contains invalid format or byte sequence'
         invalid_response: 'Invalid response'
         invalid_version_header:
           problem: 'invalid version header'
@@ -48,8 +45,6 @@ en:
         presence: 'is missing'
         regexp: 'is invalid'
         same_as: 'is not the same as %{parameter}'
-        too_deep_parameters: 'query params are recursively nested over the specified limit (%{limit})'
-        too_many_multipart_files: 'the number of uploaded files exceeded the system''s configured limit (%{limit})'
         unknown_auth_strategy: 'unknown auth strategy: %{strategy}'
         unknown_options: 'unknown options: %{options}'
         unknown_parameter: 'unknown parameter: %{param}'

lib/grape/middleware/base.rb

@@ -79,10 +79,8 @@ def content_type
 
       def query_params
         rack_request.GET
-      rescue Rack::QueryParser::ParamsTooDeepError
-        raise Grape::Exceptions::TooDeepParameters.new(Rack::Utils.param_depth_limit)
-      rescue Rack::Utils::ParameterTypeError
-        raise Grape::Exceptions::ConflictingTypes
+      rescue *Grape::RACK_ERRORS
+        raise Grape::Exceptions::RequestError
       end
 
       private

lib/grape/request.rb

@@ -166,16 +166,8 @@ def grape_routing_args
 
     def make_params
       @params_builder.call(rack_params).deep_merge!(grape_routing_args)
-    rescue EOFError
-      raise Grape::Exceptions::EmptyMessageBody.new(content_type)
-    rescue Rack::Multipart::MultipartPartLimitError, Rack::Multipart::MultipartTotalPartLimitError
-      raise Grape::Exceptions::TooManyMultipartFiles.new(Rack::Utils.multipart_part_limit)
-    rescue Rack::QueryParser::ParamsTooDeepError
-      raise Grape::Exceptions::TooDeepParameters.new(Rack::Utils.param_depth_limit)
-    rescue Rack::Utils::ParameterTypeError
-      raise Grape::Exceptions::ConflictingTypes
-    rescue Rack::Utils::InvalidParameterError
-      raise Grape::Exceptions::InvalidParameters
+    rescue *Grape::RACK_ERRORS
+      raise Grape::Exceptions::RequestError
     end
 
     def build_headers

spec/grape/endpoint_spec.rb

@@ -464,7 +464,6 @@ def handle_argument_error
         end
         post '/upload', { file: '' }, 'CONTENT_TYPE' => 'multipart/form-data; boundary=foobar'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('file is invalid')
       end
     end
 
@@ -476,16 +475,15 @@ def handle_argument_error
         Rack::Utils.multipart_part_limit = limit
       end
 
-      it 'returns a 413 if given too many multipart files' do
+      it 'returns a 400 if given too many multipart files' do
         subject.params do
           requires :file, type: Rack::Multipart::UploadedFile
         end
         subject.post '/upload' do
           params[:file][:filename]
         end
         post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain'), extra: Rack::Test::UploadedFile.new(__FILE__, 'text/plain') }
-        expect(last_response.status).to eq(413)
-        expect(last_response.body).to eq("the number of uploaded files exceeded the system's configured limit (1)")
+        expect(last_response.status).to eq(400)
       end
     end
 

spec/grape/middleware/base_spec.rb

@@ -241,15 +241,15 @@ def before
 
     context 'when query params are conflicting' do
       it 'raises an ConflictingTypes error' do
-        expect { get '/?x[y]=1&x[y]z=2' }.to raise_error(Grape::Exceptions::ConflictingTypes)
+        expect { get '/?x[y]=1&x[y]z=2' }.to raise_error(Grape::Exceptions::RequestError)
       end
     end
 
     context 'when query params is over the specified limit' do
       let(:query_params) { "foo#{'[a]' * Rack::Utils.param_depth_limit}=bar" }
 
       it 'raises an ConflictingTypes error' do
-        expect { get "/?foo#{'[a]' * Rack::Utils.param_depth_limit}=bar" }.to raise_error(Grape::Exceptions::TooDeepParameters)
+        expect { get "/?foo#{'[a]' * Rack::Utils.param_depth_limit}=bar" }.to raise_error(Grape::Exceptions::RequestError)
       end
     end
   end

spec/grape/request_spec.rb

@@ -60,75 +60,68 @@
       end
     end
 
-    context 'when rack_params raises an EOF error' do
-      before do
-        allow(request).to receive(:rack_params).and_raise(EOFError)
-      end
-
-      let(:message) { Grape::Exceptions::EmptyMessageBody.new(nil).to_s }
+    context 'when rack_params raises an EOFError' do
+      before { allow(request).to receive(:rack_params).and_raise(EOFError) }
 
-      it 'raises an Grape::Exceptions::EmptyMessageBody' do
-        expect { request.params }.to raise_error(Grape::Exceptions::EmptyMessageBody, message)
+      it 'raises a Grape::Exceptions::RequestError' do
+        expect { request.params }.to raise_error(Grape::Exceptions::RequestError)
       end
     end
 
-    context 'when rack_params raises a Rack::Multipart::MultipartPartLimitError' do
-      before do
-        allow(request).to receive(:rack_params).and_raise(Rack::Multipart::MultipartPartLimitError)
-      end
+    # Rack 3 introduced Rack::BadRequest as a marker module included by all bad request
+    # exception classes, so a single `rescue Rack::BadRequest` covers them all.
+    # On Rack 2, there is no such module and each exception class must be tested individually.
+    if defined?(Rack::BadRequest)
+      context 'when rack_params raises a custom error that includes Rack::BadRequest' do
+        let(:custom_rack_error) do
+          Class.new(StandardError) { include Rack::BadRequest }
+        end
 
-      let(:message) { Grape::Exceptions::TooManyMultipartFiles.new(Rack::Utils.multipart_part_limit).to_s }
+        before { allow(request).to receive(:rack_params).and_raise(custom_rack_error) }
 
-      it 'raises an Rack::Multipart::MultipartPartLimitError' do
-        expect { request.params }.to raise_error(Grape::Exceptions::TooManyMultipartFiles, message)
+        it 'raises a Grape::Exceptions::RequestError' do
+          expect { request.params }.to raise_error(Grape::Exceptions::RequestError)
+        end
       end
-    end
+    else
+      context 'when rack_params raises a Rack::Multipart::MultipartPartLimitError' do
+        before { allow(request).to receive(:rack_params).and_raise(Rack::Multipart::MultipartPartLimitError) }
 
-    context 'when rack_params raises a Rack::Multipart::MultipartTotalPartLimitError' do
-      before do
-        allow(request).to receive(:rack_params).and_raise(Rack::Multipart::MultipartTotalPartLimitError)
+        it 'raises a Grape::Exceptions::RequestError' do
+          expect { request.params }.to raise_error(Grape::Exceptions::RequestError)
+        end
       end
 
-      let(:message) { Grape::Exceptions::TooManyMultipartFiles.new(Rack::Utils.multipart_part_limit).to_s }
-
-      it 'raises an Rack::Multipart::MultipartPartLimitError' do
-        expect { request.params }.to raise_error(Grape::Exceptions::TooManyMultipartFiles, message)
-      end
-    end
+      context 'when rack_params raises a Rack::Multipart::MultipartTotalPartLimitError' do
+        before { allow(request).to receive(:rack_params).and_raise(Rack::Multipart::MultipartTotalPartLimitError) }
 
-    context 'when rack_params raises a Rack::QueryParser::ParamsTooDeepError' do
-      before do
-        allow(request).to receive(:rack_params).and_raise(Rack::QueryParser::ParamsTooDeepError)
+        it 'raises a Grape::Exceptions::RequestError' do
+          expect { request.params }.to raise_error(Grape::Exceptions::RequestError)
+        end
       end
 
-      let(:message) { Grape::Exceptions::TooDeepParameters.new(Rack::Utils.param_depth_limit).to_s }
+      context 'when rack_params raises a Rack::Utils::ParameterTypeError' do
+        before { allow(request).to receive(:rack_params).and_raise(Rack::Utils::ParameterTypeError) }
 
-      it 'raises a Grape::Exceptions::TooDeepParameters' do
-        expect { request.params }.to raise_error(Grape::Exceptions::TooDeepParameters, message)
-      end
-    end
-
-    context 'when rack_params raises a Rack::Utils::ParameterTypeError' do
-      before do
-        allow(request).to receive(:rack_params).and_raise(Rack::Utils::ParameterTypeError)
+        it 'raises a Grape::Exceptions::RequestError' do
+          expect { request.params }.to raise_error(Grape::Exceptions::RequestError)
+        end
       end
 
-      let(:message) { Grape::Exceptions::ConflictingTypes.new.to_s }
-
-      it 'raises a Grape::Exceptions::ConflictingTypes' do
-        expect { request.params }.to raise_error(Grape::Exceptions::ConflictingTypes, message)
-      end
-    end
+      context 'when rack_params raises a Rack::Utils::InvalidParameterError' do
+        before { allow(request).to receive(:rack_params).and_raise(Rack::Utils::InvalidParameterError) }
 
-    context 'when rack_params raises a Rack::Utils::InvalidParameterError' do
-      before do
-        allow(request).to receive(:rack_params).and_raise(Rack::Utils::InvalidParameterError)
+        it 'raises a Grape::Exceptions::RequestError' do
+          expect { request.params }.to raise_error(Grape::Exceptions::RequestError)
+        end
       end
 
-      let(:message) { Grape::Exceptions::InvalidParameters.new.to_s }
+      context 'when rack_params raises a Rack::QueryParser::ParamsTooDeepError' do
+        before { allow(request).to receive(:rack_params).and_raise(Rack::QueryParser::ParamsTooDeepError) }
 
-      it 'raises an Rack::Multipart::MultipartPartLimitError' do
-        expect { request.params }.to raise_error(Grape::Exceptions::InvalidParameters, message)
+        it 'raises a Grape::Exceptions::RequestError' do
+          expect { request.params }.to raise_error(Grape::Exceptions::RequestError)
+        end
       end
     end
   end