Skip to content

Commit

Permalink
Revert "Do not modify column type while modifying fk constraint (#644)"
Browse files Browse the repository at this point in the history 
This reverts commit 75ddf59.
  • Loading branch information
@josevalim
josevalim authored Nov 19, 2024
1 parent 75ddf59 commit 3c5b0cc
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 209 deletions.
81 changes: 0 additions & 81 deletions integration_test/pg/migrations_test.exs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ->
Expand Down Expand Up @@ -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 =
Expand Down
94 changes: 21 additions & 73 deletions lib/ecto/adapters/postgres/connection.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}),
Expand All @@ -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 ->
[]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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: []
Expand Down
42 changes: 7 additions & 35 deletions lib/ecto/migration.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 1 addition & 20 deletions test/ecto/adapters/postgres_test.exs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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])}

Expand Down

0 comments on commit 3c5b0cc

Please sign in to comment.