New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AR: take precision into count when assigning a value to timestamp attribute #20317

Merged
merged 1 commit into from Sep 23, 2015

Conversation

Projects
None yet
3 participants
@bogdan
Contributor

bogdan commented May 27, 2015

The Problem

Consider this following on MySQL (that have 0 precision on datetime by default):

d1 = Developer.create!(name: 'bogdan')
d2 = Developer.find(d1.id)
d1.created_at.usec # => 837728
d2.created_at.usec # => 0
# It results in a madness:
d1.created_at == d2.created_at # => false
d1.cache_key == d2.cache_key # => false

Rails 4.1.10 and master are affected. Not sure about others.

Timestamp column can have less precision than ruby timestamp
In result in how big a fraction of a second can be stored in the
database.

If the precision is low enough, (mysql default is 0 so it is always low
enough by default) the value changes when model is reloaded from the
database.

The Patch

In order to fix that we just use a precision of a column when assigning a value to the timestamp attribute as time object (not as string - that seems working).

Fixing tests

When I've applied a patch, the new problem raised: existing tests are written incorrectly for mysql that have 0 precision by default.
There are many tests that check that updated_at changes on particular model. But it is only true for ruby level but not in the database. This patch reveals the problem.

In order to make them pass I've set the precision for tables that are used to check if updated_at is changing big enough and consistent for all databases.

Further problems

datetime column type works differently in different databases now because if different default precision. It results in different behaviour of the same AR schema on different databases.

It should be consistent. It can be done by setting up a precision option of add_column to some value all the time and disallowing it to be nil.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 27, 2015

Member

Tests are broken because datetime precision is not supported at the version of mysql that we have in our servers. This make me wonder, would this patch work on old MySQL versions?

Member

rafaelfranca commented May 27, 2015

Tests are broken because datetime precision is not supported at the version of mysql that we have in our servers. This make me wonder, would this patch work on old MySQL versions?

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 27, 2015

Contributor

@rafaelfranca yes it will: at the rails level, unsupported precision works like precision # => 0. It means that all seconds fractions will be removed.

The fact that older mysql don't support precision raises the following problem:

  def test_cache_key_changes_when_child_touched
    owner = owners(:blackbeard)
    pet   = pets(:parrot)

    owner.update_column :updated_at, Time.current
    key = owner.cache_key

    assert pet.touch
    assert_not_equal key, owner.reload.cache_key
  end

It will only work stable if there will be at least 1 second between the creation of owner and touch of pet. It can be solved by putting sleep(1) there or by marking as pending on old versions of mysql.

I've put sleep(1) initially but it didn't work because too many tests would require it and they end up being very slow.

I'll think more how we can solve it. If you have an idea, please share with me.

Contributor

bogdan commented May 27, 2015

@rafaelfranca yes it will: at the rails level, unsupported precision works like precision # => 0. It means that all seconds fractions will be removed.

The fact that older mysql don't support precision raises the following problem:

  def test_cache_key_changes_when_child_touched
    owner = owners(:blackbeard)
    pet   = pets(:parrot)

    owner.update_column :updated_at, Time.current
    key = owner.cache_key

    assert pet.touch
    assert_not_equal key, owner.reload.cache_key
  end

It will only work stable if there will be at least 1 second between the creation of owner and touch of pet. It can be solved by putting sleep(1) there or by marking as pending on old versions of mysql.

I've put sleep(1) initially but it didn't work because too many tests would require it and they end up being very slow.

I'll think more how we can solve it. If you have an idea, please share with me.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented May 28, 2015

cc @sgrif

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jun 13, 2015

Contributor

Hope I've fixed the build on mysql5.6: I made it differently with travel so that tests don't relay on ruby-is-slow.

Can someone advice how to go from PR to corresponding CI build? Find it here: https://travis-ci.org/rails/rails/pull_requests manually seems impossible.

Contributor

bogdan commented Jun 13, 2015

