Skip to content

Commit 78b74e9

Browse files
Deprecate initialize memcache store with dalli client
Why: ---- Following up on [rails#47323](rails#47323). Many options are not forwarded to the Dalli client when it is initialized from the `ActiveSupport::Cache::MemCacheStore`. This is to support a broader set of features powered by the implementation. When an instance of a client is passed on the initializer, it takes precedence, and we have no control over which attributes will be overridden or re-processed on the client side; this is by design and should remain as such to allow both projects to progress independently. Having this option introduces several potential bugs that are difficult to pinpoint and get multiplied by which version of the tool is used and how each evolves. During the conversation on the issue, the `Dalli` client maintainer supports [deprecating](rails#47323 (comment)) this option. How: ---- Removing this implicit dependency will ensure each library can evolve separately and cements the usage of `Dalli::Client` as an [implementation detail](rails#21595 (comment)) We can not remove a supported feature overnight, so I propose we add a deprecation warning for the next minor release(7.2 at this time). There was a constant on the `Cache` namespace only used to restrict options passed to the `Dalli::Client` initializer that now lives on the `MemCacheStore` class. Co-authored-by: Eileen M. Uchitelle <[email protected]>
1 parent 0b95524 commit 78b74e9

File tree

3 files changed

+22
-5
lines changed

3 files changed

+22
-5
lines changed

activesupport/CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
* Deprecated initializing a `ActiveSupport::Cache::MemCacheStore` with an instance of `Dalli::Client`.
2+
3+
Deprecate the undocumented option of providing an already-initialized instance of `Dalli::Client` to `ActiveSupport::Cache::MemCacheStore`. Such clients could be configured with unrecognized options, which could lead to unexpected behavior. Instead, provide addresses as documented.
4+
5+
*aledustet*
6+
17
* Stub `Time.new()` in `TimeHelpers#travel_to`
28

39
```ruby

activesupport/lib/active_support/cache/mem_cache_store.rb

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ module Cache
2828
# MemCacheStore implements the Strategy::LocalCache strategy which implements
2929
# an in-memory cache inside of a block.
3030
class MemCacheStore < Store
31+
# These options represent behavior overridden by this implementation and should
32+
# not be allowed to get down to the Dalli client
33+
OVERRIDDEN_OPTIONS = UNIVERSAL_OPTIONS
34+
3135
# Advertise cache versioning support.
3236
def self.supports_cache_versioning?
3337
true
@@ -108,6 +112,7 @@ def self.build_mem_cache(*addresses) # :nodoc:
108112
#
109113
# If no addresses are provided, but <tt>ENV['MEMCACHE_SERVERS']</tt> is defined, it will be used instead. Otherwise,
110114
# MemCacheStore will connect to localhost:11211 (the default memcached port).
115+
# Passing a +Dalli::Client+ instance is deprecated and will be removed. Please pass an address instead.
111116
def initialize(*addresses)
112117
addresses = addresses.flatten
113118
options = addresses.extract_options!
@@ -117,15 +122,19 @@ def initialize(*addresses)
117122
super(options)
118123

119124
unless [String, Dalli::Client, NilClass].include?(addresses.first.class)
120-
raise ArgumentError, "First argument must be an empty array, an array of hosts or a Dalli::Client instance."
125+
raise ArgumentError, "First argument must be an empty array, address, or array of addresses."
121126
end
122127
if addresses.first.is_a?(Dalli::Client)
128+
ActiveSupport.deprecator.warn(<<~MSG)
129+
Initializing MemCacheStore with a Dalli::Client is deprecated and will be removed in Rails 7.2.
130+
Use memcached server addresses instead.
131+
MSG
123132
@data = addresses.first
124133
else
125134
mem_cache_options = options.dup
126135
# The value "compress: false" prevents duplicate compression within Dalli.
127136
mem_cache_options[:compress] = false
128-
(UNIVERSAL_OPTIONS - %i(compress)).each { |name| mem_cache_options.delete(name) }
137+
(OVERRIDDEN_OPTIONS - %i(compress)).each { |name| mem_cache_options.delete(name) }
129138
@data = self.class.build_mem_cache(*(addresses + [mem_cache_options]))
130139
end
131140
end

activesupport/test/cache/stores/mem_cache_store_test.rb

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -271,9 +271,11 @@ def test_unless_exist_expires_when_configured
271271
end
272272

273273
def test_uses_provided_dalli_client_if_present
274-
cache = lookup_store(Dalli::Client.new("custom_host"))
275-
276-
assert_equal ["custom_host"], servers(cache)
274+
assert_deprecated(ActiveSupport.deprecator) do
275+
host = "custom_host"
276+
cache = lookup_store(Dalli::Client.new(host))
277+
assert_equal [host], servers(cache)
278+
end
277279
end
278280

279281
def test_forwards_string_addresses_if_present

0 commit comments

Comments
 (0)