Merge pull request #1913 from bikolya/fix-errors-for-multiple-params-…

…validations

Fix error messages for nested multiple params validations

CHANGELOG.md

@@ -19,6 +19,7 @@
 * [#1898](https://github.com/ruby-grape/grape/pull/1898): Refactor `ValidatorFactory` to improve memory allocation - [@Bhacaz](https://github.com/Bhacaz).
 * [#1900](https://github.com/ruby-grape/grape/pull/1900): Define boolean for `Grape::Api::Instance` - [@Bhacaz](https://github.com/Bhacaz).
 * [#1903](https://github.com/ruby-grape/grape/pull/1903): Allow nested params renaming (Hash/Array) - [@bikolya](https://github.com/bikolya).
+* [#1913](https://github.com/ruby-grape/grape/pull/1913): Fix multiple params validators to return correct messages for nested params - [@bikolya](https://github.com/bikolya).
 
 ### 1.2.4 (2019/06/13)
 

lib/grape.rb

@@ -225,6 +225,8 @@ module ServeFile
 
 require 'grape/validations/validators/base'
 require 'grape/validations/attributes_iterator'
+require 'grape/validations/single_attribute_iterator'
+require 'grape/validations/multiple_attributes_iterator'
 require 'grape/validations/validators/allow_blank'
 require 'grape/validations/validators/as'
 require 'grape/validations/validators/at_least_one_of'

lib/grape/validations/attributes_iterator.rb

@@ -41,11 +41,13 @@ def do_each(params_to_process, parent_indicies = [], &block)
             @scope.index = index
           end
 
-          @attrs.each do |attr_name|
-            yield resource_params, attr_name, inside_array
-          end
+          yield_attributes(resource_params, @attrs, &block)
         end
       end
+
+      def yield_attributes(_resource_params, _attrs)
+        raise NotImplementedError
+      end
     end
   end
 end

lib/grape/validations/multiple_attributes_iterator.rb

@@ -0,0 +1,11 @@
+module Grape
+  module Validations
+    class MultipleAttributesIterator < AttributesIterator
+      private
+
+      def yield_attributes(resource_params, _attrs)
+        yield resource_params
+      end
+    end
+  end
+end

lib/grape/validations/single_attribute_iterator.rb

@@ -0,0 +1,13 @@
+module Grape
+  module Validations
+    class SingleAttributeIterator < AttributesIterator
+      private
+
+      def yield_attributes(resource_params, attrs)
+        attrs.each do |attr_name|
+          yield resource_params, attr_name
+        end
+      end
+    end
+  end
+end

lib/grape/validations/validators/all_or_none.rb

@@ -1,19 +1,12 @@
+require 'grape/validations/validators/multiple_params_base'
+
 module Grape
   module Validations
-    require 'grape/validations/validators/multiple_params_base'
     class AllOrNoneOfValidator < MultipleParamsBase
-      def validate!(params)
-        super
-        if scope_requires_params && only_subset_present
-          raise Grape::Exceptions::Validation, params: all_keys, message: message(:all_or_none)
-        end
-        params
-      end
-
-      private
-
-      def only_subset_present
-        scoped_params.any? { |resource_params| !keys_in_common(resource_params).empty? && keys_in_common(resource_params).length < attrs.length }
+      def validate_params!(params)
+        keys = keys_in_common(params)
+        return if keys.empty? || keys.length == all_keys.length
+        raise Grape::Exceptions::Validation, params: all_keys, message: message(:all_or_none)
       end
     end
   end

lib/grape/validations/validators/at_least_one_of.rb

@@ -3,20 +3,9 @@
 module Grape
   module Validations
     class AtLeastOneOfValidator < MultipleParamsBase
-      def validate!(params)
-        super
-        if scope_requires_params && no_exclusive_params_are_present
-          scoped_params = all_keys.map { |key| @scope.full_name(key) }
-          raise Grape::Exceptions::Validation, params: scoped_params,
-                                               message: message(:at_least_one)
-        end
-        params
-      end
-
-      private
-
-      def no_exclusive_params_are_present
-        scoped_params.any? { |resource_params| keys_in_common(resource_params).empty? }
+      def validate_params!(params)
+        return unless keys_in_common(params).empty?
+        raise Grape::Exceptions::Validation, params: all_keys, message: message(:at_least_one)
       end
     end
   end

lib/grape/validations/validators/base.rb

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

lib/grape/validations/validators/default.rb

@@ -18,7 +18,7 @@ def validate_param!(attr_name, params)
       end
 
       def validate!(params)
-        attrs = AttributesIterator.new(self, @scope, params)
+        attrs = SingleAttributeIterator.new(self, @scope, params)
         attrs.each do |resource_params, attr_name|
           if resource_params.is_a?(Hash) && resource_params[attr_name].nil?
             validate_param!(attr_name, resource_params)

lib/grape/validations/validators/exactly_one_of.rb

@@ -1,28 +1,11 @@
+require 'grape/validations/validators/multiple_params_base'
+
 module Grape
   module Validations
-    require 'grape/validations/validators/mutual_exclusion'
-    class ExactlyOneOfValidator < MutualExclusionValidator
-      def validate!(params)
-        super
-        if scope_requires_params && none_of_restricted_params_is_present
-          raise Grape::Exceptions::Validation, params: all_keys, message: message(:exactly_one)
-        end
-        params
-      end
-
-      def message(default_key = nil)
-        options = instance_variable_get(:@option)
-        if options_key?(:message)
-          (options_key?(default_key, options[:message]) ? options[:message][default_key] : options[:message])
-        else
-          default_key
-        end
-      end
-
-      private
-
-      def none_of_restricted_params_is_present
-        scoped_params.any? { |resource_params| keys_in_common(resource_params).empty? }
+    class ExactlyOneOfValidator < MultipleParamsBase
+      def validate_params!(params)
+        return if keys_in_common(params).length == 1
+        raise Grape::Exceptions::Validation, params: all_keys, message: message(:exactly_one)
       end
     end
   end

lib/grape/validations/validators/multiple_params_base.rb

@@ -1,26 +1,30 @@
 module Grape
   module Validations
     class MultipleParamsBase < Base
-      attr_reader :scoped_params
-
       def validate!(params)
-        @scoped_params = [@scope.params(params)].flatten
-        params
-      end
+        attributes = MultipleAttributesIterator.new(self, @scope, params)
+        array_errors = []
 
-      private
+        attributes.each do |resource_params|
+          begin
+            validate_params!(resource_params)
+          rescue Grape::Exceptions::Validation => e
+            array_errors << e
+          end
+        end
 
-      def scope_requires_params
-        @scope.required? || scoped_params.any? { |param| param.respond_to?(:any?) && param.any? }
+        raise Grape::Exceptions::ValidationArrayErrors, array_errors if array_errors.any?
       end
 
+      private
+
       def keys_in_common(resource_params)
         return [] unless resource_params.is_a?(Hash)
-        (all_keys & resource_params.stringify_keys.keys).map(&:to_s)
+        all_keys & resource_params.keys.map! { |attr| @scope.full_name(attr) }
       end
 
       def all_keys
-        attrs.map(&:to_s)
+        attrs.map { |attr| @scope.full_name(attr) }
       end
     end
   end

lib/grape/validations/validators/mutual_exclusion.rb

@@ -1,24 +1,12 @@
+require 'grape/validations/validators/multiple_params_base'
+
 module Grape
   module Validations
-    require 'grape/validations/validators/multiple_params_base'
     class MutualExclusionValidator < MultipleParamsBase
-      attr_reader :processing_keys_in_common
-
-      def validate!(params)
-        super
-        if two_or_more_exclusive_params_are_present
-          raise Grape::Exceptions::Validation, params: processing_keys_in_common, message: message(:mutual_exclusion)
-        end
-        params
-      end
-
-      private
-
-      def two_or_more_exclusive_params_are_present
-        scoped_params.any? do |resource_params|
-          @processing_keys_in_common = keys_in_common(resource_params)
-          @processing_keys_in_common.length > 1
-        end
+      def validate_params!(params)
+        keys = keys_in_common(params)
+        return if keys.length <= 1
+        raise Grape::Exceptions::Validation, params: keys, message: message(:mutual_exclusion)
       end
     end
   end

spec/grape/exceptions/validation_errors_spec.rb

@@ -77,10 +77,12 @@ def app
       end
       get '/exactly_one_of', beer: 'string', wine: 'anotherstring'
       expect(last_response.status).to eq(400)
-      expect(JSON.parse(last_response.body)).to eq([
-                                                     'params' => %w[beer wine],
-                                                     'messages' => ['are mutually exclusive']
-                                                   ])
+      expect(JSON.parse(last_response.body)).to eq(
+        [
+          'params' => %w[beer wine juice],
+          'messages' => ['are missing, exactly one parameter must be provided']
+        ]
+      )
     end
   end
 end

spec/grape/validations/multiple_attributes_iterator_spec.rb

@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+describe Grape::Validations::MultipleAttributesIterator do
+  describe '#each' do
+    subject(:iterator) { described_class.new(validator, scope, params) }
+    let(:scope) { Grape::Validations::ParamsScope.new(api: Class.new(Grape::API)) }
+    let(:validator) { double(attrs: %i[first second third]) }
+
+    context 'when params is a hash' do
+      let(:params) do
+        { first: 'string', second: 'string' }
+      end
+
+      it 'yields the whole params hash without the list of attrs' do
+        expect { |b| iterator.each(&b) }.to yield_with_args(params)
+      end
+    end
+
+    context 'when params is an array' do
+      let(:params) do
+        [{ first: 'string1', second: 'string1' }, { first: 'string2', second: 'string2' }]
+      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])
+      end
+    end
+  end
+end

spec/grape/validations/single_attribute_iterator_spec.rb

@@ -0,0 +1,33 @@
+require 'spec_helper'
+
+describe Grape::Validations::SingleAttributeIterator do
+  describe '#each' do
+    subject(:iterator) { described_class.new(validator, scope, params) }
+    let(:scope) { Grape::Validations::ParamsScope.new(api: Class.new(Grape::API)) }
+    let(:validator) { double(attrs: %i[first second third]) }
+
+    context 'when params is a hash' do
+      let(:params) do
+        { first: 'string', second: 'string' }
+      end
+
+      it 'yields params and every single attribute from the list' do
+        expect { |b| iterator.each(&b) }
+          .to yield_successive_args([params, :first], [params, :second], [params, :third])
+      end
+    end
+
+    context 'when params is an array' do
+      let(:params) do
+        [{ first: 'string1', second: 'string1' }, { first: 'string2', second: 'string2' }]
+      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(
+          [params[0], :first], [params[0], :second], [params[0], :third],
+          [params[1], :first], [params[1], :second], [params[1], :third]
+        )
+      end
+    end
+  end
+end