fix: drop an unnamed custom index before recreating it under the same…

lib/migration_generator/operation_deps.ex

@@ -136,9 +136,10 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
   Index-scoped (`key = {schema, table, index}`, same shape as a column-scoped
   key but the third element is an index name, not a column):
 
-  - `:custom_index_removed` — a specific *named* custom index has been
-    dropped, so a same-named index can be recreated (Postgres index names
-    are unique per schema).
+  - `:custom_index_removed` — a specific custom index has been dropped, so
+    a same-named index can be recreated (Postgres index names are unique per
+    schema). Keyed by the explicit `name:`, or for an unnamed index by the
+    name derived from its table and fields (see `custom_index_name/2`).
 
   Column-set-scoped (`key = {schema, table, sorted_columns}`, the third
   element a sorted list of column atoms):
@@ -256,16 +257,7 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
                key(table, schema, AshPostgres.CustomIndex.column_name(field))}
             end)
 
-        # An index without an explicit `name:` gets one auto-derived by
-        # Postgres/Ecto from its columns, so two *different* unnamed indexes
-        # would otherwise collide on the same `nil` fact key and get
-        # spuriously coupled — only track this for explicitly named indexes,
-        # where a real name collision is possible.
-        if index.name do
-          [{:custom_index_removed, key(table, schema, index.name)} | base]
-        else
-          base
-        end
+        [{:custom_index_removed, key(table, schema, custom_index_name(table, index))} | base]
 
       %Operation.AddReferenceIndex{table: table, schema: schema} ->
         structure_ready_facts(table, schema)
@@ -492,14 +484,10 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
 
         base = [{:table_ready, key(table, schema)} | column_requirements]
 
-        # a `RemoveCustomIndex` sharing this index's *explicit* name must run
-        # first — Postgres can't create an index under a name that's still in
-        # use. See the matching guard in `provides/1`.
-        if index.name do
-          [{:custom_index_removed, key(table, schema, index.name)} | base]
-        else
-          base
-        end
+        # a `RemoveCustomIndex` sharing this index's name must run first —
+        # Postgres can't create an index under a name that's still in use
+        # (e.g. an unnamed index recreated with a different `where`).
+        [{:custom_index_removed, key(table, schema, custom_index_name(table, index))} | base]
 
       %Operation.AddReferenceIndex{table: table, schema: schema, source: source} ->
         # a `RemoveReferenceIndex` on the same source column must run first —
@@ -667,4 +655,12 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
 
   defp normalize_schema(nil), do: "public"
   defp normalize_schema(schema), do: schema
+
+  # An index without an explicit `name:` is named from its table and fields
+  # (the same name `AshPostgres.CustomIndex.name/2` gives it elsewhere), so
+  # two unnamed indexes on the same fields really do collide. Stringified so
+  # an explicit name and the equal derived one (an atom) share a fact key.
+  defp custom_index_name(table, index) do
+    to_string(index.name || AshPostgres.CustomIndex.name(table, index))
+  end
 end

test/migration_generator/operation_deps_test.exs

@@ -405,32 +405,75 @@ defmodule AshPostgres.MigrationGenerator.OperationDepsTest do
       assert fact in OperationDeps.requires(add)
     end
 
-    test "two differently-shaped unnamed indexes are NOT spuriously linked by a shared nil name" do
-      # Regression: an auto-named (nil `name:`) index previously collapsed to
-      # the same `{schema, table, nil}` fact key as any other unnamed index on
-      # the same table, incorrectly coupling unrelated index changes.
+    test "RemoveCustomIndex and AddCustomIndex on the same fields without a name are linked" do
+      # Both get the name derived from table and fields (posts_title_index), so
+      # recreating the index (e.g. with a new `where`) must drop the old first.
       remove = %Operation.RemoveCustomIndex{
         table: "posts",
         schema: nil,
-        index: %{name: nil, fields: []},
+        index: %{name: nil, fields: [:title]},
         base_filter: nil,
         multitenancy: %{attribute: nil, strategy: nil, global: nil}
       }
 
       add = %Operation.AddCustomIndex{
         table: "posts",
         schema: nil,
-        index: %{name: nil},
+        index: %{name: nil, fields: [:title]},
         base_filter: nil,
         multitenancy: %{attribute: nil, strategy: nil, global: nil}
       }
 
