From 3c5b0cc062f2d7434cc36152840dc77e220a37c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 19 Nov 2024 15:37:38 +0100 Subject: [PATCH] Revert "Do not modify column type while modifying fk constraint (#644)" This reverts commit 75ddf591fbe56977b041836079c01be298384024. --- integration_test/pg/migrations_test.exs | 81 -------------------- lib/ecto/adapters/postgres/connection.ex | 94 ++++++------------------ lib/ecto/migration.ex | 42 ++--------- test/ecto/adapters/postgres_test.exs | 21 +----- 4 files changed, 29 insertions(+), 209 deletions(-) diff --git a/integration_test/pg/migrations_test.exs b/integration_test/pg/migrations_test.exs index 4b59a1be..a6c87cbd 100644 --- a/integration_test/pg/migrations_test.exs +++ b/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 = diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex index ad39f41d..996e8e18 100644 --- a/lib/ecto/adapters/postgres/connection.ex +++ b/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: [] diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex index ab258e3e..8b0d8191 100644 --- a/lib/ecto/migration.ex +++ b/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 diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs index 9c157290..e9a64cf5 100644 --- a/test/ecto/adapters/postgres_test.exs +++ b/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])}