fix: drop constraints outside of phases

lib/migration_generator/migration_generator.ex

@@ -606,6 +606,21 @@ defmodule AshPostgres.MigrationGenerator do
     name in keys
   end
 
+  defp after?(%Operation.AlterAttribute{table: table}, %Operation.DropForeignKey{
+         table: table,
+         direction: :up
+       }),
+       do: true
+
+  defp after?(
+         %Operation.DropForeignKey{
+           table: table,
+           direction: :down
+         },
+         %Operation.AlterAttribute{table: table}
+       ),
+       do: true
+
   defp after?(%Operation.AddAttribute{table: table}, %Operation.CreateTable{table: table}) do
     true
   end
@@ -804,12 +819,37 @@ defmodule AshPostgres.MigrationGenerator do
       end)
 
     alter_attribute_events =
-      Enum.map(attributes_to_alter, fn {new_attribute, old_attribute} ->
-        %Operation.AlterAttribute{
-          new_attribute: new_attribute,
-          old_attribute: old_attribute,
-          table: snapshot.table
-        }
+      Enum.flat_map(attributes_to_alter, fn {new_attribute, old_attribute} ->
+        if has_reference?(old_snapshot.multitenancy, old_attribute) and
+             Map.get(old_attribute, :references) != Map.get(new_attribute, :references) do
+          [
+            %Operation.DropForeignKey{
+              attribute: old_attribute,
+              table: snapshot.table,
+              multitenancy: old_snapshot.multitenancy,
+              direction: :up
+            },
+            %Operation.AlterAttribute{
+              new_attribute: new_attribute,
+              old_attribute: old_attribute,
+              table: snapshot.table
+            },
+            %Operation.DropForeignKey{
+              attribute: new_attribute,
+              table: snapshot.table,
+              multitenancy: snapshot.multitenancy,
+              direction: :down
+            }
+          ]
+        else
+          [
+            %Operation.AlterAttribute{
+              new_attribute: new_attribute,
+              old_attribute: old_attribute,
+              table: snapshot.table
+            }
+          ]
+        end
       end)
 
     remove_attribute_events =
@@ -821,6 +861,13 @@ defmodule AshPostgres.MigrationGenerator do
       alter_attribute_events ++ remove_attribute_events ++ rename_attribute_events
   end
 
+  def has_reference?(multitenancy, attribute) do
+    not is_nil(Map.get(attribute, :references)) and
+      !(attribute.references.multitenancy &&
+          attribute.references.multitenancy.strategy == :context &&
+          (is_nil(multitenancy) || multitenancy.strategy == :attribute))
+  end
+
   def get_existing_snapshot(snapshot, opts) do
     repo_name = snapshot.repo |> Module.split() |> List.last() |> Macro.underscore()
 

lib/migration_generator/operation.ex

@@ -137,36 +137,20 @@ defmodule AshPostgres.MigrationGenerator.Operation do
 
     def up(%{
           multitenancy: multitenancy,
-          old_multitenancy: old_multitenancy,
-          table: table,
           old_attribute: old_attribute,
           new_attribute: attribute
         }) do
-      dropped_constraints =
-        if has_reference?(old_multitenancy, old_attribute) and
-             Map.get(old_attribute, :references) != Map.get(attribute, :references) do
-          AshPostgres.MigrationGenerator.Operation.RemoveAttribute.drop_foreign_key_constraint(
-            old_attribute,
-            old_multitenancy,
-            table
-          )
-        else
-          ""
-        end
-
       type_or_reference =
-        if has_reference?(multitenancy, attribute) and
+        if AshPostgres.MigrationGenerator.has_reference?(multitenancy, attribute) and
              Map.get(old_attribute, :references) != Map.get(attribute, :references) do
           reference(multitenancy, attribute)
         else
           inspect(attribute.type)
         end
 
-      dropped_constraints <>
-        "\n\n" <>
-        "modify #{inspect(attribute.name)}, #{type_or_reference}#{
-          alter_opts(attribute, old_attribute)
-        }"
+      "modify #{inspect(attribute.name)}, #{type_or_reference}#{
+        alter_opts(attribute, old_attribute)
+      }"
     end
 
     defp reference(%{strategy: :context}, %{
@@ -206,13 +190,6 @@ defmodule AshPostgres.MigrationGenerator.Operation do
       "references(#{inspect(table)}, type: #{inspect(type)}, column: #{inspect(destination_field)})"
     end
 
-    defp has_reference?(multitenancy, attribute) do
-      not is_nil(Map.get(attribute, :references)) and
-        !(attribute.references.multitenancy &&
-            attribute.references.multitenancy.strategy == :context &&
-            (is_nil(multitenancy) || multitenancy.strategy == :attribute))
-    end
-
     def down(op) do
       up(%{
         op
@@ -224,6 +201,34 @@ defmodule AshPostgres.MigrationGenerator.Operation do
     end
   end
 
+  defmodule DropForeignKey do
+    @moduledoc false
+    # We only run this migration in one direction, based on the input
+    # This is because the creation of a foreign key is handled by `references/3`
+    # We only need to drop it before altering an attribute with `references/3`
+    defstruct [:attribute, :table, :multitenancy, :direction, no_phase: true]
+
+    def up(%{attribute: attribute, table: table, multitenancy: multitenancy, direction: :up}) do
+      if multitenancy && multitenancy.strategy == :context do
+        "drop constraint(:#{table}, \"\#\{prefix\}_#{table}_#{attribute.name}_fkey\")"
+      else
+        "drop constraint(:#{table}, \"#{table}_#{attribute.name}_fkey\")"
+      end
+    end
+
+    def up(_), do: ""
+
+    def down(%{attribute: attribute, table: table, multitenancy: multitenancy, direction: :down}) do
+      if multitenancy && multitenancy.strategy == :context do
+        "drop constraint(:#{table}, \"\#\{prefix\}_#{table}_#{attribute.name}_fkey\")"
+      else
+        "drop constraint(:#{table}, \"#{table}_#{attribute.name}_fkey\")"
+      end
+    end
+
+    def down(_), do: ""
+  end
+
   defmodule RenameAttribute do
     @moduledoc false
     defstruct [:old_attribute, :new_attribute, :table, :multitenancy, :old_multitenancy]
@@ -241,20 +246,8 @@ defmodule AshPostgres.MigrationGenerator.Operation do
     @moduledoc false
     defstruct [:attribute, :table, :multitenancy, :old_multitenancy]
 
-    def up(%{multitenancy: multitenancy, attribute: attribute, table: table}) do
-      drop_foreign_key_constraint(attribute, multitenancy, table) <>
-        "\n\n" <>
-        "remove #{inspect(attribute.name)}"
-    end
-
-    def drop_foreign_key_constraint(%{references: nil}, _, _table), do: ""
-
-    def drop_foreign_key_constraint(attribute, multitenancy, table) do
-      if multitenancy do
-        "drop constraint(\"\#\{prefix\}_#{table}_#{attribute.name}_fkey\")"
-      else
-        "drop constraint(\"#{table}_#{attribute.name}_fkey\")"
-      end
+    def up(%{attribute: attribute}) do
+      "remove #{inspect(attribute.name)}"
     end
 
     def down(%{attribute: attribute, multitenancy: multitenancy}) do

lib/migration_generator/phase.ex

@@ -35,11 +35,11 @@ defmodule AshPostgres.MigrationGenerator.Phase do
         Enum.map_join(operations, "\n", fn operation -> operation.__struct__.up(operation) end)
 
       if multitenancy.strategy == :context do
-        "alter table(#{inspect(table)}, prefix: prefix) do\n" <>
+        "alter table(:#{table}, prefix: prefix) do\n" <>
           body <>
           "\nend"
       else
-        "alter table(#{inspect(table)}) do\n" <>
+        "alter table(:#{table}) do\n" <>
           body <>
           "\nend"
       end