Less query params parsing (#2553)

ericproulx · web-flow · commit c42b66fbef82 · 2025-04-08T22:20:45.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@
 * [#2547](https://github.com/ruby-grape/grape/pull/2547): Remove jsonapi related code - [@ericproulx](https://github.com/ericproulx).
 * [#2548](https://github.com/ruby-grape/grape/pull/2548): Formatting from header acts like versioning from header - [@ericproulx](https://github.com/ericproulx).
 * [#2552](https://github.com/ruby-grape/grape/pull/2552): Fix declared params optional array - [@ericproulx](https://github.com/ericproulx).
+* [#2553](https://github.com/ruby-grape/grape/pull/2553): Improve performance of query params parsing - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 ### 2.3.0 (2025-02-08)
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,6 +1,11 @@
 Upgrading Grape
 ===============
 
+### Grape::Middleware::Base
+
+- Constant `TEXT_HTML` has been removed in favor of using literal string 'text/html'.
+- `rack_request` and `query_params` have been added. Feel free to call these in your middlewares.
+
 #### Params Builder
 
 - Passing a class to `build_with` or `Grape.config.param_builder` has been deprecated in favor of a symbolized short_name. See `SHORTNAME_LOOKUP` in [params_builder](lib/grape/params_builder.rb).
diff --git a/lib/grape/exceptions/conflicting_types.rb b/lib/grape/exceptions/conflicting_types.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Grape
+  module Exceptions
+    class ConflictingTypes < Base
+      def initialize
+        super(message: compose_message(:conflicting_types), status: 400)
+      end
+    end
+  end
+end
diff --git a/lib/grape/exceptions/invalid_parameters.rb b/lib/grape/exceptions/invalid_parameters.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Grape
+  module Exceptions
+    class InvalidParameters < Base
+      def initialize
+        super(message: compose_message(:invalid_parameters), status: 400)
+      end
+    end
+  end
+end
diff --git a/lib/grape/exceptions/too_deep_parameters.rb b/lib/grape/exceptions/too_deep_parameters.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module Grape
+  module Exceptions
+    class TooDeepParameters < Base
+      def initialize(limit)
+        super(message: compose_message(:too_deep_parameters, limit: limit), status: 400)
+      end
+    end
+  end
+end
diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml
@@ -58,4 +58,6 @@ en:
           problem: 'invalid version header'
           resolution: '%{message}'
         invalid_response: 'Invalid response'
-        query_parsing: 'query params are not parsable'
+        conflicting_types: 'query params contains conflicting types'
+        invalid_parameters: 'query params contains invalid format or byte sequence'
+        too_deep_parameters: 'query params are recursively nested over the specified limit (%{limit})'
diff --git a/lib/grape/middleware/base.rb b/lib/grape/middleware/base.rb
@@ -8,8 +8,6 @@ class Base
 
       attr_reader :app, :env, :options
 
-      TEXT_HTML = 'text/html'
-
       # @param [Rack Application] app The standard argument for a Rack middleware.
       # @param [Hash] options A hash of options, simply stored for use by subclasses.
       def initialize(app, *options)
@@ -54,6 +52,10 @@ def before; end
       # @return [Response, nil] a Rack SPEC response or nil to call the application afterwards.
       def after; end
 
+      def rack_request
+        @rack_request ||= Rack::Request.new(env)
+      end
+
       def response
         return @app_response if @app_response.is_a?(Rack::Response)
 
@@ -73,7 +75,15 @@ def content_type_for(format)
       end
 
       def content_type
-        content_type_for(env[Grape::Env::API_FORMAT] || options[:format]) || TEXT_HTML
+        content_type_for(env[Grape::Env::API_FORMAT] || options[:format]) || 'text/html'
+      end
+
+      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
       end
 
       private
diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb
@@ -39,7 +39,7 @@ def call!(env)
       private
 
       def rack_response(status, headers, message)
-        message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == TEXT_HTML
+        message = Rack::Utils.escape_html(message) if headers[Rack::CONTENT_TYPE] == 'text/html'
         Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), Grape::Util::Header.new.merge(headers))
       end
 
diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb
@@ -3,9 +3,6 @@
 module Grape
   module Middleware
     class Formatter < Base
-      CHUNKED = 'chunked'
-      FORMAT = 'format'
-
       def default_options
         {
           default_format: :txt,
@@ -69,34 +66,27 @@ def ensure_content_type(headers)
         end
       end
 
-      def request
-        @request ||= Rack::Request.new(env)
-      end
-
-      # store read input in env['api.request.input']
       def read_body_input
-        return unless
-          (request.post? || request.put? || request.patch? || request.delete?) &&
-          (!request.form_data? || !request.media_type) &&
-          !request.parseable_data? &&
-          (request.content_length.to_i.positive? || request.env[Grape::Http::Headers::HTTP_TRANSFER_ENCODING] == CHUNKED)
-
-        return unless (input = env[Rack::RACK_INPUT])
+        input = rack_request.body # reads RACK_INPUT
+        return if input.nil?
+        return unless read_body_input?
 
         input.try(:rewind)
         body = env[Grape::Env::API_REQUEST_INPUT] = input.read
         begin
-          read_rack_input(body) if body && !body.empty?
+          read_rack_input(body)
         ensure
           input.try(:rewind)
         end
       end
 
-      # store parsed input in env['api.request.body']
       def read_rack_input(body)
-        fmt = request.media_type ? mime_types[request.media_type] : options[:default_format]
+        return if body.empty?
+
+        media_type = rack_request.media_type
+        fmt = media_type ? mime_types[media_type] : options[:default_format]
 
-        throw :error, status: 415, message: "The provided content-type '#{request.media_type}' is not supported." unless content_type_for(fmt)
+        throw :error, status: 415, message: "The provided content-type '#{media_type}' is not supported." unless content_type_for(fmt)
         parser = Grape::Parser.parser_for fmt, options[:parsers]
         if parser
           begin
@@ -119,8 +109,21 @@ def read_rack_input(body)
         end
       end
 
+      # this middleware will not try to format the following content-types since Rack already handles them
+      # when calling Rack's `params` function
+      # - application/x-www-form-urlencoded
+      # - multipart/form-data
+      # - multipart/related
+      # - multipart/mixed
+      def read_body_input?
+        (rack_request.post? || rack_request.put? || rack_request.patch? || rack_request.delete?) &&
+          !(rack_request.form_data? && rack_request.content_type) &&
+          !rack_request.parseable_data? &&
+          (rack_request.content_length.to_i.positive? || rack_request.env[Grape::Http::Headers::HTTP_TRANSFER_ENCODING] == 'chunked')
+      end
+
       def negotiate_content_type
-        fmt = format_from_extension || format_from_params || options[:format] || format_from_header || options[:default_format]
+        fmt = format_from_extension || query_params['format'] || options[:format] || format_from_header || options[:default_format]
         if content_type_for(fmt)
           env[Grape::Env::API_FORMAT] = fmt.to_sym
         else
@@ -129,18 +132,14 @@ def negotiate_content_type
       end
 
       def format_from_extension
-        request_path = request.path.try(:scrub)
+        request_path = rack_request.path.try(:scrub)
         dot_pos = request_path.rindex('.')
         return unless dot_pos
 
         extension = request_path[dot_pos + 1..]
         extension if content_type_for(extension)
       end
 
-      def format_from_params
-        Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING])[FORMAT]
-      end
-
       def format_from_header
         accept_header = env[Grape::Http::Headers::HTTP_ACCEPT].try(:scrub)
         return if accept_header.blank?
diff --git a/lib/grape/middleware/versioner/param.rb b/lib/grape/middleware/versioner/param.rb
@@ -20,12 +20,11 @@ module Versioner
       #   env['api.version'] => 'v1'
       class Param < Base
         def before
-          potential_version = Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING])[parameter_key]
+          potential_version = query_params[parameter_key]
           return if potential_version.blank?
 
           version_not_found! unless potential_version_match?(potential_version)
