Don't store ddl on schema definitions

We'll ensure that any consumers that require ddl obtain it by visiting
the schema definition. Consequently, we should also stop visiting the
schema definition in the "schema definition builder" methods -- callers
will need to both build a schema definition, and then visit it using a
SchemaCreation object. This means schema definitions will not be populated
with SQL type information, or any other information that is set when
the definition is visited.

Author: adrianna-chang-shopify

activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb

@@ -26,7 +26,6 @@ def visit_AlterTable(o)
           sql << o.foreign_key_drops.map { |fk| visit_DropForeignKey fk }.join(" ")
           sql << o.check_constraint_adds.map { |con| visit_AddCheckConstraint con }.join(" ")
           sql << o.check_constraint_drops.map { |con| visit_DropCheckConstraint con }.join(" ")
-          o.ddl = sql
         end
 
         def visit_ColumnDefinition(o)
@@ -67,7 +66,7 @@ def visit_TableDefinition(o)
           create_sql << "(#{statements.join(', ')})" if statements.present?
           add_table_options!(create_sql, o)
           create_sql << " AS #{to_sql(o.as)}" if o.as
-          o.ddl = create_sql
+          create_sql
         end
 
         def visit_PrimaryKeyDefinition(o)
@@ -107,8 +106,7 @@ def visit_CreateIndexDefinition(o)
           sql << "(#{quoted_columns(index)})"
           sql << "WHERE #{index.where}" if supports_partial_index? && index.where
 
-          sql = sql.join(" ")
-          o.ddl = sql
+          sql.join(" ")
         end
 
         def visit_CheckConstraintDefinition(o)

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

@@ -92,11 +92,11 @@ def aliased_types(name, fallback)
 
     AddColumnDefinition = Struct.new(:column) # :nodoc:
 
-    ChangeColumnDefinition = Struct.new(:column, :name, :ddl) # :nodoc:
+    ChangeColumnDefinition = Struct.new(:column, :name) # :nodoc:
 
-    ChangeColumnDefaultDefinition = Struct.new(:column, :default, :ddl) # :nodoc:
+    ChangeColumnDefaultDefinition = Struct.new(:column, :default) # :nodoc:
 
-    CreateIndexDefinition = Struct.new(:index, :algorithm, :if_not_exists, :ddl) # :nodoc:
+    CreateIndexDefinition = Struct.new(:index, :algorithm, :if_not_exists) # :nodoc:
 
     PrimaryKeyDefinition = Struct.new(:name) # :nodoc:
 
@@ -335,7 +335,6 @@ class TableDefinition
       include ColumnMethods
 
       attr_reader :name, :temporary, :if_not_exists, :options, :as, :comment, :indexes, :foreign_keys, :check_constraints