Hope I've fixed the build on mysql5.6: I made it differently with travel so that tests don't relay on ruby-is-slow.

Can someone advice how to go from PR to corresponding CI build? Find it here: https://travis-ci.org/rails/rails/pull_requests manually seems impossible.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jul 15, 2015

Contributor

@sgrif @rafaelfranca I understand the work load of rails core team, but this is regression problem from 4.0 to 4.1 and we kind of expect a feedback sooner than later. Can you please merge this in and backport to 4.1 or point out why it can not be done?

Contributor

bogdan commented Jul 15, 2015

@sgrif @rafaelfranca I understand the work load of rails core team, but this is regression problem from 4.0 to 4.1 and we kind of expect a feedback sooner than later. Can you please merge this in and backport to 4.1 or point out why it can not be done?

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 15, 2015

Member

Forcing a Travis build.

Member

sgrif commented Jul 15, 2015

Forcing a Travis build.

@sgrif sgrif closed this Jul 15, 2015

@sgrif sgrif reopened this Jul 15, 2015

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jul 15, 2015

Member

I'm traveling and don't have my computer right now but I'll take care of this next chance I get.

Member

sgrif commented Jul 15, 2015

I'm traveling and don't have my computer right now but I'll take care of this next chance I get.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Sep 21, 2015

Contributor

@sgrif if you have back to work, it would be cool to receive another review here

Contributor

bogdan commented Sep 21, 2015

@sgrif if you have back to work, it would be cool to receive another review here

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 21, 2015

Member

Are you Github stalking me? :trollface: Taking a look now.

Member

sgrif commented Sep 21, 2015

Are you Github stalking me? :trollface: Taking a look now.

Show outdated Hide outdated activerecord/test/cases/timestamp_test.rb
in_instance = d.created_at
from_db = Parrot.find(d.id).created_at
assert_equal from_db.usec, in_instance.usec
assert_equal from_db, in_instance

This comment has been minimized.

@sgrif

sgrif Sep 21, 2015

Member

I find this test hard to follow, and I don't think this is relevant to the behavior of AR::Timestamp. Can we move this to a separate test in something like test/cases/type/date_time_test.rb, and have it look something like this?

model = Model.new(date_time_attr_that_isnt_auto_touched: Time.now)
assert_equal model.date_time_attr, model.tap(&:save).reload.date_time_attr
@sgrif

sgrif Sep 21, 2015

Member

I find this test hard to follow, and I don't think this is relevant to the behavior of AR::Timestamp. Can we move this to a separate test in something like test/cases/type/date_time_test.rb, and have it look something like this?

model = Model.new(date_time_attr_that_isnt_auto_touched: Time.now)
assert_equal model.date_time_attr, model.tap(&:save).reload.date_time_attr

This comment has been minimized.

@bogdan

bogdan Sep 22, 2015

Contributor

I don't see any test like that inside am/test/cases/type/ that uses Model. They all just instantiate the AM::Type::* object and call #cast method there.

Also, testing this feature in AM will not cover this change:
https://github.com/rails/rails/pull/20317/files#diff-2695559f74d901580d9c9314a9f4aff7R14
that caused postgresql specific bug.

I would stick with test we have know. I will only change it with use of #reload.

@bogdan

bogdan Sep 22, 2015

Contributor

I don't see any test like that inside am/test/cases/type/ that uses Model. They all just instantiate the AM::Type::* object and call #cast method there.

Also, testing this feature in AM will not cover this change:
https://github.com/rails/rails/pull/20317/files#diff-2695559f74d901580d9c9314a9f4aff7R14
that caused postgresql specific bug.

I would stick with test we have know. I will only change it with use of #reload.

This comment has been minimized.

@sgrif

sgrif Sep 22, 2015

Member

If it uses a model, create a test in the active record namespace.

@sgrif

sgrif Sep 22, 2015

Member

If it uses a model, create a test in the active record namespace.

This comment has been minimized.

@sgrif

sgrif Sep 22, 2015

Member

There are already several tests of this nature in activerecord/test/cases/type

@sgrif

sgrif Sep 22, 2015

Member

There are already several tests of this nature in activerecord/test/cases/type

Show outdated Hide outdated activerecord/lib/active_record/type/helpers/time_value.rb
@@ -20,6 +16,13 @@ def serialize(value)
value
end
def apply_precision(value)

This comment has been minimized.

@sgrif

sgrif Sep 21, 2015

Member

What do you think about naming this apply_nanosecond_precision instead?

@sgrif

sgrif Sep 21, 2015

Member

What do you think about naming this apply_nanosecond_precision instead?

This comment has been minimized.

@sgrif

sgrif Sep 21, 2015

Member

Or maybe normalize_subsecond_precision?

@sgrif

sgrif Sep 21, 2015

Member

Or maybe normalize_subsecond_precision?

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Sep 21, 2015

Member

A few nitpicks. This will need to be rebased. The relevant classes have been moved to Active Model.

Member

sgrif commented Sep 21, 2015

A few nitpicks. This will need to be rebased. The relevant classes have been moved to Active Model.

Fixed taking precision into count when assigning a value to timestamp…
… attribute

Timestamp column can have less precision than ruby timestamp
In result in how big a fraction of a second can be stored in the
database.

  m = Model.create!
  m.created_at.usec == m.reload.created_at.usec
    # => false
    # due to different seconds precision in Time.now and database column

If the precision is low enough, (mysql default is 0, so it is always low
enough by default) the value changes when model is reloaded from the
database. This patch fixes that issue ensuring that any timestamp
assigned as an attribute is converted to column precision under the
attribute.
@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Sep 23, 2015

Contributor

You are right about that test. I didn't get what you meant from the first try.

So:

  • Renamed method to apply_seconds_precision. Feel like it is the best.
  • Reworked test to be in proper place and don't depend on autoupdated time stamps.
Contributor

bogdan commented Sep 23, 2015

You are right about that test. I didn't get what you meant from the first try.

So:

  • Renamed method to apply_seconds_precision. Feel like it is the best.
  • Reworked test to be in proper place and don't depend on autoupdated time stamps.

@sgrif sgrif merged commit d03f519 into rails:master Sep 23, 2015

sgrif added a commit that referenced this pull request Sep 23, 2015

Merge pull request #20317
AR: take precision into count when assigning a value to timestamp
attribute

sgrif added a commit that referenced this pull request Sep 23, 2015

Don't attempt to specify datetime precision unless supported
Specifically, versions of MySQL prior to 5.6 do not support this, which
is what's used on Travis by default. The method `mysql_56?` appeared to
only ever be used to conditionally apply subsecond precision, so I've
generalized it and used it more liberally.

This should fix the test failures caused by #20317

sgrif added a commit that referenced this pull request Sep 23, 2015

Don't rely on subsecond precision being applied in tests
When I originally reviewed the #20317, I believe these changes were
present, but it appears that it was later updated so that they were
removed. Since Travis hadn't re-run the build, this slipped through.

@tarzan tarzan referenced this pull request Oct 6, 2015

Merged

Fix precision on cache_key #21883

sgrif added a commit to sgrif/rails that referenced this pull request Jul 25, 2016

Partially backport #20317
The patch does not apply at all cleanly, so this is a manual backport of
the most relevant bits

sgrif added a commit to sgrif/rails that referenced this pull request Jul 25, 2016

Don't attempt to specify datetime precision unless supported
Specifically, versions of MySQL prior to 5.6 do not support this, which
is what's used on Travis by default. The method `mysql_56?` appeared to
only ever be used to conditionally apply subsecond precision, so I've
generalized it and used it more liberally.

This should fix the test failures caused by #20317

pixeltrix added a commit that referenced this pull request Mar 11, 2018

Apply time column precision on assignment
In #20317, datetime columns had their precision applied on assignment but
that behaviour wasn't applied to time columns - this commit fixes that.

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