diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f7a46533..478a6b4d5 100644 --- 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 index a1d82bf80..9e7b8ade2 100644 --- 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 new file mode 100644 index 000000000..9228cd903 --- /dev/null +++ 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 new file mode 100644 index 000000000..c060c8c08 --- /dev/null +++ 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 new file mode 100644 index 000000000..c71267705 --- /dev/null +++ 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 index 35cdeef4e..212e98999 100644 --- 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 index af7195f86..d7a50d598 100644 --- 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 index 2dd39bc16..56a6d5443 100644 --- 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 index b35500c9e..73b12b454 100644 --- 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,7 +132,7 @@ 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 @@ -137,10 +140,6 @@ def format_from_extension 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 index 771faf616..102d30c55 100644 --- 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 index 1b9469c8a..45ee801e7 100644 --- 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 index a3acc39d5..7bfedcfe6 100644 --- 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 index 3ed8d8f03..4e00747cf 100644 --- 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