config.time_zone and datetime update #3965

Closed
schiza opened this Issue Dec 13, 2011 · 14 comments

Comments

Projects
None yet
9 participants
@schiza

schiza commented Dec 13, 2011

In my application I have: config.time_zone = 'Warsaw'

A strange issue I have, is that it seems like Rails are having problems with comparision of datetime fields.
If I change the datetime 1 hour back (and Warsaw is currently in timezone +0100), Rails won't update the database, even if the field has changed. However, if I change the field once again, then the update will go to the database.
Example:
(Rails 3.1.0, ruby-1.9.2-p290, fresh rails app):

$ rails g model User starts_at:datetime
$ rake db:migrate
$ rails c
Loading development environment (Rails 3.1.0)
ruby-1.9.2-p290 :001 > u = User.create({:starts_at => "2011-01-01 10:00"})
SQL (21.3ms)  INSERT INTO "users" ("created_at", "starts_at", "updated_at") VALUES (?, ?, ?)  [["created_at", Tue, 13 Dec 2011 11:32:50 CET +01:00], ["starts_at", Sat, 01 Jan 2011 10:00:00 CET +01:00], ["updated_at", Tue, 13 Dec 2011 11:32:50 CET +01:00]]
 => #<User id: 1, starts_at: "2011-01-01 09:00:00", created_at: "2011-12-13 10:32:50", updated_at: "2011-12-13 10:32:50"> 
ruby-1.9.2-p290 :002 > u.starts_at
 => Sat, 01 Jan 2011 10:00:00 CET +01:00 # datetime created
ruby-1.9.2-p290 :003 > u.starts_at = "2011-01-01 09:00:00" # new datetime with one hour back
 => "2011-01-01 09:00:00" 
ruby-1.9.2-p290 :004 > u.starts_at
 => Sat, 01 Jan 2011 09:00:00 CET +01:00 # changed datetime
ruby-1.9.2-p290 :005 > u.save 
 => true 
ruby-1.9.2-p290 :006 > u.starts_at = "2011-01-01 09:00:00"
 => "2011-01-01 09:00:00" 
ruby-1.9.2-p290 :007 > u.save
   (0.3ms)  UPDATE "users" SET "starts_at" = '2011-01-01 08:00:00.000000', "updated_at" = '2011-12-13 10:33:17.919092' WHERE "users"."id" = 1
 => true 

I've tested it in this fresh app, because I have a problem with this in larger application. What is going on? I've tried to browse the Rails code, tried to re-copy the relevant code 'by-hand' in console (like update, assign_attributes, even checked time_zone_conversion) and it worked, but not in 'real world'..

@borisd

This comment has been minimized.

Show comment
Hide comment
@borisd

borisd Dec 20, 2011

Had something similar

The problem appears to be here:
https://github.com/rails/rails/blob/3-1-stable/activerecord/lib/active_record/attribute_methods/dirty.rb#L62

When rails it testing if the value was changed it compares old & new:

old = From cache (which is Time in your current timezone)
new = Time in UTC (+00:00) as saved in the database

If the difference in time is the UTC offset, the above erroneously succeeds (luckly the new cached value holds the intended change).

The next save/update compares with the new (and correct) cached value and marks the field as changed.

Appears like the fix should be:
old = old.in_time_zone if clone_with_time_zone_conversion_attribute?(attr, old)
to
old = old.in_time_zone(0) if clone_with_time_zone_conversion_attribute?(attr, old)

But I am yet to test the changes, so take this with a grain of salt.

Boris

borisd commented Dec 20, 2011

Had something similar

The problem appears to be here:
https://github.com/rails/rails/blob/3-1-stable/activerecord/lib/active_record/attribute_methods/dirty.rb#L62

When rails it testing if the value was changed it compares old & new:

old = From cache (which is Time in your current timezone)
new = Time in UTC (+00:00) as saved in the database

If the difference in time is the UTC offset, the above erroneously succeeds (luckly the new cached value holds the intended change).

The next save/update compares with the new (and correct) cached value and marks the field as changed.

Appears like the fix should be:
old = old.in_time_zone if clone_with_time_zone_conversion_attribute?(attr, old)
to
old = old.in_time_zone(0) if clone_with_time_zone_conversion_attribute?(attr, old)

But I am yet to test the changes, so take this with a grain of salt.

Boris

@StefanH

This comment has been minimized.

Show comment
Hide comment
@StefanH

StefanH Feb 8, 2012

Contributor

I'm having the same issue in rails 3.2.1.

Can someone explain why the 'enhanced write method' in time_zone_conversion.rb:45 passes the original time to write_attribute (and thus to the dirty tracking), and not the time converted to the local time zone?:

write_attribute(:#{attr_name}, original_time)
@attributes_cache["#{attr_name}"] = time

this does not seem right.

Contributor

StefanH commented Feb 8, 2012

I'm having the same issue in rails 3.2.1.

Can someone explain why the 'enhanced write method' in time_zone_conversion.rb:45 passes the original time to write_attribute (and thus to the dirty tracking), and not the time converted to the local time zone?:

write_attribute(:#{attr_name}, original_time)
@attributes_cache["#{attr_name}"] = time

this does not seem right.

@stefankroes

This comment has been minimized.

Show comment
Hide comment
@stefankroes

stefankroes Feb 8, 2012

+1

Having the same issue on Rails 3.2.1.

+1

Having the same issue on Rails 3.2.1.

@mdi

This comment has been minimized.

Show comment
Hide comment
@mdi

mdi Feb 10, 2012

A similar issue to this is here: https://rails.lighthouseapp.com/projects/8994/tickets/3658-dirty-datetime-attributes-lose-time-zone-info

It was marked as resolved, but I'm still seeing the same behavior in 3.2.1.

mdi commented Feb 10, 2012

A similar issue to this is here: https://rails.lighthouseapp.com/projects/8994/tickets/3658-dirty-datetime-attributes-lose-time-zone-info

It was marked as resolved, but I'm still seeing the same behavior in 3.2.1.

@vanderhoorn

This comment has been minimized.

Show comment
Hide comment
@vanderhoorn

vanderhoorn Feb 18, 2012

Contributor

I'm experiencing the same bug.

It's kind of awkward to tell customers they cannot change the time 1hr (or 2hrs, depending on daylight savings time). :)

Contributor

vanderhoorn commented Feb 18, 2012

I'm experiencing the same bug.

It's kind of awkward to tell customers they cannot change the time 1hr (or 2hrs, depending on daylight savings time). :)

@lest

This comment has been minimized.

Show comment
Hide comment
@lest

lest Feb 19, 2012

Contributor

Could someone create a test script that reproduces the problem?

Contributor

lest commented Feb 19, 2012

Could someone create a test script that reproduces the problem?

@StefanH

This comment has been minimized.

Show comment
Hide comment
@StefanH

StefanH Feb 20, 2012

Contributor

I've written a test script, it's here: https://gist.github.com/1868608. It gives the same results in 3.2.1 as in edge. I had to put in ActiveRecord::Base.time_zone_aware_attributes = true and ActiveRecord::Base.default_timezone = :utc in addition to the instructions in the article.

expected:

true
false

actual:

false
true
Contributor

StefanH commented Feb 20, 2012

I've written a test script, it's here: https://gist.github.com/1868608. It gives the same results in 3.2.1 as in edge. I had to put in ActiveRecord::Base.time_zone_aware_attributes = true and ActiveRecord::Base.default_timezone = :utc in addition to the instructions in the article.

expected:

true
false

actual:

false
true
@StefanH

This comment has been minimized.

Show comment
Hide comment
@StefanH

StefanH Feb 20, 2012

Contributor

Also, when I change activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb:60 from:

write_attribute(:#{attr_name}, original_time)

to

write_attribute(:#{attr_name}, time)

It works as expected.

Contributor

StefanH commented Feb 20, 2012

Also, when I change activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb:60 from:

write_attribute(:#{attr_name}, original_time)

to

write_attribute(:#{attr_name}, time)

It works as expected.

@lest

This comment has been minimized.

Show comment
Hide comment
@lest

lest Feb 20, 2012

Contributor

Confirmed that https://gist.github.com/1868608 reproduces error.

@StefanH Are you going to write a test case and submit a pull request?

Contributor

lest commented Feb 20, 2012

Confirmed that https://gist.github.com/1868608 reproduces error.

@StefanH Are you going to write a test case and submit a pull request?

@StefanH

This comment has been minimized.

Show comment
Hide comment
@StefanH

StefanH Feb 20, 2012

Contributor

Yes I will look into it further. Never submitted a pull request before however, hope I do it right :). Any must-read articles on that before I try? (like http://jonathanleighton.com/articles/2011/awesome-active-record-bug-reports/)

Contributor

StefanH commented Feb 20, 2012

Yes I will look into it further. Never submitted a pull request before however, hope I do it right :). Any must-read articles on that before I try? (like http://jonathanleighton.com/articles/2011/awesome-active-record-bug-reports/)

@lest

This comment has been minimized.

Show comment
Hide comment
@lest

lest Feb 20, 2012

Contributor

There is a screencast about it.

Contributor

lest commented Feb 20, 2012

There is a screencast about it.

@StefanH

This comment has been minimized.

Show comment
Hide comment
@StefanH

StefanH Feb 21, 2012

Contributor

I've written a test to reproduce the problem, and an ugly solution: https://gist.github.com/1875597

It turns out that it is not as simple as changing write_attribute(:#{attr_name}, original_time) to write_attribute(:#{attr_name}, time), because that causes before_type_cast to fail. I've put some extra code in dirty.rb to handle time zone conversion (see gist). This fixes the problem, but it's bascally the same code that time_zone_conversion.rb:51 generates, so this is not very DRY.

I think the underlying problem is that the value that dirty.rb uses to determine if an attribute is dirty and the value that read_attribute returns eventually are not generated through the same code path. I think there is some duplication there (both in processing and in code).

Anyone have a better suggestion?

Contributor

StefanH commented Feb 21, 2012

I've written a test to reproduce the problem, and an ugly solution: https://gist.github.com/1875597

It turns out that it is not as simple as changing write_attribute(:#{attr_name}, original_time) to write_attribute(:#{attr_name}, time), because that causes before_type_cast to fail. I've put some extra code in dirty.rb to handle time zone conversion (see gist). This fixes the problem, but it's bascally the same code that time_zone_conversion.rb:51 generates, so this is not very DRY.

I think the underlying problem is that the value that dirty.rb uses to determine if an attribute is dirty and the value that read_attribute returns eventually are not generated through the same code path. I think there is some duplication there (both in processing and in code).

Anyone have a better suggestion?

lest added a commit to lest/rails that referenced this issue Feb 27, 2012

josevalim added a commit that referenced this issue Feb 28, 2012

Merge pull request #5196 from lest/patch-3
force datetime attributes to be changed, fix GH #3965
@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders May 5, 2012

Contributor

Is this still an issue?

Contributor

isaacsanders commented May 5, 2012

Is this still an issue?

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 15, 2012

Member

Since #5196 was merged, and it says it fixes this, I'm closing.

Member

steveklabnik commented May 15, 2012

Since #5196 was merged, and it says it fixes this, I'm closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment