From 840b2cf0c393d06acc1ad69853dedfa9afbe8939 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Thu, 10 Sep 2026 21:52:25 +0200 Subject: [PATCH] Settle the attributes iterator's per-scope state once per pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For every element of an array scope, and for every validator on it, `AttributesIterator#do_each` re-derived things that depend only on the scope and the request: whether the scope iterates elements, whether its params are an Array, which scope an element's index is recorded against, and the request's `ParamScopeTracker`, a fiber-storage read. They are now settled once per `#each` and handed down, and the top-level call no longer allocates its empty `parent_indices`. An element-iterating scope whose params are not an Array returns before iterating, since it yielded and recorded nothing anyway. On a ten-element array, one validator's iteration overhead went from 5.46 to 4.64 µs. The branch where a lateral scope, such as a `with` group inside an array, records each element's index against its nearest array ancestor had no spec: removing it passed the suite while naming the wrong element in errors. It now has one, which passes before and after this change. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + lib/grape/validations/attributes_iterator.rb | 50 +++++++++++--------- spec/grape/validations/params_scope_spec.rb | 26 ++++++++++ 3 files changed, 55 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fdc7d3a3e..152b0642a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * [#2918](https://github.com/ruby-grape/grape/pull/2918): Skip the dry-types round trip when a value already is the declared type - [@ericproulx](https://github.com/ericproulx). * [#2917](https://github.com/ruby-grape/grape/pull/2917): Read path captures out of the router's union match instead of re-running the route's pattern - [@ericproulx](https://github.com/ericproulx). * [#2921](https://github.com/ruby-grape/grape/pull/2921): Pin the router's request-time isolation regressions through requests instead of its instance variables - [@ericproulx](https://github.com/ericproulx). +* [#2928](https://github.com/ruby-grape/grape/pull/2928): Settle the attributes iterator's per-scope state once per pass instead of once per array element - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 4.0.0 (2026-09-07) diff --git a/lib/grape/validations/attributes_iterator.rb b/lib/grape/validations/attributes_iterator.rb index 2af39e7ec..32ee8c60f 100644 --- a/lib/grape/validations/attributes_iterator.rb +++ b/lib/grape/validations/attributes_iterator.rb @@ -19,20 +19,42 @@ def initialize(attrs, scope) def each(params, &) original_params = @scope.params(params) + iterates_elements = @scope.iterates_elements? # A scope resolves to a Hash unless the declaration nests arrays, and # then #do_each has nothing to do but hand it straight back: Array.wrap # boxes it, the loop unboxes it on its only iteration, and with no Array # anywhere neither the nesting descent nor the index bookkeeping # applies. Every validator on a flat +params+ block comes through here. - return yield_attributes(original_params, &) if original_params.is_a?(Hash) && !@scope.iterates_elements? + return yield_attributes(original_params, &) if original_params.is_a?(Hash) && !iterates_elements + + array_params = original_params.is_a?(Array) + # Do not validate the content of an array scope that did not get one. + return if iterates_elements && !array_params + + # Where each element's index is recorded depends on the scope and on + # whether its params are an Array, never on the element, so it is + # settled once here rather than per element. A lateral scope (no + # @element) whose params resolved to an array hands its index to the + # nearest element-iterating ancestor, so full_name still produces the + # right bracketed index. + index_scope = iterates_elements ? @scope : (@scope.nearest_array_ancestor if array_params) + # No tracker means we're outside a ParamScopeTracker.track block (e.g. + # a unit test that invokes a validator directly). Index tracking is + # skipped — full_name will produce bracket-less names — but validation + # continues rather than crashing. + tracker = ParamScopeTracker.current if index_scope # because we need recursion for nested arrays - do_each(Array.wrap(original_params), original_params, &) + do_each(Array.wrap(original_params), tracker, index_scope, NO_PARENT_INDICES, &) end private - def do_each(params_to_process, original_params, parent_indices = [], &block) + # The top-level call's parent indices; only ever read. + NO_PARENT_INDICES = [].freeze + private_constant :NO_PARENT_INDICES + + def do_each(params_to_process, tracker, index_scope, 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 @@ -42,32 +64,16 @@ def do_each(params_to_process, original_params, parent_indices = [], &block) # validators see a non-hash and fail it the same way any other # unexpected element type does. if resource_params.is_a?(Array) && parent_indices.size < @max_nesting - do_each(resource_params, original_params, [index] + parent_indices, &block) + do_each(resource_params, tracker, index_scope, [index] + parent_indices, &block) next end - if @scope.iterates_elements? - 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) - # Lateral scope (no @element) whose params resolved to an array — - # delegate index tracking to the nearest element-iterating ancestor - # so that full_name produces the correct bracketed index. - target = @scope.nearest_array_ancestor - store_indices(target, index, parent_indices) if target - end - + store_indices(tracker, index_scope, index, parent_indices) if tracker yield_attributes(resource_params, &block) end end - def store_indices(target_scope, index, parent_indices) - # No tracker means we're outside a ParamScopeTracker.track block (e.g. - # a unit test that invokes a validator directly). Index tracking is - # skipped — full_name will produce bracket-less names — but validation - # continues rather than crashing. - tracker = ParamScopeTracker.current or return + def store_indices(tracker, target_scope, index, parent_indices) parent_scope = target_scope.parent parent_indices.each do |parent_index| break unless parent_scope diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb index 099e9a6f0..596feca42 100644 --- a/spec/grape/validations/params_scope_spec.rb +++ b/spec/grape/validations/params_scope_spec.rb @@ -1023,6 +1023,32 @@ def initialize(value) end end + # A with group inside an array scope is a lateral scope whose params are + # the array itself, so each element's index is recorded against the array + # scope. Failing a first element as well as the last shows it was: a stale + # index names the wrong element. + context 'array with a with group' do + before do + subject.params do + requires :array, type: Array do + requires :a, type: Integer + with(type: Integer) do + requires :b + end + end + end + + subject.post '/array_with_group' + end + + it 'names the elements that failed' do + params = { array: [{ a: 1 }, { a: 3, b: 4 }, { a: 5 }] } + post '/array_with_group', params.to_json, 'CONTENT_TYPE' => 'application/json' + expect(last_response.body).to eq('array[0][b] is missing, array[2][b] is missing') + expect(last_response.status).to eq(400) + end + end + context 'nested json array with given' do before do subject.params do