Check for Existing but nil :precision Option

[ahoglund]

Motivation / Background

The code to assign a default of precision to 6 in the timestamps method was removed in a previous PR, but it had a side effect of causing certain timestamp fields to be changed from datetime(6) to datetime when performing a schema migration in the 6.1 version of the migration class.

This is due to options[:precision] being set to nil here, and therefore not being set to the default of 6 later on in the code path.

This comment indicates that Rails 6.1 migrations should be precision: nil by default although it seems that wasn't actually the case. It was getting set to 6 in the timestamps method and therefore not being set to nil in the compatibility class.

Detail

This PR just adds back the precision assignment to the timestamps method.

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.

[sambostock]

I found that there is a possibility for the precision key to exist in the options hash due to it being added in automatically in lib/active_record/migration/compatibility.rb.

Could you provide a specific example scenario which produced undesired behavior? For example, is there an issue when migrating from an old Rails version?

[sambostock]

I'm not sure we want this behavior. If I'm not mistaken, the expected behavior is

t.datetime :name	Implicitly use Active Record's default precision: precision: 6 as of Rails 7 precision: nil prior to that
t.datetime :name, precision: 3	Explicitly use three decimals of precision.
t.datetime :name, precision: 0	Explicitly use zero decimals of precision.
t.datetime :name, precision: nil	Explicitly use whatever the database's default precision is.

[ahoglund]

Even in the case where you specify t.datetime :name the precision: nil option is getting added as an option here:

https://github.com/rails/rails/blob/main/activerecord/lib/active_record/migration/compatibility.rb#L138

So in the case where you don't specify something it will add the nil for precision and then subsequently use the database default rather than 6 as you state is what is expected for Rails 7.

Here is the column definition after being created in the TableDefinition:

"created_at"=>
 #<struct ActiveRecord::ConnectionAdapters::ColumnDefinition
  name="created_at",
  type=:datetime,
  options={:null=>false, :precision=>nil, :primary_key=>false},
  sql_type=nil>}

If I remove the options[:precision] = nil it will then see that i've not passed anything to t.timestamps and then use the Rails default of 6:

created_at      datetime(6)     NO              NULL
updated_at      datetime(6)     NO              NULL

[ahoglund]

@sambostock - Yeah, I think I jumped the gun on this PR before understanding fully what should be done. I've been testing this on the github monolith. I found that for certain tables the datetime precision is getting changed from datetime(6) to datetime when running migrations. The curious thing is that it's not all tables only a handful.

My latest attempt to troubleshoot was to revert these changes to the timestamps method commit. I tested our codebase against this and it fixed the problem. I still don't understand why that change had side effects. There must be a scenario later in the code path that is getting bypassed when attempting to set the precision. It’s not random but still not understood why only certain tables.