Fix/236 unique index before self fk

lib/migration_generator/migration_generator.ex

@@ -1518,6 +1518,76 @@ defmodule AshPostgres.MigrationGenerator do
     true
   end
 
+  # CreateTable must appear before AddUniqueIndex for the same table (table must exist first).
+  defp after?(
+         %Operation.CreateTable{table: table, schema: schema},
+         %Operation.AddUniqueIndex{table: table, schema: schema}
+       ),
+       do: false
+
+  # Unique index must be created before any alter that adds FKs referencing it.
+  defp after?(
+         %Operation.AddUniqueIndex{table: table, schema: schema},
+         %Operation.AlterAttribute{table: table, schema: schema}
+       ),
+       do: false
+
+  # Do not place AddUniqueIndex after CreateTable (must be after columns are added).
+  defp after?(
+         %Operation.AddUniqueIndex{table: table, schema: schema},
+         %Operation.CreateTable{table: table, schema: schema}
+       ),
+       do: false
+
+  # Place AddUniqueIndex after the last AddAttribute for the same table so it
+  # appears before AlterAttributes (issue #236).
+  defp after?(
+         %Operation.AddUniqueIndex{
+           insert_after_attribute_order: max_order,
+           table: table,
+           schema: schema
+         },
+         %Operation.AddAttribute{
+           table: table,
+           schema: schema,
+           attribute: %{order: order}
+         }
+       )
+       when not is_nil(max_order) and order == max_order,
+       do: true
+
+  defp after?(
+         %Operation.AddUniqueIndex{
+           insert_after_attribute_order: max_order,
+           table: table,
+           schema: schema
+         },
+         %Operation.AddAttribute{
+           table: table,
+           schema: schema,
+           attribute: %{order: order}
+         }
+       )
+       when not is_nil(max_order) and order != max_order,
+       do: false
+
+  defp after?(
+         %Operation.AddUniqueIndex{
+           identity: %{keys: keys},
+           table: table,
+           schema: schema
+         },
+         %Operation.AlterAttribute{
+           table: table,
+           schema: schema,
+           new_attribute: %{
+             references: %{table: table, destination_attribute: destination_attribute}
+           }
+         }
+       ) do
+    destination_attribute not in List.wrap(keys)
+  end
+
   defp after?(
          %Operation.AddUniqueIndex{
            table: table,
@@ -2263,10 +2333,22 @@ defmodule AshPostgres.MigrationGenerator do
         end)
       end
       |> Enum.map(fn identity ->
+        orders =
+          identity.keys
+          |> Enum.map(&Enum.find_index(snapshot.attributes, fn attr -> attr.source == &1 end))
+          |> Enum.reject(&is_nil/1)
+
+        insert_after_attribute_order =
+          case orders do
+            [] -> nil
+            _ -> Enum.max(orders)
+          end
+
         %Operation.AddUniqueIndex{
           identity: identity,
           schema: snapshot.schema,
           table: snapshot.table,
+          insert_after_attribute_order: insert_after_attribute_order,
           concurrently: opts.concurrent_indexes
         }
       end)
@@ -2301,13 +2383,22 @@ defmodule AshPostgres.MigrationGenerator do
         }
       end)
 
+    # Place unique indexes after create/add attributes but before alter attributes
+    # so FKs that reference identity columns see the unique index (issue #236).
+    {creates_and_adds, alter_and_rest} =
+      Enum.split_while(attribute_operations, fn
+        %Operation.AlterAttribute{} -> false
+        _ -> true
+      end)
+
     [
       pkey_operations,
       unique_indexes_to_remove,
-      attribute_operations,
+      creates_and_adds,
+      unique_indexes_to_add,
+      alter_and_rest,
       reference_indexes_to_add,
       reference_indexes_to_remove,
-      unique_indexes_to_add,
       unique_indexes_to_rename,
       constraints_to_remove,
       constraints_to_add,
@@ -2786,7 +2877,6 @@ defmodule AshPostgres.MigrationGenerator do
 
       is_nil(old_refs) or is_nil(new_refs) ->
         true
-
       true ->
         old_without_index = Map.delete(old_refs, :index?)
         new_without_index = Map.delete(new_refs, :index?)

lib/migration_generator/operation.ex

@@ -862,6 +862,7 @@ defmodule AshPostgres.MigrationGenerator.Operation do
       :schema,
       :multitenancy,
       :old_multitenancy,
+      :insert_after_attribute_order,
       no_phase: true,
       concurrently: false
     ]

test/migration_generator_test.exs

@@ -99,6 +99,13 @@ defmodule AshPostgres.MigrationGeneratorTest do
     end
   end
 
+  defp position_of_substring(string, substring) do
+    case :binary.match(string, substring) do
+      {pos, _len} -> pos
+      :nomatch -> nil
+    end
+  end
+
   defmacrop defresource(mod, table, do: body) do
     quote do
       Code.compiler_options(ignore_module_conflict: true)
@@ -1677,6 +1684,78 @@ defmodule AshPostgres.MigrationGeneratorTest do
                ~S[references(:posts, column: :id, name: "posts_post_id_fkey", type: :uuid, prefix: "public")]
     end
 
+    @tag :issue_236
+    test "unique index is created before dependent foreign key (issue #236)", %{
+      snapshot_path: snapshot_path,
+      migration_path: migration_path
+    } do
+      defresource Template, "templates" do
+        attributes do
+          uuid_primary_key(:id)
+        end
+      end
+
+      defresource Phase, "phases" do
+        attributes do
+          uuid_primary_key(:id)
+        end
+      end
+
+      defresource TemplatePhase, "template_phase" do
+        attributes do
+          uuid_primary_key(:id)
+          attribute(:name, :string, allow_nil?: false, public?: true)
+        end
+
+        identities do
+          identity(:id, [:id])
+        end
+
+        relationships do
+          belongs_to(:template, Template, primary_key?: true, allow_nil?: false, public?: true)
+          belongs_to(:phase, Phase, primary_key?: true, allow_nil?: false, public?: true)
+
+          belongs_to(:template_phase, __MODULE__) do
+            source_attribute(:follows)
+            destination_attribute(:id)
+            attribute_writable?(true)
+            allow_nil?(true)
+            public?(true)
+          end
+        end
+      end
+
+      defdomain([Template, Phase, TemplatePhase])
+
+      AshPostgres.MigrationGenerator.generate(Domain,
+        snapshot_path: snapshot_path,
+        migration_path: migration_path,
+        quiet: true,
+        format: false,
+        auto_name: true
+      )
+
+      assert [file] =
+               Path.wildcard("#{migration_path}/**/*_migrate_resources*.exs")
+               |> Enum.reject(&String.contains?(&1, "extensions"))
+
+      file_contents = File.read!(file)
+
+      unique_index_pos =
+        position_of_substring(
+          file_contents,
+          ~S{create unique_index(:template_phase, [:id], name: "template_phase_id_index")}
+        )
+
+      follows_fk_pos = position_of_substring(file_contents, "references(:template_phase")
+
+      assert unique_index_pos && follows_fk_pos,
+             "expected migration to contain both the unique index and the follows foreign key"
+
+      assert unique_index_pos < follows_fk_pos,
+             "expected unique index creation to appear before the follows foreign key modification"
+    end
+
     test "references are inferred automatically if the attribute has a different type", %{
       snapshot_path: snapshot_path,
       migration_path: migration_path