Fix erroneous nil default precision on virtual datetime columns #46110

sambostock · 2022-09-22T21:15:07Z

Motivation / Background

Prior to this change, virtual datetime columns did not have the same default precision as regular datetime columns, resulting in the following being erroneously equivalent:

t.virtual :name, type: datetime,                 as: "expression"
t.virtual :name, type: datetime, precision: nil, as: "expression"

This change fixes the default precision lookup, so virtual and regular datetime column default precisions match.

Detail

The existing code paths look roughly like:

t.datetime is delegated to the column method
- column sees type == :datetime and sets a default precision is none was given
- column calls new_column_definition to add the result to @columns_hash
t.virtual is delegated to the column method
- column sees type == :virtual, not :datetime, so skips the default lookup
- column calls new_column_definition to add the result to @columns_hash
  - new_column_definition sees type == :virtual, so sets type = options[:type]

This PR moves the location of the default lookup, resulting in the following code paths:

t.datetime is delegated to the column method
- column calls new_column_definition to add the result to @columns_hash
  - new_column_definition sees type == :datetime and sets a default precision is none was given
t.virtual is delegated to the column method
- column calls new_column_definition to add the result to @columns_hash
  - new_column_definition sees type == :virtual, so sets type = options[:type]
  - new_column_definition sees type == :datetime and sets a default precision is none was given

Additional information

In our application, we use LHM to make schema changes to existing tables. In LHM, migrations describe columns in SQL:

change_table :things do |m|
  m.add_column(:my_column, "DATETIME(6) AS (expression)")

The above LHM results in the following schema entry, as expected:

t.virtual "my_column", type: :datetime, as: "expression"

However, if a developer runs a command that loads the schema from db/schema.rb (e.g. rails db:schema:load or rails db:reset), the column is silently created with the incorrect precision. If they later run a command that dumps the schema (e.g. rails db:schema:dump, or rails db:migrate), the file is updated to reflect the corrupted precision:

t.virtual "my_column", type: :datetime, precision: nil, as: "expression"

We only noticed this bug because migrations started causing unrelated changes to the schema file. It is possible that others have run into this bug with regular migrations, but may not have noticed the precision mismatch.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
There are no typos in commit messages and comments.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Feature branch is up-to-date with main (if not - rebase it).
Pull request only contains one commit for bug fixes and small features. If it's a larger feature, multiple commits are permitted but must be descriptive.
Tests are added if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
PR is not in a draft state.
CI is passing.

activerecord/CHANGELOG.md

sambostock · 2022-09-22T21:55:20Z

activerecord/test/cases/migration/change_schema_test.rb

@@ -286,7 +286,7 @@ def test_add_column_with_timestamp_type
        elsif current_adapter?(:OracleAdapter)
          assert_equal "TIMESTAMP(6)", column.sql_type
        else
-          assert_equal connection.type_to_sql("datetime"), column.sql_type
+          assert_equal connection.type_to_sql("datetime(6)"), column.sql_type


I'm not sure about this change, but it gets CI passing again

This is concerning. Do you know you needed this change?

No. I made the change to see if it would fail any other tests, but it didn't. I'm not familiar with these tests, and haven't had a chance to deep dive and figure out why it was required.

I think the SQL generated on SQLite3 for the t.column :foo, :timestamp now includes the precision of 6, when before it didn't. We need to investigate why this happened before merging it. I think this is the right behavior, but it is a behavior change, so we need to make sure the previous behavior was indeed a bug.

Got it. This is indeed the right behavior.

Now you moved the code to add new default precision to after aliased_types is called. So :timestamp becomes :datetime and the new default is used.

For posterity, here is a link to the failing build.

I've figured out why it was failing, and it's a similar reason to why this bug existed:

The incorrect virtual datetime precision was because the default assignment happened before we finalized the type from virtual to datetime. Similarly, alias resolution happened after default precision assignment, so timestamp type columns implemented as datetime were using nil precision as their default instead of 6.

So

if that behavior is not expected, then this PR introduces a bug, but

if that behavior is expected, then this PR fixes both bugs.

Based on the description of #42297, I believe this is intended:

Set precision 6 by default for datetime columns, this is the standard behaviour for timestamps #34970 it would good to extend this to all datetimes by default.

In fact, I think we have an opportunity to refactor and remove duplicate code in the t.timestamps path, since we apply the default precision to all datetime columns.

Edit: I must not have refreshed and missed your replies. Glad we both independently confirmed the expected behavior. 👍

Active Record erroneously uses a precision of `nil` instead of the default precision, when adding a virtual datetime column. This tests that behavior ahead of fixing it.

