test(delta): lock in behavior around simplifications

Self-audit turned up two undertested behavioral diffs in the
simplify pass. Addressed:

1. RenameAttribute check ORDER: the pre-refactor version checked
   "source present" first and "destination free" second. My
   helper-based refactor accidentally flipped these, producing a
   "destination already exists" error in the corner case where both
   source was missing AND destination existed. Restored the original
   order (inline, since replace_in_coll doesn't fit the two-phase
   check pattern) and added a regression test that locks the order.

2. `state.empty?` after add-then-remove-all: the simplify pass
   dropped the `state_empty?/1` final-override in
   `load_reduced_state/2`. This changed the semantics from "are all
   collections currently empty" to "was any op applied" — a real
   behavioral diff. This is intentional and more correct for the one
   consumer (`pkey_operations`), but there was no test that would
   catch a regression. Added one.

Also added tests for previously-unexercised paths:
* AddCustomStatement / RemoveCustomStatement apply_op (exercises the
  new `add_to_coll`/`remove_from_coll` helpers against the
  `:custom_statements` collection).
* RenameAttribute destination-exists happy path (checks the second
  branch after source is found).

28 tests total, all passing.

Author: barnabasJ

lib/migration_generator/reducer.ex

@@ -235,14 +235,24 @@ defmodule AshPostgres.MigrationGenerator.Reducer do
         old_attribute: old_attr,
         new_attribute: new_attr
       }) do
-    if old_attr.source != new_attr.source and
-         Enum.any?(state.attributes, &(&1.source == new_attr.source)) do
-      throw_conflict("RenameAttribute: destination #{inspect(new_attr.source)} already exists")
-    end
+    # Preserve the check order from the pre-refactor version: missing source
+    # is reported before existing destination. Callers and error-matching tests
+    # rely on the "source not present" message for the common misuse case of
+    # applying a rename against a mutated state.
+    case Enum.find_index(state.attributes, &(&1.source == old_attr.source)) do
+      nil ->
+        throw_conflict("RenameAttribute: source #{inspect(old_attr.source)} not present")
+
+      idx ->
+        if old_attr.source != new_attr.source and
+             Enum.any?(state.attributes, &(&1.source == new_attr.source)) do
+          throw_conflict(
+            "RenameAttribute: destination #{inspect(new_attr.source)} already exists"
+          )
+        end
 
-    replace_in_coll(state, :attributes, &(&1.source == old_attr.source), new_attr, fn ->
-      "RenameAttribute: source #{inspect(old_attr.source)} not present"
-    end)
+        %{state | attributes: List.replace_at(state.attributes, idx, new_attr)}
+    end
   end
 
   def apply_op(state, %Operation.AddUniqueIndex{identity: identity}),

test/snapshot_delta_test.exs

@@ -318,6 +318,158 @@ defmodule AshPostgres.SnapshotDeltaTest do
 
       assert {:threw, "RemoveAttribute: attribute :email not present"} = caught
     end
