Repository: elixir-ecto/ecto_sql
Pull request #624 (2024-07-23 15:33:38)

Add `Ecto.Migration.remove_if_exists/1`
This creates a `...if_exists` mirror for `remove/1`. I'm guessing `remove_if_exists/2` is needed for mysql, but psql will drop fkey constraints as part of dropping the column, so `remove_if_exists/2` is unnecessary in that case.

I also updated the documentation for `remove_if_exists/2` to help make it clearer why the type can/should be passed

josevalim: Sorry, I may be missing something, but shouldn't we promote `remove_if_exists/2` instead? I believe `remove/1` exists mostly for backwards compatibility.
novaugust: > Sorry, I may be missing something, but shouldn't we promote `remove_if_exists/2` instead?

unlike `remove/2`, `remove_if_exists/2` isn't reversible. it uses the type solely to drop fkey constraints - any non `references(...)` type is just fully ignored. if it _is_ a `references(...)` type, it still doesn't matter in the case of psql, which removes any related constraints as part of removing the column. i can't speak to mysql, but assume it still requires fkey constraints are dropped before their columns are, and is the whole reason `/2` exists. 

personally, i often reach for `remove/1` over `remove/2` because we only ever roll production forward - there's no `ecto.rollback` in prod. i went to do the same thing here but with `remove_if_exists` and was surprised that this didn't exist, then sad that i had to go look up the exact types for the 12 columns i was dropping, only to discover that actually that argument was ignored anyways and thus didn't matter. 

since the second argument doesn't matter for any use-case for psql users and lots of use-cases for mysql folks, `remove_if_exists/1` seemed like a better api to me =)

Patch:
diff --git a/.formatter.exs b/.formatter.exs
index 60c8890..3497908 100644
--- a/.formatter.exs
+++ b/.formatter.exs
@@ -19,6 +19,7 @@ locals_without_parens = [
   remove: 1,
   remove: 2,
   remove: 3,
+  remove_if_exists: 1,
   remove_if_exists: 2,
   rename: 2,
   rename: 3,
diff --git a/lib/ecto/adapter/migration.ex b/lib/ecto/adapter/migration.ex
index d637583..d627459 100644
--- a/lib/ecto/adapter/migration.ex
+++ b/lib/ecto/adapter/migration.ex
@@ -39,7 +39,8 @@ defmodule Ecto.Adapter.Migration do
           | {:remove, field :: atom, type :: Ecto.Type.t() | Reference.t() | binary(),
              Keyword.t()}
           | {:remove, field :: atom}
-          | {:remove_if_exists, type :: Ecto.Type.t() | Reference.t() | binary()}
+          | {:remove_if_exists, field :: atom, type :: Ecto.Type.t() | Reference.t() | binary()}
+          | {:remove_if_exists, field :: atom}
 
   @typedoc """
   A struct that represents a table or index in a database schema.
diff --git a/lib/ecto/adapters/myxql/connection.ex b/lib/ecto/adapters/myxql/connection.ex
index 4f1fb57..cc951c5 100644
--- a/lib/ecto/adapters/myxql/connection.ex
+++ b/lib/ecto/adapters/myxql/connection.ex
@@ -1239,7 +1239,10 @@ if Code.ensure_loaded?(MyXQL) do
       [drop_constraint_if_exists_expr(ref, table, name), "DROP IF EXISTS ", quote_name(name)]
     end
 
-    defp column_change(_table, {:remove_if_exists, name, _type}),
+    defp column_change(table, {:remove_if_exists, name, _type}),
+      do: column_change(table, {:remove_if_exists, name})
+
+    defp column_change(_table, {:remove_if_exists, name}),
       do: ["DROP IF EXISTS ", quote_name(name)]
 
     defp column_options(opts) do
diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex
index 53a8af3..b7eac16 100644
--- a/lib/ecto/adapters/postgres/connection.ex
+++ b/lib/ecto/adapters/postgres/connection.ex
@@ -1588,7 +1588,10 @@ if Code.ensure_loaded?(Postgrex) do
       ]
     end
 
-    defp column_change(_table, {:remove_if_exists, name, _type}),
+    defp column_change(table, {:remove_if_exists, name, _type}),
+      do: column_change(table, {:remove_if_exists, name})
+
+    defp column_change(_table, {:remove_if_exists, name}),
       do: ["DROP COLUMN IF EXISTS ", quote_name(name)]
 
     defp modify_null(name, opts) do
diff --git a/lib/ecto/adapters/tds/connection.ex b/lib/ecto/adapters/tds/connection.ex
index 75c0d37..2323520 100644
--- a/lib/ecto/adapters/tds/connection.ex
+++ b/lib/ecto/adapters/tds/connection.ex
@@ -1470,10 +1470,13 @@ if Code.ensure_loaded?(Tds) do
     defp column_change(statement_prefix, _table, {:remove, name, _type, _opts}),
       do: [statement_prefix, "DROP COLUMN ", quote_name(name)]
 
+    defp column_change(statement_prefix, table, {:remove_if_exists, column_name, _}),
+      do: column_change(statement_prefix, table, {:remove_if_exists, column_name})
+
     defp column_change(
            statement_prefix,
            %{name: table, prefix: prefix},
-           {:remove_if_exists, column_name, _}
+           {:remove_if_exists, column_name}
          ) do
       [
         [
diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex
index 13f6c51..7093d38 100644
--- a/lib/ecto/migration.ex
+++ b/lib/ecto/migration.ex
@@ -1358,15 +1358,32 @@ defmodule Ecto.Migration do
   end
 
   @doc """
-  Removes a column only if the column exists when altering the constraint if the reference type is passed
-  once it only has the constraint name on reference structure.
+  Removes a column if the column exists.
 
-  This command is not reversible as Ecto does not know about column existence before the removal attempt.
+  This command is not reversible as Ecto does not know whether or not the column existed before the removal attempt.
 
   ## Examples
 
       alter table("posts") do
-        remove_if_exists :title, :string
+        remove_if_exists :title
+      end
+
+  """
+  def remove_if_exists(column) when is_atom(column) do
+    Runner.subcommand({:remove_if_exists, column})
+  end
+
+  @doc """
+  Removes a column if the column exists.
+
+  If the type is a reference, removes the foreign key constraint for the reference first, if it exists.
+
+  This command is not reversible as Ecto does not know whether or not the column existed before the removal attempt.
+
+  ## Examples
+
+      alter table("posts") do
+        remove_if_exists :author_id, references(:authors)
       end
 
   """


Test patch:
diff --git a/test/ecto/adapters/myxql_test.exs b/test/ecto/adapters/myxql_test.exs
index 89da3fb..c2d499d 100644
--- a/test/ecto/adapters/myxql_test.exs
+++ b/test/ecto/adapters/myxql_test.exs
@@ -1987,6 +1987,7 @@ defmodule Ecto.Adapters.MyXQLTest do
          {:remove, :body, :text, []},
          {:remove, :space_id, %Reference{table: :author}, []},
          {:remove_if_exists, :body, :text},
+         {:remove_if_exists, :body},
          {:remove_if_exists, :space_id, %Reference{table: :author}}
        ]}
 
@@ -2015,6 +2016,7 @@ defmodule Ecto.Adapters.MyXQLTest do
              DROP FOREIGN KEY `posts_space_id_fkey`,
              DROP `space_id`,
              DROP IF EXISTS `body`,
+             DROP IF EXISTS `body`,
              DROP FOREIGN KEY IF EXISTS `posts_space_id_fkey`,
              DROP IF EXISTS `space_id`
              """
diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs
index a5f358f..1c31727 100644
--- a/test/ecto/adapters/postgres_test.exs
+++ b/test/ecto/adapters/postgres_test.exs
@@ -2503,6 +2503,7 @@ defmodule Ecto.Adapters.PostgresTest do
          {:remove, :body, :text, []},
          {:remove, :space_id, %Reference{table: :author}, []},
          {:remove_if_exists, :body, :text},
+         {:remove_if_exists, :body},
          {:remove_if_exists, :space_id, %Reference{table: :author}}
        ]}
 
@@ -2541,6 +2542,7 @@ defmodule Ecto.Adapters.PostgresTest do
              DROP CONSTRAINT "posts_space_id_fkey",
              DROP COLUMN "space_id",
              DROP COLUMN IF EXISTS "body",
+             DROP COLUMN IF EXISTS "body",
              DROP CONSTRAINT IF EXISTS "posts_space_id_fkey",
              DROP COLUMN IF EXISTS "space_id"
              """
diff --git a/test/ecto/migration_test.exs b/test/ecto/migration_test.exs
index 1c86518..4b0ede8 100644
--- a/test/ecto/migration_test.exs
+++ b/test/ecto/migration_test.exs
@@ -469,6 +469,7 @@ defmodule Ecto.MigrationTest do
         remove :views
         remove :status, :string
         remove_if_exists :status, :string
+        remove_if_exists :status
       end
 
       flush()
@@ -481,7 +482,8 @@ defmodule Ecto.MigrationTest do
                   {:modify, :title, :text, []},
                   {:remove, :views},
                   {:remove, :status, :string, []},
-                  {:remove_if_exists, :status, :string}
+                  {:remove_if_exists, :status, :string},
+                  {:remove_if_exists, :status}
                 ]}
     end