Skip to content

Commit

Permalink
Make before_type_cast available for datetime fields
Browse files Browse the repository at this point in the history 
Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information
@amatsuda @spastorino
amatsuda authored and spastorino committed Feb 1, 2011
1 parent 0448247 commit c8b7606
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 14 deletions.
5 changes: 3 additions & 2 deletions activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
View file
Expand Up @@ -37,12 +37,13 @@ def #{attr_name}(reload = false)
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}=(time)
def #{attr_name}=(original_time)
time = original_time.dup

This comment has been minimized.

Sign in to view

Copy link
@nragaz

nragaz Feb 6, 2011

Contributor

original_time.dup gets called even if original_time is nil, resulting in an exception (can't dup NilClass). An unless original_time.nil? would help.

This comment has been minimized.

Sign in to view

Copy link
@amatsuda

amatsuda Feb 6, 2011

Author Member

Oh, thanks @nragaz, that was my mistake...

This comment has been minimized.

Sign in to view

Copy link
@amatsuda

amatsuda Feb 6, 2011

Author Member 
From 9138bb1ad26d8b0c8a12722f9ac07e5c433f3f9f Mon Sep 17 00:00:00 2001
From: Akira Matsuda <ronnie@dio.jp>
Date: Mon, 7 Feb 2011 08:29:06 +0900
Subject: [PATCH] avoid nil.dup

---
 .../attribute_methods/time_zone_conversion.rb      |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
index a72eecb..76218d2 100644
--- a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
+++ b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
@@ -41,7 +41,7 @@ module ActiveRecord
             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
+                  time = original_time.dup unless original_time.nil?
                   unless time.acts_like?(:time)
                     time = time.is_a?(String) ? Time.zone.parse(time) : time.to_time rescue time
                   end
-- 
1.7.3.5

This comment has been minimized.

Sign in to view

Copy link
@nragaz

nragaz Feb 6, 2011

Contributor

Thank you! Was just about to do a patch myself.

This comment has been minimized.

Sign in to view

Copy link
@spastorino

spastorino Feb 7, 2011

Contributor

Can you provide a patch with a test?, thanks.

This comment has been minimized.

Sign in to view

Copy link
@stid

stid Feb 9, 2011

Guys, this was released (not patched) in 3.0.4, I'm getting this error in authologic when he try to update the last_login column after a new session/user is created and in a bunch of tests.

Update: Sorry, my mistake, I was using the 30-stable branch. This was not released in 3.0.4 final.

This comment has been minimized.

Sign in to view

Copy link
@alindeman

alindeman Feb 28, 2011

Contributor

This breaks when using the home_run gem since Date#dup (Date.allocate) is not implemented: "TypeError: allocator undefined for Date"

What exactly is being achieved by duping the Date? Is there another way this can be achieved?

This comment has been minimized.

Sign in to view

Copy link
@alindeman

alindeman Feb 28, 2011

Contributor

Actually, looks like it's being reported as a bug on home_run: https://github.com/jeremyevans/home_run/issues#issue/21

This comment has been minimized.

Sign in to view

Copy link
@adzap

adzap Mar 1, 2011

Contributor

Although that is an issue for home_run to fix, as dup is valid on a Date instance. I have submitted a ticket which removes the dup because it is not necessary. No in-place change is made to the time value which could effect the original_time value.

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6489-fix-before_type_cast-for-timezone-aware-datetime-attributes#ticket-6489-3
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)
write_attribute(:#{attr_name}, (time || original_time))
end
EOV
generated_attribute_methods.module_eval(method_body, __FILE__, line)
Expand Down
20 changes: 8 additions & 12 deletions activerecord/test/cases/attribute_methods_test.rb
View file
Expand Up @@ -119,20 +119,16 @@ def test_read_attributes_before_type_cast_on_boolean
unless current_adapter?(:Mysql2Adapter)
def test_read_attributes_before_type_cast_on_datetime
developer = Developer.find(:first)
# Oracle adapter returns Time before type cast
unless current_adapter?(:OracleAdapter)
assert_equal developer.created_at.to_s(:db) , developer.attributes_before_type_cast["created_at"]
else
assert_equal developer.created_at.to_s(:db) , developer.attributes_before_type_cast["created_at"].to_s(:db)
assert_equal developer.created_at.to_s(:db), developer.attributes_before_type_cast["created_at"].to_s

developer.created_at = "345643456"
assert_equal developer.created_at_before_type_cast, "345643456"
assert_equal developer.created_at, nil
developer.created_at = "345643456"

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
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.to_s, "2010-03-21 21:23:32"

This comment has been minimized.

Sign in to view

Copy link
@adzap

adzap Feb 7, 2011

Contributor

You do a to_s on the before_type_cast value which will convert the Time instance back to a string. So this does not test that a string value is stored for the before_type_cast when the original_time is a valid time string.

This comment has been minimized.

Sign in to view

Copy link
@amatsuda

amatsuda Feb 7, 2011

Author Member

You're absolutely right.
Just pushed the fix. 65e08cf
Thanks for your advice!

This comment has been minimized.

Sign in to view

Copy link
@adzap

adzap Feb 7, 2011

Contributor

No problem. But I am surprised that this change actually passes. From the code, if the time string is successfully parsed, the time variable will be Time instance. Therefore when (time || original_time) is evaluated, it will pass the time instance to be stored by write_attribute. So when you call before_type_cast you should get the same Time instance, and not the original string value.

Am I missing something?
assert_equal developer.created_at, Time.parse("2010-03-21 21:23:32")
end
end

Expand Down

1 comment on commit c8b7606

@adzap
Copy link
Contributor

@adzap adzap commented on c8b7606 Mar 1, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see this ticket for patches to fix this code and why the tests are broken.

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/6489-fix-before_type_cast-for-timezone-aware-datetime-attributes#ticket-6489-3

Please sign in to comment.