From f01fa43db26f13bfaeb43bc40e5f8d23726b10b6 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:32:25 -0600 Subject: [PATCH 01/10] Fix cache key collision between different finders ArgumentKeys joined the argument values with no separator and dropped the field names, so three different finders shared one cache entry: Collide.cached_find_by_alpha_and_beta("x", "y") # => {alpha: "x", beta: "y"} Collide.cached_find_by_delta_and_gamma("x", "y") # => {alpha: "x", beta: "y"} Collide.cached_find_by_guid("xy") # => {alpha: "x", beta: "y"} The second and third calls returned the first record. ArgumentKeys.for_fields now names each field and escapes the separators inside a value, so the key is one-to-one with the arguments. The separators survive both the remove-characters and the replace-characters option. This changes every cache key. Existing entries become a miss. Co-Authored-By: Claude Opus 5 (1M context) --- lib/active_remote/cached.rb | 38 ++++++++---- lib/active_remote/cached/argument_keys.rb | 32 ++++++++++ .../cached/argument_keys_spec.rb | 60 +++++++++++++++++++ .../cached_delete_methods_spec.rb | 6 +- .../cached_exist_methods_spec.rb | 3 +- .../active_remote/cached_find_methods_spec.rb | 10 ++-- .../cached_search_methods_spec.rb | 10 ++-- spec/active_remote/cached_spec.rb | 4 +- 8 files changed, 135 insertions(+), 28 deletions(-) diff --git a/lib/active_remote/cached.rb b/lib/active_remote/cached.rb index f2ef218..e095ade 100644 --- a/lib/active_remote/cached.rb +++ b/lib/active_remote/cached.rb @@ -198,10 +198,22 @@ def _expanded_search_args(method_arguments) method_arguments.map { |method_argument| ":#{method_argument} => #{method_argument}," }.join end + # Returns the source text that builds the cache key for a generated + # method. The key names each field, so that two finders that take the + # same values do not share one cache entry. + def _expanded_cache_key_args(method_arguments) + sorted_arguments = method_arguments.sort + field_names = sorted_arguments.map { |argument| ":#{argument}" }.join(',') + + '::ActiveRemote::Cached::ArgumentKeys.for_fields(' \ + "[#{field_names}], [#{sorted_arguments.join(',')}], __active_remote_cached_options" \ + ').cache_key' + end + def _define_cached_delete_method(method_name, *method_arguments, cached_finder_options) method_arguments.flatten! expanded_method_args = method_arguments.join(',') - sorted_method_args = method_arguments.sort.join(',') + expanded_cache_key_args = _expanded_cache_key_args(method_arguments) cached_methods << method_name class_eval <<-RUBY, __FILE__, __LINE__ + 1 @@ -216,7 +228,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#find", - ::ActiveRemote::Cached::ArgumentKeys.new(#{sorted_method_args}, __active_remote_cached_options).cache_key + #{expanded_cache_key_args} ].compact search_cache_key = [ @@ -224,7 +236,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#search", - ::ActiveRemote::Cached::ArgumentKeys.new(#{sorted_method_args}, __active_remote_cached_options).cache_key + #{expanded_cache_key_args} ].compact ::ActiveRemote::Cached.cache.delete(find_cache_key) @@ -236,7 +248,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options def _define_cached_exist_find_method(method_name, *method_arguments, cached_finder_options) method_arguments.flatten! expanded_method_args = method_arguments.join(',') - sorted_method_args = method_arguments.sort.join(',') + expanded_cache_key_args = _expanded_cache_key_args(method_arguments) cached_methods << method_name cached_methods << "#{method_name}?" @@ -252,7 +264,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#find", - ::ActiveRemote::Cached::ArgumentKeys.new(#{sorted_method_args}, __active_remote_cached_options).cache_key + #{expanded_cache_key_args} ].compact ::ActiveRemote::Cached.cache.exist?(cache_key) @@ -265,7 +277,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options def _define_cached_exist_search_method(method_name, *method_arguments, cached_finder_options) method_arguments.flatten! expanded_method_args = method_arguments.join(',') - sorted_method_args = method_arguments.sort.join(',') + expanded_cache_key_args = _expanded_cache_key_args(method_arguments) cached_methods << method_name cached_methods << "#{method_name}?" @@ -281,7 +293,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#search", - ::ActiveRemote::Cached::ArgumentKeys.new(#{sorted_method_args}, __active_remote_cached_options).cache_key + #{expanded_cache_key_args} ].compact ::ActiveRemote::Cached.cache.exist?(cache_key) @@ -294,7 +306,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options def _define_cached_find_method(method_name, *method_arguments, cached_finder_options) method_arguments.flatten! expanded_method_args = method_arguments.join(',') - sorted_method_args = method_arguments.sort.join(',') + expanded_cache_key_args = _expanded_cache_key_args(method_arguments) cached_methods << method_name expanded_search_args = _expanded_search_args(method_arguments) @@ -319,7 +331,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#find", - ::ActiveRemote::Cached::ArgumentKeys.new(#{sorted_method_args}, __active_remote_cached_options).cache_key + #{expanded_cache_key_args} ].compact ::ActiveRemote::Cached.cache.fetch(cache_key, __active_remote_cached_options) do @@ -336,7 +348,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options def _define_cached_search_method(method_name, *method_arguments, cached_finder_options) method_arguments.flatten! expanded_method_args = method_arguments.join(',') - sorted_method_args = method_arguments.sort.join(',') + expanded_cache_key_args = _expanded_cache_key_args(method_arguments) cached_methods << method_name expanded_search_args = _expanded_search_args(method_arguments) @@ -365,7 +377,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#search", - ::ActiveRemote::Cached::ArgumentKeys.new(#{sorted_method_args}, __active_remote_cached_options).cache_key + #{expanded_cache_key_args} ].compact ::ActiveRemote::Cached.cache.fetch(cache_key, __active_remote_cached_options) do @@ -382,7 +394,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options def _define_cached_search_bang_method(method_name, *method_arguments, cached_finder_options) method_arguments.flatten! expanded_method_args = method_arguments.join(',') - sorted_method_args = method_arguments.sort.join(',') + expanded_cache_key_args = _expanded_cache_key_args(method_arguments) cached_methods << method_name expanded_search_args = _expanded_search_args(method_arguments) @@ -416,7 +428,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#search", - ::ActiveRemote::Cached::ArgumentKeys.new(#{sorted_method_args}, __active_remote_cached_options).cache_key + #{expanded_cache_key_args} ].compact ::ActiveRemote::Cached.cache.fetch(cache_key, __active_remote_cached_options) do diff --git a/lib/active_remote/cached/argument_keys.rb b/lib/active_remote/cached/argument_keys.rb index 5a658a6..ad8da4b 100644 --- a/lib/active_remote/cached/argument_keys.rb +++ b/lib/active_remote/cached/argument_keys.rb @@ -22,6 +22,38 @@ class ArgumentKeys [',', 'COM'] ].freeze + # The separators below are absent from both REMOVE_CHARACTERS and + # REPLACE_MAP, so they survive either option. escape_value/1 escapes them + # inside a value, which makes the key one-to-one with the arguments. + FIELD_SEPARATOR = '.' + PAIR_SEPARATOR = '/' + ESCAPE_MAP = { '%' => '%25', FIELD_SEPARATOR => '%2E', PAIR_SEPARATOR => '%2F' }.freeze + ESCAPE_CHARACTERS = %r{[%./]} + + # Build a key that names each field, so that two finders with the same + # values do not share one cache entry. + # + # for_fields([:alpha, :beta], ['x', 'y'], {}).cache_key + # # => "alpha.x/beta.y" + # + def self.for_fields(field_names, values, options) + pairs = field_names.each_with_index.map do |field_name, index| + "#{field_name}#{FIELD_SEPARATOR}#{normalize_value(values[index])}" + end + + new(pairs.join(PAIR_SEPARATOR), options) + end + + def self.normalize_value(value) + [value].flatten.compact.map { |element| escape_value(element) }.join(FIELD_SEPARATOR) + end + private_class_method :normalize_value + + def self.escape_value(value) + value.to_s.gsub(ESCAPE_CHARACTERS, ESCAPE_MAP) + end + private_class_method :escape_value + def initialize(*arguments, options) @options = options @arguments = arguments.flatten.compact diff --git a/spec/active_remote/cached/argument_keys_spec.rb b/spec/active_remote/cached/argument_keys_spec.rb index b0b3b58..29211a9 100644 --- a/spec/active_remote/cached/argument_keys_spec.rb +++ b/spec/active_remote/cached/argument_keys_spec.rb @@ -53,6 +53,66 @@ expect(::ActiveRemote::Cached::ArgumentKeys.new('hello {}', options).cache_key).to eq('hello') end + describe '.for_fields' do + it 'names each field in the key' do + argument_keys = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[alpha beta], %w[x y], {}) + + expect(argument_keys.cache_key).to eq('alpha.x/beta.y') + end + + it 'gives two finders with the same values two different keys' do + alpha_beta = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[alpha beta], %w[x y], {}) + gamma_delta = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[gamma delta], %w[x y], {}) + + expect(alpha_beta.cache_key).not_to eq(gamma_delta.cache_key) + end + + it 'gives a single field a different key than two fields with the same characters' do + one_field = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[guid], %w[xy], {}) + two_fields = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[alpha beta], %w[x y], {}) + + expect(one_field.cache_key).not_to eq(two_fields.cache_key) + end + + it 'joins an array value with a comma' do + argument_keys = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[guid], [%w[a b]], {}) + + expect(argument_keys.cache_key).to eq('guid.a.b') + end + + it 'removes a nil inside an array value' do + argument_keys = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[guid], [['a', nil, 'b']], {}) + + expect(argument_keys.cache_key).to eq('guid.a.b') + end + + it 'gives an empty value for a nil argument' do + argument_keys = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[alpha beta], ['x', nil], {}) + + expect(argument_keys.cache_key).to eq('alpha.x/beta.') + end + + it 'escapes a separator inside a value' do + argument_keys = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[guid], ['a/b.c%d'], {}) + + expect(argument_keys.cache_key).to eq('guid.a%2Fb%2Ec%25d') + end + + it 'keeps the separators when the remove option is given' do + options = { :active_remote_cached_remove_characters => true } + argument_keys = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[alpha beta], ['hello {}', 'y'], options) + + expect(argument_keys.cache_key).to eq('alpha.hello/beta.y') + end + + it 'keeps the separators when the replace option is given' do + options = { :active_remote_cached_replace_characters => true } + argument_keys = ::ActiveRemote::Cached::ArgumentKeys.for_fields(%i[alpha beta], ['hello {}', 'y'], options) + + expect(argument_keys.cache_key).to eq('alpha.helloSPLBRB/beta.y') + end + end + # #cache_key calls gsub! on an instance variable, so a second call must not # replace the characters of the first result again. it 'returns the same key when :active_remote_cached_replace_characters is called twice' do diff --git a/spec/active_remote/cached_delete_methods_spec.rb b/spec/active_remote/cached_delete_methods_spec.rb index 52869f5..b06f956 100644 --- a/spec/active_remote/cached_delete_methods_spec.rb +++ b/spec/active_remote/cached_delete_methods_spec.rb @@ -84,10 +84,12 @@ def self.search(*) describe 'namespaced cache' do it 'deletes the namespaced find and search cache keys' do expect(::ActiveRemote::Cached.cache).to receive(:delete).with( - [::ActiveRemote::Cached::RUBY_AND_ACTIVE_SUPPORT_VERSION, 'MyApp', DeleteMethodClass.name, '#find', 'guid'] + [::ActiveRemote::Cached::RUBY_AND_ACTIVE_SUPPORT_VERSION, 'MyApp', DeleteMethodClass.name, '#find', + 'guid.guid'] ) expect(::ActiveRemote::Cached.cache).to receive(:delete).with( - [::ActiveRemote::Cached::RUBY_AND_ACTIVE_SUPPORT_VERSION, 'MyApp', DeleteMethodClass.name, '#search', 'guid'] + [::ActiveRemote::Cached::RUBY_AND_ACTIVE_SUPPORT_VERSION, 'MyApp', DeleteMethodClass.name, '#search', + 'guid.guid'] ) DeleteMethodClass.cached_delete_by_guid(:guid, :namespace => 'MyApp') diff --git a/spec/active_remote/cached_exist_methods_spec.rb b/spec/active_remote/cached_exist_methods_spec.rb index f82c6ba..ddd2994 100644 --- a/spec/active_remote/cached_exist_methods_spec.rb +++ b/spec/active_remote/cached_exist_methods_spec.rb @@ -171,7 +171,8 @@ def self.search(*) describe 'namespaced cache' do it 'uses the namespace as a prefix to the cache key' do expect(::ActiveRemote::Cached.cache).to receive(:exist?).with( - [::ActiveRemote::Cached::RUBY_AND_ACTIVE_SUPPORT_VERSION, 'MyApp', ExistMethodClass.name, '#find', 'guid'] + [::ActiveRemote::Cached::RUBY_AND_ACTIVE_SUPPORT_VERSION, 'MyApp', ExistMethodClass.name, '#find', + 'guid.guid'] ).and_return(true) expect(ExistMethodClass.cached_exist_find_by_guid?(:guid, :namespace => 'MyApp')).to eq(true) diff --git a/spec/active_remote/cached_find_methods_spec.rb b/spec/active_remote/cached_find_methods_spec.rb index dc21f81..aa0a37c 100644 --- a/spec/active_remote/cached_find_methods_spec.rb +++ b/spec/active_remote/cached_find_methods_spec.rb @@ -79,7 +79,7 @@ def self.search it 'merges the default options in for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, FindMethodClass.name, '#find', 'guid'], { :expires_in => 100 } + [versioned_prefix, FindMethodClass.name, '#find', 'guid.guid'], { :expires_in => 100 } ).and_return(:hello) expect(FindMethodClass).not_to receive(:find) expect(FindMethodClass.cached_find_by_guid(:guid)).to eq(:hello) @@ -87,7 +87,7 @@ def self.search it 'overrides the default options with local options for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, FindMethodClass.name, '#find', 'guid'], { :expires_in => 200 } + [versioned_prefix, FindMethodClass.name, '#find', 'guid.guid'], { :expires_in => 200 } ).and_return(:hello) expect(FindMethodClass).not_to receive(:find) @@ -101,7 +101,7 @@ def self.search it 'uses the namespace as a prefix to the cache key' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, 'MyApp', FindMethodClass.name, '#find', 'guid'], { :expires_in => 100 } + [versioned_prefix, 'MyApp', FindMethodClass.name, '#find', 'guid.guid'], { :expires_in => 100 } ).and_return(:hello) expect(FindMethodClass).not_to receive(:find) @@ -122,7 +122,7 @@ def self.search it 'overrides the default options with cached_finder options for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, FindMethodClass.name, '#find', 'foo'], { :expires_in => 500 } + [versioned_prefix, FindMethodClass.name, '#find', 'foo.foo'], { :expires_in => 500 } ).and_return(:hello) expect(FindMethodClass).not_to receive(:find) @@ -131,7 +131,7 @@ def self.search it 'overrides the cached_finder options with local options for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, FindMethodClass.name, '#find', 'foo'], { :expires_in => 200 } + [versioned_prefix, FindMethodClass.name, '#find', 'foo.foo'], { :expires_in => 200 } ).and_return(:hello) expect(FindMethodClass).not_to receive(:find) diff --git a/spec/active_remote/cached_search_methods_spec.rb b/spec/active_remote/cached_search_methods_spec.rb index d5e31e3..3e2d012 100644 --- a/spec/active_remote/cached_search_methods_spec.rb +++ b/spec/active_remote/cached_search_methods_spec.rb @@ -164,7 +164,7 @@ def self.search it 'merges the default options in for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, SearchMethodClass.name, '#search', 'guid'], { :expires_in => 100 } + [versioned_prefix, SearchMethodClass.name, '#search', 'guid.guid'], { :expires_in => 100 } ).and_return(:hello) expect(SearchMethodClass).not_to receive(:search) @@ -173,7 +173,7 @@ def self.search it 'overrides the default options with local options for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, SearchMethodClass.name, '#search', 'guid'], { :expires_in => 200 } + [versioned_prefix, SearchMethodClass.name, '#search', 'guid.guid'], { :expires_in => 200 } ).and_return(:hello) expect(SearchMethodClass).not_to receive(:search) @@ -187,7 +187,7 @@ def self.search it 'uses the namespace as a prefix to the cache key' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, 'MyApp', SearchMethodClass.name, '#search', 'guid'], { :expires_in => 100 } + [versioned_prefix, 'MyApp', SearchMethodClass.name, '#search', 'guid.guid'], { :expires_in => 100 } ).and_return(:hello) expect(SearchMethodClass).not_to receive(:search) @@ -208,7 +208,7 @@ def self.search it 'overrides the default options with cached_finder options for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, SearchMethodClass.name, '#search', 'foo'], { :expires_in => 500 } + [versioned_prefix, SearchMethodClass.name, '#search', 'foo.foo'], { :expires_in => 500 } ).and_return(:hello) expect(SearchMethodClass).not_to receive(:find) @@ -217,7 +217,7 @@ def self.search it 'overrides the cached_finder options with local options for the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, SearchMethodClass.name, '#search', 'foo'], { :expires_in => 200 } + [versioned_prefix, SearchMethodClass.name, '#search', 'foo.foo'], { :expires_in => 200 } ).and_return(:hello) expect(SearchMethodClass).not_to receive(:find) diff --git a/spec/active_remote/cached_spec.rb b/spec/active_remote/cached_spec.rb index 4aca861..45ef5ec 100644 --- a/spec/active_remote/cached_spec.rb +++ b/spec/active_remote/cached_spec.rb @@ -87,7 +87,7 @@ class ChildFinderClass < ConfigurationClass; end describe 'RUBY_AND_ACTIVE_SUPPORT_VERSION' do it 'prefixes every cache key' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, ConfigurationClass.name, '#find', 'guid'], {} + [versioned_prefix, ConfigurationClass.name, '#find', 'guid.guid'], {} ).and_return(:find_result) ConfigurationClass.cached_find_by_guid(:guid) @@ -115,7 +115,7 @@ class ChildFinderClass < ConfigurationClass; end it 'passes the options through to the fetch call' do expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( - [versioned_prefix, ConfigurationClass.name, '#find', 'guid'], { :expires_in => 200 } + [versioned_prefix, ConfigurationClass.name, '#find', 'guid.guid'], { :expires_in => 200 } ).and_return(:hello) expect(ConfigurationClass.cached_find({ :guid => :guid }, :expires_in => 200)).to eq(:hello) From 5efdf37f564bf24cd279710bbd34337d4bd12aae Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:32:57 -0600 Subject: [PATCH 02/10] Raise the right error from the cached search bang method Two problems in the generated cached_search_by_*! method: raise ::ActiveRemote::RemoteRecordNotFound.new(self.class) if results.first.nil? self.class is Class inside a `def self.` body, so the message read "Class does not exist" and error.remote_record_class returned Class. results.first raised NoMethodError when search returned nil, where the caller expects RemoteRecordNotFound. Co-Authored-By: Claude Opus 5 (1M context) --- lib/active_remote/cached.rb | 4 ++-- .../cached_search_methods_spec.rb | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/lib/active_remote/cached.rb b/lib/active_remote/cached.rb index e095ade..00267bc 100644 --- a/lib/active_remote/cached.rb +++ b/lib/active_remote/cached.rb @@ -412,7 +412,7 @@ def _define_cached_search_bang_method(method_name, *method_arguments, cached_fin # results = self.search(:user_guid => user_guid) # end # - # raise ::ActiveRemote::RemoteRecordNotFound.new(self.class) if results.size <= 0 + # raise ::ActiveRemote::RemoteRecordNotFound, self if results.nil? || results.first.nil? # results # end # end @@ -440,7 +440,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options results = self.search(#{expanded_search_args}) end - raise ::ActiveRemote::RemoteRecordNotFound.new(self.class) if results.first.nil? + raise ::ActiveRemote::RemoteRecordNotFound, self if results.nil? || results.first.nil? results end end diff --git a/spec/active_remote/cached_search_methods_spec.rb b/spec/active_remote/cached_search_methods_spec.rb index 3e2d012..d2c9add 100644 --- a/spec/active_remote/cached_search_methods_spec.rb +++ b/spec/active_remote/cached_search_methods_spec.rb @@ -261,6 +261,28 @@ def self.search end.to raise_error ::ActiveRemote::RemoteRecordNotFound end + it 'raises ActiveRemote::RemoteRecordNotFound when the results are nil' do + expect(SearchMethodClass).to receive(:search).and_return(nil) + expect do + SearchMethodClass.cached_search_by_foo!(:foo) + end.to raise_error ::ActiveRemote::RemoteRecordNotFound + end + + it 'names the model class in the error' do + expect(SearchMethodClass).to receive(:search).and_return([]) + expect do + SearchMethodClass.cached_search_by_foo!(:foo) + end.to raise_error(::ActiveRemote::RemoteRecordNotFound, /SearchMethodClass/) + end + + it 'reports the model class on the error' do + expect(SearchMethodClass).to receive(:search).and_return([]) + + SearchMethodClass.cached_search_by_foo!(:foo) + rescue ::ActiveRemote::RemoteRecordNotFound => e + expect(e.remote_record_class).to eq(SearchMethodClass) + end + it 'does not cache the results when it raises' do expect do SearchMethodClass.cached_search_by_foo!(:foo) { [] } From f365e79d5da23efde888eab0293d828779dc9a7d Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:35:09 -0600 Subject: [PATCH 03/10] Stop interpolating the finder options into the generated source Each generated method built its options through Hash#to_s: .merge(#{cached_finder_options}) The result has to parse as Ruby source, and a common value does not: cached_finders_for :guid, :expires_in => 5.minutes # => SyntaxError 5.minutes.to_s produces "5 minutes". Any object without a literal form failed the same way. The failure also left the class broken. cached_methods << method_name ran before class_eval, so the name was registered with no method behind it. method_missing then dispatched to that same missing name and recursed until Ruby raised SystemStackError. The options now live in a per-class registry that the generated method reads at call time, and the name is registered only after class_eval returns. The lookup walks the superclass chain, so a subclass keeps the declared options. Co-Authored-By: Claude Opus 5 (1M context) --- lib/active_remote/cached.rb | 75 ++++++++++++++++++++------- spec/active_remote/cached_spec.rb | 86 +++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 20 deletions(-) diff --git a/lib/active_remote/cached.rb b/lib/active_remote/cached.rb index 00267bc..5f0da36 100644 --- a/lib/active_remote/cached.rb +++ b/lib/active_remote/cached.rb @@ -38,6 +38,29 @@ def cached_methods @cached_methods end + # The options each cached finder was declared with, keyed by method name. + # The generated methods read this at call time. Interpolating the options + # into the generated source instead would require every value to have a + # literal form, and a value such as 5.minutes does not. + def cached_finder_options + @cached_finder_options ||= {} + end + + # A subclass inherits the finder methods its parent defined, but not the + # parent registry, so the lookup walks up the superclass chain. + def _cached_finder_options_for(method_name) + klass = self + + while klass.respond_to?(:cached_finder_options) + options = klass.cached_finder_options[method_name] + return options if options + + klass = klass.superclass + end + + {} + end + def cached_finders_for(*cached_finder_keys) options = cached_finder_keys.extract_options! @@ -210,18 +233,17 @@ def _expanded_cache_key_args(method_arguments) ').cache_key' end - def _define_cached_delete_method(method_name, *method_arguments, cached_finder_options) + def _define_cached_delete_method(method_name, *method_arguments, cached_finder_options_hash) method_arguments.flatten! expanded_method_args = method_arguments.join(',') expanded_cache_key_args = _expanded_cache_key_args(method_arguments) - cached_methods << method_name class_eval <<-RUBY, __FILE__, __LINE__ + 1 # def self.cached_delete_by_user_guid(user_guid, options = {}) # ::ActiveRemote::Cached.cache.delete([name, user_guid]) # end def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options = {}) - __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(#{cached_finder_options}).merge(__active_remote_cached_options) + __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(_cached_finder_options_for('#{method_name}')).merge(__active_remote_cached_options) namespace = __active_remote_cached_options.delete(:namespace) find_cache_key = [ RUBY_AND_ACTIVE_SUPPORT_VERSION, @@ -243,21 +265,22 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options ::ActiveRemote::Cached.cache.delete(search_cache_key) end RUBY + + cached_finder_options[method_name] = cached_finder_options_hash + cached_methods << method_name end - def _define_cached_exist_find_method(method_name, *method_arguments, cached_finder_options) + def _define_cached_exist_find_method(method_name, *method_arguments, cached_finder_options_hash) method_arguments.flatten! expanded_method_args = method_arguments.join(',') expanded_cache_key_args = _expanded_cache_key_args(method_arguments) - cached_methods << method_name - cached_methods << "#{method_name}?" class_eval <<-RUBY, __FILE__, __LINE__ + 1 # def self.cached_exist_find_by_user_guid(user_guid, options = {}) # ::ActiveRemote::Cached.cache.exist?([name, user_guid]) # end def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options = {}) - __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(#{cached_finder_options}).merge(__active_remote_cached_options) + __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(_cached_finder_options_for('#{method_name}')).merge(__active_remote_cached_options) namespace = __active_remote_cached_options.delete(:namespace) cache_key = [ RUBY_AND_ACTIVE_SUPPORT_VERSION, @@ -272,21 +295,23 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options RUBY singleton_class.send(:alias_method, "#{method_name}?", method_name) + + cached_finder_options[method_name] = cached_finder_options_hash + cached_methods << method_name + cached_methods << "#{method_name}?" end - def _define_cached_exist_search_method(method_name, *method_arguments, cached_finder_options) + def _define_cached_exist_search_method(method_name, *method_arguments, cached_finder_options_hash) method_arguments.flatten! expanded_method_args = method_arguments.join(',') expanded_cache_key_args = _expanded_cache_key_args(method_arguments) - cached_methods << method_name - cached_methods << "#{method_name}?" class_eval <<-RUBY, __FILE__, __LINE__ + 1 # def self.cached_exist_search_by_user_guid(user_guid, options = {}) # ::ActiveRemote::Cached.cache.exist?([namespace, name, "#search", user_guid]) # end def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options = {}) - __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(#{cached_finder_options}).merge(__active_remote_cached_options) + __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(_cached_finder_options_for('#{method_name}')).merge(__active_remote_cached_options) namespace = __active_remote_cached_options.delete(:namespace) cache_key = [ RUBY_AND_ACTIVE_SUPPORT_VERSION, @@ -301,13 +326,16 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options RUBY singleton_class.send(:alias_method, "#{method_name}?", method_name) + + cached_finder_options[method_name] = cached_finder_options_hash + cached_methods << method_name + cached_methods << "#{method_name}?" end - def _define_cached_find_method(method_name, *method_arguments, cached_finder_options) + def _define_cached_find_method(method_name, *method_arguments, cached_finder_options_hash) method_arguments.flatten! expanded_method_args = method_arguments.join(',') expanded_cache_key_args = _expanded_cache_key_args(method_arguments) - cached_methods << method_name expanded_search_args = _expanded_search_args(method_arguments) @@ -324,7 +352,7 @@ def _define_cached_find_method(method_name, *method_arguments, cached_finder_opt # of the result object is maintained for requests/responses # def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options = {}) - __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(#{cached_finder_options}).merge(__active_remote_cached_options) + __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(_cached_finder_options_for('#{method_name}')).merge(__active_remote_cached_options) namespace = __active_remote_cached_options.delete(:namespace) cache_key = [ RUBY_AND_ACTIVE_SUPPORT_VERSION, @@ -343,13 +371,15 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options end end RUBY + + cached_finder_options[method_name] = cached_finder_options_hash + cached_methods << method_name end - def _define_cached_search_method(method_name, *method_arguments, cached_finder_options) + def _define_cached_search_method(method_name, *method_arguments, cached_finder_options_hash) method_arguments.flatten! expanded_method_args = method_arguments.join(',') expanded_cache_key_args = _expanded_cache_key_args(method_arguments) - cached_methods << method_name expanded_search_args = _expanded_search_args(method_arguments) @@ -370,7 +400,7 @@ def _define_cached_search_method(method_name, *method_arguments, cached_finder_o # of the result object is maintained for requests/responses # def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options = {}) - __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(#{cached_finder_options}).merge(__active_remote_cached_options) + __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(_cached_finder_options_for('#{method_name}')).merge(__active_remote_cached_options) namespace = __active_remote_cached_options.delete(:namespace) cache_key = [ RUBY_AND_ACTIVE_SUPPORT_VERSION, @@ -389,13 +419,15 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options end end RUBY + + cached_finder_options[method_name] = cached_finder_options_hash + cached_methods << method_name end - def _define_cached_search_bang_method(method_name, *method_arguments, cached_finder_options) + def _define_cached_search_bang_method(method_name, *method_arguments, cached_finder_options_hash) method_arguments.flatten! expanded_method_args = method_arguments.join(',') expanded_cache_key_args = _expanded_cache_key_args(method_arguments) - cached_methods << method_name expanded_search_args = _expanded_search_args(method_arguments) @@ -421,7 +453,7 @@ def _define_cached_search_bang_method(method_name, *method_arguments, cached_fin # of the result object is maintained for requests/responses # def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options = {}) - __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(#{cached_finder_options}).merge(__active_remote_cached_options) + __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(_cached_finder_options_for('#{method_name}')).merge(__active_remote_cached_options) namespace = __active_remote_cached_options.delete(:namespace) cache_key = [ RUBY_AND_ACTIVE_SUPPORT_VERSION, @@ -445,6 +477,9 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options end end RUBY + + cached_finder_options[method_name] = cached_finder_options_hash + cached_methods << method_name end end diff --git a/spec/active_remote/cached_spec.rb b/spec/active_remote/cached_spec.rb index 45ef5ec..12ad1fc 100644 --- a/spec/active_remote/cached_spec.rb +++ b/spec/active_remote/cached_spec.rb @@ -2,6 +2,9 @@ require 'spec_helper' +# 5.minutes below. The library does not require this itself. +require 'active_support/core_ext/numeric/time' + class ConfigurationClass include ::ActiveRemote::Cached @@ -36,6 +39,24 @@ def self.search(*) class ChildFinderClass < ConfigurationClass; end +class DurationOptionClass + include ::ActiveRemote::Cached + + def self.find(*) + :find_result + end + + def self.search(*) + [:search_result] + end + + # 5.minutes has no literal form. The options must not be interpolated into + # the generated source. + cached_finders_for :guid, :expires_in => 5.minutes +end + +class DurationChildClass < DurationOptionClass; end + describe ::ActiveRemote::Cached do let(:versioned_prefix) { ::ActiveRemote::Cached::RUBY_AND_ACTIVE_SUPPORT_VERSION } @@ -197,6 +218,71 @@ class ChildFinderClass < ConfigurationClass; end end end + describe 'a finder whose definition fails' do + let(:broken_class) do + Class.new do + include ::ActiveRemote::Cached + + def self.find(*) + :find_result + end + + def self.search(*) + [:search_result] + end + end + end + + # cached_methods used to be written before class_eval ran, so a failure + # registered a name with no method behind it. method_missing then + # dispatched to that same missing name and recursed until the stack ran out. + it 'registers no method name' do + expect { broken_class.cached_finders_for :'bad-name' }.to raise_error(::SyntaxError) + + expect(broken_class.cached_methods).to eq([]) + end + + it 'raises NoMethodError rather than recursing' do + begin + broken_class.cached_finders_for :'bad-name' + rescue ::SyntaxError # rubocop:disable Lint/SuppressedException + end + + expect { broken_class.cached_delete_by_guid('x') }.to raise_error(::NoMethodError) + end + end + + describe 'a finder declared with an option that has no literal form' do + it 'defines the finder methods' do + expect(DurationOptionClass).to respond_to(:cached_find_by_guid) + expect(DurationOptionClass).to respond_to(:cached_search_by_guid) + end + + it 'passes the option through to the fetch call' do + expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( + [versioned_prefix, DurationOptionClass.name, '#find', 'guid.guid'], { :expires_in => 5.minutes } + ).and_return(:hello) + + expect(DurationOptionClass.cached_find_by_guid(:guid)).to eq(:hello) + end + + it 'lets a local option override the declared option' do + expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( + [versioned_prefix, DurationOptionClass.name, '#find', 'guid.guid'], { :expires_in => 200 } + ).and_return(:hello) + + expect(DurationOptionClass.cached_find_by_guid(:guid, :expires_in => 200)).to eq(:hello) + end + + it 'applies the declared option in a subclass' do + expect(::ActiveRemote::Cached.cache).to receive(:fetch).with( + [versioned_prefix, DurationChildClass.name, '#find', 'guid.guid'], { :expires_in => 5.minutes } + ).and_return(:hello) + + expect(DurationChildClass.cached_find_by_guid(:guid)).to eq(:hello) + end + end + describe 'a subclass of a class with cached finders' do it 'responds to the finders the parent defined' do expect(ChildFinderClass).to respond_to(:cached_find_by_alpha_and_beta) From ca4216eb215a85a58a33707397a6da25682d3457 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:36:13 -0600 Subject: [PATCH 04/10] Raise ArgumentError on a bad call to a reordered finder _args_in_sorted_order looked each field name up by index and passed the result through without a check: index = orignal_args_name.index(arg_name) args_in_order << args[index] A missing positional argument became nil with no error, and the finder then cached a record under a key built from that nil: E.cached_find_by_beta_and_alpha("only_one") # => {alpha: nil, beta: "only_one"} E.cached_find_by_alpha_and_beta("only_one") # => ArgumentError The same call raised in the defined order and passed nil in the reordered order. A field name absent from the called name raised TypeError from args[nil] rather than a useful error. Also corrects the orignal_args_name typo and the match_1/match_2 names, which drops the Naming/VariableNumber and Metrics/AbcSize suppressions. Co-Authored-By: Claude Opus 5 (1M context) --- lib/active_remote/cached.rb | 41 ++++++++++++++++--------------- spec/active_remote/cached_spec.rb | 22 +++++++++++++++++ 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/lib/active_remote/cached.rb b/lib/active_remote/cached.rb index 5f0da36..227e16b 100644 --- a/lib/active_remote/cached.rb +++ b/lib/active_remote/cached.rb @@ -128,37 +128,38 @@ def _method_missing_name(m) "#{::Regexp.last_match(1)}#{params.sort.join('_and_')}#{::Regexp.last_match(3)}".to_sym end - # rubocop:disable Metrics/AbcSize def _args_in_sorted_order(m, args) regex = /cached_(?:delete|exist_search|search|exist_find|find)_by_([0-9a-zA-Z_]*)(!|\?)?/ - method_name = _method_missing_name(m) + called_match = m.match(regex) + sorted_match = _method_missing_name(m).match(regex) - match_1 = m.match(regex) - match_2 = method_name.match(regex) + return args unless called_match[1] && sorted_match[1] - args_in_order = [] + called_field_names = called_match[1].split('_and_') + sorted_field_names = sorted_match[1].split('_and_') - if match_1[1] && match_2[1] - orignal_args_name = match_1[1].split('_and_') - args_names_in_order = match_2[1].split('_and_') + _reorder_args(m, args, called_field_names, sorted_field_names) + end - args_names_in_order.each do |arg_name| - index = orignal_args_name.index(arg_name) - args_in_order << args[index] - end + def _reorder_args(m, args, called_field_names, sorted_field_names) + if args.size < called_field_names.size + raise ::ArgumentError, + "wrong number of arguments to #{m} (given #{args.size}, expected #{called_field_names.size})" + end - if args.size > args_in_order.size - # Add options if passed - args_in_order << args.last - end + args_in_order = sorted_field_names.map do |field_name| + index = called_field_names.index(field_name) + raise ::ArgumentError, "#{m} has no argument named #{field_name}" if index.nil? - args_in_order - else - args + args[index] end + + # Add the options hash if the caller passed one. + args_in_order << args.last if args.size > called_field_names.size + + args_in_order end - # rubocop:enable Metrics/AbcSize # rubocop:disable Metrics/AbcSize def _create_cached_finder_for(cached_finder_key, options = {}) diff --git a/spec/active_remote/cached_spec.rb b/spec/active_remote/cached_spec.rb index 12ad1fc..de22553 100644 --- a/spec/active_remote/cached_spec.rb +++ b/spec/active_remote/cached_spec.rb @@ -198,6 +198,28 @@ class DurationChildClass < DurationOptionClass; end end end + describe '._args_in_sorted_order' do + it 'reorders the arguments to match the defined method' do + expect(ConfigurationClass).to receive(:cached_find_by_alpha_and_beta).with('A', 'B') + + ConfigurationClass.cached_find_by_beta_and_alpha('B', 'A') + end + + it 'passes the options hash through as the last argument' do + expect(ConfigurationClass).to receive(:cached_find_by_alpha_and_beta).with('A', 'B', { :expires_in => 10 }) + + ConfigurationClass.cached_find_by_beta_and_alpha('B', 'A', { :expires_in => 10 }) + end + + # A missing argument used to become nil, and the finder then cached a + # record under a key built from that nil. + it 'raises ArgumentError when an argument is missing' do + expect do + ConfigurationClass.cached_find_by_beta_and_alpha('B') + end.to raise_error(::ArgumentError, /given 1, expected 2/) + end + end + describe '._method_missing_name' do it 'returns nil when the method name is not a finder' do expect(ConfigurationClass._method_missing_name(:not_a_finder)).to be_nil From 815c735bc344f51453eb5ffc9cb883ed728863c1 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:37:49 -0600 Subject: [PATCH 05/10] Remove the write-then-delete round trip for a nil value Cache#fetch let the provider write the value, then deleted it when the value was not valid. Three searches that returned nil produced this call sequence: [:fetch, :write, :delete, :fetch, :write, :delete, :fetch, :write, :delete] Two of every three calls were waste. Against Redis or Memcached that is two network round trips per request, on the cache-miss path. The provider now receives :skip_nil unless the caller passed :allow_nil, so the nil is never written and needs no delete. The empty-value case still writes and deletes, because ActiveSupport has no matching option. The generated delete method also built the same ArgumentKeys twice. It now builds the key once and uses it for both the find and the search key. Co-Authored-By: Claude Opus 5 (1M context) --- lib/active_remote/cached.rb | 5 +-- lib/active_remote/cached/cache.rb | 23 ++++++++++-- spec/active_remote/cached/cache_spec.rb | 47 +++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/lib/active_remote/cached.rb b/lib/active_remote/cached.rb index 227e16b..5d4a5e2 100644 --- a/lib/active_remote/cached.rb +++ b/lib/active_remote/cached.rb @@ -246,12 +246,13 @@ def _define_cached_delete_method(method_name, *method_arguments, cached_finder_o def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options = {}) __active_remote_cached_options = ::ActiveRemote::Cached.default_options.merge(_cached_finder_options_for('#{method_name}')).merge(__active_remote_cached_options) namespace = __active_remote_cached_options.delete(:namespace) + argument_cache_key = #{expanded_cache_key_args} find_cache_key = [ RUBY_AND_ACTIVE_SUPPORT_VERSION, namespace, name, "#find", - #{expanded_cache_key_args} + argument_cache_key ].compact search_cache_key = [ @@ -259,7 +260,7 @@ def self.#{method_name}(#{expanded_method_args}, __active_remote_cached_options namespace, name, "#search", - #{expanded_cache_key_args} + argument_cache_key ].compact ::ActiveRemote::Cached.cache.delete(find_cache_key) diff --git a/lib/active_remote/cached/cache.rb b/lib/active_remote/cached/cache.rb index ffb8e25..a84b5a0 100644 --- a/lib/active_remote/cached/cache.rb +++ b/lib/active_remote/cached/cache.rb @@ -34,9 +34,10 @@ def exist?(*args) end def fetch(name, options = {}) - fetch_value = nested_cache_provider.fetch(name, options) { super } + provider_options = provider_fetch_options(options) + fetch_value = nested_cache_provider.fetch(name, provider_options) { super(name, provider_options) } - delete(name) unless valid_fetched_value?(fetch_value, options) + delete(name) if delete_after_fetch?(fetch_value, options, provider_options) fetch_value end @@ -54,6 +55,24 @@ def write(*args) attr_reader :nested_cache_provider + # :skip_nil tells the provider not to write a nil at all, which saves a + # write and the delete that follows it. Only an ActiveSupport store is + # known to honor the option. + def provider_fetch_options(options) + return options if options.fetch(:allow_nil, false) + return options unless cache_provider.is_a?(::ActiveSupport::Cache::Store) + + options.merge(:skip_nil => true) + end + + def delete_after_fetch?(value, options, provider_options) + return false if valid_fetched_value?(value, options) + # The provider already skipped the write. + return false if value.nil? && provider_options[:skip_nil] + + true + end + def valid_fetched_value?(value, options = {}) return false if value.nil? && !options.fetch(:allow_nil, false) return false if !options.fetch(:allow_empty, false) && value.respond_to?(:empty?) && value.empty? diff --git a/spec/active_remote/cached/cache_spec.rb b/spec/active_remote/cached/cache_spec.rb index 16851f2..86027ce 100644 --- a/spec/active_remote/cached/cache_spec.rb +++ b/spec/active_remote/cached/cache_spec.rb @@ -71,6 +71,53 @@ end end + describe '#fetch round trips' do + let(:counting_provider) do + Class.new(::ActiveSupport::Cache::MemoryStore) do + def initialize(*args) + @calls = [] + super + end + + attr_reader :calls + + def fetch(*args, **options, &block) + @calls << :fetch + super + end + + def write(*args, **options) + @calls << :write + super + end + + def delete(*args, **options) + @calls << :delete + super + end + end.new + end + + # The provider used to write the nil and then take a second round trip to + # delete it. :skip_nil makes the provider skip the write. + it 'takes one provider call for a nil value' do + cache = ::ActiveRemote::Cached::Cache.new(counting_provider) + + cache.fetch('key') { nil } + + expect(counting_provider.calls).to eq([:fetch]) + end + + it 'still writes a nil when :allow_nil is given' do + cache = ::ActiveRemote::Cached::Cache.new(counting_provider) + + cache.fetch('key', :allow_nil => true) { nil } + + expect(counting_provider.calls).to eq(%i[fetch write]) + expect(counting_provider.exist?('key')).to eq(true) + end + end + describe '#enable_nested_caching!' do it 'writes to the cache provider only until nested caching is enabled' do cache.write('key', 'value') From 5d6833f05492222dfe4b6200b9d5d6e862951d19 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:38:32 -0600 Subject: [PATCH 06/10] Raise a real error class from the cache provider validator validate_provider_method_present raised a bare String, so callers got a RuntimeError whose message carried the heredoc indentation. It now raises ActiveRemote::Cached::Cache::InvalidCacheProvider with a single-line message. The raise was also indented two columns past the return above it. That one line was why .rubocop.yml disabled Layout/IndentationConsistency for the whole repository. Six cop exclusions are now dead and removed. I forced each one on and got no offenses: Layout/AccessModifierIndentation, Layout/CommentIndentation, Layout/IndentationConsistency, Style/AccessModifierDeclarations, Style/MissingRespondToMissing (respond_to_missing? is defined), and Naming/VariableNumber. The global Metrics/MethodLength disable becomes a per-file exclusion, so a long method in a new file is still flagged. Co-Authored-By: Claude Opus 5 (1M context) --- .rubocop.yml | 21 ++++----------------- lib/active_remote/cached/cache.rb | 11 +++++++---- spec/active_remote/cached/cache_spec.rb | 11 ++++++----- 3 files changed, 17 insertions(+), 26 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 9a995b5..5d79382 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -12,16 +12,6 @@ AllCops: Exclude: - 'gemfiles/**/*' -Layout/AccessModifierIndentation: - Enabled: false -Layout/CommentIndentation: - Enabled: false -Layout/IndentationConsistency: - Enabled: false - -Style/AccessModifierDeclarations: - Enabled: false - Naming/MethodParameterName: Enabled: false @@ -47,11 +37,11 @@ Metrics/ModuleLength: Style/Documentation: Enabled: false +# The generated finder methods in this file are long heredocs. Metrics/MethodLength: - Enabled: false - -Naming/VariableNumber: - Enabled: false + Exclude: + - 'lib/active_remote/cached.rb' + - 'spec/**/*' Style/HashSyntax: Description: >- @@ -65,6 +55,3 @@ Style/HashSyntax: # Use lambdas instead of stabbys Style/Lambda: EnforcedStyle: lambda - -Style/MissingRespondToMissing: - Enabled: false diff --git a/lib/active_remote/cached/cache.rb b/lib/active_remote/cached/cache.rb index a84b5a0..267911a 100644 --- a/lib/active_remote/cached/cache.rb +++ b/lib/active_remote/cached/cache.rb @@ -5,6 +5,10 @@ module ActiveRemote module Cached class Cache < ::SimpleDelegator + # Raised when the given cache provider is missing a method the library + # calls on it. + class InvalidCacheProvider < ::StandardError; end + attr_reader :cache_provider def initialize(new_cache_provider) @@ -83,10 +87,9 @@ def valid_fetched_value?(value, options = {}) def validate_provider_method_present(method_name) return if cache_provider.respond_to?(method_name) - raise <<-CACHE_METHOD - ActiveRemote::Cached::Cache must respond_to? #{method_name} - in order to be used as a caching interface for ActiveRemote - CACHE_METHOD + raise InvalidCacheProvider, + "ActiveRemote::Cached::Cache must respond_to? #{method_name} " \ + 'in order to be used as a caching interface for ActiveRemote' end end end diff --git a/spec/active_remote/cached/cache_spec.rb b/spec/active_remote/cached/cache_spec.rb index 86027ce..b077dbd 100644 --- a/spec/active_remote/cached/cache_spec.rb +++ b/spec/active_remote/cached/cache_spec.rb @@ -3,33 +3,34 @@ require 'spec_helper' describe ::ActiveRemote::Cached::Cache do + let(:invalid_provider_error) { ::ActiveRemote::Cached::Cache::InvalidCacheProvider } let(:cache_provider) { ::ActiveSupport::Cache::MemoryStore.new } let(:cache) { ::ActiveRemote::Cached::Cache.new(cache_provider) } describe 'API' do it 'validates #delete present' do cache = OpenStruct.new(:write => nil, :fetch => nil, :read => nil, :exist? => nil) - expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(RuntimeError, /respond_to.*delete/i) + expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(invalid_provider_error, /respond_to.*delete/i) end it 'validates #exist? present' do cache = OpenStruct.new(:write => nil, :delete => nil, :read => nil, :fetch => nil) - expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(RuntimeError, /respond_to.*exist/i) + expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(invalid_provider_error, /respond_to.*exist/i) end it 'validates #fetch present' do cache = OpenStruct.new(:write => nil, :delete => nil, :read => nil, :exist? => nil) - expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(RuntimeError, /respond_to.*fetch/i) + expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(invalid_provider_error, /respond_to.*fetch/i) end it 'validates #read present' do cache = OpenStruct.new(:write => nil, :delete => nil, :fetch => nil, :exist? => nil) - expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(RuntimeError, /respond_to.*read/i) + expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(invalid_provider_error, /respond_to.*read/i) end it 'validates #write present' do cache = OpenStruct.new(:read => nil, :delete => nil, :fetch => nil, :exist? => nil) - expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(RuntimeError, /respond_to.*write/i) + expect { ::ActiveRemote::Cached.cache(cache) }.to raise_error(invalid_provider_error, /respond_to.*write/i) end end From c130217e5f62f2053d5108c1d3c402ff4a749b32 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:39:23 -0600 Subject: [PATCH 07/10] Fix the gemspec metadata and file list - homepage was an empty string; set the GitHub URL. - No license was declared, though LICENSE.txt says MIT. gem build warned. - test_files is deprecated; removed. - files called git ls-files with no fallback, so a build from a released tarball produced an empty file list. - activesupport had no version floor, though the code calls ActiveSupport::Cache::NullStore and reads ActiveSupport::VERSION::STRING. Set >= 6.1 to match the active_remote floor. - Added metadata with rubygems_mfa_required. Co-Authored-By: Claude Opus 5 (1M context) --- .rubocop.yml | 1 + active_remote-cached.gemspec | 30 ++++++++++++++++++++++++++---- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 5d79382..31cab56 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -18,6 +18,7 @@ Naming/MethodParameterName: Metrics/BlockLength: Exclude: - 'spec/**/*' + - '*.gemspec' # ActiveSupport cache #fetch only calls the block on a miss, so the block form # is not the same as the Hash#fetch default value form. diff --git a/active_remote-cached.gemspec b/active_remote-cached.gemspec index 45dcde9..da0ae7f 100644 --- a/active_remote-cached.gemspec +++ b/active_remote-cached.gemspec @@ -6,6 +6,21 @@ lib = File.expand_path('lib', __dir__) $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) require 'active_remote/cached/version' +HOMEPAGE = 'https://github.com/mxenabled/active_remote-cached' + +# git ls-files returns nothing outside a checkout, so a build from a released +# tarball needs the glob. +def gem_files + files = if File.directory?(File.join(__dir__, '.git')) + `git ls-files`.split($INPUT_RECORD_SEPARATOR) + else + Dir.glob('{lib,spec}/**/*', File::FNM_DOTMATCH) + + %w[LICENSE.txt README.md Rakefile Appraisals active_remote-cached.gemspec] + end + + files.reject { |file| File.directory?(file) } +end + Gem::Specification.new do |gem| gem.name = 'active_remote-cached' gem.version = ActiveRemote::Cached::VERSION @@ -13,16 +28,23 @@ Gem::Specification.new do |gem| gem.email = ['brandonsdewitt@gmail.com', 'devexperience@mx.com'] gem.description = ' Provides "cached" finders and a DSL to enumerate which finders should have cached versions ' gem.summary = ' Provides a configuration for caching mechanisms and finders on ActiveRemote models' - gem.homepage = '' + gem.homepage = HOMEPAGE + gem.license = 'MIT' + + gem.metadata = { + 'homepage_uri' => HOMEPAGE, + 'source_code_uri' => HOMEPAGE, + 'rubygems_mfa_required' => 'true' + } gem.required_ruby_version = '>= 3.1' - gem.files = `git ls-files`.split($INPUT_RECORD_SEPARATOR) + gem.files = gem_files gem.executables = gem.files.grep(%r{^bin/}).map { |f| File.basename(f) } - gem.test_files = gem.files.grep(%r{^(test|spec|features)/}) gem.require_paths = ['lib'] gem.add_dependency 'active_remote', '>= 6.1' - gem.add_dependency 'activesupport' + # NullStore and ActiveSupport::VERSION::STRING. Matches the active_remote floor. + gem.add_dependency 'activesupport', '>= 6.1' gem.add_development_dependency 'appraisal' gem.add_development_dependency 'bundler' From ccf3c9f36ad09a030519e94a9545bc3c78983cb4 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:40:26 -0600 Subject: [PATCH 08/10] Keep nested caching on when the cache provider is replaced ActiveRemote::Cached.cache(provider) builds a new Cache, and a new Cache starts with a NullStore as its nested provider. A nested cache that enable_nested_caching! had turned on was silently lost: cache.enable_nested_caching! ActiveRemote::Cached.cache(other_store) # nested provider is a NullStore again The Railtie calls enable_nested_caching! after it sets the provider, so Rails was unaffected. An application that swapped the store later was not. Adds Cache#nested_caching? and carries the setting to the new Cache. Co-Authored-By: Claude Opus 5 (1M context) --- lib/active_remote/cached.rb | 7 ++++++- lib/active_remote/cached/cache.rb | 4 ++++ spec/active_remote/cached/cache_spec.rb | 26 +++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/lib/active_remote/cached.rb b/lib/active_remote/cached.rb index 5d4a5e2..864aecb 100644 --- a/lib/active_remote/cached.rb +++ b/lib/active_remote/cached.rb @@ -21,7 +21,12 @@ module Cached RUBY_AND_ACTIVE_SUPPORT_VERSION = "#{RUBY_ENGINE_VERSION}:#{ActiveSupport::VERSION::STRING}".freeze def self.cache(cache_provider = nil) - @cache_provider = ::ActiveRemote::Cached::Cache.new(cache_provider) if cache_provider + if cache_provider + nested_caching = @cache_provider&.nested_caching? + @cache_provider = ::ActiveRemote::Cached::Cache.new(cache_provider) + # A new Cache starts with nested caching off, so carry the setting over. + @cache_provider.enable_nested_caching! if nested_caching + end @cache_provider end diff --git a/lib/active_remote/cached/cache.rb b/lib/active_remote/cached/cache.rb index 267911a..187098e 100644 --- a/lib/active_remote/cached/cache.rb +++ b/lib/active_remote/cached/cache.rb @@ -33,6 +33,10 @@ def enable_nested_caching! @nested_cache_provider = ::ActiveSupport::Cache::MemoryStore.new end + def nested_caching? + !nested_cache_provider.is_a?(::ActiveSupport::Cache::NullStore) + end + def exist?(*args) nested_cache_provider.exist?(*args) || super end diff --git a/spec/active_remote/cached/cache_spec.rb b/spec/active_remote/cached/cache_spec.rb index b077dbd..8defcf7 100644 --- a/spec/active_remote/cached/cache_spec.rb +++ b/spec/active_remote/cached/cache_spec.rb @@ -34,6 +34,12 @@ end end + describe '#nested_caching?' do + it 'is false before nested caching is enabled' do + expect(cache.nested_caching?).to eq(false) + end + end + describe 'delegation' do it 'exposes the cache provider it was given' do expect(cache.cache_provider).to be(cache_provider) @@ -159,6 +165,26 @@ def delete(*args, **options) expect(cache.exist?('key')).to eq(true) end + it 'reports that nested caching is on' do + expect(cache.nested_caching?).to eq(true) + end + + # A new Cache starts with nested caching off, so swapping the provider + # used to turn the setting off without saying so. + it 'keeps nested caching on when the cache provider is replaced' do + original_cache = ::ActiveRemote::Cached.cache + + ::ActiveRemote::Cached.cache(cache_provider) + ::ActiveRemote::Cached.cache.enable_nested_caching! + ::ActiveRemote::Cached.cache(::ActiveSupport::Cache::MemoryStore.new) + + expect(::ActiveRemote::Cached.cache.nested_caching?).to eq(true) + ensure + # .cache now carries the nested setting forward, so reset the module + # rather than call it again. + ::ActiveRemote::Cached.instance_variable_set(:@cache_provider, original_cache) + end + # #read joins the two providers with ||, so a false value in the nested # cache falls through to the cache provider. This records that behavior. it 'falls through to the cache provider when the nested value is false' do From 2b2d32a3528e38c2d92f4e9854a75db8ea5434c1 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:41:08 -0600 Subject: [PATCH 09/10] Cover the same whitespace in both ArgumentKeys character maps REMOVE_CHARACTERS uses [[:space:]], so it strips a tab and a newline. REPLACE_MAP listed only a plain space, so it left them in the key: remove "a\tb" # => "ab" replace "a\tb" # => "a\tb" tab survives into the key A raw tab or newline in a cache key breaks the Memcached protocol. REPLACE_MAP now covers tab, newline, carriage return, form feed, and vertical tab. It is also a Hash rather than an array of pairs, so #cache_key makes one gsub pass over the string instead of 13. Co-Authored-By: Claude Opus 5 (1M context) --- lib/active_remote/cached/argument_keys.rb | 43 +++++++++++-------- .../cached/argument_keys_spec.rb | 26 +++++++++++ 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/lib/active_remote/cached/argument_keys.rb b/lib/active_remote/cached/argument_keys.rb index ad8da4b..e39ff8d 100644 --- a/lib/active_remote/cached/argument_keys.rb +++ b/lib/active_remote/cached/argument_keys.rb @@ -6,21 +6,29 @@ class ArgumentKeys attr_reader :arguments, :argument_string, :options REMOVE_CHARACTERS = /[[:space:]+=><{}\[\];:\-,]/ - REPLACE_MAP = [ - [' ', 'SP'], - ['+', 'PL'], - ['=', 'EQ'], - ['>', 'GT'], - ['<', 'LT'], - ['{', 'LB'], - ['}', 'RB'], - ['[', 'LB2'], - [']', 'RB2'], - [';', 'SC'], - [':', 'CO'], - ['-', 'DA'], - [',', 'COM'] - ].freeze + # Covers the same characters as REMOVE_CHARACTERS. A tab or a newline + # left in a key breaks the Memcached protocol. + REPLACE_MAP = { + ' ' => 'SP', + "\t" => 'TB', + "\n" => 'NL', + "\r" => 'CR', + "\f" => 'FF', + "\v" => 'VT', + '+' => 'PL', + '=' => 'EQ', + '>' => 'GT', + '<' => 'LT', + '{' => 'LB', + '}' => 'RB', + '[' => 'LB2', + ']' => 'RB2', + ';' => 'SC', + ':' => 'CO', + '-' => 'DA', + ',' => 'COM' + }.freeze + REPLACE_CHARACTERS = ::Regexp.union(REPLACE_MAP.keys) # The separators below are absent from both REMOVE_CHARACTERS and # REPLACE_MAP, so they survive either option. escape_value/1 escapes them @@ -64,9 +72,8 @@ def cache_key return @argument_string.gsub(REMOVE_CHARACTERS, '') if remove_characters? return @argument_string unless replace_characters? - REPLACE_MAP.inject(@argument_string) do |key, (character, replacement)| - key.gsub(character, replacement) - end + # One pass, rather than one gsub for each entry in the map. + @argument_string.gsub(REPLACE_CHARACTERS, REPLACE_MAP) end def to_s diff --git a/spec/active_remote/cached/argument_keys_spec.rb b/spec/active_remote/cached/argument_keys_spec.rb index 29211a9..079adad 100644 --- a/spec/active_remote/cached/argument_keys_spec.rb +++ b/spec/active_remote/cached/argument_keys_spec.rb @@ -25,6 +25,32 @@ expect(::ActiveRemote::Cached::ArgumentKeys.new('hello {}', options).cache_key).to eq('helloSPLBRB') end + it 'replaces a tab when :active_remote_cached_replace_characters' do + options = { :active_remote_cached_replace_characters => true } + expect(::ActiveRemote::Cached::ArgumentKeys.new("a\tb", options).cache_key).to eq('aTBb') + end + + it 'replaces a newline when :active_remote_cached_replace_characters' do + options = { :active_remote_cached_replace_characters => true } + expect(::ActiveRemote::Cached::ArgumentKeys.new("a\nb", options).cache_key).to eq('aNLb') + end + + # REMOVE_CHARACTERS uses [[:space:]], so both options must cover the same + # whitespace. A raw tab or newline in a key breaks the Memcached protocol. + it 'leaves no whitespace in the key under either option' do + argument = "a b\tc\nd\re\ff\vg" + + removed = ::ActiveRemote::Cached::ArgumentKeys.new( + argument, :active_remote_cached_remove_characters => true + ).cache_key + replaced = ::ActiveRemote::Cached::ArgumentKeys.new( + argument, :active_remote_cached_replace_characters => true + ).cache_key + + expect(removed).not_to match(/[[:space:]]/) + expect(replaced).not_to match(/[[:space:]]/) + end + it 'joins multiple arguments into one key' do expect(::ActiveRemote::Cached::ArgumentKeys.new('hello', 'world', {}).cache_key).to eq('helloworld') end From a78b43c9d747fbbc35969182bee556bd4c222a35 Mon Sep 17 00:00:00 2001 From: John Bolliger Date: Wed, 26 Aug 2026 23:42:11 -0600 Subject: [PATCH 10/10] Bump to 1.2.0 and document the upgrade The cache key format changed, so every existing entry becomes a miss. A dynamic finder called with too few arguments now raises ArgumentError rather than caching a record under a nil, and the cache provider validator raises InvalidCacheProvider rather than a bare RuntimeError. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 45 +++++++++++++++++++++++++++-- lib/active_remote/cached/version.rb | 2 +- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 24b32b6..06841aa 100644 --- a/README.md +++ b/README.md @@ -149,6 +149,44 @@ CI runs this matrix on Ruby 3.1, Ruby 3.4, JRuby 9.4, and JRuby 10.0. `active_remote` 8.0 requires Ruby 3.2 or later. CI does not run that version on Ruby 3.1 or JRuby 9.4. +## Upgrading to 1.2.0 + +### Every cache key changes + +Before 1.2.0 the cache key held only the argument values, joined with no +separator. Three different finders shared one cache entry: + +```ruby +Customer.cached_find_by_name_and_email("x", "y") # key: "xy" +Customer.cached_find_by_city_and_state("x", "y") # key: "xy" same entry +Customer.cached_find_by_id("xy") # key: "xy" same entry +``` + +The key now names each field, so each finder gets its own entry: + +```ruby +Customer.cached_find_by_name_and_email("x", "y") # key: "email.y/name.x" +``` + +Every existing cache entry becomes a miss after the upgrade. Expect one cold +period. The gem already causes this on an ActiveSupport upgrade, through +`RUBY_AND_ACTIVE_SUPPORT_VERSION`. + +### A bad call now raises + +A dynamic finder called with too few arguments used to pass `nil` for the +missing field and cache the result. It now raises `ArgumentError`: + +```ruby +Customer.cached_find_by_email_and_name("only_one") # => ArgumentError +``` + +### The cache provider validator raises a new class + +`ActiveRemote::Cached::Cache::InvalidCacheProvider` replaces the bare +`RuntimeError` that `ActiveRemote::Cached.cache` raised for a provider that is +missing a method. + ## Known behavior Two behaviors are recorded in the specs. Neither is fixed. Read @@ -161,9 +199,10 @@ method named `not_cached_find_by_guid` resolves to `cached_find_by_guid`. ### A subclass has its own empty cached_methods list -A subclass inherits the finder methods its parent defined. It does not inherit -the `cached_methods` list. The parent accepts the finder arguments in any -order. The subclass accepts them only in the order the method was defined. +A subclass inherits the finder methods its parent defined, and the options +those finders were declared with. It does not inherit the `cached_methods` +list. The parent accepts the finder arguments in any order. The subclass +accepts them only in the order the method was defined. ```ruby Parent.cached_find_by_beta_and_alpha('B', 'A') # works diff --git a/lib/active_remote/cached/version.rb b/lib/active_remote/cached/version.rb index e029360..5d5d5b4 100644 --- a/lib/active_remote/cached/version.rb +++ b/lib/active_remote/cached/version.rb @@ -2,6 +2,6 @@ module ActiveRemote module Cached - VERSION = '1.1.1' + VERSION = '1.2.0' end end