This project is archived and is in readonly mode.

#2946 open
Geoff Buesing

quoted_date converts time objects to default_timezone

Reported by Geoff Buesing | July 24th, 2009 @ 04:23 AM | in 3.0.6

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.

Comments and changes to this ticket

  • Michael Koziarski

    Michael Koziarski July 24th, 2009 @ 05:09 AM

    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)

    }}}

  • Tekin

    Tekin July 24th, 2009 @ 02:04 PM

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

  • Xavier Noria

    Xavier Noria July 25th, 2009 @ 09:18 AM

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

  • Tekin

    Tekin July 25th, 2009 @ 11:32 AM

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

  • Sam Oliver

    Sam Oliver July 25th, 2009 @ 11:49 AM

    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.

  • Michael Koziarski

    Michael Koziarski July 26th, 2009 @ 10:21 PM

    So no objections, I say go for it geoff.

  • Will Bryant

    Will Bryant July 27th, 2009 @ 12:43 PM

    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.

  • Geoff Buesing

    Geoff Buesing July 27th, 2009 @ 01:44 PM

    • State changed from “new” to “open”

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

  • Will Bryant

    Will Bryant July 27th, 2009 @ 02:23 PM

    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).

  • Will Bryant

    Will Bryant July 27th, 2009 @ 02:25 PM

    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).

  • Will Bryant

    Will Bryant July 27th, 2009 @ 02:26 PM

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

  • Geoff Buesing

    Geoff Buesing July 28th, 2009 @ 05:10 AM

    @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.

  • Will Bryant

    Will Bryant July 28th, 2009 @ 12:05 PM

    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?

  • Michael Koziarski

    Michael Koziarski July 28th, 2009 @ 09:01 PM

    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?

  • Repository

    Repository August 4th, 2009 @ 04:04 AM

    • State changed from “open” to “resolved”

    (from [6f97ad07ded847f29159baf71050c63f04282170]) 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/6f97ad07ded847f29159baf71050c6...

  • Will Bryant

    Will Bryant August 4th, 2009 @ 11:50 AM

    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.

  • r.s.seidler (at gmail)

    r.s.seidler (at gmail) August 4th, 2009 @ 01:36 PM

    • Assigned user changed from “Geoff Buesing” to “Frederick Cheung”

    Hello!

  • Raimonds Simanovskis

    Raimonds Simanovskis August 7th, 2009 @ 03:15 PM

    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).

  • Geoff Buesing

    Geoff Buesing August 11th, 2009 @ 04:33 AM

    • State changed from “resolved” to “open”

    @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.

  • Geoff Buesing

    Geoff Buesing August 11th, 2009 @ 04:34 AM

    • Assigned user changed from “Frederick Cheung” to “Geoff Buesing”
  • Raimonds Simanovskis

    Raimonds Simanovskis August 11th, 2009 @ 09:03 AM

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

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

  • Rohit Arondekar

    Rohit Arondekar August 3rd, 2010 @ 07:47 AM

    • Importance changed from “” to “”

    Any updates here?

  • Jeremy Kemper
  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
  • Santiago Pastorino
  • Colin Kelley

    Colin Kelley December 5th, 2010 @ 12:53 AM

    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.

  • Santiago Pastorino
  • Santiago Pastorino

    Santiago Pastorino February 27th, 2011 @ 03:15 AM

    • Milestone changed from 3.0.5 to 3.0.6
  • Will Bryant

    Will Bryant April 12th, 2011 @ 11:51 AM

    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.

  • Colin Kelley

    Colin Kelley April 12th, 2011 @ 06:47 PM

    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. :)

  • Espen Antonsen

    Espen Antonsen April 20th, 2011 @ 09:39 AM

    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
    @@@@
    
  • Espen Antonsen

    Espen Antonsen April 20th, 2011 @ 10:30 AM

    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'"

  • Espen Antonsen

    Espen Antonsen April 20th, 2011 @ 10:31 AM

    • Tag changed from active_record, timezone to active_record, postgresql, timezone
  • Espen Antonsen

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages