Incorrect changed for datetimes in mysql adapters #6975

Closed
pacoguzman opened this Issue Jul 5, 2012 · 14 comments

Comments

Projects
None yet
5 participants
Contributor

pacoguzman commented Jul 5, 2012

Hi guys, I've found a similar issue of #6591 in current rails 3-2-stable. Adding the following test to activerecord/test/cases/dirty_test.rb

  def test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change
    in_time_zone 'Paris' do
      target = Class.new(ActiveRecord::Base)
      target.table_name = 'pirates'

      created_on = Time.now

      pirate = target.create(:created_on => created_on)
      pirate.reload # Here mysql truncate the usec value to 0

      pirate.created_on = created_on
      assert !pirate.created_on_changed?
    end
  end

I getting the following:

ARCONN=mysql2 ruby -Itest test/cases/dirty_test.rb -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_changeUsing mysql2 with Identity Map off
Run options: -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change

# Running tests:

F

Finished tests in 0.087599s, 11.4157 tests/s, 11.4157 assertions/s.

  1) Failure:
test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change(DirtyTest) [test/cases/dirty_test.rb:104]:
Failed assertion, no message given.

1 tests, 1 assertions, 1 failures, 0 errors, 0 skips
ARCONN=sqlite3 ruby -Itest test/cases/dirty_test.rb -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change
Using sqlite3 with Identity Map off
Run options: -n test_setting_time_attributes_with_time_zone_field_to_same_time_should_not_be_marked_as_a_change

# Running tests:

.

Finished tests in 0.058396s, 17.1245 tests/s, 17.1245 assertions/s.

1 tests, 1 assertions, 0 failures, 0 errors, 0 skips

I have a clue in why this is happening due to the lack of precision of the Mysql databases. Mysql does not save micro seconds

Thanks in advance

Contributor

kennyj commented Jul 5, 2012

? I tested on master and 3-2-stable. But test wasn't success on mysql, mysql2, postgresql and sqlite3.

$ ruby -v
ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-linux]

Contributor

kennyj commented Jul 5, 2012

You are right. Certainly, this issue is usec problem.
It seems that I fixed by kennyj/rails@b426808 .

But I don't know which should we round timestamps... wdyt?

Contributor

pacoguzman commented Jul 5, 2012

The test is success in sqlite3 without any change in

ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-darwin11.3.0]

Did you say it's success in sqlite3?

Anyway, If this is happening in all those adapters I think the round is justified. If only happened with some or one adapter maybe we have to find other solution that apply only in those cases.

Other thing I've notice is that round is not defined in 1.8.7 we need to use change(:usec => 0) so the code's going to be more complex

Contributor

kennyj commented Jul 5, 2012

No, it was not success in sqlite3. Odd...

I don't know 1.8.7 hasn't round method. Thanks.
I'm going to fix it after work.

Contributor

kennyj commented Jul 6, 2012

I updated this.
kennyj/rails@aaaeb19 .

@rafaelfranca wdyt ? I don't know we should round usec.

Contributor

pacoguzman commented Jul 6, 2012

@kennyj great!! Thanks

Owner

rafaelfranca commented Jul 6, 2012

@kennyj I commented in the commit, but I think you should open a pull request. I want to take some feedback from @tenderlove.

Contributor

kennyj commented Jul 6, 2012

@rafaelfranca thanks! I updated it (and added one private method), and sent PR #6986.

@rafaelfranca rafaelfranca added a commit that referenced this issue Aug 21, 2012

@rafaelfranca rafaelfranca Merge pull request #6986 from kennyj/fix_6975
Fix #6975. Round usec when writing timestamp attribute.

Conflicts:
	activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb
8905c1f
Owner

pixeltrix commented Jan 21, 2013

Reopening this because the fixes resulted in #8460

pixeltrix reopened this Jan 21, 2013

pixeltrix was assigned Jan 21, 2013

Owner

rafaelfranca commented Jan 21, 2013

Ok. Let me know if you want some help

Owner

fxn commented Jan 22, 2013

I believe the fix should be reverted. Some databases store microseconds, some don't. Code that relies on the precision is database dependent and should program to that contract.

In my opinion the rounding patch should be reverted.

Owner

pixeltrix commented Jan 22, 2013

Reverted and regression test added to 3-2-stable and master.

pixeltrix closed this Jan 22, 2013

Owner

pixeltrix commented Jan 22, 2013

@pacoguzman unfortunately this is an unfortunate fact of life when dealing with MySQL. We can't even special case it for MySQL since there are situations where you would want to track fractional second changes such as cache keys even if it eventually gets rounded off when written to the database. Sorry, you just need to handle it in your application and tests in a way that suits you best.

Contributor

pacoguzman commented Jan 23, 2013

No problem, we'll try to handle it in our application, thanks

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