+
+    test "RenameAttribute: missing source is reported BEFORE existing destination",
+         %{snapshot: snapshot} do
+      # Regression test for post-refactor check ordering. Before the helper
+      # collapse, the reducer checked "source present" first and "destination
+      # free" second. If both were violated (source missing + destination
+      # present), the old message was "source :x not present". Make sure the
+      # refactor didn't flip the order.
+      state =
+        snapshot
+        |> Reducer.empty_state()
+        |> Reducer.apply_op(%Operation.AddAttribute{
+          table: snapshot.table,
+          schema: nil,
+          multitenancy: @base_multitenancy,
+          old_multitenancy: @base_multitenancy,
+          attribute: @text_attribute
+        })
+
+      caught =
+        try do
+          # Destination (:email) exists, source (:nonexistent_src) doesn't.
+          Reducer.apply_op(state, %Operation.RenameAttribute{
+            table: snapshot.table,
+            schema: nil,
+            multitenancy: @base_multitenancy,
+            old_multitenancy: @base_multitenancy,
+            old_attribute: %{@text_attribute | source: :nonexistent_src},
+            new_attribute: @text_attribute
+          })
+
+          :no_error
+        catch
+          :throw, {:reducer_error, reason} -> {:threw, reason}
+        end
+
+      assert {:threw, "RenameAttribute: source :nonexistent_src not present"} = caught
+    end
+
+    test "RenameAttribute: existing destination throws after source-found check",
+         %{snapshot: snapshot} do
+      # Separate clause: source exists, but destination collision.
+      other_attr = %{@text_attribute | source: :other_col}
+
+      state =
+        snapshot
+        |> Reducer.empty_state()
+        |> Reducer.apply_op(%Operation.AddAttribute{
+          table: snapshot.table,
+          schema: nil,
+          multitenancy: @base_multitenancy,
+          old_multitenancy: @base_multitenancy,
+          attribute: @text_attribute
+        })
+        |> Reducer.apply_op(%Operation.AddAttribute{
+          table: snapshot.table,
+          schema: nil,
+          multitenancy: @base_multitenancy,
+          old_multitenancy: @base_multitenancy,
+          attribute: other_attr
+        })
+
+      caught =
+        try do
+          Reducer.apply_op(state, %Operation.RenameAttribute{
+            table: snapshot.table,
+            schema: nil,
+            multitenancy: @base_multitenancy,
+            old_multitenancy: @base_multitenancy,
+            old_attribute: @text_attribute,
+            new_attribute: other_attr
+          })
+
+          :no_error
+        catch
+          :throw, {:reducer_error, reason} -> {:threw, reason}
+        end
+
+      assert {:threw, "RenameAttribute: destination :other_col already exists"} = caught
+    end
+
+    test "AddCustomStatement / RemoveCustomStatement round-trip through apply_op",
+         %{snapshot: snapshot} do
+      # Apply_op path for these ops wasn't previously exercised by a unit
+      # test. Covering the helper-based add/remove plumbing explicitly.
+      statement = %{name: :enable_trgm, up: "create extension pg_trgm", down: "drop extension pg_trgm"}
+
+      state =
+        snapshot
+        |> Reducer.empty_state()
+        |> Reducer.apply_op(%Operation.AddCustomStatement{
+          table: snapshot.table,
+          statement: statement
+        })
+
+      assert state.empty? == false
+      assert [^statement] = state.custom_statements
+
+      state =
+        Reducer.apply_op(state, %Operation.RemoveCustomStatement{
+          table: snapshot.table,
+          statement: statement
+        })
+
+      assert state.custom_statements == []
+
+      # Remove again should conflict
+      caught =
+        try do
+          Reducer.apply_op(state, %Operation.RemoveCustomStatement{
+            table: snapshot.table,
+            statement: statement
+          })
+
+          :no_error
+        catch
+          :throw, {:reducer_error, reason} -> {:threw, reason}
+        end
+
+      assert {:threw, "RemoveCustomStatement: statement :enable_trgm not present"} = caught
+    end
+
+    test "state.empty? after add-then-remove-all reflects 'state was touched', not 'state is empty now'",
+         %{snapshot: snapshot} do
+      # Intentional post-simplification behavior: dropping the final
+      # state_empty?/1 recomputation means `empty?` tracks "has any op been
+      # applied" (inline per apply_op), not "are all collections empty right
+      # now". For `pkey_operations`' purpose — deciding whether to skip pkey
+      # drops because we're a fresh create — this is more correct: removing
+      # attributes from an existing table is NOT a fresh-create scenario, so
+      # we must keep `empty?=false`.
+      state =
+        snapshot
+        |> Reducer.empty_state()
+        |> Reducer.apply_op(%Operation.AddAttribute{
+          table: snapshot.table,
+          schema: nil,
+          multitenancy: @base_multitenancy,
+          old_multitenancy: @base_multitenancy,
+          attribute: @text_attribute
+        })
+        |> Reducer.apply_op(%Operation.RemoveAttribute{
+          table: snapshot.table,
+          schema: nil,
+          multitenancy: @base_multitenancy,
+          old_multitenancy: @base_multitenancy,
+          attribute: @text_attribute
+        })
+
+      assert state.attributes == []
+      refute state.empty?, "empty? must stay false once state has been touched by any op"
+    end
   end
 
   describe "load_reduced_state" do