From 30028cd027d71e532666b2abb645db5205e3aa01 Mon Sep 17 00:00:00 2001 From: Tomasz Marek Sulima Date: Thu, 31 Oct 2019 18:13:10 +0100 Subject: [PATCH 1/2] Ensuring that flush() will raise on rollback if called from change/0. --- integration_test/sql/migration.exs | 23 +++++++++++++++++++++++ lib/ecto/migration.ex | 18 ++++++++++-------- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/integration_test/sql/migration.exs b/integration_test/sql/migration.exs index 2a55a8f7..0a7f6a80 100644 --- a/integration_test/sql/migration.exs +++ b/integration_test/sql/migration.exs @@ -330,6 +330,19 @@ defmodule Ecto.Integration.MigrationTest do end end + defmodule EmptyUpDownMigration do + use Ecto.Migration + + def up, do: flush() + def down, do: flush() + end + + defmodule EmptyChangeMigration do + use Ecto.Migration + + def change, do: flush() + end + import Ecto.Query, only: [from: 2] import Ecto.Migrator, only: [up: 4, down: 4] @@ -341,6 +354,16 @@ defmodule Ecto.Integration.MigrationTest do {:ok, migration_number: System.unique_integer([:positive]) + @base_migration} end + test "flush", %{migration_number: num} do + assert :ok == up(PoolRepo, num, EmptyUpDownMigration, log: false) + assert :ok == down(PoolRepo, num, EmptyUpDownMigration, log: false) + assert :ok == up(PoolRepo, num, EmptyChangeMigration, log: false) + message = "Calling flush() inside change when doing rollback is not supported." + assert_raise(ArgumentError, message, fn -> + down(PoolRepo, num, EmptyChangeMigration, log: false) + end) + end + test "create and drop table and indexes", %{migration_number: num} do assert :ok == up(PoolRepo, num, CreateMigration, log: false) assert :ok == down(PoolRepo, num, CreateMigration, log: false) diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex index 2d1d8228..30eaafbc 100644 --- a/lib/ecto/migration.ex +++ b/lib/ecto/migration.ex @@ -1096,14 +1096,16 @@ defmodule Ecto.Migration do struct(%Constraint{table: table, name: name}, opts) end - @doc """ - Executes queue migration commands. - - Reverses the order in which commands are executed when doing a rollback - on a `change/0` function and resets the commands queue. - """ - def flush do - Runner.flush + @doc "Executes queue migration commands." + defmacro flush do + quote bind_quoted: [caller: __CALLER__.module] do + if direction() == :down and function_exported?(caller, :change, 0) do + message = "Calling flush() inside change when doing rollback is not supported." + raise ArgumentError, message + else + Runner.flush() + end + end end # Validation helpers From 5174195433a3eeba99097a52fc0873d915d1c27a Mon Sep 17 00:00:00 2001 From: Tomasz Marek Sulima Date: Fri, 1 Nov 2019 10:29:17 +0100 Subject: [PATCH 2/2] Applied suggested changes: 1. Use __MODULE__ inside quote instead of __CALLER__ in bind_quoted 2. Fix check in if 3. Changed error message to start with lowercase 4. Use raise/1 instead of raise/2 with ArgumentError 5. Moved test from integration_test/sql/migration.ex to test/ecto/migrator_test.exs --- integration_test/sql/migration.exs | 23 ----------------------- lib/ecto/migration.ex | 7 +++---- test/ecto/migrator_test.exs | 25 +++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/integration_test/sql/migration.exs b/integration_test/sql/migration.exs index 0a7f6a80..2a55a8f7 100644 --- a/integration_test/sql/migration.exs +++ b/integration_test/sql/migration.exs @@ -330,19 +330,6 @@ defmodule Ecto.Integration.MigrationTest do end end - defmodule EmptyUpDownMigration do - use Ecto.Migration - - def up, do: flush() - def down, do: flush() - end - - defmodule EmptyChangeMigration do - use Ecto.Migration - - def change, do: flush() - end - import Ecto.Query, only: [from: 2] import Ecto.Migrator, only: [up: 4, down: 4] @@ -354,16 +341,6 @@ defmodule Ecto.Integration.MigrationTest do {:ok, migration_number: System.unique_integer([:positive]) + @base_migration} end - test "flush", %{migration_number: num} do - assert :ok == up(PoolRepo, num, EmptyUpDownMigration, log: false) - assert :ok == down(PoolRepo, num, EmptyUpDownMigration, log: false) - assert :ok == up(PoolRepo, num, EmptyChangeMigration, log: false) - message = "Calling flush() inside change when doing rollback is not supported." - assert_raise(ArgumentError, message, fn -> - down(PoolRepo, num, EmptyChangeMigration, log: false) - end) - end - test "create and drop table and indexes", %{migration_number: num} do assert :ok == up(PoolRepo, num, CreateMigration, log: false) assert :ok == down(PoolRepo, num, CreateMigration, log: false) diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex index 30eaafbc..d791cdc7 100644 --- a/lib/ecto/migration.ex +++ b/lib/ecto/migration.ex @@ -1098,10 +1098,9 @@ defmodule Ecto.Migration do @doc "Executes queue migration commands." defmacro flush do - quote bind_quoted: [caller: __CALLER__.module] do - if direction() == :down and function_exported?(caller, :change, 0) do - message = "Calling flush() inside change when doing rollback is not supported." - raise ArgumentError, message + quote do + if direction() == :down and not function_exported?(__MODULE__, :down, 0) do + raise "calling flush() inside change when doing rollback is not supported." else Runner.flush() end diff --git a/test/ecto/migrator_test.exs b/test/ecto/migrator_test.exs index de5cbc8a..82c3b889 100644 --- a/test/ecto/migrator_test.exs +++ b/test/ecto/migrator_test.exs @@ -121,6 +121,19 @@ defmodule Ecto.MigratorTest do use Ecto.Repo, otp_app: :ecto_sql, adapter: EctoSQL.TestAdapter end + defmodule EmptyUpDownMigration do + use Ecto.Migration + + def up, do: flush() + def down, do: flush() + end + + defmodule EmptyChangeMigration do + use Ecto.Migration + + def change, do: flush() + end + Application.put_env(:ecto_sql, MigrationSourceRepo, [migration_source: "my_schema_migrations"]) setup do @@ -156,6 +169,18 @@ defmodule Ecto.MigratorTest do """ end + @tag :current + test "flush" do + num = System.unique_integer([:positive]) + assert :ok == up(TestRepo, num, EmptyUpDownMigration, log: false) + assert :ok == down(TestRepo, num, EmptyUpDownMigration, log: false) + assert :ok == up(TestRepo, num, EmptyChangeMigration, log: false) + message = "calling flush() inside change when doing rollback is not supported." + assert_raise(RuntimeError, message, fn -> + down(TestRepo, num, EmptyChangeMigration, log: false) + end) + end + test "custom schema migrations table is right" do assert SchemaMigration.get_source(TestRepo) == "schema_migrations" assert SchemaMigration.get_source(MigrationSourceRepo) == "my_schema_migrations"