From 0a099e9c346e4c4ce358263e2123f90d18048948 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Fri, 15 May 2026 10:51:05 +0200 Subject: [PATCH] Move declaration-coherence checks into ValidationsSpec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check_incompatible_option_values` and `validate_value_coercion` were private methods on `ParamsScope`, invoked from `#validates` right after building the `ValidationsSpec`. Both are pure cross-field invariants computed entirely from fields the spec already owns (`default`, `values`, `except_values`, `coerce_type`). `ValidationsSpec` already enforced one construction-time invariant (the `:type` + `:types` mutual-exclusion `ArgumentError`), so concentrating the rest there makes "you cannot construct an incoherent spec" true by definition: the invariant no longer depends on a caller remembering to run the checks after `.from(...)`. `ParamsScope#validates` shrinks to build-spec → document → dispatch. - Moved both methods into `ValidationsSpec` as private; a new private `validate!` runs them from `initialize`, before `freeze`. - Removed both from `ParamsScope` and the two call sites in `#validates`. - `check_coerce_with` deliberately stays in `ParamsScope#validate_coerce`: it is gated on the remountable-API skip (`return unless coerce_options[:type]`, because a base instance has no resolved type until the mounted instance replays) and is therefore not a pure construction-time invariant. Spec fixture update: `params_documentation_spec` built a spec with `default: 42, values: [1, 2, 3]` purely to exercise `document_params`; that combination can no longer be constructed, so the fixture now uses a coherent `default: 1`. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 1 + lib/grape/validations/params_scope.rb | 25 -------------- lib/grape/validations/validations_spec.rb | 34 +++++++++++++++++++ .../validations/params_documentation_spec.rb | 4 +-- 4 files changed, 37 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61e3e6eca..7e7e71006 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ * [#2710](https://github.com/ruby-grape/grape/pull/2710): Tidy up `Grape::DeclaredParamsHandler` - [@ericproulx](https://github.com/ericproulx). * [#2714](https://github.com/ruby-grape/grape/pull/2714): Drop unused `Grape::Middleware::Globals` and its `grape.request*` env constants - [@ericproulx](https://github.com/ericproulx). * [#2717](https://github.com/ruby-grape/grape/pull/2717): Convert `Grape::Exceptions::ErrorResponse` to a `Data` value object - [@ericproulx](https://github.com/ericproulx). +* [#2720](https://github.com/ruby-grape/grape/pull/2720): Move declaration-coherence checks into `Grape::Validations::ValidationsSpec` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb index aaeb5f5fd..ae3d8c17e 100644 --- a/lib/grape/validations/params_scope.rb +++ b/lib/grape/validations/params_scope.rb @@ -334,8 +334,6 @@ def validates(attrs, validations) process_oneof!(validations) if validations.key?(:oneof) spec = ValidationsSpec.from(validations) - check_incompatible_option_values(spec.default, spec.values, spec.except_values) - validate_value_coercion(spec.coerce_type, spec.values, spec.except_values) document_params(attrs, spec) # Presence runs first — `required` is forwarded to every subsequent @@ -380,16 +378,6 @@ def validate_coerce(spec, attrs) validate('coerce', coerce_options, attrs, spec.required?, spec.shared_opts) end - def check_incompatible_option_values(default, values, except_values) - return if default.nil? || default.is_a?(Proc) - - raise Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :values, values) if values && !values.is_a?(Proc) && Array(default).any? { |def_val| !values.include?(def_val) } - - return unless except_values && !except_values.is_a?(Proc) && Array(default).any? { |def_val| except_values.include?(def_val) } - - raise Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :except, except_values) - end - # Translate a `oneof: [proc, proc, ...]` declaration into a list of # captured validator arrays — one array per variant. Each variant's # block is evaluated in its own +ParamsScope+ backed by an @@ -418,19 +406,6 @@ def validate(type, options, attrs, required, opts) @api.inheritable_setting.namespace_stackable[:validations] = validator_instance end - def validate_value_coercion(coerce_type, *values_list) - return unless coerce_type - - coerce_type = coerce_type.first if coerce_type.is_a?(Enumerable) - 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) - end - end - def all_element_blank?(scoped_params) scoped_params.respond_to?(:all?) && scoped_params.all?(&:blank?) end diff --git a/lib/grape/validations/validations_spec.rb b/lib/grape/validations/validations_spec.rb index 40f3b549b..95f2eb42e 100644 --- a/lib/grape/validations/validations_spec.rb +++ b/lib/grape/validations/validations_spec.rb @@ -55,6 +55,8 @@ def initialize(raw) @shared_opts = { allow_blank: @allow_blank, fail_fast: @fail_fast }.freeze @validator_entries = build_validator_entries(raw) + validate! + freeze end @@ -68,6 +70,38 @@ def coerce_options private + # Cross-field consistency checks on the parsed declaration. Run at + # construction so an incoherent spec (e.g. a +default+ outside +values+, + # or +values+ whose elements don't match +type+) can never exist — + # callers no longer have to remember to invoke these separately. + def validate! + check_incompatible_option_values(@default, @values, @except_values) + validate_value_coercion(@coerce_type, @values, @except_values) + end + + def check_incompatible_option_values(default, values, except_values) + return if default.nil? || default.is_a?(Proc) + + raise Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :values, values) if values && !values.is_a?(Proc) && Array(default).any? { |def_val| !values.include?(def_val) } + + return unless except_values && !except_values.is_a?(Proc) && Array(default).any? { |def_val| except_values.include?(def_val) } + + raise Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :except, except_values) + end + + def validate_value_coercion(coerce_type, *values_list) + return unless coerce_type + + coerce_type = coerce_type.first if coerce_type.is_a?(Enumerable) + 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) + end + end + def build_validator_entries(raw) raw.reject do |k, _| SPEC_CONSUMED_KEYS.include?(k) || ParamsScope::RESERVED_DOCUMENTATION_KEYWORDS.include?(k) diff --git a/spec/grape/validations/params_documentation_spec.rb b/spec/grape/validations/params_documentation_spec.rb index e6acedb07..b6e1fdcab 100644 --- a/spec/grape/validations/params_documentation_spec.rb +++ b/spec/grape/validations/params_documentation_spec.rb @@ -35,7 +35,7 @@ def spec_for(validations) validations = { presence: true, type: Integer, - default: 42, + default: 1, length: { min: 1, max: 10 }, desc: 'A foo', documentation: { note: 'doc' }, @@ -50,7 +50,7 @@ def spec_for(validations) type: 'Integer', values: [1, 2, 3], except_values: [4, 5, 6], - default: 42, + default: 1, min_length: 1, max_length: 10, desc: 'A foo',