Merge pull request #886 from jrichter1/required_params_type

dblock · dblock · commit 89591f25e65b · 2015-01-26T11:44:18.000-05:00
Make the type in group of required params required (#722)
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@ Next Release
 * [#879](https://github.com/intridea/grape/pull/879): The `route_info` value is no longer included in `params` Hash - [@rodzyn](https://github.com/rodzyn).
 * [#881](https://github.com/intridea/grape/issues/881): Fixed `Grape::Validations::ValuesValidator` support for `Range` type - [@ajvondrak](https://github.com/ajvondrak).
 * [#901](https://github.com/intridea/grape/pull/901): Fix: callbacks defined in a version block are only called for the routes defined in that block - [@kushkella](https://github.com/kushkella).
+* [#886](https://github.com/intridea/grape/pull/886): Group of parameters made to require an explicit type of Hash or Array - [@jrichter1](https://github.com/jrichter1).
 * Your contribution here.
 
 0.10.1 (12/28/2014)
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -26,6 +26,14 @@ Callbacks defined in a version block are only called for the routes defined in t
 
 See [#901](https://github.com/intridea/grape/pull/901) for more information.
 
+
+#### Make type of group of parameters required
+
+Groups of parameters now require their type to be set explicitly as Array or Hash.
+Not setting the type now results in MissingGroupTypeError, unsupported type will raise UnsupportedTypeError.
+
+See [#886](https://github.com/intridea/grape/pull/886) for more information.
+
 ### Upgrading to >= 0.10.1
 
 #### Changes to `declared(params, include_missing: false)`
diff --git a/lib/grape.rb b/lib/grape.rb
@@ -55,6 +55,8 @@ module Exceptions
     autoload :UnknownOptions,                 'grape/exceptions/unknown_options'
     autoload :InvalidWithOptionForRepresent,  'grape/exceptions/invalid_with_option_for_represent'
     autoload :IncompatibleOptionValues,       'grape/exceptions/incompatible_option_values'
+    autoload :MissingGroupTypeError,          'grape/exceptions/missing_group_type'
+    autoload :UnsupportedGroupTypeError,      'grape/exceptions/unsupported_group_type'
   end
 
   module ErrorFormatter
diff --git a/lib/grape/dsl/parameters.rb b/lib/grape/dsl/parameters.rb
@@ -38,6 +38,13 @@ def optional(*attrs, &block)
         orig_attrs = attrs.clone
 
         opts = attrs.last.is_a?(Hash) ? attrs.pop : {}
+        type = opts[:type]
+
+        # check type for optional parameter group
+        if attrs && block_given?
+          fail Grape::Exceptions::MissingGroupTypeError.new if type.nil?
+          fail Grape::Exceptions::UnsupportedGroupTypeError.new unless [Array, Hash].include?(type)
+        end
 
         validate_attributes(attrs, opts, &block)
 
diff --git a/lib/grape/exceptions/missing_group_type.rb b/lib/grape/exceptions/missing_group_type.rb
@@ -0,0 +1,10 @@
+# encoding: utf-8
+module Grape
+  module Exceptions
+    class MissingGroupTypeError < Base
+      def initialize
+        super(message: compose_message('missing_group_type'))
+      end
+    end
+  end
+end
diff --git a/lib/grape/exceptions/unsupported_group_type.rb b/lib/grape/exceptions/unsupported_group_type.rb
@@ -0,0 +1,10 @@
+# encoding: utf-8
+module Grape
+  module Exceptions
+    class UnsupportedGroupTypeError < Base
+      def initialize
+        super(message: compose_message('unsupported_group_type'))
+      end
+    end
+  end
+end
diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml
@@ -33,4 +33,6 @@ en:
         at_least_one: 'are missing, at least one parameter must be provided'
         exactly_one: 'are missing, exactly one parameter must be provided'
         all_or_none: 'provide all or none of parameters'
+        missing_group_type: 'group type is required'
+        unsupported_group_type: 'group type must be Array or Hash'
 
diff --git a/lib/grape/validations/params_scope.rb b/lib/grape/validations/params_scope.rb
@@ -71,6 +71,13 @@ def validate_attributes(attrs, opts, &block)
       end
 
       def new_scope(attrs, optional = false, &block)
+        # if required params are grouped and no type or unsupported type is provided, raise an error
+        type = attrs[1] ? attrs[1][:type] : nil
+        if attrs.first && !optional
+          fail Grape::Exceptions::MissingGroupTypeError.new if type.nil?
+          fail Grape::Exceptions::UnsupportedGroupTypeError.new unless [Array, Hash].include?(type)
+        end
+
         opts = attrs[1] || { type: Array }
         ParamsScope.new(api: @api, element: attrs.first, parent: self, optional: optional, type: opts[:type], &block)
       end
diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb
@@ -2166,11 +2166,11 @@ def static
     it 'groups nested params and prevents overwriting of params with same name in different groups' do
       subject.desc 'method'
       subject.params do
-        group :group1 do
+        group :group1, type: Array do
           optional :param1, desc: 'group1 param1 desc'
           requires :param2, desc: 'group1 param2 desc'
         end
-        group :group2 do
+        group :group2, type: Array do
           optional :param1, desc: 'group2 param1 desc'
           requires :param2, desc: 'group2 param2 desc'
         end
@@ -2190,7 +2190,7 @@ def static
       subject.desc 'nesting'
       subject.params do
         requires :root_param, desc: 'root param'
-        group :nested do
+        group :nested, type: Array do
           requires :nested_param, desc: 'nested param'
         end
       end
diff --git a/spec/grape/validations/params_scope_spec.rb b/spec/grape/validations/params_scope_spec.rb
@@ -102,4 +102,80 @@ def app
       end
     end
   end
+
+  context 'parameters in group' do
+    it 'errors when no type is provided' do
+      expect do
+        subject.params do
+          group :a do
+            requires :b
+          end
+        end
+      end.to raise_error Grape::Exceptions::MissingGroupTypeError
+
+      expect do
+        subject.params do
+          optional :a do
+            requires :b
+          end
+        end
+      end.to raise_error Grape::Exceptions::MissingGroupTypeError
+    end
+
+    it 'allows Hash as type' do
+      subject.params do
+        group :a, type: Hash do
+          requires :b
+        end
+      end
+      subject.get('/group') { 'group works' }
+      get '/group', a: { b: true }
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('group works')
+
+      subject.params do
+        optional :a, type: Hash do
+          requires :b
+        end
+      end
+      get '/optional_type_hash'
+    end
+
+    it 'allows Array as type' do
+      subject.params do
+        group :a, type: Array do
+          requires :b
+        end
+      end
+      subject.get('/group') { 'group works' }
+      get '/group', a: [{ b: true }]
+      expect(last_response.status).to eq(200)
+      expect(last_response.body).to eq('group works')
+
+      subject.params do
+        optional :a, type: Array do
+          requires :b
+        end
+      end
+      get '/optional_type_array'
+    end
+
+    it 'errors with an unsupported type' do
+      expect do
+        subject.params do
+          group :a, type: Set do
+            requires :b
+          end
+        end
+      end.to raise_error Grape::Exceptions::UnsupportedGroupTypeError
+
+      expect do
+        subject.params do
+          optional :a, type: Set do
+            requires :b
+          end
+        end
+      end.to raise_error Grape::Exceptions::UnsupportedGroupTypeError
+    end
+  end
 end
diff --git a/spec/grape/validations/validators/values_spec.rb b/spec/grape/validations/validators/values_spec.rb
@@ -71,7 +71,7 @@ class API < Grape::API
         end
 
         params do
-          optional :optional do
+          optional :optional, type: Array do
             requires :type, values: %w(a b)
           end
         end
diff --git a/spec/grape/validations_spec.rb b/spec/grape/validations_spec.rb
@@ -228,7 +228,7 @@ def define_requires_none
 
       it 'adds to declared parameters' do
         subject.params do
-          requires :items do
+          requires :items, type: Array do
             requires :key
           end
         end
@@ -272,7 +272,7 @@ def define_requires_none
 
       it 'adds to declared parameters' do
         subject.params do
-          requires :items do
+          requires :items, type: Array do
             requires :key
           end
         end
@@ -283,7 +283,7 @@ def define_requires_none
     context 'group' do
       before do
         subject.params do
-          group :items do
+          group :items, type: Array do
             requires :key
           end
         end
@@ -306,7 +306,7 @@ def define_requires_none
 
       it 'adds to declared parameters' do
         subject.params do
-          group :items do
+          group :items, type: Array do
             requires :key
           end
         end
@@ -319,7 +319,7 @@ def define_requires_none
 
       before do
         subject.params do
-          optional :items do
+          optional :items, type: Array do
             optional :key1, type: String
             optional :key2, type: String
           end
@@ -395,9 +395,9 @@ def validate_param!(attr_name, params)
     context 'validation within arrays' do
       before do
         subject.params do
-          group :children do
+          group :children, type: Array do
             requires :name
-            group :parents do
+            group :parents, type: Array do
               requires :name
             end
           end
@@ -457,7 +457,7 @@ def validate_param!(attr_name, params)
     context 'with block param' do
       before do
         subject.params do
-          requires :planets do
+          requires :planets, type: Array do
             requires :name
           end
         end
@@ -469,7 +469,7 @@ def validate_param!(attr_name, params)
         end
 
         subject.params do
-          group :stars do
+          group :stars, type: Array do
             requires :name
           end
         end
@@ -482,7 +482,7 @@ def validate_param!(attr_name, params)
 
         subject.params do
           requires :name
-          optional :moons do
+          optional :moons, type: Array do
             requires :name
           end
         end
@@ -549,9 +549,9 @@ def validate_param!(attr_name, params)
     context 'validation within arrays with JSON' do
       before do
         subject.params do
-          group :children do
+          group :children, type: Array do
             requires :name
-            group :parents do
+            group :parents, type: Array do
               requires :name
             end
           end
@@ -630,7 +630,7 @@ def validate_param!(attr_name, params)
 
       it 'adds to declared parameters' do
         subject.params do
-          optional :items do
+          optional :items, type: Array do
             requires :key
           end
         end
@@ -692,10 +692,10 @@ def validate_param!(attr_name, params)
 
       it 'adds to declared parameters' do
         subject.params do
-          optional :items do
+          optional :items, type: Array do
             requires :key
-            optional(:optional_subitems) { requires :value }
-            requires(:required_subitems) { requires :value }
+            optional(:optional_subitems, type: Array) { requires :value }
+            requires(:required_subitems, type: Array) { requires :value }
           end
         end
         expect(subject.route_setting(:declared_params)).to eq([items: [:key, { optional_subitems: [:value] }, { required_subitems: [:value] }]])
