Merge pull request #2623 from ruby-grape/fix_coercer_cache

Refactor Coercer Caching to Use Grape::Util::Cache

Author: dblock

CHANGELOG.md

@@ -24,6 +24,7 @@
 * [#2612](https://github.com/ruby-grape/grape/pull/2612): Avoid multiple mount pollution - [@alexanderadam](https://github.com/alexanderadam).
 * [#2617](https://github.com/ruby-grape/grape/pull/2617): Migrate from `ActiveSupport::Configurable` to `Dry::Configurable` - [@ericproulx](https://github.com/ericproulx).
 * [#2618](https://github.com/ruby-grape/grape/pull/2618): Modernize argument delegation for Ruby 3+ compatibility - [@ericproulx](https://github.com/ericproulx).
+* [#2623](https://github.com/ruby-grape/grape/pull/2623): Refactor coercer caching to use `Grape::Util::Cache` - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 #### Fixes

lib/grape/dry_types.rb

@@ -2,9 +2,53 @@
 
 module Grape
   module DryTypes
-    # Call +Dry.Types()+ to add all registered types to +DryTypes+ which is
-    # a container in this case. Check documentation for more information
-    # https://dry-rb.org/gems/dry-types/1.2/getting-started/
-    include Dry.Types()
+    # https://dry-rb.org/gems/dry-types/main/getting-started/
+    # limit to what Grape is using
+    include Dry.Types(:params, :coercible, :strict)
+
+    class StrictCache < Grape::Util::Cache
+      MAPPING = {
+        Grape::API::Boolean => DryTypes::Strict::Bool,
+        BigDecimal => DryTypes::Strict::Decimal,
+        Numeric => DryTypes::Strict::Integer | DryTypes::Strict::Float | DryTypes::Strict::Decimal,
+        TrueClass => DryTypes::Strict::Bool.constrained(eql: true),
+        FalseClass => DryTypes::Strict::Bool.constrained(eql: false)
+      }.freeze
+
+      def initialize
+        super
+        @cache = Hash.new do |h, strict_type|
+          h[strict_type] = MAPPING.fetch(strict_type) do
+            DryTypes.wrapped_dry_types_const_get(DryTypes::Strict, strict_type)
+          end
+        end
+      end
+    end
+
+    class ParamsCache < Grape::Util::Cache
+      MAPPING = {
+        Grape::API::Boolean => DryTypes::Params::Bool,
+        BigDecimal => DryTypes::Params::Decimal,
+        Numeric => DryTypes::Params::Integer | DryTypes::Params::Float | DryTypes::Params::Decimal,
+        TrueClass => DryTypes::Params::Bool.constrained(eql: true),
+        FalseClass => DryTypes::Params::Bool.constrained(eql: false),
+        String => DryTypes::Coercible::String
+      }.freeze
+
+      def initialize
+        super
+        @cache = Hash.new do |h, params_type|
+          h[params_type] = MAPPING.fetch(params_type) do
+            DryTypes.wrapped_dry_types_const_get(DryTypes::Params, params_type)
+          end
+        end
+      end
+    end
+
+    def self.wrapped_dry_types_const_get(dry_type, type)
+      dry_type.const_get(type.name, false)
+    rescue NameError
+      raise ArgumentError, "type #{type} should support coercion via `[]`" unless type.respond_to?(:[])
+    end
   end
 end

lib/grape/validations/types.rb

@@ -155,9 +155,10 @@ def map_special(type)
       # @return [Object] object to be used
       #   for coercion and type validation
       def build_coercer(type, method: nil, strict: false)
-        cache_instance(type, method, strict) do
-          create_coercer_instance(type, method, strict)
-        end
+        # no cache since unique
+        return create_coercer_instance(type, method, strict) if method.respond_to?(:call)
+
+        CoercerCache[[type, method, strict]]
       end
 
       def create_coercer_instance(type, method, strict)
@@ -184,30 +185,14 @@ def create_coercer_instance(type, method, strict)
         end
       end
 
-      def cache_instance(type, method, strict, &_block)
-        key = cache_key(type, method, strict)
-
-        return @__cache[key] if @__cache.key?(key)
-
-        instance = yield
-
-        @__cache_write_lock.synchronize do
-          @__cache[key] = instance
+      class CoercerCache < Grape::Util::Cache
+        def initialize
+          super
+          @cache = Hash.new do |h, (type, method, strict)|
+            h[[type, method, strict]] = Grape::Validations::Types.create_coercer_instance(type, method, strict)
+          end
         end
-
-        instance
       end
-
-      def cache_key(type, method, strict)
-        [type, method, strict].each_with_object(+'_') do |val, memo|
-          next if val.nil?
-
-          memo << '_' << val.to_s
-        end
-      end
-
-      instance_variable_set(:@__cache,            {})
-      instance_variable_set(:@__cache_write_lock, Mutex.new)
     end
   end
 end

lib/grape/validations/types/array_coercer.rb

@@ -7,15 +7,14 @@ module Types
       # an array of arrays of integers.
       #
       # It could've been possible to use an +of+
-      # method (https://dry-rb.org/gems/dry-types/1.2/array-with-member/)
+      # method (https://dry-rb.org/gems/dry-types/main/array-with-member/)
       # provided by dry-types. Unfortunately, it doesn't work for Grape because of
       # behavior of Virtus which was used earlier, a `Grape::Validations::Types::PrimitiveCoercer`
       # maintains Virtus behavior in coercing.
       class ArrayCoercer < DryTypeCoercer
         def initialize(type, strict = false)
           super
-
-          @coercer = scope::Array
+          @coercer = strict ? DryTypes::Strict::Array : DryTypes::Params::Array
           @subtype = type.first
         end
 

lib/grape/validations/types/dry_type_coercer.rb

@@ -1,19 +1,12 @@
 # frozen_string_literal: true
 
-module DryTypes
-  # Call +Dry.Types()+ to add all registered types to +DryTypes+ which is
-  # a container in this case. Check documentation for more information
-  # https://dry-rb.org/gems/dry-types/1.2/getting-started/
-  include Dry.Types()
-end
-
 module Grape
   module Validations
     module Types
       # A base class for classes which must identify a coercer to be used.
       # If the +strict+ argument is true, it won't coerce the given value
       # but check its type. More information there
-      # https://dry-rb.org/gems/dry-types/1.2/built-in-types/
+      # https://dry-rb.org/gems/dry-types/main/built-in-types/
       class DryTypeCoercer
         class << self
           # Returns a collection coercer which corresponds to a given type.
@@ -42,7 +35,7 @@ def coercer_instance_for(type, strict = false)
         def initialize(type, strict = false)
           @type = type
           @strict = strict
-          @scope = strict ? DryTypes::Strict : DryTypes::Params
+          @cache_coercer = strict ? DryTypes::StrictCache : DryTypes::ParamsCache
         end
 
         # Coerces the given value to a type which was specified during
@@ -53,13 +46,13 @@ def call(val)
           return if val.nil?
 
           @coercer[val]
-        rescue Dry::Types::CoercionError => _e
+        rescue Dry::Types::CoercionError
           InvalidValue.new
         end
 
         protected
 
-        attr_reader :scope, :type, :strict
+        attr_reader :type, :strict, :cache_coercer
       end
     end
   end

lib/grape/validations/types/primitive_coercer.rb

@@ -7,37 +7,10 @@ module Types
       # initialization. When +strict+ is true, it doesn't coerce a value but check
       # that it has the proper type.
       class PrimitiveCoercer < DryTypeCoercer
-        MAPPING = {
-          Grape::API::Boolean => DryTypes::Params::Bool,
-          BigDecimal => DryTypes::Params::Decimal,
-          Numeric => DryTypes::Params::Integer | DryTypes::Params::Float | DryTypes::Params::Decimal,
-          TrueClass => DryTypes::Params::Bool.constrained(eql: true),
-          FalseClass => DryTypes::Params::Bool.constrained(eql: false),
-
-          # unfortunately, a +Params+ scope doesn't contain String
-          String => DryTypes::Coercible::String
-        }.freeze
-
-        STRICT_MAPPING = {
-          Grape::API::Boolean => DryTypes::Strict::Bool,
-          BigDecimal => DryTypes::Strict::Decimal,
-          Numeric => DryTypes::Strict::Integer | DryTypes::Strict::Float | DryTypes::Strict::Decimal,
-          TrueClass => DryTypes::Strict::Bool.constrained(eql: true),
-          FalseClass => DryTypes::Strict::Bool.constrained(eql: false)
-        }.freeze
-
         def initialize(type, strict = false)
           super
 
-          @type = type
-
-          @coercer = (strict ? STRICT_MAPPING : MAPPING).fetch(type) do
-            scope.const_get(type.name, false)
-          rescue NameError
-            raise ArgumentError, "type #{type} should support coercion via `[]`" unless type.respond_to?(:[])
-
-            type
-          end
+          @coercer = cache_coercer[type]
         end
 
         def call(val)

spec/grape/validations/types/primitive_coercer_spec.rb

@@ -114,7 +114,7 @@
       let(:type) { Complex }
 
       it 'raises error on init' do
-        expect(DryTypes::Params.constants).not_to include(type.name.to_sym)
+        expect(Grape::DryTypes::Params.constants).not_to include(type.name.to_sym)
         expect { subject }.to raise_error(/type Complex should support coercion/)
       end
     end

spec/grape/validations/types_spec.rb

@@ -92,21 +92,10 @@ def self.parse; end
   end
 
   describe '::build_coercer' do
-    it 'has internal cache variables' do
-      expect(described_class.instance_variable_get(:@__cache)).to be_a(Hash)
-      expect(described_class.instance_variable_get(:@__cache_write_lock)).to be_a(Mutex)
-    end
-
     it 'caches the result of the build_coercer method' do
-      original_cache = described_class.instance_variable_get(:@__cache)
-      described_class.instance_variable_set(:@__cache, {})
-
       a_coercer = described_class.build_coercer(Array[String])
       b_coercer = described_class.build_coercer(Array[String])
-
       expect(a_coercer.object_id).to eq(b_coercer.object_id)
-
-      described_class.instance_variable_set(:@__cache, original_cache)
     end
   end
 end