Fix array indicies in error messages. (#1463)

.rubocop_todo.yml

@@ -17,7 +17,7 @@ Metrics/BlockNesting:
 # Offense count: 7
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 264
+  Max: 266
 
 # Offense count: 24
 Metrics/CyclomaticComplexity:

CHANGELOG.md

@@ -3,6 +3,7 @@
 
 #### Features
 
+* [#1463](https://github.com/ruby-grape/grape/pull/1463): Fix array indicies in error messages - [@ffloyd](https://github.com/ffloyd).
 * [#1393](https://github.com/ruby-grape/grape/pull/1393): Middleware can be inserted before or after default Grape middleware - [@ridiculous](https://github.com/ridiculous).
 * [#1390](https://github.com/ruby-grape/grape/pull/1390): Allows inserting middleware at arbitrary points in the middleware stack - [@Rosa](https://github.com/Rosa).
 * [#1366](https://github.com/ruby-grape/grape/pull/1366): Stores `message_key` on `Grape::Exceptions::Validation` - [@mkou](https://github.com/mkou).

lib/grape.rb

@@ -58,6 +58,7 @@ module Exceptions
     extend ActiveSupport::Autoload
     autoload :Base
     autoload :Validation
+    autoload :ValidationArrayErrors
     autoload :ValidationErrors
     autoload :MissingVendorOption
     autoload :MissingMimeType

lib/grape/dsl/parameters.rb

@@ -191,7 +191,8 @@ def params(params)
         params = @parent.params(params) if @parent
         if @element
           params = if params.is_a?(Array)
-                     params.flat_map { |el| el[@element] || {} }
+                     # used for calculating parent array indices for error messages
+                     params.map { |el| el[@element] || {} }
                    elsif params.is_a?(Hash)
                      params[@element] || {}
                    else

lib/grape/endpoint.rb

@@ -335,6 +335,8 @@ def run_validators(validators, request)
           validator.validate(request)
         rescue Grape::Exceptions::Validation => e
           validation_errors << e
+        rescue Grape::Exceptions::ValidationArrayErrors => e
+          validation_errors += e.errors
         end
       end
 

lib/grape/exceptions/validation_array_errors.rb

@@ -0,0 +1,11 @@
+module Grape
+  module Exceptions
+    class ValidationArrayErrors < Base
+      attr_reader :errors
+
+      def initialize(errors)
+        @errors = errors
+      end
+    end
+  end
+end

lib/grape/validations/attributes_iterator.rb

@@ -8,16 +8,41 @@ class AttributesIterator
       def initialize(validator, scope, params)
         @scope = scope
         @attrs = validator.attrs
-        @params = Array.wrap(scope.params(params))
+        @original_params = scope.params(params)
+        @params = Array.wrap(@original_params)
       end
 
-      def each
-        @params.each do |resource_params|
-          @attrs.each_with_index do |attr_name, index|
-            if resource_params.is_a?(Hash) && resource_params[attr_name].is_a?(Array)
-              scope.index = index
+      def each(&block)
+        do_each(@params, &block) # because we need recursion for nested arrays
+      end
+
+      private
+
+      def do_each(params_to_process, parent_indicies = [], &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 indicies
+          if resource_params.is_a?(Array)
+            do_each(resource_params, [index] + parent_indicies, &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
+            inside_array = true
+          end
+          if inside_array
+            # fill current and parent scopes with correct array indicies
+            parent_scope = @scope.parent
+            parent_indicies.each do |parent_index|
+              parent_scope.index = parent_index
+              parent_scope = parent_scope.parent
             end
-            yield resource_params, attr_name
+            @scope.index = index
+          end
+
+          @attrs.each do |attr_name|
+            yield resource_params, attr_name, inside_array
           end
         end
       end

lib/grape/validations/params_scope.rb

@@ -2,6 +2,7 @@ module Grape
   module Validations
     class ParamsScope
       attr_accessor :element, :parent, :index
+      attr_reader :type
 
       include Grape::DSL::Parameters
 
@@ -57,7 +58,7 @@ def full_name(name)
         case
         when nested?
           # Find our containing element's name, and append ours.
-          "#{@parent.full_name(@element)}#{parent_index}[#{name}]"
+          "#{@parent.full_name(@element)}#{array_index}[#{name}]"
         when lateral?
           # Find the name of the element as if it was at the
           # same nesting level as our parent.
@@ -68,8 +69,8 @@ def full_name(name)
         end
       end
 
-      def parent_index
-        "[#{@parent.index}]" if @parent.present? && @parent.index.present?
+      def array_index
+        "[#{@index}]" if @index.present?
       end
 
       # @return [Boolean] whether or not this scope is the root-level scope

lib/grape/validations/types/build_coercer.rb

@@ -26,7 +26,15 @@ def self.build_coercer(type, method = nil)
         converter_options = {
           nullify_blank: true
         }
-        conversion_type = type
+        conversion_type = if method == JSON
+                            Object
+                            # because we want just parsed JSON content:
+                            # if type is Array and data is `"{}"`
+                            # result will be [] because Virtus converts hashes
+                            # to arrays
+                          else
+                            type
+                          end
 
         # Use a special coercer for multiply-typed parameters.
         if Types.multiple?(type)

lib/grape/validations/validators/base.rb

@@ -35,11 +35,21 @@ def validate(request)
       # @return [void]
       def validate!(params)
         attributes = AttributesIterator.new(self, @scope, params)
-        attributes.each do |resource_params, attr_name|
-          if @required || (resource_params.respond_to?(:key?) && resource_params.key?(attr_name))
+        array_errors = []
+        attributes.each do |resource_params, attr_name, inside_array|
+          next unless @required || (resource_params.respond_to?(:key?) && resource_params.key?(attr_name))
+
+          begin
             validate_param!(attr_name, resource_params)
+          rescue Grape::Exceptions::Validation => e
+            raise e unless inside_array
+            # we collect errors inside array because
+            # there may be more than one error per field
+            array_errors << e
           end
         end
+
+        raise Grape::Exceptions::ValidationArrayErrors, array_errors if array_errors.any?
       end
 
       def self.convert_to_short_name(klass)

spec/grape/validations/validators/coerce_spec.rb

@@ -400,7 +400,7 @@ class User
 
         get '/ints', ints: '{"i":1,"j":"2"}'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('ints[0][i] is missing, ints[0][i] is invalid, ints[0][j] is missing')
+        expect(last_response.body).to eq('ints is invalid')
 
         get '/ints', ints: '[{"i":"1","j":"2"}]'
         expect(last_response.status).to eq(200)

spec/grape/validations_spec.rb

@@ -245,7 +245,7 @@ def define_requires_none
       it 'errors when param is not an Array' do
         get '/required', items: 'hello'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('items is invalid, items[key] is missing')
+        expect(last_response.body).to eq('items is invalid')
 
         get '/required', items: { key: 'foo' }
         expect(last_response.status).to eq(400)
@@ -337,7 +337,7 @@ def define_requires_none
 
         get '/required', items: [{ key: 'hash in array' }]
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('items is invalid, items[0][key] does not have a valid value')
+        expect(last_response.body).to eq('items is invalid, items[key] does not have a valid value')
       end
 
       it 'works when all params match' do
@@ -466,7 +466,7 @@ def validate_param!(attr_name, params)
           group :children, type: Array do
             requires :name
             group :parents, type: Array do
-              requires :name
+              requires :name, allow_blank: false
             end
           end
         end
@@ -486,13 +486,33 @@ def validate_param!(attr_name, params)
 
       it 'errors when a parameter is not present' do
         get '/within_array', children: [
-          { name: 'Jim', parents: [{}] },
-          { name: 'Job', parents: [{ name: 'Joy' }] }
+          { name: 'Jim', parents: [{ name: 'Joy' }] },
+          { name: 'Job', parents: [{}] }
         ]
         # NOTE: with body parameters in json or XML or similar this
         # should actually fail with: children[parents][name] is missing.
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('children[0][parents] is missing')
+        expect(last_response.body).to eq('children[1][parents] is missing')
+      end
+
+      it 'errors when a parameter is not present in array within array' do
+        get '/within_array', children: [
+          { name: 'Jim', parents: [{ name: 'Joy' }] },
+          { name: 'Job', parents: [{ name: 'Bill' }, { name: '' }] }
+        ]
+
+        expect(last_response.status).to eq(400)
+        expect(last_response.body).to eq('children[1][parents][1][name] is empty')
+      end
+
+      it 'handle errors for all array elements' do
+        get '/within_array', children: [
+          { name: 'Jim', parents: [] },
+          { name: 'Job', parents: [] }
+        ]
+
+        expect(last_response.status).to eq(400)
+        expect(last_response.body).to eq('children[0][parents] is missing, children[1][parents] is missing')
       end
 
       it 'safely handles empty arrays and blank parameters' do
@@ -507,14 +527,13 @@ def validate_param!(attr_name, params)
       end
 
       it 'errors when param is not an Array' do
-        # NOTE: would be nicer if these just returned 'children is invalid'
         get '/within_array', children: 'hello'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('children is invalid, children[name] is missing, children[parents] is missing, children[parents] is invalid, children[parents][name] is missing')
+        expect(last_response.body).to eq('children is invalid')
 
         get '/within_array', children: { name: 'foo' }
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('children is invalid, children[parents] is missing')
+        expect(last_response.body).to eq('children is invalid')
 
         get '/within_array', children: [name: 'Jay', parents: { name: 'Fred' }]
         expect(last_response.status).to eq(400)
@@ -565,7 +584,7 @@ def validate_param!(attr_name, params)
       it 'requires defaults to Array type' do
         get '/req', planets: 'Jupiter, Saturn'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('planets is invalid, planets[name] is missing')
+        expect(last_response.body).to eq('planets is invalid')
 
         get '/req', planets: { name: 'Jupiter' }
         expect(last_response.status).to eq(400)
@@ -581,7 +600,7 @@ def validate_param!(attr_name, params)
       it 'optional defaults to Array type' do
         get '/opt', name: 'Jupiter', moons: 'Europa, Ganymede'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('moons is invalid, moons[name] is missing')
+        expect(last_response.body).to eq('moons is invalid')
 
         get '/opt', name: 'Jupiter', moons: { name: 'Ganymede' }
         expect(last_response.status).to eq(400)
@@ -600,7 +619,7 @@ def validate_param!(attr_name, params)
       it 'group defaults to Array type' do
         get '/grp', stars: 'Sun'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('stars is invalid, stars[name] is missing')
+        expect(last_response.body).to eq('stars is invalid')
 
         get '/grp', stars: { name: 'Sun' }
         expect(last_response.status).to eq(400)
@@ -689,7 +708,7 @@ def validate_param!(attr_name, params)
       it "errors when param is present but isn't an Array" do
         get '/optional_group', items: 'hello'
         expect(last_response.status).to eq(400)
-        expect(last_response.body).to eq('items is invalid, items[key] is missing')
+        expect(last_response.body).to eq('items is invalid')
 
         get '/optional_group', items: { key: 'foo' }
         expect(last_response.status).to eq(400)