Repository: elixir-ecto/ecto_sql
Pull request #571 (2023-10-31 19:12:18)

Don't attempt to add PK when calling remove/3
Closes https://github.com/elixir-ecto/ecto_sql/issues/570

We don't have to worry about remove_if_not_exists because it doesn't accept options.

greg-rychlewski: actually sorry i believe this is fixed with this change https://github.com/elixir-ecto/ecto_sql/commit/6dd7df5ba5f3bb1a9901a7eeb3f5dbc2a846ab8c
greg-rychlewski: Actually I confused myself. I thought they were reporting issues with the reverse direction but it's the forward one. 

Maybe instead of the changes I have here we should just raise if primary_key is used as an option here?

```elixir
def remove(column, type, opts \\ []) when is_atom(column) do
    validate_type!(type)
    Runner.subcommand({:remove, column, type, opts})
  end
```

Patch:
diff --git a/lib/ecto/adapters/myxql/connection.ex b/lib/ecto/adapters/myxql/connection.ex
index b5cc5bf..04b807e 100644
--- a/lib/ecto/adapters/myxql/connection.ex
+++ b/lib/ecto/adapters/myxql/connection.ex
@@ -1086,7 +1086,8 @@ if Code.ensure_loaded?(MyXQL) do
 
     defp pk_definitions(columns, prefix) do
       pks =
-        for {_, name, _, opts} <- columns,
+        for {action, name, _, opts} <- columns,
+            action != :remove,
             opts[:primary_key],
             do: name
 
diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex
index 980edd5..09d3c2f 100644
--- a/lib/ecto/adapters/postgres/connection.ex
+++ b/lib/ecto/adapters/postgres/connection.ex
@@ -1340,7 +1340,8 @@ if Code.ensure_loaded?(Postgrex) do
 
     defp pk_definition(columns, prefix) do
       pks =
-        for {_, name, _, opts} <- columns,
+        for {action, name, _, opts} <- columns,
+            action != :remove,
             opts[:primary_key],
             do: name
 
diff --git a/lib/ecto/adapters/tds/connection.ex b/lib/ecto/adapters/tds/connection.ex
index 8ef788d..bba4568 100644
--- a/lib/ecto/adapters/tds/connection.ex
+++ b/lib/ecto/adapters/tds/connection.ex
@@ -1307,7 +1307,8 @@ if Code.ensure_loaded?(Tds) do
 
     defp pk_definitions(columns, prefix) do
       pks =
-        for {_, name, _, opts} <- columns,
+        for {action, name, _, opts} <- columns,
+            action != :remove,
             opts[:primary_key],
             do: name
 
@@ -1435,6 +1436,9 @@ if Code.ensure_loaded?(Tds) do
       [statement_prefix, "DROP COLUMN ", quote_name(name), "; "]
     end
 
+    defp column_change(statement_prefix, _table, {:remove, name, _type, _opts}),
+      do: [statement_prefix, "DROP COLUMN ", quote_name(name)]
+
     defp column_change(
            statement_prefix,
            %{name: table, prefix: prefix},


Test patch:
diff --git a/test/ecto/adapters/myxql_test.exs b/test/ecto/adapters/myxql_test.exs
index 37996bb..4968b06 100644
--- a/test/ecto/adapters/myxql_test.exs
+++ b/test/ecto/adapters/myxql_test.exs
@@ -2009,8 +2009,9 @@ defmodule Ecto.Adapters.MyXQLTest do
            ]
   end
 
-  test "alter table with primary key" do
-    alter = {:alter, table(:posts), [{:add, :my_pk, :serial, [primary_key: true]}]}
+  test "alter table add/3 with primary key" do
+    alter =
+      {:alter, table(:posts), [{:add, :my_pk, :serial, [primary_key: true]}]}
 
     assert execute_ddl(alter) == [
              """
@@ -2022,6 +2023,13 @@ defmodule Ecto.Adapters.MyXQLTest do
            ]
   end
 
+  test "alter table remove/3 with primary key" do
+    alter =
+      {:alter, table(:posts), [{:remove, :my_pk, :serial, [primary_key: true]}]}
+
+    assert execute_ddl(alter) == ["ALTER TABLE `posts` DROP `my_pk`" |> remove_newlines]
+  end
+
   test "alter table with invalid reference opts" do
     alter =
       {:alter, table(:posts),
diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs
index 0e15be9..c7c2f6b 100644
--- a/test/ecto/adapters/postgres_test.exs
+++ b/test/ecto/adapters/postgres_test.exs
@@ -2542,6 +2542,15 @@ defmodule Ecto.Adapters.PostgresTest do
            ]
   end
 
+  test "alter table remove/3 with primary key" do
+    alter =
+      {:alter, table(:posts), [{:remove, :my_pk, :serial, [primary_key: true]}]}
+
+    assert execute_ddl(alter) == [
+             "ALTER TABLE \"posts\" DROP COLUMN \"my_pk\"" |> remove_newlines
+           ]
+  end
+
   test "create index" do
     create = {:create, index(:posts, [:category_id, :permalink])}
 
diff --git a/test/ecto/adapters/tds_test.exs b/test/ecto/adapters/tds_test.exs
index b9e344c..567e142 100644
--- a/test/ecto/adapters/tds_test.exs
+++ b/test/ecto/adapters/tds_test.exs
@@ -1646,6 +1646,13 @@ defmodule Ecto.Adapters.TdsTest do
              ]
   end
 
+  test "alter table remove/3 with primary key" do
+    alter =
+      {:alter, table(:posts), [{:remove, :my_pk, :serial, [primary_key: true]}]}
+
+    assert execute_ddl(alter) == ["ALTER TABLE [posts] DROP COLUMN [my_pk]" |> remove_newlines]
+  end
+
   test "alter table with invalid reference opts" do
     alter =
       {:alter, table(:posts),