Merge pull request #27610 from Envek/fix_and_speed_up_duration_parsing

Fix inconsistent parsing of Durations with both months and years

6 files changed, 57 insertions, 20 deletions

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 873a39dbf6..fb9f5a447c 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,28 @@
+*   Fix inconsistent results when parsing large durations and constructing durations from code
+
+        ActiveSupport::Duration.parse('P3Y') == 3.years # It should be true
+
+    Duration parsing made independent from any moment of time:
+    Fixed length in seconds is assigned to each duration part during parsing.
+
+    Changed duration of months and years in seconds to more accurate and logical:
+
+     1. The value of 365.2425 days in Gregorian year is more accurate
+        as it accounts for every 400th non-leap year.
+
+     2. Month's length is bound to year's duration, which makes
+        sensible comparisons like `12.months == 1.year` to be `true`
+        and nonsensical ones like `30.days == 1.month` to be `false`.
+
+    Calculations on times and dates with durations shouldn't be affected as
+    duration's numeric value isn't used in calculations, only parts are used.
+
+    Methods on `Numeric` like `2.days` now use these predefined durations
+    to avoid duplicating of duration constants through the codebase and
+    eliminate creation of intermediate durations.
+
+    *Andrey Novikov, Andrew White*
+
 *   Change return value of `Rational#duplicable?`, `ComplexClass#duplicable?`
     to false.
 
diff --git a/activesupport/lib/active_support/core_ext/integer/time.rb b/activesupport/lib/active_support/core_ext/integer/time.rb
index 4a64872392..66ac3ca1b4 100644
--- a/activesupport/lib/active_support/core_ext/integer/time.rb
+++ b/activesupport/lib/active_support/core_ext/integer/time.rb
@@ -18,12 +18,12 @@ class Integer
   #   # equivalent to Time.now.advance(months: 4, years: 5)
   #   (4.months + 5.years).from_now
   def months
-    ActiveSupport::Duration.new(self * 30.days, [[:months, self]])
+    ActiveSupport::Duration.new(self * ActiveSupport::Duration::PARTS_IN_SECONDS[:months].to_i, [[:months, self]])
   end
   alias :month :months
 
   def years
-    ActiveSupport::Duration.new(self * 365.25.days.to_i, [[:years, self]])
+    ActiveSupport::Duration.new(self * ActiveSupport::Duration::PARTS_IN_SECONDS[:years].to_i, [[:years, self]])
   end
   alias :year :years
 end
diff --git a/activesupport/lib/active_support/core_ext/numeric/time.rb b/activesupport/lib/active_support/core_ext/numeric/time.rb
index 809dfd4e07..6ca2468047 100644
--- a/activesupport/lib/active_support/core_ext/numeric/time.rb
+++ b/activesupport/lib/active_support/core_ext/numeric/time.rb
@@ -27,7 +27,7 @@ class Numeric
   #
   #   2.minutes # => 2 minutes
   def minutes
-    ActiveSupport::Duration.new(self * 60, [[:minutes, self]])
+    ActiveSupport::Duration.new(self * ActiveSupport::Duration::PARTS_IN_SECONDS[:minutes], [[:minutes, self]])
   end
   alias :minute :minutes
 
@@ -35,7 +35,7 @@ class Numeric
   #
   #   2.hours # => 2 hours
   def hours
-    ActiveSupport::Duration.new(self * 3600, [[:hours, self]])
+    ActiveSupport::Duration.new(self * ActiveSupport::Duration::PARTS_IN_SECONDS[:hours], [[:hours, self]])
   end
   alias :hour :hours
 
@@ -43,7 +43,7 @@ class Numeric
   #
   #   2.days # => 2 days
   def days
-    ActiveSupport::Duration.new(self * 24.hours, [[:days, self]])
+    ActiveSupport::Duration.new(self * ActiveSupport::Duration::PARTS_IN_SECONDS[:days], [[:days, self]])
   end
   alias :day :days
 
@@ -51,7 +51,7 @@ class Numeric
   #
   #   2.weeks # => 2 weeks
   def weeks
-    ActiveSupport::Duration.new(self * 7.days, [[:weeks, self]])
+    ActiveSupport::Duration.new(self * ActiveSupport::Duration::PARTS_IN_SECONDS[:weeks], [[:weeks, self]])
   end
   alias :week :weeks
 
@@ -59,7 +59,7 @@ class Numeric
   #
   #   2.fortnights # => 4 weeks
   def fortnights
-    ActiveSupport::Duration.new(self * 2.weeks, [[:weeks, self * 2]])
+    ActiveSupport::Duration.new(self * 2 * ActiveSupport::Duration::PARTS_IN_SECONDS[:weeks], [[:weeks, self * 2]])
   end
   alias :fortnight :fortnights
 
diff --git a/activesupport/lib/active_support/duration.rb b/activesupport/lib/active_support/duration.rb
index c9e8c8fdc4..71b6001503 100644
--- a/activesupport/lib/active_support/duration.rb
+++ b/activesupport/lib/active_support/duration.rb
@@ -7,7 +7,15 @@ module ActiveSupport
   #
   #   1.month.ago       # equivalent to Time.now.advance(months: -1)
   class Duration
