From ab47b68875ab1d9de80d93e960cb8dcc50872c2d Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Fri, 11 Sep 2026 16:13:22 +0200 Subject: [PATCH] Speed up header versioning, content negotiation, params validation and error responses Thirteen request-path changes, each measured on its own and together: Versioning and content negotiation - Header versioner: answer the Accept headers most requests send (every declared media type, */*, none) from a table built once per list of media types and shared through Grape::Util::Cache, instead of running Rack::Utils.best_q_match and MediaType.parse per request. MediaType is now immutable, so the api.* env strings it writes are frozen (UPGRADING). - Versioners read cascade/parameter/strict/vendor off instance variables instead of two Forwardable hops and two Data readers. - Formatter: the same kind of shared table for Accept-negotiated formats when no format is pinned. Params validation - A required, dependency-free Hash scope (the root scope included) is validated without the attributes iterator, and skips should_validate?, which always answers true for it. - So are the elements of an Array scope that is the only one iterating elements on such a chain, required or optional. - The attributes iterator settles its per-scope state once per pass. - qualifying_params returns early for scopes other than `given`. - declared skips the renamed-params lookup when nothing is renamed. - DryTypeCoercer uses dry-types' non-raising block form, so a rejected value no longer builds a re-raised backtrace. Error responses - The default rescue handler no longer reads the exception's backtrace unless one is asked for. - Formatter lets a parser's Grape errors through without re-raising them. - rack_response hands its fresh headers to Rack::Response as they are, Method rescue handlers are called directly, and ensure_utf8 returns a valid UTF-8 message as it is. - ValidationErrors#full_messages is translated once instead of on every call. Adds specs for behaviour the changes brought to light and nothing pinned, each found by a mutation that passed the suite. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + UPGRADING.md | 14 ++++ lib/grape/declared_params_handler.rb | 5 +- lib/grape/error_formatter/json.rb | 3 + lib/grape/exceptions/validation_errors.rb | 14 +++- lib/grape/middleware/error.rb | 26 ++++++- lib/grape/middleware/formatter.rb | 65 +++++++++++++--- lib/grape/middleware/versioner/base.rb | 11 ++- lib/grape/middleware/versioner/header.rb | 38 +++++++++- lib/grape/util/media_type.rb | 17 +++-- lib/grape/validations/attributes_iterator.rb | 50 +++++++------ lib/grape/validations/params_scope.rb | 24 ++++++ .../validations/types/dry_type_coercer.rb | 30 +++++--- lib/grape/validations/validators/base.rb | 74 ++++++++++++++++++- spec/grape/error_formatter/json_spec.rb | 30 ++++++++ .../exceptions/validation_errors_spec.rb | 9 +++ spec/grape/middleware/formatter_spec.rb | 23 ++++++ .../grape/middleware/versioner/header_spec.rb | 11 +++ spec/grape/validations/params_scope_spec.rb | 61 +++++++++++++++ spec/grape/validations_spec.rb | 26 +++++++ 20 files changed, 472 insertions(+), 60 deletions(-) create mode 100644 spec/grape/error_formatter/json_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index ff891c6ad..32084c506 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ * [#2921](https://github.com/ruby-grape/grape/pull/2921): Pin the router's request-time isolation regressions through requests instead of its instance variables - [@ericproulx](https://github.com/ericproulx). * [#2935](https://github.com/ruby-grape/grape/pull/2935): Return the elements of an Array params scope that are not a Hash from `declared` instead of raising - [@ericproulx](https://github.com/ericproulx). * [#2937](https://github.com/ruby-grape/grape/pull/2937): Forward the class methods kept off an API class to its base instance explicitly, and pin what `delegate_missing_to` still answers - [@ericproulx](https://github.com/ericproulx). +* [#2936](https://github.com/ruby-grape/grape/pull/2936): Speed up header versioning, content negotiation, params validation and error responses on the request path - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 4.0.0 (2026-09-07) diff --git a/UPGRADING.md b/UPGRADING.md index 965065fc5..2a2e408d0 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,6 +1,20 @@ Upgrading Grape =============== +### Upgrading to >= 4.1.0 + +#### The header versioner's `api.*` env values are frozen + +`version ..., using: :header` now answers the Accept headers most requests send from a table built once, so every request sending the same header is handed the same parsed media type ([#2936](https://github.com/ruby-grape/grape/pull/2936)). The strings it writes into the env — `api.type`, `api.subtype`, `api.vendor`, `api.version` and `api.format` — are therefore frozen, as is `Grape::Util::MediaType` itself. Code that altered one of them in place now raises `FrozenError`; build a new String instead: + +```ruby +# Before +env['api.version'] << '-beta' + +# After +env['api.version'] = "#{env['api.version']}-beta" +``` + ### Upgrading to >= 4.0.0 #### A positional options Hash is no longer accepted by `auth`, `http_basic` or `desc` diff --git a/lib/grape/declared_params_handler.rb b/lib/grape/declared_params_handler.rb index 254abfba2..b9a9a0001 100644 --- a/lib/grape/declared_params_handler.rb +++ b/lib/grape/declared_params_handler.rb @@ -96,8 +96,11 @@ def declare_leaf(passed_params, declared_param:, params_nested_path:, memo:, ren end end + # The lookup key is the param's whole path, built fresh -- two Arrays and a + # String per declared param -- and only an API using +as:+ has anything to + # find with it, so the common empty table is not asked. def build_memo_key(params_nested_path, declared_param, renamed_params) - renamed_param_name = renamed_params[nested_path_for(params_nested_path, declared_param)] + renamed_param_name = renamed_params[nested_path_for(params_nested_path, declared_param)] if renamed_params.any? param = renamed_param_name || declared_param @stringify ? param.to_s : param.to_sym end diff --git a/lib/grape/error_formatter/json.rb b/lib/grape/error_formatter/json.rb index fe34a904b..5b3ef469a 100644 --- a/lib/grape/error_formatter/json.rb +++ b/lib/grape/error_formatter/json.rb @@ -23,8 +23,11 @@ def wrap_message(message) { error: ensure_utf8(message) } end + # Re-encoding a String that already is valid UTF-8 only copies it, for + # about 260 ns on every error response. def ensure_utf8(message) return message unless message.respond_to? :encode + return message if message.is_a?(String) && message.encoding == Encoding::UTF_8 && message.valid_encoding? message.encode('UTF-8', invalid: :replace, undef: :replace) end diff --git a/lib/grape/exceptions/validation_errors.rb b/lib/grape/exceptions/validation_errors.rb index 9fad6f94c..71a0723a7 100644 --- a/lib/grape/exceptions/validation_errors.rb +++ b/lib/grape/exceptions/validation_errors.rb @@ -23,7 +23,19 @@ def to_json(*_opts) as_json.to_json end + # Translated once, when the error is built for its #message, and handed + # out as a copy from then on. Every lookup here is an I18n call of a few + # microseconds, and the README's recipe for answering with the list, + # +error!({ messages: e.full_messages }, 400)+, asked for all of them a + # second time. It also keeps the list the same as #message, which was + # already fixed at that point, if the locale changes in between. def full_messages + (@full_messages ||= translate_full_messages).dup + end + + private + + def translate_full_messages messages = errors.flat_map do |attributes, errs| errs.map do |error| translate( @@ -39,8 +51,6 @@ def full_messages messages end - private - def translate_attributes(keys) keys.map do |key| translate(key, scope: 'grape.errors.attributes', default: key.to_s) diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index a2dd7c48a..4cda7d7a4 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -66,9 +66,13 @@ def call!(env) private + # +headers+ is handed over as it is: Rack::Response copies it into a + # Headers of its own on every supported Rack (Rack 2.2 through + # +HeaderHash[]+, which only adopts a Hash that already is one), and every + # caller builds it fresh for this response. def rack_response(status, headers, message) body = html_content_type?(headers[Rack::CONTENT_TYPE]) ? Rack::Utils.escape_html(message) : message - Rack::Response.new(Array.wrap(body), Rack::Utils.status_code(status), Grape::Util::Header.new.merge!(headers)) + Rack::Response.new(Array.wrap(body), Rack::Utils.status_code(status), headers) end # Escaping must key off the media type only, case-insensitively. Comparing @@ -192,11 +196,13 @@ def failsafe_payload(headers) ) end + # No +backtrace:+: #resolved_backtrace reads it off +original_exception+ + # when the API asked for one, and only then, since building it is the + # dearest part of rendering the error. def default_rescue_handler(exception) error_response( Grape::Exceptions::ErrorResponse.new( message: exception.message, - backtrace: exception.backtrace, original_exception: exception ) ) @@ -268,7 +274,7 @@ def rescue_handler_for_any_class(klass) def run_rescue_handler(handler, error, endpoint, redispatched: false) callable = handler.is_a?(Symbol) ? endpoint.public_method(handler) : handler response = catch(:error) do - callable.arity.zero? ? endpoint.instance_exec(&callable) : endpoint.instance_exec(error, &callable) + call_rescue_handler(callable, error, endpoint) rescue StandardError => e return redispatch(e, endpoint, redispatched) end @@ -279,6 +285,20 @@ def run_rescue_handler(handler, error, endpoint, redispatched: false) run_rescue_handler(method(:default_rescue_handler), Grape::Exceptions::InvalidResponse.new, endpoint) end + # A +rescue_from+ block runs as the endpoint. A Method (the middleware's + # own handlers, or the endpoint's for a +with:+ Symbol) is already bound + # to a receiver that instance_exec cannot change, so it is called as it + # is rather than turned into a Proc first. + def call_rescue_handler(callable, error, endpoint) + if callable.is_a?(Method) + callable.arity.zero? ? callable.call : callable.call(error) + elsif callable.arity.zero? + endpoint.instance_exec(&callable) + else + endpoint.instance_exec(error, &callable) + end + end + # Route an exception raised inside a +rescue_from+ block. # # * If we have already redispatched once (the redispatched handler diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 775c026fd..8ddfb3d9e 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -13,6 +13,40 @@ def initialize(content_types: nil, default_format: :txt, format: nil, formatters ALL_MEDIA_TYPES = '*/*' + # @api private + # The format an Accept header asks for out of +mime_types+, or nil. + # Callers scrub the header first. + # + # Media types are case-insensitive (RFC 9110 §8.3.1) but the registered + # ones are spelled in lower case and Rack matches them literally, so an + # `Accept: TEXT/PLAIN` found nothing and fell through to the default + # format — the client quietly got something other than what it asked for. + def self.format_for_accept(accept_header, mime_types) + return if accept_header.blank? || accept_header == ALL_MEDIA_TYPES + + media_type = Rack::Utils.best_q_match(accept_header.downcase, mime_types.keys) + mime_types[media_type] if media_type + end + + # +format_for_accept+ answers from the header and +mime_types+ alone. A + # client that names the format it wants sends one of those media types as + # it is registered (+application/json+), and most others send +*/*+ or + # nothing, so those answers are worked out once instead of re-running + # Rack's q-value match on every request. Any other header misses and is + # negotiated as before; only registered media types are keys, so a client + # cannot grow a table. + # + # Shared per +mime_types+, which Grape::ContentTypes already shares per + # content-type registry: every endpoint builds its own formatter. + class FormatForAcceptCache < Grape::Util::Cache + def initialize + super + @cache = Hash.new do |h, mime_types| + h[mime_types] = [*mime_types.keys, ALL_MEDIA_TYPES, nil].to_h { |accept| [accept, Formatter.format_for_accept(accept, mime_types)] }.freeze + end + end + end + # The request methods that can carry a body worth parsing. See # {#read_body_input?}, which tests the env against this before anything # asks for a Rack::Request. QUERY is here because its content *is* the @@ -36,6 +70,8 @@ def initialize(app, **options) @formatters = config.formatters @parsers = config.parsers mime_types + # Only an API that pins no format negotiates one from the Accept header. + @format_for_accept = FormatForAcceptCache[mime_types] unless format end def before @@ -152,13 +188,23 @@ def read_rack_input(body) end env[Rack::RACK_REQUEST_FORM_INPUT] = env[Rack::RACK_INPUT] end - rescue Grape::Exceptions::Base => e - raise e - rescue StandardError => e + rescue ForeignParserError => e throw :error, Grape::Exceptions::ErrorResponse.new(status: 400, message: e.message, backtrace: e.backtrace, original_exception: e) end end + # What a parser raises that is not a Grape error, and so is answered as a + # 400 here. A Grape error goes on to the error middleware as it is. It + # used to be rescued just to be raised again, and re-raising at request + # depth cost about 25 µs -- paid by every malformed body, since the + # built-in parsers report one as InvalidMessageBody. + module ForeignParserError + def self.===(exception) + exception.is_a?(StandardError) && !exception.is_a?(Grape::Exceptions::Base) + end + end + private_constant :ForeignParserError + # 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 @@ -221,16 +267,11 @@ def format_from_query query_params['format'] end - # Media types are case-insensitive (RFC 9110 §8.3.1) but the registered - # ones are spelled in lower case and Rack matches them literally, so an - # `Accept: TEXT/PLAIN` found nothing and fell through to the default - # format — the client quietly got something other than what it asked for. + # The keys are registered media types -- valid strings, which scrubbing + # leaves alone -- so only a miss needs the header scrubbed. def format_from_header - accept_header = try_scrub(env['HTTP_ACCEPT']) - return if accept_header.blank? || accept_header == ALL_MEDIA_TYPES - - media_type = Rack::Utils.best_q_match(accept_header.downcase, mime_types.keys) - mime_types[media_type] if media_type + accept_header = env['HTTP_ACCEPT'] + @format_for_accept.fetch(accept_header) { Formatter.format_for_accept(try_scrub(accept_header), mime_types) } end end end diff --git a/lib/grape/middleware/versioner/base.rb b/lib/grape/middleware/versioner/base.rb index 646bcbf23..d13b048ac 100644 --- a/lib/grape/middleware/versioner/base.rb +++ b/lib/grape/middleware/versioner/base.rb @@ -27,11 +27,20 @@ def self.inherited(klass) attr_reader :available_media_types, :error_headers, :versions + # Read off ivars rather than delegated through +version_options+ into + # +config+: the versioners ask for +vendor+, +strict+ or +parameter+ on + # every request, and each read went two Forwardable frames and two Data + # readers deep for a value fixed when the middleware was built. + attr_reader :cascade, :parameter, :strict, :vendor + def_delegators :config, :mount_path, :prefix, :version_options - def_delegators :version_options, :cascade, :parameter, :strict, :vendor def initialize(app, **options) super + @cascade = version_options.cascade + @parameter = version_options.parameter + @strict = version_options.strict + @vendor = version_options.vendor @versions = config.versions&.map(&:to_s) # making sure versions are strings to ease potential match @error_headers = cascade ? CASCADE_PASS_HEADER : {} @available_media_types = build_available_media_types diff --git a/lib/grape/middleware/versioner/header.rb b/lib/grape/middleware/versioner/header.rb index 26d83050c..2fd9f70ab 100644 --- a/lib/grape/middleware/versioner/header.rb +++ b/lib/grape/middleware/versioner/header.rb @@ -22,6 +22,42 @@ module Versioner # X-Cascade header to alert Grape::Router to attempt the next matched # route. class Header < Base + # Accept headers answered up front besides the declared media types: + # +*/*+, what curl, Net::HTTP and most HTTP libraries send unless told + # otherwise, and no Accept header at all. + COMMON_ACCEPT_HEADERS = ['*/*', nil].freeze + + # +MediaType.best_quality+ answers from the header and + # +available_media_types+ alone, and the latter is fixed once the + # middleware is built. Most requests send one of a handful of headers -- + # a declared media type spelled as declared (+application/vnd.acme-v1+json+) + # or one of COMMON_ACCEPT_HEADERS -- so their answers are worked out + # once instead of re-running Rack's q-value match and a parse on every + # request. Any other header (a q-value list, another casing) is not a + # key and takes the full path, so a client cannot grow a table. + # + # Shared per list: every endpoint builds its own versioner, and all of + # an API's declare the same media types. The key is a frozen copy, as + # the middleware's own list stays reachable through + # +available_media_types+. + class MediaTypeForAcceptCache < Grape::Util::Cache + def initialize + super + @cache = Hash.new do |h, available_media_types| + declared = available_media_types.map(&:-@).freeze + h[declared] = [*declared, *COMMON_ACCEPT_HEADERS].each_with_object({}) do |accept, media_types| + media_type = Grape::Util::MediaType.best_quality(accept, declared) + media_types[accept] = media_type if media_type + end.freeze + end + end + end + + def initialize(app, **options) + super + @media_type_for_accept = MediaTypeForAcceptCache[available_media_types] + end + def before match_best_quality_media_type! do |media_type| env.update( @@ -40,7 +76,7 @@ def match_best_quality_media_type! return unless vendor strict_header_checks! - media_type = Grape::Util::MediaType.best_quality(accept_header, available_media_types) + media_type = @media_type_for_accept[accept_header] || Grape::Util::MediaType.best_quality(accept_header, available_media_types) return yield media_type if media_type fail! diff --git a/lib/grape/util/media_type.rb b/lib/grape/util/media_type.rb index e4f041f19..05331f834 100644 --- a/lib/grape/util/media_type.rb +++ b/lib/grape/util/media_type.rb @@ -13,14 +13,19 @@ class MediaType # in the case they will be compared in. VENDOR_VERSION_HEADER_REGEX = /\Avnd\.(?[a-z0-9.\-_!^]+?)(?:-(?[a-z0-9*.]+))?(?:\+(?[a-z0-9*\-.]+))?\z/ + # Immutable, strings included: the header versioner shares one instance + # per declared media type across every request that sends it, and these + # strings are what it writes into the env. The arguments are copied + # rather than frozen, as they are the caller's. def initialize(type:, subtype:) - @type = type - @subtype = subtype - VENDOR_VERSION_HEADER_REGEX.match(subtype) do |m| - @vendor = m[:vendor] - @version = m[:version] - @format = m[:format] + @type = -type + @subtype = -subtype + VENDOR_VERSION_HEADER_REGEX.match(@subtype) do |m| + @vendor = m[:vendor].freeze + @version = m[:version].freeze + @format = m[:format].freeze end + freeze end def ==(other) diff --git a/lib/grape/validations/attributes_iterator.rb b/lib/grape/validations/attributes_iterator.rb index 2af39e7ec..32ee8c60f 100644 --- a/lib/grape/validations/attributes_iterator.rb +++ b/lib/grape/validations/attributes_iterator.rb @@ -19,20 +19,42 @@ def initialize(attrs, scope) def each(params, &) original_params = @scope.params(params) + iterates_elements = @scope.iterates_elements? # A scope resolves to a Hash unless the declaration nests arrays, and # then #do_each has nothing to do but hand it straight back: Array.wrap # boxes it, the loop unboxes it on its only iteration, and with no Array # anywhere neither the nesting descent nor the index bookkeeping # applies. Every validator on a flat +params+ block comes through here. - return yield_attributes(original_params, &) if original_params.is_a?(Hash) && !@scope.iterates_elements? + return yield_attributes(original_params, &) if original_params.is_a?(Hash) && !iterates_elements + + array_params = original_params.is_a?(Array) + # Do not validate the content of an array scope that did not get one. + return if iterates_elements && !array_params + + # Where each element's index is recorded depends on the scope and on + # whether its params are an Array, never on the element, so it is + # settled once here rather than per element. A lateral scope (no + # @element) whose params resolved to an array hands its index to the + # nearest element-iterating ancestor, so full_name still produces the + # right bracketed index. + index_scope = iterates_elements ? @scope : (@scope.nearest_array_ancestor if array_params) + # No tracker means we're outside a ParamScopeTracker.track block (e.g. + # a unit test that invokes a validator directly). Index tracking is + # skipped — full_name will produce bracket-less names — but validation + # continues rather than crashing. + tracker = ParamScopeTracker.current if index_scope # because we need recursion for nested arrays - do_each(Array.wrap(original_params), original_params, &) + do_each(Array.wrap(original_params), tracker, index_scope, NO_PARENT_INDICES, &) end private - def do_each(params_to_process, original_params, parent_indices = [], &block) + # The top-level call's parent indices; only ever read. + NO_PARENT_INDICES = [].freeze + private_constant :NO_PARENT_INDICES + + def do_each(params_to_process, tracker, index_scope, parent_indices, &block) params_to_process.each_with_index do |resource_params, index| # when we get arrays of arrays it means that target element located inside array # we need this because we want to know parent arrays indices @@ -42,32 +64,16 @@ def do_each(params_to_process, original_params, parent_indices = [], &block) # validators see a non-hash and fail it the same way any other # unexpected element type does. if resource_params.is_a?(Array) && parent_indices.size < @max_nesting - do_each(resource_params, original_params, [index] + parent_indices, &block) + do_each(resource_params, tracker, index_scope, [index] + parent_indices, &block) next end - if @scope.iterates_elements? - next unless original_params.is_a?(Array) # do not validate content of array if it isn't array - - store_indices(@scope, index, parent_indices) - elsif original_params.is_a?(Array) - # Lateral scope (no @element) whose params resolved to an array — - # delegate index tracking to the nearest element-iterating ancestor - # so that full_name produces the correct bracketed index. - target = @scope.nearest_array_ancestor - store_indices(target, index, parent_indices) if target - end - + store_indices(tracker, index_scope, index, parent_indices) if tracker yield_attributes(resource_params, &block) end end - def store_indices(target_scope, index, parent_indices) - # No tracker means we're outside a ParamScopeTracker.track block (e.g. - # a unit test that invokes a validator directly). Index tracking is - # skipped — full_name will produce bracket-less names — but validation - # continues rather than crashing. - tracker = ParamScopeTracker.current or return + def store_indices(tracker, target_scope, index, parent_indices) parent_scope = target_scope.parent parent_indices.each do |parent_index| break unless parent_scope diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index be26ab992..f7a244b2e 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -5,7 +5,14 @@ module Validations class ParamsScope attr_reader :parent, :type, :nearest_array_ancestor, :array_depth, :full_path + # The elements a +given+ scope narrowed its Array params down to during + # this request (see #meets_dependency?). Only a scope with a dependency + # ever stores any, so every other scope answers without the fiber-storage + # and tracker lookups -- which #params pays on each nested resolution, + # twice per validator, on every request. def qualifying_params + return unless @dependent_on + ParamScopeTracker.current&.qualifying_params(self) end @@ -166,6 +173,23 @@ def root? !@parent end + # Whether #should_validate? answers true for every request: this scope + # and each of its ancestors is required and depends on nothing, so + # neither the params nor the parent chain have anything to say. + # @return [Boolean] + def always_validated? + !@optional && validated_when_given? + end + + # Whether #should_validate? has nothing to ask but whether this scope's + # own params were given: it depends on no other param, and every scope + # above it is always validated. A required scope like that is then + # always validated, an optional one whenever its params are there. + # @return [Boolean] + def validated_when_given? + !@dependent_on && (@parent.nil? || @parent.always_validated?) + end + # A nested scope is contained in one of its parent's elements. # @return [Boolean] whether or not this scope is nested def nested? diff --git a/lib/grape/validations/types/dry_type_coercer.rb b/lib/grape/validations/types/dry_type_coercer.rb index a39fc02db..2a9ef9501 100644 --- a/lib/grape/validations/types/dry_type_coercer.rb +++ b/lib/grape/validations/types/dry_type_coercer.rb @@ -17,14 +17,10 @@ class << self # collection_coercer_for(Array) # #=> Grape::Validations::Types::ArrayCoercer def collection_coercer_for(type) - case type - when Array - ArrayCoercer - when Set - SetCoercer - else - raise ArgumentError, "unknown type: `#{type}`" - end + return ArrayCoercer if type.is_a?(Array) + return SetCoercer if type.is_a?(Set) + + raise ArgumentError, "unknown type: `#{type}`" end # Returns an instance of a coercer for a given type @@ -43,13 +39,25 @@ def initialize(type, strict: false) # Coerces the given value to a type which was specified during # initialization as a type argument. # + # Given a block, dry-types reports a value it cannot coerce by calling + # the block instead of raising. Raising is what cost: its CoercionError + # is re-raised with the backtrace of the error underneath, and building + # that backtrace as strings at request depth took about 25 µs for every + # rejected value -- every `types: [Integer, String]` param given a + # string, every 400 for a mistyped value. + # + # Every coercion dry-types runs takes that block, so none of them + # raises a CoercionError here. Anything that raises something else -- + # +Kernel#String+, which +Coercible::String+ is built from, ignores the + # block and raises TypeError -- is answered by + # +CoerceValidator#coerce_value+, which rescues StandardError with the + # same InvalidValue. + # # @param val [Object] def call(val) return if val.nil? - @coercer[val] - rescue Dry::Types::CoercionError - InvalidValue.new + @coercer.call(val) { InvalidValue.new } end protected diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 735661eff..2881fe5fd 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -69,6 +69,12 @@ def initialize(attrs, options, required, scope, opts) @opts = SharedOptions.new(**opts.slice(:allow_blank, :fail_fast)) @exception_message = message(self.class.default_message_key) if self.class.default_message_key @iterator = iterator_class.new(@attrs, @scope).freeze + # A scope's parent, optionality, dependency and type are all set + # before its block declares anything, so these are settled by the + # time a validator is built. See #validate and #validate!. + @always_validated = scope.always_validated? + @direct = @always_validated && !scope.iterates_elements? + @direct_elements = scope.validated_when_given? && scope.iterates_elements? && scope.array_depth == 1 end # Validates a given request. @@ -78,7 +84,7 @@ def initialize(attrs, options, required, scope, opts) # @return [void] def validate(request) params = request.params - return unless scope.should_validate?(params) + return unless @always_validated || scope.should_validate?(params) validate!(params) end @@ -90,6 +96,11 @@ def validate(request) # @raise [Grape::Exceptions::Validation] if validation failed # @return [void] def validate!(params) + return validate_elements!(scope.params(params)) if @direct_elements + + scoped = scope.params(params) if @direct + return validate_attributes!(scoped) if scoped.is_a?(Hash) + # we collect errors inside array because # there may be more than one error per field array_errors = nil @@ -123,6 +134,67 @@ def validate_param!(attr_name, params) alias required? required + # #validate! on a scope that always validates and does not iterate + # elements, once its params resolved to a Hash: the root scope and the + # required Hash scopes under it, which is most validators of most + # endpoints. There the iterator hands back that Hash once per + # attribute, and every scope on the chain is required with no + # dependency, so the per-attribute checks reduce to this. The + # machinery cost more than the validation itself. + def validate_attributes!(params) + array_errors = nil + + @attrs.each do |attr_name| + validate_param!(attr_name, params) if required? || params.key?(attr_name) + rescue Grape::Exceptions::Validation => e + (array_errors ||= []) << e + end + + raise Grape::Exceptions::ValidationArrayErrors.new(array_errors) if array_errors + end + + # #validate_attributes! for each element of an Array scope, such as + # +requires :items, type: Array do+ at the root, when it is the only + # scope on the chain that iterates elements: its params are then the + # request's Array as it came in, with no nesting for the iterator to + # descend into. It depends on no other param and every scope above it + # is always validated, so the iterator's per-element checks come down + # to the index it records for the error names and, for an optional + # scope, passing over an empty element. An element that is not a Hash + # goes through the same +hash_like?+ test as on the iterator path, and + # the members of a scope that did not get an Array are left alone, as + # they are there: the scope's own type check reports it. + def validate_elements!(elements) + return unless elements.is_a?(Array) + + tracker = ParamScopeTracker.current + optional = !scope.required? + array_errors = nil + + elements.each_with_index do |element, index| + tracker&.store_index(scope, index) + next if optional && empty_element?(element) + + @attrs.each do |attr_name| + validate_param!(attr_name, element) if required? || (hash_like?(element) && element.key?(attr_name)) + rescue Grape::Exceptions::Validation => e + (array_errors ||= []) << e + end + end + + raise Grape::Exceptions::ValidationArrayErrors.new(array_errors) if array_errors + end + + # What the iterator passes over in an optional scope: an element given + # empty, or the placeholder +map_params+ puts where an optional scope's + # params were not given at all, which it can only do here when a scope + # above was handed an Array instead of a Hash. + def empty_element?(element) + return true if Grape::DSL::Parameters::EmptyOptionalValue.equal?(element) + + element.respond_to?(:empty?) ? element.empty? : element.nil? + end + # The AttributesIterator subclass used to walk this validator's # attributes. Built once in #initialize and reused across requests. def iterator_class diff --git a/spec/grape/error_formatter/json_spec.rb b/spec/grape/error_formatter/json_spec.rb new file mode 100644 index 000000000..2b9676a4a --- /dev/null +++ b/spec/grape/error_formatter/json_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +describe Grape::ErrorFormatter::Json do + let(:app) do + Class.new(Grape::API) do + format :json + get('/utf8') { error!('café', 400) } + get('/binary') { error!("caf\xC3\xA9".b, 400) } + get('/malformed') { error!("caf\xC3", 400) } + end + end + + # A String message goes out as UTF-8 whatever it arrived as: one that already + # is valid UTF-8 as it is, anything else converted, with what does not + # convert replaced rather than failing the response. + it 'renders a UTF-8 message as it is' do + get '/utf8' + expect(JSON.parse(last_response.body)).to eq('error' => 'café') + end + + it 'converts a binary message, replacing the bytes it cannot map' do + get '/binary' + expect(JSON.parse(last_response.body)).to eq('error' => 'caf��') + end + + it 'replaces the invalid bytes of a malformed UTF-8 message' do + get '/malformed' + expect(JSON.parse(last_response.body)).to eq('error' => 'caf�') + end +end diff --git a/spec/grape/exceptions/validation_errors_spec.rb b/spec/grape/exceptions/validation_errors_spec.rb index 8c0839f9b..a80416fc2 100644 --- a/spec/grape/exceptions/validation_errors_spec.rb +++ b/spec/grape/exceptions/validation_errors_spec.rb @@ -65,6 +65,15 @@ expect(subject.first).to eq('admin_field Can not set admin-only field') end end + + context 'when the caller changes the array it was given' do + subject(:error) { described_class.new(exceptions: [Grape::Exceptions::Validation.new(params: ['id'], message: :presence)]) } + + it 'returns the same messages the next time' do + error.full_messages << 'name is missing' + expect(error.full_messages).to eq(['id is missing']) + end + end end context 'api' do diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb index c3ea04ddb..648811e07 100644 --- a/spec/grape/middleware/formatter_spec.rb +++ b/spec/grape/middleware/formatter_spec.rb @@ -527,4 +527,27 @@ def self.call(_, _) expect(error.original_exception.class).to eq StandardError end end + + # Only a parser's StandardErrors are answered with a 400. Anything else -- + # an Interrupt, a SystemExit -- is not the body's fault and keeps going. + context 'custom parser raises an exception that is not a StandardError' do + it 'lets it through rather than answering 400' do + subject = described_class.new( + app, + parsers: { json: ->(_object, _env) { raise NotImplementedError, 'fatal' } } + ) + io = StringIO.new('{}') + expect do + catch(:error) do + subject.call( + Rack::PATH_INFO => '/info', + Rack::REQUEST_METHOD => Rack::POST, + 'CONTENT_TYPE' => 'application/json', + Rack::RACK_INPUT => io, + 'CONTENT_LENGTH' => io.length.to_s + ) + end + end.to raise_error(NotImplementedError, 'fatal') + end + end end diff --git a/spec/grape/middleware/versioner/header_spec.rb b/spec/grape/middleware/versioner/header_spec.rb index 3e46f1133..62d38d8e3 100644 --- a/spec/grape/middleware/versioner/header_spec.rb +++ b/spec/grape/middleware/versioner/header_spec.rb @@ -145,6 +145,17 @@ expect(exception.message).to include('API version not found') end end + + # Requests sending the same Accept header are all answered from one parsed + # media type, so what a request is handed must not be alterable in place. + it 'cannot be altered by one request for the next' do + keys = [Grape::Env::API_TYPE, Grape::Env::API_SUBTYPE, Grape::Env::API_VENDOR, Grape::Env::API_VERSION, Grape::Env::API_FORMAT] + _, _, env = subject.call('HTTP_ACCEPT' => 'application/vnd.vendor-v1+json') + keys.each { |key| expect { env[key] << '-altered' }.to raise_error(FrozenError) } + + _, _, env = subject.call('HTTP_ACCEPT' => 'application/vnd.vendor-v1+json') + expect(env.values_at(*keys)).to eq(%w[application vnd.vendor-v1+json vendor v1 json]) + end end it 'succeeds if :strict is not set' do diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb index 099e9a6f0..cafb62d2f 100644 --- a/spec/grape/validations/params_scope_spec.rb +++ b/spec/grape/validations/params_scope_spec.rb @@ -209,6 +209,41 @@ def initialize(value) end end + # An Array group handed something other than an Array fails its own type + # check, and its members are not then validated against that value as + # though it were one element. + context 'array group given a Hash' do + it 'reports the group as invalid without validating its members' do + subject.params do + requires :items, type: Array do + requires :id, type: Integer + end + end + subject.post('/items') { 'ok' } + + post '/items', { items: { name: 'x' } }.to_json, 'CONTENT_TYPE' => 'application/json' + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('items is invalid') + end + end + + context 'hash group given an Array' do + it 'reports the group as invalid without validating the optional array group inside it' do + subject.params do + requires :meta, type: Hash do + optional :items, type: Array do + requires :id, type: Integer + end + end + end + subject.post('/meta') { 'ok' } + + post '/meta', { meta: [{}] }.to_json, 'CONTENT_TYPE' => 'application/json' + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('meta is invalid') + end + end + context 'coercing values validation with a variant-member-type collection' do it 'accepts values compatible with the declared member types' do expect do @@ -1023,6 +1058,32 @@ def initialize(value) end end + # A with group inside an array scope is a lateral scope whose params are + # the array itself, so each element's index is recorded against the array + # scope. Failing a first element as well as the last shows it was: a stale + # index names the wrong element. + context 'array with a with group' do + before do + subject.params do + requires :array, type: Array do + requires :a, type: Integer + with(type: Integer) do + requires :b + end + end + end + + subject.post '/array_with_group' + end + + it 'names the elements that failed' do + params = { array: [{ a: 1 }, { a: 3, b: 4 }, { a: 5 }] } + post '/array_with_group', params.to_json, 'CONTENT_TYPE' => 'application/json' + expect(last_response.body).to eq('array[0][b] is missing, array[2][b] is missing') + expect(last_response.status).to eq(400) + end + end + context 'nested json array with given' do before do subject.params do diff --git a/spec/grape/validations_spec.rb b/spec/grape/validations_spec.rb index ebe51b742..7d0f30cb7 100644 --- a/spec/grape/validations_spec.rb +++ b/spec/grape/validations_spec.rb @@ -881,6 +881,32 @@ def validate_param!(attr_name, params) expect(last_response.body).to eq('items[0][key] is missing') end + it 'skips the elements that are empty' do + subject.params do + optional :items, type: Array do + requires :key + end + end + subject.post('/optional_group') { 'optional group works' } + + post_with_json '/optional_group', items: [{}, { not_key: 'foo' }] + expect(last_response.status).to eq(400) + expect(last_response.body).to eq('items[1][key] is missing') + end + + it "doesn't validate the group when every element is blank" do + subject.params do + optional :items, type: Array do + requires :key + end + end + subject.post('/optional_group') { 'optional group works' } + + post_with_json '/optional_group', items: [false, ' '] + expect(last_response.status).to eq(201) + expect(last_response.body).to eq('optional group works') + end + it "errors when param is present but isn't an Array" do get '/optional_group', items: 'hello' expect(last_response.status).to eq(400)