ActiveSupport::TimeWithZone#eql?(self.dup) returns false, if created from DateTime object #14178

Closed
meismann opened this Issue Feb 24, 2014 · 14 comments

Projects

None yet

9 participants

@meismann

When creating an ActiveSupport::TimeWithZone from a DateTime object like so, I expect #eql? to return true:

2.1-head :001 > n = DateTime.now.in_time_zone
=> Mon, 24 Feb 2014 12:33:43 UTC +00:00
2.1-head :002 > n.eql? n.dup
 => false

Here is the faulty code in ActiveSupport::TimeWithZone#eql?:

def eql?(other)
  utc.eql?(other)
end

#utc returns the original DateTime object, whose #eql? cannot handle a comparison with an ActiveSupport::TimeWithZone correctly.

This is bad for example in Hash merging, because comparison of Hash keys is done with #eql?:

2.1-head :009 > h1 = { n => 1 }; h2 = { n.dup => 2 }
2.1-head :010 > h1.merge h2
# Expecting a Hash with a single key here with the value 2:
 => {Mon, 24 Feb 2014 13:23:24 UTC +00:00=>1, Mon, 24 Feb 2014 13:23:24 UTC +00:00=>2} 

However, if the ActiveSupport::TimeWithZone object is created from a Time object instead of from a DateTime object, the code works as expected:

2.1-head :004 > n = Time.now.in_time_zone
 => Mon, 24 Feb 2014 13:07:37 UTC +00:00 
2.1-head :005 > n.eql? n.dup
 => true 

The issue occurs in Rails 4.0.3, Rails 4.1.0. rc, and on Ruby versions 2.1-head as well as 2.0.0. It was mentioned in #3431, but not solved.

@miguelsan

That's a two years old bug? Au!

@dskang

I'm unable to reproduce this on ActiveSupport 4.0.3 and Ruby 2.0.0.

$ ruby -v
ruby 2.0.0p247 (2013-06-27 revision 41674) [x86_64-darwin12.4.1]
[1] pry(main)> gem 'activesupport', '= 4.0.3'
=> true
[2] pry(main)> require 'active_support/all'
=> true
[3] pry(main)> n = DateTime.now.in_time_zone
=> Fri, 28 Feb 2014 15:03:21 -0500
[4] pry(main)> n.eql? n.dup
=> true

Is there something that I'm missing?

EDIT: My bad, I didn't include a time zone. Using DateTime.now.in_time_zone 'UTC' reproduces the bug.

@kuldeepaggarwal kuldeepaggarwal added a commit to kuldeepaggarwal/rails that referenced this issue Feb 28, 2014
@kuldeepaggarwal kuldeepaggarwal fixes #14178
  Proper comparison of DateTime#eql? with ActiveSupport::TimeWithZone instance.

  Previously:
    n = DateTime.now.in_time_zone # => Fri, 28 Feb 2014 22:00:55 UTC +00:00
    n.eql?(n.dup) # => false
  Now:
    n = DateTime.now.in_time_zone # => Fri, 28 Feb 2014 22:00:55 UTC +00:00
    n.eql?(n.dup) # => true
47251e6
@kuldeepaggarwal kuldeepaggarwal added a commit to kuldeepaggarwal/rails that referenced this issue Feb 28, 2014
@kuldeepaggarwal kuldeepaggarwal fixes #14178
  Proper comparison of DateTime#eql? with ActiveSupport::TimeWithZone instance.

  Previously:
    n = DateTime.now.in_time_zone # => Fri, 28 Feb 2014 22:00:55 UTC +00:00
    n.eql?(n.dup) # => false
  Now:
    n = DateTime.now.in_time_zone # => Fri, 28 Feb 2014 22:00:55 UTC +00:00
    n.eql?(n.dup) # => true
baec5d7
@jsgarvin

I thought I'd take a stab at this one, and after much poking around in the code, I think @kuldeepaggarwal had the right fix in #14235. His fix directly mimics how the behavior is currently implemented on master for Time objects.

In master right now in activesupport/test/core_ext/time_ext_test.rb, we have the code...

def test_eql?
  assert_equal true, Time.utc(2000).eql?( ActiveSupport::TimeWithZone.new(Time.utc(2000), ActiveSupport::TimeZone['UTC']) )
  assert_equal true, Time.utc(2000).eql?( ActiveSupport::TimeWithZone.new(Time.utc(2000), ActiveSupport::TimeZone["Hawaii"]) )
  assert_equal false,Time.utc(2000, 1, 1, 0, 0, 1).eql?( ActiveSupport::TimeWithZone.new(Time.utc(2000), ActiveSupport::TimeZone['UTC']) )
end

and activesupport/lib/active_support/core_ext/time/calculations.rb has...

# Layers additional behavior on Time#eql? so that ActiveSupport::TimeWithZone instances
# can be eql? to an equivalent Time
def eql_with_coercion(other)
  # if other is an ActiveSupport::TimeWithZone, coerce a Time instance from it so we can do eql? comparison
  other = other.comparable_time if other.respond_to?(:comparable_time)
  eql_without_coercion(other)
end
alias_method :eql_without_coercion, :eql?
alias_method :eql?, :eql_with_coercion

Both of these examples from the existing code on master for the Time class bear a striking resemblance to #14235 to provide the same behavior for DateTime.

@rafaelfranca
Ruby on Rails member

cc @jeremy

@rails-bot rails-bot added the stale label Aug 19, 2014
@rails-bot

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@kuldeepaggarwal

It is still on master branch.

@why-el

This works correctly on master, as this reproducing test shows. However it fails on my console running the latest beta. Any thoughts @rafaelfranca ?

@rafaelfranca
Ruby on Rails member

Does the reproducing test fails with the beta?

@rafaelfranca rafaelfranca removed the stale label Sep 12, 2014
@kuldeepaggarwal

It is still on master. Attached snapshot is from newly created app.

screen shot 2014-09-13 at 12 46 47 am

@why-el

@rafaelfranca test pass for the latest beta also, but what @kuldeepaggarwal reports above happens to me too.

@ghost

This is still an issue. And it sucks hard when you want to use timestamps as keys for hashes. (I just had this ... took a while to track down.)

The reproducing test referenced by @why-el may pass depending on whether or not the default time-zone is utc. It should fail whenever it is not utc. Therefore if you simply add a non-utc timezone parameter to the in_time_zone call, it will definitely fail.

While I'd agree with @jeremy here that one cannot

expect standard library classes to be aware of all other classes the may be equal to

I cannot help but wonder why it is okay for Time to get its eql? monkey patched but not for DateTime. If the argument were that TimeWithZone is a wrapper around Time and not DateTime, then why can DateTime be wrapped in TimeWithZone?

Anyway the current implementation has most surprising behaviour. The easiest way I see to fix the issue outlined in the gist is to call utc on both self and other for all comparisons (I think it's only <=> and eql?).
However that would not address the leaky abstraction of TimeWithZone, that is when you wrap a Time object in TimeWithZone and compare it to a DateTime object wrapped in TimeWithZone with eql? they are not the same, even though their obj#hash values are equal.

@rails-bot

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot rails-bot added the stale label Jun 26, 2015
@kuldeepaggarwal

It is still on master branch.

screen shot 2015-06-26 at 8 53 07 am

@rafaelfranca
Ruby on Rails member

Thanks. Now the bot will not mark this issue as stale

@kaspth kaspth closed this in #20944 Jul 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment