Do not modify column type while modifying fk constraint (#644)

---------

Co-authored-by: Matt Enlow <[email protected]>
Co-authored-by: José Valim <[email protected]>

Author: 3 people

integration_test/pg/migrations_test.exs

@@ -40,6 +40,47 @@ 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 ->
@@ -90,6 +131,46 @@ 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,35 +1546,76 @@ if Code.ensure_loaded?(Postgrex) do
     end
 
     defp column_change(table, {:modify, name, %Reference{} = ref, opts}) do
-      [
-        drop_reference_expr(opts[:from], table, name),
-        "ALTER COLUMN ",
-        quote_name(name),
-        " TYPE ",
-        reference_column_type(ref.type, opts),
-        ", ADD ",
+      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 = [
         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
-      [
-        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)
-      ]
+      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
     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(ref, table, name), "DROP COLUMN ", quote_name(name)]
+      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)]
     end
 
     defp column_change(_table, {:remove, name, _type, _opts}),
@@ -1595,17 +1636,23 @@ 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 -> [", ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
-        false -> [", ALTER COLUMN ", quote_name(name), " SET NOT NULL"]
+        true -> [prefix, "ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
+        false -> [prefix, "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} ->
-          [", ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
+          [prefix, "ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
 
         :error ->
           []
@@ -1797,6 +1844,11 @@ 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
@@ -1833,7 +1885,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,13 +1274,41 @@ defmodule Ecto.Migration do
 
   See `add/3` for more information on supported types.
 
-  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.
+  > #### 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
 
   ## Examples
 

test/ecto/adapters/postgres_test.exs

@@ -2543,7 +2543,6 @@ 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,
@@ -2644,6 +2643,26 @@ 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])}