feat: don't drop columns unless explicitly told to

lib/migration_generator/migration_generator.ex

@@ -15,7 +15,8 @@ defmodule AshPostgres.MigrationGenerator do
             tenant_migration_path: nil,
             quiet: false,
             format: true,
-            dry_run: false
+            dry_run: false,
+            drop_columns: false
 
   def generate(apis, opts \\ []) do
     apis = List.wrap(apis)
@@ -49,7 +50,7 @@ defmodule AshPostgres.MigrationGenerator do
       snapshots = Enum.map(deduped, &elem(&1, 0))
 
       deduped
-      |> fetch_operations()
+      |> fetch_operations(opts)
       |> Enum.uniq()
       |> case do
         [] ->
@@ -700,15 +701,15 @@ defmodule AshPostgres.MigrationGenerator do
 
   defp after?(_, _), do: false
 
-  defp fetch_operations(snapshots) do
+  defp fetch_operations(snapshots, opts) do
     Enum.flat_map(snapshots, fn {snapshot, existing_snapshot} ->
-      do_fetch_operations(snapshot, existing_snapshot)
+      do_fetch_operations(snapshot, existing_snapshot, opts)
     end)
   end
 
-  defp do_fetch_operations(snapshot, existing_snapshot, acc \\ [])
+  defp do_fetch_operations(snapshot, existing_snapshot, opts, acc \\ [])
 
-  defp do_fetch_operations(snapshot, nil, acc) do
+  defp do_fetch_operations(snapshot, nil, opts, acc) do
     empty_snapshot = %{
       attributes: [],
       identities: [],
@@ -721,7 +722,7 @@ defmodule AshPostgres.MigrationGenerator do
       }
     }
 
