Skip to content

Commit 3c5b0cc

Browse files
authored
Revert "Do not modify column type while modifying fk constraint (#644)"
This reverts commit 75ddf59.
1 parent 75ddf59 commit 3c5b0cc

File tree

4 files changed

+29
-209
lines changed

4 files changed

+29
-209
lines changed

integration_test/pg/migrations_test.exs

Lines changed: 0 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -40,47 +40,6 @@ defmodule Ecto.Integration.MigrationsTest do
4040
end
4141
end
4242

43-
defmodule AlterColumnMigrationViaModify do
44-
use Ecto.Migration
45-
46-
def up do
47-
create table(:my_users) do
48-
add :from_null_to_not_null, :integer
49-
50-
add :from_default_to_no_default, :integer, default: 0
51-
add :from_no_default_to_default, :integer
52-
end
53-
54-
create table(:my_comments) do
55-
add :user_id, references(:users)
56-
end
57-
58-
create table(:my_posts) do
59-
add :user_id, :bigserial
60-
end
61-
62-
alter table(:my_users) do
63-
modify :from_null_to_not_null, :string, null: false, from: {:string, null: true}
64-
modify :from_default_to_no_default, :integer, default: nil, from: {:integer, default: 0}
65-
modify :from_no_default_to_default, :integer, default: 0, from: {:integer, default: nil}
66-
end
67-
68-
alter table(:my_comments) do
69-
modify :user_id, references(:my_users, on_delete: :nilify_all), from: references(:my_users, on_delete: :nothing)
70-
end
71-
72-
alter table(:my_posts) do
73-
modify :user_id, references(:my_users), from: :bigserial
74-
end
75-
end
76-
77-
def down do
78-
drop table(:my_posts)
79-
drop table(:my_comments)
80-
drop table(:my_users)
81-
end
82-
end
83-
8443
test "logs Postgres notice messages" do
8544
log =
8645
capture_log(fn ->
@@ -131,46 +90,6 @@ defmodule Ecto.Integration.MigrationsTest do
13190
assert down_log =~ "commit []"
13291
end
13392

134-
test "does not alter column type when enough info is provided to modify/3" do
135-
num = @base_migration + System.unique_integer([:positive])
136-
up_log =
137-
capture_log(fn ->
138-
Ecto.Migrator.up(PoolRepo, num, AlterColumnMigrationViaModify, log_migrator_sql: :info, log_migrations_sql: :info, log: :info)
139-
end)
140-
141-
142-
143-
assert Regex.scan(~r/(begin \[\])/, up_log) |> length() == 2
144-
assert up_log =~ ~s(ALTER TABLE "my_users")
145-
refute up_log =~ ~s(ALTER COLUMN "from_null_to_not_null" TYPE)
146-
assert up_log =~ ~s(ALTER COLUMN "from_null_to_not_null" SET NOT NULL,)
147-
refute up_log =~ ~s(ALTER COLUMN "from_default_to_no_default" TYPE)
148-
assert up_log =~ ~s(ALTER COLUMN "from_default_to_no_default" SET DEFAULT NULL,)
149-
refute up_log =~ ~s(ALTER COLUMN "from_no_default_to_default" TYPE)
150-
assert up_log =~ ~s(ALTER COLUMN "from_no_default_to_default" SET DEFAULT 0)
151-
152-
assert up_log =~ ~s(ALTER TABLE "my_comments")
153-
assert up_log =~ ~s(DROP CONSTRAINT "my_comments_user_id_fkey",)
154-
refute up_log =~ ~s(ALTER COLUMN "user_id" TYPE)
155-
assert up_log =~ ~s/ADD CONSTRAINT "my_comments_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "my_users"("id") ON DELETE SET NULL/
156-
157-
assert up_log =~ ~s(ALTER TABLE "my_posts")
158-
refute up_log =~ ~s(ALTER COLUMN "user_id" TYPE)
159-
assert up_log =~ ~s/ADD CONSTRAINT "my_posts_user_id_fkey" FOREIGN KEY ("user_id") REFERENCES "my_users"("id")/
160-
assert Regex.scan(~r/(commit \[\])/, up_log) |> length() == 2
161-
162-
down_log =
163-
capture_log(fn ->
164-
Ecto.Migrator.down(PoolRepo, num, AlterColumnMigrationViaModify, log_migrator_sql: :info, log_migrations_sql: :info, log: :info)
165-
end)
166-
167-
assert down_log =~ "begin []"
168-
assert down_log =~ ~s(DROP TABLE "my_comments")
169-
assert down_log =~ ~s(DROP TABLE "my_posts")
170-
assert down_log =~ ~s(DROP TABLE "my_users")
171-
assert down_log =~ "commit []"
172-
end
173-
17493
test "logs advisory lock and transaction commands" do
17594
num = @base_migration + System.unique_integer([:positive])
17695
up_log =

lib/ecto/adapters/postgres/connection.ex

Lines changed: 21 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -1546,76 +1546,35 @@ if Code.ensure_loaded?(Postgrex) do
15461546
end
15471547

15481548
defp column_change(table, {:modify, name, %Reference{} = ref, opts}) do
1549-
reference_column_type = reference_column_type(ref.type, opts)
1550-
from_column_type = extract_column_type(opts[:from])
1551-
1552-
drop_reference_expr = drop_reference_expr(opts[:from], table, name)
1553-
prefix_with_comma = (drop_reference_expr != [] && ", ") || ""
1554-
1555-
common_suffix = [
1549+
[
1550+
drop_reference_expr(opts[:from], table, name),
1551+
"ALTER COLUMN ",
1552+
quote_name(name),
1553+
" TYPE ",
1554+
reference_column_type(ref.type, opts),
1555+
", ADD ",
15561556
reference_expr(ref, table, name),
15571557
modify_null(name, opts),
15581558
modify_default(name, ref.type, opts)
15591559
]
1560-
1561-
if reference_column_type == reference_column_type(from_column_type, opts) do
1562-
[
1563-
drop_reference_expr,
1564-
prefix_with_comma,
1565-
"ADD " | common_suffix
1566-
]
1567-
else
1568-
[
1569-
drop_reference_expr,
1570-
prefix_with_comma,
1571-
"ALTER COLUMN ",
1572-
quote_name(name),
1573-
" TYPE ",
1574-
reference_column_type,
1575-
", ADD " | common_suffix
1576-
]
1577-
end
15781560
end
15791561

15801562
defp column_change(table, {:modify, name, type, opts}) do
1581-
column_type = column_type(type, opts)
1582-
from_column_type = extract_column_type(opts[:from])
1583-
1584-
drop_reference_expr = drop_reference_expr(opts[:from], table, name)
1585-
any_drop_ref? = drop_reference_expr != []
1586-
1587-
if column_type == column_type(from_column_type, opts) do
1588-
modify_null = modify_null(name, Keyword.put(opts, :prefix_with_comma, any_drop_ref?))
1589-
any_modify_null? = modify_null != []
1590-
1591-
modify_default =
1592-
modify_default(
1593-
name,
1594-
type,
1595-
Keyword.put(opts, :prefix_with_comma, any_drop_ref? or any_modify_null?)
1596-
)
1597-
1598-
[drop_reference_expr, modify_null, modify_default]
1599-
else
1600-
[
1601-
drop_reference_expr,
1602-
(any_drop_ref? && ", ") || "",
1603-
"ALTER COLUMN ",
1604-
quote_name(name),
1605-
" TYPE ",
1606-
column_type,
1607-
modify_null(name, opts),
1608-
modify_default(name, type, opts)
1609-
]
1610-
end
1563+
[
1564+
drop_reference_expr(opts[:from], table, name),
1565+
"ALTER COLUMN ",
1566+
quote_name(name),
1567+
" TYPE ",
1568+
column_type(type, opts),
1569+
modify_null(name, opts),
1570+
modify_default(name, type, opts)
1571+
]
16111572
end
16121573

16131574
defp column_change(_table, {:remove, name}), do: ["DROP COLUMN ", quote_name(name)]
16141575

16151576
defp column_change(table, {:remove, name, %Reference{} = ref, _opts}) do
1616-
drop_reference_expr = drop_reference_expr(ref, table, name)
1617-
prefix_with_comma = (drop_reference_expr != [] && ", ") || ""
1618-
[drop_reference_expr, prefix_with_comma, "DROP COLUMN ", quote_name(name)]
1577+
[drop_reference_expr(ref, table, name), "DROP COLUMN ", quote_name(name)]
16191578
end
16201579

16211580
defp column_change(_table, {:remove, name, _type, _opts}),
@@ -1636,23 +1595,17 @@ if Code.ensure_loaded?(Postgrex) do
16361595
do: ["DROP COLUMN IF EXISTS ", quote_name(name)]
16371596

16381597
defp modify_null(name, opts) do
1639-
prefix_with_comma = Keyword.get(opts, :prefix_with_comma, true)
1640-
prefix = if prefix_with_comma, do: ", ", else: ""
1641-
16421598
case Keyword.get(opts, :null) do
1643-
true -> [prefix, "ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
1644-
false -> [prefix, "ALTER COLUMN ", quote_name(name), " SET NOT NULL"]
1599+
true -> [", ALTER COLUMN ", quote_name(name), " DROP NOT NULL"]
1600+
false -> [", ALTER COLUMN ", quote_name(name), " SET NOT NULL"]
16451601
nil -> []
16461602
end
16471603
end
16481604

16491605
defp modify_default(name, type, opts) do
1650-
prefix_with_comma = Keyword.get(opts, :prefix_with_comma, true)
1651-
prefix = if prefix_with_comma, do: ", ", else: ""
1652-
16531606
case Keyword.fetch(opts, :default) do
16541607
{:ok, val} ->
1655-
[prefix, "ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
1608+
[", ALTER COLUMN ", quote_name(name), " SET", default_expr({:ok, val}, type)]
16561609

16571610
:error ->
16581611
[]
@@ -1844,11 +1797,6 @@ if Code.ensure_loaded?(Postgrex) do
18441797
[type, generated_expr(generated)]
18451798
end
18461799

1847-
defp extract_column_type({type, _}) when is_atom(type), do: type
1848-
defp extract_column_type(type) when is_atom(type), do: type
1849-
defp extract_column_type(%Reference{type: type}), do: type
1850-
defp extract_column_type(_), do: nil
1851-
18521800
defp generated_expr(nil), do: []
18531801

18541802
defp generated_expr(expr) when is_binary(expr) do
@@ -1885,7 +1833,7 @@ if Code.ensure_loaded?(Postgrex) do
18851833
do: drop_reference_expr(ref, table, name)
18861834

18871835
defp drop_reference_expr(%Reference{} = ref, table, name),
1888-
do: ["DROP CONSTRAINT ", reference_name(ref, table, name)]
1836+
do: ["DROP CONSTRAINT ", reference_name(ref, table, name), ", "]
18891837

18901838
defp drop_reference_expr(_, _, _),
18911839
do: []

lib/ecto/migration.ex

Lines changed: 7 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,41 +1274,13 @@ defmodule Ecto.Migration do
12741274
12751275
See `add/3` for more information on supported types.
12761276
1277-
> #### Modifying a column without changing its type {: .warning}
1278-
>
1279-
> If you want to modify a column without changing its type,
1280-
> such as adding or dropping a null constraint, consider using
1281-
> the `execute/2` command with the relevant SQL command instead
1282-
> of `modify/3`, if supported by your database. This may avoid
1283-
> redundant type updates and be more efficient, as an unnecessary
1284-
> type update can lock the table, even if the type actually
1285-
> doesn't change.
1286-
>
1287-
> These undesired locks can be avoided when using the PostgreSQL adapter by
1288-
> providing the `:from` option and ensuring its type matches the type provided
1289-
> to `modify/3`. In that scenario, the type change part of the migration is omitted.
1290-
>
1291-
> Examples:
1292-
>
1293-
> # modify column with rollback options
1294-
> alter table("posts") do
1295-
> modify :title, :text, null: false, from: {:text, null: true}
1296-
> end
1297-
>
1298-
> # adding a new foreign key constraint
1299-
> alter table("posts") do
1300-
> modify :author_id, references(:authors, type: :id, validate: false), from: :id
1301-
> end
1302-
>
1303-
> # Modify the :on_delete option of an existing foreign key
1304-
> alter table("comments") do
1305-
> modify :post_id, references(:posts, on_delete: :delete_all),
1306-
> from: references(:posts, on_delete: :nothing)
1307-
> end
1308-
>
1309-
> The previous syntax offers two benefits:
1310-
> 1. the migrations are reversible
1311-
> 2. the PostgreSQL adapter will skip the type update, due to the `:from` type matching the modify type
1277+
If you want to modify a column without changing its type,
1278+
such as adding or dropping a null constraints, consider using
1279+
the `execute/2` command with the relevant SQL command instead
1280+
of `modify/3`, if supported by your database. This may avoid
1281+
redundant type updates and be more efficient, as an unnecessary
1282+
type update can lock the table, even if the type actually
1283+
doesn't change.
13121284
13131285
## Examples
13141286

test/ecto/adapters/postgres_test.exs

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2543,6 +2543,7 @@ defmodule Ecto.Adapters.PostgresTest do
25432543
ALTER COLUMN "space_id" TYPE integer,
25442544
ALTER COLUMN "space_id" DROP NOT NULL,
25452545
DROP CONSTRAINT "posts_group_id_fkey",
2546+
ALTER COLUMN "group_id" TYPE bigint,
25462547
ADD CONSTRAINT "posts_group_id_fkey" FOREIGN KEY ("group_id") REFERENCES "groups"("gid"),
25472548
ALTER COLUMN "status" TYPE varchar(100),
25482549
ALTER COLUMN "status" SET NOT NULL,
@@ -2643,26 +2644,6 @@ defmodule Ecto.Adapters.PostgresTest do
26432644
]
26442645
end
26452646

2646-
test "alter table without updating column type via modify/3" do
2647-
alter =
2648-
{:alter, table(:posts),
2649-
[
2650-
{:modify, :category_id, %Reference{table: :categories, type: :id}, from: :id},
2651-
{:modify, :author_id, %Reference{table: :authors, type: :id, on_delete: :delete_all},
2652-
from: %Reference{table: :authors, type: :id, on_delete: :nothing}}
2653-
]}
2654-
2655-
assert execute_ddl(alter) ==
2656-
[
2657-
remove_newlines("""
2658-
ALTER TABLE "posts"
2659-
ADD CONSTRAINT "posts_category_id_fkey" FOREIGN KEY ("category_id") REFERENCES "categories"("id"),
2660-
DROP CONSTRAINT "posts_author_id_fkey",
2661-
ADD CONSTRAINT "posts_author_id_fkey" FOREIGN KEY ("author_id") REFERENCES "authors"("id") ON DELETE CASCADE
2662-
""")
2663-
]
2664-
end
2665-
26662647
test "create index" do
26672648
create = {:create, index(:posts, [:category_id, :permalink])}
26682649

0 commit comments

Comments
 (0)