diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ea3458..f54396f5 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/documentation/dsls/DSL-AshPostgres.DataLayer.md b/documentation/dsls/DSL-AshPostgres.DataLayer.md index 3cb82251..e2650606 100644 --- a/documentation/dsls/DSL-AshPostgres.DataLayer.md +++ b/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. | diff --git a/lib/data_layer.ex b/lib/data_layer.ex index b5af83ae..99b35206 100644 --- a/lib/data_layer.ex +++ b/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 """ ], diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index db27355a..fad77774 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/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. diff --git a/lib/migration_generator/operation_deps.ex b/lib/migration_generator/operation_deps.ex index dbfe1982..6078174b 100644 --- a/lib/migration_generator/operation_deps.ex +++ b/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)}) _ -> [] diff --git a/lib/statement.ex b/lib/statement.ex index 009dc87c..726977e3 100644 --- a/lib/statement.ex +++ b/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. """ ] ] diff --git a/test/migration_generator/operation_deps_test.exs b/test/migration_generator/operation_deps_test.exs index f122d993..93aef945 100644 --- a/test/migration_generator/operation_deps_test.exs +++ b/test/migration_generator/operation_deps_test.exs @@ -9,6 +9,7 @@ defmodule AshPostgres.MigrationGenerator.OperationDepsTest do """ use ExUnit.Case, async: true + alias AshPostgres.MigrationGenerator alias AshPostgres.MigrationGenerator.Operation alias AshPostgres.MigrationGenerator.OperationDeps @@ -561,7 +562,7 @@ defmodule AshPostgres.MigrationGenerator.OperationDepsTest do statement = %Operation.AddCustomStatement{ table: "widget", schema: nil, - statement: %{name: :some_statement, up: "", down: "", code?: false, after_tables: []} + statement: %{name: :some_statement, up: "", down: "", code?: false} } [fact] = @@ -570,14 +571,20 @@ defmodule AshPostgres.MigrationGenerator.OperationDepsTest do assert fact in OperationDeps.requires(statement) end - test "AddCustomStatement with after_tables is satisfied by a CreateTable for the declared table (via table_finalized)" do + test "after_tables waits for table structure without waiting for its custom statements" do create = %Operation.CreateTable{table: "parents", schema: nil} - statement = %Operation.AddCustomStatement{ - table: "widget", + parent_statement = %Operation.AddCustomStatement{ + table: "parents", + schema: nil, + statement: %{name: :parent_view, up: "", down: "", code?: false} + } + + child_statement = %Operation.AddCustomStatement{ + table: "children", schema: nil, statement: %{ - name: :widget_parent_composite_fk, + name: :child_view, up: "", down: "", code?: false, @@ -585,77 +592,114 @@ defmodule AshPostgres.MigrationGenerator.OperationDepsTest do } } - [fact] = OperationDeps.provides(create) |> Enum.filter(&match?({:table_finalized, _}, &1)) + [structure_fact] = + OperationDeps.provides(create) + |> Enum.filter(&match?({:table_structure_ready, _}, &1)) - assert fact in OperationDeps.requires(statement) + assert structure_fact in OperationDeps.requires(child_statement) + refute structure_fact in OperationDeps.provides(parent_statement) end - test "AddCustomStatement with after_tables is satisfied by another custom statement declared on the target table" do - # This is the whole point of the two-tier fact split: a shared, - # foundational custom statement (e.g. one that creates a structure - # another table's FK needs) can live on the table it actually concerns, - # and other resources' `after_tables` will wait for it too — not just - # for that table's plain structural (DDL) operations. - parent_statement = %Operation.AddCustomStatement{ - table: "parents", + test "custom statements preserve declaration order around after_tables dependencies" do + table_a = %Operation.CreateTable{table: "widgets", schema: nil} + table_b = %Operation.CreateTable{table: "audit_entries", schema: nil} + + function_statement = %Operation.AddCustomStatement{ + table: "widgets", schema: nil, statement: %{ - name: :parents_composite_unique_index, - up: "", - down: "", + name: :create_function, + up: "CREATE FUNCTION audit_widget() RETURNS trigger ...", + down: "DROP FUNCTION audit_widget()", code?: false, - after_tables: [] + after_tables: ["audit_entries"] } } - child_statement = %Operation.AddCustomStatement{ - table: "widget", + trigger_statement = %Operation.AddCustomStatement{ + table: "widgets", schema: nil, statement: %{ - name: :widget_parent_composite_fk, + name: :create_trigger, + up: "CREATE TRIGGER audit_widget EXECUTE FUNCTION audit_widget()", + down: "DROP TRIGGER audit_widget ON widgets", + code?: false, + after_tables: ["audit_entries"] + } + } + + operations = + MigrationGenerator.toposort_operations([ + table_a, + function_statement, + trigger_statement, + table_b + ]) + + statement_names = + for %Operation.AddCustomStatement{statement: statement} <- operations, do: statement.name + + assert statement_names == [:create_function, :create_trigger] + assert Enum.reverse(statement_names) == [:create_trigger, :create_function] + end + + test "a later custom statement cannot overtake an earlier statement with after_tables" do + table_a = %Operation.CreateTable{table: "widgets", schema: nil} + table_b = %Operation.CreateTable{table: "audit_entries", schema: nil} + + function_statement = %Operation.AddCustomStatement{ + table: "widgets", + schema: nil, + statement: %{ + name: :create_function, up: "", down: "", code?: false, - after_tables: ["parents"] + after_tables: ["audit_entries"] } } - [fact] = - OperationDeps.provides(parent_statement) - |> Enum.filter(&match?({:table_finalized, _}, &1)) + trigger_statement = %Operation.AddCustomStatement{ + table: "widgets", + schema: nil, + statement: %{name: :create_trigger, up: "", down: "", code?: false} + } + + operations = + MigrationGenerator.toposort_operations([ + table_a, + function_statement, + trigger_statement, + table_b + ]) + + statement_names = + for %Operation.AddCustomStatement{statement: statement} <- operations, do: statement.name - assert fact in OperationDeps.requires(child_statement) + assert statement_names == [:create_function, :create_trigger] end - test "two custom statements declared on the same table do not require each other (no sibling cycle)" do - statement_a = %Operation.AddCustomStatement{ - table: "widget", + test "removed custom statements use reverse declaration order" do + remove_function = %Operation.RemoveCustomStatement{ + table: "widgets", schema: nil, - statement: %{name: :a, up: "", down: "", code?: false, after_tables: []} + statement: %{name: :create_function, up: "", down: "", code?: false} } - statement_b = %Operation.AddCustomStatement{ - table: "widget", + remove_trigger = %Operation.RemoveCustomStatement{ + table: "widgets", schema: nil, - statement: %{name: :b, up: "", down: "", code?: false, after_tables: []} + statement: %{name: :create_trigger, up: "", down: "", code?: false} } - # Each provides :table_finalized for their shared table (so *other* - # tables' after_tables can depend on either of them), but neither's own - # implicit requirement is written in terms of that same broad fact — - # only the narrower :table_structure_ready, which neither custom - # statement provides. If this ever regresses, `AddCustomStatement`s on - # a shared table would deadlock (a real cycle) via each other's - # `:table_finalized`. - refute Enum.any?( - OperationDeps.provides(statement_a), - &match?({:table_structure_ready, _}, &1) - ) + operations = + MigrationGenerator.toposort_operations([remove_function, remove_trigger]) - refute Enum.any?( - OperationDeps.requires(statement_b), - &match?({:table_finalized, _}, &1) - ) + statement_names = + for %Operation.RemoveCustomStatement{statement: statement} <- operations, + do: statement.name + + assert statement_names == [:create_trigger, :create_function] end end