-    do_fetch_operations(snapshot, empty_snapshot, [
+    do_fetch_operations(snapshot, empty_snapshot, opts, [
       %Operation.CreateTable{
         table: snapshot.table,
         multitenancy: snapshot.multitenancy,
@@ -731,8 +732,8 @@ defmodule AshPostgres.MigrationGenerator do
     ])
   end
 
-  defp do_fetch_operations(snapshot, old_snapshot, acc) do
-    attribute_operations = attribute_operations(snapshot, old_snapshot)
+  defp do_fetch_operations(snapshot, old_snapshot, opts, acc) do
+    attribute_operations = attribute_operations(snapshot, old_snapshot, opts)
 
     unique_indexes_to_remove =
       old_snapshot.identities
@@ -767,7 +768,7 @@ defmodule AshPostgres.MigrationGenerator do
     |> Enum.map(&Map.put(&1, :old_multitenancy, old_snapshot.multitenancy))
   end
 
-  defp attribute_operations(snapshot, old_snapshot) do
+  defp attribute_operations(snapshot, old_snapshot, opts) do
     attributes_to_add =
       Enum.reject(snapshot.attributes, fn attribute ->
         Enum.find(old_snapshot.attributes, &(&1.name == attribute.name))
@@ -854,7 +855,11 @@ defmodule AshPostgres.MigrationGenerator do
 
     remove_attribute_events =
       Enum.map(attributes_to_remove, fn attribute ->
-        %Operation.RemoveAttribute{attribute: attribute, table: snapshot.table}
+        %Operation.RemoveAttribute{
+          attribute: attribute,
+          table: snapshot.table,
+          commented?: !opts.drop_columns
+        }
       end)
 
     add_attribute_events ++
@@ -891,6 +896,8 @@ defmodule AshPostgres.MigrationGenerator do
 
   defp resolve_renames(adding, []), do: {adding, [], []}
 
+  defp resolve_renames([], removing), do: {[], removing, []}
+
   defp resolve_renames([adding], [removing]) do
     if Mix.shell().yes?("Are you renaming :#{removing.name} to :#{adding.name}?") do
       {[], [], [{adding, removing}]}

lib/migration_generator/operation.ex

@@ -244,12 +244,41 @@ defmodule AshPostgres.MigrationGenerator.Operation do
 
   defmodule RemoveAttribute do
     @moduledoc false
-    defstruct [:attribute, :table, :multitenancy, :old_multitenancy]
+    defstruct [:attribute, :table, :multitenancy, :old_multitenancy, commented?: true]
+
+    def up(%{attribute: attribute, commented?: true}) do
+      """
+      # Attribute removal has been commented out to avoid data loss. See the migration generator documentation for more
+      # If you uncomment this, be sure to also uncomment the corresponding attribute *addition* in the `down` migration
+      # remove #{inspect(attribute.name)}
+      """
+    end
 
     def up(%{attribute: attribute}) do
       "remove #{inspect(attribute.name)}"
     end
 
+    def down(%{attribute: attribute, multitenancy: multitenancy, commented?: true}) do
+      prefix = """
+      # This is the `down` migration of the statement:
+      #
+      #     remove #{inspect(attribute.name)}
+      #
+      """
+
+      contents =
+        %AshPostgres.MigrationGenerator.Operation.AddAttribute{
+          attribute: attribute,
+          multitenancy: multitenancy
+        }
+        |> AshPostgres.MigrationGenerator.Operation.AddAttribute.up()
+        |> String.split("\n")
+        |> Enum.map(&"# #{&1}")
+        |> Enum.join("\n")
+
+      prefix <> "\n" <> contents
+    end
+
     def down(%{attribute: attribute, multitenancy: multitenancy}) do
       AshPostgres.MigrationGenerator.Operation.AddAttribute.up(
         %AshPostgres.MigrationGenerator.Operation.AddAttribute{

lib/mix/tasks/ash_postgres.generate_migrations.ex

@@ -9,6 +9,7 @@ defmodule Mix.Tasks.AshPostgres.GenerateMigrations do
   * `migration_path` - a custom path to store the migrations, defaults to "priv".
     Migrations are stored in a folder for each repo, so `priv/repo_name/migrations`
   * `tenant_migration_path` - Same as `migration_path`, except for any tenant specific migrations
+  * `drop_columns` - whether or not to drop columns as attributes are removed. See below for more
 
   Flags:
 
@@ -16,6 +17,19 @@ defmodule Mix.Tasks.AshPostgres.GenerateMigrations do
   * `no_format` - files that are created will not be formatted with the code formatter
   * `dry_run` - no files are created, instead the new migration is printed
 
+
+  #### Dropping columns
+
+  Generally speaking, it is bad practice to drop columns when you deploy a change that
+  would remove an attribute. The main reasons for this are backwards compatibility and rolling restarts.
+  If you deploy an attribute removal, and run migrations. Regardless of your deployment sstrategy, you
+  won't be able to roll back, because the data has been deleted. In a rolling restart situation, some of
+  the machines/pods/whatever may still be running after the column has been deleted, causing errors. With
+  this in mind, its best not to delete those columns until later, after the data has been confirmed unnecessary.
+  To that end, the migration generator leaves the column dropping code commented. You can pass `--drop_columns`
+  to tell it to uncomment those statements. Additionally, you can just uncomment that code on a case by case
+  basis.
+
   #### Conflicts/Multiple Resources
 
   The migration generator can support multiple schemas using the same table.
@@ -54,7 +68,8 @@ defmodule Mix.Tasks.AshPostgres.GenerateMigrations do
           migration_path: :string,
           quiet: :boolean,
           no_format: :boolean,
-          dry_run: :boolean
+          dry_run: :boolean,
+          drop_columns: :boolean
         ]
       )
 

mix.lock

@@ -1,5 +1,5 @@
 %{
-  "ash": {:hex, :ash, "1.22.1", "f3426edbd3b39db16e021f9ef3a58c32c5b5a19b1ffde4e7f3d11cfcd267d2ed", [:mix], [{:ecto, "~> 3.4", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8.0", [hex: :ets, repo: "hexpm", optional: false]}, {:nimble_options, "~> 0.3.0", [hex: :nimble_options, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.1.5", [hex: :picosat_elixir, repo: "hexpm", optional: false]}], "hexpm", "f2ea5c166191121f55c7272e4ad15675d329fc4b376c3c039f03762efd87dc60"},
+  "ash": {:hex, :ash, "1.22.1", "8bdd9a61f99a99d438845d8c5c5edb6203179299a04e7856f5fae76df498309b", [:mix], [{:ecto, "~> 3.4", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8.0", [hex: :ets, repo: "hexpm", optional: false]}, {:nimble_options, "~> 0.3.0", [hex: :nimble_options, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.1.5", [hex: :picosat_elixir, repo: "hexpm", optional: false]}], "hexpm", "1a185fafe7e5aaa500c7944fc35f4acd6c596eda9b7b865362e1d96f174fbede"},
   "bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"},
   "certifi": {:hex, :certifi, "2.5.2", "b7cfeae9d2ed395695dd8201c57a2d019c0c43ecaf8b8bcb9320b40d6662f340", [:rebar3], [{:parse_trans, "~>3.3", [hex: :parse_trans, repo: "hexpm", optional: false]}], "hexpm", "3b3b5f36493004ac3455966991eaf6e768ce9884693d9968055aeeeb1e575040"},
   "connection": {:hex, :connection, "1.0.4", "a1cae72211f0eef17705aaededacac3eb30e6625b04a6117c1b2db6ace7d5976", [:mix], [], "hexpm", "4a0850c9be22a43af9920a71ab17c051f5f7d45c209e40269a1938832510e4d9"},