Rails Ruby on Rails → 3.0.6

Comments and changes to this ticket

• 

  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)

  }}}

• You flagged this item as spam.
  

  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 July 25th, 2009 @ 09:18 AM

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

• You flagged this item as spam.
  

  Tekin July 25th, 2009 @ 11:32 AM

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

• You flagged this item as spam.
  

  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 July 26th, 2009 @ 10:21 PM

  So no objections, I say go for it geoff.

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

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

• 

  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.

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

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

  Hello!

• You flagged this item as spam.
  

  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 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 August 11th, 2009 @ 04:34 AM

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

• You flagged this item as spam.
  

  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 August 3rd, 2010 @ 07:47 AM

  • Importance changed from “” to “”

  Any updates here?

• 

  Jeremy Kemper August 30th, 2010 @ 04:10 AM

  • Milestone cleared.

  [bulk edit]

• 

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

  • Milestone set to “3.0.2”

  [bulk edit]

• 

  Santiago Pastorino November 15th, 2010 @ 09:15 PM

  • Milestone cleared.

  [bulk edit]

• You flagged this item as spam.
  

  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 February 12th, 2011 @ 09:58 PM

  • Milestone set to “3.0.5”

  [bulk edit]

• 

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

  • Milestone changed from “3.0.5” to “3.0.6”

  [bulk edit]

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

  • Tag changed from “active_record, timezone” to “active_record, postgresql, timezone”

• You flagged this item as spam.
  

  Espen Antonsen April 20th, 2011 @ 11:27 AM

  Same bug? https://rails.lighthouseapp.com/projects/8994/tickets/1562-postgres...