Fix Array/Set custom-type coercer dropping symbolized hash keys

CHANGELOG.md

@@ -26,6 +26,7 @@
 
 * [#2678](https://github.com/ruby-grape/grape/pull/2678): Update rubocop to 1.86.0 and autocorrect offenses - [@ericproulx](https://github.com/ericproulx).
 * [#2682](https://github.com/ruby-grape/grape/pull/2682): Fix `Style/OptionalBooleanParameter` offenses - [@ericproulx](https://github.com/ericproulx).
+* [#2699](https://github.com/ruby-grape/grape/pull/2699): Fix `Grape::Validations::Types::CustomTypeCoercer` dropping symbolized hash keys for `Array`/`Set` types; refactor the class for readability - [@ericproulx](https://github.com/ericproulx).
 * [#2700](https://github.com/ruby-grape/grape/pull/2700): Fix README typos, remove obsolete Ruby 2.4 / Fixnum section, and replace incorrect `requires + values + allow_blank` note with a correct one covering `optional + values` semantics (closes #2631) - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 

lib/grape/validations/types/custom_type_coercer.rb

@@ -33,6 +33,10 @@ module Types
       # contract as +coerced?+, and must be supplied with a coercion
       # +method+.
       class CustomTypeCoercer
+        TYPE_CHECK_METHODS = %i[coerced? parsed?].freeze
+        COLLECTION_TYPES = [Array, Set].freeze
+        private_constant :TYPE_CHECK_METHODS, :COLLECTION_TYPES
+
         # A new coercer for the given type specification
         # and coercion method.
         #
@@ -41,8 +45,7 @@ class CustomTypeCoercer
         # @param method [#parse,#call]
         #   optional coercion method. See class docs.
         def initialize(type, method = nil)
-          coercion_method = infer_coercion_method type, method
-          @method = enforce_symbolized_keys type, coercion_method
+          @method = build_coercion_method(type, method)
           @type_check = infer_type_check(type)
         end
 
@@ -66,90 +69,50 @@ def coerced?(val)
 
         private
 
-        # Determine the coercion method we're expected to use
-        # based on the parameters given.
-        #
-        # @param type see #new
-        # @param method see #new
-        # @return [#call] coercion method
+        def build_coercion_method(type, method)
+          coercion_method = infer_coercion_method(type, method)
+          return hash_symbolizer(coercion_method) if type == Hash
+          return collection_symbolizer(coercion_method) if COLLECTION_TYPES.include?(type)
+
+          coercion_method
+        end
+
         def infer_coercion_method(type, method)
           return type.method(:parse) unless method
           return method unless method.respond_to?(:parse)
 
           method.method(:parse)
         end
 
-        # Determine how the type validity of a coerced
-        # value should be decided.
-        #
-        # @param type see #new
-        # @return [#call] a procedure which accepts a single parameter
-        #   and returns +true+ if the passed object is of the correct type.
+        def hash_symbolizer(method)
+          ->(val) { method.call(val).deep_symbolize_keys }
+        end
+
+        def collection_symbolizer(method)
+          ->(val) { method.call(val).map! { |item| symbolize_if_hash(item) } }
+        end
+
+        def symbolize_if_hash(item)
+          item.is_a?(Hash) ? item.deep_symbolize_keys : item
+        end
+
         def infer_type_check(type)
-          # First check for special class methods
-          if type.respond_to? :coerced?
-            type.method :coerced?
-          elsif type.respond_to? :parsed?
-            type.method :parsed?
-          elsif type.respond_to? :call
-            # Arbitrary proc passed for type validation.
-            # Note that this will fail unless a method is also
-            # passed, or if the type also implements a parse() method.
-            type
-          elsif type.is_a?(Enumerable)
-            lambda do |value|
-              value.is_a?(Enumerable) && value.all? do |val|
-                recursive_type_check(type.first, val)
-              end
-            end
-          else
-            # By default, do a simple type check
-            ->(value) { value.is_a? type }
-          end
+          method_name = TYPE_CHECK_METHODS.detect { |m| type.respond_to?(m) }
+          return type.method(method_name) if method_name
+          return type if type.respond_to?(:call)
+          return enumerable_type_check(type) if type.is_a?(Enumerable)
+
+          ->(value) { value.is_a? type }
         end
 
-        def recursive_type_check(type, value)
-          if type.is_a?(Enumerable) && value.is_a?(Enumerable)
-            value.all? { |val| recursive_type_check(type.first, val) }
-          else
-            !type.is_a?(Enumerable) && value.is_a?(type)
-          end
+        def enumerable_type_check(type)
+          ->(value) { value.is_a?(Enumerable) && value.all? { |val| recursive_type_check(type.first, val) } }
         end
 
-        # Enforce symbolized keys for complex types
-        # by wrapping the coercion method such that
-        # any Hash objects in the immediate heirarchy
-        # have their keys recursively symbolized.
-        # This helps common libs such as JSON to work easily.
-        #
-        # @param type see #new
-        # @param method see #infer_coercion_method
-        # @return [#call] +method+ wrapped in an additional
-        #   key-conversion step, or just returns +method+
-        #   itself if no conversion is deemed to be
-        #   necessary.
-        def enforce_symbolized_keys(type, method)
-          # Collections have all values processed individually
-          if [Array, Set].include?(type)
-            lambda do |val|
-              method.call(val).tap do |new_val|
-                new_val.map do |item|
-                  item.is_a?(Hash) ? item.deep_symbolize_keys : item
-                end
-              end
-            end
-
-          # Hash objects are processed directly
-          elsif type == Hash
-            lambda do |val|
-              method.call(val).deep_symbolize_keys
-            end
-
-          # Simple types are not processed.
-          # This includes Array<primitive> types.
-          else
-            method
-          end
+        def recursive_type_check(type, value)
+          return value.all? { |val| recursive_type_check(type.first, val) } if type.is_a?(Enumerable) && value.is_a?(Enumerable)
+
+          !type.is_a?(Enumerable) && value.is_a?(type)
         end
       end
     end

spec/grape/validations/types/custom_type_coercer_spec.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+describe Grape::Validations::Types::CustomTypeCoercer do
+  describe '#call' do
+    context 'when the type is a collection of hashes' do
+      subject(:coercer) { described_class.new(type, coerce_method) }
+
+      let(:coerce_method) { ->(val) { JSON.parse(val) } }
+
+      context 'with an Array type' do
+        let(:type) { Array }
+
+        it 'symbolizes keys of nested hashes' do
+          expect(coercer.call('[{"foo":"bar"}]')).to eq([{ foo: 'bar' }])
+        end
+      end
+
+      context 'with a Set type' do
+        let(:type) { Set }
+        let(:coerce_method) { ->(val) { Set.new(JSON.parse(val)) } }
+
+        it 'symbolizes keys of nested hashes' do
+          expect(coercer.call('[{"foo":"bar"}]')).to eq(Set[{ foo: 'bar' }])
+        end
+      end
+    end
+  end
+end