Merge pull request rails#48381 from Shopify/as-cache-strict-arguments

Eagerly validate pool arguments in Redis and MemCache stores

Author: byroot

activesupport/lib/active_support/cache.rb

@@ -186,35 +186,44 @@ class << self
           private_constant :DEFAULT_POOL_OPTIONS
 
           def retrieve_pool_options(options)
-            unless options.key?(:pool) || options.key?(:pool_size) || options.key?(:pool_timeout)
-              options[:pool] = true
-            end
+            if options.key?(:pool)
+              pool_options = options.delete(:pool)
+            elsif options.key?(:pool_size) || options.key?(:pool_timeout)
+              pool_options = {}
+
+              if options.key?(:pool_size)
+                ActiveSupport.deprecator.warn(<<~MSG)
+                  Using :pool_size is deprecated and will be removed in Rails 7.2.
+                  Use `pool: { size: #{options[:pool_size].inspect} }` instead.
+                MSG
+                pool_options[:size] = options.delete(:pool_size)
+              end
 
-            if (pool_options = options.delete(:pool))
-              if Hash === pool_options
-                DEFAULT_POOL_OPTIONS.merge(pool_options)
-              else
-                DEFAULT_POOL_OPTIONS
+              if options.key?(:pool_timeout)
+                ActiveSupport.deprecator.warn(<<~MSG)
+                  Using :pool_timeout is deprecated and will be removed in Rails 7.2.
+                  Use `pool: { timeout: #{options[:pool_timeout].inspect} }` instead.
+                MSG
+                pool_options[:timeout] = options.delete(:pool_timeout)
               end
             else
-              {}.tap do |pool_options|
-                if options[:pool_size]
-                  ActiveSupport.deprecator.warn(<<~MSG)
-                    Using :pool_size is deprecated and will be removed in Rails 7.2.
-                    Use `pool: { size: #{options[:pool_size].inspect} }` instead.
-                  MSG
-                  pool_options[:size] = options.delete(:pool_size)
-                end
-
-                if options[:pool_timeout]
-                  ActiveSupport.deprecator.warn(<<~MSG)
-                    Using :pool_timeout is deprecated and will be removed in Rails 7.2.
-                    Use `pool: { timeout: #{options[:pool_timeout].inspect} }` instead.
-                  MSG
-                  pool_options[:timeout] = options.delete(:pool_timeout)
-                end
-              end
+              pool_options = true
             end
+
+            case pool_options
+            when false, nil
+              return false
+            when true
+              pool_options = DEFAULT_POOL_OPTIONS
+            when Hash
+              pool_options[:size] = Integer(pool_options[:size]) if pool_options.key?(:size)
+              pool_options[:timeout] = Float(pool_options[:timeout]) if pool_options.key?(:timeout)
+              pool_options = DEFAULT_POOL_OPTIONS.merge(pool_options)
+            else
+              raise TypeError, "Invalid :pool argument, expected Hash, got: #{pool_options.inspect}"
+            end
+
+            pool_options unless pool_options.empty?
           end
       end
 

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -99,10 +99,10 @@ def self.build_mem_cache(*addresses) # :nodoc:
         addresses = nil if addresses.compact.empty?
         pool_options = retrieve_pool_options(options)
 
-        if pool_options.empty?
-          Dalli::Client.new(addresses, options)
-        else
+        if pool_options
           ConnectionPool.new(pool_options) { Dalli::Client.new(addresses, options.merge(threadsafe: false)) }
+        else
+          Dalli::Client.new(addresses, options)
         end
       end
 

activesupport/lib/active_support/cache/redis_cache_store.rb

@@ -105,6 +105,7 @@ def build_redis_client(**redis_options)
 
       attr_reader :redis_options
       attr_reader :max_key_bytesize
+      attr_reader :redis
 
       # Creates a new Redis cache store.
       #
@@ -141,7 +142,11 @@ def build_redis_client(**redis_options)
       #   cache.exist?('foo') # => true
       #   cache.exist?('bar') # => false
       def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, coder: default_coder, expires_in: nil, race_condition_ttl: nil, error_handler: DEFAULT_ERROR_HANDLER, skip_nil: false, **redis_options)
-        @redis_options = redis_options
+        if pool_options = self.class.send(:retrieve_pool_options, redis_options)
+          @redis = ::ConnectionPool.new(pool_options) { self.class.build_redis(**redis_options) }
+        else
+          @redis = self.class.build_redis(**redis_options)
+        end
 
         @max_key_bytesize = MAX_KEY_BYTESIZE
         @error_handler = error_handler
@@ -153,21 +158,8 @@ def initialize(namespace: nil, compress: true, compress_threshold: 1.kilobyte, c
           coder: coder, skip_nil: skip_nil
       end
 
-      def redis
-        @redis ||= begin
-          pool_options = self.class.send(:retrieve_pool_options, redis_options)
-
-          if pool_options.any?
-            ::ConnectionPool.new(pool_options) { self.class.build_redis(**redis_options) }
-          else
-            self.class.build_redis(**redis_options)
-          end
-        end
-      end
-
       def inspect
-        instance = @redis || @redis_options
-        "#<#{self.class} options=#{options.inspect} redis=#{instance.inspect}>"
+        "#<#{self.class} options=#{options.inspect} redis=#{redis.inspect}>"
       end
 
       # Cache Store API implementation.

activesupport/test/cache/stores/mem_cache_store_test.rb

@@ -86,6 +86,22 @@ def after_teardown
   include ConnectionPoolBehavior
   include FailureSafetyBehavior
 
+  test "validate pool arguments" do
+    assert_raises TypeError do
+      ActiveSupport::Cache::MemCacheStore.new(pool: { size: [] })
+    end
+
+    assert_raises TypeError do
+      ActiveSupport::Cache::MemCacheStore.new(pool: { timeout: [] })
+    end
+
+    ActiveSupport::Cache::MemCacheStore.new(pool: { size: "12", timeout: "1.5" })
+  end
+
+  test "instantiating the store doesn't connect to Memcache" do
+    ActiveSupport::Cache::MemCacheStore.new("memcached://localhost:1")
+  end
+
   # Overrides test from LocalCacheBehavior in order to stub out the cache clear
   # and replace it with a delete.
   def test_clear_also_clears_local_cache

activesupport/test/cache/stores/redis_cache_store_test.rb

@@ -110,9 +110,25 @@ class InitializationTest < ActiveSupport::TestCase
       assert_same @cache.redis, redis_instance
     end
 
+    test "validate pool arguments" do
+      assert_raises TypeError do
+        build(url: REDIS_URL, pool: { size: [] })
+      end
+
+      assert_raises TypeError do
+        build(url: REDIS_URL, pool: { timeout: [] })
+      end
+
+      build(url: REDIS_URL, pool: { size: "12", timeout: "1.5" })
+    end
+
+    test "instantiating the store doesn't connect to Redis" do
+      build(url: "redis://localhost:1")
+    end
+
     private
       def build(**kwargs)
-        ActiveSupport::Cache::RedisCacheStore.new(**kwargs.merge(pool: false)).tap(&:redis)
+        ActiveSupport::Cache::RedisCacheStore.new(pool: false, **kwargs).tap(&:redis)
       end
   end