Merge pull request rails#47891 from ghiculescu/expires_in-warning

Cache: warning if `expires_in` is given an incorrect value

Author: byroot

activesupport/lib/active_support/cache.rb

@@ -740,15 +740,13 @@ def merged_options(call_options)
             expires_at = call_options.delete(:expires_at)
             call_options[:expires_in] = (expires_at - Time.now) if expires_at
 
+            if call_options[:expires_in].is_a?(Time)
+              expires_in = call_options[:expires_in]
+              raise ArgumentError.new("expires_in parameter should not be a Time. Did you mean to use expires_at? Got: #{expires_in}")
+            end
             if call_options[:expires_in]&.negative?
               expires_in = call_options.delete(:expires_in)
-              error = ArgumentError.new("Cache expiration time is invalid, cannot be negative: #{expires_in}")
-              if ActiveSupport::Cache::Store.raise_on_invalid_cache_expiration_time
-                raise error
-              else
-                ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning)
-                logger.error("#{error.class}: #{error.message}") if logger
-              end
+              handle_invalid_expires_in("Cache expiration time is invalid, cannot be negative: #{expires_in}")
             end
 
             if options.empty?
@@ -761,6 +759,16 @@ def merged_options(call_options)
           end
         end
 
+        def handle_invalid_expires_in(message)
+          error = ArgumentError.new(message)
+          if ActiveSupport::Cache::Store.raise_on_invalid_cache_expiration_time
+            raise error
+          else
+            ActiveSupport.error_reporter&.report(error, handled: true, severity: :warning)
+            logger.error("#{error.class}: #{error.message}") if logger
+          end
+        end
+
         # Normalize aliased options to their canonical form
         def normalize_options(options)
           options = options.dup

activesupport/test/cache/behaviors/cache_store_behavior.rb

@@ -602,6 +602,7 @@ def test_invalid_expiration_time_raises_an_error_when_raise_on_invalid_cache_exp
         @cache.write(key, "bar", expires_in: -60)
       end
       assert_equal "Cache expiration time is invalid, cannot be negative: -60", error.message
+      assert_nil @cache.read(key)
     end
   end
 
@@ -612,13 +613,25 @@ def test_invalid_expiration_time_reports_and_logs_when_raise_on_invalid_cache_ex
         logs = capture_logs do
           key = SecureRandom.uuid
           @cache.write(key, "bar", expires_in: -60)
+          assert_equal "bar", @cache.read(key)
         end
         assert_includes logs, "ArgumentError: #{error_message}"
       end
       assert_includes report.error.message, error_message
     end
   end
 
+  def test_expires_in_from_now_raises_an_error
+    time = 1.minute.from_now
+
+    key = SecureRandom.uuid
+    error = assert_raises(ArgumentError) do
+      @cache.write(key, "bar", expires_in: time)
+    end
+    assert_equal "expires_in parameter should not be a Time. Did you mean to use expires_at? Got: #{time}", error.message
+    assert_nil @cache.read(key)
+  end
+
   def test_race_condition_protection_skipped_if_not_defined
     key = SecureRandom.alphanumeric
     @cache.write(key, "bar")