Prior to this change t.virtual :column_name, type: :datetime would erroneously produce the same result as t.virtual :column_name, type: :datetime, precision: nil This is because the code path for `virtual` skipped the default lookup: - `t.datetime` is delegated to the `column` method - `column` sees `type == :datetime` and sets a default `precision` is none was given - `column` calls `new_column_definition` to add the result to `@columns_hash` - `t.virtual` is delegated to the `column` method - `column` sees `type == :virtual`, not `:datetime`, so skips the default lookup - `column` calls `new_column_definition` to add the result to `@columns_hash` - `new_column_definition` sees `type == :virtual`, so sets `type = options[:type]` By moving the default lookup, we get consistent code paths: - `t.datetime` is delegated to the `column` method - `column` calls `new_column_definition` to add the result to `@columns_hash` - `new_column_definition` sees `type == :datetime` and sets a default `precision` is none was given - `t.virtual` is delegated to the `column` method - `column` calls `new_column_definition` to add the result to `@columns_hash` - `new_column_definition` sees `type == :virtual`, so sets `type = options[:type]` - `new_column_definition` sees `type == :datetime` and sets a default `precision` is none was given

…cision Fix erroneous nil default precision on virtual datetime columns

In the [backport][1] I put the tile for #46108 instead of #46110. [1]: 5ffa8f4

Since Rails 7.0.5, `Timestamp` columns use precision 6 by default. We need to specify the same precision to work `CURRENT_TIMESTAMP` correctly. Ref: rails/rails#46110

rails-bot bot added the activerecord label Sep 22, 2022

rafaelfranca reviewed Sep 22, 2022

View reviewed changes

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved

sambostock commented Sep 22, 2022

View reviewed changes

sambostock added 2 commits September 22, 2022 17:55

Add test for existing virtual datetime precision

919dd4a

Active Record erroneously uses a precision of `nil` instead of the default precision, when adding a virtual datetime column. This tests that behavior ahead of fixing it.

sambostock force-pushed the virtual-datetime-column-precision branch from e889868 to b85ee91 Compare September 22, 2022 21:58

rafaelfranca merged commit dd163f8 into rails:main Sep 22, 2022

rafaelfranca added a commit that referenced this pull request Sep 22, 2022

Merge pull request #46110 from sambostock/virtual-datetime-column-pre…

5ffa8f4

…cision Fix erroneous nil default precision on virtual datetime columns

sambostock deleted the virtual-datetime-column-precision branch September 22, 2022 22:32

sambostock mentioned this pull request Sep 22, 2022

Remove duplicate datetime precision default #46112

Merged

7 tasks

ahoglund mentioned this pull request Sep 27, 2022

Check for Existing but nil :precision Option #46140

Merged

9 tasks

sambostock mentioned this pull request Oct 20, 2022

Applications using Sprockets, Rails UJS, but not CoffeeScript cannot consume Rails via git prior to Rails UJS abandoning CoffeeScript #46292

Closed

fatkodima mentioned this pull request Jan 7, 2023

change_table alters default precision for datetime columns #46889

Closed

rafaelfranca added a commit that referenced this pull request May 25, 2023

Fix CHANGELOG entry for Active Record

682898f

In the [backport][1] I put the tile for #46108 instead of #46110. [1]: 5ffa8f4

aidanharan mentioned this pull request May 29, 2023

Tests failing following release of Rails v7.0.5 rails-sqlserver/activerecord-sqlserver-adapter#1056

Closed

inductor mentioned this pull request Jun 6, 2023

update rails cloudnativedaysjp/dreamkast#1931

Merged

aidanharan mentioned this pull request Jun 8, 2023

Set precision 6 by default for timestamps rails-sqlserver/activerecord-sqlserver-adapter#1057

Merged

y-yagi mentioned this pull request Jun 13, 2023

Fix broken spec with Rails 7.0.5 ridgepole/ridgepole#420

Merged

yuyat137 mentioned this pull request Jul 14, 2023

下書き yuyat137/Memo#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix erroneous nil default precision on virtual datetime columns #46110

Fix erroneous nil default precision on virtual datetime columns #46110

sambostock commented Sep 22, 2022 •

edited

sambostock Sep 22, 2022

rafaelfranca Sep 22, 2022

sambostock Sep 22, 2022

rafaelfranca Sep 22, 2022

rafaelfranca Sep 22, 2022

sambostock Sep 22, 2022 •

edited

Fix erroneous nil default precision on virtual datetime columns #46110

Fix erroneous nil default precision on virtual datetime columns #46110

Conversation

sambostock commented Sep 22, 2022 • edited

Motivation / Background

Detail

Additional information

Checklist

sambostock Sep 22, 2022

Choose a reason for hiding this comment

rafaelfranca Sep 22, 2022

Choose a reason for hiding this comment

sambostock Sep 22, 2022

Choose a reason for hiding this comment

rafaelfranca Sep 22, 2022

Choose a reason for hiding this comment

rafaelfranca Sep 22, 2022

Choose a reason for hiding this comment

sambostock Sep 22, 2022 • edited

Choose a reason for hiding this comment

sambostock commented Sep 22, 2022 •

edited

sambostock Sep 22, 2022 •

edited