Repository: elixir-ecto/ecto
Pull request #4277 (2023-09-15 00:22:10)

Add returning to delete
This PR adds the returning option to the delete function, which aligns with update and insert.

This is useful if you want to listen to telemetry events for deleted rows from another process that would execute: `MyRepo.delete(%MySchema{}, returning: true)`

The Ecto.SQL companion PR can be found here:  [#559](https://github.com/elixir-ecto/ecto_sql/pull/559).

Currently, TDS and Postgres support returning; for MySQL, one could use a temporary table: https://stackoverflow.com/questions/33115526/in-mysql-can-i-return-deleted-rows-after-a-deletion and https://dev.mysql.com/doc/refman/8.0/en/internal-temporary-tables.html.

I copied and pasted the code from insert and update, and I'm unsure if I need to call: `add_read_after_writes` or `fields_to_sources` after `returning`. If anyone has some guidance on this, it would be highly appreciated!

greg-rychlewski: Thanks for the PR. I think it makes sense but I'm not sure what the stance will be on [breaking the existing behaviour for adapters](https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/adapter/schema.ex#L90).

If it is accepted then some comments:

- We should add integration tests with the tag `:returning`
- We should document the option
- `fields_to_sources` is a must because it maps schema field names to database column names if they are different. also it puts `returning` into the right structure to be used by the adapters
- using `reads_after_writes` makes sense to me since insert/update are doing it. the one argument i see against it is that it will change the return value of existing apps. i'm not sure if that is enough to not use it though. 
josevalim: We could keep it backwards compatible by checking inside the Ecto.Repo.Schema if the adapter implements delete/5 or delete/4 with `function_exported?(module, :delete, 5)`. If there is no delete/5, we don't pass the return types for now. Then we can remove the check on later Ecto versions.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 311d59c4..0d5369a4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@
   * [Ecto.Query] Allow hints to inject SQL using `unsafe_fragment`
   * [Ecto.Repo] Add `:on_preload_spawn` option to `preload/3`
   * [Ecto.Schema] Support `:load_in_query` option for embeds
+  * [Ecto.Schema] Support `:returning` option for delete
 
 ### Bug fixes
 
diff --git a/lib/ecto/adapter/schema.ex b/lib/ecto/adapter/schema.ex
index 09f66394..57c7bca2 100644
--- a/lib/ecto/adapter/schema.ex
+++ b/lib/ecto/adapter/schema.ex
@@ -87,6 +87,6 @@ defmodule Ecto.Adapter.Schema do
   in case there is no record matching the given filters,
   `{:error, :stale}` is returned.
   """
-  @callback delete(adapter_meta, schema_meta, filters, options) ::
+  @callback delete(adapter_meta, schema_meta, filters, returning, options) ::
               {:ok, fields} | {:invalid, constraints} | {:error, :stale}
 end
diff --git a/lib/ecto/changeset.ex b/lib/ecto/changeset.ex
index 627e3a7e..84ba1302 100644
--- a/lib/ecto/changeset.ex
+++ b/lib/ecto/changeset.ex
@@ -367,6 +367,7 @@ defmodule Ecto.Changeset do
         }
   @type data :: map()
   @type types :: map()
+  @type traverse_result :: %{atom => [term] | traverse_result}
 
   @typedoc """
   A possible value that you can pass to the `:empty_values` option.
@@ -3996,7 +3997,7 @@ defmodule Ecto.Changeset do
   description.
   """
   @spec traverse_errors(t, (error -> String.t()) | (Changeset.t(), atom, error -> String.t())) ::
-          %{atom => [term]}
+          traverse_result
   def traverse_errors(
         %Changeset{errors: errors, changes: changes, types: types} = changeset,
         msg_func
@@ -4093,7 +4094,7 @@ defmodule Ecto.Changeset do
   @spec traverse_validations(
           t,
           (error -> String.t()) | (Changeset.t(), atom, error -> String.t())
-        ) :: %{atom => [term]}
+        ) :: traverse_result
   def traverse_validations(
         %Changeset{validations: validations, changes: changes, types: types} = changeset,
         msg_func
diff --git a/lib/ecto/embedded.ex b/lib/ecto/embedded.ex
index 7f0099f7..da42ad80 100644
--- a/lib/ecto/embedded.ex
+++ b/lib/ecto/embedded.ex
@@ -168,14 +168,14 @@ defmodule Ecto.Embedded do
   end
 
   defp prepare_each(%{cardinality: :one} = embed, changeset, adapter, repo, repo_action) do
-    action = check_action!(changeset.action, repo_action, embed)
+    action = normalize_action(changeset.action, repo_action, embed)
     changeset = run_prepare(changeset, repo)
     to_struct(changeset, action, embed, adapter)
   end
 
   defp prepare_each(%{cardinality: :many} = embed, changesets, adapter, repo, repo_action) do
     for changeset <- changesets,
-        action = check_action!(changeset.action, repo_action, embed),
+        action = normalize_action(changeset.action, repo_action, embed),
         changeset = run_prepare(changeset, repo),
         prepared = to_struct(changeset, action, embed, adapter),
         do: prepared
@@ -244,15 +244,9 @@ defmodule Ecto.Embedded do
     struct(struct, changes)
   end
 
-  defp check_action!(:replace, action, %{on_replace: :delete} = embed),
-    do: check_action!(:delete, action, embed)
-
-  defp check_action!(:update, :insert, %{related: schema}) do
-    raise ArgumentError,
-          "got action :update in changeset for embedded #{inspect(schema)} while inserting"
-  end
-
-  defp check_action!(action, _, _), do: action
+  defp normalize_action(:replace, _, %{on_replace: :delete}), do: :delete
+  defp normalize_action(:update, :insert, _), do: :insert
+  defp normalize_action(action, _, _), do: action
 
   defp autogenerate_id(changes, _struct, :insert, schema, adapter) do
     case schema.__schema__(:autogenerate_id) do
diff --git a/lib/ecto/repo.ex b/lib/ecto/repo.ex
index e7739163..f74074ea 100644
--- a/lib/ecto/repo.ex
+++ b/lib/ecto/repo.ex
@@ -1888,6 +1888,12 @@ defmodule Ecto.Repo do
 
   ## Options
 
+    * `:returning` - selects which fields to return. It accepts a list
+      of fields to be returned from the database. When `true`, returns
+      all fields. When `false`, no extra fields are returned. It will
+      always include all fields in `read_after_writes`. Not all
+      databases support this option.
+
     * `:prefix` - The prefix to run the query on (such as the schema path
       in Postgres or the database in MySQL). This overrides the prefix set
       in the query and any `@schema_prefix` set in the schema.
diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex
index ec661da4..86a02659 100644
--- a/lib/ecto/repo/schema.ex
+++ b/lib/ecto/repo/schema.ex
@@ -540,6 +540,12 @@ defmodule Ecto.Repo.Schema do
     dumper = schema.__schema__(:dump)
     changeset = put_repo_and_action(changeset, :delete, repo, tuplet)
 
+    {return_types, return_sources} =
+      schema
+      |> returning(opts)
+      |> add_read_after_writes(schema)
+      |> fields_to_sources(dumper)
+
     wrap_in_transaction(adapter, adapter_meta, opts, assocs != [], prepare, fn ->
       changeset = run_prepare(changeset, prepare)
 
@@ -555,11 +561,16 @@ defmodule Ecto.Repo.Schema do
         schema_meta = metadata(struct, schema.__schema__(:autogenerate_id), opts)
         filter_values = Enum.map(filters, &elem(&1, 1))
         opts = Keyword.put(opts, :cast_params, filter_values)
-        args = [adapter_meta, schema_meta, dump_filters, opts]
+        # Remove backwards compatibility in later release
+        args = if function_exported?(adapter, :delete, 5) do
+                [adapter_meta, schema_meta, dump_filters, return_sources, opts]
+               else
+                [adapter_meta, schema_meta, dump_filters, opts]
+               end
 
         case apply(changeset, adapter, :delete, args) do
           {:ok, values} ->
-            changeset = load_changes(changeset, :deleted, [], values, %{}, [], adapter, schema_meta)
+            changeset = load_changes(changeset, :deleted, return_types, values, %{}, [], adapter, schema_meta)
             {:ok, changeset.data}
 
           {:error, _} = error ->


Test patch:
diff --git a/test/ecto/repo_test.exs b/test/ecto/repo_test.exs
index aa5c5528..5960479e 100644
--- a/test/ecto/repo_test.exs
+++ b/test/ecto/repo_test.exs
@@ -36,6 +36,11 @@ defmodule Ecto.RepoTest do
 
     embedded_schema do
       field :x, :string
+      field :y, :string
+    end
+
+    def changeset(struct, params) do
+      Ecto.Changeset.cast(struct, params, [:x])
     end
   end
 
@@ -131,7 +136,7 @@ defmodule Ecto.RepoTest do
 
     schema "my_schema" do
       field :x, :string
-      embeds_many :embeds, MyEmbed
+      embeds_many :embeds, MyEmbed, on_replace: :delete
     end
   end
 
@@ -521,6 +526,18 @@ defmodule Ecto.RepoTest do
       TestRepo.update(changeset, returning: false)
       assert_received {:update, %{source: "my_schema", returning: []}}
     end
+
+    test "on delete" do
+      changeset = Ecto.Changeset.change(%MySchemaWithAssoc{id: 1}, %{n: 2})
+      TestRepo.delete(changeset, returning: [:id])
+      assert_received {:delete, %{source: "my_schema", returning: [:id]}}
+      TestRepo.delete(changeset, returning: [:parent_id])
+      assert_received {:delete, %{source: "my_schema", returning: [:parent_id]}}
+      TestRepo.delete(changeset, returning: true)
+      assert_received {:delete, %{source: "my_schema", returning: [:parent_id, :n, :id]}}
+      TestRepo.delete(changeset, returning: false)
+      assert_received {:delete, %{source: "my_schema", returning: []}}
+    end
   end
 
   describe "insert_all" do
@@ -1196,6 +1213,30 @@ defmodule Ecto.RepoTest do
         TestRepo.insert_or_update %MySchema{}
       end
     end
+
+    test "insert surfaces embed fields" do
+      # embeds_one
+      inserted =
+        %MySchemaEmbedsOne{embed: %MyEmbed{x: "old_x", y: "old_y"}}
+        |> Ecto.Changeset.cast(%{embed: %{x: "new_x"}}, [])
+        |> Ecto.Changeset.cast_embed(:embed)
+        |> TestRepo.insert!()
+
+      assert %{x: "new_x", y: "old_y"} = inserted.embed
+
+      # embeds_many
+      data_embed1 = %MyEmbed{id: 1, x: "old_x_1", y: "old_y_1"}
+      data_embed2 = %MyEmbed{id: 2, x: "old_x_2"}
+
+      inserted =
+        %MySchemaEmbedsMany{embeds: [data_embed1, data_embed2]}
+        |> Ecto.Changeset.cast(%{embeds: [%{id: 1, x: "new_x_1"}, %{}]}, [])
+        |> Ecto.Changeset.cast_embed(:embeds)
+        |> TestRepo.insert!()
+
+      assert [%{id: 1, x: "new_x_1", y: "old_y_1"}, %{id: new_id, x: nil}] = inserted.embeds
+      assert new_id != data_embed2.id
+    end
   end
 
   test "get, get_by, one and all sets schema prefix" do
diff --git a/test/support/test_repo.exs b/test/support/test_repo.exs
index 18b9d902..26e6bc0d 100644
--- a/test/support/test_repo.exs
+++ b/test/support/test_repo.exs
@@ -117,13 +117,13 @@ defmodule Ecto.TestAdapter do
     context
   end
 
-  def delete(_, %{context: nil} = meta, filters, _opts) do
-    meta = Map.merge(meta, %{filters: filters})
+  def delete(_, %{context: nil} = meta, filters, returning, _opts) do
+    meta = Map.merge(meta, %{filters: filters, returning: returning})
     send(self(), {:delete, meta})
-    {:ok, []}
+    {:ok, Enum.zip(returning, 1..length(returning))}
   end
 
-  def delete(_, %{context: context}, _filters, _opts) do
+  def delete(_, %{context: context}, _filters, _returning, _opts) do
     context
   end