Fix/custom statement dependencies

CHANGELOG.md

@@ -20,7 +20,7 @@ See [Conventional Commits](https://www.conventionalcommits.org) for commit guide
 
 * add match_tenant? option for attribute multitenancy FKs (#813) by TravelCurry02
 
-* add after_tables to custom_statements so raw SQL can depend on another table (#799) by Jechol Lee
+* add after_resource to custom_statements so raw SQL can depend on another resource (#799) by Jechol Lee
 
 ### Improvements:
 

documentation/dsls/DSL-AshPostgres.DataLayer.md

@@ -136,7 +136,8 @@ A section for configuring custom statements to be added to migrations.
 By default, a statement has no declared dependency on other tables, so `down` statements run before any other
 operation and `up` statements run after all other operations for that statement's table. If your statement's `up`
 depends on structure from another table (e.g. a foreign key referencing a unique index defined via `identities`),
-declare it with `after_tables` so the migration generator orders it correctly relative to that table's operations.
+declare it with `after_tables` so the migration generator orders it correctly relative to that table's structure.
+Custom statements on the same table run in declaration order, such as creating a function before a trigger that invokes it.
 
 Additionally, when changing a custom statement, we must make some assumptions, i.e that we should migrate
 the old structure down using the previously configured `down` and recreate it.
@@ -164,6 +165,12 @@ custom_statements do
     up "ALTER TABLE children ADD CONSTRAINT children_parent_fk FOREIGN KEY (region_id, parent_id) REFERENCES parents (region_id, id);"
     down "ALTER TABLE children DROP CONSTRAINT children_parent_fk;"
   end
+
+  statement :create_audit_trigger do
+    after_tables ["audit_entries"]
+    up "CREATE TRIGGER ..."
+    down "DROP TRIGGER ..."
+  end
 end
 
 ```
@@ -206,7 +213,7 @@ end
 | [`down`](#postgres-custom_statements-statement-down){: #postgres-custom_statements-statement-down .spark-required} | `String.t` |  | How to tear down the structure of the statement |
 | [`code?`](#postgres-custom_statements-statement-code?){: #postgres-custom_statements-statement-code? } | `boolean` | `false` | By default, we place the strings inside of ecto migration's `execute/1` function and assume they are sql. Use this option if you want to provide custom elixir code to be placed directly in the migrations |
 | [`global?`](#postgres-custom_statements-statement-global?){: #postgres-custom_statements-statement-global? } | `boolean` | `false` | By default, a multi-tenant resource's custom statements will be written into the tenant migration folder. Set this to true for statements that create global, shared structures so they are written into the public migration folder even when defined on a tenant resource. |
-| [`after_tables`](#postgres-custom_statements-statement-after_tables){: #postgres-custom_statements-statement-after_tables } | `list(String.t)` | `[]` | Table names that this statement's `up` depends on being fully finalized (including their columns and indexes) before it runs. Use this when a raw SQL statement references structure (e.g. a foreign key referencing a unique index) on another table so the migration generator can order it correctly. |
+| [`after_tables`](#postgres-custom_statements-statement-after_tables){: #postgres-custom_statements-statement-after_tables } | `list(String.t)` | `[]` | Table names whose structural operations must be complete before this statement's `up` runs. This does not wait for custom statements declared on those tables. Use this for raw SQL that references another table's columns or indexes. |
 
 
 

lib/data_layer.ex

@@ -101,7 +101,8 @@ defmodule AshPostgres.DataLayer do
     By default, a statement has no declared dependency on other tables, so `down` statements run before any other
     operation and `up` statements run after all other operations for that statement's table. If your statement's `up`
     depends on structure from another table (e.g. a foreign key referencing a unique index defined via `identities`),
-    declare it with `after_tables` so the migration generator orders it correctly relative to that table's operations.
+    declare it with `after_tables` so the migration generator orders it correctly relative to that table's structure.
+    Custom statements on the same table run in declaration order, such as creating a function before a trigger that invokes it.
 
     Additionally, when changing a custom statement, we must make some assumptions, i.e that we should migrate
     the old structure down using the previously configured `down` and recreate it.
@@ -124,6 +125,12 @@ defmodule AshPostgres.DataLayer do
           up "ALTER TABLE children ADD CONSTRAINT children_parent_fk FOREIGN KEY (region_id, parent_id) REFERENCES parents (region_id, id);"
           down "ALTER TABLE children DROP CONSTRAINT children_parent_fk;"
         end
+
+        statement :create_audit_trigger do
+          after_tables ["audit_entries"]
+          up "CREATE TRIGGER ..."
+          down "DROP TRIGGER ..."
+        end
       end
       """
     ],

lib/migration_generator/migration_generator.ex

@@ -1807,10 +1807,13 @@ defmodule AshPostgres.MigrationGenerator do
   # dependencies at the same time) are broken by that original index, so
   # unconstrained operations keep their original relative order (matching
   # resource declaration order).
-  defp toposort_operations(operations) do
+  @doc false
+  def toposort_operations(operations) do
     count = length(operations)
     indexed = Enum.with_index(operations)
 
+    declaration_deps_by_index = custom_statement_declaration_dependencies(indexed)
+
     provides_index =
       Enum.reduce(indexed, %{}, fn {op, index}, acc ->
         op
@@ -1846,6 +1849,7 @@ defmodule AshPostgres.MigrationGenerator do
     dependencies =
       Map.new(indexed, fn {op, index} ->
         fact_deps = Map.fetch!(fact_deps_by_index, index)
+        declaration_deps = Map.get(declaration_deps_by_index, index, [])
 
         tier_deps =
           if OperationDeps.early_tier?(op) || MapSet.member?(required_by_early_tier, index) do
@@ -1855,7 +1859,7 @@ defmodule AshPostgres.MigrationGenerator do
           end
 
         deps =
-          (fact_deps ++ tier_deps)
+          (fact_deps ++ declaration_deps ++ tier_deps)
           |> Enum.reject(&(&1 == index))
           |> Enum.uniq()
 
@@ -1918,6 +1922,48 @@ defmodule AshPostgres.MigrationGenerator do
     toposort_operation_indices(queue, adjacency, in_degrees, [index | acc])
   end
 
+  # Preserve custom statement declaration order for additions and reverse it
+  # for removals because their SQL may contain dependencies we cannot inspect.
+  defp custom_statement_declaration_dependencies(indexed) do
+    {dependencies, _last_add, _last_remove} =
+      Enum.reduce(indexed, {%{}, %{}, %{}}, fn
+        {%Operation.AddCustomStatement{table: table, schema: schema}, index},
+        {dependencies, last_add, last_remove} ->
+          key = {schema_key(schema), table}
+
+          dependencies =
+            case Map.fetch(last_add, key) do
+              {:ok, previous_index} ->
+                Map.update(dependencies, index, [previous_index], &[previous_index | &1])
+
+              :error ->
+                dependencies
+            end
+
+          {dependencies, Map.put(last_add, key, index), last_remove}
+
+        {%Operation.RemoveCustomStatement{table: table, schema: schema}, index},
+        {dependencies, last_add, last_remove} ->
+          key = {schema_key(schema), table}
+
+          dependencies =
+            case Map.fetch(last_remove, key) do
+              {:ok, previous_index} ->
+                Map.update(dependencies, previous_index, [index], &[index | &1])
+
+              :error ->
+                dependencies
+            end
+
+          {dependencies, last_add, Map.put(last_remove, key, index)}
+
+        {_operation, _index}, acc ->
+          acc
+      end)
+
+    dependencies
+  end
+
   defp fetch_operations(snapshots, opts) do
     # Reference diffs need to know when a prefix change is caused by moving
     # the referenced table instead of by changing the foreign key itself.

lib/migration_generator/operation_deps.ex

@@ -8,7 +8,7 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
 
   Each operation may *provide* facts (things that become true once it runs)
   and *require* facts (things that must already be true before it can run).
-  `AshPostgres.MigrationGenerator.MigrationGenerator.toposort_operations/1`
+  `AshPostgres.MigrationGenerator.toposort_operations/1`
   turns these into a dependency graph and topologically sorts it.
 
   There is deliberately no symmetric "late tier" counterpart to
@@ -21,8 +21,8 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
 
   Requiring a fact waits on *every* operation that provides it, not just one
   — see `toposort_operations/1`'s `provides_index`. That's what makes
-  `:table_structure_ready`/`:table_finalized` work as catch-alls: many
-  operation types provide them, so an op that requires one (e.g.
+  `:table_structure_ready` works as a catch-all: many operation types provide
+  it, so an op that requires it (e.g.
   `AddCustomStatement`'s own-table or `after_tables` requirement)
   transparently waits for all of that table's work, without needing to
   enumerate every attribute/index/constraint by hand.
@@ -69,8 +69,8 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
   (`CreateTable`/`RenameTable`/`MoveTableSchema`) from `:table_columns_settled`
   (`AddAttribute`/`RenameAttribute`/`AlterAttribute`/`RemoveAttribute`), so
   requiring both together is *not* redundant — neither subsumes the other.
-  Both converge at `:table_structure_ready` and `:table_finalized`, which
-  every structural operation provides:
+  Both converge at `:table_structure_ready`, which every structural operation
+  provides:
 
   - `:table_ready` — the table exists (`CreateTable`/`RenameTable`/
     `MoveTableSchema`).
@@ -92,17 +92,6 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
     `DropForeignKey` operations even though `DropTable` is early-tier (the
     `required_by_early_tier` exemption in `toposort_operations/1` is what lets
     that specific dependency win over the blanket barrier).
-  - `:table_finalized` — this table is *truly* done, including any
-    `custom_statements` declared on it: provided by everything that provides
-    `:table_structure_ready`, plus each `AddCustomStatement` on the table (a
-    table with no custom statements is finalized as soon as its structure is
-    ready). Kept separate from `:table_structure_ready` because
-    `AddCustomStatement`'s own implicit "wait for my own table" requirement
-    must use the narrower fact — were it to require `:table_finalized`, two
-    custom statements on the same table would each provide and require the
-    same fact, a guaranteed cycle. Only the explicit, opt-in `after_tables`
-    cross-table reference requires `:table_finalized`, so it also waits for
-    the target table's own custom statements.
 
   Column-scoped (`key = {schema, table, column}`):
 
@@ -325,16 +314,13 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
       %Operation.RemovePrimaryKeyDown{table: table, schema: schema} ->
         structure_ready_facts(table, schema)
 
-      %Operation.AddCustomStatement{table: own_table, schema: schema} ->
-        [{:table_finalized, key(own_table, schema)}]
-
       _ ->
         []
     end
   end
 
   defp structure_ready_facts(table, schema) do
-    [{:table_structure_ready, key(table, schema)}, {:table_finalized, key(table, schema)}]
+    [{:table_structure_ready, key(table, schema)}]
   end
 
   @doc "Facts that must already be provided (by some other operation) before `op` can run."
@@ -536,15 +522,8 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do
       %Operation.AddCustomStatement{table: own_table, schema: schema, statement: statement} ->
         after_tables = statement |> Map.get(:after_tables) |> List.wrap()
 
-        # Own table: the narrower `:table_structure_ready` (not
-        # `:table_finalized`) — using the broader fact here would make two
-        # custom statements on the same table each require the other's
-        # `:table_finalized` (each provides it too), a guaranteed cycle.
-        # Declared `after_tables` targets: the broader `:table_finalized`,
-        # so this statement also waits for *that* table's own custom
-        # statements, not just its structure.
         [{:table_structure_ready, key(own_table, schema)}] ++
-          Enum.map(after_tables, &{:table_finalized, key(&1, schema)})
+          Enum.map(after_tables, &{:table_structure_ready, key(&1, schema)})
 
       _ ->
         []

lib/statement.ex

@@ -56,7 +56,7 @@ defmodule AshPostgres.Statement do
       type: {:list, :string},
       default: [],
       doc: """
-      Table names that this statement's `up` depends on being fully finalized (including their columns and indexes) before it runs. Use this when a raw SQL statement references structure (e.g. a foreign key referencing a unique index) on another table so the migration generator can order it correctly.
+      Table names whose structural operations must be complete before this statement's `up` runs. This does not wait for custom statements declared on those tables. Use this for raw SQL that references another table's columns or indexes.
       """
     ]
   ]