diff --git a/CHANGELOG.md b/CHANGELOG.md index e2e91ffab..f08679fc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,6 +111,7 @@ * [#2902](https://github.com/ruby-grape/grape/pull/2902): Make `Grape::ErrorFormatter.formatter_for` a lookup that answers `nil` for an unregistered format, like `Grape::Parser.parser_for`, and move the `default_error_formatter` / `Grape::ErrorFormatter::Txt` fallback to `Grape::Middleware::Error`, which owns it; `default_error_formatter` naming nothing registered now raises `Grape::Exceptions::UnknownErrorFormatter` instead of silently storing `Txt` (see UPGRADING) - [@ericproulx](https://github.com/ericproulx). * [#2904](https://github.com/ruby-grape/grape/pull/2904): Document and spec route `requirements` given as a Mustermann capture type, which constrains the match and hands the endpoint the converted value - [@ericproulx](https://github.com/ericproulx). * [#2905](https://github.com/ruby-grape/grape/pull/2905): Nest any `route_param` requirement under the param name, not just a Regexp, and reject the `requirements` shapes that have no param to attach to where they are written rather than on the first request - [@ericproulx](https://github.com/ericproulx). +* [#2908](https://github.com/ruby-grape/grape/pull/2908): Stop reassigning method parameters across `lib`, so a parameter keeps the value its caller passed for the whole method; `Grape::Middleware::Formatter#ensure_content_type` and the `oneof` collection in `Grape::Validations::ParamsScope` no longer write into the Hash they were given - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 3.3.5 (2026-07-30) diff --git a/lib/grape/dsl/desc.rb b/lib/grape/dsl/desc.rb index bf5bcf71f..2d17baf4c 100644 --- a/lib/grape/dsl/desc.rb +++ b/lib/grape/dsl/desc.rb @@ -54,19 +54,22 @@ module Desc # end # def desc(description, **options, &config_block) - if options.key?(:default) - Grape.deprecator.warn('The `default` option of `desc` is deprecated. Use `default_response` instead.') - # Rebuilt rather than mutated in place: an explicit +default_response+ - # still wins, since the merged Hash is the one that keeps its key. - options = { default_response: options[:default] }.merge(options.except(:default)) - end + resolved_options = + if options.key?(:default) + Grape.deprecator.warn('The `default` option of `desc` is deprecated. Use `default_response` instead.') + # Rebuilt rather than mutated in place: an explicit +default_response+ + # still wins, since the merged Hash is the one that keeps its key. + { default_response: options[:default] }.merge(options.except(:default)) + else + options + end settings = if config_block endpoint_config = defined?(configuration) ? configuration : nil Grape::Util::ApiDescription.new(description, endpoint_config, &config_block).settings else - options.merge(description:) + resolved_options.merge(description:) end # Only the route scope is consumed downstream (by +route+ and the # route's readers, e.g. +http_codes+); the namespace scope was diff --git a/lib/grape/dsl/inside_route.rb b/lib/grape/dsl/inside_route.rb index cdcf5bd43..ad4930cf3 100644 --- a/lib/grape/dsl/inside_route.rb +++ b/lib/grape/dsl/inside_route.rb @@ -27,10 +27,10 @@ def configuration # @param backtrace [Array] The backtrace of the exception that caused the error. # @param original_exception [Exception] The original exception that caused the error. def error!(message, status = nil, additional_headers = nil, backtrace = nil, original_exception = nil) - status = self.status(status || inheritable_setting.default_error_status) + resolved_status = self.status(status || inheritable_setting.default_error_status) headers = additional_headers.present? ? header.merge(additional_headers) : header throw :error, Grape::Exceptions::ErrorResponse.new( - message:, status:, headers:, backtrace:, original_exception: + message:, status: resolved_status, headers:, backtrace:, original_exception: ) end diff --git a/lib/grape/dsl/parameters.rb b/lib/grape/dsl/parameters.rb index 86dc62919..fae41e83e 100644 --- a/lib/grape/dsl/parameters.rb +++ b/lib/grape/dsl/parameters.rb @@ -191,9 +191,8 @@ def declared_param?(param) # @return hash of parameters relevant for the current scope # @api private def params(params) - params = @parent.qualifying_params.presence || @parent.params(params) if @parent - params = map_params(params, @element) if @element - params + scoped = @parent ? (@parent.qualifying_params.presence || @parent.params(params)) : params + @element ? map_params(scoped, @element) : scoped end private @@ -211,7 +210,7 @@ def params(params) # +except+ and +:none+ uses it to drop fields entirely. def declare(attrs, opts, required:, using:, except:, as:, &block) merged_opts = @group&.deep_merge(opts) || opts - as ||= merged_opts[:as] + declared_as = as || merged_opts[:as] if using return require_required_and_optional_fields(attrs.first, using:, except:) if required @@ -220,9 +219,9 @@ def declare(attrs, opts, required:, using:, except:, as:, &block) end validates(attrs, merged_opts, required:) - return push_declared_params(attrs, as:) unless block + return push_declared_params(attrs, as: declared_as) unless block - new_scope(attrs.first, type: merged_opts[:type], as:, optional: !required, &block) + new_scope(attrs.first, type: merged_opts[:type], as: declared_as, optional: !required, &block) end def legacy_options?(args) diff --git a/lib/grape/dsl/routing.rb b/lib/grape/dsl/routing.rb index 4e97796f0..51488e1aa 100644 --- a/lib/grape/dsl/routing.rb +++ b/lib/grape/dsl/routing.rb @@ -124,17 +124,18 @@ def do_not_document! end def mount(mounts, opts = {}) + mount_opts = opts if opts[:refresh_already_mounted] Grape.deprecator.warn('`refresh_already_mounted` is not a `mount` option and will be ignored in a future release.') drop_endpoints_mounted_for(mounts) # Dropped before the recursion below re-enters with the same options, # so a Grape API does not warn once per mount and once per instance. - opts = opts.except(:refresh_already_mounted) + mount_opts = opts.except(:refresh_already_mounted) end normalize_mounts(mounts).each_pair do |app, path| if app.respond_to?(:mount_instance) - mount({ app.mount_instance(configuration: opts[:with] || {}) => path }, opts) + mount({ app.mount_instance(configuration: mount_opts[:with] || {}) => path }, mount_opts) next end in_setting = inheritable_setting @@ -260,7 +261,7 @@ def route_param(param, requirements: nil, type: nil, **, &) # capture this namespace does not introduce, and belongs on +namespace+. raise ArgumentError, "route_param :#{param} constrains :#{param}; pass the constraint itself, or a Hash of requirements to the enclosing namespace" if requirements.respond_to?(:to_hash) - param_requirements = { param.to_sym => requirements } unless requirements.nil? + param_requirements = requirements ? { param.to_sym => requirements } : requirements Grape::Validations::ParamsScope.new(api: self) do requires param, type: type diff --git a/lib/grape/error_formatter/base.rb b/lib/grape/error_formatter/base.rb index 643332841..0a692641f 100644 --- a/lib/grape/error_formatter/base.rb +++ b/lib/grape/error_formatter/base.rb @@ -26,12 +26,13 @@ def present(message, env) # Extract it here so the presenter can be resolved and the key is not serialized in the response. # See spec/integration/grape_entity/entity_spec.rb for examples. with = nil + payload = message if message.is_a?(Hash) && message.key?(:with) - message = message.dup - with = message.delete(:with) + payload = message.dup + with = payload.delete(:with) end - presenter = with || env[Grape::Env::API_ENDPOINT].entity_class_for_obj(message) + presenter = with || env[Grape::Env::API_ENDPOINT].entity_class_for_obj(payload) unless presenter || env[Grape::Env::GRAPE_ROUTING_ARGS].nil? # env['api.endpoint'].route does not work when the error occurs within a middleware @@ -44,11 +45,11 @@ def present(message, env) presenter = found_code[2] if found_code end - return message unless presenter + return payload unless presenter embeds = { env: } embeds[:version] = env[Grape::Env::API_VERSION] if env.key?(Grape::Env::API_VERSION) - presenter.represent(message, embeds).serializable_hash + presenter.represent(payload, embeds).serializable_hash end def wrap_message(message) diff --git a/lib/grape/exceptions/validation.rb b/lib/grape/exceptions/validation.rb index 765b4f165..539b99ec0 100644 --- a/lib/grape/exceptions/validation.rb +++ b/lib/grape/exceptions/validation.rb @@ -9,15 +9,16 @@ class Validation < Base def initialize(params:, message: nil, status: nil, headers: nil) @params = Array(params) - if message - @message_key = case message - when Symbol then message - when Hash then message[:key] - end - message = translate_message(message) - end + translated = + if message + @message_key = case message + when Symbol then message + when Hash then message[:key] + end + translate_message(message) + end - super(status:, message:, headers:) + super(status:, message: translated, headers:) # Pre-seed the backtrace so Ruby's raise skips capture. Validation errors are # a hot path (raised per bad attribute) and end up as 400 Bad Request responses; # backtraces here point into Grape internals and have no diagnostic value. diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb index a0d138700..0b43730c0 100644 --- a/lib/grape/middleware/error.rb +++ b/lib/grape/middleware/error.rb @@ -27,9 +27,14 @@ def initialize( # keyword defaults above, so restore them here rather than letting nil # propagate to `def_delegator :rescue_options, :backtrace` or to the # formatter lookup in `#format_message`. - rescue_options ||= Grape::DSL::RescueOptions.new - default_error_formatter ||= Grape::ErrorFormatter::Txt - super + super( + rescue_options: rescue_options || Grape::DSL::RescueOptions.new, + default_error_formatter: default_error_formatter || Grape::ErrorFormatter::Txt, + all_rescue_handler:, base_only_rescue_handlers:, content_types:, + default_message:, default_status:, error_formatters:, format:, + grape_exceptions_rescue_handler:, internal_grape_exceptions_rescue_handler:, + rescue_all:, rescue_grape_exceptions:, rescue_handlers: + ) end end @@ -62,8 +67,8 @@ def call!(env) private def rack_response(status, headers, message) - message = Rack::Utils.escape_html(message) if html_content_type?(headers[Rack::CONTENT_TYPE]) - Rack::Response.new(Array.wrap(message), Rack::Utils.status_code(status), Grape::Util::Header.new.merge(headers)) + 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)) end # Escaping must key off the media type only, case-insensitively. Comparing @@ -261,9 +266,9 @@ def rescue_handler_for_any_class(klass) end def run_rescue_handler(handler, error, endpoint, redispatched: false) - handler = endpoint.public_method(handler) if handler.is_a?(Symbol) + callable = handler.is_a?(Symbol) ? endpoint.public_method(handler) : handler response = catch(:error) do - handler.arity.zero? ? endpoint.instance_exec(&handler) : endpoint.instance_exec(error, &handler) + callable.arity.zero? ? endpoint.instance_exec(&callable) : endpoint.instance_exec(error, &callable) rescue StandardError => e return redispatch(e, endpoint, redispatched) end diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index 80ad76f06..64c985a8e 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -56,25 +56,25 @@ def after private def build_formatted_response(status, headers, bodies) - headers = ensure_content_type(headers) + typed_headers = ensure_content_type(headers) if bodies.is_a?(Grape::ServeStream::StreamResponse) - Grape::ServeStream::SendfileResponse.new([], status, headers) do |resp| + Grape::ServeStream::SendfileResponse.new([], status, typed_headers) do |resp| resp.body = bodies.stream end else # Allow content-type to be explicitly overwritten - formatter = fetch_formatter(headers) + formatter = fetch_formatter(typed_headers) bodymap = instrument_format_response(formatter) do bodies.map { |body| formatter.call(body, env) } end - # A bare Rack tuple rather than a Rack::Response: +headers+ is already + # A bare Rack tuple rather than a Rack::Response: +typed_headers+ is already # a Grape::Util::Header (a Rack::Headers on Rack 3), so wrapping only # re-normalizes the same keys into a second Headers hash that # +Middleware::Base#call+ unwraps again with +to_a+ on the way out. # The 204/304 bodies Rack::Response#finish would blank are returned # above, before this point. - [status, headers, bodymap] + [status, typed_headers, bodymap] end rescue Grape::Exceptions::InvalidFormatter => e throw :error, Grape::Exceptions::ErrorResponse.new(status: 500, message: e.message, backtrace: e.backtrace, original_exception: e) @@ -101,8 +101,10 @@ def fetch_formatter(headers) def ensure_content_type(headers) return headers if headers[Rack::CONTENT_TYPE] - headers[Rack::CONTENT_TYPE] = content_type_for(env[Grape::Env::API_FORMAT]) - headers + # Merged rather than written in place: +headers+ belongs to the response + # the app returned, and negotiating a content type for it is not a reason + # to reach back into it. + headers.merge(Rack::CONTENT_TYPE => content_type_for(env[Grape::Env::API_FORMAT])) end def read_body_input @@ -140,12 +142,12 @@ def read_rack_input(body) return env[Grape::Env::API_REQUEST_BODY] = body unless parser begin - body = (env[Grape::Env::API_REQUEST_BODY] = parser.call(body, env)) - if body.is_a?(Hash) + parsed = (env[Grape::Env::API_REQUEST_BODY] = parser.call(body, env)) + if parsed.is_a?(Hash) if (form_hash = env[Rack::RACK_REQUEST_FORM_HASH]) - form_hash.merge!(body) + form_hash.merge!(parsed) else - env[Rack::RACK_REQUEST_FORM_HASH] = body + env[Rack::RACK_REQUEST_FORM_HASH] = parsed end env[Rack::RACK_REQUEST_FORM_INPUT] = env[Rack::RACK_INPUT] end diff --git a/lib/grape/middleware/stack.rb b/lib/grape/middleware/stack.rb index da9e6e7de..ce643d225 100644 --- a/lib/grape/middleware/stack.rb +++ b/lib/grape/middleware/stack.rb @@ -64,15 +64,15 @@ def initialize end def insert(index, klass, *args, &block) - index = assert_index(index, :before) - middlewares.insert(index, self.class::Middleware.new(klass, args, block)) + at = assert_index(index, :before) + middlewares.insert(at, self.class::Middleware.new(klass, args, block)) end alias insert_before insert def insert_after(index, ...) - index = assert_index(index, :after) - insert(index + 1, ...) + at = assert_index(index, :after) + insert(at + 1, ...) end def use(klass, *args, &block) diff --git a/lib/grape/util/path_normalizer.rb b/lib/grape/util/path_normalizer.rb index c2a85c576..cea0dfc90 100644 --- a/lib/grape/util/path_normalizer.rb +++ b/lib/grape/util/path_normalizer.rb @@ -19,17 +19,18 @@ def self.call(path) # same predicate, and the scan stays in C without building a match. return path if path.start_with?('/') && !(path.end_with?('/') || path.include?('%') || path.include?('//')) - # Slow path + # Slow path. The bangs below are safe because +normalized+ is a fresh + # String built here, never the one the caller passed in. encoding = path.encoding - path = "/#{path}" - path.squeeze!('/') + normalized = "/#{path}" + normalized.squeeze!('/') - unless path == '/' - path.delete_suffix!('/') - path.gsub!(/(%[a-f0-9]{2})/) { ::Regexp.last_match(1).upcase } + unless normalized == '/' + normalized.delete_suffix!('/') + normalized.gsub!(/(%[a-f0-9]{2})/) { ::Regexp.last_match(1).upcase } end - path.force_encoding(encoding) + normalized.force_encoding(encoding) end end end diff --git a/lib/grape/validations/contract_scope.rb b/lib/grape/validations/contract_scope.rb index e8f59e5f5..35baba44b 100644 --- a/lib/grape/validations/contract_scope.rb +++ b/lib/grape/validations/contract_scope.rb @@ -9,19 +9,20 @@ class ContractScope # @yield a block yielding a new schema class. Optional. def initialize(api, contract = nil, &block) # When block is passed, the first arg is either schema or nil. - contract = Dry::Schema.Params(parent: contract, &block) if block + declared = block ? Dry::Schema.Params(parent: contract, &block) : contract - if contract.respond_to?(:schema) + if declared.respond_to?(:schema) # It's a Dry::Validation::Contract, then. - contract = contract.new - key_map = contract.schema.key_map + schema = declared.new + key_map = schema.schema.key_map else # Dry::Schema::Processor, hopefully. - key_map = contract.key_map + schema = declared + key_map = declared.key_map end api.inheritable_setting.add_contract_key_map(key_map) - api.inheritable_setting.add_validation(Validators::ContractScopeValidator.new(schema: contract)) + api.inheritable_setting.add_validation(Validators::ContractScopeValidator.new(schema:)) end end end diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index d856e4376..f76af4a3b 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -364,10 +364,9 @@ def validates(attrs, validations, required: false) Grape.deprecator.warn('Passing a `presence` option is deprecated and it will be ignored in a future release. Declare the parameter with `requires` to make it required, `optional` to make it optional.') end - validations = validations.merge(presence: { value: true, message: validations[:message] }) if required - - process_oneof!(validations) if validations.key?(:oneof) - spec = ValidationsSpec.from(validations) + declared = required ? validations.merge(presence: { value: true, message: validations[:message] }) : validations + declared = declared.merge(oneof: collected_oneof(declared)) if declared.key?(:oneof) + spec = ValidationsSpec.from(declared) document_params(attrs, spec) @@ -419,14 +418,17 @@ def validate_coerce(spec, attrs) # {OneofCollector} so the full params DSL is available inside variants # and the resulting validators are kept out of the real API's # registration list. - def process_oneof!(validations) + # Returns the collected variants rather than writing them back into + # +validations+, which is the options Hash the +requires+/+optional+ call + # site built. + def collected_oneof(validations) raise ArgumentError, 'oneof: requires type: Hash' unless validations[:type] == Hash variants = validations[:oneof] raise ArgumentError, 'oneof: must be a non-empty Array of blocks' unless variants.is_a?(Array) && variants.any? raise ArgumentError, 'oneof: each variant must be a Proc' unless variants.all?(Proc) - validations[:oneof] = variants.map { |block| OneofCollector.collect(block) } + variants.map { |block| OneofCollector.collect(block) } end def validate(type, options, attrs, required, opts) diff --git a/lib/grape/validations/types.rb b/lib/grape/validations/types.rb index 134983b68..9d18cf738 100644 --- a/lib/grape/validations/types.rb +++ b/lib/grape/validations/types.rb @@ -162,20 +162,20 @@ def build_coercer(type, method: nil, strict: false) def create_coercer_instance(type, method, strict) # map_special doesn't recurse into collections — applied only to the top-level type here. - type = map_special(type) + mapped = map_special(type) # Multiply-typed parameters, e.g. types: [Integer, String]. - return MultipleTypeCoercer.new(type, method) if multiple?(type) + return MultipleTypeCoercer.new(mapped, method) if multiple?(mapped) # User-supplied coercion method, or a custom type with its own #parse. - return CustomTypeCoercer.new(type, method) if method || custom?(type) + return CustomTypeCoercer.new(mapped, method) if method || custom?(mapped) # Array/Set of a custom type — CustomTypeCoercer already handles single # custom types when an explicit coercion method is supplied. - return CustomTypeCollectionCoercer.new(map_special(type.first), set: type.is_a?(Set)) if collection_of_custom?(type) + return CustomTypeCollectionCoercer.new(map_special(mapped.first), set: mapped.is_a?(Set)) if collection_of_custom?(mapped) # Fallback: let dry-types handle primitives, structures, and known specials. - DryTypeCoercer.coercer_instance_for(type, strict:) + DryTypeCoercer.coercer_instance_for(mapped, strict:) end class CoercerCache < Grape::Util::Cache diff --git a/lib/grape/validations/types/multiple_type_coercer.rb b/lib/grape/validations/types/multiple_type_coercer.rb index 08e5893e8..8c939773b 100644 --- a/lib/grape/validations/types/multiple_type_coercer.rb +++ b/lib/grape/validations/types/multiple_type_coercer.rb @@ -41,12 +41,12 @@ def initialize(types, method = nil) # of {InvalidValue} if the value could not be coerced. def call(val) # once the value is coerced by the custom method, its type should be checked - val = @method.call(val) if @method + candidate = @method ? @method.call(val) : val coerced_val = InvalidValue.new @type_coercers.each do |coercer| - coerced_val = coercer.call(val) + coerced_val = coercer.call(candidate) return coerced_val unless coerced_val.is_a?(InvalidValue) end diff --git a/lib/grape/validations/types/variant_collection_coercer.rb b/lib/grape/validations/types/variant_collection_coercer.rb index 6ef3c3b3a..0de85a0cf 100644 --- a/lib/grape/validations/types/variant_collection_coercer.rb +++ b/lib/grape/validations/types/variant_collection_coercer.rb @@ -48,15 +48,15 @@ def to_s def call(value) return unless value.is_a? Array - value = + coerced = if @method @method.call(value) else value.map { |v| @member_coercer.call(v) } end - return Set.new value if @types.is_a? Set + return Set.new coerced if @types.is_a? Set - value + coerced end end end diff --git a/lib/grape/validations/validations_spec.rb b/lib/grape/validations/validations_spec.rb index 0a020645f..feca03160 100644 --- a/lib/grape/validations/validations_spec.rb +++ b/lib/grape/validations/validations_spec.rb @@ -97,13 +97,13 @@ def check_incompatible_option_values(default, values, except_values) def validate_value_coercion(coerce_type, *values_list) return unless coerce_type - coerce_type = coerce_type.first if coerce_type.is_a?(Enumerable) + element_type = coerce_type.is_a?(Enumerable) ? coerce_type.first : coerce_type values_list.each do |values| next if !values || values.is_a?(Proc) value_types = values.is_a?(Range) ? [values.begin, values.end].compact : values - value_types = value_types.map { |type| Grape::API::Boolean.build(type) } if coerce_type == Grape::API::Boolean - raise Grape::Exceptions::IncompatibleOptionValues.new(:type, coerce_type, :values, values) unless value_types.all?(coerce_type) + value_types = value_types.map { |type| Grape::API::Boolean.build(type) } if element_type == Grape::API::Boolean + raise Grape::Exceptions::IncompatibleOptionValues.new(:type, element_type, :values, values) unless value_types.all?(element_type) end end