diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index db1433f9..ccdbf063 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -74,12 +74,14 @@ defmodule AshPostgres.MigrationGenerator do extension_migration_files = create_extension_migrations(repos, opts) - tenant_migration_files = + {tenant_migration_files, tenant_review_flags} = create_migrations(tenant_snapshots, opts, true, snapshots, unmanaged_tenant_snapshots) - migration_files = + {migration_files, global_review_flags} = create_migrations(snapshots, opts, false, [], unmanaged_snapshots) + review_flags = merge_review_flag_maps(tenant_review_flags, global_review_flags) + case extension_migration_files ++ tenant_migration_files ++ migration_files do [] -> if !opts.check || opts.dry_run do @@ -91,6 +93,10 @@ defmodule AshPostgres.MigrationGenerator do :ok files -> + if !(opts.check or opts.quiet or opts.no_shell?) do + emit_review_warnings(review_flags) + end + cond do opts.check -> raise Ash.Error.Framework.PendingCodegen, @@ -460,7 +466,7 @@ defmodule AshPostgres.MigrationGenerator do ) do snapshots |> Enum.group_by(& &1.repo) - |> Enum.flat_map(fn {repo, snapshots} -> + |> Enum.reduce({[], initial_review_flags()}, fn {repo, snapshots}, {files_acc, flags_acc} -> deduped = deduplicate_snapshots(snapshots, opts, non_tenant_snapshots) managed_table_keys = @@ -539,12 +545,13 @@ defmodule AshPostgres.MigrationGenerator do |> Enum.concat(drop_operations) |> Enum.uniq() - operations - |> case do + case operations do [] -> - [] + {files_acc, flags_acc} operations -> + repo_flags = classify_operations(operations) + dev_migrations = get_dev_migrations(opts, tenant?, repo) if !opts.dev and dev_migrations != [] do @@ -582,16 +589,89 @@ defmodule AshPostgres.MigrationGenerator do operations_to_migrations(public_operations, repo, opts, false) end - migrations - |> Enum.concat(create_new_snapshot(snapshots, repo_name(repo), opts, tenant?)) - |> then(fn files -> - remove_orphan_snapshots(orphan_snapshots, opts) - files - end) + files = + migrations + |> Enum.concat(create_new_snapshot(snapshots, repo_name(repo), opts, tenant?)) + |> then(fn files -> + remove_orphan_snapshots(orphan_snapshots, opts) + files + end) + + {files_acc ++ files, merge_review_flag_maps(flags_acc, repo_flags)} end end) end + defp emit_review_warnings(review_flags) do + shell = Mix.shell() + + review_flags + |> review_warning_messages() + |> Enum.each(&shell.info/1) + end + + defp review_warning_messages(review_flags) do + [ + review_baseline_message(), + review_commented_safeguards_message(review_flags), + review_destructive_message(review_flags) + ] + |> Enum.reject(&is_nil/1) + end + + defp review_baseline_message do + "Please always manually review the generated migrations for correctness." + end + + defp review_commented_safeguards_message(%{commented_safeguards?: true}) do + "Warning: there are migration steps commented out and in need of manual review." + end + + defp review_commented_safeguards_message(_), do: nil + + defp review_destructive_message(%{destructive_uncommented?: true}) do + "Warning: this migration includes destructive operations (e.g. drops or column removals). Review carefully before migrating." + end + + defp review_destructive_message(_), do: nil + + defp classify_operations(operations) do + Enum.reduce(operations, initial_review_flags(), &merge_review_flags/2) + end + + defp initial_review_flags do + %{commented_safeguards?: false, destructive_uncommented?: false} + end + + defp merge_review_flags(op, acc) do + acc + |> Map.update!(:commented_safeguards?, &(&1 or commented_safeguard?(op))) + |> Map.update!(:destructive_uncommented?, &(&1 or destructive_uncommented?(op))) + end + + defp merge_review_flag_maps(a, b) do + %{ + commented_safeguards?: a.commented_safeguards? or b.commented_safeguards?, + destructive_uncommented?: a.destructive_uncommented? or b.destructive_uncommented? + } + end + + defp commented_safeguard?(%{commented?: true}), do: true + defp commented_safeguard?(_), do: false + + defp destructive_uncommented?(%Operation.DropTable{}), do: true + defp destructive_uncommented?(%Operation.RemoveAttribute{commented?: false}), do: true + defp destructive_uncommented?(%Operation.RemovePrimaryKey{}), do: true + defp destructive_uncommented?(%Operation.RemovePrimaryKeyDown{commented?: false}), do: true + defp destructive_uncommented?(%Operation.DropForeignKey{}), do: true + defp destructive_uncommented?(%Operation.RemoveCustomStatement{}), do: true + defp destructive_uncommented?(%Operation.RemoveCustomIndex{}), do: true + defp destructive_uncommented?(%Operation.RemoveReferenceIndex{}), do: true + defp destructive_uncommented?(%Operation.RemoveUniqueIndex{}), do: true + defp destructive_uncommented?(%Operation.RemoveCheckConstraint{}), do: true + defp destructive_uncommented?(%Operation.AddPrimaryKeyDown{remove_old?: true}), do: true + defp destructive_uncommented?(_), do: false + defp operations_to_migrations([], _repo, _opts, _tenant?), do: [] defp operations_to_migrations(operations, repo, opts, tenant?) do diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index 92f83925..7b0cf378 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -7,6 +7,11 @@ defmodule AshPostgres.MigrationGeneratorTest do @moduletag :migration @moduletag :tmp_dir + @baseline "Please always manually review the generated migrations for correctness." + @commented_warning "migration steps commented out and in need of manual review" + @destructive_warning "destructive operations" + @warning_prefix "Warning:" + import ExUnit.CaptureLog setup %{tmp_dir: tmp_dir} do @@ -106,6 +111,27 @@ defmodule AshPostgres.MigrationGeneratorTest do end end + defp flush_mix_shell do + receive do + {:mix_shell, _, _} -> flush_mix_shell() + after + 0 -> :ok + end + end + + defp mix_shell_info_messages(acc \\ []) do + receive do + {:mix_shell, :info, message} -> + mix_shell_info_messages([to_string(message) | acc]) + after + 0 -> Enum.reverse(acc) + end + end + + defp shell_output do + mix_shell_info_messages() |> Enum.join("\n") + end + defmacrop defresource(mod, table, do: body) do quote do Code.compiler_options(ignore_module_conflict: true) @@ -5495,6 +5521,377 @@ defmodule AshPostgres.MigrationGeneratorTest do end end + describe "review warnings" do + test "emits baseline reminder for additive migrations", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnAddPost, "review_warn_add_posts" do + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnAddPost]) + + flush_mix_shell() + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: false, + format: false, + auto_name: true, + name: "review_warn_add" + ) + + output = shell_output() + + assert output =~ @baseline + refute output =~ @warning_prefix + end + + test "emits commented safeguard escalation with dont_drop_columns", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnColumnPost, "review_warn_column_posts" do + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnColumnPost]) + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: true, + format: false, + auto_name: true, + name: "review_warn_setup_column" + ) + + defresource ReviewWarnColumnPost, "review_warn_column_posts" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnColumnPost]) + + send(self(), {:mix_shell_input, :yes?, false}) + flush_mix_shell() + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: false, + format: false, + auto_name: true, + name: "review_warn_comment_column", + dont_drop_columns: true + ) + + output = shell_output() + + latest = + Path.wildcard("#{migration_path}/**/*.exs") + |> Enum.reject(&String.contains?(&1, "extensions")) + |> Enum.sort() + |> List.last() + |> File.read!() + + assert latest =~ "Attribute removal has been commented out" + refute latest =~ ~r/^\s*remove "title"/m + + assert output =~ @baseline + assert output =~ @commented_warning + refute output =~ @destructive_warning + end + + test "emits destructive escalation when dropping a table", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnKeepPost, "review_warn_keep_posts" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defresource ReviewWarnDropPost, "review_warn_drop_posts" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnKeepPost, ReviewWarnDropPost]) + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: true, + format: false, + auto_name: true, + name: "review_warn_setup_drop" + ) + + defdomain([ReviewWarnKeepPost]) + + send(self(), {:mix_shell_input, :yes?, true}) + flush_mix_shell() + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: false, + format: false, + auto_name: true, + name: "review_warn_drop" + ) + + output = shell_output() + + latest = + Path.wildcard("#{migration_path}/**/*.exs") + |> Enum.reject(&String.contains?(&1, "extensions")) + |> Enum.sort() + |> List.last() + |> File.read!() + + assert latest =~ "drop table(:review_warn_drop_posts)" + assert output =~ @baseline + assert output =~ @destructive_warning + refute output =~ @commented_warning + end + + test "emits destructive escalation when removing a column", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnRemoveColumn, "review_warn_remove_column" do + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnRemoveColumn]) + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: true, + format: false, + auto_name: true, + name: "review_warn_setup_column" + ) + + defresource ReviewWarnRemoveColumn, "review_warn_remove_column" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnRemoveColumn]) + + send(self(), {:mix_shell_input, :yes?, false}) + flush_mix_shell() + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: false, + format: false, + auto_name: true, + name: "review_warn_remove_column" + ) + + output = shell_output() + + latest = + Path.wildcard("#{migration_path}/**/*.exs") + |> Enum.reject(&String.contains?(&1, "extensions")) + |> Enum.sort() + |> List.last() + |> File.read!() + + assert latest =~ ~r/^\s*remove :title/m + assert output =~ @baseline + assert output =~ @destructive_warning + refute output =~ @commented_warning + end + + test "does not emit review warnings when quiet is true", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnQuietPost, "review_warn_quiet_posts" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnQuietPost]) + + flush_mix_shell() + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: true, + format: false, + auto_name: true, + name: "review_warn_quiet" + ) + + output = shell_output() + + refute output =~ @baseline + refute output =~ @warning_prefix + end + + test "does not emit review warnings when check is true", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnCheckPost, "review_warn_check_posts" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnCheckPost]) + + flush_mix_shell() + + assert_raise Ash.Error.Framework.PendingCodegen, fn -> + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + check: true, + quiet: false, + format: false, + auto_name: true + ) + end + + refute File.exists?( + Path.wildcard("#{snapshot_path}/test_repo/review_warn_check_posts/*.json") + ) + + refute File.exists?(Path.wildcard("#{migration_path}/**/*_migrate_resources*.exs")) + + assert shell_output() == "" + end + + test "does not emit review warnings when no_shell? is true", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnNoShellPost, "review_warn_noshell_posts" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnNoShellPost]) + + flush_mix_shell() + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + quiet: false, + no_shell?: true, + format: false, + auto_name: true, + name: "review_warn_noshell" + ) + + output = shell_output() + + refute output =~ @baseline + refute output =~ @warning_prefix + end + + test "emits review warnings on dry_run", %{ + snapshot_path: snapshot_path, + migration_path: migration_path + } do + defresource ReviewWarnDryRunPost, "review_warn_dry_run_posts" do + attributes do + uuid_primary_key(:id) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + end + + defdomain([ReviewWarnDryRunPost]) + + flush_mix_shell() + + AshPostgres.MigrationGenerator.generate(Domain, + snapshot_path: snapshot_path, + migration_path: migration_path, + dry_run: true, + quiet: false, + format: false, + auto_name: true + ) + + output = shell_output() + + assert output =~ @baseline + refute output =~ @warning_prefix + end + end + describe "renaming tables when resources change" do test "generates a rename table migration when a resource table is renamed", %{ snapshot_path: snapshot_path, diff --git a/test/support/resources/post.ex b/test/support/resources/post.ex index 706c2e3d..4c628ba4 100644 --- a/test/support/resources/post.ex +++ b/test/support/resources/post.ex @@ -1124,7 +1124,6 @@ defmodule AshPostgres.Test.Post do calculate(:c_times_p, :integer, expr(count_of_comments * count_of_linked_posts)) - calculate( :score_category, :integer,