From 480efd4cf95a6c28ee5202d88a62c7cd8bbe827f Mon Sep 17 00:00:00 2001 From: Jechol Lee Date: Sun, 19 Jul 2026 18:28:37 +0900 Subject: [PATCH 1/2] feat: add after_tables to custom_statements so raw SQL can depend on another table --- lib/data_layer.ex | 14 +- lib/migration_generator/operation_deps.ex | 122 ++++++++++-------- lib/statement.ex | 10 +- .../operation_deps_test.exs | 92 ++++++++++++- 4 files changed, 180 insertions(+), 58 deletions(-) diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 99f5b9d3..f548bc60 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -98,9 +98,10 @@ defmodule AshPostgres.DataLayer do describe: """ A section for configuring custom statements to be added to migrations. - Changing custom statements may require manual intervention, because Ash can't determine what order they should run - in (i.e if they depend on table structure that you've added, or vice versa). As such, any `down` statements we run - for custom statements happen first, and any `up` statements happen last. + 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. 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. @@ -116,6 +117,13 @@ defmodule AshPostgres.DataLayer do up "CREATE INDEX pgweb_idx ON pgweb USING GIN (to_tsvector('english', title || ' ' || body));" down "DROP INDEX pgweb_idx;" end + + statement :children_parent_composite_fk do + # ensures this runs after `parents`'s columns and unique indexes are finalized + after_tables ["parents"] + 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 end """ ], diff --git a/lib/migration_generator/operation_deps.ex b/lib/migration_generator/operation_deps.ex index 6d1310c1..071512a0 100644 --- a/lib/migration_generator/operation_deps.ex +++ b/lib/migration_generator/operation_deps.ex @@ -21,11 +21,11 @@ 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` work as a catch-all: many operation types provide - it, so an op that requires it (currently only `AddCustomStatement`'s - own-table requirement) transparently waits for all of that table's - structural work, without needing to enumerate every attribute/index/ - constraint by hand. + `:table_structure_ready`/`:table_finalized` work as catch-alls: many + operation types provide them, so an op that requires one (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. Requiring a fact that nothing in the current batch provides is vacuously satisfied — it adds no dependency edge at all, rather than blocking or @@ -64,13 +64,13 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do Table-scoped (`key = {schema, table}`): - These first three track "how done is this table", but not as one single + These first four track "how done is this table", but not as one single chain — `:table_ready` is provided by a disjoint set of operations (`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`, which every structural operation - provides: + Both converge at `:table_structure_ready` and `:table_finalized`, which + every structural operation provides: - `:table_ready` — the table exists (`CreateTable`/`RenameTable`/ `MoveTableSchema`). @@ -80,10 +80,20 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do a check constraint's `check:`) might reference a column that's about to be added, altered, renamed, or removed, and can't be parsed to know which columns it actually touches. - - `:table_structure_ready` — provided by every structural operation on this - table (including `:table_ready`'s and `:table_columns_settled`'s - providers); a consumer requires it once to mean "wait for all of this - table's structural work". + - `:table_structure_ready` — this table's structural (DDL) work is done: + provided by every structural operation on this table (including + `:table_ready`'s and `:table_columns_settled`'s providers). + - `: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}`): @@ -154,20 +164,19 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do def provides(op) do case op do %Operation.CreateTable{table: table, schema: schema} -> - [{:table_ready, key(table, schema)}, {:table_structure_ready, key(table, schema)}] + [{:table_ready, key(table, schema)}] ++ structure_ready_facts(table, schema) %Operation.RenameTable{table: table, schema: schema} -> - [{:table_ready, key(table, schema)}, {:table_structure_ready, key(table, schema)}] + [{:table_ready, key(table, schema)}] ++ structure_ready_facts(table, schema) %Operation.MoveTableSchema{table: table, new_schema: schema} -> - [{:table_ready, key(table, schema)}, {:table_structure_ready, key(table, schema)}] + [{:table_ready, key(table, schema)}] ++ structure_ready_facts(table, schema) %Operation.AddAttribute{table: table, schema: schema, attribute: attribute} -> [ {:column_ready, key(table, schema, attribute.source)}, - {:table_columns_settled, key(table, schema)}, - {:table_structure_ready, key(table, schema)} - ] + {:table_columns_settled, key(table, schema)} + ] ++ structure_ready_facts(table, schema) %Operation.RenameAttribute{ table: table, @@ -176,21 +185,18 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do } -> [ {:column_ready, key(table, schema, new_attribute.source)}, - {:table_columns_settled, key(table, schema)}, - {:table_structure_ready, key(table, schema)} - ] + {:table_columns_settled, key(table, schema)} + ] ++ structure_ready_facts(table, schema) %Operation.AlterAttribute{table: table, schema: schema} -> [ - {:table_columns_settled, key(table, schema)}, - {:table_structure_ready, key(table, schema)} - ] + {:table_columns_settled, key(table, schema)} + ] ++ structure_ready_facts(table, schema) %Operation.RemoveAttribute{table: table, schema: schema} -> [ - {:table_columns_settled, key(table, schema)}, - {:table_structure_ready, key(table, schema)} - ] + {:table_columns_settled, key(table, schema)} + ] ++ structure_ready_facts(table, schema) %Operation.AddUniqueIndex{table: table, schema: schema, identity: identity} = op -> columns = @@ -200,15 +206,13 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do Map.get(op, :multitenancy) ) - [ - {:table_structure_ready, key(table, schema)}, - {:unique_index_created, key(table, schema, columns)} - ] + structure_ready_facts(table, schema) ++ + [{:unique_index_created, key(table, schema, columns)}] %Operation.RemoveUniqueIndex{table: table, schema: schema, identity: identity} -> keys = List.wrap(identity.keys) - [{:table_structure_ready, key(table, schema)}] ++ + structure_ready_facts(table, schema) ++ Enum.map(keys, &{:column_unique_index_removed, key(table, schema, &1)}) %Operation.AddCustomIndex{table: table, schema: schema, index: index} = op -> @@ -226,11 +230,11 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do [] end - [{:table_structure_ready, key(table, schema)}] ++ unique_facts + structure_ready_facts(table, schema) ++ unique_facts %Operation.RemoveCustomIndex{table: table, schema: schema, index: index} -> base = - [{:table_structure_ready, key(table, schema)}] ++ + structure_ready_facts(table, schema) ++ Enum.map(index.fields, fn field -> {:column_custom_index_removed, key(table, schema, AshPostgres.CustomIndex.column_name(field))} @@ -248,16 +252,14 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do end %Operation.AddReferenceIndex{table: table, schema: schema} -> - [{:table_structure_ready, key(table, schema)}] + structure_ready_facts(table, schema) %Operation.RemoveReferenceIndex{table: table, schema: schema, source: source} -> - [ - {:table_structure_ready, key(table, schema)}, - {:reference_index_removed, key(table, schema, source)} - ] + structure_ready_facts(table, schema) ++ + [{:reference_index_removed, key(table, schema, source)}] %Operation.AddCheckConstraint{table: table, schema: schema} -> - [{:table_structure_ready, key(table, schema)}] + structure_ready_facts(table, schema) %Operation.RemoveCheckConstraint{ table: table, @@ -267,7 +269,7 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do } -> cols = check_constraint_columns(constraint, multitenancy) - [{:table_structure_ready, key(table, schema)}] ++ + structure_ready_facts(table, schema) ++ Enum.map(cols, &{:column_check_constraint_removed, key(table, schema, &1)}) %Operation.DropForeignKey{ @@ -281,33 +283,39 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do dest_schema = Map.get(reference, :schema) [ - {:column_fk_dropped, key(dest_table, dest_schema, dest_column)}, - {:table_structure_ready, key(table, schema)} - ] + {:column_fk_dropped, key(dest_table, dest_schema, dest_column)} + ] ++ structure_ready_facts(table, schema) %Operation.DropForeignKey{table: table, schema: schema} -> - [{:table_structure_ready, key(table, schema)}] + structure_ready_facts(table, schema) %Operation.AlterDeferrability{table: table, schema: schema} -> - [{:table_structure_ready, key(table, schema)}] + structure_ready_facts(table, schema) %Operation.AddPrimaryKey{table: table, schema: schema} -> - [{:table_structure_ready, key(table, schema)}] + structure_ready_facts(table, schema) %Operation.AddPrimaryKeyDown{table: table, schema: schema} -> - [{:table_structure_ready, key(table, schema)}] + structure_ready_facts(table, schema) %Operation.RemovePrimaryKey{table: table, schema: schema} -> - [{:table_structure_ready, key(table, schema)}] + structure_ready_facts(table, schema) %Operation.RemovePrimaryKeyDown{table: table, schema: schema} -> - [{:table_structure_ready, key(table, 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)}] + end + @doc "Facts that must already be provided (by some other operation) before `op` can run." def requires(op) do case op do @@ -461,8 +469,18 @@ defmodule AshPostgres.MigrationGenerator.OperationDeps do %Operation.RemovePrimaryKey{table: table, schema: schema, keys: keys} -> Enum.map(List.wrap(keys), &{:column_fk_dropped, key(table, schema, &1)}) - %Operation.AddCustomStatement{table: own_table, schema: schema} -> - [{:table_structure_ready, key(own_table, schema)}] + %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)}) _ -> [] diff --git a/lib/statement.ex b/lib/statement.ex index 5e41b872..009dc87c 100644 --- a/lib/statement.ex +++ b/lib/statement.ex @@ -10,7 +10,8 @@ defmodule AshPostgres.Statement do :up, :down, :code?, - :global? + :global?, + :after_tables ] defstruct @fields ++ [:__spark_metadata__] @@ -50,6 +51,13 @@ defmodule AshPostgres.Statement do type: :string, doc: "How to tear down the structure of the statement", required: true + ], + after_tables: [ + 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. + """ ] ] diff --git a/test/migration_generator/operation_deps_test.exs b/test/migration_generator/operation_deps_test.exs index 3c60294b..0f24bb43 100644 --- a/test/migration_generator/operation_deps_test.exs +++ b/test/migration_generator/operation_deps_test.exs @@ -484,13 +484,13 @@ defmodule AshPostgres.MigrationGenerator.OperationDepsTest do end describe "custom_statements" do - test "AddCustomStatement's own-table requirement is satisfied by a CreateTable for that same table" do + test "AddCustomStatement's own-table requirement is satisfied by a CreateTable for that same table (via table_structure_ready)" do create = %Operation.CreateTable{table: "widget", schema: nil} statement = %Operation.AddCustomStatement{ table: "widget", schema: nil, - statement: %{name: :some_statement, up: "", down: "", code?: false} + statement: %{name: :some_statement, up: "", down: "", code?: false, after_tables: []} } [fact] = @@ -498,6 +498,94 @@ 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 + create = %Operation.CreateTable{table: "parents", schema: nil} + + statement = %Operation.AddCustomStatement{ + table: "widget", + schema: nil, + statement: %{ + name: :widget_parent_composite_fk, + up: "", + down: "", + code?: false, + after_tables: ["parents"] + } + } + + [fact] = OperationDeps.provides(create) |> Enum.filter(&match?({:table_finalized, _}, &1)) + + assert fact in OperationDeps.requires(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", + schema: nil, + statement: %{ + name: :parents_composite_unique_index, + up: "", + down: "", + code?: false, + after_tables: [] + } + } + + child_statement = %Operation.AddCustomStatement{ + table: "widget", + schema: nil, + statement: %{ + name: :widget_parent_composite_fk, + up: "", + down: "", + code?: false, + after_tables: ["parents"] + } + } + + [fact] = + OperationDeps.provides(parent_statement) + |> Enum.filter(&match?({:table_finalized, _}, &1)) + + assert fact in OperationDeps.requires(child_statement) + 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", + schema: nil, + statement: %{name: :a, up: "", down: "", code?: false, after_tables: []} + } + + statement_b = %Operation.AddCustomStatement{ + table: "widget", + schema: nil, + statement: %{name: :b, up: "", down: "", code?: false, after_tables: []} + } + + # 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) + ) + + refute Enum.any?( + OperationDeps.requires(statement_b), + &match?({:table_finalized, _}, &1) + ) + end end describe "early tier" do From 251aaec2677ed571370b0b3d0bd503807cb15822 Mon Sep 17 00:00:00 2001 From: Jechol Lee Date: Thu, 23 Jul 2026 07:58:00 +0900 Subject: [PATCH 2/2] chore: update deps --- mix.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mix.lock b/mix.lock index dbd08951..c4024605 100644 --- a/mix.lock +++ b/mix.lock @@ -16,7 +16,7 @@ "eflame": {:hex, :eflame, "1.0.1", "0664d287e39eef3c413749254b3af5f4f8b00be71c1af67d325331c4890be0fc", [:mix], [], "hexpm", "e0b08854a66f9013129de0b008488f3411ae9b69b902187837f994d7a99cf04e"}, "erlex": {:hex, :erlex, "0.2.9", "7debbbaa9f4f368b8cd648983e0f1d7963028508e9c59e9d4ed504e94ef52a55", [:mix], [], "hexpm", "8cfffc0ec7159e6d73de2ab28a588064de80f88b2798d5cbe4482cbbc200178b"}, "ets": {:hex, :ets, "0.9.0", "79c6a6c205436780486f72d84230c6cba2f8a9920456750ddd1e47389107d5fd", [:mix], [], "hexpm", "2861fdfb04bcaeff370f1a5904eec864f0a56dcfebe5921ea9aadf2a481c822b"}, - "ex_ast": {:hex, :ex_ast, "0.12.10", "1126eb2afe3218e7a8a53ce85adf1056c528c88fbb5adcbe0191b647b842b377", [:mix], [{:jason, "~> 1.4", [hex: :jason, repo: "hexpm", optional: false]}, {:sourceror, "~> 1.7", [hex: :sourceror, repo: "hexpm", optional: false]}], "hexpm", "e03f668c4354e3a1382c3d762c0fcc82ca9670f6f37e62b9097ce752be6adf39"}, + "ex_ast": {:hex, :ex_ast, "0.13.0", "5e5453e2ad73c26e6c8137ea977cce1d010159099a9f257f122db1a821a49f86", [:mix], [{:jason, "~> 1.4", [hex: :jason, repo: "hexpm", optional: false]}, {:sourceror, "~> 1.7", [hex: :sourceror, repo: "hexpm", optional: false]}], "hexpm", "bb94a77d056e0c61e8aa65ab7807bea8405b290eb853fba17d09685c232fd5bc"}, "ex_check": {:hex, :ex_check, "0.16.0", "07615bef493c5b8d12d5119de3914274277299c6483989e52b0f6b8358a26b5f", [:mix], [], "hexpm", "4d809b72a18d405514dda4809257d8e665ae7cf37a7aee3be6b74a34dec310f5"}, "ex_doc": {:hex, :ex_doc, "0.40.3", "4a972ffe64bc07dc605af487e98fc19b72a4185f55ca031b94c0552d6071c1d9", [:mix], [{:earmark_parser, "~> 1.4.44", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_c, ">= 0.1.0", [hex: :makeup_c, repo: "hexpm", optional: true]}, {:makeup_elixir, "~> 0.14 or ~> 1.0", [hex: :makeup_elixir, repo: "hexpm", optional: false]}, {:makeup_erlang, "~> 0.1 or ~> 1.0", [hex: :makeup_erlang, repo: "hexpm", optional: false]}, {:makeup_html, ">= 0.1.0", [hex: :makeup_html, repo: "hexpm", optional: true]}], "hexpm", "2756e357742fecd9749b489b85d67c9ce99c465f2e75728d9e6dc8d704b973de"}, "file_system": {:hex, :file_system, "1.1.1", "31864f4685b0148f25bd3fbef2b1228457c0c89024ad67f7a81a3ffbc0bbad3a", [:mix], [], "hexpm", "7a15ff97dfe526aeefb090a7a9d3d03aa907e100e262a0f8f7746b78f8f87a5d"},