Fix inconsistent parsing of Durations with both months and years #27610

Merged
merged 1 commit into from Jan 12, 2017

Projects

None yet

3 participants

@Envek
Contributor
Envek commented Jan 8, 2017

@boazy reported in #16919 (comment) that the following code may fail (success depends on the current time):

ActiveSupport::Duration.parse(2.months.iso8601) == 2.months
ActiveSupport::Duration.parse(3.years.iso8601) == 3.years

The solution is make duration parsing independent of any moment of time and just assign fixed values of seconds to each duration part (as it's really done in methods like 2.days).

I have not used suggested solution with total_seconds += value.send(part.to_sym) as it will create many intermediate durations, slowing down parsing and littering memory. Instead I've added a part-to-seconds mapping hash constant to durations itself.

Failing regression test is included.

Duration parsing got 1.5x faster: https://gist.github.com/Envek/d0763db337d34ef78bf8d0a1aceca9aa

r? @pixeltrix

P.S>

Methods like beforementioned 2.days can be changed to use this constant too, this will speed up them, move all magic duration constants in single file and constants and eliminate creation of a bunch of intermediate durations. Now calling 2.years will call 365.25.days which in turn will call 24.hours which in turn will create duration for 2 * 365.25 * 24 * 3600 seconds from which seconds will be taken and all 3 intermediate durations will be thrown. I can change them too in this or separate pull request.

So

def years
  ActiveSupport::Duration.new(self * 365.25.days.to_i, [[:years, self]])
end

will become

def years
  ActiveSupport::Duration.new(self * ActiveSupport::Duration::PARTS_IN_SECONDS[:years].to_i, [[:years, self]])
end
@pixeltrix pixeltrix was assigned by rails-bot Jan 8, 2017
@pixeltrix pixeltrix added this to the 5.0.x milestone Jan 9, 2017
@pixeltrix
Member

@Envek hmm, you could also make the case that the current behavior of 1.year == 12.months is wrong, e.g:

>> 1.year == 12.months
=> false

and if I merge this PR it'll break this:

>> ActiveSupport::Duration.parse("P1Y") == ActiveSupport::Duration.parse("P12M")
=> true

So you could argue that the PR should be the other way round - change 1.year to use EPOCH and 1.year == 12.months will return true. Not that I'm suggesting that's what we do - I just need to give it some thought first.

@boazy
boazy commented Jan 9, 2017

Thanks @Envek.

@pixeltrix I can see what you're pointing at, but you can't have any proper definition of duration where 1 year = 12 months. And making some year be 365 days and other years be 366 days as well as making month length arbitrary in the same way just to make sure than 12.months == 1.year isn't really going to help make things consistent.

@pixeltrix
Member

@boazy if you take your examples which are inconsistent, they're only inconsistent because of the use of fixed constants in 1.year. If you used the epoch date then that inconsistency would go away. You argue that it's not possible to have 1.year == 12.months but that's entirely possible and would be expected given the following:

>> Date.current + 12.months
=> Tue, 09 Jan 2018
>> Date.current + 1.year
=> Tue, 09 Jan 2018
>> (Date.current + 1.year) == (Date.current + 12.months)
=> true

Also the year value of 365.25 days is for a Julian year, but we should be using a Gregorian year value of 365.2425 to account for leap years in 2000, 2400, etc.

My biggest problem with switching to using advance to calculate the duration is that it'll be much slower, so that's probably going to be a blocker on making the switch. As I said - I need to think about it 😄

@Envek
Contributor
Envek commented Jan 9, 2017

In 5.0 and current master things are not consistent:

1.year == 12.months                                                           # => false
ActiveSupport::Duration.parse("P1Y") == ActiveSupport::Duration.parse("P12M") # => true

In current version of this PR things get consistent by changing parsing to match the numeric extensions:

1.year == 12.months                                                           # => false
ActiveSupport::Duration.parse("P1Y") == ActiveSupport::Duration.parse("P12M") # => false

The calculation on times and dates with durations are working same as numeric value of Durations are not used in calculations, only parts are used, because of using advance. So next durations with nonsense values still work:

(Date.current + 1.year) == (Date.current + 12.months) # true
ActiveSupport::Duration.new(42, [[:months, 12]]) == 12.months # => false
now = Time.current; 
now + ActiveSupport::Duration.new(42, [[:months, 12]]) == now + 12.months # => true

May be the best approach is to ignore value at all and do comparisons of durations only by comparing normalized parts (so 12.months will actually become 1.year)?

@pixeltrix
Member

I was looking at the values for 1.month and 1.year and noticed that 365.2425 days is divisible by 12 so we could have 31556952 seconds for years and 2629746 seconds for months. This would mean that 12.months would then equal 1.year automatically. The value for 365.2425 days is what we should be using anyway since that's the gregorian value. The problem is that this would break 1.month == 30.days, but that's nonsense anyway plus I doubt anyone was using 365.25.days == 1.year

wdyt ?

@Envek
Contributor
Envek commented Jan 9, 2017

@pixeltrix, sounds reasonable for me. I've added these changes in separate commits to this pull request. Please take a look and play with them.

@pixeltrix
Member

There's also couple of comments that'll need changing where we state that 1.month == 30.days and 1.year == 365.25.days.

@pixeltrix
Member

Also I think changing 365.25 to 365.2425 can only be on master - the backport will have to use the old values for year and month with 1.year != 12.months.

@Envek
Contributor
Envek commented Jan 9, 2017

Searched repository for usage of x.month(s) and y.year(s) and corrected tests and comments accordingly.

If everything is fine I will squash and rebase and then create separate PR with these changes without changed constants to 5-0-stable.

@pixeltrix
Member

@Envek 👍

@Envek Envek Fix inconsistent results when parsing large durations and constructin…
…g 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.
cb9d0e4
@Envek
Contributor
Envek commented Jan 9, 2017

Squashed and rebased. Partial backport is on the way.

@Envek Envek added a commit to Envek/rails that referenced this pull request Jan 9, 2017
@Envek Envek Fix inconsistent results when parsing large durations and constructin…
…g 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.

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.

Partial backport of rails#27610
8c1ffc3
@boazy
boazy commented Jan 9, 2017

@pixeltrix @Envek
Thanks, I think this solution is good too. You break 1.month == 30.days, but that's not reasonable to rely on that either.
The problem with EPOCH is that's it's still rather arbitrary. If you make 2000 your EPOCH, then the first year will be a leap year with 366 days. If you make it 2001, then it would be 365 days. If you make it 2005, then it would also have a leap second (though I'm not sure if Ruby's Time actually does leap seconds right now). It's way better than using the current time, but setting constant values is more consistent (besides being faster).

@Envek
Contributor
Envek commented Jan 10, 2017

@boazy: Ruby's Time supports leap seconds on platforms that supports leap seconds with correct configuration. See https://bugs.ruby-lang.org/issues/8885 (So, it seems to be only Linux with special TZ environment variable value prefixed with right/, like right/Asia/Barnaul).

Rails' ActiveSupport::TimeWithZone may lack this support, but anyway this is out of scope for this PR. If you need this please try it and create new issue if it doesn't work as you expected.

@boazy
boazy commented Jan 12, 2017

@Envek Thanks. Since this PR removes reliance on EPOCH and hence on Ruby's Time, it doesn't matter whether Ruby supports leap seconds or not, since now duration values will be fixed, regardless of varying month lengths, leap years or leap seconds. This is a good thing, in my opinion.

@pixeltrix pixeltrix modified the milestone: 5.1.0, 5.0.x Jan 12, 2017
@pixeltrix pixeltrix merged commit 033f654 into rails:master Jan 12, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix
Member

@Envek thanks for your patience! 👍

@pixeltrix pixeltrix added a commit that referenced this pull request Jan 12, 2017
@pixeltrix pixeltrix Add additional tests for #27610
Since 1.month no longer equals 30.days add some tests to ensure that
addition maintains the same day in the month or is the last day in
the month if the month has less days than the current day. Also add
a test for the behaviour of 12.months == 1.year.
2a5ae2b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment