From 2e1103af5db8008e668f5e1bd0c937331850ce31 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Tue, 11 Nov 2025 22:29:25 +0100 Subject: [PATCH] Refactor coercer caching to use Grape::Util::Cache - Replace manual cache management with Grape::Util::Cache - Move type mappings to dedicated StrictCache and ParamsCache classes - Simplify build_coercer method by removing manual cache logic - Update dry-types documentation links from 1.2 to main - Refactor PrimitiveCoercer to use centralized cache classes --- CHANGELOG.md | 1 + lib/grape/dry_types.rb | 52 +++++++++++++++++-- lib/grape/validations/types.rb | 35 ++++--------- lib/grape/validations/types/array_coercer.rb | 5 +- .../validations/types/dry_type_coercer.rb | 15 ++---- .../validations/types/primitive_coercer.rb | 29 +---------- .../types/primitive_coercer_spec.rb | 2 +- spec/grape/validations/types_spec.rb | 11 ---- 8 files changed, 67 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fce53e03e..8f6065997 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/grape/dry_types.rb b/lib/grape/dry_types.rb index 5f1bc3cde..6a74e6124 100644 --- a/lib/grape/dry_types.rb +++ b/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 diff --git a/lib/grape/validations/types.rb b/lib/grape/validations/types.rb index 86f9c9b60..efcafd16b 100644 --- a/lib/grape/validations/types.rb +++ b/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 diff --git a/lib/grape/validations/types/array_coercer.rb b/lib/grape/validations/types/array_coercer.rb index ec4ca41de..ecc8c7ea8 100644 --- a/lib/grape/validations/types/array_coercer.rb +++ b/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 diff --git a/lib/grape/validations/types/dry_type_coercer.rb b/lib/grape/validations/types/dry_type_coercer.rb index f9672198e..b361f4636 100644 --- a/lib/grape/validations/types/dry_type_coercer.rb +++ b/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 diff --git a/lib/grape/validations/types/primitive_coercer.rb b/lib/grape/validations/types/primitive_coercer.rb index e59b5c6eb..4375aa518 100644 --- a/lib/grape/validations/types/primitive_coercer.rb +++ b/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) diff --git a/spec/grape/validations/types/primitive_coercer_spec.rb b/spec/grape/validations/types/primitive_coercer_spec.rb index cc0dd1995..0590fbfa9 100644 --- a/spec/grape/validations/types/primitive_coercer_spec.rb +++ b/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 diff --git a/spec/grape/validations/types_spec.rb b/spec/grape/validations/types_spec.rb index 003088a43..a09c8ad2a 100644 --- a/spec/grape/validations/types_spec.rb +++ b/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