Merge pull request #2129 from dwhenry/optional-validation-errors-multiparam

Fix incorrect optional validation issues for MultipleParamsBase

Author: dblock

CHANGELOG.md

@@ -7,6 +7,7 @@
 #### Fixes
 
 * Your contribution here.
+* [#2129](https://github.com/ruby-grape/grape/pull/2129): Fix validation error when Required Array nested inside an optional array, for Multiparam validators - [@dwhenry](https://github.com/dwhenry).
 * [#2128](https://github.com/ruby-grape/grape/pull/2128): Fix validation error when Required Array nested inside an optional array - [@dwhenry](https://github.com/dwhenry).
 * [#2127](https://github.com/ruby-grape/grape/pull/2127): Fix a performance issue with dependent params - [@dnesteryuk](https://github.com/dnesteryuk).
 * [#2126](https://github.com/ruby-grape/grape/pull/2126): Fix warnings about redefined attribute accessors in `AttributeTranslator` - [@samsonjs](https://github.com/samsonjs).

lib/grape/validations/attributes_iterator.rb

@@ -48,6 +48,14 @@ def do_each(params_to_process, parent_indicies = [], &block)
       def yield_attributes(_resource_params, _attrs)
         raise NotImplementedError
       end
+
+      # This is a special case so that we can ignore tree's where option
+      # values are missing lower down. Unfortunately we can remove this
+      # are the parameter parsing stage as they are required to ensure
+      # the correct indexing is maintained
+      def skip?(val)
+        val == Grape::DSL::Parameters::EmptyOptionalValue
+      end
     end
   end
 end

lib/grape/validations/multiple_attributes_iterator.rb

@@ -6,7 +6,7 @@ class MultipleAttributesIterator < AttributesIterator
       private
 
       def yield_attributes(resource_params, _attrs)
-        yield resource_params
+        yield resource_params, skip?(resource_params)
       end
     end
   end

lib/grape/validations/single_attribute_iterator.rb

@@ -11,15 +11,6 @@ def yield_attributes(val, attrs)
         end
       end
 
-
-      # This is a special case so that we can ignore tree's where option
-      # values are missing lower down. Unfortunately we can remove this
-      # are the parameter parsing stage as they are required to ensure
-      # the correct indexing is maintained
-      def skip?(val)
-        val == Grape::DSL::Parameters::EmptyOptionalValue
-      end
-
       # Primitives like Integers and Booleans don't respond to +empty?+.
       # It could be possible to use +blank?+ instead, but
       #

lib/grape/validations/validators/multiple_params_base.rb

@@ -7,7 +7,8 @@ def validate!(params)
         attributes = MultipleAttributesIterator.new(self, @scope, params)
         array_errors = []
 
-        attributes.each do |resource_params|
+        attributes.each do |resource_params, skip_value|
+          next if skip_value
           begin
             validate_params!(resource_params)
           rescue Grape::Exceptions::Validation => e

spec/grape/validations/multiple_attributes_iterator_spec.rb

@@ -13,8 +13,8 @@
         { 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)
+      it 'yields the whole params hash and the skipped flag without the list of attrs' do
+        expect { |b| iterator.each(&b) }.to yield_with_args(params, false)
       end
     end
 
@@ -24,7 +24,17 @@
       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(&b) }.to yield_successive_args([params[0], false], [params[1], false])
+      end
+    end
+
+    context 'when params is empty optional placeholder' do
+      let(:params) do
+        [Grape::DSL::Parameters::EmptyOptionalValue, { 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([Grape::DSL::Parameters::EmptyOptionalValue, true], [params[1], false])
       end
     end
   end

spec/grape/validations_spec.rb

@@ -1023,7 +1023,7 @@ def validate_param!(attr_name, params)
           end
 
           it "with valid data" do
-            data_without_errors = {
+            data = {
               top: [
                 { top_id: 1, middle_1: [
                   {middle_1_id: 11}, {middle_1_id: 12, middle_2: [
@@ -1037,7 +1037,7 @@ def validate_param!(attr_name, params)
               ]
             }
 
-            get '/multi_level', data_without_errors
+            get '/multi_level', data
             expect(last_response.body).to eq("multi_level works!")
             expect(last_response.status).to eq(200)
           end
@@ -1067,6 +1067,90 @@ def validate_param!(attr_name, params)
           end
         end
       end
+
+      it "exactly_one_of" do
+        subject.params do
+          requires :orders, type: Array do
+            requires :id, type: Integer
+            optional :drugs, type: Hash do
+              optional :batch_no, type: String
+              optional :batch_id, type: String
+              exactly_one_of :batch_no, :batch_id
+            end
+          end
+        end
+
+        subject.get '/exactly_one_of' do
+          'exactly_one_of works!'
+        end
+
+        data = {
+          orders: [
+            { id: 77, drugs: {batch_no: "A1234567"}},
+            { id: 70 }
+          ]
+        }
+
+        get '/exactly_one_of', data
+        expect(last_response.body).to eq("exactly_one_of works!")
+        expect(last_response.status).to eq(200)
+      end
+
+      it "at_least_one_of" do
+        subject.params do
+          requires :orders, type: Array do
+            requires :id, type: Integer
+            optional :drugs, type: Hash do
+              optional :batch_no, type: String
+              optional :batch_id, type: String
+              at_least_one_of :batch_no, :batch_id
+            end
+          end
+        end
+
+        subject.get '/at_least_one_of' do
+          'at_least_one_of works!'
+        end
+
+        data = {
+          orders: [
+            { id: 77, drugs: {batch_no: "A1234567"}},
+            { id: 70 }
+          ]
+        }
+
+        get '/at_least_one_of', data
+        expect(last_response.body).to eq("at_least_one_of works!")
+        expect(last_response.status).to eq(200)
+      end
+
+      it "all_or_none_of" do
+        subject.params do
+          requires :orders, type: Array do
+            requires :id, type: Integer
+            optional :drugs, type: Hash do
+              optional :batch_no, type: String
+              optional :batch_id, type: String
+              all_or_none_of :batch_no, :batch_id
+            end
+          end
+        end
+
+        subject.get '/all_or_none_of' do
+          'all_or_none_of works!'
+        end
+
+        data = {
+          orders: [
+            { id: 77, drugs: {batch_no: "A1234567", batch_id: "12"}},
+            { id: 70 }
+          ]
+        }
+
+        get '/all_or_none_of', data
+        expect(last_response.body).to eq("all_or_none_of works!")
+        expect(last_response.status).to eq(200)
+      end
     end
 
     context 'multiple validation errors' do