From 70ff9f14878e161d8281fb83cd162f8e8b076d91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Sat, 11 May 2019 10:44:43 +0200 Subject: [PATCH] Add Ecto.Migrator.with_repo/2 to start/stop apps for migrations (#113) This will be useful to create minimum release scripts that migrate or rollback the repository, such as: defmodule MyApp.Release do @app :my_app def migrate do for repo <- repos() do Ecto.Migrator.with_repo(repo, &Ecto.Migrator.run(&1, :up, all: true)) end end def rollback(repo, version) do Ecto.Migrator.with_repo(repo, &Ecto.Migrator.run(&1, :down, to: version)) end defp repos do Application.get_env(@app, :ecto_repos, []) end end There is also a configuration called :start_apps_before_migration that will help guarantee any auxiliary app is started before migrations run. --- lib/ecto/migration.ex | 5 +++ lib/ecto/migrator.ex | 63 ++++++++++++++++++++++++++++++++ lib/mix/ecto_sql.ex | 35 ------------------ lib/mix/tasks/ecto.migrate.ex | 25 ++++++++----- lib/mix/tasks/ecto.migrations.ex | 46 ++++++++++++----------- lib/mix/tasks/ecto.rollback.ex | 25 ++++++++----- test/ecto/migrator_test.exs | 39 ++++++++++++++++++++ test/mix/ecto_sql_test.exs | 20 ---------- 8 files changed, 164 insertions(+), 94 deletions(-) diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex index d0614faf..bea783ba 100644 --- a/lib/ecto/migration.ex +++ b/lib/ecto/migration.ex @@ -204,6 +204,11 @@ defmodule Ecto.Migration do config :app, App.Repo, migration_default_prefix: "my_prefix" + * `:start_apps_before_migration` - A list of applications to be started before + running migrations. Used by `Ecto.Migrator.with_repo/3` and the migration tasks: + + config :app, App.Repo, start_apps_before_migration: [:ssl, :some_custom_logger] + """ @doc """ diff --git a/lib/ecto/migrator.ex b/lib/ecto/migrator.ex index 3b156833..756246e2 100644 --- a/lib/ecto/migrator.ex +++ b/lib/ecto/migrator.ex @@ -25,6 +25,69 @@ defmodule Ecto.Migrator do alias Ecto.Migration.Runner alias Ecto.Migration.SchemaMigration + @doc """ + Ensures the repo is started to perform migration operations. + + All of the application reqeuired to run the repo will be started + before hand with chosen mode. If the repo has not yet been started, + it is manually started before, with a `:pool_size` of 2, the given + function is executed, and the repo is terminated. If the repo was + already started, then the function is directly executed, without + terminating the repo afterwards. + + The repo may also configure a `:start_apps_before_migration` option + which is a list of applications to be started before the migration + runs. + + It returns `{:ok, fun_return, apps}`, with all apps that have been + started, or `{:error, term}`. + + ## Options + + * `:pool_size` - The pool size to start the repo for migrations. + Defaults to 2. + * `:mode` - The mode to start all applications. + Defaults to `:permanent`. + + ## Examples + + {:ok, _, _} = + Ecto.Migrator.with_repo(repo, fn repo -> + Ecto.Migrator.run(repo, :up, all: true) + end) + + """ + def with_repo(repo, fun, opts \\ []) do + config = repo.config() + mode = Keyword.get(opts, :mode, :permanent) + apps = [:ecto_sql | config[:start_apps_before_migration] || []] + + extra_started = + Enum.flat_map(apps, fn app -> + {:ok, started} = Application.ensure_all_started(app, mode) + started + end) + + {:ok, repo_started} = repo.__adapter__.ensure_all_started(config, mode) + started = extra_started ++ repo_started + pool_size = Keyword.get(opts, :pool_size, 2) + + case repo.start_link(pool_size: pool_size) do + {:ok, _} -> + try do + {:ok, fun.(repo), started} + after + repo.stop() + end + + {:error, {:already_started, _pid}} -> + {:ok, fun.(repo), started} + + {:error, _} = error -> + error + end + end + @doc """ Gets the migrations path from a repository. """ diff --git a/lib/mix/ecto_sql.ex b/lib/mix/ecto_sql.ex index 0d6b53af..aa3ffb64 100644 --- a/lib/mix/ecto_sql.ex +++ b/lib/mix/ecto_sql.ex @@ -1,41 +1,6 @@ defmodule Mix.EctoSQL do @moduledoc false - @doc """ - Ensures the given repository is started and running. - """ - @spec ensure_started(Ecto.Repo.t, Keyword.t) :: {:ok, pid | nil, [atom]} - def ensure_started(repo, opts) do - {:ok, started} = Application.ensure_all_started(:ecto_sql) - - # If we starting EctoSQL just now, assume - # logger has not been properly booted yet. - if :ecto_sql in started && Process.whereis(Logger) do - backends = Application.get_env(:logger, :backends, []) - try do - Logger.App.stop - Application.put_env(:logger, :backends, [:console]) - :ok = Logger.App.start - after - Application.put_env(:logger, :backends, backends) - end - end - - {:ok, apps} = repo.__adapter__.ensure_all_started(repo.config(), :temporary) - pool_size = Keyword.get(opts, :pool_size, 2) - - case repo.start_link(pool_size: pool_size) do - {:ok, pid} -> - {:ok, pid, apps} - - {:error, {:already_started, _pid}} -> - {:ok, nil, apps} - - {:error, error} -> - Mix.raise "Could not start repo #{inspect repo}, error: #{inspect error}" - end - end - @doc """ Ensures the given repository's migrations path exists on the file system. """ diff --git a/lib/mix/tasks/ecto.migrate.ex b/lib/mix/tasks/ecto.migrate.ex index b1052399..9c4662dd 100644 --- a/lib/mix/tasks/ecto.migrate.ex +++ b/lib/mix/tasks/ecto.migrate.ex @@ -74,7 +74,7 @@ defmodule Mix.Tasks.Ecto.Migrate do * `--to` - run all migrations up to and including version * `--quiet` - do not log migration commands * `--prefix` - the prefix to run migrations on - * `--pool-size` - the pool size if the repository is started only for the task (defaults to 1) + * `--pool-size` - the pool size if the repository is started only for the task (defaults to 2) * `--log-sql` - log the raw sql migrations are running * `--strict-version-order` - abort when applying a migration with old timestamp * `--no-compile` - does not compile applications before migrating @@ -97,21 +97,28 @@ defmodule Mix.Tasks.Ecto.Migrate do do: Keyword.merge(opts, [log: false, log_sql: false]), else: opts - Enum.each repos, fn repo -> + # Start ecto_sql explicitly before as we don't need + # to restart those apps if migrated. + {:ok, _} = Application.ensure_all_started(:ecto_sql) + + for repo <- repos do ensure_repo(repo, args) path = ensure_migrations_path(repo) - {:ok, pid, apps} = ensure_started(repo, opts) - pool = repo.config[:pool] - migrated = + + fun = if function_exported?(pool, :unboxed_run, 2) do - pool.unboxed_run(repo, fn -> migrator.(repo, path, :up, opts) end) + &pool.unboxed_run(&1, fn -> migrator.(&1, path, :up, opts) end) else - migrator.(repo, path, :up, opts) + &migrator.(&1, path, :up, opts) end - pid && repo.stop() - restart_apps_if_migrated(apps, migrated) + case Ecto.Migrator.with_repo(repo, fun, [mode: :temporary] ++ opts) do + {:ok, migrated, apps} -> restart_apps_if_migrated(apps, migrated) + {:error, error} -> Mix.raise "Could not start repo #{inspect repo}, error: #{inspect error}" + end end + + :ok end end diff --git a/lib/mix/tasks/ecto.migrations.ex b/lib/mix/tasks/ecto.migrations.ex index dc384dab..794d73b4 100644 --- a/lib/mix/tasks/ecto.migrations.ex +++ b/lib/mix/tasks/ecto.migrations.ex @@ -36,27 +36,31 @@ defmodule Mix.Tasks.Ecto.Migrations do def run(args, migrations \\ &Ecto.Migrator.migrations/2, puts \\ &IO.puts/1) do repos = parse_repo(args) - result = - Enum.map(repos, fn repo -> - ensure_repo(repo, args) - path = ensure_migrations_path(repo) - {:ok, pid, _} = ensure_started(repo, all: true) - repo_status = migrations.(repo, path) - pid && repo.stop() - - """ - - Repo: #{inspect(repo)} - - Status Migration ID Migration Name - -------------------------------------------------- - """ <> - Enum.map_join(repo_status, "\n", fn {status, number, description} -> - " #{format(status, 10)}#{format(number, 16)}#{description}" - end) <> "\n" - end) - - puts.(Enum.join(result, "\n")) + for repo <- repos do + ensure_repo(repo, args) + path = ensure_migrations_path(repo) + + case Ecto.Migrator.with_repo(repo, &migrations.(&1, path), [mode: :temporary]) do + {:ok, repo_status, _} -> + puts.( + """ + + Repo: #{inspect(repo)} + + Status Migration ID Migration Name + -------------------------------------------------- + """ <> + Enum.map_join(repo_status, "\n", fn {status, number, description} -> + " #{format(status, 10)}#{format(number, 16)}#{description}" + end) <> "\n" + ) + + {:error, error} -> + Mix.raise "Could not start repo #{inspect repo}, error: #{inspect error}" + end + end + + :ok end defp format(content, pad) do diff --git a/lib/mix/tasks/ecto.rollback.ex b/lib/mix/tasks/ecto.rollback.ex index 65728938..70571c18 100644 --- a/lib/mix/tasks/ecto.rollback.ex +++ b/lib/mix/tasks/ecto.rollback.ex @@ -71,7 +71,7 @@ defmodule Mix.Tasks.Ecto.Rollback do * `--to` - revert all migrations down to and including version * `--quiet` - do not log migration commands * `--prefix` - the prefix to run migrations on - * `--pool-size` - the pool size if the repository is started only for the task (defaults to 1) + * `--pool-size` - the pool size if the repository is started only for the task (defaults to 2) * `--log-sql` - log the raw sql migrations are running * `--no-compile` - does not compile applications before rolling back * `--no-deps-check` - does not check depedendencies before rolling back @@ -93,21 +93,28 @@ defmodule Mix.Tasks.Ecto.Rollback do do: Keyword.merge(opts, [log: false, log_sql: false]), else: opts - Enum.each repos, fn repo -> + # Start ecto_sql explicitly before as we don't need + # to restart those apps if migrated. + {:ok, _} = Application.ensure_all_started(:ecto_sql) + + for repo <- repos do ensure_repo(repo, args) path = ensure_migrations_path(repo) - {:ok, pid, apps} = ensure_started(repo, opts) - pool = repo.config[:pool] - migrated = + + fun = if function_exported?(pool, :unboxed_run, 2) do - pool.unboxed_run(repo, fn -> migrator.(repo, path, :down, opts) end) + &pool.unboxed_run(&1, fn -> migrator.(&1, path, :down, opts) end) else - migrator.(repo, path, :down, opts) + &migrator.(&1, path, :down, opts) end - pid && repo.stop() - restart_apps_if_migrated(apps, migrated) + case Ecto.Migrator.with_repo(repo, fun, [mode: :temporary] ++ opts) do + {:ok, migrated, apps} -> restart_apps_if_migrated(apps, migrated) + {:error, error} -> Mix.raise "Could not start repo #{inspect repo}, error: #{inspect error}" + end end + + :ok end end diff --git a/test/ecto/migrator_test.exs b/test/ecto/migrator_test.exs index e6c79146..e5a70555 100644 --- a/test/ecto/migrator_test.exs +++ b/test/ecto/migrator_test.exs @@ -512,4 +512,43 @@ defmodule Ecto.MigratorTest do refute log =~ "before_commit" end end + + describe "with_repo" do + defmodule Repo do + def start_link(opts) do + assert opts[:pool_size] == 2 + Process.get(:start_link) + end + + def stop() do + Process.put(:stopped, true) + end + + def __adapter__ do + EctoSQL.TestAdapter + end + + def config do + [priv: Process.get(:priv), otp_app: :ecto_sql] + end + end + + test "starts and stops repo" do + Process.put(:start_link, {:ok, self()}) + assert with_repo(Repo, fn Repo -> :one end) == {:ok, :one, []} + assert Process.get(:stopped) + end + + test "runs with existing repo" do + Process.put(:start_link, {:error, {:already_started, self()}}) + assert with_repo(Repo, fn Repo -> :two end) == {:ok, :two, []} + refute Process.get(:stopped) + end + + test "handles errors" do + Process.put(:start_link, {:error, :oops}) + assert with_repo(Repo, fn Repo -> raise "never invoked" end) == {:error, :oops} + refute Process.get(:stopped) + end + end end diff --git a/test/mix/ecto_sql_test.exs b/test/mix/ecto_sql_test.exs index 2b55ec9e..368452d1 100644 --- a/test/mix/ecto_sql_test.exs +++ b/test/mix/ecto_sql_test.exs @@ -3,31 +3,11 @@ defmodule Mix.EctoSQLTest do import Mix.EctoSQL defmodule Repo do - def start_link(opts) do - assert opts[:pool_size] == 2 - Process.get(:start_link) - end - - def __adapter__ do - EctoSQL.TestAdapter - end - def config do [priv: Process.get(:priv), otp_app: :ecto_sql] end end - test "ensure_started" do - Process.put(:start_link, {:ok, self()}) - assert ensure_started(Repo, []) == {:ok, self(), []} - - Process.put(:start_link, {:error, {:already_started, self()}}) - assert ensure_started(Repo, []) == {:ok, nil, []} - - Process.put(:start_link, {:error, self()}) - assert_raise Mix.Error, fn -> ensure_started(Repo, []) end - end - test "source_priv_repo" do Process.put(:priv, nil) assert source_repo_priv(Repo) == Path.expand("priv/repo", File.cwd!)