Fix change_column not setting precision for sqlite

There were a few 6.1 migration compatibility fixes in [previous][1]
[commits][2]. Most importantly, those commits reorganized some of the
compatibility tests to ensure that the tests would run against every
Migration version. To continue the effort of improving test coverage for
Migration compatibility, this commit converts tests for create_table and
change_column setting the correct precision on datetime columns.

While the create_table tests all pass, the change_column test did not
pass for 7.0 versioned Migrations on sqlite. This was due to the sqlite
adapter not using new_column_definition to set the options on the new
column (new_column_definition is where precision: 6 gets set if no
precision is specified). This happens because columns can't be modified
in place in sqlite and instead the whole table must be recreated and the
data copied. Before this commit, change_column would use the options
of the existing column as a base and merge in the exact options (and
type) passed to change_column.

This commit changes the change_column method to replace the existing
column without using the existing options. This ensures that precision:
6 is set consistently across adapters when change_column is used to
create a datetime column.

[1]: c2f838e
[2]: 9b07b2d

Author: skipkayhil

activerecord/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Fix `change_column` not setting `precision: 6` on `datetime` columns when
+    using 7.0+ Migrations and SQLite.
+
+    *Hartley McGuire*
+
 *   Support composite identifiers in `to_key`
 
     `to_key` avoids wrapping `#id` value into an `Array` if `#id` already an array

activerecord/lib/active_record/connection_adapters/sqlite3/schema_definitions.rb

@@ -5,6 +5,12 @@ module ConnectionAdapters
     module SQLite3
       # = Active Record SQLite3 Adapter \Table Definition
       class TableDefinition < ActiveRecord::ConnectionAdapters::TableDefinition
+        def change_column(column_name, type, **options)
+          name = column_name.to_s
+          @columns_hash[name] = nil
+          column(name, type, **options)
+        end
+
         def references(*args, **options)
           super(*args, type: :integer, **options)
         end

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -328,10 +328,7 @@ def change_column_null(table_name, column_name, null, default = nil) # :nodoc:
 
       def change_column(table_name, column_name, type, **options) # :nodoc:
         alter_table(table_name) do |definition|
-          definition[column_name].instance_eval do
-            self.type = aliased_types(type.to_s, type)
-            self.options.merge!(options)
-          end
+          definition.change_column(column_name, type, **options)
         end
       end
 

activerecord/test/cases/migration/compatibility_test.rb

@@ -336,22 +336,6 @@ def migrate(x)
         connection.drop_table :more_testings rescue nil
       end
 
-      def test_datetime_doesnt_set_precision_on_create_table
-        migration = Class.new(ActiveRecord::Migration[6.1]) {
-          def migrate(x)
-            create_table :more_testings do |t|
-              t.datetime :published_at
-            end
-          end
-        }.new
-
-        ActiveRecord::Migrator.new(:up, [migration], @schema_migration, @internal_metadata).migrate
-
-        assert connection.column_exists?(:more_testings, :published_at, **precision_implicit_default)
-      ensure
-        connection.drop_table :more_testings rescue nil
-      end
-
       def test_datetime_doesnt_set_precision_on_add_column_5_0
         migration = Class.new(ActiveRecord::Migration[5.0]) {
           def migrate(x)
@@ -376,28 +360,6 @@ def migrate(x)
         assert connection.column_exists?(:testings, :published_at, **precision_implicit_default)
       end
 
-      def test_datetime_doesnt_set_precision_on_change_column_6_1
-        create_migration = Class.new(ActiveRecord::Migration[6.1]) {
-          def migrate(x)
-            create_table :more_testings do |t|
-              t.date :published_at
-            end
-          end
-        }.new(nil, 0)
-
-        change_migration = Class.new(ActiveRecord::Migration[6.1]) {
-          def migrate(x)
-            change_column :more_testings, :published_at, :datetime
-          end
-        }.new(nil, 1)
-
-        ActiveRecord::Migrator.new(:up, [create_migration, change_migration], @schema_migration, @internal_metadata).migrate
-
-        assert connection.column_exists?(:more_testings, :published_at, **precision_implicit_default)
-      ensure
-        connection.drop_table :more_testings rescue nil
-      end
-
       def test_change_table_allows_if_exists_option_on_7_0
         migration = Class.new(ActiveRecord::Migration[7.0]) {
           def migrate(x)
@@ -723,6 +685,44 @@ def migrate(x)
     connection.drop_table :more_testings rescue nil
   end
 
+  def test_datetime_doesnt_set_precision_on_create_table
+    migration = Class.new(migration_class) {
+      def migrate(x)
+        create_table :more_testings do |t|
+          t.datetime :published_at
+        end
+      end
+    }.new
+
+    ActiveRecord::Migrator.new(:up, [migration], @schema_migration, @internal_metadata).migrate
+
+    assert connection.column_exists?(:more_testings, :published_at, **precision_implicit_default)
+  ensure
+    connection.drop_table :more_testings rescue nil
+  end
+
+  def test_datetime_doesnt_set_precision_on_change_column
+    create_migration = Class.new(migration_class) {
+      def migrate(x)
+        create_table :more_testings do |t|
+          t.date :published_at
+        end
+      end
+    }.new(nil, 0)
+
+    change_migration = Class.new(migration_class) {
+      def migrate(x)
+        change_column :more_testings, :published_at, :datetime
+      end
+    }.new(nil, 1)
+
+    ActiveRecord::Migrator.new(:up, [create_migration, change_migration], @schema_migration, @internal_metadata).migrate
+
+    assert connection.column_exists?(:more_testings, :published_at, **precision_implicit_default)
+  ensure
+    connection.drop_table :more_testings rescue nil
+  end
+
   private
     def precision_implicit_default
       if current_adapter?(:Mysql2Adapter, :TrilogyAdapter)
@@ -757,6 +757,46 @@ def migrate(x)
   ensure
     connection.drop_table :more_testings rescue nil
   end
+
+  def test_datetime_sets_precision_6_on_create_table
+    migration = Class.new(migration_class) {
+      def migrate(x)
+        create_table :more_testings do |t|
+          t.datetime :published_at
+        end
+      end
+    }.new
+
+    ActiveRecord::Migrator.new(:up, [migration], @schema_migration, @internal_metadata).migrate
+
+    assert connection.column_exists?(:more_testings, :published_at, precision: 6)
+  ensure
+    connection.drop_table :more_testings rescue nil
+  end
+
+  def test_datetime_sets_precision_6_on_change_column
+    create_migration = Class.new(migration_class) {
+      def migrate(x)
+        create_table :more_testings do |t|
+          t.date :published_at
+        end
+      end
+    }.new(nil, 0)
+
+    $global = true
+
+    change_migration = Class.new(migration_class) {
+      def migrate(x)
+        change_column :more_testings, :published_at, :datetime
+      end
+    }.new(nil, 1)
+
+    ActiveRecord::Migrator.new(:up, [create_migration, change_migration], @schema_migration, @internal_metadata).migrate
+
+    assert connection.column_exists?(:more_testings, :published_at, precision: 6)
+  ensure
+    connection.drop_table :more_testings rescue nil
+  end
 end
 
 class BaseCompatibilityTest < ActiveRecord::TestCase