Merge pull request #28204 from rails/deprecate-implicit-coercion-of-durations

Deprecate implicit coercion of `ActiveSupport::Duration`

4 files changed, 147 insertions, 6 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 2999820c42..6ca227f40a 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,55 @@
+*   Deprecate implicit coercion of `ActiveSupport::Duration`
+
+    Currently `ActiveSupport::Duration` implicitly converts to a seconds
+    value when used in a calculation except for the explicit examples of
+    addition and subtraction where the duration is the receiver, e.g:
+
+        >> 2 * 1.day
+        => 172800
+
+    This results in lots of confusion especially when using durations
+    with dates because adding/subtracting a value from a date treats
+    integers as a day and not a second, e.g:
+
+        >> Date.today
+        => Wed, 01 Mar 2017
+        >> Date.today + 2 * 1.day
+        => Mon, 10 Apr 2490
+
+    To fix this we're implementing `coerce` so that we can provide a
+    deprecation warning with the intent of removing the implicit coercion
+    in Rails 5.2, e.g:
+
+        >> 2 * 1.day
+        DEPRECATION WARNING: Implicit coercion of ActiveSupport::Duration
+        to a Numeric is deprecated and will raise a TypeError in Rails 5.2.
+        => 172800
+
+    In Rails 5.2 it will raise `TypeError`, e.g:
+
+        >> 2 * 1.day
+        TypeError: ActiveSupport::Duration can't be coerced into Integer
+
+    This is the same behavior as with other types in Ruby, e.g:
+
+        >> 2 * "foo"
+        TypeError: String can't be coerced into Integer
+        >> "foo" * 2
+        => "foofoo"
+
+    As part of this deprecation add `*` and `/` methods to `AS::Duration`
+    so that calculations that keep the duration as the receiver work
+    correctly whether the final receiver is a `Date` or `Time`, e.g:
+
+        >> Date.today
+        => Wed, 01 Mar 2017
+        >> Date.today + 1.day * 2
+        => Fri, 03 Mar 2017
+
+    Fixes #27457.
+
+    *Andrew White*
+
 *   Update `DateTime#change` to support `:usec` and `:nsec` options.
 
     Adding support for these options now allows us to update the `DateTime#end_of`
diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb
index e09cee3335..5eee04a34e 100644
--- a/activesupport/lib/active_support/cache/mem_cache_store.rb
+++ b/activesupport/lib/active_support/cache/mem_cache_store.rb
@@ -156,7 +156,7 @@ module ActiveSupport
           expires_in = options[:expires_in].to_i
           if expires_in > 0 && !options[:raw]
             # Set the memcache expire a few minutes in the future to support race condition ttls on read
-            expires_in += 5.minutes
+            expires_in += 300
           end
           rescue_error_with false do
             @data.send(method, key, value, expires_in, options)
diff --git a/activesupport/lib/active_support/duration.rb b/activesupport/lib/active_support/duration.rb
index 70cf78519d..b92891c511 100644
--- a/activesupport/lib/active_support/duration.rb
+++ b/activesupport/lib/active_support/duration.rb
@@ -1,5 +1,7 @@
 require "active_support/core_ext/array/conversions"
 require "active_support/core_ext/object/acts_like"
+require "active_support/core_ext/string/filters"
+require "active_support/deprecation"
 
 module ActiveSupport
   # Provides accurate date and time measurements using Date#advance and
@@ -88,6 +90,25 @@ module ActiveSupport
       @parts.default = 0
     end
 
+    def coerce(other) #:nodoc:
+      ActiveSupport::Deprecation.warn(<<-MSG.squish)
+        Implicit coercion of ActiveSupport::Duration to a Numeric
+        is deprecated and will raise a TypeError in Rails 5.2.
+      MSG
+
+      [other, value]
+    end
+
+    # Compares one Duration with another or a Numeric to this Duration.
+    # Numeric values are treated as seconds.
+    def <=>(other)
+      if Duration === other
+        value <=> other.value
+      elsif Numeric === other
+        value <=> other
+      end
+    end
+
     # Adds another Duration or a Numeric to this Duration. Numeric values
     # are treated as seconds.
     def +(other)
@@ -109,6 +130,24 @@ module ActiveSupport
       self + (-other)
     end
 
