Reuse one AttributesIterator per validator; drop Enumerable

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

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.

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:)

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?

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)
 

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,15 +23,15 @@
       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
 
     context 'when params is empty optional placeholder' do
       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

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