-      attr_accessor :ddl
 
       def initialize(
         conn,
@@ -582,7 +581,6 @@ class AlterTable # :nodoc:
       attr_reader :adds
       attr_reader :foreign_key_adds, :foreign_key_drops
       attr_reader :check_constraint_adds, :check_constraint_drops
-      attr_accessor :ddl
 
       def initialize(td)
         @td   = td

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

@@ -299,7 +299,7 @@ def create_table(table_name, id: :primary_key, primary_key: nil, force: nil, **o
           schema_cache.clear_data_source_cache!(table_name.to_s)
         end
 
-        result = execute(td.ddl)
+        result = execute schema_creation.accept(td)
 
         unless supports_indexes_in_create?
           td.indexes.each do |column_name, index_options|
@@ -329,7 +329,6 @@ def build_create_table_definition(table_name, id: :primary_key, primary_key: nil
 
         yield table_definition if block_given?
 
-        schema_creation.accept(table_definition)
         table_definition
       end
 
@@ -620,7 +619,7 @@ def add_column(table_name, column_name, type, **options)
         add_column_def = build_add_column_definition(table_name, column_name, type, **options)
         return unless add_column_def
 
-        execute(add_column_def.ddl)
+        execute schema_creation.accept(add_column_def)
       end
 
       def add_columns(table_name, *column_names, type:, **options) # :nodoc:
@@ -645,7 +644,6 @@ def build_add_column_definition(table_name, column_name, type, **options) # :nod
 
         alter_table = create_alter_table(table_name)
         alter_table.add_column(column_name, type, **options)
-        schema_creation.accept(alter_table)
         alter_table
       end
 
@@ -875,7 +873,7 @@ def rename_column(table_name, column_name, new_column_name)
       # For more information see the {"Transactional Migrations" section}[rdoc-ref:Migration].
       def add_index(table_name, column_name, **options)
         create_index = build_create_index_definition(table_name, column_name, **options)
-        execute(create_index.ddl)
+        execute schema_creation.accept(create_index)
       end
 
       # Builds a CreateIndexDefinition object.
@@ -885,10 +883,7 @@ def add_index(table_name, column_name, **options)
       # passing a +table_name+, +column_name+, and other additional options that can be passed.
       def build_create_index_definition(table_name, column_name, **options) # :nodoc:
         index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
-
-        create_index = CreateIndexDefinition.new(index, algorithm, if_not_exists)
-        schema_creation.accept(create_index)
-        create_index
+        CreateIndexDefinition.new(index, algorithm, if_not_exists)
       end
 
       # Removes the given index from the table.
@@ -1715,7 +1710,7 @@ def add_column_for_alter(table_name, column_name, type, **options)
 
         def change_column_default_for_alter(table_name, column_name, default_or_changes)
           cd = build_change_column_default_definition(table_name, column_name, default_or_changes)
-          cd.ddl
+          schema_creation.accept(cd)
         end
 
         def rename_column_sql(table_name, column_name, new_column_name)

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -355,10 +355,7 @@ def build_change_column_default_definition(table_name, column_name, default_or_c
         return unless column
 
         default = extract_new_default_value(default_or_changes)
-        change_column_default_definition = ChangeColumnDefaultDefinition.new(column, default)
-        schema_creation.accept(change_column_default_definition)
-
-        change_column_default_definition
+        ChangeColumnDefaultDefinition.new(column, default)
       end
 
       def change_column_null(table_name, column_name, null, default = nil) # :nodoc:
@@ -411,10 +408,7 @@ def build_change_column_definition(table_name, column_name, type, **options) # :
 
         td = create_table_definition(table_name)
         cd = td.new_column_definition(column.name, type, **options)
-        change_column_def = ChangeColumnDefinition.new(cd, column.name)
-        schema_creation.accept(change_column_def)
-
-        change_column_def
+        ChangeColumnDefinition.new(cd, column.name)
       end
 
       def rename_column(table_name, column_name, new_column_name) # :nodoc:
@@ -426,17 +420,15 @@ def add_index(table_name, column_name, **options) # :nodoc:
         create_index = build_create_index_definition(table_name, column_name, **options)
         return unless create_index
 
-        execute(create_index.ddl)
+        execute schema_creation.accept(create_index)
       end
 
       def build_create_index_definition(table_name, column_name, **options) # :nodoc:
         index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
 
         return if if_not_exists && index_exists?(table_name, column_name, name: index.name)
 
-        create_index = CreateIndexDefinition.new(index, algorithm)
-        schema_creation.accept(create_index)
-        create_index
+        CreateIndexDefinition.new(index, algorithm)
       end
 
       def add_sql_comment!(sql, comment) # :nodoc:
@@ -782,7 +774,7 @@ def translate_exception(exception, message:, sql:, binds:)
 
         def change_column_for_alter(table_name, column_name, type, **options)
           cd = build_change_column_definition(table_name, column_name, type, **options)
-          cd.ddl
+          schema_creation.accept(cd)
         end
 
         def rename_column_for_alter(table_name, column_name, new_column_name)

activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb

@@ -21,7 +21,7 @@ def visit_AddColumnDefinition(o)
 
           def visit_ChangeColumnDefinition(o)
             change_column_sql = +"CHANGE #{quote_column_name(o.name)} #{accept(o.column)}"
-            o.ddl = add_column_position!(change_column_sql, column_options(o.column))
+            add_column_position!(change_column_sql, column_options(o.column))
           end
 
           def visit_ChangeColumnDefaultDefinition(o)
@@ -31,13 +31,12 @@ def visit_ChangeColumnDefaultDefinition(o)
             else
               sql << quote_default_expression(o.default, o.column)
             end
-            o.ddl = sql
           end
 
           def visit_CreateIndexDefinition(o)
             sql = visit_IndexDefinition(o.index, true)
             sql << " #{o.algorithm}" if o.algorithm
-            o.ddl = sql
+            sql
           end
 
           def visit_IndexDefinition(o, create = false)

activerecord/lib/active_record/connection_adapters/postgresql/schema_creation.rb

@@ -10,7 +10,6 @@ def visit_AlterTable(o)
             sql << o.constraint_validations.map { |fk| visit_ValidateConstraint fk }.join(" ")
             sql << o.exclusion_constraint_adds.map { |con| visit_AddExclusionConstraint con }.join(" ")
             sql << o.exclusion_constraint_drops.map { |con| visit_DropExclusionConstraint con }.join(" ")
-            o.ddl = sql
           end
 
           def visit_AddForeignKey(o)
@@ -84,7 +83,7 @@ def visit_ChangeColumnDefinition(o)
               change_column_sql << ", ALTER COLUMN #{quoted_column_name} #{options[:null] ? 'DROP' : 'SET'} NOT NULL"
             end
 
-            o.ddl = change_column_sql
+            change_column_sql
           end
 
           def visit_ChangeColumnDefaultDefinition(o)
@@ -94,7 +93,6 @@ def visit_ChangeColumnDefaultDefinition(o)
             else
               sql << "SET DEFAULT #{quote_default_expression(o.default, o.column)}"
             end
-            o.ddl = sql
           end
 
           def add_column_options!(sql, options)

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -417,10 +417,7 @@ def change_column(table_name, column_name, type, **options) # :nodoc:
         def build_change_column_definition(table_name, column_name, type, **options) # :nodoc:
           td = create_table_definition(table_name)
           cd = td.new_column_definition(column_name, type, **options)
-          change_column_def = ChangeColumnDefinition.new(cd, column_name)
-          schema_creation.accept(change_column_def)
-
-          change_column_def
+          ChangeColumnDefinition.new(cd, column_name)
         end
 
         # Changes the default value of a table column.
@@ -433,10 +430,7 @@ def build_change_column_default_definition(table_name, column_name, default_or_c
           return unless column
 
           default = extract_new_default_value(default_or_changes)
-          change_column_default_definition = ChangeColumnDefaultDefinition.new(column, default)
-          schema_creation.accept(change_column_default_definition)
-
-          change_column_default_definition
+          ChangeColumnDefaultDefinition.new(column, default)
         end
 
         def change_column_null(table_name, column_name, null, default = nil) # :nodoc:
@@ -473,7 +467,7 @@ def rename_column(table_name, column_name, new_column_name) # :nodoc:
 
         def add_index(table_name, column_name, **options) # :nodoc:
           create_index = build_create_index_definition(table_name, column_name, **options)
-          result = execute(create_index.ddl)
+          result = execute schema_creation.accept(create_index)
 
           index = create_index.index
           execute "COMMENT ON INDEX #{quote_column_name(index.name)} IS #{quote(index.comment)}" if index.comment
@@ -482,10 +476,7 @@ def add_index(table_name, column_name, **options) # :nodoc:
 
         def build_create_index_definition(table_name, column_name, **options) # :nodoc:
           index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
-
-          create_index = CreateIndexDefinition.new(index, algorithm, if_not_exists)
-          schema_creation.accept(create_index)
-          create_index
+          CreateIndexDefinition.new(index, algorithm, if_not_exists)
         end
 
         def remove_index(table_name, column_name = nil, **options) # :nodoc:
@@ -861,7 +852,7 @@ def add_column_for_alter(table_name, column_name, type, **options)
 
           def change_column_for_alter(table_name, column_name, type, **options)
             change_col_def = build_change_column_definition(table_name, column_name, type, **options)
-            sqls = [change_col_def.ddl]
+            sqls = [schema_creation.accept(change_col_def)]
             sqls << Proc.new { change_column_comment(table_name, column_name, options[:comment]) } if options.key?(:comment)
             sqls
           end

activerecord/test/cases/migration/schema_definitions_test.rb

@@ -18,22 +18,16 @@ def test_build_create_table_definition_with_block
 
         id_column = td.columns.find { |col| col.name == "id" }
         assert_predicate id_column, :present?
-        assert id_column.type
-        assert id_column.sql_type
 
         foo_column = td.columns.find { |col| col.name == "foo" }
         assert_predicate foo_column, :present?
-        assert foo_column.type
-        assert foo_column.sql_type
       end
 
       def test_build_create_table_definition_without_block
         td = connection.build_create_table_definition(:test)
 
         id_column = td.columns.find { |col| col.name == "id" }
         assert_predicate id_column, :present?
-        assert id_column.type
-        assert id_column.sql_type
       end
 
       def test_build_create_join_table_definition_with_block
@@ -64,7 +58,6 @@ def test_build_create_index_definition
         end
         create_index = connection.build_create_index_definition(:test, :foo)
 
-        assert_match "CREATE INDEX", create_index.ddl
         assert_equal "index_test_on_foo", create_index.index.name
       ensure
         connection.drop_table(:test) if connection.table_exists?(:test)
@@ -91,12 +84,8 @@ def test_build_change_column_definition
           end
 
           change_cd = connection.build_change_column_definition(:test, :foo, :integer)
-          assert change_cd.ddl
-
           change_col = change_cd.column
           assert_equal "foo", change_col.name.to_s
-          assert change_col.type
-          assert change_col.sql_type
         ensure
           connection.drop_table(:test) if connection.table_exists?(:test)
         end
@@ -107,13 +96,10 @@ def test_build_change_column_default_definition
           end
 
           change_default_cd = connection.build_change_column_default_definition(:test, :foo, "new")
-          assert_match "SET DEFAULT 'new'", change_default_cd.ddl
           assert_equal "new", change_default_cd.default
 
           change_col = change_default_cd.column
           assert_equal "foo", change_col.name.to_s
-          assert change_col.type
-          assert change_col.sql_type
         ensure
           connection.drop_table(:test) if connection.table_exists?(:test)
         end