+    # Multiplies this Duration by a Numeric and returns a new Duration.
+    def *(other)
+      if Numeric === other
+        Duration.new(value * other, parts.map { |type, number| [type, number * other] })
+      else
+        value * other
+      end
+    end
+
+    # Devides this Duration by a Numeric and returns a new Duration.
+    def /(other)
+      if Numeric === other
+        Duration.new(value / other, parts.map { |type, number| [type, number / other] })
+      else
+        value / other
+      end
+    end
+
     def -@ #:nodoc:
       Duration.new(-value, parts.map { |type, number| [type, -number] })
     end
@@ -212,8 +251,6 @@ module ActiveSupport
       ISO8601Serializer.new(self, precision: precision).serialize
     end
 
-    delegate :<=>, to: :value
-
     private
 
       def sum(sign, time = ::Time.current)
diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb
index 6facb04f1f..2b1a715b7a 100644
--- a/activesupport/test/core_ext/duration_test.rb
+++ b/activesupport/test/core_ext/duration_test.rb
@@ -84,8 +84,46 @@ class DurationTest < ActiveSupport::TestCase
     assert_nothing_raised { Date.today - Date.today }
   end
 
+  def test_plus
+    assert_equal 2.seconds, 1.second + 1.second
+    assert_instance_of ActiveSupport::Duration, 1.second + 1.second
+    assert_equal 2.seconds, 1.second + 1
+    assert_instance_of ActiveSupport::Duration, 1.second + 1
+  end
+
+  def test_minus
+    assert_equal 1.second, 2.seconds - 1.second
+    assert_instance_of ActiveSupport::Duration, 2.seconds - 1.second
+    assert_equal 1.second, 2.seconds - 1
+    assert_instance_of ActiveSupport::Duration, 2.seconds - 1
+  end
+
+  def test_multiply
+    assert_equal 7.days, 1.day * 7
+    assert_instance_of ActiveSupport::Duration, 1.day * 7
+
+    assert_deprecated do
+      assert_equal 86400, 1.day * 1.second
+    end
+  end
+
+  def test_divide
+    assert_equal 1.day, 7.days / 7
+    assert_instance_of ActiveSupport::Duration, 7.days / 7
+
+    assert_deprecated do
+      assert_equal 1, 1.day / 1.day
+    end
+  end
+
+  def test_date_added_with_multiplied_duration
+    assert_equal Date.civil(2017, 1, 3), Date.civil(2017, 1, 1) + 1.day * 2
+  end
+
   def test_plus_with_time
-    assert_equal 1 + 1.second, 1.second + 1, "Duration + Numeric should == Numeric + Duration"
+    assert_deprecated do
+      assert_equal 1 + 1.second, 1.second + 1, "Duration + Numeric should == Numeric + Duration"
+    end
   end
 
   def test_time_plus_duration_returns_same_time_datatype
@@ -104,6 +142,13 @@ class DurationTest < ActiveSupport::TestCase
     assert_equal 'expected a time or date, got ""', e.message, "ensure ArgumentError is not being raised by dependencies.rb"
   end
 
+  def test_implicit_coercion_is_deprecated
+    assert_deprecated { 1 + 1.second }
+    assert_deprecated { 1 - 1.second }
+    assert_deprecated { 1 * 1.second }
+    assert_deprecated { 1 / 1.second }
+  end
+
   def test_fractional_weeks
     assert_equal((86400 * 7) * 1.5, 1.5.weeks)
     assert_equal((86400 * 7) * 1.7, 1.7.weeks)
@@ -241,13 +286,20 @@ class DurationTest < ActiveSupport::TestCase
   def test_comparable
     assert_equal(-1, (0.seconds <=> 1.second))
     assert_equal(-1, (1.second <=> 1.minute))
-    assert_equal(-1, (1 <=> 1.minute))
+
+    assert_deprecated do
+      assert_equal(-1, (1 <=> 1.minute))
+    end
+
     assert_equal(0, (0.seconds <=> 0.seconds))
     assert_equal(0, (0.seconds <=> 0.minutes))
     assert_equal(0, (1.second <=> 1.second))
     assert_equal(1, (1.second <=> 0.second))
     assert_equal(1, (1.minute <=> 1.second))
-    assert_equal(1, (61 <=> 1.minute))
+
+    assert_deprecated do
+      assert_equal(1, (61 <=> 1.minute))
+    end
   end
 
   def test_twelve_months_equals_one_year