-          env[Grape::Env::API_VERSION] = potential_version
-          env[Rack::RACK_REQUEST_QUERY_HASH].delete(parameter_key) if env.key? Rack::RACK_REQUEST_QUERY_HASH
+          env[Grape::Env::API_VERSION] = env[Rack::RACK_REQUEST_QUERY_HASH].delete(parameter_key)
         end
       end
     end
diff --git a/lib/grape/request.rb b/lib/grape/request.rb
@@ -37,8 +37,14 @@ 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
+    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
     end
 
     def build_headers
diff --git a/spec/grape/middleware/base_spec.rb b/spec/grape/middleware/base_spec.rb
@@ -223,4 +223,36 @@ def after
       expect(last_response.headers['X-Test-Overwriting']).to eq('Bye')
     end
   end
+
+  describe 'query_params' do
+    let(:dummy_middleware) do
+      Class.new(Grape::Middleware::Base) do
+        def before
+          query_params
+        end
+      end
+    end
+
+    let(:app) do
+      context = self
+      Rack::Builder.app do
+        use context.dummy_middleware
+        run ->(_) { [200, {}, ['Yeah']] }
+      end
+    end
+
+    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)
+      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)
+      end
+    end
+  end
 end
diff --git a/spec/grape/request_spec.rb b/spec/grape/request_spec.rb
@@ -59,6 +59,78 @@
         expect(request.params).to eq(ActiveSupport::HashWithIndifferentAccess.new(a: '123', b: 'xyz', c: 'ccc'))
       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 }
+
+      it 'raises an Grape::Exceptions::EmptyMessageBody' do
+        expect { request.params }.to raise_error(Grape::Exceptions::EmptyMessageBody, message)
+      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
+
+      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 do
+        allow(request).to receive(:rack_params).and_raise(Rack::Multipart::MultipartTotalPartLimitError)
+      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::QueryParser::ParamsTooDeepError' do
+      before do
+        allow(request).to receive(:rack_params).and_raise(Rack::QueryParser::ParamsTooDeepError)
+      end
+
+      let(:message) { Grape::Exceptions::TooDeepParameters.new(Rack::Utils.param_depth_limit).to_s }
+
+      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)
+      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 do
+        allow(request).to receive(:rack_params).and_raise(Rack::Utils::InvalidParameterError)
+      end
+
+      let(:message) { Grape::Exceptions::InvalidParameters.new.to_s }
+
+      it 'raises an Rack::Multipart::MultipartPartLimitError' do
+        expect { request.params }.to raise_error(Grape::Exceptions::InvalidParameters, message)
+      end
+    end
   end
 
   describe '#headers' do
