Skip to content
Browse files

Fix before_type_cast for timezone aware attributes by caching convert…

…ed value on write. Also remove read method reload arg on timezone attributes.
  • Loading branch information...
1 parent 5d43977 commit 0823bbd757f3654a08d300e27873758da606f06a @adzap adzap committed with tenderlove Mar 1, 2011
View
5 activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
@@ -38,12 +38,13 @@ def define_method_attribute=(attr_name)
if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name])
method_body, line = <<-EOV, __LINE__ + 1
def #{attr_name}=(original_time)
- time = original_time.dup unless original_time.nil?
+ time = original_time
unless time.acts_like?(:time)
time = time.is_a?(String) ? Time.zone.parse(time) : time.to_time rescue time
end
time = time.in_time_zone rescue nil if time
- write_attribute(:#{attr_name}, (time || original_time))
+ write_attribute(:#{attr_name}, original_time)
@nragaz
nragaz added a note Mar 24, 2011

This changes the behaviour -- the original_time is now stored in the database instead of the converted time. So doing:

event.occurred_at = "2011-03-24"

... now saves "2011-03-24 00:00:00" into the database instead of "2011-03-24 05:00:00" (what I expect in my timezone). When the column is read from the database, it will be converted to "2011-03-23 19:00:00" -- a different date entirely.

I don't believe this was intentional?

@josevalim
Ruby on Rails member
josevalim added a note Mar 24, 2011

Not intentional. Can you provide a test case? if this is actually broken, it is a pretty bad bug.

@nragaz
nragaz added a note Mar 24, 2011

I'll work on a test case - give me an hr. But I can absolutely guarantee that it's broken! Just had to fix every record created today. :)

(Note: would be way more able to get this going if bundle install didn't break on MRI 1.9 -- apparently the text-hyphen gem can't install on 1.9, but I have no idea what it's a dependency of! And whatever it is, it's not in the mri_18 group or the jruby group. Thoughts? - UPDATE: no idea why that was happening. Fixed on current master.)

@nragaz
nragaz added a note Mar 24, 2011

Here is a failing test diff: https://gist.github.com/885887

I'm afraid that apart from reverting to the original, my efforts to fix this are only resulting in more failures. I will keep trying.

(UPDATE: The problem, it seems to me, is that the conversion of the string to a time with zone should be happening in string_to_time(string) in schema_definitions.rb, rather than in time_zone_conversion.rb. Otherwise there is no clean way to distinguish between the value before typecasting and after, both when the record has been saved and when it has not.)

@tenderlove
Ruby on Rails member
tenderlove added a note Mar 24, 2011

This is a patch that removes the current test. Can you provide us a new test that fails?

@nragaz
nragaz added a note Mar 24, 2011

Durr. Apparently git has surprises for me! I will create a new diff when I get home, but in the meantime the actual code in that gist is my new test. I don't know why it decided to create a diff that removed the test instead of adding it.

@tenderlove
Ruby on Rails member
tenderlove added a note Mar 24, 2011

No problem. I'll give this test a try!

@tenderlove
Ruby on Rails member
tenderlove added a note Mar 26, 2011

@nragaz I've added your test on 3-0-stable. The test passes on 1.8.7 and 1.9.2 with pg, mysql, mysql2, and sqlite3. If there is a problem can you please supply us with a failing test. Thanks!

@nragaz
nragaz added a note Mar 26, 2011

Thanks @tenderlove for adding the test. But it fails for me -- I get the following result:

test_read_attributes_after_type_cast_on_datetime(AttributeMethodsTest) [test/cases/attribute_methods_test.rb:147]:
<Thu, 24 Mar 2011 00:00:00 PDT -07:00> expected but was
<Wed, 23 Mar 2011 21:00:00 PDT -07:00>.

I wonder if it passes for you because you are actually ON Pacific Time? If I change it to Eastern Time (my own timezone), for example, it passes for me, too.

@tenderlove
Ruby on Rails member
tenderlove added a note Mar 26, 2011

Ah, I bet that is the problem. I am on PST. I wonder if we can make this test TZ agnostic.

@adzap
adzap added a note Mar 26, 2011

I have just picked this conversation after being on holiday.

I had look at the code and can this is a (bad) bug. The problem lies in that arel_attribute_values method calls the read_attribute to get the value for each attribute to when persisting. The read_attribute method will try the _ form of read method first and then fallback to _read_attribute() which will do a direct typecast on the raw value.

Timezone aware attributes don't define the _ read method, and so will read a typecast raw value. This will cause this issue observed if the string is parsed while in any other timezone than the one intended at the time of write.

Regular attributes define the _ read method, which uses the default read method code for the attribute and is aliased to the public read method for the attribute, by default. I suggest we can just do the same for timezone aware attributes and it should solve the problem.

I will work up a fix.

@adzap
adzap added a note Mar 26, 2011

Sorry, ignore all that. I was reading the code incorrectly. The _ read method is defined.

@adzap
adzap added a note Mar 26, 2011

I am not on PST and this new test passes for me on sqlite3 and postgres (having unrelated setup issues with mysql).

@tenderlove
Ruby on Rails member
tenderlove added a note Mar 27, 2011

@adzap I changed the TZ to Berlin (I am not in Berlin), and the test failed, but only on 3-0-stable.

@adzap
adzap added a note Mar 27, 2011

@tenderlove Thanks. I see this issue. It's that read_attribute method on 3-0-stable does not attempt to call the attribute's default read method, as it does on master. Instead, it will just attempt a direct typecast on the value in the @attributes hash, which is a string.

This behaviour changed with this commit e0e3adf. If this was applied to 3-0-stable, then we can get it to work. Otherwise, I think that before_type_cast for timezone aware attributes can only be applied to master.

@tenderlove
Ruby on Rails member
tenderlove added a note Mar 27, 2011

@adzap backporting that commit definitely fixes the test. I don't mind backporting, but I'd like to talk it over with @jeremy first. After backporting, all AR tests pass, but I'm a little worried about changing existing behavior this deep in AR with just a bugfix release.

@adzap
adzap added a note Mar 27, 2011

@tenderlove sure thing. I can understand the hesitation.

@tenderlove
Ruby on Rails member
tenderlove added a note Mar 28, 2011

I talked to @jeremy, and he thinks it's safe to backport that commit. So I did. :-)

@nragaz
nragaz added a note Mar 28, 2011

Sweet, thanks guys, sorry I wasn't able to be more help resolving this. I will test the fix this afternoon. (UPDATE: seems to be working great! Thanks again.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ @attributes_cache["#{attr_name}"] = time
end
EOV
generated_attribute_methods.module_eval(method_body, __FILE__, line)
View
26 activerecord/test/cases/attribute_methods_test.rb
@@ -117,22 +117,18 @@ def test_read_attributes_before_type_cast_on_boolean
end
def test_read_attributes_before_type_cast_on_datetime
- developer = Developer.find(:first)
- if current_adapter?(:Mysql2Adapter, :OracleAdapter)
- # Mysql2 and Oracle adapters keep the value in Time instance
- assert_equal developer.created_at.to_s(:db), developer.attributes_before_type_cast["created_at"].to_s(:db)
- else
- assert_equal developer.created_at.to_s(:db), developer.attributes_before_type_cast["created_at"].to_s
+ in_time_zone "Pacific Time (US & Canada)" do
+ record = @target.new
+
+ record.written_on = "345643456"
+ assert_equal "345643456", record.written_on_before_type_cast
+ assert_equal nil, record.written_on
+
+ record.written_on = "2009-10-11 12:13:14"
+ assert_equal "2009-10-11 12:13:14", record.written_on_before_type_cast
+ assert_equal Time.zone.parse("2009-10-11 12:13:14"), record.written_on
+ assert_equal ActiveSupport::TimeZone["Pacific Time (US & Canada)"], record.written_on.time_zone
end
-
- developer.created_at = "345643456"
-
- assert_equal developer.created_at_before_type_cast, "345643456"
- assert_equal developer.created_at, nil
-
- developer.created_at = "2010-03-21 21:23:32"
- assert_equal developer.created_at_before_type_cast, "2010-03-21 21:23:32"
- assert_equal developer.created_at, Time.parse("2010-03-21 21:23:32")
end
def test_hash_content

0 comments on commit 0823bbd

Please sign in to comment.
Something went wrong with that request. Please try again.