diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index e2cd9edd..bbd9b573 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/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?) diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index e206ebfe..53a1a87f 100644 --- a/lib/migration_generator/operation.ex +++ b/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 ] diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index 29b66731..167e783b 100644 --- a/test/migration_generator_test.exs +++ b/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