From cb9d0e4864fa68fad9c49b880c32e90ddf0545bd Mon Sep 17 00:00:00 2001 From: Andrey Novikov Date: Mon, 9 Jan 2017 01:43:49 +0300 Subject: 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. --- activesupport/CHANGELOG.md | 25 ++++++++++++++++++++ .../lib/active_support/core_ext/integer/time.rb | 4 ++-- .../lib/active_support/core_ext/numeric/time.rb | 10 ++++---- activesupport/lib/active_support/duration.rb | 27 +++++++++++++++------- activesupport/test/core_ext/duration_test.rb | 1 + activesupport/test/core_ext/numeric_ext_test.rb | 10 ++++---- 6 files changed, 57 insertions(+), 20 deletions(-) (limited to 'activesupport') 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 -- cgit v1.2.3