Merge pull request #886 from jrichter1/required_params_type

Make the type in group of required params required (#722)

Author: dblock

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)

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)`

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

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)
 

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

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

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'
 

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

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

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