Fix #6975. Round usec when writing timestamp attribute. #6986

Closed
wants to merge 1 commit into
from

3 participants

@kennyj

Please see #6975.
I think we should also merge to 3-2-stable.

cc/ @tenderlove @rafaelfranca

@rafaelfranca
Ruby on Rails member

@tenderlove could you review this one?

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Aug 15, 2012
...tive_record/attribute_methods/time_zone_conversion.rb
@@ -61,11 +61,14 @@ def #{attr_name}=(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
- changed = read_attribute(:#{attr_name}) != time
- write_attribute(:#{attr_name}, original_time)
- #{attr_name}_will_change! if changed
- @attributes_cache["#{attr_name}"] = time
+ time = round_usec(time.in_time_zone) rescue nil if time
+ attr = round_usec(read_attribute(:#{attr_name}))
+ if !time || !attr || attr != time
+ original = original_time.acts_like?(:time) ? round_usec(original_time) : original_time
+ write_attribute(:#{attr_name}, original)
@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Aug 15, 2012

I think we should not write the rounded time in the database, we should only round they to compare. WDYT?

@kennyj
kennyj added a line comment Aug 16, 2012

I can't judge it.
IMHO, that behavior is a little tricky...

@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Aug 18, 2012

Could you try to write the not rounded time in the database and in the attributes_cache and see if the tests will still pass

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

@rafaelfranca I updated this PR. All tests are green.

@tenderlove tenderlove and 1 other commented on an outdated diff Aug 21, 2012
...tive_record/attribute_methods/time_zone_conversion.rb
@@ -59,11 +59,14 @@ def #{attr_name}=(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
- changed = read_attribute(:#{attr_name}) != time
- write_attribute(:#{attr_name}, original_time)
- #{attr_name}_will_change! if changed
- @attributes_cache["#{attr_name}"] = time
+ zoned_time = time ? time.in_time_zone : nil rescue nil
+ rounded_time = round_usec(zoned_time)
+ attr = round_usec(read_attribute(:#{attr_name}))
@tenderlove
Ruby on Rails member
tenderlove added a line comment Aug 21, 2012

Don't convert to a symbol here, it just gets converted back to a string.

@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Aug 21, 2012

maybe rounded_current_value or something like this is a better name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove commented on an outdated diff Aug 21, 2012
...tive_record/attribute_methods/time_zone_conversion.rb
@@ -59,11 +59,14 @@ def #{attr_name}=(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
- changed = read_attribute(:#{attr_name}) != time
- write_attribute(:#{attr_name}, original_time)
- #{attr_name}_will_change! if changed
- @attributes_cache["#{attr_name}"] = time
+ zoned_time = time ? time.in_time_zone : nil rescue nil
@tenderlove
Ruby on Rails member
tenderlove added a line comment Aug 21, 2012

I think this can be time && time.in_time_zone rescue nil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tenderlove tenderlove commented on an outdated diff Aug 21, 2012
...tive_record/attribute_methods/time_zone_conversion.rb
@@ -79,6 +82,11 @@ def create_time_zone_conversion_attribute?(name, column)
[:datetime, :timestamp].include?(column.type)
end
end
+
+ private
+ def round_usec(value)
+ !value ? nil : value.change(:usec => 0)
@tenderlove
Ruby on Rails member
tenderlove added a line comment Aug 21, 2012

Also value && value.change(:usec => 0), thought it might be more clear to just do:

return unless value
value.change(:usec => 0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca commented on an outdated diff Aug 21, 2012
...tive_record/attribute_methods/time_zone_conversion.rb
@@ -59,11 +59,14 @@ def #{attr_name}=(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
- changed = read_attribute(:#{attr_name}) != time
- write_attribute(:#{attr_name}, original_time)
- #{attr_name}_will_change! if changed
- @attributes_cache["#{attr_name}"] = time
+ zoned_time = time ? time.in_time_zone : nil rescue nil
+ rounded_time = round_usec(zoned_time)
+ attr = round_usec(read_attribute(:#{attr_name}))
+ if !rounded_time || !attr || attr != rounded_time
@rafaelfranca
Ruby on Rails member
rafaelfranca added a line comment Aug 21, 2012

I think here should be:

if (attr != rounded_time) || (!attr && original_time)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kennyj

@tenderlove @rafaelfranca Thanks for many comments. I rebased and fixed some codes.

@rafaelfranca rafaelfranca added a commit that referenced this pull request Aug 21, 2012
@rafaelfranca rafaelfranca Add CHANGELOG entry for #6986 55d943c
@sferik

I have a concern that this patch does not do what it claims. In my testing, it seems to round times to the nearest whole second, not the nearest microsecond. Since this was merged into 3-2-stable, it has created an incompatibility between versions 3.2.8 and 3.2.9. 😦

@rafaelfranca
Ruby on Rails member

@sferik the data is not changed, we round the value only to compare.

Yes we round to the nearest second because MySQL already does this when store the value.

This behavior in 3.2.8 was buggy since 3.2.6 and now it seems fixed, but if you think it is wrong please give us some way to reproduce your issue.

@sferik

If this behavior is specific to MySQL, doesn't it belong in the MySQL adapter?

Here is an example of the problems caused by this change:

class Parent < ActiveRecord::Base
  has_one :child
end

class Child < ActiveRecord::Base
  belongs_to :parent, :touch => true
end

setup do
  @parent = Parent.create
  Child.create(:parent => @parent)
end

def test_cache_key_changes_when_child_touched
  key = @parent.cache_key
  @parent.child.touch
  @parent.reload
  assert_not_equal key, @parent.cache_key # F
end

The only way I can make this test pass (without monkey patching) is by adding sleep 1 before @object.child.touch. 😞

@rafaelfranca
Ruby on Rails member

@sferik now I see your problem. Could you open a new issue? I'll investigate

@pixeltrix pixeltrix added a commit that referenced this pull request Jan 22, 2013
@pixeltrix pixeltrix Revert "Merge pull request #6986 from kennyj/fix_6975"
This reverts commit 8905c1f.

Closes #8460

Conflicts:
	activerecord/test/cases/dirty_test.rb
97a4db9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment