From a55438bcab7a6ec834a04a79e0ada4437a3376a3 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Sat, 16 May 2026 12:49:28 +0200 Subject: [PATCH] Reuse one AttributesIterator per validator; drop Enumerable AttributesIterator's attrs/scope are static per validator; only params varies per request. Build the iterator once in Base#initialize (frozen) via an overridable iterator_class, and thread params through #each instead of storing request-derived state in @original_params/@params. The reused instance is shared across threads, so it must hold no mutable per-request state. - Remove `include Enumerable` (dead + misleading: #each never returned an Enumerator and nothing used map/select/etc.) and the unused `attr_reader :scope`. - MultipleParamsBase#validate! now lazily builds array_errors (`nil` -> `||= []`) like Base, avoiding an empty-array allocation on the no-error path. Allocation: one fewer object (~80 B) per validator per request; speed-neutral. Full validation/endpoint suites green. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 1 + lib/grape/validations/attributes_iterator.rb | 26 +++++++++---------- lib/grape/validations/validators/base.rb | 10 +++++-- .../validators/default_validator.rb | 3 +-- .../validators/multiple_params_base.rb | 13 ++++++---- .../multiple_attributes_iterator_spec.rb | 8 +++--- .../single_attribute_iterator_spec.rb | 10 +++---- 7 files changed, 40 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61e3e6eca..60deb3a31 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). +* [#2726](https://github.com/ruby-grape/grape/pull/2726): Reuse one `AttributesIterator` per validator and drop the unused `Enumerable` mixin - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/lib/grape/validations/attributes_iterator.rb b/lib/grape/validations/attributes_iterator.rb index 59b3790c3..578ca3fab 100644 --- a/lib/grape/validations/attributes_iterator.rb +++ b/lib/grape/validations/attributes_iterator.rb @@ -3,37 +3,37 @@ module Grape module Validations class AttributesIterator - include Enumerable - - attr_reader :scope - - def initialize(attrs, scope, params) + # +attrs+ and +scope+ are static per validator; only +params+ varies + # per request, so an instance can be built once and reused (it keeps + # no request-derived state). Reused instances are shared across + # threads, so +each+ must stay free of mutable instance state. + def initialize(attrs, scope) @attrs = attrs @scope = scope - @original_params = scope.params(params) - @params = Array.wrap(@original_params) end - def each(&) - do_each(@params, &) # because we need recursion for nested arrays + def each(params, &) + original_params = @scope.params(params) + # because we need recursion for nested arrays + do_each(Array.wrap(original_params), original_params, &) end private - def do_each(params_to_process, parent_indices = [], &block) + def do_each(params_to_process, original_params, 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 if resource_params.is_a?(Array) - do_each(resource_params, [index] + parent_indices, &block) + do_each(resource_params, original_params, [index] + parent_indices, &block) next end if @scope.type == Array - next unless @original_params.is_a?(Array) # do not validate content of array if it isn't array + 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) + elsif original_params.is_a?(Array) # Lateral scope (no @element) whose params resolved to an array — # delegate index tracking to the nearest array-typed ancestor so # that full_name produces the correct bracketed index. diff --git a/lib/grape/validations/validators/base.rb b/lib/grape/validations/validators/base.rb index 9d82f575c..71da7f66a 100644 --- a/lib/grape/validations/validators/base.rb +++ b/lib/grape/validations/validators/base.rb @@ -61,6 +61,7 @@ def initialize(attrs, options, required, scope, opts) @scope = scope @fail_fast, @allow_blank = opts.values_at(:fail_fast, :allow_blank) @exception_message = message(self.class.default_message_key) if self.class.default_message_key + @iterator = iterator_class.new(@attrs, @scope).freeze end # Validates a given request. @@ -85,12 +86,11 @@ def fail_fast? # @raise [Grape::Exceptions::Validation] if validation failed # @return [void] def validate!(params) - attributes = SingleAttributeIterator.new(@attrs, @scope, params) # we collect errors inside array because # there may be more than one error per field array_errors = nil - attributes.each do |val, attr_name, empty_val| + @iterator.each(params) do |val, attr_name, empty_val| next if !@scope.required? && empty_val next unless @scope.meets_dependency?(val, params) @@ -115,6 +115,12 @@ def validate_param!(attr_name, params) private + # The AttributesIterator subclass used to walk this validator's + # attributes. Built once in #initialize and reused across requests. + def iterator_class + SingleAttributeIterator + end + def validation_error!(attr_name_or_params, message = @exception_message) params = attr_name_or_params.is_a?(Array) ? attr_name_or_params : @scope.full_name(attr_name_or_params) raise Grape::Exceptions::Validation.new(params:, message:) diff --git a/lib/grape/validations/validators/default_validator.rb b/lib/grape/validations/validators/default_validator.rb index 5b57b499d..72ab1edce 100644 --- a/lib/grape/validations/validators/default_validator.rb +++ b/lib/grape/validations/validators/default_validator.rb @@ -18,8 +18,7 @@ def initialize(attrs, options, required, scope, opts) end def validate!(params) - attrs = SingleAttributeIterator.new(@attrs, @scope, params) - attrs.each do |resource_params, attr_name| + @iterator.each(params) do |resource_params, attr_name| next unless @scope.meets_dependency?(resource_params, params) resource_params[attr_name] = @default_call.call(resource_params) if hash_like?(resource_params) && resource_params[attr_name].nil? diff --git a/lib/grape/validations/validators/multiple_params_base.rb b/lib/grape/validations/validators/multiple_params_base.rb index 1ba846982..c61efa038 100644 --- a/lib/grape/validations/validators/multiple_params_base.rb +++ b/lib/grape/validations/validators/multiple_params_base.rb @@ -5,20 +5,23 @@ module Validations module Validators class MultipleParamsBase < Base def validate!(params) - attributes = MultipleAttributesIterator.new(@attrs, @scope, params) - array_errors = [] + array_errors = nil - attributes.each do |resource_params| + @iterator.each(params) do |resource_params| validate_params!(resource_params) rescue Grape::Exceptions::Validation => e - array_errors << e + (array_errors ||= []) << e end - raise Grape::Exceptions::ValidationArrayErrors.new(array_errors) if array_errors.any? + raise Grape::Exceptions::ValidationArrayErrors.new(array_errors) if array_errors end private + def iterator_class + MultipleAttributesIterator + end + def keys_in_common(resource_params, known_keys = all_keys) return [] unless hash_like?(resource_params) diff --git a/spec/grape/validations/multiple_attributes_iterator_spec.rb b/spec/grape/validations/multiple_attributes_iterator_spec.rb index 14988b981..cd1ed0eb6 100644 --- a/spec/grape/validations/multiple_attributes_iterator_spec.rb +++ b/spec/grape/validations/multiple_attributes_iterator_spec.rb @@ -2,7 +2,7 @@ describe Grape::Validations::MultipleAttributesIterator do describe '#each' do - subject(:iterator) { described_class.new(validator, scope, params) } + subject(:iterator) { described_class.new(validator, scope) } let(:scope) { Grape::Validations::ParamsScope.new(api: Class.new(Grape::API)) } let(:validator) { double(attrs: %i[first second third]) } @@ -13,7 +13,7 @@ end it 'yields the whole params hash without the list of attrs' do - expect { |b| iterator.each(&b) }.to yield_with_args(params) + expect { |b| iterator.each(params, &b) }.to yield_with_args(params) end end @@ -23,7 +23,7 @@ end it 'yields each element of the array without the list of attrs' do - expect { |b| iterator.each(&b) }.to yield_successive_args(params[0], params[1]) + expect { |b| iterator.each(params, &b) }.to yield_successive_args(params[0], params[1]) end end @@ -31,7 +31,7 @@ let(:params) { [Grape::DSL::Parameters::EmptyOptionalValue] } it 'does not yield it' do - expect { |b| iterator.each(&b) }.to yield_successive_args + expect { |b| iterator.each(params, &b) }.to yield_successive_args end end end diff --git a/spec/grape/validations/single_attribute_iterator_spec.rb b/spec/grape/validations/single_attribute_iterator_spec.rb index a0e5a708c..dc762d01d 100644 --- a/spec/grape/validations/single_attribute_iterator_spec.rb +++ b/spec/grape/validations/single_attribute_iterator_spec.rb @@ -2,7 +2,7 @@ describe Grape::Validations::SingleAttributeIterator do describe '#each' do - subject(:iterator) { described_class.new(%i[first second], scope, params) } + subject(:iterator) { described_class.new(%i[first second], scope) } let(:scope) { Grape::Validations::ParamsScope.new(api: Class.new(Grape::API)) } @@ -12,7 +12,7 @@ end it 'yields params and every single attribute from the list' do - expect { |b| iterator.each(&b) } + expect { |b| iterator.each(params, &b) } .to yield_successive_args([params, :first, false], [params, :second, false]) end end @@ -23,7 +23,7 @@ end it 'yields every single attribute from the list for each of the array elements' do - expect { |b| iterator.each(&b) }.to yield_successive_args( + expect { |b| iterator.each(params, &b) }.to yield_successive_args( [params[0], :first, false], [params[0], :second, false], [params[1], :first, false], [params[1], :second, false] ) @@ -33,7 +33,7 @@ let(:params) { [{}, '', 10] } it 'marks params with empty values' do - expect { |b| iterator.each(&b) }.to yield_successive_args( + expect { |b| iterator.each(params, &b) }.to yield_successive_args( [params[0], :first, true], [params[0], :second, true], [params[1], :first, true], [params[1], :second, true], [params[2], :first, false], [params[2], :second, false] @@ -45,7 +45,7 @@ let(:params) { [Grape::DSL::Parameters::EmptyOptionalValue, 10] } it 'does not yield skipped values' do - expect { |b| iterator.each(&b) }.to yield_successive_args( + expect { |b| iterator.each(params, &b) }.to yield_successive_args( [params[1], :first, false], [params[1], :second, false] ) end