Repository: elixir-ecto/ecto
Pull request #2452 (2018-02-28 08:55:00)

Raise error when using where in on_conflict with MySQL
Solution as discussed in #2450.

I also added a spec for `:conflict_target` on MySQL (which was otherwise untested) to build some confidence before adding my own spec for changed behaviour.

ebert[bot]: Hello, @derekkraan! 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/2452). Please check our [documentation](https://docs.ebertapp.io) for more information. 

Patch:
diff --git a/lib/ecto/adapters/mysql/connection.ex b/lib/ecto/adapters/mysql/connection.ex
index a7f16980..54548aa9 100644
--- a/lib/ecto/adapters/mysql/connection.ex
+++ b/lib/ecto/adapters/mysql/connection.ex
@@ -151,9 +151,12 @@ if Code.ensure_loaded?(Mariaex) do
          [quoted, " = VALUES(", quoted, ?)]
        end)]
     end
-    defp on_conflict({query, _, []}, _header) do
+    defp on_conflict({%{wheres: []} = query, _, []}, _header) do
       [" ON DUPLICATE KEY " | update_all(query, "UPDATE ")]
     end
+    defp on_conflict({query, _, []}, _header) do
+      error!(nil, "Using a query with :where in combination with the :on_conflict option is not supported by MySQL")
+    end
 
     defp insert_all(rows) do
       intersperse_map(rows, ?,, fn row ->
diff --git a/lib/ecto/adapters/sql.ex b/lib/ecto/adapters/sql.ex
index 4ebec10e..6681ff1d 100644
--- a/lib/ecto/adapters/sql.ex
+++ b/lib/ecto/adapters/sql.ex
@@ -299,7 +299,7 @@ defmodule Ecto.Adapters.SQL do
       @doc """
       A convenience function for SQL-based repositories that executes the given query.
 
-      See `Ecto.Adapters.SQL.query/3` for more information.
+      See `Ecto.Adapters.SQL.query/4` for more information.
       """
       def query(sql, params \\ [], opts \\ []) do
         Ecto.Adapters.SQL.query(__MODULE__, sql, params, opts)
@@ -308,7 +308,7 @@ defmodule Ecto.Adapters.SQL do
       @doc """
       A convenience function for SQL-based repositories that executes the given query.
 
-      See `Ecto.Adapters.SQL.query/3` for more information.
+      See `Ecto.Adapters.SQL.query!/4` for more information.
       """
       def query!(sql, params \\ [], opts \\ []) do
         Ecto.Adapters.SQL.query!(__MODULE__, sql, params, opts)
@@ -317,7 +317,7 @@ defmodule Ecto.Adapters.SQL do
       @doc """
       A convenience function for SQL-based repositories that translates the given query to SQL.
 
-      See `Ecto.Adapters.SQL.query/3` for more information.
+      See `Ecto.Adapters.SQL.to_sql/3` for more information.
       """
       def to_sql(operation, queryable) do
         Ecto.Adapters.SQL.to_sql(operation, __MODULE__, queryable)
diff --git a/lib/ecto/migrator.ex b/lib/ecto/migrator.ex
index 18ed1416..1efe8ca7 100644
--- a/lib/ecto/migrator.ex
+++ b/lib/ecto/migrator.ex
@@ -25,6 +25,17 @@ defmodule Ecto.Migrator do
   alias Ecto.Migration.Runner
   alias Ecto.Migration.SchemaMigration
 
+  @doc """
+  Gets the migrations path from a repository.
+  """
+  @spec migrations_path(Ecto.Repo.t) :: String.t
+  def migrations_path(repo) do
+    config = repo.config()
+    priv = config[:priv] || "priv/#{repo |> Module.split |> List.last |> Macro.underscore}"
+    app = Keyword.fetch!(config, :otp_app)
+    Application.app_dir(app, Path.join(priv, "migrations"))
+  end
+
   @doc """
   Gets all migrated versions.
 
@@ -161,6 +172,19 @@ defmodule Ecto.Migrator do
     end
   end
 
+  @doc """
+  Runs migrations for the given repository.
+
+  Equivalent to:
+
+      Ecto.Migrator.run(repo, Ecto.Migrator.migrations_path(repo), direction, opts)
+
+  """
+  @spec run(Ecto.Repo.t, atom, Keyword.t) :: [integer]
+  def run(repo, direction, opts) do
+    run(repo, migrations_path(repo), direction, opts)
+  end
+
   @doc """
   Apply migrations to a repository with a given strategy.
 
@@ -207,7 +231,21 @@ defmodule Ecto.Migrator do
   Returns an array of tuples as the migration status of the given repo,
   without actually running any migrations.
 
+  Equivalent to:
+
+      Ecto.Migrator.migrations(repo, Ecto.Migrator.migrations_path(repo))
+
+  """
+  @spec migrations(Ecto.Repo.t) :: [{:up | :down, id :: integer(), name :: String.t}]
+  def migrations(repo) do
+    migrations(repo, migrations_path(repo))
+  end
+
+  @doc """
+  Returns an array of tuples as the migration status of the given repo,
+  without actually running any migrations.
   """
+  @spec migrations(Ecto.Repo.t, String.t) :: [{:up | :down, id :: integer(), name :: String.t}]
   def migrations(repo, directory) do
     repo
     |> migrated_versions
diff --git a/lib/mix/ecto.ex b/lib/mix/ecto.ex
index b092f42a..fd77b87b 100644
--- a/lib/mix/ecto.ex
+++ b/lib/mix/ecto.ex
@@ -115,9 +115,9 @@ defmodule Mix.Ecto do
   @spec ensure_migrations_path(Ecto.Repo.t) :: Ecto.Repo.t | no_return
   def ensure_migrations_path(repo) do
     with false <- Mix.Project.umbrella?,
-         path = Path.relative_to(migrations_path(repo), Mix.Project.app_path),
+         path = Path.join(source_repo_priv(repo), "migrations"),
          false <- File.dir?(path),
-         do: raise_missing_migrations(path, repo)
+         do: raise_missing_migrations(Path.relative_to_cwd(path), repo)
     repo
   end
 
@@ -155,20 +155,13 @@ defmodule Mix.Ecto do
     Logger.add_backend(:console, flush: true)
   end
 
-  @doc """
-  Gets the migrations path from a repository.
-  """
-  @spec migrations_path(Ecto.Repo.t) :: String.t
-  def migrations_path(repo) do
-    Path.join(source_repo_priv(repo), "migrations")
-  end
-
   @doc """
   Returns the private repository path relative to the source.
   """
   def source_repo_priv(repo) do
-    priv = repo.config()[:priv] || "priv/#{repo |> Module.split |> List.last |> Macro.underscore}"
-    app = Keyword.fetch!(repo.config(), :otp_app)
+    config = repo.config()
+    priv = config[:priv] || "priv/#{repo |> Module.split |> List.last |> Macro.underscore}"
+    app = Keyword.fetch!(config, :otp_app)
     Path.join(Mix.Project.deps_paths[app] || File.cwd!, priv)
   end
 
diff --git a/lib/mix/tasks/ecto.gen.migration.ex b/lib/mix/tasks/ecto.gen.migration.ex
index f09f3f82..28835129 100644
--- a/lib/mix/tasks/ecto.gen.migration.ex
+++ b/lib/mix/tasks/ecto.gen.migration.ex
@@ -46,7 +46,7 @@ defmodule Mix.Tasks.Ecto.Gen.Migration do
       case OptionParser.parse(args, switches: @switches) do
         {opts, [name], _} ->
           ensure_repo(repo, args)
-          path = migrations_path(repo)
+          path = Path.join(source_repo_priv(repo), "migrations")
           base_name = "#{underscore(name)}.exs"
           file = Path.join(path, "#{timestamp()}_#{base_name}")
           create_directory path
diff --git a/lib/mix/tasks/ecto.migrate.ex b/lib/mix/tasks/ecto.migrate.ex
index 8167b8aa..1ed8b13f 100644
--- a/lib/mix/tasks/ecto.migrate.ex
+++ b/lib/mix/tasks/ecto.migrate.ex
@@ -55,7 +55,7 @@ defmodule Mix.Tasks.Ecto.Migrate do
   """
 
   @doc false
-  def run(args, migrator \\ &Ecto.Migrator.run/4) do
+  def run(args, migrator \\ &Ecto.Migrator.run/3) do
     repos = parse_repo(args)
 
     {opts, _, _} = OptionParser.parse args,
@@ -81,9 +81,9 @@ defmodule Mix.Tasks.Ecto.Migrate do
       pool = repo.config[:pool]
       migrated =
         if function_exported?(pool, :unboxed_run, 2) do
-          pool.unboxed_run(repo, fn -> migrator.(repo, migrations_path(repo), :up, opts) end)
+          pool.unboxed_run(repo, fn -> migrator.(repo, :up, opts) end)
         else
-          migrator.(repo, migrations_path(repo), :up, opts)
+          migrator.(repo, :up, opts)
         end
 
       pid && repo.stop(pid)
diff --git a/lib/mix/tasks/ecto.migrations.ex b/lib/mix/tasks/ecto.migrations.ex
index 483cd2d8..12625293 100644
--- a/lib/mix/tasks/ecto.migrations.ex
+++ b/lib/mix/tasks/ecto.migrations.ex
@@ -30,7 +30,7 @@ defmodule Mix.Tasks.Ecto.Migrations do
   """
 
   @doc false
-  def run(args, migrations \\ &Ecto.Migrator.migrations/2, puts \\ &IO.puts/1) do
+  def run(args, migrations \\ &Ecto.Migrator.migrations/1, puts \\ &IO.puts/1) do
     repos = parse_repo(args)
 
     result = Enum.map(repos, fn repo ->
@@ -38,7 +38,7 @@ defmodule Mix.Tasks.Ecto.Migrations do
       ensure_migrations_path(repo)
       {:ok, pid, _} = ensure_started(repo, all: true)
 
-      repo_status = migrations.(repo, migrations_path(repo))
+      repo_status = migrations.(repo)
 
       pid && repo.stop(pid)
 
diff --git a/lib/mix/tasks/ecto.rollback.ex b/lib/mix/tasks/ecto.rollback.ex
index bd6a9954..602de125 100644
--- a/lib/mix/tasks/ecto.rollback.ex
+++ b/lib/mix/tasks/ecto.rollback.ex
@@ -48,7 +48,7 @@ defmodule Mix.Tasks.Ecto.Rollback do
   """
 
   @doc false
-  def run(args, migrator \\ &Ecto.Migrator.run/4) do
+  def run(args, migrator \\ &Ecto.Migrator.run/3) do
     repos = parse_repo(args)
 
     {opts, _, _} = OptionParser.parse args,
@@ -74,9 +74,9 @@ defmodule Mix.Tasks.Ecto.Rollback do
       pool = repo.config[:pool]
       migrated =
         if function_exported?(pool, :unboxed_run, 2) do
-          pool.unboxed_run(repo, fn -> migrator.(repo, migrations_path(repo), :down, opts) end)
+          pool.unboxed_run(repo, fn -> migrator.(repo, :down, opts) end)
         else
-          migrator.(repo, migrations_path(repo), :down, opts)
+          migrator.(repo, :down, opts)
         end
 
       pid && repo.stop(pid)


Test patch:
diff --git a/test/ecto/adapters/mysql_test.exs b/test/ecto/adapters/mysql_test.exs
index 2a39e5a9..c7e133a5 100644
--- a/test/ecto/adapters/mysql_test.exs
+++ b/test/ecto/adapters/mysql_test.exs
@@ -538,6 +538,15 @@ defmodule Ecto.Adapters.MySQLTest do
 
     query = insert(nil, "schema", [:x, :y], [[:x, :y]], {[:x, :y], [], []}, [])
     assert query == ~s{INSERT INTO `schema` (`x`,`y`) VALUES (?,?) ON DUPLICATE KEY UPDATE `x` = VALUES(`x`),`y` = VALUES(`y`)}
+
+    assert_raise ArgumentError, "The :conflict_target option is not supported in insert/insert_all by MySQL", fn ->
+      insert(nil, "schema", [:x, :y], [[:x, :y]], {[:x, :y], [], [:x]}, [])
+    end
+
+    assert_raise ArgumentError, "Using a query with :where in combination with the :on_conflict option is not supported by MySQL", fn ->
+      update = from("schema", update: [set: [x: ^"foo"]], where: [z: "bar"]) |> normalize(:update_all)
+      insert(nil, "schema", [:x, :y], [[:x, :y]], {update, [], []}, [])
+    end
   end
 
   test "update" do
diff --git a/test/mix/ecto_test.exs b/test/mix/ecto_test.exs
index b3c7d494..76c530ea 100644
--- a/test/mix/ecto_test.exs
+++ b/test/mix/ecto_test.exs
@@ -61,10 +61,10 @@ defmodule Mix.EctoTest do
     assert_raise Mix.Error, fn -> ensure_started(Repo, []) end
   end
 
-  test "migrations path" do
+  test "source_priv_repo" do
     Process.put(:priv, nil)
-    assert migrations_path(Repo) == Path.expand("priv/repo/migrations", File.cwd!)
+    assert source_repo_priv(Repo) == Path.expand("priv/repo", File.cwd!)
     Process.put(:priv, "hello")
-    assert migrations_path(Repo) == Path.expand("hello/migrations", File.cwd!)
+    assert source_repo_priv(Repo) == Path.expand("hello", File.cwd!)
   end
 end
diff --git a/test/mix/tasks/ecto.migrate_test.exs b/test/mix/tasks/ecto.migrate_test.exs
index 944c7280..ce2182e3 100644
--- a/test/mix/tasks/ecto.migrate_test.exs
+++ b/test/mix/tasks/ecto.migrate_test.exs
@@ -56,7 +56,7 @@ defmodule Mix.Tasks.Ecto.MigrateTest do
 
   test "runs the migrator with app_repo config" do
     Application.put_env(:ecto, :ecto_repos, [Repo])
-    run ["--no-start"], fn _, _, _, _ ->
+    run ["--no-start"], fn _, _, _ ->
       Process.put(:migrated, true)
       []
     end
@@ -67,7 +67,7 @@ defmodule Mix.Tasks.Ecto.MigrateTest do
   end
 
   test "runs the migrator after starting repo" do
-    run ["-r", to_string(Repo), "--no-start"], fn _, _, _, _ ->
+    run ["-r", to_string(Repo), "--no-start"], fn _, _, _ ->
       Process.put(:migrated, true)
       []
     end
@@ -76,7 +76,7 @@ defmodule Mix.Tasks.Ecto.MigrateTest do
   end
 
   test "runs the migrator with the already started repo" do
-    run ["-r", to_string(StartedRepo), "--no-start"], fn _, _, _, _ ->
+    run ["-r", to_string(StartedRepo), "--no-start"], fn _, _, _ ->
       Process.put(:migrated, true)
       []
     end
@@ -85,7 +85,7 @@ defmodule Mix.Tasks.Ecto.MigrateTest do
   end
 
   test "runs the migrator with two repos" do
-    run ["-r", to_string(Repo), "-r", to_string(StartedRepo), "--no-start"], fn _, _, _, _ ->
+    run ["-r", to_string(Repo), "-r", to_string(StartedRepo), "--no-start"], fn _, _, _ ->
       Process.put(:migrated, true)
       []
     end
@@ -95,9 +95,8 @@ defmodule Mix.Tasks.Ecto.MigrateTest do
   end
 
   test "runs the migrator yielding the repository and migrations path" do
-    run ["-r", to_string(Repo), "--quiet", "--prefix", "foo"], fn repo, path, direction, opts ->
+    run ["-r", to_string(Repo), "--quiet", "--prefix", "foo"], fn repo, direction, opts ->
       assert repo == Repo
-      assert path == Path.expand("tmp/#{inspect(Ecto.Migrate)}/migrations", File.cwd!)
       assert direction == :up
       assert opts[:all] == true
       assert opts[:log] == false
@@ -110,7 +109,7 @@ defmodule Mix.Tasks.Ecto.MigrateTest do
   test "raises when migrations path does not exist" do
     File.rm_rf!(@migrations_path)
     assert_raise Mix.Error, fn ->
-      run ["-r", to_string(Repo)], fn _, _, _, _ -> [] end
+      run ["-r", to_string(Repo)], fn _, _, _ -> [] end
     end
     assert !Process.get(:started)
   end
diff --git a/test/mix/tasks/ecto.migrations_test.exs b/test/mix/tasks/ecto.migrations_test.exs
index 05eb724f..08706013 100644
--- a/test/mix/tasks/ecto.migrations_test.exs
+++ b/test/mix/tasks/ecto.migrations_test.exs
@@ -39,7 +39,7 @@ defmodule Mix.Tasks.Ecto.MigrationsTest do
   test "migrations displays the up and down status for the default repo" do
     Application.put_env(:ecto, :ecto_repos, [Repo])
 
-    migrations = fn _, _ ->
+    migrations = fn _ ->
       [
         {:up,   20160000000001, "up_migration_1"},
         {:up,   20160000000002, "up_migration_2"},
@@ -65,7 +65,7 @@ defmodule Mix.Tasks.Ecto.MigrationsTest do
   end
 
   test "migrations displays the up and down status for any given repo" do
-    migrations = fn _, _ ->
+    migrations = fn _ ->
       [
         {:up,   20160000000001, "up_migration_1"},
         {:down, 20160000000002, "down_migration_1"}
diff --git a/test/mix/tasks/ecto.rollback_test.exs b/test/mix/tasks/ecto.rollback_test.exs
index 49067e2b..d857f8ec 100644
--- a/test/mix/tasks/ecto.rollback_test.exs
+++ b/test/mix/tasks/ecto.rollback_test.exs
@@ -54,7 +54,7 @@ defmodule Mix.Tasks.Ecto.RollbackTest do
   end
 
   test "runs the migrator after starting repo" do
-    run ["-r", to_string(Repo), "--no-start"], fn _, _, _, _ ->
+    run ["-r", to_string(Repo), "--no-start"], fn _, _, _ ->
       Process.put(:migrated, true)
       []
     end
@@ -63,7 +63,7 @@ defmodule Mix.Tasks.Ecto.RollbackTest do
   end
 
   test "runs the migrator with already started repo" do
-    run ["-r", to_string(StartedRepo), "--no-start"], fn _, _, _, _ ->
+    run ["-r", to_string(StartedRepo), "--no-start"], fn _, _, _ ->
       Process.put(:migrated, true)
       []
     end
@@ -71,9 +71,8 @@ defmodule Mix.Tasks.Ecto.RollbackTest do
   end
 
   test "runs the migrator yielding the repository and migrations path" do
-    run ["-r", to_string(Repo), "--prefix", "foo"], fn repo, path, direction, opts ->
+    run ["-r", to_string(Repo), "--prefix", "foo"], fn repo, direction, opts ->
       assert repo == Repo
-      assert path == Path.expand("tmp/#{inspect(Ecto.Migrate)}/migrations", File.cwd!)
       assert direction == :down
       assert opts[:step] == 1
       assert opts[:prefix] == "foo"
@@ -85,7 +84,7 @@ defmodule Mix.Tasks.Ecto.RollbackTest do
   test "raises when migrations path does not exist" do
     File.rm_rf!(@migrations_path)
     assert_raise Mix.Error, fn ->
-      run ["-r", to_string(Repo)], fn _, _, _, _ -> [] end
+      run ["-r", to_string(Repo)], fn _, _, _ -> [] end
     end
     assert !Process.get(:started)
   end