Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add prefix option to check repo status plug #181

Merged
merged 2 commits into from
Oct 25, 2024
Merged

Add prefix option to check repo status plug #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1dafcc2
Add prefix option to CheckRepoStatus
stevehodgkiss Oct 20, 2024
6c36d02
Handle the case where error is raised without migration_opts
stevehodgkiss Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions lib/phoenix_ecto/check_repo_status.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ defmodule Phoenix.Ecto.CheckRepoStatus do
* `:otp_app` - name of the application which the repos are fetched from
* `:migration_paths` - a function that accepts a repo and returns a migration directory, or a list of migration directories, that is used to check for pending migrations
* `:migration_lock` - the locking strategy used by the Ecto Adapter when checking for pending migrations. Set to `false` to disable migration locks.
* `:prefix` - the prefix used to check for pending migrations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of prefix, what if we allow migration_paths to be {path, opts} pairs? This way you can specify opts per path?

Copy link
Contributor Author

@stevehodgkiss stevehodgkiss Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be more flexible & wouldn't require multiple plug configurations.

defmodule MyMod do
  def migrations_paths(repo) do
    default_migrations_path = Migrator.migrations_path(repo)

    tenant_migrations_path =
      Path.join(default_migrations_path, "../tenant_migrations") |> Path.expand()

    [default_migrations_path, {tenant_migrations_path, prefix: "tenant_acme"}]
  end
end

# endpoint

    plug Phoenix.Ecto.CheckRepoStatus,
      otp_app: :myapp,
      migration_paths: &MyMod.migrations_paths/1

Could also fetch the tenant schema names from the db rather than use a fixed value. Will update tomorrow 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR to allow tuples in migration_paths. There's a slight change in behaviour in 5648d73, where given fn -> ["a", "b"] end it'll run the migrator once for a and again for b.

I'm not sure how well the option name migration_paths fits with this new functionality. Alternative names could be migrate_runs or a migrate: fn -> ["a", {"b", prefix: "tenant"}]. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, I am sorry. I think the change in behaviour is not acceptable because it does change how the migrator sees and runs migrations. I think your original version is the way to go indeed. Can you please revert? And thank you for exploring this path. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted 6c36d02 which means the migrator will run once for the ["a", "b"] case (although the code to maintain this behaviour is a bit complicated). Oh, did you mean revert back to the original version with the prefix option (i.e. to include only the first 2 commits in this PR)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback to the original version. It would require you to list the plug twice but I think that's alright?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted to the original version. Yeah, listing the plug twice is fine for this use case (a known dev tenant prefix).

"""

@behaviour Plug

alias Plug.Conn

@migration_opts [:migration_lock]
@migration_opts [:migration_lock, :prefix]
@compile {:no_warn_undefined, Ecto.Migrator}

def init(opts) do
Expand Down Expand Up @@ -60,16 +61,23 @@ defmodule Phoenix.Ecto.CheckRepoStatus do
end)

true = is_function(migrations_fun, 3)
migration_opts = Keyword.take(opts, @migration_opts)

try do
repo
|> migrations_fun.(dirs, Keyword.take(opts, @migration_opts))
|> migrations_fun.(dirs, migration_opts)
|> Enum.any?(fn {status, _version, _migration} -> status == :down end)
rescue
_ -> false
else
true -> raise Phoenix.Ecto.PendingMigrationError, repo: repo, directories: dirs
false -> true
true ->
raise Phoenix.Ecto.PendingMigrationError,
repo: repo,
directories: dirs,
migration_opts: migration_opts

false ->
true
end
end

Expand Down
4 changes: 2 additions & 2 deletions lib/phoenix_ecto/exceptions.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ defmodule Phoenix.Ecto.StorageNotCreatedError do
end

defmodule Phoenix.Ecto.PendingMigrationError do
@enforce_keys [:repo, :directories]
defexception [:repo, :directories]
@enforce_keys [:repo, :directories, :migration_opts]
defexception [:repo, :directories, :migration_opts]

def message(%__MODULE__{repo: repo}) do
"there are pending migrations for repo: #{inspect(repo)}. " <>
Expand Down
8 changes: 5 additions & 3 deletions lib/phoenix_ecto/plug.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,17 @@ unless Phoenix.Ecto.PendingMigrationError in excluded_exceptions do
defimpl Plug.Exception, for: Phoenix.Ecto.PendingMigrationError do
def status(_error), do: 503

def actions(%{repo: repo, directories: directories}),
def actions(%{repo: repo, directories: directories, migration_opts: migration_opts}),
do: [
%{
label: "Run migrations for repo",
handler: {__MODULE__, :migrate, [repo, directories]}
handler: {__MODULE__, :migrate, [repo, directories, migration_opts]}
}
]

def migrate(repo, directories), do: Ecto.Migrator.run(repo, directories, :up, all: true)
def migrate(repo, directories, migration_opts) do
Ecto.Migrator.run(repo, directories, :up, Keyword.merge(migration_opts || [], all: true))
end
end
end

Expand Down
49 changes: 41 additions & 8 deletions test/phoenix_ecto/check_repo_status_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,47 @@ defmodule Phoenix.Ecto.CheckRepoStatusTest do

conn = conn(:get, "/")

assert_raise(Phoenix.Ecto.PendingMigrationError, fn ->
CheckRepoStatus.call(
conn,
otp_app: :check_repo_ready,
mock_migrations_fn: mock_migrations_fn,
migration_lock: false
)
end)
exception =
assert_raise(Phoenix.Ecto.PendingMigrationError, fn ->
CheckRepoStatus.call(
conn,
otp_app: :check_repo_ready,
mock_migrations_fn: mock_migrations_fn,
migration_lock: false
)
end)

assert exception.migration_opts == [migration_lock: false]
after
Application.delete_env(:check_repo_ready, :ecto_repos)
Process.unregister(StorageUpRepo)
end

test "supports Ecto's prefix option" do
Process.register(self(), StorageUpRepo)
Application.put_env(:check_repo_ready, :ecto_repos, [StorageUpRepo])

mock_migrations_fn = fn _repo, _directories, opts ->
if opts[:prefix] == "tenant_1" do
[{:down, 1, "migration"}]
else
[]
end
end

conn = conn(:get, "/")

exception =
assert_raise(Phoenix.Ecto.PendingMigrationError, fn ->
CheckRepoStatus.call(
conn,
otp_app: :check_repo_ready,
mock_migrations_fn: mock_migrations_fn,
prefix: "tenant_1"
)
end)

assert exception.migration_opts == [prefix: "tenant_1"]
after
Application.delete_env(:check_repo_ready, :ecto_repos)
Process.unregister(StorageUpRepo)
Expand Down
Loading