Repository: elixir-ecto/ecto
Pull request #2272 (2017-10-12 19:54:49)

Pass the executing repo module as the first argument to Multi.run
Originating from forum post: https://groups.google.com/forum/#!topic/elixir-ecto/uqUkVCKhd7c

Passes the repo module through during execution to all run functions as the first argument.

ebert[bot]: Hello, @stavro! This is your first Pull Request that will be reviewed by [Ebert](https://ebertapp.io/), an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its [review page](https://ebertapp.io/github/elixir-ecto/ecto/pulls/2272). Please check our [documentation](https://docs.ebertapp.io) for more information. 
stavro: @josevalim Should we also pass `repo` to functions provided to `Multi.merge/2` and `Multi.merge/4`?

Any run blocks in merged multis will receive the repo just fine, but I'm not familiar with the intended use case of the function for `merge`.

Thanks!
ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 fixed issue! 🎉

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2272.

<!-- Ebert auto generated comment -->

ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 fixed issue! 🎉

But beware that this branch is **[3 commits behind the `elixir-ecto:master` branch](https://github.com/elixir-ecto/ecto/compare/stavro:master...elixir-ecto:master)**, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2272.

<!-- Ebert auto generated comment -->

ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 fixed issue! 🎉

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2272.

<!-- Ebert auto generated comment -->

stavro: This has been updated with the backwards-compatible anonymous function.

I also added a separate commit to fix the typespecs, which I believe were wrong before this PR (the anonymous run function should not receive the Multi as an argument).

Outstanding question: Should we also pass repo to functions provided to Multi.merge/2 and Multi.merge/4? Any run blocks in merged multis will receive the repo just fine, but I'm not familiar with the intended use case of the function for merge.



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 58a69861..1759046c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -29,7 +29,7 @@ And should be rewritten as:
 
 This means Ecto no longer requires Ecto-specific extensions on databases such as Postgres which leads to better integration with 3rd party libraries and faster compilation times.
 
-## v3.0.0-dev (2017-08-18)
+## v3.0.0-dev (In Progress)
 
 ### Enhancements
 
@@ -40,6 +40,7 @@ This means Ecto no longer requires Ecto-specific extensions on databases such as
 
 ### Backwards incompatible changes
 
+  * [Ecto.Multi] `Ecto.Multi.run/3` and `Ecto.Multi.run/5` now receive the repo in which the transaction is executing as the first argument to functions, and the changes so far as the second argument.
   * [Ecto.DateTime] `Ecto.Date`, `Ecto.Time` and `Ecto.DateTime` were previously deprecated and have now been removed
   * [Ecto.Schema]`:time`, `:naive_datetime` and `:utc_datetime` no longer keep microseconds information. If you want to keep microseconds, use `:time_usec`, `:naive_datetime_usec`, `:utc_datetime_usec`
 
diff --git a/lib/ecto/multi.ex b/lib/ecto/multi.ex
index 3f1ba847..96ebe03d 100644
--- a/lib/ecto/multi.ex
+++ b/lib/ecto/multi.ex
@@ -27,10 +27,11 @@ defmodule Ecto.Multi do
   ## Run
 
   Multi allows you to run arbitrary functions as part of your transaction
-  via `run/3` and `run/5`. This is very useful when an operation depends
-  on the value of a previous operation. For this reason, the function given
-  as callback to `run/3` and `run/5` will receive all changes performed by
-  the multi so far as a map in the first argument.
+  via `run/3` and `run/5`. This is especially useful when an operation
+  depends on the value of a previous operation. For this reason, the
+  function given as a callback to `run/3` and `run/5` will receive the repo
+  as the first argument, and all changes performed by the multi so far as a
+  map for the second argument.
 
   The function given to `run` must return `{:ok, value}` or `{:error, value}`
   as its result. Returning an error will abort any further operations
@@ -99,7 +100,7 @@ defmodule Ecto.Multi do
 
   defstruct operations: [], names: MapSet.new
 
-  @type run :: (t -> {:ok | :error, any}) | {module, atom, [any]}
+  @type run :: ((Ecto.Repo.t, map) -> {:ok | :error, any}) | {module, atom, [any]}
   @type merge :: (map -> t) | {module, atom, [any]}
   @typep schema_or_source :: binary | {binary | nil, binary} | atom
   @typep operation :: {:changeset, Changeset.t, Keyword.t} |
@@ -132,8 +133,8 @@ defmodule Ecto.Multi do
 
   ## Example
 
-      iex> lhs = Ecto.Multi.new |> Ecto.Multi.run(:left, &{:ok, &1})
-      iex> rhs = Ecto.Multi.new |> Ecto.Multi.run(:right, &{:error, &1})
+      iex> lhs = Ecto.Multi.new |> Ecto.Multi.run(:left, fn _, changes -> {:ok, changes} end)
+      iex> rhs = Ecto.Multi.new |> Ecto.Multi.run(:right, fn _, changes -> {:error, changes} end)
       iex> Ecto.Multi.append(lhs, rhs) |> Ecto.Multi.to_list |> Keyword.keys
       [:left, :right]
 
@@ -150,8 +151,8 @@ defmodule Ecto.Multi do
 
   ## Example
 
-      iex> lhs = Ecto.Multi.new |> Ecto.Multi.run(:left, &{:ok, &1})
-      iex> rhs = Ecto.Multi.new |> Ecto.Multi.run(:right, &{:error, &1})
+      iex> lhs = Ecto.Multi.new |> Ecto.Multi.run(:left, fn _, changes -> {:ok, changes} end)
+      iex> rhs = Ecto.Multi.new |> Ecto.Multi.run(:right, fn _, changes -> {:error, changes} end)
       iex> Ecto.Multi.prepend(lhs, rhs) |> Ecto.Multi.to_list |> Keyword.keys
       [:right, :left]
 
@@ -239,7 +240,7 @@ defmodule Ecto.Multi do
   def update(multi, name, %Changeset{} = changeset, opts \\ []) do
     add_changeset(multi, :update, name, changeset, opts)
   end
-  
+
   @doc """
   Inserts or updates a changeset depending on whether the changeset was persisted or not.
 
@@ -303,10 +304,17 @@ defmodule Ecto.Multi do
   Adds a function to run as part of the multi.
 
   The function should return either `{:ok, value}` or `{:error, value}`,
-  and receives changes so far as an argument.
+  and receives the repo as the first argument, and the changes so far
+  as the second argument.
   """
-  @spec run(t, name, (t -> {:ok | :error, any})) :: t
+  @spec run(t, name, run) :: t
   def run(multi, name, run) when is_function(run, 1) do
+    IO.warn "Giving an anonymous function with 1 argument to Ecto.Multi.run/3 is deprecated. " <>
+              "Please pass an anonymous function that receives the repository and the changes so far"
+    run(multi, name, fn _repo, changes -> run.(changes) end)
+  end
+
+  def run(multi, name, run) when is_function(run, 2) do
     add_operation(multi, name, {:run, run})
   end
 
@@ -315,8 +323,8 @@ defmodule Ecto.Multi do
 
   Similar to `run/3`, but allows to pass module name, function and arguments.
   The function should return either `{:ok, value}` or `{:error, value}`, and
-  will receive changes so far as the first argument (prepended to those passed in
-  the call to the function).
+  receives the repo as the first argument, and the changes so far as the
+  second argument (prepended to those passed in the call to the function).
   """
   @spec run(t, name, module, function, args) :: t when function: atom, args: [any]
   def run(multi, name, mod, fun, args)
@@ -433,8 +441,8 @@ defmodule Ecto.Multi do
 
   defp apply_operation({:changeset, changeset, opts}, _acc, _apply_args, repo),
     do: apply(repo, changeset.action, [changeset, opts])
-  defp apply_operation({:run, run}, acc, _apply_args, _repo),
-    do: apply_fun(run, acc)
+  defp apply_operation({:run, run}, acc, _apply_args, repo),
+    do: apply_run_fun(run, repo, acc)
   defp apply_operation({:error, value}, _acc, _apply_args, _repo),
     do: {:error, value}
   defp apply_operation({:insert_all, source, entries, opts}, _acc, _apply_args, repo),
@@ -444,10 +452,13 @@ defmodule Ecto.Multi do
   defp apply_operation({:delete_all, query, opts}, _acc, _apply_args, repo),
     do: {:ok, repo.delete_all(query, opts)}
   defp apply_operation({:merge, merge}, acc, {wrap, return}, repo),
-    do: {:merge, __apply__(apply_fun(merge, acc), repo, wrap, return)}
+    do: {:merge, __apply__(apply_merge_fun(merge, acc), repo, wrap, return)}
+
+  defp apply_merge_fun({mod, fun, args}, acc), do: apply(mod, fun, [acc | args])
+  defp apply_merge_fun(fun, acc),              do: apply(fun, [acc])
 
-  defp apply_fun({mod, fun, args}, acc), do: apply(mod, fun, [acc | args])
-  defp apply_fun(fun, acc),              do: apply(fun, [acc])
+  defp apply_run_fun({mod, fun, args}, repo, acc), do: apply(mod, fun, [repo, acc | args])
+  defp apply_run_fun(fun, repo, acc),              do: apply(fun, [repo, acc])
 
   defp merge_results(changes, new_changes, names) do
     new_names = new_changes |> Map.keys |> MapSet.new


Test patch:
diff --git a/test/ecto/multi_test.exs b/test/ecto/multi_test.exs
index 83bcc9b7..b443236d 100644
--- a/test/ecto/multi_test.exs
+++ b/test/ecto/multi_test.exs
@@ -15,8 +15,8 @@ defmodule Ecto.MultiTest do
     end
   end
 
-  def ok(x), do: {:ok, x}
-  def multi(x), do: Multi.new |> Multi.update(:update, Changeset.change(x.insert))
+  def run_ok(repo, _changes), do: {:ok, repo}
+  def multi(changes), do: Multi.new |> Multi.update(:update, Changeset.change(changes.insert))
 
   test "new" do
     assert Multi.new == %Multi{}
@@ -105,7 +105,7 @@ defmodule Ecto.MultiTest do
   end
 
   test "run with fun" do
-    fun = fn changes -> {:ok, changes} end
+    fun = fn _repo, changes -> {:ok, changes} end
     multi =
       Multi.new
       |> Multi.run(:fun, fun)
@@ -115,7 +115,7 @@ defmodule Ecto.MultiTest do
   end
 
   test "run named with tuple" do
-    fun = fn changes -> {:ok, changes} end
+    fun = fn _repo, changes -> {:ok, changes} end
     multi =
       Multi.new
       |> Multi.run({:fun, 3}, fun)
@@ -125,7 +125,7 @@ defmodule Ecto.MultiTest do
   end
 
   test "run named with char_list" do
-    fun = fn changes -> {:ok, changes} end
+    fun = fn _repo, changes -> {:ok, changes} end
     multi =
       Multi.new
       |> Multi.run('myFunction', fun)
@@ -137,10 +137,10 @@ defmodule Ecto.MultiTest do
   test "run with mfa" do
     multi =
       Multi.new
-      |> Multi.run(:fun, __MODULE__, :ok, [])
+      |> Multi.run(:fun, __MODULE__, :run_ok, [])
 
     assert multi.names      == MapSet.new([:fun])
-    assert multi.operations == [{:fun, {:run, {__MODULE__, :ok, []}}}]
+    assert multi.operations == [{:fun, {:run, {__MODULE__, :run_ok, []}}}]
   end
 
   test "insert_all" do
@@ -174,7 +174,7 @@ defmodule Ecto.MultiTest do
   end
 
   test "append/prepend without repetition" do
-    fun = fn _ -> {:ok, :ok} end
+    fun = fn _, _ -> {:ok, :ok} end
     lhs = Multi.new |> Multi.run(:one, fun) |> Multi.run(:two, fun)
     rhs = Multi.new |> Multi.run(:three, fun) |> Multi.run(:four, fun)
 
@@ -190,7 +190,7 @@ defmodule Ecto.MultiTest do
   end
 
   test "append/prepend with repetition" do
-    fun   = fn _ -> {:ok, :ok} end
+    fun   = fn _, _ -> {:ok, :ok} end
     multi = Multi.new |> Multi.run(:run, fun)
 
     assert_raise ArgumentError, ~r"both declared operations: \[:run\]", fn ->
@@ -207,7 +207,7 @@ defmodule Ecto.MultiTest do
     multi =
       Multi.new
       |> Multi.insert(:insert, changeset)
-      |> Multi.run(:run, fn changes -> {:ok, changes} end)
+      |> Multi.run(:run, fn _repo, changes -> {:ok, changes} end)
       |> Multi.update(:update, changeset)
       |> Multi.delete(:delete, changeset)
       |> Multi.insert_all(:insert_all, Comment, [[x: 1]])
@@ -240,7 +240,7 @@ defmodule Ecto.MultiTest do
   end
 
   test "repeating an operation" do
-    fun = fn _ -> {:ok, :ok} end
+    fun = fn _, _ -> {:ok, :ok} end
     assert_raise RuntimeError, ~r":run is already a member", fn ->
       Multi.new |> Multi.run(:run, fun) |> Multi.run(:run, fun)
     end
@@ -274,8 +274,8 @@ defmodule Ecto.MultiTest do
     end
 
     test "rollbacks on errors" do
-      error = fn _ -> {:error, :error} end
-      ok    = fn _ -> {:ok, :ok} end
+      error = fn _, _ -> {:error, :error} end
+      ok    = fn _, _ -> {:ok, :ok} end
 
       multi =
         Multi.new
@@ -293,7 +293,7 @@ defmodule Ecto.MultiTest do
     end
 
     test "does not allow repeated operations" do
-      fun = fn _ -> {:ok, :ok} end
+      fun = fn _, _ -> {:ok, :ok} end
 
       multi =
         Multi.new
@@ -323,7 +323,7 @@ defmodule Ecto.MultiTest do
       multi =
         Multi.new
         |> Multi.insert(:insert, changeset)
-        |> Multi.run(:run, fn changes -> {:ok, changes} end)
+        |> Multi.run(:run, fn _repo, changes -> {:ok, changes} end)
         |> Multi.update(:update, changeset)
         |> Multi.delete(:delete, changeset)
         |> Multi.insert_all(:insert_all, Comment, [[x: 1]])
@@ -356,7 +356,7 @@ defmodule Ecto.MultiTest do
       multi =
         Multi.new
         |> Multi.insert(:insert, changeset)
-        |> Multi.run(:run, fn _changes -> {:error, "error from run"} end)
+        |> Multi.run(:run, fn _repo, _changes -> {:error, "error from run"} end)
         |> Multi.update(:update, changeset)
         |> Multi.delete(:delete, changeset)
 
@@ -369,7 +369,7 @@ defmodule Ecto.MultiTest do
       refute Map.has_key?(changes, :update)
     end
 
-  test "rolls back on error" do
+    test "rolls back on error" do
       changeset = Changeset.change(%Comment{id: 1}, x: 1)
       multi =
         Multi.new
@@ -395,7 +395,7 @@ defmodule Ecto.MultiTest do
       multi =
         Multi.new
         |> Multi.insert(:insert, changeset)
-        |> Multi.run(:run, fn _changes -> {:ok, "ok"} end)
+        |> Multi.run(:run, fn _repo, _changes -> {:ok, "ok"} end)
         |> Multi.update(:update, invalid)
         |> Multi.delete(:delete, changeset)
 
@@ -418,4 +418,19 @@ defmodule Ecto.MultiTest do
       refute_received {:transaction, _}
     end
   end
+
+  describe "Multi.run receives the repo module as the first argument" do
+    test "with anonymous functions" do
+      fun = fn repo, _changes -> {:ok, repo} end
+      multi = Multi.new |> Multi.run(:run, fun)
+      assert {:ok, changes} = TestRepo.transaction(multi)
+      assert changes.run == TestRepo
+    end
+
+    test "with mfa functions" do
+      multi = Multi.new |> Multi.run(:run, __MODULE__, :run_ok, [])
+      assert {:ok, changes} = TestRepo.transaction(multi)
+      assert changes.run == TestRepo
+    end
+  end
 end