`-ActiveSupport::Duration.build(1)` vs `ActiveSupport::Duration.build(-1)`

rubyonrails-core
1

TL;DR: (time + -ActiveSupport::Duration.build(1)) != (time + ActiveSupport::Duration.build(-1))

I am not sure if this is intended, because when I browse closed issues concerning unexpected ActiveSupport::Duration behaviour, some users learn that ActiveSupport::Duration has some counter-intuitive behaviour.

However, I have a hunch that I found unintended behaviour. https://github.com/rails/rails/pull/43795

I’ve added assertions showing expected behaviour that works, and a final assertion that shows expected behaviour that fails.

I did some cursory analysis of the error, and if it helps I will produce a chart that shows the error rate for a range of whole number values within a year. I suspect a potential rounding error, but I haven’t dug into the algorithm because my quick fix seemed to work.

2

I just had a thought … It doesn’t appear that there are any/many users of ActiveSupport::Duration.build (within rails) – except within ActiveSupport::Duration itself … is build intended as a private method?

ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Interval#serialize sidesteps this issue by using Numeric#seconds instead of ActiveSupport::Duration.build

3

Hmm, seems like other user’s have had difficulties too: Backout use of ActiveSupport::Duration.build · jeremyevans/sequel@52b2a75 · GitHub

4

Both expressions evaluate to different values, hence the discrepancy.

[28] pry(main)> ActiveSupport::Duration.build(-1)
=> -1 years, 11 months, 4 weeks, 2 days, 10 hours, 29 minutes, and 5 seconds
[29] pry(main)> -ActiveSupport::Duration.build(1)
=> -1 seconds
5

A very wild guess, the method shouldn’t be allowed to take in negative value perhaps? https://github.com/rails/rails/blob/83217025a171593547d1268651b446d3533e2019/activesupport/lib/active_support/duration.rb#L183

Moreover, the way its used generally yields consistent values.

(-1).second == -(1.second)

evaluates to true.

6

Both expressions evaluate to different values, hence the discrepancy.

They are equivalent too.

-ActiveSupport::Duration.build(1) == ActiveSupport.build(-1)

But when applied, they have a more than 10 hour difference. Is this a learning moment for me about the Gregorian Calendar, or coordinated universal time?

A very wild guess, the method shouldn’t be allowed to take in negative value perhaps?

You’ll see in my PR that is an aspect of my proposed solution.

(-1).second == -(1.second)

Yes, and that is what the pg interval serializer is based on. That is what I’ve shipped to the part of my codebase needing to deal with negative durations.

But, the essence of my proposal is to help the next person that is dealing with negative intervals.

In my tests I could change the last assert_equals to a refute_equals and add documentation to build to advise not to use it … for negative intervals. IF my proposed enhancement (which now supports negative intervals) is unwanted :grin:

1 Like
7

The root cause is the modulo based algorithm does not work with negative values. I have a couple people on my team supporting the approach for my fix :blush: