From 9186a50901b87ed037dad0f6302c592799ceee03 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Fri, 18 Sep 2026 11:52:02 +0200 Subject: [PATCH] Reject what a collection with multiple member types cannot coerce A param declared as `type: Array[Integer, String]` -- or with such a collection among its `types:`, as in the README's `types: [Integer, String, Array[Integer, String]]` -- is coerced by VariantCollectionCoercer, which checked neither its input nor its output: - a value that is not an Array returned nil rather than an InvalidValue, so CoerceValidator wrote nil over it and `requires :ids, type: Array[Integer, String]` let `ids=x` through as nil; - each member went through the member coercer and the results were returned as they came, so a member none of the types accepts reached the endpoint as a Grape::Validations::Types::InvalidValue object. Array[Integer] answers 400 to both, and now so does a collection with multiple member types. nil and '' still coerce to nil, as specs pin. The coercion method, when one is given, was never called for anything but an Array: a `coerce_with` that splits a String into the collection saw nothing and the param came out nil. It is now handed the value whatever it is, as MultipleTypeCoercer hands it for `types:`. UPGRADING covers both, since each turns a request that used to pass into a 400. Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 1 + UPGRADING.md | 8 ++++- .../types/variant_collection_coercer.rb | 34 +++++++++++++------ .../types/variant_collection_coercer_spec.rb | 20 +++++++++-- .../validators/coerce_validator_spec.rb | 16 +++++++++ 5 files changed, 66 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 252d3e6a8..1f7853adf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ * [#2956](https://github.com/ruby-grape/grape/pull/2956): Keep every mount of an API mounted more than once when a later declaration refreshes the mounts - [@ericproulx](https://github.com/ericproulx). * [#2958](https://github.com/ruby-grape/grape/pull/2958): Answer a `HEAD` request for a path no route matches without a body - [@ericproulx](https://github.com/ericproulx). * [#2953](https://github.com/ruby-grape/grape/pull/2953): Answer an `error!` String message with its own status instead of a `500` when the route names a `failure` entity for that status - [@ericproulx](https://github.com/ericproulx). +* [#2955](https://github.com/ruby-grape/grape/pull/2955): Reject a value that is not an Array, and a member of none of the types, in a collection with multiple member types instead of passing them on as `nil` and `InvalidValue` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 4.0.1 (2026-09-15) diff --git a/UPGRADING.md b/UPGRADING.md index 1ec5d7c7a..b387e2ef4 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -3,6 +3,12 @@ Upgrading Grape ### Upgrading to >= 4.1.0 +#### A collection with multiple member types rejects what it cannot coerce + +A param declared as a collection whose members may be of several types — `type: Array[Integer, String]`, or such a collection listed in `types:` — is now validated the way a collection of one type is ([#2955](https://github.com/ruby-grape/grape/pull/2955)). A value that is not an Array, and an Array holding a member none of the types accepts, answer `400` with `is invalid`. The first used to reach the endpoint as `nil`, even under `requires`, and the second with an internal `Grape::Validations::Types::InvalidValue` object where the member was. A `nil` value or an empty String is still `nil`. + +A `coerce_with` method given to such a param is now handed every value, as it is for `types:`, instead of only an Array. A method that splits a String into the collection works now; one that only ever expected an Array may raise on a String, which answers `400`. + #### A route's failure entity no longer presents a String error message An entity named for a status in a route's `failure` (or `http_codes`) is now applied to a structured `error!` message only — a Hash or an object — and a String message renders as it does without one ([#2953](https://github.com/ruby-grape/grape/pull/2953)). An entity exposing attributes cannot read any off a String, so `error!('Unauthorized', 401)` under `desc failure: [[401, 'Unauthorized', API::Error]]` raised while rendering and was answered with a `500`; it now answers `401` with the message. An entity written to take the String itself is no longer handed one; give it a Hash to expose from instead: @@ -20,7 +26,6 @@ class API::Error < Grape::Entity end error!({ message: 'Unauthorized' }, 401) ``` - #### `lint!` checks every response once, from the API that is served `lint!` and `Grape.config.lint` used to put `Rack::Lint` in each endpoint's stack, which missed two kinds of response: the 404 the router answers for a path nothing matches, and those of a Rack app mounted with `mount`, which the router calls directly. A single `Rack::Lint` around the router now checks every response the API gives ([#2960](https://github.com/ruby-grape/grape/pull/2960)). Two things follow from that. @@ -35,6 +40,7 @@ MyAPI.call({}) # After MyAPI.call(Rack::MockRequest.env_for('/')) +``` #### 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: diff --git a/lib/grape/validations/types/variant_collection_coercer.rb b/lib/grape/validations/types/variant_collection_coercer.rb index 0de85a0cf..9791b1318 100644 --- a/lib/grape/validations/types/variant_collection_coercer.rb +++ b/lib/grape/validations/types/variant_collection_coercer.rb @@ -41,23 +41,37 @@ def to_s # Coerce the given value. # + # A value that is not an Array, and an Array holding a member none of + # the types accepts, are invalid, as they are for a collection of one + # type. A coercion method is handed the value whatever it is, as it is + # for +types:+, so it can build the collection out of a String. + # # @param value [Array] collection of values to be coerced - # @return [Array,Set,InvalidValue] - # the coerced result, or an instance - # of {InvalidValue} if the value could not be coerced. + # @return [Array,Set,InvalidValue,nil] + # the coerced result, nil when the value is nil or an empty String, + # or an instance of {InvalidValue} if the value could not be coerced. def call(value) - return unless value.is_a? Array + return if value.nil? || (value.is_a?(String) && value.empty?) - coerced = - if @method - @method.call(value) - else - value.map { |v| @member_coercer.call(v) } - end + coerced = @method ? @method.call(value) : coerce_members(value) + return coerced if coerced.is_a?(InvalidValue) return Set.new coerced if @types.is_a? Set coerced end + + private + + def coerce_members(value) + return InvalidValue.new unless value.is_a?(Array) + + value.map do |member| + coerced = @member_coercer.call(member) + return coerced if coerced.is_a?(InvalidValue) + + coerced + end + end end end end diff --git a/spec/grape/validations/types/variant_collection_coercer_spec.rb b/spec/grape/validations/types/variant_collection_coercer_spec.rb index 506a6eae3..bc28aeeca 100644 --- a/spec/grape/validations/types/variant_collection_coercer_spec.rb +++ b/spec/grape/validations/types/variant_collection_coercer_spec.rb @@ -14,9 +14,14 @@ end describe '#call' do - it 'returns nil for a non-Array value' do + it 'returns nil for a nil or empty value' do coercer = described_class.new([Integer, String]) - expect(coercer.call('not an array')).to be_nil + expect([coercer.call(nil), coercer.call('')]).to eq([nil, nil]) + end + + it 'returns an InvalidValue for a non-Array value' do + coercer = described_class.new([Integer, String]) + expect(coercer.call('not an array')).to be_a(Grape::Validations::Types::InvalidValue) end it 'coerces each member via the member coercer when no method is given' do @@ -24,12 +29,23 @@ expect(coercer.call(%w[1 abc])).to eq([1, 'abc']) end + it 'returns an InvalidValue when a member is none of the types' do + coercer = described_class.new([Integer, String]) + expect(coercer.call([1, {}])).to be_a(Grape::Validations::Types::InvalidValue) + end + it 'coerces the whole collection via the given method' do method = ->(value) { value.map(&:upcase) } coercer = described_class.new([String], method) expect(coercer.call(%w[a b])).to eq(%w[A B]) end + it 'hands the given method a value that is not an Array' do + method = ->(value) { value.split(',') } + coercer = described_class.new([Integer, String], method) + expect(coercer.call('1,a')).to eq(%w[1 a]) + end + it 'returns a Set when the declared types are a Set' do coercer = described_class.new(Set[Integer, String]) expect(coercer.call(%w[1 abc])).to eq(Set[1, 'abc']) diff --git a/spec/grape/validations/validators/coerce_validator_spec.rb b/spec/grape/validations/validators/coerce_validator_spec.rb index 263e491f4..125501f84 100644 --- a/spec/grape/validations/validators/coerce_validator_spec.rb +++ b/spec/grape/validations/validators/coerce_validator_spec.rb @@ -1173,6 +1173,22 @@ def self.parse(_val) expect(last_response).to be_successful expect(last_response.body).to eq([1, 'two'].to_set.to_s) end + + it 'rejects a collection with multiple types given something other than a collection' do + get '/', c: 'three' + expect(last_response).to be_bad_request + expect(last_response.body).to eq('c is invalid') + end + + it 'rejects a collection with multiple types holding a member of none of them' do + get '/', c: [1, { two: 2 }] + expect(last_response).to be_bad_request + expect(last_response.body).to eq('c is invalid') + + get '/', d: [{ one: 1 }] + expect(last_response).to be_bad_request + expect(last_response.body).to eq('d is invalid') + end end context 'custom coercion rules' do