Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

quoted_date converts time objects to default_timezone #627

Closed
lighthouse-import opened this Issue · 33 comments

1 participant

@lighthouse-import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/2946
Created by Geoff Buesing - 2011-01-29 13:33:21 UTC

This patch adds functionality to ActiveRecord::ConnectionAdapters::Quoting#quoted_date so that Time and time-like objects are converted to ActiveRecord::Base.default_timezone before being sent to the database.

Currently, if ActiveRecord::Base.default_timezone == :utc (the Rails default as of 2.1), and you use Time.now in find conditions, ex:

Article.all :conditions => ['posted_at < ?', Time.now]

...you'll create a query for a posted_at time that will be off by however many hours your system time zone differs from UTC. Given that Rails does automatic time zone conversions when reading and assigning model attributes, you'd expect that it would handle time zone conversions for you in find conditions (I've seen many reports of people being confused by this.)

To further confuse things, this query will work correctly with default_timezone == :utc:

Article.all :conditions => ['posted_at < ?', 1.day.ago]

...but this one won't:

Article.all :conditions => ['posted_at < ?', Time.now - 1.day]

This patch will fix this behavior, so that any time-like object passed in to find conditions will be sent to the database in the appropriate zone.

Another benefit of this patch: it will allow ActiveRecord time_zone_aware_attributes to be used with default_timezone == :local, a combination that doesn't work correctly right now. Tests still needed to confirm this.

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Koziarski - 2009-07-24 04:09:54 UTC

This seems reasonable to me, it makes the query conversion match the input conversion right?

{{{

n = Time.now
@foo.bar = n
@foo.save

assert_equal @foo, Foo.find_by_bar(n)

}}}

@lighthouse-import

Imported from Lighthouse.
Comment by Tekin - 2009-07-24 13:04:21 UTC

Big plus one from me as this recently bit me, the fixed behaviour makes much more sense.

@lighthouse-import

Imported from Lighthouse.
Comment by Xavier Noria - 2009-07-25 08:18:24 UTC

Yeah I've been bitten by that one as well. This is the behaviour I'd expect. +1.

@lighthouse-import

Imported from Lighthouse.
Comment by Tekin - 2009-07-25 10:32:34 UTC

Alternatively, I'll just wait until winter when the clocks go forward. ;)

@lighthouse-import

Imported from Lighthouse.
Comment by Sam Oliver - 2009-07-25 10:49:54 UTC

I'm also fighting with this.

Agree with Tekin, we should just wait for the clocks to go forward and all will be well again.

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Koziarski - 2009-07-26 21:21:54 UTC

So no objections, I say go for it geoff.

@lighthouse-import

Imported from Lighthouse.
Comment by Will Bryant - 2009-07-27 11:43:26 UTC

+1. I discovered the other day that if I have config.time_zone = 'Wellington', I need to explicitly convert my times using #utc before using them in certain conditions, or else they would be incorrectly expressed in the SQL - inconsistent with what we do elsewhere.

This patch removes the need for me to, and it doesn't fail the test for where I'm using the workaround either, so it looks good to me. Tested against 2.3.

@lighthouse-import

Imported from Lighthouse.
Comment by Geoff Buesing - 2009-07-27 12:44:07 UTC

Great, thanks for the feedback, I'm going to write a few more tests around this, and then I'll pull it in.

@lighthouse-import

Imported from Lighthouse.
Comment by Will Bryant - 2009-07-27 13:23:01 UTC

Hmm, one thing that warrents note. I tried this out in a second project and it did cause some spec failures. The underlying problem is that String#to_time returns UTC by default -

Time.parse('2007-02-20 23:59:59')
=> Tue Feb 20 23:59:59 +1300 2007
'2007-02-20 23:59:59'.to_time
=> Tue Feb 20 23:59:59 UTC 2007
'2007-02-20 23:59:59'.to_time(:local)
=> Tue Feb 20 23:59:59 +1300 2007

That's all fine. However, if say you are running with no config.active_record.default_timezone - so the local db is in the local system time, as is the app - then if you ran #to_time on a string and used the result to initialize an attribute on a model, it would effectively be assumed to be already in the appropriate timezone, because the timezone conversion was not performed:

r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59')
=> #
r.reload
=> #
r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59'.to_time)
=> #
r.reload
=> #

This is wrong, but it seems pretty reasonable when you look at the above output. What will happen with Geoff's patch applied looks like a bug:

r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59')
=> #
r.reload
=> #
r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59'.to_time)
=> #
r.reload
=> #

It's not though - it's now more consistent, but
1. You might assume otherwise, since the AR inspect format shows datetimes without a timezone, making it look like the time has changed on reload (it hasn't, it's just reported in a different timezone).
2. Anyone who's been misusing #to_time up till now will get regressions.

Time.parse already returns results in the local timezone by default, so no issue there - so I'm ok with this, I think people should be using Time.parse instead of String#to_time normally anyway.

But just wanted to get something in this ticket to help people confused by this breakage - and clarify that this patch doesn't only affect queries, it can also affect attribute writes in the (rare) case where you're parsing strings yourself using #to_time or any other function that returns times in a different timezone to the database timezone (itself rare, since most of us run the database in UTC, making this change have no behavioural impact).

@lighthouse-import

Imported from Lighthouse.
Comment by Will Bryant - 2009-07-27 13:25:40 UTC

Once again, with formatting:

Hmm, one thing that warrants note. I tried this out in a second project and it did cause some spec failures. The underlying problem is that String#to_time returns UTC by default -

>> Time.parse('2007-02-20 23:59:59')
=> Tue Feb 20 23:59:59 +1300 2007
>> '2007-02-20 23:59:59'.to_time
=> Tue Feb 20 23:59:59 UTC 2007
>> '2007-02-20 23:59:59'.to_time(:local)
=> Tue Feb 20 23:59:59 +1300 2007

That's all fine. However, if say you are running with no config.active_record.default_timezone - so the local db is in the local system time, as is the app - then if you ran #to_time on a string and used the result to initialize an attribute on a model, it would effectively be assumed to be already in the appropriate timezone, because the timezone conversion was not performed:

>> r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59')
=> #<MyModel id: 3, some_datetime: "2007-02-20 23:59:59">
>> r.reload
=> #<MyModel id: 3, some_datetime: "2007-02-20 23:59:59">
>> r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59'.to_time)
=> #<MyModel id: 4, some_datetime: "2007-02-20 23:59:59">
>> r.reload
=> #<MyModel id: 4, some_datetime: "2007-02-20 23:59:59">

This is wrong, but it seems pretty reasonable when you look at the above output. What will happen with Geoff's patch applied looks like a bug:

>> r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59')
=> #<MyModel id: 1, some_datetime: "2007-02-20 23:59:59">
>> r.reload
=> #<MyModel id: 1, some_datetime: "2007-02-20 23:59:59">
>> r = MyModel.create!(:some_datetime => '2007-02-20 23:59:59'.to_time)
=> #<MyModel id: 2, some_datetime: "2007-02-20 23:59:59">
>> r.reload
=> #<MyModel id: 2, some_datetime: "2007-02-21 12:59:59">

It's not though - it's now more consistent, but
1. You might assume otherwise, since the AR inspect format shows datetimes without a timezone, making it look like the time has changed on reload (it hasn't, it's just reported in a different timezone).
2. Anyone who's been misusing #to_time up till now will get regressions.

Time.parse already returns results in the local timezone by default, so no issue there - so I'm ok with this, I think people should be using Time.parse instead of String#to_time normally anyway.

But just wanted to get something in this ticket to help people confused by this breakage - and clarify that this patch doesn't only affect queries, it can also affect attribute writes in the (rare) case where you're parsing strings yourself using #to_time or any other function that returns times in a different timezone to the database timezone (itself rare, since most of us run the database in UTC, making this change have no behavioural impact).

@lighthouse-import

Imported from Lighthouse.
Comment by Will Bryant - 2009-07-27 13:26:43 UTC

Also, wanted to buy: accurate lighthouse list formatting instructions :|.

@lighthouse-import

Imported from Lighthouse.
Comment by Geoff Buesing - 2009-07-28 04:10:55 UTC

@Will -- thanks for the report. Indeed, this change has the potential to break apps that use Time.utc and Time.local objects interchangeably when building find conditions, or, if time_zone_aware_attributes are turned off, assigning to model attributes.

Time#to_s(:db) doesn't do any zone conversion when it reports the time to the database, so you can currently get away with treating Time.utc and Time.local instances interchanegably:

>> Time.utc(2000).to_s(:db)
=> "2000-01-01 00:00:00"
>> Time.local(2000).to_s(:db)
=> "2000-01-01 00:00:00"

With this patch, Time.utc and Time.local report themselves in a zone appropriate manner (I'm adding in a #getutc call to illustrate coercion to default_timezone :utc):

>> Time.utc(2000).getutc.to_s(:db)
=> "2000-01-01 00:00:00"
>> Time.local(2000).getutc.to_s(:db)
=> "2000-01-01 06:00:00"

...which is more appropriate behavior, given that these two objects represent different times. Apps that treat them as the same will get breakage -- best we can do is, document this well. I'm putting together a Rails guide on time handling; I'll make sure add info about this as a potential gotcha.

@lighthouse-import

Imported from Lighthouse.
Comment by Will Bryant - 2009-07-28 11:05:21 UTC

Yeah sounds good, thanks.

So, maybe this is a q for Koz - think this should land in 2.3 as well as 3.0? I think the issue I mentioned is rare, so I wouldn't let that alone hold it back?

@lighthouse-import

Imported from Lighthouse.
Comment by Michael Koziarski - 2009-07-28 20:01:30 UTC

I'm not 100% sure that it justifies a backport to 2-3-stable, but
could probably be convinced.

The behaviour has been present from the beginning, and dates and
timezones are ... tricky enough... to mean people are aware of this as
is?

@lighthouse-import

Imported from Lighthouse.
Comment by Repository - 2009-08-04 03:04:47 UTC

(from [6f97ad0]) quoted_date converts time-like objects to ActiveRecord::Base.default_timezone before serialization. This allows you to use Time.now in find conditions and have it correctly be serialized as the current time in UTC when default_timezone == :utc [#2946 state:resolved]
http://github.com/rails/rails/commit/6f97ad07ded847f29159baf71050c63f04282170

@lighthouse-import

Imported from Lighthouse.
Comment by Will Bryant - 2009-08-04 10:50:59 UTC

Yeah, we've fixed other timezone-handling bugs of similar magnitude in minor 2.x releases, so I'd vote for this to be applied to 2.3 too, personally.

@lighthouse-import

Imported from Lighthouse.
Comment by r.s.seidler (at gmail) - 2009-08-04 12:36:09 UTC

Hello!

@lighthouse-import

Imported from Lighthouse.
Comment by Raimonds Simanovskis - 2009-08-07 14:15:22 UTC

Already commented in github on this commit:

quoted_date will not change DateTime values local time zone if ActiveRecord::Base.default_timezone == :local
because DateTime objects do not have getlocal method (just getutc method).

Either this implementation should be changed or maybe ActiveSupport could add getlocal method to DateTime class (so that it would behave similar to Time class in this aspect).

Because of this issue test_saves_both_date_and_time in date_time_test.rb is not failing but should be failing (because DateTime UTC value is stored in database and then compared with local time zone Time value).

@lighthouse-import

Imported from Lighthouse.
Comment by Geoff Buesing - 2009-08-11 03:33:31 UTC

@Raimonds Simanovskis -- thanks for catching this. Indeed, that's unexpected behavior, given that all other combinations of default_timezone and Time-like objects will do a conversion.

Makes sense to add a DateTime#getlocal method, so that conversion will occur. test_saves_both_date_and_time should then be modified so that the now input value is in the proper local offset.

@lighthouse-import

Imported from Lighthouse.
Comment by Raimonds Simanovskis - 2009-08-11 08:03:53 UTC

Here is patch that I created (otherwise my Oracle enhanced adapter was failing on JRuby):

http://github.com/rsim/rails/commit/4ed223327c11b903904fd2bad8691c188a801d44

@lighthouse-import

Imported from Lighthouse.
Comment by Rohit Arondekar - 2010-08-03 06:47:59 UTC

Any updates here?

@lighthouse-import

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-08-30 03:10:30 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-10-15 22:01:34 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2010-11-15 21:15:11 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Colin Kelley - 2010-12-05 00:53:21 UTC

Hi Geoff,

We too have been burned by this problem so I was happy to see it addressed here. We also patched our copy of Rails to fix it. Initially our patch matched yours, in the quoted_value method. But we still found it very surprising that

  Time.now.to_s(:db)

returned a string in system local time zone whereas

  Time.zone.now.to_s(:db)

returned a string in UTC. This was a problem because we have methods that were using to_s(:db) and counting on the conversion to UTC to happen. Given Ruby's duck typing, the callers could pass either Time or TimeWithZone and it would appear to work but yield wrong results.

So we decided to fix this problem at its root: we patched active_support/core_ext/time/conversions.rb. Currently in Rails 3.0.3 that reads like:

  def to_formatted_s(format = :default)
    if formatter = DATE_FORMATS[format]
      formatter.respond_to?(:call) ? formatter.call(self).to_s : strftime(formatter)
    else
      to_default_s
    end
  end

Here it is patched:

  def to_formatted_s(format = :default)
    if formatter = DATE_FORMATS[format]
      instance =
        if format == :db
          ActiveRecord::Base.default_timezone == :utc ? dup.utc : dup.localtime # avoid changing self by calling utc or localtime
        else
          self
        end
      formatter.respond_to?(:call) ? formatter.call(instance).to_s : instance.strftime(formatter)
    else
      to_default_s
    end
  end

I believe ActiveSupport shouldn't be coupled to ActiveRecord like this, so one last step would be to move that default_time_zone down to ActiveSupport with a pass-through accessor at the ActiveRecord level.

What do you think of this approach? If it looks good I could submit a patch with tests.

@lighthouse-import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-12 21:58:20 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-27 03:15:37 UTC

[bulk edit]

@lighthouse-import

Imported from Lighthouse.
Comment by Will Bryant - 2011-04-12 10:51:43 UTC

Colin, I agree that passing the timezone setting down from activerecord to activesupport would be a much better approach (if #to_s needs to know about this at all).

Activesupport could/should ship without a :db format at all, and activerecord should add this format using the proc support already present, doing the necessary timezone conversion in the proc before calling the normal strftime.

Still not entirely convinced we need to do all this, since quoted_date and friends are actually used for any query stuff now, but at least it'd unbreak Rails 2 upgraders who have non-UTC databases.

@lighthouse-import

Imported from Lighthouse.
Comment by Colin Kelley - 2011-04-12 17:47:49 UTC

Will,

Activesupport could/should ship without a :db format at all, and activerecord should add this format using the proc support already present, doing the necessary timezone conversion in the proc before calling the normal strftime.

I agree that shipping ActiveSupport without a :db format and then overriding it at the ActiveRecord level would work too.

The tie-breaker would probably be whether anyone who just uses ActiveSupport finds the :db format useful, or whether very many are using it already. Both seem like decent possibilities to me.

Still not entirely convinced we need to do all this, since quoted_date and friends are actually used for any query stuff now, but at least it'd unbreak Rails 2 upgraders who have non-UTC databases.

I think it's important to do something because the current behavior violates the Principle of Least Surprise. (I believe this principle is critical with duck typing since there is an implicit assumption in duck typing that methods with identical names accept the same parameters with the same calling contracts. I guess overloading to_s() to take a parameter was already on this ice in this regard.)

Imagine a method write_to_file() that takes a timestamp parameter that it writes to the file in :db format. It's really easy for calling code to be changed from this:

write_to_file(Time.now)

to this:

write_to_file(1.second.ago) # timestamp should always be in the past

How many would realize they just changed the parameter type from a Time to a TimeWithZone?

Currently making the change above causes time zone to shift unexpectedly, which you can't directly tell since the time zone is not included in the :db format. You can waste a lot of time debugging this. I know this from experience. More than once in fact. :)

@lighthouse-import

Imported from Lighthouse.
Comment by Espen Antonsen - 2011-04-20 08:39:45 UTC

I have been using this patch for so long I forgot I had it. Tried without it today and see that this is still not fixed in Rails 3.0.7. Is there a reason why this has not been implemented?

ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.module_eval do
  def quoted_date(value)
    if value.acts_like?(:time) && value.respond_to?(:usec)
      begin
        "#{value.getutc.to_s(:db)}.#{sprintf("%06d", value.usec)} #{value.formatted_offset}"
      rescue
        "#{super}.#{sprintf("%06d", value.usec)}"
      end
    else
      super
    end
  end
end
```@
@lighthouse-import

Imported from Lighthouse.
Comment by Espen Antonsen - 2011-04-20 09:30:48 UTC

Person.where(:created_at => Time.zone.now).to_sql
=> "SELECT \"people\".* FROM \"people\" WHERE \"people\".\"created_at\" = '2011-04-20 16:44:52.349977'"
Time.zone.now
=> Wed, 20 Apr 2011 10:44:57 CEST +02:00
exit # apply override patch
Loading development environment (Rails 3.0.7)
Person.where(:created_at => Time.zone.now).to_sql
=> "SELECT \"people\".* FROM \"people\" WHERE \"people\".\"created_at\" = '2011-04-20 08:45:16.063987 +02:00'"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.