From f4d2c1c6090d79e5077ed63aa8fc2f56f2c58344 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sat, 22 Jul 2023 20:46:41 -0400 Subject: [PATCH 01/20] Add prefix to dalli cache Want to be able to cache in multi-tenant. --- lib/flipper/adapters/dalli.rb | 16 +++++++++++----- spec/flipper/adapters/dalli_spec.rb | 27 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/lib/flipper/adapters/dalli.rb b/lib/flipper/adapters/dalli.rb index 31f65d2e1..79bf184fb 100644 --- a/lib/flipper/adapters/dalli.rb +++ b/lib/flipper/adapters/dalli.rb @@ -17,17 +17,22 @@ class Dalli # Public: The ttl for all cached data. attr_reader :ttl + # Public: The prefix for all keys written to memcached. + attr_reader :prefix + # Public - def initialize(adapter, cache, ttl = 0) + def initialize(adapter, cache, ttl = 0, prefix: nil) @adapter = adapter @name = :dalli @cache = cache @ttl = ttl + @prefix = prefix @cache_version = 'v1'.freeze - @namespace = "flipper/#{@cache_version}".freeze - @features_key = "#{@namespace}/features".freeze - @get_all_key = "#{@namespace}/get_all".freeze + @namespace = "flipper/#{@cache_version}" + @namespace = @namespace.prepend(@prefix) if @prefix + @features_key = "#{@namespace}/features" + @get_all_key = "#{@namespace}/get_all" end # Public @@ -72,7 +77,8 @@ def get_all if @cache.add(@get_all_key, Time.now.to_i, @ttl) response = @adapter.get_all response.each do |key, value| - @cache.set(key_for(key), value, @ttl) + key = key_for(key) + @cache.set(key, value, @ttl) end @cache.set(@features_key, response.keys.to_set, @ttl) response diff --git a/spec/flipper/adapters/dalli_spec.rb b/spec/flipper/adapters/dalli_spec.rb index 812df06e7..ba0550e18 100644 --- a/spec/flipper/adapters/dalli_spec.rb +++ b/spec/flipper/adapters/dalli_spec.rb @@ -21,6 +21,33 @@ it_should_behave_like 'a flipper adapter' + context "when using a prefix" do + let(:adapter) { described_class.new(memory_adapter, cache, 0, prefix: "foo/") } + it_should_behave_like 'a flipper adapter' + + it "uses the prefix for all keys" do + # check individual feature get cached with prefix + adapter.get(flipper[:stats]) + expect(cache.get("foo/flipper/v1/feature/stats")).not_to be(nil) + + # check individual feature expired with prefix + adapter.remove(flipper[:stats]) + expect(cache.get("foo/flipper/v1/feature/stats")).to be(nil) + + # enable some stuff + flipper.enable_percentage_of_actors(:search, 10) + flipper.enable(:stats) + + # populate the cache + adapter.get_all + + # verify cached with prefix + expect(cache.get("foo/flipper/v1/get_all")).not_to be(nil) + expect(cache.get("foo/flipper/v1/feature/search")[:percentage_of_actors]).to eq("10") + expect(cache.get("foo/flipper/v1/feature/stats")[:boolean]).to eq("true") + end + end + describe '#remove' do it 'expires feature' do feature = flipper[:stats] From 13e67b045b187e9ed27f1672e301aab2a956655b Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 23 Jul 2023 15:18:57 -0400 Subject: [PATCH 02/20] Add prefix to active support cache --- .../adapters/active_support_cache_store.rb | 10 ++++--- .../active_support_cache_store_spec.rb | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index 424335769..b07f3fcb2 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -16,18 +16,20 @@ class ActiveSupportCacheStore attr_reader :name # Public - def initialize(adapter, cache, expires_in: nil, write_through: false) + def initialize(adapter, cache, expires_in: nil, write_through: false, prefix: nil) @adapter = adapter @name = :active_support_cache_store @cache = cache @write_options = {} @write_options[:expires_in] = expires_in if expires_in @write_through = write_through + @prefix = prefix @cache_version = 'v1'.freeze - @namespace = "flipper/#{@cache_version}".freeze - @features_key = "#{@namespace}/features".freeze - @get_all_key = "#{@namespace}/get_all".freeze + @namespace = "flipper/#{@cache_version}" + @namespace = @namespace.prepend(@prefix) if @prefix + @features_key = "#{@namespace}/features" + @get_all_key = "#{@namespace}/get_all" end # Public diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 21b94b938..3588e87b6 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -19,6 +19,33 @@ it_should_behave_like 'a flipper adapter' + context "when using a prefix" do + let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10.seconds, prefix: "foo/") } + it_should_behave_like 'a flipper adapter' + + it "uses the prefix for all keys" do + # check individual feature get cached with prefix + adapter.get(flipper[:stats]) + expect(cache.read("foo/flipper/v1/feature/stats")).not_to be(nil) + + # check individual feature expired with prefix + adapter.remove(flipper[:stats]) + expect(cache.read("foo/flipper/v1/feature/stats")).to be(nil) + + # enable some stuff + flipper.enable_percentage_of_actors(:search, 10) + flipper.enable(:stats) + + # populate the cache + adapter.get_all + + # verify cached with prefix + expect(cache.read("foo/flipper/v1/get_all")).not_to be(nil) + expect(cache.read("foo/flipper/v1/feature/search")[:percentage_of_actors]).to eq("10") + expect(cache.read("foo/flipper/v1/feature/stats")[:boolean]).to eq("true") + end + end + describe '#remove' do let(:feature) { flipper[:stats] } From e65c3dd42c9acd183d24e7e0cad68dd41364c4d6 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 23 Jul 2023 15:19:12 -0400 Subject: [PATCH 03/20] Add prefix to redis cache --- lib/flipper/adapters/redis_cache.rb | 10 +++++---- spec/flipper/adapters/redis_cache_spec.rb | 27 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index 0c359dbd1..f42230032 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -15,16 +15,18 @@ class RedisCache attr_reader :name # Public - def initialize(adapter, cache, ttl = 3600) + def initialize(adapter, cache, ttl = 3600, prefix: nil) @adapter = adapter @name = :redis_cache @cache = cache @ttl = ttl + @prefix = prefix @version = 'v1'.freeze - @namespace = "flipper/#{@version}".freeze - @features_key = "#{@namespace}/features".freeze - @get_all_key = "#{@namespace}/get_all".freeze + @namespace = "flipper/#{@version}" + @namespace = @namespace.prepend(@prefix) if @prefix + @features_key = "#{@namespace}/features" + @get_all_key = "#{@namespace}/get_all" end # Public diff --git a/spec/flipper/adapters/redis_cache_spec.rb b/spec/flipper/adapters/redis_cache_spec.rb index 5932c793e..c5e2f527b 100644 --- a/spec/flipper/adapters/redis_cache_spec.rb +++ b/spec/flipper/adapters/redis_cache_spec.rb @@ -24,6 +24,33 @@ it_should_behave_like 'a flipper adapter' + context "when using a prefix" do + let(:adapter) { described_class.new(memory_adapter, client, 3600, prefix: "foo/") } + it_should_behave_like 'a flipper adapter' + + it "uses the prefix for all keys" do + # check individual feature get cached with prefix + adapter.get(flipper[:stats]) + expect(Marshal.load(client.get("foo/flipper/v1/feature/stats"))).not_to be(nil) + + # check individual feature expired with prefix + adapter.remove(flipper[:stats]) + expect(client.get("foo/flipper/v1/feature/stats")).to be(nil) + + # enable some stuff + flipper.enable_percentage_of_actors(:search, 10) + flipper.enable(:stats) + + # populate the cache + adapter.get_all + + # verify cached with prefix + expect(client.get("foo/flipper/v1/get_all")).not_to be(nil) + expect(Marshal.load(client.get("foo/flipper/v1/feature/search"))[:percentage_of_actors]).to eq("10") + expect(Marshal.load(client.get("foo/flipper/v1/feature/stats"))[:boolean]).to eq("true") + end + end + describe '#remove' do it 'expires feature' do feature = flipper[:stats] From 4afbc4704255efb7ac6458f77b0e22e57868c273 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 23 Jul 2023 15:36:48 -0400 Subject: [PATCH 04/20] Expose adapter being cached in cache adapters --- lib/flipper/adapters/active_support_cache_store.rb | 3 +++ lib/flipper/adapters/dalli.rb | 3 +++ lib/flipper/adapters/redis_cache.rb | 3 +++ 3 files changed, 9 insertions(+) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index b07f3fcb2..ada1714a3 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -12,6 +12,9 @@ class ActiveSupportCacheStore # Internal attr_reader :cache + # Public: The adapter being cached. + attr_reader :adapter + # Public: The name of the adapter. attr_reader :name diff --git a/lib/flipper/adapters/dalli.rb b/lib/flipper/adapters/dalli.rb index 79bf184fb..097d63f01 100644 --- a/lib/flipper/adapters/dalli.rb +++ b/lib/flipper/adapters/dalli.rb @@ -11,6 +11,9 @@ class Dalli # Internal attr_reader :cache + # Public: The adapter being cached. + attr_reader :adapter + # Public: The name of the adapter. attr_reader :name diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index f42230032..211771080 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -11,6 +11,9 @@ class RedisCache # Internal attr_reader :cache + # Public: The adapter being cached. + attr_reader :adapter + # Public: The name of the adapter. attr_reader :name From 65291c83f927f2b42b383164bae9968e9afad51e Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Thu, 27 Jul 2023 16:17:36 -0400 Subject: [PATCH 05/20] Stop storing a get_all key It wouldn't even really work. The first to acquire lock would do the get_all but then the next would do a get_multi until the get_all was finished. Let's just do a features + get_multi all the time. Then the path is always the same. Also simplifies expiration and write through stuff. --- .../adapters/active_support_cache_store.rb | 85 ++++++++++--------- lib/flipper/adapters/dalli.rb | 74 ++++++++-------- lib/flipper/adapters/operation_logger.rb | 8 +- lib/flipper/adapters/redis_cache.rb | 72 +++++++++------- .../active_support_cache_store_spec.rb | 14 ++- spec/flipper/adapters/dalli_spec.rb | 13 ++- spec/flipper/adapters/redis_cache_spec.rb | 14 ++- 7 files changed, 156 insertions(+), 124 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index ada1714a3..edad1aded 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -5,34 +5,35 @@ module Flipper module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in ActiveSupport::ActiveSupportCacheStore caches. - # class ActiveSupportCacheStore include ::Flipper::Adapter - # Internal - attr_reader :cache - # Public: The adapter being cached. attr_reader :adapter # Public: The name of the adapter. attr_reader :name + # Public: The ActiveSupport::Cache::Store to cache with. + attr_reader :cache + + # Public: The number of seconds cached data should expire in. + attr_reader :expires_in + + alias_method :ttl, :expires_in + # Public def initialize(adapter, cache, expires_in: nil, write_through: false, prefix: nil) @adapter = adapter @name = :active_support_cache_store @cache = cache - @write_options = {} - @write_options[:expires_in] = expires_in if expires_in + @expires_in = expires_in @write_through = write_through - @prefix = prefix @cache_version = 'v1'.freeze @namespace = "flipper/#{@cache_version}" - @namespace = @namespace.prepend(@prefix) if @prefix - @features_key = "#{@namespace}/features" - @get_all_key = "#{@namespace}/get_all" + @namespace = @namespace.prepend(prefix) if prefix + @features_cache_key = "#{@namespace}/features" end # Public @@ -43,19 +44,19 @@ def features # Public def add(feature) result = @adapter.add(feature) - @cache.delete(@features_key) + expire_features_cache result end ## Public def remove(feature) result = @adapter.remove(feature) - @cache.delete(@features_key) + expire_features_cache if @write_through - @cache.write(key_for(feature.key), default_config, @write_options) + @cache.write(feature_cache_key(feature.key), default_config, expires_in: @expires_in) else - @cache.delete(key_for(feature.key)) + expire_feature_cache(feature.key) end result @@ -64,13 +65,13 @@ def remove(feature) ## Public def clear(feature) result = @adapter.clear(feature) - @cache.delete(key_for(feature.key)) + expire_feature_cache(feature.key) result end ## Public def get(feature) - @cache.fetch(key_for(feature.key), @write_options) do + @cache.fetch(feature_cache_key(feature.key), expires_in: @expires_in) do @adapter.get(feature) end end @@ -80,17 +81,8 @@ def get_multi(features) end def get_all - if @cache.write(@get_all_key, Time.now.to_i, @write_options.merge(unless_exist: true)) - response = @adapter.get_all - response.each do |key, value| - @cache.write(key_for(key), value, @write_options) - end - @cache.write(@features_key, response.keys.to_set, @write_options) - response - else - features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } - read_many_features(features) - end + features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } + read_many_features(features) end ## Public @@ -98,9 +90,9 @@ def enable(feature, gate, thing) result = @adapter.enable(feature, gate, thing) if @write_through - @cache.write(key_for(feature.key), @adapter.get(feature), @write_options) + @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @expires_in) else - @cache.delete(key_for(feature.key)) + expire_feature_cache(feature.key) end result @@ -111,43 +103,56 @@ def disable(feature, gate, thing) result = @adapter.disable(feature, gate, thing) if @write_through - @cache.write(key_for(feature.key), @adapter.get(feature), @write_options) + @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @expires_in) else - @cache.delete(key_for(feature.key)) + expire_feature_cache(feature.key) end result end - private - - def key_for(key) + # Public: Generate the cache key for a given feature. + # + # key - The String or Symbol feature key. + def feature_cache_key(key) "#{@namespace}/feature/#{key}" end + # Public: Expire the cache for the set of known feature names. + def expire_features_cache + @cache.delete(@features_cache_key) + end + + # Public: Expire the cache for a given feature. + def expire_feature_cache(key) + @cache.delete(feature_cache_key(key)) + end + + private + # Internal: Returns an array of the known feature keys. def read_feature_keys - @cache.fetch(@features_key, @write_options) { @adapter.features } + @cache.fetch(@features_cache_key, expires_in: @expires_in) { @adapter.features } end # Internal: Given an array of features, attempts to read through cache in # as few network calls as possible. def read_many_features(features) - keys = features.map { |feature| key_for(feature.key) } + keys = features.map { |feature| feature_cache_key(feature.key) } cache_result = @cache.read_multi(*keys) - uncached_features = features.reject { |feature| cache_result[key_for(feature)] } + uncached_features = features.reject { |feature| cache_result[feature_cache_key(feature)] } if uncached_features.any? response = @adapter.get_multi(uncached_features) response.each do |key, value| - @cache.write(key_for(key), value, @write_options) - cache_result[key_for(key)] = value + @cache.write(feature_cache_key(key), value, expires_in: @expires_in) + cache_result[feature_cache_key(key)] = value end end result = {} features.each do |feature| - result[feature.key] = cache_result[key_for(feature.key)] + result[feature.key] = cache_result[feature_cache_key(feature.key)] end result end diff --git a/lib/flipper/adapters/dalli.rb b/lib/flipper/adapters/dalli.rb index 097d63f01..75ffc100c 100644 --- a/lib/flipper/adapters/dalli.rb +++ b/lib/flipper/adapters/dalli.rb @@ -8,20 +8,19 @@ module Adapters class Dalli include ::Flipper::Adapter - # Internal - attr_reader :cache - # Public: The adapter being cached. attr_reader :adapter # Public: The name of the adapter. attr_reader :name + # Public: The Dalli::Client instance to cache with. + attr_reader :cache + # Public: The ttl for all cached data. attr_reader :ttl - # Public: The prefix for all keys written to memcached. - attr_reader :prefix + alias_method :expires_in, :ttl # Public def initialize(adapter, cache, ttl = 0, prefix: nil) @@ -29,13 +28,11 @@ def initialize(adapter, cache, ttl = 0, prefix: nil) @name = :dalli @cache = cache @ttl = ttl - @prefix = prefix @cache_version = 'v1'.freeze @namespace = "flipper/#{@cache_version}" - @namespace = @namespace.prepend(@prefix) if @prefix - @features_key = "#{@namespace}/features" - @get_all_key = "#{@namespace}/get_all" + @namespace = @namespace.prepend(prefix) if prefix + @features_cache_key = "#{@namespace}/features" end # Public @@ -46,28 +43,28 @@ def features # Public def add(feature) result = @adapter.add(feature) - @cache.delete(@features_key) + expire_features_cache result end # Public def remove(feature) result = @adapter.remove(feature) - @cache.delete(@features_key) - @cache.delete(key_for(feature.key)) + expire_features_cache + expire_feature_cache(feature.key) result end # Public def clear(feature) result = @adapter.clear(feature) - @cache.delete(key_for(feature.key)) + expire_feature_cache(feature.key) result end # Public def get(feature) - @cache.fetch(key_for(feature.key), @ttl) do + @cache.fetch(feature_cache_key(feature.key), @ttl) do @adapter.get(feature) end end @@ -77,62 +74,65 @@ def get_multi(features) end def get_all - if @cache.add(@get_all_key, Time.now.to_i, @ttl) - response = @adapter.get_all - response.each do |key, value| - key = key_for(key) - @cache.set(key, value, @ttl) - end - @cache.set(@features_key, response.keys.to_set, @ttl) - response - else - features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } - read_many_features(features) - end + features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } + read_many_features(features) end # Public def enable(feature, gate, thing) result = @adapter.enable(feature, gate, thing) - @cache.delete(key_for(feature.key)) + expire_feature_cache(feature.key) result end # Public def disable(feature, gate, thing) result = @adapter.disable(feature, gate, thing) - @cache.delete(key_for(feature.key)) + expire_feature_cache(feature.key) result end - private - - def key_for(key) + # Public: Generate the cache key for a given feature. + # + # key - The String or Symbol feature key. + def feature_cache_key(key) "#{@namespace}/feature/#{key}" end + # Public: Expire the cache for the set of known feature names. + def expire_features_cache + @cache.delete(@features_cache_key) + end + + # Public: Expire the cache for a given feature. + def expire_feature_cache(key) + @cache.delete(feature_cache_key(key)) + end + + private + def read_feature_keys - @cache.fetch(@features_key, @ttl) { @adapter.features } + @cache.fetch(@features_cache_key, @ttl) { @adapter.features } end # Internal: Given an array of features, attempts to read through cache in # as few network calls as possible. def read_many_features(features) - keys = features.map { |feature| key_for(feature.key) } + keys = features.map { |feature| feature_cache_key(feature.key) } cache_result = @cache.get_multi(keys) - uncached_features = features.reject { |feature| cache_result[key_for(feature.key)] } + uncached_features = features.reject { |feature| cache_result[feature_cache_key(feature.key)] } if uncached_features.any? response = @adapter.get_multi(uncached_features) response.each do |key, value| - @cache.set(key_for(key), value, @ttl) - cache_result[key_for(key)] = value + @cache.set(feature_cache_key(key), value, @ttl) + cache_result[feature_cache_key(key)] = value end end result = {} features.each do |feature| - result[feature.key] = cache_result[key_for(feature.key)] + result[feature.key] = cache_result[feature_cache_key(feature.key)] end result end diff --git a/lib/flipper/adapters/operation_logger.rb b/lib/flipper/adapters/operation_logger.rb index 7cd438c41..c8fca21c9 100644 --- a/lib/flipper/adapters/operation_logger.rb +++ b/lib/flipper/adapters/operation_logger.rb @@ -112,8 +112,12 @@ def export(format: :json, version: 1) end # Public: Count the number of times a certain operation happened. - def count(type) - type(type).size + def count(type = nil) + if type + type(type).size + else + @operations.size + end end # Public: Get all operations of a certain type. diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index 211771080..e75485d41 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -4,32 +4,35 @@ module Flipper module Adapters # Public: Adapter that wraps another adapter with the ability to cache - # adapter calls in Redis + # adapter calls in Redis. class RedisCache include ::Flipper::Adapter - # Internal - attr_reader :cache - # Public: The adapter being cached. attr_reader :adapter # Public: The name of the adapter. attr_reader :name + # Public: The Redis instance to cache with. + attr_reader :cache + + # Public: The ttl for all cached data. + attr_reader :ttl + + alias_method :expires_in, :ttl + # Public def initialize(adapter, cache, ttl = 3600, prefix: nil) @adapter = adapter @name = :redis_cache @cache = cache @ttl = ttl - @prefix = prefix @version = 'v1'.freeze @namespace = "flipper/#{@version}" - @namespace = @namespace.prepend(@prefix) if @prefix - @features_key = "#{@namespace}/features" - @get_all_key = "#{@namespace}/get_all" + @namespace = @namespace.prepend(prefix) if prefix + @features_cache_key = "#{@namespace}/features" end # Public @@ -40,28 +43,28 @@ def features # Public def add(feature) result = @adapter.add(feature) - @cache.del(@features_key) + expire_features_cache result end # Public def remove(feature) result = @adapter.remove(feature) - @cache.del(@features_key) - @cache.del(key_for(feature.key)) + expire_features_cache + expire_feature_cache(feature.key) result end # Public def clear(feature) result = @adapter.clear(feature) - @cache.del(key_for(feature.key)) + expire_feature_cache(feature.key) result end # Public def get(feature) - fetch(key_for(feature.key)) do + fetch(feature_cache_key(feature.key)) do @adapter.get(feature) end end @@ -71,42 +74,45 @@ def get_multi(features) end def get_all - if @cache.setnx(@get_all_key, Time.now.to_i) - @cache.expire(@get_all_key, @ttl) - response = @adapter.get_all - response.each do |key, value| - set_with_ttl key_for(key), value - end - set_with_ttl @features_key, response.keys.to_set - response - else - features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } - read_many_features(features) - end + features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } + read_many_features(features) end # Public def enable(feature, gate, thing) result = @adapter.enable(feature, gate, thing) - @cache.del(key_for(feature.key)) + expire_feature_cache(feature.key) result end # Public def disable(feature, gate, thing) result = @adapter.disable(feature, gate, thing) - @cache.del(key_for(feature.key)) + expire_feature_cache(feature.key) result end - private - - def key_for(key) + # Public: Generate the cache key for a given feature. + # + # key - The String or Symbol feature key. + def feature_cache_key(key) "#{@namespace}/feature/#{key}" end + # Public: Expire the cache for the set of known feature names. + def expire_features_cache + @cache.del(@features_cache_key) + end + + # Public: Expire the cache for a given feature. + def expire_feature_cache(key) + @cache.del(feature_cache_key(key)) + end + + private + def read_feature_keys - fetch(@features_key) { @adapter.features } + fetch(@features_cache_key) { @adapter.features } end def read_many_features(features) @@ -117,7 +123,7 @@ def read_many_features(features) if uncached_features.any? response = @adapter.get_multi(uncached_features) response.each do |key, value| - set_with_ttl(key_for(key), value) + set_with_ttl(feature_cache_key(key), value) cache_result[key] = value end end @@ -147,7 +153,7 @@ def set_with_ttl(key, value) def multi_cache_get(keys) return [] if keys.empty? - cache_keys = keys.map { |key| key_for(key) } + cache_keys = keys.map { |key| feature_cache_key(key) } @cache.mget(*cache_keys).map do |value| value ? Marshal.load(value) : nil end diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 3588e87b6..5e65082d1 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -19,6 +19,10 @@ it_should_behave_like 'a flipper adapter' + it "aliases expires_in to ttl" do + expect(adapter.ttl).to eq(adapter.expires_in) + end + context "when using a prefix" do let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10.seconds, prefix: "foo/") } it_should_behave_like 'a flipper adapter' @@ -40,7 +44,7 @@ adapter.get_all # verify cached with prefix - expect(cache.read("foo/flipper/v1/get_all")).not_to be(nil) + expect(cache.read("foo/flipper/v1/features")).to eq(Set["stats", "search"]) expect(cache.read("foo/flipper/v1/feature/search")[:percentage_of_actors]).to eq("10") expect(cache.read("foo/flipper/v1/feature/stats")[:boolean]).to eq("true") end @@ -157,17 +161,19 @@ adapter.get_all expect(cache.read("flipper/v1/feature/#{stats.key}")[:boolean]).to eq('true') expect(cache.read("flipper/v1/feature/#{search.key}")[:boolean]).to be(nil) - expect(cache.read("flipper/v1/get_all")).to be_within(2).of(Time.now.to_i) + expect(cache.read("flipper/v1/features")).to eq(Set["stats", "search"]) end it 'returns same result when already cached' do expect(adapter.get_all).to eq(adapter.get_all) end - it 'only invokes one call to wrapped adapter' do + it 'only invokes two calls to wrapped adapter (for features set and gate data for each feature in set)' do memory_adapter.reset 5.times { adapter.get_all } - expect(memory_adapter.count(:get_all)).to eq(1) + expect(memory_adapter.count(:features)).to eq(1) + expect(memory_adapter.count(:get_multi)).to eq(1) + expect(memory_adapter.count).to eq(2) end end diff --git a/spec/flipper/adapters/dalli_spec.rb b/spec/flipper/adapters/dalli_spec.rb index ba0550e18..1af22e419 100644 --- a/spec/flipper/adapters/dalli_spec.rb +++ b/spec/flipper/adapters/dalli_spec.rb @@ -21,6 +21,10 @@ it_should_behave_like 'a flipper adapter' + it "aliases ttl to expires_in" do + expect(adapter.expires_in).to eq(adapter.ttl) + end + context "when using a prefix" do let(:adapter) { described_class.new(memory_adapter, cache, 0, prefix: "foo/") } it_should_behave_like 'a flipper adapter' @@ -42,7 +46,6 @@ adapter.get_all # verify cached with prefix - expect(cache.get("foo/flipper/v1/get_all")).not_to be(nil) expect(cache.get("foo/flipper/v1/feature/search")[:percentage_of_actors]).to eq("10") expect(cache.get("foo/flipper/v1/feature/stats")[:boolean]).to eq("true") end @@ -95,17 +98,19 @@ adapter.get_all expect(cache.get("flipper/v1/feature/#{stats.key}")[:boolean]).to eq('true') expect(cache.get("flipper/v1/feature/#{search.key}")[:boolean]).to be(nil) - expect(cache.get("flipper/v1/get_all")).to be_within(2).of(Time.now.to_i) + expect(cache.get("flipper/v1/features")).to eq(Set["stats", "search"]) end it 'returns same result when already cached' do expect(adapter.get_all).to eq(adapter.get_all) end - it 'only invokes one call to wrapped adapter' do + it 'only invokes two calls to wrapped adapter (for features set and gate data for each feature in set)' do memory_adapter.reset 5.times { adapter.get_all } - expect(memory_adapter.count(:get_all)).to eq(1) + expect(memory_adapter.count(:features)).to eq(1) + expect(memory_adapter.count(:get_multi)).to eq(1) + expect(memory_adapter.count).to eq(2) end end diff --git a/spec/flipper/adapters/redis_cache_spec.rb b/spec/flipper/adapters/redis_cache_spec.rb index c5e2f527b..afe983d18 100644 --- a/spec/flipper/adapters/redis_cache_spec.rb +++ b/spec/flipper/adapters/redis_cache_spec.rb @@ -24,6 +24,10 @@ it_should_behave_like 'a flipper adapter' + it "aliases ttl to expires_in" do + expect(adapter.expires_in).to eq(adapter.ttl) + end + context "when using a prefix" do let(:adapter) { described_class.new(memory_adapter, client, 3600, prefix: "foo/") } it_should_behave_like 'a flipper adapter' @@ -45,7 +49,7 @@ adapter.get_all # verify cached with prefix - expect(client.get("foo/flipper/v1/get_all")).not_to be(nil) + expect(Marshal.load(client.get("foo/flipper/v1/features"))).to eq(Set["stats", "search"]) expect(Marshal.load(client.get("foo/flipper/v1/feature/search"))[:percentage_of_actors]).to eq("10") expect(Marshal.load(client.get("foo/flipper/v1/feature/stats"))[:boolean]).to eq("true") end @@ -109,17 +113,19 @@ adapter.get_all expect(Marshal.load(client.get("flipper/v1/feature/#{stats.key}"))[:boolean]).to eq('true') expect(Marshal.load(client.get("flipper/v1/feature/#{search.key}"))[:boolean]).to be(nil) - expect(client.get("flipper/v1/get_all").to_i).to be_within(2).of(Time.now.to_i) + expect(Marshal.load(client.get("flipper/v1/features"))).to eq(Set["stats", "search"]) end it 'returns same result when already cached' do expect(adapter.get_all).to eq(adapter.get_all) end - it 'only invokes one call to wrapped adapter' do + it 'only invokes two calls to wrapped adapter (for features set and gate data for each feature in set)' do memory_adapter.reset 5.times { adapter.get_all } - expect(memory_adapter.count(:get_all)).to eq(1) + expect(memory_adapter.count(:features)).to eq(1) + expect(memory_adapter.count(:get_multi)).to eq(1) + expect(memory_adapter.count).to eq(2) end end From 082f011fe439cc0a25577b0f5cfe28cda25e7476 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Thu, 27 Jul 2023 17:00:51 -0400 Subject: [PATCH 06/20] Remove some duplication in cache adapters Not perfect but less tweaking of each of them now than before and they are more similar. --- .../adapters/active_support_cache_store.rb | 113 +++++------------- lib/flipper/adapters/cache_base.rb | 94 +++++++++++++++ lib/flipper/adapters/dalli.rb | 104 ++-------------- lib/flipper/adapters/redis_cache.rb | 102 ++-------------- .../active_support_cache_store_spec.rb | 32 ++++- spec/flipper/adapters/dalli_spec.rb | 34 +++++- spec/flipper/adapters/redis_cache_spec.rb | 34 +++++- 7 files changed, 238 insertions(+), 275 deletions(-) create mode 100644 lib/flipper/adapters/cache_base.rb diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index edad1aded..14ae66c28 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -1,114 +1,49 @@ require 'flipper' +require 'flipper/adapters/cache_base' require 'active_support/notifications' module Flipper module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in ActiveSupport::ActiveSupportCacheStore caches. - class ActiveSupportCacheStore + class ActiveSupportCacheStore < CacheBase include ::Flipper::Adapter - # Public: The adapter being cached. - attr_reader :adapter - - # Public: The name of the adapter. - attr_reader :name - - # Public: The ActiveSupport::Cache::Store to cache with. - attr_reader :cache - - # Public: The number of seconds cached data should expire in. - attr_reader :expires_in - - alias_method :ttl, :expires_in - - # Public - def initialize(adapter, cache, expires_in: nil, write_through: false, prefix: nil) - @adapter = adapter + def initialize(adapter, cache, expires_in: 300, write_through: false, prefix: nil) + super(adapter, cache, expires_in, prefix: prefix) @name = :active_support_cache_store - @cache = cache - @expires_in = expires_in @write_through = write_through - - @cache_version = 'v1'.freeze - @namespace = "flipper/#{@cache_version}" - @namespace = @namespace.prepend(prefix) if prefix - @features_cache_key = "#{@namespace}/features" - end - - # Public - def features - read_feature_keys - end - - # Public - def add(feature) - result = @adapter.add(feature) - expire_features_cache - result end - ## Public def remove(feature) - result = @adapter.remove(feature) - expire_features_cache - if @write_through - @cache.write(feature_cache_key(feature.key), default_config, expires_in: @expires_in) + result = @adapter.remove(feature) + expire_features_cache + @cache.write(feature_cache_key(feature.key), default_config, expires_in: @ttl) + result else - expire_feature_cache(feature.key) + super end - - result end - ## Public - def clear(feature) - result = @adapter.clear(feature) - expire_feature_cache(feature.key) - result - end - - ## Public - def get(feature) - @cache.fetch(feature_cache_key(feature.key), expires_in: @expires_in) do - @adapter.get(feature) - end - end - - def get_multi(features) - read_many_features(features) - end - - def get_all - features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } - read_many_features(features) - end - - ## Public def enable(feature, gate, thing) - result = @adapter.enable(feature, gate, thing) - if @write_through - @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @expires_in) + result = @adapter.enable(feature, gate, thing) + @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @ttl) + result else - expire_feature_cache(feature.key) + super end - - result end - ## Public def disable(feature, gate, thing) - result = @adapter.disable(feature, gate, thing) - if @write_through - @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @expires_in) + result = @adapter.disable(feature, gate, thing) + @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @ttl) + result else - expire_feature_cache(feature.key) + super end - - result end # Public: Generate the cache key for a given feature. @@ -130,12 +65,12 @@ def expire_feature_cache(key) private - # Internal: Returns an array of the known feature keys. + # Private: Returns the Set of known feature keys. def read_feature_keys - @cache.fetch(@features_cache_key, expires_in: @expires_in) { @adapter.features } + @cache.fetch(@features_cache_key, expires_in: @ttl) { @adapter.features } end - # Internal: Given an array of features, attempts to read through cache in + # Private: Given an array of features, attempts to read through cache in # as few network calls as possible. def read_many_features(features) keys = features.map { |feature| feature_cache_key(feature.key) } @@ -145,7 +80,7 @@ def read_many_features(features) if uncached_features.any? response = @adapter.get_multi(uncached_features) response.each do |key, value| - @cache.write(feature_cache_key(key), value, expires_in: @expires_in) + @cache.write(feature_cache_key(key), value, expires_in: @ttl) cache_result[feature_cache_key(key)] = value end end @@ -156,6 +91,12 @@ def read_many_features(features) end result end + + def read_feature(feature) + @cache.fetch(feature_cache_key(feature.key), expires_in: @ttl) do + @adapter.get(feature) + end + end end end end diff --git a/lib/flipper/adapters/cache_base.rb b/lib/flipper/adapters/cache_base.rb new file mode 100644 index 000000000..46e5bb837 --- /dev/null +++ b/lib/flipper/adapters/cache_base.rb @@ -0,0 +1,94 @@ +module Flipper + module Adapters + class CacheBase + include ::Flipper::Adapter + + # Public: The adapter being cached. + attr_reader :adapter + + # Public: The name of the adapter. + attr_reader :name + + # Public: The ActiveSupport::Cache::Store to cache with. + attr_reader :cache + + # Public: The ttl for all cached data. + attr_reader :ttl + + def initialize(adapter, cache, ttl = 300, prefix: nil) + @adapter = adapter + @cache = cache + @ttl = ttl + + @cache_version = 'v1'.freeze + @namespace = "flipper/#{@cache_version}" + @namespace = @namespace.prepend(prefix) if prefix + @features_cache_key = "#{@namespace}/features" + end + + # Public + def features + read_feature_keys + end + + # Public + def add(feature) + result = @adapter.add(feature) + expire_features_cache + result + end + + # Public + def remove(feature) + result = @adapter.remove(feature) + expire_features_cache + expire_feature_cache(feature.key) + result + end + + # Public + def clear(feature) + result = @adapter.clear(feature) + expire_feature_cache(feature.key) + result + end + + # Public + def get(feature) + read_feature(feature) + end + + # Public + def get_multi(features) + read_many_features(features) + end + + # Public + def get_all + features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } + read_many_features(features) + end + + # Public + def enable(feature, gate, thing) + result = @adapter.enable(feature, gate, thing) + expire_feature_cache(feature.key) + result + end + + # Public + def disable(feature, gate, thing) + result = @adapter.disable(feature, gate, thing) + expire_feature_cache(feature.key) + result + end + + # Public: Generate the cache key for a given feature. + # + # key - The String or Symbol feature key. + def feature_cache_key(key) + "#{@namespace}/feature/#{key}" + end + end + end +end diff --git a/lib/flipper/adapters/dalli.rb b/lib/flipper/adapters/dalli.rb index 75ffc100c..a32698561 100644 --- a/lib/flipper/adapters/dalli.rb +++ b/lib/flipper/adapters/dalli.rb @@ -1,102 +1,15 @@ require 'dalli' require 'flipper' +require 'flipper/adapters/cache_base' module Flipper module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Memcached using the Dalli gem. - class Dalli - include ::Flipper::Adapter - - # Public: The adapter being cached. - attr_reader :adapter - - # Public: The name of the adapter. - attr_reader :name - - # Public: The Dalli::Client instance to cache with. - attr_reader :cache - - # Public: The ttl for all cached data. - attr_reader :ttl - - alias_method :expires_in, :ttl - - # Public - def initialize(adapter, cache, ttl = 0, prefix: nil) - @adapter = adapter + class Dalli < CacheBase + def initialize(adapter, cache, ttl = 300, prefix: nil) + super @name = :dalli - @cache = cache - @ttl = ttl - - @cache_version = 'v1'.freeze - @namespace = "flipper/#{@cache_version}" - @namespace = @namespace.prepend(prefix) if prefix - @features_cache_key = "#{@namespace}/features" - end - - # Public - def features - read_feature_keys - end - - # Public - def add(feature) - result = @adapter.add(feature) - expire_features_cache - result - end - - # Public - def remove(feature) - result = @adapter.remove(feature) - expire_features_cache - expire_feature_cache(feature.key) - result - end - - # Public - def clear(feature) - result = @adapter.clear(feature) - expire_feature_cache(feature.key) - result - end - - # Public - def get(feature) - @cache.fetch(feature_cache_key(feature.key), @ttl) do - @adapter.get(feature) - end - end - - def get_multi(features) - read_many_features(features) - end - - def get_all - features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } - read_many_features(features) - end - - # Public - def enable(feature, gate, thing) - result = @adapter.enable(feature, gate, thing) - expire_feature_cache(feature.key) - result - end - - # Public - def disable(feature, gate, thing) - result = @adapter.disable(feature, gate, thing) - expire_feature_cache(feature.key) - result - end - - # Public: Generate the cache key for a given feature. - # - # key - The String or Symbol feature key. - def feature_cache_key(key) - "#{@namespace}/feature/#{key}" end # Public: Expire the cache for the set of known feature names. @@ -111,11 +24,12 @@ def expire_feature_cache(key) private + # Private: Returns the Set of known feature keys. def read_feature_keys @cache.fetch(@features_cache_key, @ttl) { @adapter.features } end - # Internal: Given an array of features, attempts to read through cache in + # Private: Given an array of features, attempts to read through cache in # as few network calls as possible. def read_many_features(features) keys = features.map { |feature| feature_cache_key(feature.key) } @@ -136,6 +50,12 @@ def read_many_features(features) end result end + + def read_feature(feature) + @cache.fetch(feature_cache_key(feature.key), @ttl) do + @adapter.get(feature) + end + end end end end diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index e75485d41..f4c92d2cd 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -1,102 +1,17 @@ require 'redis' require 'flipper' +require 'flipper/adapters/cache_base' module Flipper module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Redis. - class RedisCache - include ::Flipper::Adapter - - # Public: The adapter being cached. - attr_reader :adapter - - # Public: The name of the adapter. - attr_reader :name - - # Public: The Redis instance to cache with. - attr_reader :cache - - # Public: The ttl for all cached data. - attr_reader :ttl - + class RedisCache < CacheBase alias_method :expires_in, :ttl - # Public - def initialize(adapter, cache, ttl = 3600, prefix: nil) - @adapter = adapter + def initialize(adapter, cache, ttl = 300, prefix: nil) + super @name = :redis_cache - @cache = cache - @ttl = ttl - - @version = 'v1'.freeze - @namespace = "flipper/#{@version}" - @namespace = @namespace.prepend(prefix) if prefix - @features_cache_key = "#{@namespace}/features" - end - - # Public - def features - read_feature_keys - end - - # Public - def add(feature) - result = @adapter.add(feature) - expire_features_cache - result - end - - # Public - def remove(feature) - result = @adapter.remove(feature) - expire_features_cache - expire_feature_cache(feature.key) - result - end - - # Public - def clear(feature) - result = @adapter.clear(feature) - expire_feature_cache(feature.key) - result - end - - # Public - def get(feature) - fetch(feature_cache_key(feature.key)) do - @adapter.get(feature) - end - end - - def get_multi(features) - read_many_features(features) - end - - def get_all - features = read_feature_keys.map { |key| Flipper::Feature.new(key, self) } - read_many_features(features) - end - - # Public - def enable(feature, gate, thing) - result = @adapter.enable(feature, gate, thing) - expire_feature_cache(feature.key) - result - end - - # Public - def disable(feature, gate, thing) - result = @adapter.disable(feature, gate, thing) - expire_feature_cache(feature.key) - result - end - - # Public: Generate the cache key for a given feature. - # - # key - The String or Symbol feature key. - def feature_cache_key(key) - "#{@namespace}/feature/#{key}" end # Public: Expire the cache for the set of known feature names. @@ -111,10 +26,13 @@ def expire_feature_cache(key) private + # Private: Returns the Set of known feature keys. def read_feature_keys fetch(@features_cache_key) { @adapter.features } end + # Private: Given an array of features, attempts to read through cache in + # as few network calls as possible. def read_many_features(features) keys = features.map(&:key) cache_result = Hash[keys.zip(multi_cache_get(keys))] @@ -135,6 +53,12 @@ def read_many_features(features) result end + def read_feature(feature) + fetch(feature_cache_key(feature.key)) do + @adapter.get(feature) + end + end + def fetch(cache_key) cached = @cache.get(cache_key) if cached diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 5e65082d1..2ef96329c 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -19,14 +19,42 @@ it_should_behave_like 'a flipper adapter' - it "aliases expires_in to ttl" do - expect(adapter.ttl).to eq(adapter.expires_in) + it "knows ttl" do + expect(adapter.ttl).to eq(10.seconds) + end + + it "can expire features cache" do + # cache the features + adapter.features + expect(cache.read("flipper/v1/features")).not_to be(nil) + + # expire cache + adapter.expire_features_cache + expect(cache.read("flipper/v1/features")).to be(nil) + end + + it "can expire feature cache" do + # cache the features + adapter.get(flipper[:stats]) + expect(cache.read("flipper/v1/feature/stats")).not_to be(nil) + + # expire cache + adapter.expire_feature_cache("stats") + expect(cache.read("flipper/v1/feature/stats")).to be(nil) + end + + it "can generate feature cache key" do + expect(adapter.feature_cache_key("stats")).to eq("flipper/v1/feature/stats") end context "when using a prefix" do let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10.seconds, prefix: "foo/") } it_should_behave_like 'a flipper adapter' + it "can generate feature cache key" do + expect(adapter.feature_cache_key("stats")).to eq("foo/flipper/v1/feature/stats") + end + it "uses the prefix for all keys" do # check individual feature get cached with prefix adapter.get(flipper[:stats]) diff --git a/spec/flipper/adapters/dalli_spec.rb b/spec/flipper/adapters/dalli_spec.rb index 1af22e419..be9fdfcea 100644 --- a/spec/flipper/adapters/dalli_spec.rb +++ b/spec/flipper/adapters/dalli_spec.rb @@ -7,7 +7,7 @@ Flipper::Adapters::OperationLogger.new(Flipper::Adapters::Memory.new) end let(:cache) { Dalli::Client.new(ENV['MEMCACHED_URL']) } - let(:adapter) { described_class.new(memory_adapter, cache) } + let(:adapter) { described_class.new(memory_adapter, cache, 10) } let(:flipper) { Flipper.new(adapter) } subject { adapter } @@ -21,14 +21,42 @@ it_should_behave_like 'a flipper adapter' - it "aliases ttl to expires_in" do - expect(adapter.expires_in).to eq(adapter.ttl) + it "knows ttl" do + expect(adapter.ttl).to eq(10) + end + + it "can expire features cache" do + # cache the features + adapter.features + expect(cache.get("flipper/v1/features")).not_to be(nil) + + # expire cache + adapter.expire_features_cache + expect(cache.get("flipper/v1/features")).to be(nil) + end + + it "can expire feature cache" do + # cache the features + adapter.get(flipper[:stats]) + expect(cache.get("flipper/v1/feature/stats")).not_to be(nil) + + # expire cache + adapter.expire_feature_cache("stats") + expect(cache.get("flipper/v1/feature/stats")).to be(nil) + end + + it "can generate feature cache key" do + expect(adapter.feature_cache_key("stats")).to eq("flipper/v1/feature/stats") end context "when using a prefix" do let(:adapter) { described_class.new(memory_adapter, cache, 0, prefix: "foo/") } it_should_behave_like 'a flipper adapter' + it "can generate feature cache key" do + expect(adapter.feature_cache_key("stats")).to eq("foo/flipper/v1/feature/stats") + end + it "uses the prefix for all keys" do # check individual feature get cached with prefix adapter.get(flipper[:stats]) diff --git a/spec/flipper/adapters/redis_cache_spec.rb b/spec/flipper/adapters/redis_cache_spec.rb index afe983d18..56f7903e1 100644 --- a/spec/flipper/adapters/redis_cache_spec.rb +++ b/spec/flipper/adapters/redis_cache_spec.rb @@ -11,7 +11,7 @@ let(:memory_adapter) do Flipper::Adapters::OperationLogger.new(Flipper::Adapters::Memory.new) end - let(:adapter) { described_class.new(memory_adapter, client) } + let(:adapter) { described_class.new(memory_adapter, client, 10) } let(:flipper) { Flipper.new(adapter) } subject { adapter } @@ -24,14 +24,42 @@ it_should_behave_like 'a flipper adapter' - it "aliases ttl to expires_in" do - expect(adapter.expires_in).to eq(adapter.ttl) + it "knows ttl" do + expect(adapter.ttl).to eq(10) + end + + it "can expire features cache" do + # cache the features + adapter.features + expect(client.get("flipper/v1/features")).not_to be(nil) + + # expire cache + adapter.expire_features_cache + expect(client.get("flipper/v1/features")).to be(nil) + end + + it "can expire feature cache" do + # cache the features + adapter.get(flipper[:stats]) + expect(client.get("flipper/v1/feature/stats")).not_to be(nil) + + # expire cache + adapter.expire_feature_cache("stats") + expect(client.get("flipper/v1/feature/stats")).to be(nil) + end + + it "can generate feature cache key" do + expect(adapter.feature_cache_key("stats")).to eq("flipper/v1/feature/stats") end context "when using a prefix" do let(:adapter) { described_class.new(memory_adapter, client, 3600, prefix: "foo/") } it_should_behave_like 'a flipper adapter' + it "can generate feature cache key" do + expect(adapter.feature_cache_key("stats")).to eq("foo/flipper/v1/feature/stats") + end + it "uses the prefix for all keys" do # check individual feature get cached with prefix adapter.get(flipper[:stats]) From 3953e5f55cfedf3f83835a1fb61d9687bb3c984c Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Fri, 28 Jul 2023 15:33:56 -0400 Subject: [PATCH 07/20] Make features_cache_key available if people need it --- lib/flipper/adapters/cache_base.rb | 3 +++ spec/flipper/adapters/active_support_cache_store_spec.rb | 8 ++++++++ spec/flipper/adapters/dalli_spec.rb | 8 ++++++++ spec/flipper/adapters/redis_cache_spec.rb | 8 ++++++++ 4 files changed, 27 insertions(+) diff --git a/lib/flipper/adapters/cache_base.rb b/lib/flipper/adapters/cache_base.rb index 46e5bb837..5a3682e15 100644 --- a/lib/flipper/adapters/cache_base.rb +++ b/lib/flipper/adapters/cache_base.rb @@ -15,6 +15,9 @@ class CacheBase # Public: The ttl for all cached data. attr_reader :ttl + # Public: The cache key where the set of known features is cached. + attr_reader :features_cache_key + def initialize(adapter, cache, ttl = 300, prefix: nil) @adapter = adapter @cache = cache diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 2ef96329c..faf5e9b37 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -23,6 +23,10 @@ expect(adapter.ttl).to eq(10.seconds) end + it "knows features_cache_key" do + expect(adapter.features_cache_key).to eq("flipper/v1/features") + end + it "can expire features cache" do # cache the features adapter.features @@ -51,6 +55,10 @@ let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10.seconds, prefix: "foo/") } it_should_behave_like 'a flipper adapter' + it "knows features_cache_key" do + expect(adapter.features_cache_key).to eq("foo/flipper/v1/features") + end + it "can generate feature cache key" do expect(adapter.feature_cache_key("stats")).to eq("foo/flipper/v1/feature/stats") end diff --git a/spec/flipper/adapters/dalli_spec.rb b/spec/flipper/adapters/dalli_spec.rb index be9fdfcea..59e02968c 100644 --- a/spec/flipper/adapters/dalli_spec.rb +++ b/spec/flipper/adapters/dalli_spec.rb @@ -25,6 +25,10 @@ expect(adapter.ttl).to eq(10) end + it "knows features_cache_key" do + expect(adapter.features_cache_key).to eq("flipper/v1/features") + end + it "can expire features cache" do # cache the features adapter.features @@ -53,6 +57,10 @@ let(:adapter) { described_class.new(memory_adapter, cache, 0, prefix: "foo/") } it_should_behave_like 'a flipper adapter' + it "knows features_cache_key" do + expect(adapter.features_cache_key).to eq("foo/flipper/v1/features") + end + it "can generate feature cache key" do expect(adapter.feature_cache_key("stats")).to eq("foo/flipper/v1/feature/stats") end diff --git a/spec/flipper/adapters/redis_cache_spec.rb b/spec/flipper/adapters/redis_cache_spec.rb index 56f7903e1..b25c92eb4 100644 --- a/spec/flipper/adapters/redis_cache_spec.rb +++ b/spec/flipper/adapters/redis_cache_spec.rb @@ -28,6 +28,10 @@ expect(adapter.ttl).to eq(10) end + it "knows features_cache_key" do + expect(adapter.features_cache_key).to eq("flipper/v1/features") + end + it "can expire features cache" do # cache the features adapter.features @@ -56,6 +60,10 @@ let(:adapter) { described_class.new(memory_adapter, client, 3600, prefix: "foo/") } it_should_behave_like 'a flipper adapter' + it "knows features_cache_key" do + expect(adapter.features_cache_key).to eq("foo/flipper/v1/features") + end + it "can generate feature cache key" do expect(adapter.feature_cache_key("stats")).to eq("foo/flipper/v1/feature/stats") end From 1314af1f9cb24c0ce7816b6a048c9c16c38e7eed Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 14:51:16 -0500 Subject: [PATCH 08/20] Fix cache specs --- lib/flipper/adapters/cache_base.rb | 3 --- spec/flipper/adapters/active_support_cache_store_spec.rb | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/flipper/adapters/cache_base.rb b/lib/flipper/adapters/cache_base.rb index 5a3682e15..6404ce62b 100644 --- a/lib/flipper/adapters/cache_base.rb +++ b/lib/flipper/adapters/cache_base.rb @@ -6,9 +6,6 @@ class CacheBase # Public: The adapter being cached. attr_reader :adapter - # Public: The name of the adapter. - attr_reader :name - # Public: The ActiveSupport::Cache::Store to cache with. attr_reader :cache diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 58591f02f..6ce952bee 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -8,7 +8,7 @@ end let(:cache) { ActiveSupport::Cache::MemoryStore.new } let(:write_through) { false } - let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10.seconds, write_through: write_through) } + let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10, write_through: write_through) } let(:flipper) { Flipper.new(adapter) } subject { adapter } @@ -20,7 +20,7 @@ it_should_behave_like 'a flipper adapter' it "knows ttl" do - expect(adapter.ttl).to eq(10.seconds) + expect(adapter.ttl).to eq(10) end it "knows features_cache_key" do @@ -52,7 +52,7 @@ end context "when using a prefix" do - let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10.seconds, prefix: "foo/") } + let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10, prefix: "foo/") } it_should_behave_like 'a flipper adapter' it "knows features_cache_key" do From 6764d715fd009c126725282f8e6911d7db0fc5fe Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 14:57:30 -0500 Subject: [PATCH 09/20] Fix memoizer spec for cached eager loading --- spec/flipper/middleware/memoizer_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/flipper/middleware/memoizer_spec.rb b/spec/flipper/middleware/memoizer_spec.rb index 9736f942f..8b2d21c5a 100644 --- a/spec/flipper/middleware/memoizer_spec.rb +++ b/spec/flipper/middleware/memoizer_spec.rb @@ -471,15 +471,18 @@ def get(uri, params = {}, env = {}, &block) get '/', {}, 'flipper' => flipper expect(logged_cached.count(:get_all)).to be(1) - expect(logged_memory.count(:get_all)).to be(1) + expect(logged_memory.count(:features)).to be(1) + expect(logged_memory.count(:get_multi)).to be(1) get '/', {}, 'flipper' => flipper expect(logged_cached.count(:get_all)).to be(2) - expect(logged_memory.count(:get_all)).to be(1) + expect(logged_memory.count(:features)).to be(1) + expect(logged_memory.count(:get_multi)).to be(1) get '/', {}, 'flipper' => flipper expect(logged_cached.count(:get_all)).to be(3) - expect(logged_memory.count(:get_all)).to be(1) + expect(logged_memory.count(:features)).to be(1) + expect(logged_memory.count(:get_multi)).to be(1) end end end From a57ccf779eb8e80afb3d7b541e1cd3d5efce627f Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:05:58 -0500 Subject: [PATCH 10/20] Run all cache changes when base changes --- Guardfile | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Guardfile b/Guardfile index 7d5f1c9f1..e58b005cc 100644 --- a/Guardfile +++ b/Guardfile @@ -22,6 +22,13 @@ guard 'rspec', rspec_options do watch(/shared_adapter_specs\.rb$/) { 'spec' } watch('spec/helper.rb') { 'spec' } watch('lib/flipper/adapters/http/client.rb') { 'spec/flipper/adapters/http_spec.rb' } + watch('lib/flipper/adapters/cache_base.rb') { + [ + 'spec/flipper/adapters/redis_cache_spec.rb', + 'spec/flipper/adapters/dalli_cache_spec.rb', + 'spec/flipper/adapters/active_support_cache_store_spec.rb', + ] + } # To run all specs on every change... (useful with focus and fit) # watch(%r{.*}) { 'spec' } From 3eef02dda10a175de6d976e703b131ef8a83078a Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:06:09 -0500 Subject: [PATCH 11/20] Upper case constants --- lib/flipper/adapters/read_only.rb | 4 ++-- lib/flipper/adapters/wrapper.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/flipper/adapters/read_only.rb b/lib/flipper/adapters/read_only.rb index 7fd6b812e..666e25a96 100644 --- a/lib/flipper/adapters/read_only.rb +++ b/lib/flipper/adapters/read_only.rb @@ -4,7 +4,7 @@ module Flipper module Adapters # Public: Adapter that wraps another adapter and raises for any writes. class ReadOnly < Wrapper - WriteMethods = %i[add remove clear enable disable] + WRITE_METHODS = %i[add remove clear enable disable] class WriteAttempted < Error def initialize(message = nil) @@ -19,7 +19,7 @@ def read_only? private def wrap(method, *args, **kwargs) - raise WriteAttempted if WriteMethods.include?(method) + raise WriteAttempted if WRITE_METHODS.include?(method) yield end diff --git a/lib/flipper/adapters/wrapper.rb b/lib/flipper/adapters/wrapper.rb index 13ee30100..6026ebc96 100644 --- a/lib/flipper/adapters/wrapper.rb +++ b/lib/flipper/adapters/wrapper.rb @@ -6,7 +6,7 @@ module Adapters class Wrapper include Flipper::Adapter - Methods = [ + METHODS = [ :import, :export, :features, @@ -26,7 +26,7 @@ def initialize(adapter) @adapter = adapter end - Methods.each do |method| + METHODS.each do |method| if RUBY_VERSION >= '3.0' define_method(method) do |*args, **kwargs| wrap(method, *args, **kwargs) { @adapter.public_send(method, *args, **kwargs) } From 0bf92860ff9f0a66e0f51c041148f94e1ec4561e Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:10:50 -0500 Subject: [PATCH 12/20] Remove duplicate method --- lib/flipper/adapters/active_support_cache_store.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index 3a714a0a7..f9d9fae14 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -45,13 +45,6 @@ def disable(feature, gate, thing) end end - # Public: Generate the cache key for a given feature. - # - # key - The String or Symbol feature key. - def feature_cache_key(key) - "#{@namespace}/feature/#{key}" - end - # Public: Expire the cache for the set of known feature names. def expire_features_cache @cache.delete(@features_cache_key) From 942525d795573cd884a322f70fb58a3583448b40 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:11:25 -0500 Subject: [PATCH 13/20] Move methods to top like other cache adapters --- .../adapters/active_support_cache_store.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index f9d9fae14..bd8ae6ff2 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -14,6 +14,16 @@ def initialize(adapter, cache, expires_in: 300, write_through: false, prefix: ni @write_through = write_through end + # Public: Expire the cache for the set of known feature names. + def expire_features_cache + @cache.delete(@features_cache_key) + end + + # Public: Expire the cache for a given feature. + def expire_feature_cache(key) + @cache.delete(feature_cache_key(key)) + end + def remove(feature) if @write_through result = @adapter.remove(feature) @@ -45,16 +55,6 @@ def disable(feature, gate, thing) end end - # Public: Expire the cache for the set of known feature names. - def expire_features_cache - @cache.delete(@features_cache_key) - end - - # Public: Expire the cache for a given feature. - def expire_feature_cache(key) - @cache.delete(feature_cache_key(key)) - end - private # Private: Returns the Set of known feature keys. From 396e05a2396566acde1ca680cdf7a13fb5ae50e2 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:17:53 -0500 Subject: [PATCH 14/20] Remove duplicate include --- lib/flipper/adapters/active_support_cache_store.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index bd8ae6ff2..7a5318081 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -7,8 +7,6 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in ActiveSupport::ActiveSupportCacheStore caches. class ActiveSupportCacheStore < CacheBase - include ::Flipper::Adapter - def initialize(adapter, cache, expires_in: 300, write_through: false, prefix: nil) super(adapter, cache, expires_in, prefix: prefix) @write_through = write_through From 51a13345e9178e443b24b96f1b26eb5da762e224 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:18:23 -0500 Subject: [PATCH 15/20] Add comments and alias expires in for backwards compatibility --- lib/flipper/adapters/cache_base.rb | 6 ++++++ lib/flipper/adapters/redis_cache.rb | 2 -- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/flipper/adapters/cache_base.rb b/lib/flipper/adapters/cache_base.rb index 6404ce62b..900b6b099 100644 --- a/lib/flipper/adapters/cache_base.rb +++ b/lib/flipper/adapters/cache_base.rb @@ -1,5 +1,8 @@ module Flipper module Adapters + # Base class for caching adapters. Inherit from this and then override + # read_feature_keys, read_many_features, expire_features_cache + # and expire_feature_cache. class CacheBase include ::Flipper::Adapter @@ -15,6 +18,9 @@ class CacheBase # Public: The cache key where the set of known features is cached. attr_reader :features_cache_key + # Public: Alias expires_in to ttl for compatibility. + alias_method :expires_in, :ttl + def initialize(adapter, cache, ttl = 300, prefix: nil) @adapter = adapter @cache = cache diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index 5644bc1be..8a3cc5a4d 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -7,8 +7,6 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Redis. class RedisCache < CacheBase - alias_method :expires_in, :ttl - # Public: Expire the cache for the set of known feature names. def expire_features_cache @cache.del(@features_cache_key) From ba86c7ebfb0b12f369889e2b35243f38f518d81e Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:44:48 -0500 Subject: [PATCH 16/20] Share the read many features logic This simplifies the cache stuff because now its just cache_fetch, read_multi, write and delete. --- .../adapters/active_support_cache_store.rb | 51 ++++---------- lib/flipper/adapters/cache_base.rb | 47 ++++++++++++- lib/flipper/adapters/dalli.rb | 45 +++--------- lib/flipper/adapters/redis_cache.rb | 70 +++++-------------- 4 files changed, 82 insertions(+), 131 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index 7a5318081..9e2c7c086 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -12,21 +12,11 @@ def initialize(adapter, cache, expires_in: 300, write_through: false, prefix: ni @write_through = write_through end - # Public: Expire the cache for the set of known feature names. - def expire_features_cache - @cache.delete(@features_cache_key) - end - - # Public: Expire the cache for a given feature. - def expire_feature_cache(key) - @cache.delete(feature_cache_key(key)) - end - def remove(feature) if @write_through result = @adapter.remove(feature) expire_features_cache - @cache.write(feature_cache_key(feature.key), default_config, expires_in: @ttl) + cache_write feature_cache_key(feature.key), default_config result else super @@ -36,7 +26,7 @@ def remove(feature) def enable(feature, gate, thing) if @write_through result = @adapter.enable(feature, gate, thing) - @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @ttl) + cache_write feature_cache_key(feature.key), @adapter.get(feature) result else super @@ -46,7 +36,7 @@ def enable(feature, gate, thing) def disable(feature, gate, thing) if @write_through result = @adapter.disable(feature, gate, thing) - @cache.write(feature_cache_key(feature.key), @adapter.get(feature), expires_in: @ttl) + cache_write feature_cache_key(feature.key), @adapter.get(feature) result else super @@ -55,37 +45,20 @@ def disable(feature, gate, thing) private - # Private: Returns the Set of known feature keys. - def read_feature_keys - @cache.fetch(@features_cache_key, expires_in: @ttl) { @adapter.features } + def cache_fetch(key, &block) + @cache.fetch(key, expires_in: @ttl, &block) end - # Private: Given an array of features, attempts to read through cache in - # as few network calls as possible. - def read_many_features(features) - keys = features.map { |feature| feature_cache_key(feature.key) } - cache_result = @cache.read_multi(*keys) - uncached_features = features.reject { |feature| cache_result[feature_cache_key(feature)] } - - if uncached_features.any? - response = @adapter.get_multi(uncached_features) - response.each do |key, value| - @cache.write(feature_cache_key(key), value, expires_in: @ttl) - cache_result[feature_cache_key(key)] = value - end - end + def cache_read_multi(keys) + @cache.read_multi(*keys) + end - result = {} - features.each do |feature| - result[feature.key] = cache_result[feature_cache_key(feature.key)] - end - result + def cache_write(key, value) + @cache.write(key, value, expires_in: @ttl) end - def read_feature(feature) - @cache.fetch(feature_cache_key(feature.key), expires_in: @ttl) do - @adapter.get(feature) - end + def cache_delete(key) + @cache.delete(key) end end end diff --git a/lib/flipper/adapters/cache_base.rb b/lib/flipper/adapters/cache_base.rb index 900b6b099..ce052f88b 100644 --- a/lib/flipper/adapters/cache_base.rb +++ b/lib/flipper/adapters/cache_base.rb @@ -1,8 +1,7 @@ module Flipper module Adapters # Base class for caching adapters. Inherit from this and then override - # read_feature_keys, read_many_features, expire_features_cache - # and expire_feature_cache. + # cache_fetch, cache_read_multi, cache_write, and cache_delete. class CacheBase include ::Flipper::Adapter @@ -32,6 +31,16 @@ def initialize(adapter, cache, ttl = 300, prefix: nil) @features_cache_key = "#{@namespace}/features" end + # Public: Expire the cache for the set of known feature names. + def expire_features_cache + cache_delete @features_cache_key + end + + # Public: Expire the cache for a given feature. + def expire_feature_cache(key) + cache_delete feature_cache_key(key) + end + # Public def features read_feature_keys @@ -95,6 +104,40 @@ def disable(feature, gate, thing) def feature_cache_key(key) "#{@namespace}/feature/#{key}" end + + private + + # Private: Returns the Set of known feature keys. + def read_feature_keys + cache_fetch(@features_cache_key) { @adapter.features } + end + + # Private: Read through caching for a single feature. + def read_feature(feature) + cache_fetch(feature_cache_key(feature.key)) { @adapter.get(feature) } + end + + # Private: Given an array of features, attempts to read through cache in + # as few network calls as possible. + def read_many_features(features) + keys = features.map { |feature| feature_cache_key(feature.key) } + cache_result = cache_read_multi(keys) + uncached_features = features.reject { |feature| cache_result[feature_cache_key(feature)] } + + if uncached_features.any? + response = @adapter.get_multi(uncached_features) + response.each do |key, value| + cache_write feature_cache_key(key), value + cache_result[feature_cache_key(key)] = value + end + end + + result = {} + features.each do |feature| + result[feature.key] = cache_result[feature_cache_key(feature.key)] + end + result + end end end end diff --git a/lib/flipper/adapters/dalli.rb b/lib/flipper/adapters/dalli.rb index f66c101ff..3b0c7b9b8 100644 --- a/lib/flipper/adapters/dalli.rb +++ b/lib/flipper/adapters/dalli.rb @@ -7,49 +7,22 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Memcached using the Dalli gem. class Dalli < CacheBase - # Public: Expire the cache for the set of known feature names. - def expire_features_cache - @cache.delete(@features_cache_key) - end - - # Public: Expire the cache for a given feature. - def expire_feature_cache(key) - @cache.delete(feature_cache_key(key)) - end - private - # Private: Returns the Set of known feature keys. - def read_feature_keys - @cache.fetch(@features_cache_key, @ttl) { @adapter.features } + def cache_fetch(key, &block) + @cache.fetch(key, @ttl, &block) end - # Private: Given an array of features, attempts to read through cache in - # as few network calls as possible. - def read_many_features(features) - keys = features.map { |feature| feature_cache_key(feature.key) } - cache_result = @cache.get_multi(keys) - uncached_features = features.reject { |feature| cache_result[feature_cache_key(feature.key)] } - - if uncached_features.any? - response = @adapter.get_multi(uncached_features) - response.each do |key, value| - @cache.set(feature_cache_key(key), value, @ttl) - cache_result[feature_cache_key(key)] = value - end - end + def cache_read_multi(keys) + @cache.get_multi(keys) + end - result = {} - features.each do |feature| - result[feature.key] = cache_result[feature_cache_key(feature.key)] - end - result + def cache_write(key, value) + @cache.set(key, value, @ttl) end - def read_feature(feature) - @cache.fetch(feature_cache_key(feature.key), @ttl) do - @adapter.get(feature) - end + def cache_delete(key) + @cache.delete(key) end end end diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index 8a3cc5a4d..5f420bf1b 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -7,73 +7,35 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Redis. class RedisCache < CacheBase - # Public: Expire the cache for the set of known feature names. - def expire_features_cache - @cache.del(@features_cache_key) - end - - # Public: Expire the cache for a given feature. - def expire_feature_cache(key) - @cache.del(feature_cache_key(key)) - end - private - # Private: Returns the Set of known feature keys. - def read_feature_keys - fetch(@features_cache_key) { @adapter.features } - end - - # Private: Given an array of features, attempts to read through cache in - # as few network calls as possible. - def read_many_features(features) - keys = features.map(&:key) - cache_result = Hash[keys.zip(multi_cache_get(keys))] - uncached_features = features.reject { |feature| cache_result[feature.key] } - - if uncached_features.any? - response = @adapter.get_multi(uncached_features) - response.each do |key, value| - set_with_ttl(feature_cache_key(key), value) - cache_result[key] = value - end - end - - result = {} - features.each do |feature| - result[feature.key] = cache_result[feature.key] - end - result - end - - def read_feature(feature) - fetch(feature_cache_key(feature.key)) do - @adapter.get(feature) - end - end - - def fetch(cache_key) - cached = @cache.get(cache_key) + def cache_fetch(key, &block) + cached = @cache.get(key) if cached Marshal.load(cached) else to_cache = yield - set_with_ttl(cache_key, to_cache) + cache_write key, to_cache to_cache end end - def set_with_ttl(key, value) - @cache.setex(key, @ttl, Marshal.dump(value)) - end - - def multi_cache_get(keys) - return [] if keys.empty? + def cache_read_multi(keys) + return {} if keys.empty? - cache_keys = keys.map { |key| feature_cache_key(key) } - @cache.mget(*cache_keys).map do |value| + values = @cache.mget(*keys).map do |value| value ? Marshal.load(value) : nil end + + Hash[keys.zip(values)] + end + + def cache_write(key, value) + @cache.setex(key, @ttl, Marshal.dump(value)) + end + + def cache_delete(key) + @cache.del(key) end end end From 532736213d7c4db5a4efbfc33eca53725898ba42 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 15:57:28 -0500 Subject: [PATCH 17/20] Make as cache store initialize backwards compat --- .../adapters/active_support_cache_store.rb | 13 +++++++++-- .../active_support_cache_store_spec.rb | 23 +++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index 9e2c7c086..fd382b7b4 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -7,8 +7,17 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in ActiveSupport::ActiveSupportCacheStore caches. class ActiveSupportCacheStore < CacheBase - def initialize(adapter, cache, expires_in: 300, write_through: false, prefix: nil) - super(adapter, cache, expires_in, prefix: prefix) + def initialize(adapter, cache, ttl = 300, expires_in: :none_provided, write_through: false, prefix: nil) + if expires_in == :none_provided + ttl ||= 300 + else + warn "DEPRECATION WARNING: The `expires_in` kwarg is deprecated for " + + "Flipper::Adapters::ActiveSupportCacheStore and will be removed " + + "in the next major version. Please pass in expires in as third " + + "argument instead." + ttl = expires_in + end + super(adapter, cache, ttl, prefix: prefix) @write_through = write_through end diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 6ce952bee..9d22e9583 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -8,7 +8,7 @@ end let(:cache) { ActiveSupport::Cache::MemoryStore.new } let(:write_through) { false } - let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10, write_through: write_through) } + let(:adapter) { described_class.new(memory_adapter, cache, 10, write_through: write_through) } let(:flipper) { Flipper.new(adapter) } subject { adapter } @@ -23,6 +23,25 @@ expect(adapter.ttl).to eq(10) end + it "knows ttl when only expires_in provided" do + silence do + adapter = described_class.new(memory_adapter, cache, expires_in: 10) + expect(adapter.ttl).to eq(10) + end + end + + it "knows ttl when ttl and expires_in are provided" do + silence do + adapter = described_class.new(memory_adapter, cache, 200, expires_in: 10) + expect(adapter.ttl).to eq(10) + end + end + + it "knows default when no ttl or expires_in provided" do + adapter = described_class.new(memory_adapter, cache) + expect(adapter.ttl).to eq(300) + end + it "knows features_cache_key" do expect(adapter.features_cache_key).to eq("flipper/v1/features") end @@ -52,7 +71,7 @@ end context "when using a prefix" do - let(:adapter) { described_class.new(memory_adapter, cache, expires_in: 10, prefix: "foo/") } + let(:adapter) { described_class.new(memory_adapter, cache, 10, prefix: "foo/") } it_should_behave_like 'a flipper adapter' it "knows features_cache_key" do From 952ddb58940309540d59e6883f4f4545e5a84070 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 16:14:53 -0500 Subject: [PATCH 18/20] Default as cache store ttl to forever as it use to be --- lib/flipper/adapters/active_support_cache_store.rb | 14 ++++++++++---- .../adapters/active_support_cache_store_spec.rb | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/flipper/adapters/active_support_cache_store.rb b/lib/flipper/adapters/active_support_cache_store.rb index fd382b7b4..379c5c5a0 100644 --- a/lib/flipper/adapters/active_support_cache_store.rb +++ b/lib/flipper/adapters/active_support_cache_store.rb @@ -7,9 +7,9 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in ActiveSupport::ActiveSupportCacheStore caches. class ActiveSupportCacheStore < CacheBase - def initialize(adapter, cache, ttl = 300, expires_in: :none_provided, write_through: false, prefix: nil) + def initialize(adapter, cache, ttl = nil, expires_in: :none_provided, write_through: false, prefix: nil) if expires_in == :none_provided - ttl ||= 300 + ttl ||= nil else warn "DEPRECATION WARNING: The `expires_in` kwarg is deprecated for " + "Flipper::Adapters::ActiveSupportCacheStore and will be removed " + @@ -55,7 +55,7 @@ def disable(feature, gate, thing) private def cache_fetch(key, &block) - @cache.fetch(key, expires_in: @ttl, &block) + @cache.fetch(key, write_options, &block) end def cache_read_multi(keys) @@ -63,12 +63,18 @@ def cache_read_multi(keys) end def cache_write(key, value) - @cache.write(key, value, expires_in: @ttl) + @cache.write(key, value, write_options) end def cache_delete(key) @cache.delete(key) end + + def write_options + write_options = {} + write_options[:expires_in] = @ttl if @ttl + write_options + end end end end diff --git a/spec/flipper/adapters/active_support_cache_store_spec.rb b/spec/flipper/adapters/active_support_cache_store_spec.rb index 9d22e9583..10ff969c6 100644 --- a/spec/flipper/adapters/active_support_cache_store_spec.rb +++ b/spec/flipper/adapters/active_support_cache_store_spec.rb @@ -39,7 +39,7 @@ it "knows default when no ttl or expires_in provided" do adapter = described_class.new(memory_adapter, cache) - expect(adapter.ttl).to eq(300) + expect(adapter.ttl).to be(nil) end it "knows features_cache_key" do From bb7540086a724ff048b4a7a57108ce44f79cfc21 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 16:18:02 -0500 Subject: [PATCH 19/20] Remove deprecated option --- spec/flipper/middleware/memoizer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/flipper/middleware/memoizer_spec.rb b/spec/flipper/middleware/memoizer_spec.rb index 8b2d21c5a..065451652 100644 --- a/spec/flipper/middleware/memoizer_spec.rb +++ b/spec/flipper/middleware/memoizer_spec.rb @@ -458,7 +458,7 @@ def get(uri, params = {}, env = {}, &block) logged_memory = Flipper::Adapters::OperationLogger.new(memory) cache = ActiveSupport::Cache::MemoryStore.new cache.clear - cached = Flipper::Adapters::ActiveSupportCacheStore.new(logged_memory, cache, expires_in: 10) + cached = Flipper::Adapters::ActiveSupportCacheStore.new(logged_memory, cache) logged_cached = Flipper::Adapters::OperationLogger.new(cached) memo = {} flipper = Flipper.new(logged_cached) From eadce39597805bfd161ed9ed38c78caf3821bd66 Mon Sep 17 00:00:00 2001 From: John Nunemaker Date: Sun, 18 Feb 2024 16:21:58 -0500 Subject: [PATCH 20/20] Change dalli and redis cache back to prior defaults We'll change them in breaking fashion in 2.0 --- lib/flipper/adapters/dalli.rb | 4 ++++ lib/flipper/adapters/redis_cache.rb | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/lib/flipper/adapters/dalli.rb b/lib/flipper/adapters/dalli.rb index 3b0c7b9b8..95139060c 100644 --- a/lib/flipper/adapters/dalli.rb +++ b/lib/flipper/adapters/dalli.rb @@ -7,6 +7,10 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Memcached using the Dalli gem. class Dalli < CacheBase + def initialize(adapter, cache, ttl = 0, prefix: nil) + super + end + private def cache_fetch(key, &block) diff --git a/lib/flipper/adapters/redis_cache.rb b/lib/flipper/adapters/redis_cache.rb index 5f420bf1b..001de6e3b 100644 --- a/lib/flipper/adapters/redis_cache.rb +++ b/lib/flipper/adapters/redis_cache.rb @@ -7,6 +7,10 @@ module Adapters # Public: Adapter that wraps another adapter with the ability to cache # adapter calls in Redis. class RedisCache < CacheBase + def initialize(adapter, cache, ttl = 3600, prefix: nil) + super + end + private def cache_fetch(key, &block)