-    EPOCH = ::Time.utc(2000)
+    PARTS_IN_SECONDS = {
+      seconds:                               1, # Used in parse method for ease of handling part hashes with seconds
+      minutes:                              60,
+      hours:                           60 * 60,
+      days:                       24 * 60 * 60,
+      weeks:       7            * 24 * 60 * 60,
+      months:  ((365.2425 / 12) * 24 * 60 * 60).ceil, # 1 year is always 12 months long, no sense to bind it to days
+      years:    (365.2425       * 24 * 60 * 60).ceil, # Gregorian year length to account for the every 400 non-leap year
+    }.freeze
 
     attr_accessor :value, :parts
 
@@ -78,14 +86,14 @@ module ActiveSupport
     #   1.day.to_i      # => 86400
     #
     # Note that this conversion makes some assumptions about the
-    # duration of some periods, e.g. months are always 30 days
-    # and years are 365.25 days:
+    # duration of some periods, e.g. months are always 1/12 of year
+    # and years are 365.2425 days:
     #
-    #   # equivalent to 30.days.to_i
-    #   1.month.to_i    # => 2592000
+    #   # equivalent to (1.year / 12).to_i
+    #   1.month.to_i    # => 2629746
     #
-    #   # equivalent to 365.25.days.to_i
-    #   1.year.to_i     # => 31557600
+    #   # equivalent to 365.2425.days.to_i
+    #   1.year.to_i     # => 31556952
     #
     # In such cases, Ruby's core
     # Date[http://ruby-doc.org/stdlib/libdoc/date/rdoc/Date.html] and
@@ -148,7 +156,10 @@ module ActiveSupport
     # If invalid string is provided, it will raise +ActiveSupport::Duration::ISO8601Parser::ParsingError+.
     def self.parse(iso8601duration)
       parts = ISO8601Parser.new(iso8601duration).parse!
-      new(EPOCH.advance(parts) - EPOCH, parts)
+      total_seconds = parts.inject(0) do |total, (part, value)|
+        total + value * PARTS_IN_SECONDS[part]
+      end
+      new(total_seconds, parts)
     end
 
     # Build ISO 8601 Duration string for this duration.
diff --git a/activesupport/test/core_ext/duration_test.rb b/activesupport/test/core_ext/duration_test.rb
index fc0dd41d0e..8c4c08ffd4 100644
--- a/activesupport/test/core_ext/duration_test.rb
+++ b/activesupport/test/core_ext/duration_test.rb
@@ -340,6 +340,7 @@ class DurationTest < ActiveSupport::TestCase
         travel_to Time.utc(2016, 11, 4) do
           assert_equal 604800, ActiveSupport::Duration.parse("P7D").to_i
           assert_equal 604800, ActiveSupport::Duration.parse("P1W").to_i
+          assert_equal ActiveSupport::Duration.parse(3.years.iso8601).to_i, 3.years.to_i
         end
       end
     end
diff --git a/activesupport/test/core_ext/numeric_ext_test.rb b/activesupport/test/core_ext/numeric_ext_test.rb
index 5361b7b708..5f86bf97c8 100644
--- a/activesupport/test/core_ext/numeric_ext_test.rb
+++ b/activesupport/test/core_ext/numeric_ext_test.rb
@@ -12,7 +12,7 @@ class NumericExtTimeAndDateTimeTest < ActiveSupport::TestCase
       10.minutes => 600,
       1.hour + 15.minutes => 4500,
       2.days + 4.hours + 30.minutes => 189000,
-      5.years + 1.month + 1.fortnight => 161589600
+      5.years + 1.month + 1.fortnight => 161624106
     }
   end
 
@@ -61,10 +61,10 @@ class NumericExtTimeAndDateTimeTest < ActiveSupport::TestCase
   end
 
   def test_duration_after_conversion_is_no_longer_accurate
-    assert_equal 30.days.to_i.seconds.since(@now), 1.month.to_i.seconds.since(@now)
-    assert_equal 365.25.days.to_f.seconds.since(@now), 1.year.to_f.seconds.since(@now)
-    assert_equal 30.days.to_i.seconds.since(@dtnow), 1.month.to_i.seconds.since(@dtnow)
-    assert_equal 365.25.days.to_f.seconds.since(@dtnow), 1.year.to_f.seconds.since(@dtnow)
+    assert_equal (1.year / 12).to_i.seconds.since(@now), 1.month.to_i.seconds.since(@now)
+    assert_equal 365.2425.days.to_f.seconds.since(@now), 1.year.to_f.seconds.since(@now)
+    assert_equal (1.year / 12).to_i.seconds.since(@dtnow), 1.month.to_i.seconds.since(@dtnow)
+    assert_equal 365.2425.days.to_f.seconds.since(@dtnow), 1.year.to_f.seconds.since(@dtnow)
   end
 
   def test_add_one_year_to_leap_day