-      refute Enum.any?(
-               OperationDeps.provides(remove),
-               &match?({:custom_index_removed, _}, &1)
-             )
+      [fact] =
+        OperationDeps.provides(remove) |> Enum.filter(&match?({:custom_index_removed, _}, &1))
+
+      assert fact in OperationDeps.requires(add)
+    end
+
+    test "an unnamed index and an explicit name equal to its derived name are linked" do
+      remove = %Operation.RemoveCustomIndex{
+        table: "posts",
+        schema: nil,
+        index: %{name: nil, fields: [:title]},
+        base_filter: nil,
+        multitenancy: %{attribute: nil, strategy: nil, global: nil}
+      }
+
+      add = %Operation.AddCustomIndex{
+        table: "posts",
+        schema: nil,
+        index: %{name: "posts_title_index", fields: [:title]},
+        base_filter: nil,
+        multitenancy: %{attribute: nil, strategy: nil, global: nil}
+      }
+
+      [fact] =
+        OperationDeps.provides(remove) |> Enum.filter(&match?({:custom_index_removed, _}, &1))
+
+      assert fact in OperationDeps.requires(add)
+    end
+
+    test "two unnamed indexes on different fields are NOT linked" do
+      remove = %Operation.RemoveCustomIndex{
+        table: "posts",
+        schema: nil,
+        index: %{name: nil, fields: [:title]},
+        base_filter: nil,
+        multitenancy: %{attribute: nil, strategy: nil, global: nil}
+      }
 
-      refute Enum.any?(OperationDeps.requires(add), &match?({:custom_index_removed, _}, &1))
+      add = %Operation.AddCustomIndex{
+        table: "posts",
+        schema: nil,
+        index: %{name: nil, fields: [:body]},
+        base_filter: nil,
+        multitenancy: %{attribute: nil, strategy: nil, global: nil}
+      }
+
+      [fact] =
+        OperationDeps.provides(remove) |> Enum.filter(&match?({:custom_index_removed, _}, &1))
+
+      refute fact in OperationDeps.requires(add)
     end
 
     test "RemoveReferenceIndex and AddReferenceIndex on the same source column are linked" do

test/migration_generator_test.exs

@@ -1151,6 +1151,60 @@ defmodule AshPostgres.MigrationGeneratorTest do
              end) <
                Enum.find_index(down_side_parts, fn x -> x == "create index(:posts, [:title])" end)
     end
+
+    test "it drops an unnamed index before recreating it under the same name", %{
+      snapshot_path: snapshot_path,
+      migration_path: migration_path
+    } do
+      defposts do
+        postgres do
+          custom_indexes do
+            index([:title], where: "title IS NOT NULL")
+          end
+        end
+
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:title, :string, public?: true)
+        end
+      end
+
+      defdomain([Post])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      assert [_file1, file2] =
+               Enum.sort(Path.wildcard("#{migration_path}/**/*_migrate_resources*.exs"))
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+
+      contents = File.read!(file2)
+
+      [up_side, down_side] = String.split(contents, "def down", parts: 2)
+
+      up_side_parts =
+        String.split(up_side, "\n", trim: true)
+        |> Enum.map(&String.trim/1)
+
+      # both indexes are named posts_title_index, so the old one must go first
+      assert Enum.find_index(up_side_parts, &(&1 == "drop_if_exists index(:posts, [:title])")) <
+               Enum.find_index(
+                 up_side_parts,
+                 &(&1 == ~S{create index(:posts, [:title], where: "title IS NOT NULL")})
+               )
+
+      down_side_parts =
+        String.split(down_side, "\n", trim: true)
+        |> Enum.map(&String.trim/1)
+
+      assert Enum.find_index(down_side_parts, &(&1 == "drop_if_exists index(:posts, [:title])")) <
+               Enum.find_index(down_side_parts, &(&1 == "create index(:posts, [:title])"))
+    end
   end
 
   describe "check constraint and column removed together" do