Revert "Do not modify column type while modifying fk constraint"

integration_test/pg/migrations_test.exs

@@ -40,47 +40,6 @@ defmodule Ecto.Integration.MigrationsTest do
     end
   end
 
-  defmodule AlterColumnMigrationViaModify do
-    use Ecto.Migration
-
-    def up do
-      create table(:my_users) do
-        add :from_null_to_not_null, :integer
-
-        add :from_default_to_no_default, :integer, default: 0
-        add :from_no_default_to_default, :integer
-      end
-
-      create table(:my_comments) do
-        add :user_id, references(:users)
-      end
-
-      create table(:my_posts) do
-        add :user_id, :bigserial
-      end
-
-      alter table(:my_users) do
-        modify :from_null_to_not_null, :string, null: false, from: {:string, null: true}
-        modify :from_default_to_no_default, :integer, default: nil, from: {:integer, default: 0}
-        modify :from_no_default_to_default, :integer, default: 0, from: {:integer, default: nil}
-      end
-
-      alter table(:my_comments) do
-        modify :user_id, references(:my_users, on_delete: :nilify_all), from: references(:my_users, on_delete: :nothing)
-      end
-
-      alter table(:my_posts) do
-        modify :user_id, references(:my_users), from: :bigserial
-      end
-    end
-
-    def down do
-      drop table(:my_posts)
-      drop table(:my_comments)
-      drop table(:my_users)
-    end
-  end
-
   test "logs Postgres notice messages" do
     log =
       capture_log(fn ->
@@ -131,46 +90,6 @@ defmodule Ecto.Integration.MigrationsTest do
       assert down_log =~ "commit []"
     end
 
-    test "does not alter column type when enough info is provided to modify/3" do
-      num = @base_migration + System.unique_integer([:positive])
-      up_log =
-        capture_log(fn ->
-          Ecto.Migrator.up(PoolRepo, num, AlterColumnMigrationViaModify, log_migrator_sql: :info, log_migrations_sql: :info, log: :info)
-        end)
-
-
-
-      assert Regex.scan(~r/(begin \[\])/, up_log) |> length() == 2
-      assert up_log =~ ~s(ALTER TABLE "my_users")
-      refute up_log =~ ~s(ALTER COLUMN "from_null_to_not_null" TYPE) 
-      assert up_log =~ ~s(ALTER COLUMN "from_null_to_not_null" SET NOT NULL,)
-      refute up_log =~ ~s(ALTER COLUMN "from_default_to_no_default" TYPE)
-      assert up_log =~ ~s(ALTER COLUMN "from_default_to_no_default" SET DEFAULT NULL,)
-      refute up_log =~ ~s(ALTER COLUMN "from_no_default_to_default" TYPE)
-      assert up_log =~ ~s(ALTER COLUMN "from_no_default_to_default" SET DEFAULT 0)
-
-      assert up_log =~ ~s(ALTER TABLE "my_comments")
-      assert up_log =~ ~s(DROP CONSTRAINT "my_comments_user_id_fkey",)
-      refute up_log =~ ~s(ALTER COLUMN "user_id" TYPE)
-      assert up_log =~ ~s/ADD CONSTRAINT "my_comments_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "my_users"("id") ON DELETE SET NULL/
-
-      assert up_log =~ ~s(ALTER TABLE "my_posts")
-      refute up_log =~ ~s(ALTER COLUMN "user_id" TYPE)
-      assert up_log =~ ~s/ADD CONSTRAINT "my_posts_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "my_users"("id")/
-      assert Regex.scan(~r/(commit \[\])/, up_log) |> length() == 2
-
-      down_log =
-        capture_log(fn ->
-          Ecto.Migrator.down(PoolRepo, num, AlterColumnMigrationViaModify, log_migrator_sql: :info, log_migrations_sql: :info, log: :info)
-        end)
-
-      assert down_log =~ "begin []"
-      assert down_log =~ ~s(DROP TABLE "my_comments")
-      assert down_log =~ ~s(DROP TABLE "my_posts")
-      assert down_log =~ ~s(DROP TABLE "my_users")
-      assert down_log =~ "commit []"
-    end
-
     test "logs advisory lock and transaction commands" do
       num = @base_migration + System.unique_integer([:positive])
       up_log =

lib/ecto/adapters/postgres/connection.ex

@@ -1546,76 +1546,35 @@ if Code.ensure_loaded?(Postgrex) do
     end
 
     defp column_change(table, {:modify, name, %Reference{} = ref, opts}) do
-      reference_column_type = reference_column_type(ref.type, opts)
-      from_column_type = extract_column_type(opts[:from])
-
-      drop_reference_expr = drop_reference_expr(opts[:from], table, name)
-      prefix_with_comma = (drop_reference_expr != [] && ", ") || ""
-
-      common_suffix = [
+      [
+        drop_reference_expr(opts[:from], table, name),
+        "ALTER COLUMN ",
+        quote_name(name),
+        " TYPE ",
+        reference_column_type(ref.type, opts),
+        ", ADD ",
         reference_expr(ref, table, name),
         modify_null(name, opts),
         modify_default(name, ref.type, opts)
       ]
-
-      if reference_column_type == reference_column_type(from_column_type, opts) do
-        [
-          drop_reference_expr,
-          prefix_with_comma,
-          "ADD " | common_suffix
-        ]
-      else
-        [
-          drop_reference_expr,
-          prefix_with_comma,
-          "ALTER COLUMN ",
-          quote_name(name),
-          " TYPE ",
-          reference_column_type,
-          ", ADD " | common_suffix
-        ]
-      end
     end
 
     defp column_change(table, {:modify, name, type, opts}) do
-      column_type = column_type(type, opts)
-      from_column_type = extract_column_type(opts[:from])
-
-      drop_reference_expr = drop_reference_expr(opts[:from], table, name)
-      any_drop_ref? = drop_reference_expr != []
-
-      if column_type == column_type(from_column_type, opts) do
-        modify_null = modify_null(name, Keyword.put(opts, :prefix_with_comma, any_drop_ref?))
-        any_modify_null? = modify_null != []
-
-        modify_default =
-          modify_default(
-            name,
-            type,
-            Keyword.put(opts, :prefix_with_comma, any_drop_ref? or any_modify_null?)
-          )
-
-        [drop_reference_expr, modify_null, modify_default]
-      else
-        [
-          drop_reference_expr,
-          (any_drop_ref? && ", ") || "",
-          "ALTER COLUMN ",
-          quote_name(name),
-          " TYPE ",
-          column_type,
-          modify_null(name, opts),
-          modify_default(name, type, opts)
-        ]
-      end
+      [
+        drop_reference_expr(opts[:from], table, name),
+        "ALTER COLUMN ",
+        quote_name(name),
+        " TYPE ",
+        column_type(type, opts),
+        modify_null(name, opts),
+        modify_default(name, type, opts)
+      ]
     end
 
     defp column_change(_table, {:remove, name}), do: ["DROP COLUMN ", quote_name(name)]
 
     defp column_change(table, {:remove, name, %Reference{} = ref, _opts}) do
-      drop_reference_expr = drop_reference_expr(ref, table, name)
-      prefix_with_comma = (drop_reference_expr != [] && ", ") || ""
-      [drop_reference_expr, prefix_with_comma, "DROP COLUMN ", quote_name(name)]
+      [drop_reference_expr(ref, table, name), "DROP COLUMN ", quote_name(name)]
     end
 
     defp column_change(_table, {:remove, name, _type, _opts}),
@@ -1636,23 +1595,17 @@ if Code.ensure_loaded?(Postgrex) do
       do: ["DROP COLUMN IF EXISTS ", quote_name(name)]
 
     defp modify_null(name, opts) do
-      prefix_with_comma = Keyword.get(opts, :prefix_with_comma, true)
-      prefix = if prefix_with_comma, do: ", ", else: ""
-
       case Keyword.get(opts, :null) do
-        true -> [prefix, "ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
-        false -> [prefix, "ALTER COLUMN ", quote_name(name), " SET NOT NULL"]
+        true -> [", ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
+        false -> [", ALTER COLUMN ", quote_name(name), " SET NOT NULL"]
         nil -> []
       end
     end
 
     defp modify_default(name, type, opts) do
-      prefix_with_comma = Keyword.get(opts, :prefix_with_comma, true)
-      prefix = if prefix_with_comma, do: ", ", else: ""
-
       case Keyword.fetch(opts, :default) do
         {:ok, val} ->
-          [prefix, "ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
+          [", ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
 
         :error ->
           []
@@ -1844,11 +1797,6 @@ if Code.ensure_loaded?(Postgrex) do
       [type, generated_expr(generated)]
     end
 
-    defp extract_column_type({type, _}) when is_atom(type), do: type
-    defp extract_column_type(type) when is_atom(type), do: type
-    defp extract_column_type(%Reference{type: type}), do: type
-    defp extract_column_type(_), do: nil
-
     defp generated_expr(nil), do: []
 
     defp generated_expr(expr) when is_binary(expr) do
@@ -1885,7 +1833,7 @@ if Code.ensure_loaded?(Postgrex) do
       do: drop_reference_expr(ref, table, name)
 
     defp drop_reference_expr(%Reference{} = ref, table, name),
-      do: ["DROP CONSTRAINT ", reference_name(ref, table, name)]
+      do: ["DROP CONSTRAINT ", reference_name(ref, table, name), ", "]
 
     defp drop_reference_expr(_, _, _),
       do: []

lib/ecto/migration.ex

@@ -1274,41 +1274,13 @@ defmodule Ecto.Migration do
 
   See `add/3` for more information on supported types.
 
-  > #### Modifying a column without changing its type {: .warning}
-  >
-  > If you want to modify a column without changing its type,
-  > such as adding or dropping a null constraint, consider using
-  > the `execute/2` command with the relevant SQL command instead
-  > of `modify/3`, if supported by your database. This may avoid
-  > redundant type updates and be more efficient, as an unnecessary
-  > type update can lock the table, even if the type actually
-  > doesn't change.
-  >
-  > These undesired locks can be avoided when using the PostgreSQL adapter by
-  > providing the `:from` option and ensuring its type matches the type provided
-  > to `modify/3`. In that scenario, the type change part of the migration is omitted.
-  >
-  > Examples:
-  >
-  >     # modify column with rollback options
-  >     alter table("posts") do
-  >       modify :title, :text, null: false, from: {:text, null: true}
-  >     end
-  >
-  >     # adding a new foreign key constraint
-  >     alter table("posts") do
-  >       modify :author_id, references(:authors, type: :id, validate: false), from: :id
-  >     end
-  >
-  >     # Modify the :on_delete option of an existing foreign key
-  >     alter table("comments") do
-  >       modify :post_id, references(:posts, on_delete: :delete_all),
-  >         from: references(:posts, on_delete: :nothing)
-  >     end
-  >
-  > The previous syntax offers two benefits:
-  > 1. the migrations are reversible
-  > 2. the PostgreSQL adapter will skip the type update, due to the `:from` type matching the modify type
+  If you want to modify a column without changing its type,
+  such as adding or dropping a null constraints, consider using
+  the `execute/2` command with the relevant SQL command instead
+  of `modify/3`, if supported by your database. This may avoid
+  redundant type updates and be more efficient, as an unnecessary
+  type update can lock the table, even if the type actually
+  doesn't change.
 
   ## Examples
 

test/ecto/adapters/postgres_test.exs

@@ -2543,6 +2543,7 @@ defmodule Ecto.Adapters.PostgresTest do
              ALTER COLUMN "space_id" TYPE integer,
              ALTER COLUMN "space_id" DROP NOT NULL,
              DROP CONSTRAINT "posts_group_id_fkey",
+             ALTER COLUMN "group_id" TYPE bigint,
              ADD CONSTRAINT "posts_group_id_fkey" FOREIGN KEY ("group_id") REFERENCES "groups"("gid"),
              ALTER COLUMN "status" TYPE varchar(100),
              ALTER COLUMN "status" SET NOT NULL,
@@ -2643,26 +2644,6 @@ defmodule Ecto.Adapters.PostgresTest do
            ]
   end
 
-  test "alter table without updating column type via modify/3" do
-    alter =
-      {:alter, table(:posts),
-       [
-         {:modify, :category_id, %Reference{table: :categories, type: :id}, from: :id},
-         {:modify, :author_id, %Reference{table: :authors, type: :id, on_delete: :delete_all},
-          from: %Reference{table: :authors, type: :id, on_delete: :nothing}}
-       ]}
-
-    assert execute_ddl(alter) ==
-             [
-               remove_newlines("""
-               ALTER TABLE "posts"
-               ADD CONSTRAINT "posts_category_id_fkey" FOREIGN KEY ("category_id") REFERENCES "categories"("id"),
-               DROP CONSTRAINT "posts_author_id_fkey",
-               ADD CONSTRAINT "posts_author_id_fkey" FOREIGN KEY ("author_id") REFERENCES "authors"("id") ON DELETE CASCADE
-               """)
-             ]
-  end
-
   test "create index" do
     create = {:create, index(:posts, [:category_id, :permalink])}