From fbfd72fd92032d20fb15a0a58da2a55ee80db383 Mon Sep 17 00:00:00 2001 From: Nassim Date: Fri, 9 Oct 2026 04:48:19 +0200 Subject: [PATCH 01/15] improvement: add rebuild_table, a migration helper that rebuilds a table --- lib/migration.ex | 181 +++++++++++++++++++++++ mix.exs | 3 +- test/migration_test.exs | 237 ++++++++++++++++++++++++++++++ test/support/rebuild_helper.ex | 115 +++++++++++++++ test/support/rebuild_test_repo.ex | 27 ++++ 5 files changed, 562 insertions(+), 1 deletion(-) create mode 100644 lib/migration.ex create mode 100644 test/migration_test.exs create mode 100644 test/support/rebuild_helper.ex create mode 100644 test/support/rebuild_test_repo.ex diff --git a/lib/migration.ex b/lib/migration.ex new file mode 100644 index 0000000..7482448 --- /dev/null +++ b/lib/migration.ex @@ -0,0 +1,181 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Migration do + @moduledoc """ + Helpers for migrations that rebuild a table. + + SQLite can mostly add, rename and drop columns in place. For most other changes it is + [documented](https://www.sqlite.org/lang_altertable.html#otheralter) to create the table in + its new shape, copy the rows, drop the old table and rename the new one: + + use AshSqlite.Migration + + def up do + rebuild_table :posts do + add :id, :uuid, null: false, primary_key: true + add :title, :text + end + end + + `use AshSqlite.Migration` does what `use Ecto.Migration` does, and adds what makes a rebuild + safe: foreign keys are off while it runs (dropping a table would otherwise run the `ON DELETE` + actions of the tables that point to it) and they are checked before it commits. A rebuild + either completes or changes nothing. + """ + + @pragmas_key :ash_sqlite_rebuild_pragmas + + @doc false + defmacro __using__(_opts) do + quote do + use Ecto.Migration + import AshSqlite.Migration, only: [rebuild_table: 2, rebuild_table: 3] + + defdelegate before_transaction(repo), to: AshSqlite.Migration, as: :__before_transaction__ + defdelegate after_transaction(repo), to: AshSqlite.Migration, as: :__after_transaction__ + + def before_commit, do: AshSqlite.Migration.__check_foreign_keys__(repo()) + end + end + + # `PRAGMA foreign_keys` does nothing inside a transaction, so it is set before Ecto opens one, + # on the connection it will use. The previous value is kept to put it back afterwards. + @doc false + def __before_transaction__(repo) do + Process.put(@pragmas_key, pragma(repo, "foreign_keys")) + quiet!(repo, "PRAGMA foreign_keys = OFF") + end + + @doc false + def __after_transaction__(repo) do + case Process.delete(@pragmas_key) do + nil -> :ok + foreign_keys -> quiet!(repo, "PRAGMA foreign_keys = #{foreign_keys}") + end + end + + # Ecto does not run the callbacks for a migration without a transaction + # (`@disable_ddl_transaction true`), and dropping a table with foreign keys on deletes + # from the tables that reference it. So `rebuild_table/3` checks first that they are off. + @doc false + def __check_foreign_keys_off__(repo, table) do + if pragma(repo, "foreign_keys") != 0 do + raise "refusing to rebuild #{table}: foreign keys are on, so dropping it would run " <> + "the ON DELETE actions of the tables that reference it. They are switched " <> + "off by the callbacks `use AshSqlite.Migration` adds, which Ecto only runs " <> + "when the migration runs in its own transaction: remove " <> + "`@disable_ddl_transaction true`." + end + end + + @doc """ + Rebuilds `table` with the shape `block` describes, as the block of a `create table`. + + Every column the old and the new table both have is copied as it is. A column the + old table did not have gets the `DEFAULT` of the new table. + + Options: + + * `:copy` - what to copy differently, as a keyword list from the new column to + where its value comes from: `new: :old` copies the column `old` into `new` (a + rename), and `new: "sql"` copies an SQL expression, which can look at the old + table's columns (for example `COALESCE(name, 'unknown')` for a column that is + becoming required, or `'unknown'` for a new required one). + * any other option is an option of the temporary `table/2`, for example + `options: "STRICT"`. + """ + defmacro rebuild_table(table, opts \\ [], do: block) do + quote do + table = unquote(table) + {copy, options} = Keyword.pop(unquote(opts), :copy, []) + temporary = "#{table}_rebuild" + + execute(fn -> AshSqlite.Migration.__check_foreign_keys_off__(repo(), table) end) + + create table(temporary, [primary_key: false] ++ options) do + unquote(block) + end + + # the columns of both tables are only known once the new one exists + execute(fn -> AshSqlite.Migration.__copy_rows__(repo(), table, temporary, copy) end) + + drop(table(table)) + rename(table(temporary), to: table(table)) + end + end + + @doc false + def __copy_rows__(repo, table, temporary, copy) do + old_columns = column_names(repo, table) + new_columns = column_names(repo, temporary) + + unknown = + copy |> Keyword.keys() |> Enum.map(&to_string/1) |> Enum.reject(&(&1 in new_columns)) + + if unknown != [] do + raise ArgumentError, + "`copy:` for #{table} names #{Enum.join(unknown, ", ")}, which the new table " <> + "does not have (it has #{Enum.join(new_columns, ", ")})" + end + + expressions = + for name <- new_columns, + expression = copy_expression(copy, name, old_columns), + do: {name, expression} + + if expressions == [] do + raise ArgumentError, + "`rebuild_table` for #{table} would copy nothing: the new table has no column in " <> + "common with the old one and no `copy:`, so its rows would be lost" + end + + names = Enum.map_join(expressions, ", ", fn {name, _} -> quote_name(name) end) + selected = Enum.map_join(expressions, ", ", fn {_, expression} -> expression end) + + query!( + repo, + "INSERT INTO #{quote_name(temporary)} (#{names}) SELECT #{selected} FROM #{quote_name(table)}" + ) + end + + defp copy_expression(copy, name, old_columns) do + case Enum.find(copy, fn {new, _} -> to_string(new) == name end) do + {_, from} when is_atom(from) -> quote_name(from) + {_, sql} when is_binary(sql) -> sql + nil -> if name in old_columns, do: quote_name(name) + end + end + + defp column_names(repo, table) do + %{rows: rows} = quiet!(repo, "PRAGMA table_info(#{quote_name(table)})") + Enum.map(rows, fn [_cid, name | _] -> name end) + end + + defp quote_name(name), do: ~s("#{name}") + + # not `repo.query!/1`: ecto_libsql's repo has none + # sobelow_skip ["SQL.Query"] + defp query!(repo, statement), do: Ecto.Adapters.SQL.query!(repo, statement) + + # sobelow_skip ["SQL.Query"] + defp quiet!(repo, statement), do: Ecto.Adapters.SQL.query!(repo, statement, [], log: false) + + defp pragma(repo, name) do + %{rows: [[value]]} = quiet!(repo, "PRAGMA #{name}") + value + end + + @doc false + def __check_foreign_keys__(repo) do + case quiet!(repo, "PRAGMA foreign_key_check") do + %{rows: []} -> + :ok + + %{rows: rows} -> + raise "foreign key check failed after rebuilding tables (table, rowid, parent, constraint): " <> + inspect(rows) + end + end +end diff --git a/mix.exs b/mix.exs index 120b48c..1744ac8 100644 --- a/mix.exs +++ b/mix.exs @@ -123,7 +123,8 @@ defmodule AshSqlite.MixProject do AshSqlite.TenantRepo ], Utilities: [ - AshSqlite.ManualRelationship + AshSqlite.ManualRelationship, + AshSqlite.Migration ], Introspection: [ AshSqlite.DataLayer.Info, diff --git a/test/migration_test.exs b/test/migration_test.exs new file mode 100644 index 0000000..8a80ed2 --- /dev/null +++ b/test/migration_test.exs @@ -0,0 +1,237 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MigrationTest do + @moduledoc """ + Runs migrations that use `AshSqlite.Migration` on a real SQLite file. + """ + use ExUnit.Case, async: false + @moduletag :migration + @moduletag :tmp_dir + + import AshSqlite.RebuildHelper + + setup %{tmp_dir: tmp_dir} = context do + repo = Map.get(context, :repo, AshSqlite.RebuildTestRepo) + %{ctx: start(tmp_dir, Map.get(context, :repo_config, []), repo)} + end + + # posts, and comments that are deleted with their post, with a row each + defp blog(ctx) do + write_migration(ctx, 1, "create_blog", """ + def up do + create table(:posts, primary_key: false) do + add :id, :text, primary_key: true + add :title, :text, null: false + add :subtitle, :text + end + + create table(:comments, primary_key: false) do + add :id, :text, primary_key: true + add :post_id, references(:posts, type: :text, on_delete: :delete_all) + end + end + + def down do + drop table(:comments) + drop table(:posts) + end + """) + + migrate(ctx) + sql("INSERT INTO posts (id, title) VALUES ('p', 'hello')") + sql("INSERT INTO comments (id, post_id) VALUES ('c', 'p')") + end + + @tag repo_config: [pool_size: 1] + test "rebuilds a table: its rows stay, and so do the rows that point at it", %{ctx: ctx} do + blog(ctx) + + write_migration(ctx, 2, "subtitle_required", """ + def up do + rebuild_table :posts do + add :id, :text, primary_key: true + add :title, :text + add :subtitle, :text + end + end + + def down, do: :ok + """) + + migrate(ctx) + + assert [%{name: "title", notnull: false}] = + Enum.filter(columns("posts"), &(&1.name == "title")) + + assert sql("SELECT id, title FROM posts") == [["p", "hello"]] + # dropping the old table did not run ON DELETE CASCADE on the comments + assert sql("SELECT id FROM comments") == [["c"]] + assert sql("PRAGMA foreign_keys") == [[1]] + end + + test "copy: renames a column and computes a value", %{ctx: ctx} do + blog(ctx) + + write_migration(ctx, 2, "rename_and_slug", """ + def up do + rebuild_table :posts, copy: [name: :title, slug: "lower(title) || '!'"] do + add :id, :text, primary_key: true + add :name, :text + add :slug, :text + end + end + + def down, do: :ok + """) + + migrate(ctx) + + assert sql("SELECT id, name, slug FROM posts") == [["p", "hello", "hello!"]] + end + + test "a copy: that names a column the new table does not have says so", %{ctx: ctx} do + blog(ctx) + + write_migration(ctx, 2, "typo", """ + def up do + rebuild_table :posts, copy: [nme: "'x'"] do + add :id, :text, primary_key: true + add :name, :text + end + end + + def down, do: :ok + """) + + error = migrate_error(ctx) + assert error =~ "names nme, which the new table does not have" + assert error =~ "it has id, name" + end + + test "a rebuild that would copy nothing refuses, rather than lose the rows", %{ctx: ctx} do + write_migration(ctx, 1, "create_items", """ + def up do + execute "CREATE TABLE items (old TEXT)" + execute "INSERT INTO items VALUES ('a'), ('b')" + end + + def down, do: :ok + """) + + write_migration(ctx, 2, "replace_items", """ + def up do + rebuild_table :items do + add :new, :text, default: "new" + end + end + + def down, do: :ok + """) + + assert migrate_error(ctx) =~ "would copy nothing" + assert sql("SELECT old FROM items ORDER BY old") == [["a"], ["b"]] + assert versions() == [1] + end + + @tag repo_config: [pool_size: 1] + test "when it fails, nothing is changed and foreign keys are back on", %{ctx: ctx} do + blog(ctx) + before = columns("posts") + + write_migration(ctx, 2, "subtitle_required", """ + def up do + rebuild_table :posts do + add :id, :text, primary_key: true + add :title, :text, null: false + add :subtitle, :text, null: false + end + end + + def down, do: :ok + """) + + assert migrate_error(ctx) =~ "NOT NULL constraint failed" + + assert columns("posts") == before + assert sql("SELECT id FROM comments") == [["c"]] + assert leftovers() == [] + assert versions() == [1] + assert sql("PRAGMA foreign_keys") == [[1]] + end + + @tag repo_config: [pool_size: 1] + test "a foreign key that is already violated stops it", %{ctx: ctx} do + blog(ctx) + sql("PRAGMA foreign_keys = OFF") + sql("INSERT INTO comments (id, post_id) VALUES ('orphan', 'nobody')") + + write_migration(ctx, 2, "anything", """ + def up do + rebuild_table :posts do + add :id, :text, primary_key: true + add :title, :text + add :subtitle, :text + end + end + + def down, do: :ok + """) + + assert migrate_error(ctx) =~ "foreign key check failed" + assert versions() == [1] + assert leftovers() == [] + end + + test "a migration without a transaction refuses, rather than drop with foreign keys on", %{ + ctx: ctx + } do + blog(ctx) + + write_migration(ctx, 2, "no_transaction", """ + @disable_ddl_transaction true + + def up do + rebuild_table :posts do + add :id, :text, primary_key: true + add :title, :text + add :subtitle, :text + end + end + + def down, do: :ok + """) + + assert migrate_error(ctx) =~ "refusing to rebuild posts: foreign keys are on" + assert sql("SELECT id FROM comments") == [["c"]] + end + + describe "with ecto_libsql" do + @describetag repo: AshSqlite.RebuildLibSqlRepo + + # it has no `foreign_keys:` connection option, so what the migration does to the PRAGMA is + # all there is: a rebuild that left them on would have deleted the comments + @tag repo_config: [pool_size: 1] + test "a rebuild keeps the rows that point at the table", %{ctx: ctx} do + blog(ctx) + + write_migration(ctx, 2, "title_optional", """ + def up do + rebuild_table :posts do + add :id, :text, primary_key: true + add :title, :text + add :subtitle, :text + end + end + + def down, do: :ok + """) + + migrate(ctx) + + assert sql("SELECT id FROM comments") == [["c"]] + assert sql("PRAGMA foreign_keys") == [[1]] + end + end +end diff --git a/test/support/rebuild_helper.ex b/test/support/rebuild_helper.ex new file mode 100644 index 0000000..d4e63f5 --- /dev/null +++ b/test/support/rebuild_helper.ex @@ -0,0 +1,115 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.RebuildHelper do + @moduledoc """ + Runs migrations against a real SQLite file, for the tests of `AshSqlite.Migration`. + """ + alias AshSqlite.RebuildTestRepo + + # The repo the current test started (the process dictionary: the tests are not async) + defp repo, do: Process.get(:rebuild_repo, RebuildTestRepo) + + @doc """ + Points `AshSqlite.RebuildTestRepo` at a new database file in `tmp_dir` and starts it. + `repo_config` is merged into the repo's configuration (`pool_size: 2`, ...). + Returns a context map the other helpers take. + """ + def start(tmp_dir, repo_config \\ [], repo \\ RebuildTestRepo) do + Process.put(:rebuild_repo, repo) + + ctx = %{ + db: Path.join(tmp_dir, "test.db"), + snapshot_path: Path.join(tmp_dir, "snapshots"), + migration_path: Path.join(tmp_dir, "migrations") + } + + config = + Keyword.merge( + [ + database: ctx.db, + pool_size: 1, + migration_lock: false + ], + repo_config + ) + + Application.put_env(:ash_sqlite, repo, config) + ExUnit.Callbacks.start_supervised!(repo) + ExUnit.Callbacks.on_exit(fn -> Application.delete_env(:ash_sqlite, repo) end) + + ctx + end + + @doc "Writes a migration, `body` being what goes in the module after `use AshSqlite.Migration`." + def write_migration(ctx, version, name, body) do + File.mkdir_p!(ctx.migration_path) + module = Module.concat([repo(), Migrations, Macro.camelize(name)]) + + File.write!(Path.join(ctx.migration_path, "#{version}_#{name}.exs"), """ + defmodule #{inspect(module)} do + use AshSqlite.Migration + + #{body} + end + """) + end + + @doc "Runs all pending migrations. Returns the versions run, or raises." + def migrate(ctx) do + Code.compiler_options(ignore_module_conflict: true) + + try do + Ecto.Migrator.run(repo(), ctx.migration_path, :up, all: true, log: false) + after + Code.compiler_options(ignore_module_conflict: false) + end + end + + @doc "Rolls back the last `n` migrations." + def rollback(ctx, n \\ 1) do + Code.compiler_options(ignore_module_conflict: true) + + try do + Ecto.Migrator.run(repo(), ctx.migration_path, :down, step: n, log: false) + after + Code.compiler_options(ignore_module_conflict: false) + end + end + + @doc "Runs SQL; returns the rows (lists)." + def sql(statement, params \\ []) do + Ecto.Adapters.SQL.query!(repo(), statement, params).rows + end + + def columns(table) do + "SELECT name, type, \"notnull\", dflt_value, pk FROM pragma_table_info('#{table}')" + |> sql() + |> Enum.map(fn [name, type, notnull, default, pk] -> + %{name: name, type: type, notnull: notnull == 1, default: default, pk: pk} + end) + end + + @doc "Runs the pending migrations, which must fail. Returns the exception's message." + def migrate_error(ctx) do + migrate(ctx) + flunk("expected the migration to fail") + rescue + error in ExUnit.AssertionError -> reraise error, __STACKTRACE__ + error -> Exception.message(error) + end + + defp flunk(message), do: raise(ExUnit.AssertionError, message: message) + + @doc "The versions recorded in schema_migrations." + def versions, + do: "SELECT version FROM schema_migrations ORDER BY version" |> sql() |> List.flatten() + + @doc "Tables left over from a rebuild (named `*_rebuild`)." + def leftovers do + "SELECT name FROM sqlite_master WHERE type = 'table' AND name LIKE '%\\_rebuild' ESCAPE '\\'" + |> sql() + |> List.flatten() + end +end diff --git a/test/support/rebuild_test_repo.ex b/test/support/rebuild_test_repo.ex new file mode 100644 index 0000000..e6851e7 --- /dev/null +++ b/test/support/rebuild_test_repo.ex @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.RebuildTestRepo do + @moduledoc """ + A repo with an ordinary pool (no sandbox) whose database file is chosen per test + through the application environment, see `AshSqlite.RebuildHelper`. + """ + use AshSqlite.Repo, + otp_app: :ash_sqlite + + def min_sqlite_version do + %Version{major: 3, minor: 38, patch: 0} + end +end + +defmodule AshSqlite.RebuildLibSqlRepo do + @moduledoc "Like `AshSqlite.RebuildTestRepo`, on the optional ecto_libsql adapter." + use AshSqlite.Repo, + otp_app: :ash_sqlite, + adapter: Ecto.Adapters.LibSql + + def min_sqlite_version do + %Version{major: 3, minor: 38, patch: 0} + end +end From 7384d69dc4f551e8f3c6dd53dd091d5d526e2542 Mon Sep 17 00:00:00 2001 From: Nassim Date: Fri, 9 Oct 2026 04:48:54 +0200 Subject: [PATCH 02/15] improvement: rebuild tables sqlite cannot alter in place (opt-in) --- .../development/migrations-and-tasks.md | 21 + .../migration_generator.ex | 77 +- lib/migration_generator/operation.ex | 290 ++++++ .../tasks/ash_sqlite.generate_migrations.ex | 13 +- lib/repo.ex | 2 + test/migration_generator_test.exs | 332 +++++++ test/support/rebuild_helper.ex | 32 +- test/table_rebuild_test.exs | 859 ++++++++++++++++++ 8 files changed, 1618 insertions(+), 8 deletions(-) create mode 100644 test/table_rebuild_test.exs diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index e94f53e..15dde46 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -37,6 +37,27 @@ For single-step changes or when you know the final feature name: For more information on generating migrations, run `mix help ash_sqlite.generate_migrations` (the underlying task that is called by `mix ash.codegen`) +### Changes SQLite cannot make in place + +SQLite can mostly add, rename and drop a column in place. Changing a column's type or default, adding or dropping a foreign key, changing a primary key and, as the adapters write it, making a column required or optional all need the table to be created again in its new shape, with the rows copied over. + +By default the generator writes a statement that fails when the migration runs. With `--rebuild-tables`, or `rebuild_tables: true` in the repo's config, it writes a `rebuild_table` instead, which shows the table's new shape and what triggered it: + +```elixir +def up do + # SQLite cannot change `comments` in place, so it is rebuilt from the resource's + # snapshot. Anything on the table that the resource does not describe (a column, + # index or trigger added by hand) is not kept. Changes: + # - dropping the foreign key comments_post_id_fkey + rebuild_table :comments do + add :id, :uuid, null: false, primary_key: true + add :post_id, :uuid + end +end +``` + +The rebuild runs in one transaction with foreign keys off, and changes nothing if it fails. + ### Regenerating Migrations Often, you will run into a situation where you want to make a slight change to a resource after you've already generated and run migrations. If you are using git and would like to undo those changes, then regenerate the migrations, this script may prove useful: diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index 067a445..9e357e9 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -21,7 +21,8 @@ defmodule AshSqlite.MigrationGenerator do check: false, dev: false, auto_name: false, - drop_columns: false + drop_columns: false, + rebuild_tables: nil def generate(domains, opts \\ []) do domains = List.wrap(domains) @@ -361,11 +362,12 @@ defmodule AshSqlite.MigrationGenerator do end end + phases = organize_operations(operations) + migration_files = - operations - |> organize_operations + phases |> build_up_and_down() - |> migration(repo, opts) + |> migration(repo, rebuilds?(phases), opts) snapshot_files = create_new_snapshot(snapshots, repo_name(repo), opts) @@ -853,7 +855,7 @@ defmodule AshSqlite.MigrationGenerator do repo |> Module.split() |> List.last() |> Macro.underscore() end - defp migration({up, down}, repo, opts) do + defp migration({up, down}, repo, rebuilds?, opts) do migration_path = migration_path(opts, repo) require_name!(opts) @@ -901,7 +903,7 @@ defmodule AshSqlite.MigrationGenerator do This file was autogenerated with `mix ash_sqlite.generate_migrations` \"\"\" - use Ecto.Migration + #{if rebuilds?, do: "use AshSqlite.Migration", else: "use Ecto.Migration"} def up do #{up} @@ -1009,6 +1011,8 @@ defmodule AshSqlite.MigrationGenerator do {up, down} end + defp rebuilds?(phases), do: Enum.any?(phases, &match?(%Operation.RebuildTable{}, &1)) + defp maybe_comment(text, %{commented?: true}) do text |> String.split("\n") @@ -1711,8 +1715,69 @@ defmodule AshSqlite.MigrationGenerator do |> Enum.concat() |> Enum.map(&Map.put(&1, :multitenancy, snapshot.multitenancy)) |> Enum.map(&Map.put(&1, :old_multitenancy, old_snapshot.multitenancy)) + |> rebuild_if_needed(snapshot, old_snapshot, opts) + end + + # `--rebuild-tables` (or `--no-rebuild-tables`) for this run, else the repo's `rebuild_tables`. + defp rebuild_tables?(%{rebuild_tables: nil}, repo), do: repo.config()[:rebuild_tables] == true + defp rebuild_tables?(%{rebuild_tables: rebuild_tables}, _repo), do: rebuild_tables + + defp rebuild_if_needed(operations, snapshot, old_snapshot, opts) do + if !old_snapshot[:empty?] and + Enum.any?(operations, &Operation.RebuildTable.requires_rebuild?/1) do + if rebuild_tables?(opts, snapshot.repo) do + [ + %Operation.RebuildTable{ + table: snapshot.table, + old_snapshot: old_snapshot, + snapshot: snapshot, + changes: operations + } + ] + else + hint_rebuild(operations, snapshot, opts) + operations + end + else + operations + end end + defp hint_rebuild(operations, snapshot, opts) do + unless opts.quiet do + bullets = + Enum.map_join( + Operation.RebuildTable.blocking_reasons(operations), + "\n", + fn {reason, blocks} -> " - #{reason} (#{blocked_when(blocks)})" end + ) + + warn(""" + SQLite cannot make these changes to `#{snapshot.table}` in place: + #{bullets} + To rebuild the table instead, #{rebuild_advice(opts)} + """) + end + end + + defp blocked_when(:run), do: "fails when the migration runs" + defp blocked_when(:rollback), do: "fails when rolled back" + + defp rebuild_advice(%{dry_run: true}), do: "run again with `--rebuild-tables`." + defp rebuild_advice(%{check: true}), do: "run again with `--rebuild-tables`." + + defp rebuild_advice(%{dev: true}), + do: "make the final run (`mix ash.codegen `) with `--rebuild-tables`." + + defp rebuild_advice(_opts) do + "undo this run (roll the migrations back if you ran them, editing any `down` that " <> + "raises, then delete them and the snapshots they wrote) and generate again with " <> + "`--rebuild-tables`, or with `rebuild_tables: true` in the repo's config." + end + + # yellow, to stand out in a `--dry-run` + defp warn(message), do: Mix.shell().info([:yellow, message]) + defp indexes_match?(table, left, right) do left = left diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index bef9ecf..025eab0 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -820,4 +820,294 @@ defmodule AshSqlite.MigrationGenerator.Operation do end end end + + defmodule RebuildTable do + @moduledoc false + # The operations of a table when one of them cannot be done in place (`requires_rebuild?/1`): + # a `rebuild_table` of the table's new shape, followed by its indexes. + defstruct [:table, :old_snapshot, :snapshot, :changes, no_phase: true] + + import Helper + + alias AshSqlite.MigrationGenerator.Operation + + def requires_rebuild?(%Operation.AlterAttribute{}), do: true + def requires_rebuild?(%Operation.DropForeignKey{}), do: true + def requires_rebuild?(%Operation.AlterDeferrability{}), do: true + def requires_rebuild?(%Operation.RemovePrimaryKey{}), do: true + + def requires_rebuild?(%Operation.AddAttribute{attribute: attribute}), + do: attribute.allow_nil? == false and attribute.default == "nil" + + def requires_rebuild?(_), do: false + + def up(%{old_snapshot: old, snapshot: new, changes: changes}) do + renames = renames(changes) + leftover = leftover_columns(old, new, renames, changes) + + render(%{ + table: new.table, + from: old, + from_columns: old.attributes, + to: new, + to_columns: new.attributes ++ Enum.map(leftover, &as_nullable/1), + renames: renames, + reasons: reasons(changes), + notes: type_notes(old, new, changes) ++ leftover_notes(leftover) + }) + end + + def down(%{old_snapshot: old, snapshot: new, changes: changes}) do + up_renames = renames(changes) + leftover = leftover_columns(old, new, up_renames, changes) + + render(%{ + table: new.table, + from: new, + from_columns: new.attributes ++ Enum.map(leftover, &as_nullable/1), + to: old, + to_columns: old.attributes, + renames: Map.new(up_renames, fn {to, from} -> {from, to} end), + reasons: ["undoing the rebuild above (rows in columns it added are not kept)"], + notes: [] + }) + end + + defp render(%{table: table} = plan) do + columns = plan.to_columns + + lines = plan.reasons ++ plan.notes + + options = + [ + copy_option(columns, plan), + if(plan.to.strict?, do: ~s|options: "STRICT"|) + ] + |> Enum.reject(&is_nil/1) + |> Enum.map_join("", &(", " <> &1)) + + adds = + Enum.map_join(columns, "\n", fn attribute -> + Operation.AddAttribute.up(%Operation.AddAttribute{ + attribute: attribute, + table: table, + multitenancy: plan.to.multitenancy, + old_multitenancy: plan.from.multitenancy + }) + end) + + [ + header(table, lines), + "rebuild_table :#{as_atom(table)}#{options} do\n#{adds}\nend", + Enum.map(plan.to.identities, fn identity -> + Operation.AddUniqueIndex.up(%{ + identity: identity, + table: table, + multitenancy: plan.to.multitenancy + }) + end), + Enum.map(plan.to.custom_indexes, fn index -> + Operation.AddCustomIndex.up(%{ + index: index, + table: table, + base_filter: plan.to.base_filter, + multitenancy: plan.to.multitenancy + }) + end) + ] + |> List.flatten() + |> Enum.join("\n") + end + + defp header(table, lines) do + Enum.join( + [ + "# SQLite cannot change `#{table}` in place, so it is rebuilt from the resource's", + "# snapshot. Anything on the table that the resource does not describe (a column,", + "# index or trigger added by hand) is not kept. Changes:" + | Enum.flat_map(lines, &entry_lines/1) + ], + "\n" + ) + end + + defp entry_lines(entry) do + case String.split(entry, "\n") do + [first | rest] -> ["# - #{first}" | Enum.map(rest, &"# #{&1}")] + end + end + + # `copy: [subject: :title, name: "COALESCE(name, 'x')"]`: only what differs from copying + # the column of the same name; nil when nothing does. + defp copy_option(columns, plan) do + entries = + for column <- columns, + from_name = Map.get(plan.renames, column.source, column.source), + from_column = Enum.find(plan.from_columns, &(&1.source == from_name)), + entry = copy_entry(column, from_name, from_column), + do: entry + + if entries != [], do: "copy: [#{Enum.join(entries, ", ")}]" + end + + defp copy_entry(column, from_name, from_column) do + cond do + expression = backfill(from_column, column, quote_name(from_name)) -> + "#{atom_key(column.source)} #{inspect(expression)}" + + from_name == column.source -> + nil + + true -> + "#{atom_key(column.source)} :#{as_atom(from_name)}" + end + end + + defp atom_key(name), do: Macro.inspect_atom(:key, atom(name)) + + defp atom(name) when is_atom(name), do: name + # sobelow_skip ["DOS.StringToAtom"] + defp atom(name), do: String.to_atom(name) + + defp backfill(%{allow_nil?: true}, %{allow_nil?: false} = column, expression) do + case default_sql(column.default) do + nil -> nil + default -> "COALESCE(#{expression}, #{default})" + end + end + + defp backfill(_from, _to, _expression), do: nil + + defp renames(changes) do + for %Operation.RenameAttribute{old_attribute: old, new_attribute: new} <- changes, + into: %{}, + do: {new.source, old.source} + end + + # The columns of the old table that the resource no longer describes. They stay, as nullable + # columns, unless the author asked for them to be dropped (`--drop-columns`). + defp leftover_columns(old, new, renames, changes) do + new_names = MapSet.new(new.attributes, & &1.source) + renamed_away = MapSet.new(Map.values(renames)) + + dropped = + for %Operation.RemoveAttribute{attribute: attribute, commented?: false} <- changes, + into: MapSet.new(), + do: attribute.source + + Enum.reject(old.attributes, fn attribute -> + MapSet.member?(new_names, attribute.source) or + MapSet.member?(renamed_away, attribute.source) or + MapSet.member?(dropped, attribute.source) + end) + end + + defp type_notes(_old, %{strict?: true}, _changes), do: [] + + defp type_notes(old, _new, changes) do + for %Operation.AlterAttribute{old_attribute: from, new_attribute: to} <- changes, + from.type != to.type, + Enum.any?(old.attributes, &(&1.source == from.source)) do + "REVIEW: `#{to.source}` changes type in a table that is not STRICT, so values that " <> + "do not convert are copied as they are, not rejected" + end + end + + defp leftover_notes([]), do: [] + + defp leftover_notes(leftover) do + names = Enum.map_join(leftover, ", ", &inspect(&1.source)) + + [ + "#{names} no longer in the resource, kept as nullable so no data is lost " <> + "(to drop one, delete its line from the `rebuild_table` below)" + ] + end + + defp as_nullable(attribute) do + %{attribute | allow_nil?: true, default: "nil", primary_key?: false, references: nil} + end + + def reasons(changes), do: changes |> blocking_reasons() |> Enum.map(&elem(&1, 0)) + + def blocking_reasons(changes) do + for change <- changes, + requires_rebuild?(change), + !covered_by_other_change?(change, changes), + reason = reason(change), + uniq: true, + do: {reason, blocks(change)} + end + + defp blocks(%Operation.DropForeignKey{direction: :down}), do: :rollback + defp blocks(_change), do: :run + + # Changing a foreign key is two operations, dropping it and altering the column. One line + # says it: the drop when the column no longer has a foreign key (and the alteration is only + # that), the alteration when it has another one. + defp covered_by_other_change?( + %Operation.DropForeignKey{attribute: %{source: source}}, + changes + ) do + Enum.any?( + changes, + &match?(%Operation.AlterAttribute{new_attribute: %{source: ^source, references: %{}}}, &1) + ) + end + + defp covered_by_other_change?( + %Operation.AlterAttribute{old_attribute: old, new_attribute: %{references: nil} = new}, + changes + ) do + Enum.any?( + changes, + &match?( + %Operation.DropForeignKey{attribute: %{source: source}} when source == new.source, + &1 + ) + ) and %{old | references: nil} == new + end + + defp covered_by_other_change?(_change, _changes), do: false + + defp reason(%Operation.AlterAttribute{} = change), + do: change |> Operation.AlterAttribute.up() |> String.trim() + + defp reason(%Operation.AddAttribute{attribute: attribute}) do + "add #{inspect(attribute.source)} as NOT NULL without a default, " <> + "which SQLite refuses on a table with rows" + end + + defp reason(%Operation.DropForeignKey{direction: :down, attribute: %{references: reference}}), + do: "dropping the foreign key #{reference.name} when rolling back" + + defp reason(%Operation.DropForeignKey{attribute: %{references: reference}}), + do: "dropping the foreign key #{reference.name}" + + defp reason(%Operation.RemovePrimaryKey{}), do: "changing the primary key" + + defp reason(%Operation.AlterDeferrability{references: %{name: name, deferrable: deferrable}}) + when deferrable not in [false, nil], + do: + "NOT APPLIED: the foreign key #{name} is deferrable " <> + "(SQLite migrations cannot create DEFERRABLE constraints)" + + defp reason(%Operation.AlterDeferrability{}), do: nil + + defp quote_name(name), do: ~s("#{name}") + + defp default_sql(nil), do: nil + defp default_sql("nil"), do: nil + + defp default_sql(code) when is_binary(code) do + case Code.string_to_quoted(code) do + {:ok, value} when is_binary(value) -> "'" <> String.replace(value, "'", "''") <> "'" + {:ok, value} when is_number(value) -> to_string(value) + {:ok, true} -> "1" + {:ok, false} -> "0" + {:ok, {:fragment, _, [sql]}} when is_binary(sql) -> sql + _ -> nil + end + end + end end diff --git a/lib/mix/tasks/ash_sqlite.generate_migrations.ex b/lib/mix/tasks/ash_sqlite.generate_migrations.ex index 1213209..4827ae5 100644 --- a/lib/mix/tasks/ash_sqlite.generate_migrations.ex +++ b/lib/mix/tasks/ash_sqlite.generate_migrations.ex @@ -13,6 +13,9 @@ defmodule Mix.Tasks.AshSqlite.GenerateMigrations do * `migration-path` - a custom path to store the migrations, defaults to "priv". Migrations are stored in a folder for each repo, so `priv/repo_name/migrations` * `drop-columns` - whether or not to drop columns as attributes are removed. See below for more + * `rebuild-tables` - rebuild a table when SQLite cannot make a change in place. Can be set for + every run with `rebuild_tables: true` in the repo's config, and turned off for one run with + `--no-rebuild-tables`. See "Rebuilding tables" below * `name` - names the generated migrations, prepending with the timestamp. The default is `migrate_resources_`, where `` is the count of migrations matching `*migrate_resources*` plus one. @@ -46,6 +49,13 @@ defmodule Mix.Tasks.AshSqlite.GenerateMigrations do to tell it to uncomment those statements. Additionally, you can just uncomment that code on a case by case basis. + #### Rebuilding tables + + SQLite and its Ecto adapters cannot change a column's type, default or nullability, or a foreign key, + in place. By default the generator writes a statement that fails when the migration runs, and prints a hint + saying so. With `--rebuild-tables` it writes a `rebuild_table` instead, which creates the table + again in its new shape and copies the rows. See the "Migrations" guide. + #### Conflicts/Multiple Resources It will raise on conflicts that it can't resolve, like the same field with different @@ -117,7 +127,8 @@ defmodule Mix.Tasks.AshSqlite.GenerateMigrations do check: :boolean, dev: :boolean, auto_name: :boolean, - drop_columns: :boolean + drop_columns: :boolean, + rebuild_tables: :boolean ] ) diff --git a/lib/repo.ex b/lib/repo.ex index b067efd..d25a464 100644 --- a/lib/repo.ex +++ b/lib/repo.ex @@ -20,6 +20,8 @@ defmodule AshSqlite.Repo do - `:tenant_migrations_path` - The path where your tenant migrations are stored (only relevant for a multitenant implementation) - `:snapshots_path` - The path where the resource snapshots for the migration generator are stored. + - `:rebuild_tables` - Set to `true` to rebuild tables SQLite cannot change in place, as if + `--rebuild-tables` were passed to every run (`--no-rebuild-tables` turns it off for one run). """ @doc "Use this to inform the data layer about what extensions are installed" diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index 0f131da..52d44af 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -8,6 +8,7 @@ defmodule AshSqlite.MigrationGeneratorTest do @moduletag :tmp_dir import ExUnit.CaptureLog + import AshSqlite.RebuildHelper, only: [generate: 2, generate: 3, last_migration: 1] setup %{tmp_dir: tmp_dir} do current_shell = Mix.shell() @@ -1596,4 +1597,335 @@ defmodule AshSqlite.MigrationGeneratorTest do assert File.read!(file2) =~ ~S[rename table(:posts), :contributer2_id, to: :contributer_id] end end + + describe "rebuilding tables" do + defmacrop defitem(do: body) do + quote do + defresource Item, "items" do + unquote(body) + end + end + end + + defmacrop defparent_and_child do + quote do + defresource Parent, "parents" do + attributes do + uuid_primary_key(:id) + end + end + + defresource Child, "children" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:parent, Parent) + end + end + end + end + + # `name` was required and is now optional, which SQLite cannot do in place + defp name_required_then_optional(ctx) do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + defdomain([Item]) + generate(Domain, ctx, rebuild_tables: nil) + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + # the alias made by `defdomain` only lives in this function + Domain + end + + test "without the option, a change SQLite cannot make is written as it always was", ctx do + domain = name_required_then_optional(ctx) + generate(domain, ctx, rebuild_tables: nil, quiet: false) + + migration = last_migration(ctx) + assert migration =~ "modify :name, :text, null: true" + refute migration =~ "rebuild_table" + refute migration =~ "AshSqlite.Migration" + + assert_received {:mix_shell, :info, + ["SQLite cannot make these changes to `items`" <> _ = hint]} + + assert hint =~ " - modify :name, :text, null: true (fails when the migration runs)" + assert hint =~ "--rebuild-tables" + assert hint =~ "rebuild_tables: true" + end + + test "without the option, the hint's advice depends on the run", ctx do + domain = name_required_then_optional(ctx) + + generate(domain, ctx, rebuild_tables: nil, quiet: false, dry_run: true) + assert_received {:mix_shell, :info, ["SQLite cannot make" <> _ = hint]} + assert hint =~ "run again with `--rebuild-tables`." + + # the dev migration may have been run: the final run takes care of it + generate(domain, ctx, rebuild_tables: nil, quiet: false, dev: true) + assert_received {:mix_shell, :info, ["SQLite cannot make" <> _ = hint]} + assert hint =~ "make the final run" + + generate(domain, ctx, rebuild_tables: nil, quiet: false) + assert_received {:mix_shell, :info, ["SQLite cannot make" <> _ = hint]} + assert hint =~ "undo this run" + end + + test "without the option, a dropped foreign key still raises with guidance", ctx do + defparent_and_child() + defdomain([Parent, Child]) + generate(Domain, ctx, rebuild_tables: nil) + + defresource Child, "children" do + attributes do + uuid_primary_key(:id) + attribute(:parent_id, :uuid) + end + end + + defdomain([Parent, Child]) + generate(Domain, ctx, rebuild_tables: nil) + + migration = last_migration(ctx) + assert migration =~ "SQLite does not support dropping foreign key constraints." + refute migration =~ "rebuild_table" + end + + test "without the option, the hint says that a new required column is refused", ctx do + defitem do + attributes do + uuid_primary_key(:id) + end + end + + defdomain([Item]) + generate(Domain, ctx, rebuild_tables: nil) + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:code, :string, allow_nil?: false) + end + end + + generate(Domain, ctx, rebuild_tables: nil, quiet: false) + + assert_received {:mix_shell, :info, + ["SQLite cannot make these changes to `items`" <> _ = hint]} + + assert hint =~ " - add :code as NOT NULL without a default, which SQLite refuses" + end + + test "without the option, a new foreign key column says that rolling back is blocked", ctx do + defresource Owner, "owners" do + attributes do + uuid_primary_key(:id) + end + end + + defresource Pet, "pets" do + attributes do + uuid_primary_key(:id) + end + end + + defdomain([Owner, Pet]) + generate(Domain, ctx, rebuild_tables: nil) + + defresource Pet, "pets" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:owner, Owner) + end + end + + generate(Domain, ctx, rebuild_tables: nil, quiet: false) + + # the way up works, as it always did + migration = last_migration(ctx) + assert migration =~ "add :owner_id, references(:owners" + refute migration =~ "rebuild_table" + + assert_received {:mix_shell, :info, + ["SQLite cannot make these changes to `pets`" <> _ = hint]} + + assert hint =~ + " - dropping the foreign key pets_owner_id_fkey when rolling back (fails when rolled back)" + + refute hint =~ "fails when the migration runs" + end + + test "with the option, a new foreign key column is rebuilt, so that it can be rolled back", + ctx do + defresource Owner, "owners" do + attributes do + uuid_primary_key(:id) + end + end + + defresource Pet, "pets" do + attributes do + uuid_primary_key(:id) + end + end + + defdomain([Owner, Pet]) + generate(Domain, ctx) + + defresource Pet, "pets" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:owner, Owner) + end + end + + generate(Domain, ctx) + + [up, down] = String.split(last_migration(ctx), "def down do") + assert up =~ "# - dropping the foreign key pets_owner_id_fkey when rolling back" + assert up =~ "rebuild_table :pets" + assert down =~ "rebuild_table :pets" + end + + test "with the option, there is no hint and the table is rebuilt", ctx do + domain = name_required_then_optional(ctx) + generate(domain, ctx, rebuild_tables: true, quiet: false) + + refute_received {:mix_shell, :info, ["SQLite cannot make these changes" <> _]} + assert last_migration(ctx) =~ "rebuild_table :items" + end + + test "the copy is not written when every column is simply copied", ctx do + domain = name_required_then_optional(ctx) + generate(domain, ctx) + + refute last_migration(ctx) =~ "copy:" + refute last_migration(ctx) =~ "REVIEW" + end + + test "the copy lists only the rename", ctx do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:title, :string) + attribute(:body, :string) + end + end + + defdomain([Item]) + generate(Domain, ctx) + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:subject, :string) + attribute(:body, :string, allow_nil?: false) + end + end + + send(self(), {:mix_shell_input, :yes?, true}) + send(self(), {:mix_shell_input, :prompt, "subject"}) + generate(Domain, ctx) + assert last_migration(ctx) =~ "rebuild_table :items, copy: [subject: :title] do" + end + + test "a default fills the rows of a required column, so there is no REVIEW", ctx do + defitem do + sqlite do + migration_defaults(name: "\"unnamed\"") + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + defdomain([Item]) + generate(Domain, ctx) + + defitem do + sqlite do + migration_defaults(name: "\"unnamed\"") + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + refute last_migration(ctx) =~ "REVIEW" + assert last_migration(ctx) =~ "COALESCE" + end + + test "the comment says once that a foreign key is dropped", ctx do + defparent_and_child() + defdomain([Parent, Child]) + generate(Domain, ctx) + + defresource Child, "children" do + attributes do + uuid_primary_key(:id) + attribute(:parent_id, :uuid) + end + end + + defdomain([Parent, Child]) + generate(Domain, ctx) + + [up, _down] = String.split(last_migration(ctx), "def down do") + assert up =~ "# - dropping the foreign key children_parent_id_fkey" + refute up =~ "modify :parent_id" + end + + test "the comment says once that a foreign key is changed", ctx do + defparent_and_child() + defdomain([Parent, Child]) + generate(Domain, ctx) + + defresource Child, "children" do + sqlite do + references do + reference(:parent, on_delete: :delete) + end + end + + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:parent, Parent) + end + end + + defdomain([Parent, Child]) + generate(Domain, ctx) + + [up, _down] = String.split(last_migration(ctx), "def down do") + assert up =~ "# - modify :parent_id, references(:parents" + refute up =~ "dropping the foreign key" + end + end end diff --git a/test/support/rebuild_helper.ex b/test/support/rebuild_helper.ex index d4e63f5..d3822eb 100644 --- a/test/support/rebuild_helper.ex +++ b/test/support/rebuild_helper.ex @@ -4,7 +4,7 @@ defmodule AshSqlite.RebuildHelper do @moduledoc """ - Runs migrations against a real SQLite file, for the tests of `AshSqlite.Migration`. + Runs generated migrations against a real SQLite file, for the table rebuild tests. """ alias AshSqlite.RebuildTestRepo @@ -42,6 +42,33 @@ defmodule AshSqlite.RebuildHelper do ctx end + @doc "Generates migrations for `domain` (as `mix ash_sqlite.generate_migrations` would)." + def generate(domain, ctx, opts \\ []) do + AshSqlite.MigrationGenerator.generate( + domain, + Keyword.merge( + [ + snapshot_path: ctx.snapshot_path, + migration_path: ctx.migration_path, + quiet: true, + format: false, + auto_name: true, + rebuild_tables: true + ], + opts + ) + ) + + migrations(ctx) + end + + @doc "The generated migration files, oldest first." + def migrations(ctx) do + Enum.sort(Path.wildcard("#{ctx.migration_path}/**/*_migrate_resources*.exs")) + end + + def last_migration(ctx), do: ctx |> migrations() |> List.last() |> File.read!() + @doc "Writes a migration, `body` being what goes in the module after `use AshSqlite.Migration`." def write_migration(ctx, version, name, body) do File.mkdir_p!(ctx.migration_path) @@ -83,6 +110,9 @@ defmodule AshSqlite.RebuildHelper do Ecto.Adapters.SQL.query!(repo(), statement, params).rows end + @doc "`PRAGMA foreign_key_check` rows (an empty list means all foreign keys hold)." + def fk_violations, do: sql("PRAGMA foreign_key_check") + def columns(table) do "SELECT name, type, \"notnull\", dflt_value, pk FROM pragma_table_info('#{table}')" |> sql() diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs new file mode 100644 index 0000000..623f61d --- /dev/null +++ b/test/table_rebuild_test.exs @@ -0,0 +1,859 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.TableRebuildTest do + @moduledoc """ + Generates migrations for resources that change in ways SQLite cannot do in place, runs them + on a real SQLite file and checks what happens to the data. + """ + use ExUnit.Case, async: false + @moduletag :migration + @moduletag :rebuild + @moduletag :tmp_dir + + import AshSqlite.RebuildHelper + + setup %{tmp_dir: tmp_dir} = context do + current_shell = Mix.shell() + :ok = Mix.shell(Mix.Shell.Process) + on_exit(fn -> Mix.shell(current_shell) end) + + %{ + ctx: + start( + tmp_dir, + Map.get(context, :repo_config, []), + Map.get(context, :repo, AshSqlite.RebuildTestRepo) + ) + } + end + + # Defines a resource on the rebuild test repo. Redefining a module is how a test + # moves a resource from one version to the next. + defmacrop defres(mod, table, do: body) do + quote do + defres_on(AshSqlite.RebuildTestRepo, unquote(mod), unquote(table), do: unquote(body)) + end + end + + defmacrop defres_on(repo, mod, table, do: body) do + quote do + Code.compiler_options(ignore_module_conflict: true) + + defmodule unquote(mod) do + use Ash.Resource, + domain: nil, + data_layer: AshSqlite.DataLayer + + sqlite do + table unquote(table) + repo(unquote(repo)) + end + + actions do + defaults([:create, :read, :update, :destroy]) + end + + unquote(body) + end + + Code.compiler_options(ignore_module_conflict: false) + end + end + + defmacrop defdomain(resources) do + quote do + Code.compiler_options(ignore_module_conflict: true) + + defmodule Domain do + use Ash.Domain, validate_config_inclusion?: false + + resources do + for resource <- unquote(resources) do + resource(resource) + end + end + end + + Code.compiler_options(ignore_module_conflict: false) + end + end + + describe "making a column nullable" do + test "keeps the rows and the child rows that cascade from them", %{ctx: ctx} do + defres Parent, "parents" do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + defres Child, "children" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:parent, Parent, allow_nil?: false) + end + + sqlite do + references do + reference(:parent, on_delete: :delete) + end + end + end + + defdomain([Parent, Child]) + generate(Domain, ctx) + migrate(ctx) + + sql("INSERT INTO parents (id, name) VALUES ('p1', 'one'), ('p2', 'two')") + sql("INSERT INTO children (id, parent_id) VALUES ('c1', 'p1'), ('c2', 'p2')") + + defres Parent, "parents" do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migration = last_migration(ctx) + assert migration =~ "use AshSqlite.Migration" + migrate(ctx) + + assert sql("SELECT id, name FROM parents ORDER BY id") == [["p1", "one"], ["p2", "two"]] + assert sql("SELECT id FROM children ORDER BY id") == [["c1"], ["c2"]] + assert fk_violations() == [] + + assert [%{name: "name", notnull: false}] = + Enum.filter(columns("parents"), &(&1.name == "name")) + end + end + + # A resource on the table "items", the one most scenarios change. + defmacrop defitem(do: body) do + quote do + defres Item, "items" do + unquote(body) + end + + defdomain([Item]) + end + end + + describe "making a column required" do + test "without a default, fails loudly when a row holds NULL and leaves nothing behind", %{ + ctx: ctx + } do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name) VALUES ('a', 'x'), ('b', NULL)") + versions_before = versions() + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + assert migrate_error(ctx) =~ "NOT NULL constraint failed" + + assert leftovers() == [] + assert versions() == versions_before + assert sql("SELECT id, name FROM items ORDER BY id") == [["a", "x"], ["b", nil]] + + assert [%{name: "name", notnull: false}] = + Enum.filter(columns("items"), &(&1.name == "name")) + end + + test "with a default, fills the NULLs with it", %{ctx: ctx} do + defitem do + sqlite do + migration_defaults(name: "\"unnamed\"") + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name) VALUES ('a', 'x'), ('b', NULL)") + + defitem do + sqlite do + migration_defaults(name: "\"unnamed\"") + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false, default: "unnamed") + end + end + + generate(Domain, ctx) + assert last_migration(ctx) =~ "COALESCE" + migrate(ctx) + + assert sql("SELECT id, name FROM items ORDER BY id") == [["a", "x"], ["b", "unnamed"]] + end + end + + describe "adding a required column without a default" do + test "works on an empty table and fails loudly on a table with rows", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + end + end + + generate(Domain, ctx) + migrate(ctx) + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + sql("INSERT INTO items (id) VALUES ('a')") + assert migrate_error(ctx) =~ "NOT NULL constraint failed: items_rebuild.name" + assert leftovers() == [] + assert sql("SELECT id FROM items") == [["a"]] + + sql("DELETE FROM items") + migrate(ctx) + + assert [%{name: "name", notnull: true}] = + Enum.filter(columns("items"), &(&1.name == "name")) + end + end + + describe "defaults that are not literals" do + test "a new column keeps its SQL default for the rows that exist", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name) VALUES ('a', 'x'), ('b', 'y')") + + defitem do + sqlite do + migration_defaults(stamp: "fragment(\"(datetime('now'))\")") + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false, default: "z") + attribute(:stamp, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + + assert [[stamp_a], [stamp_b]] = sql("SELECT stamp FROM items ORDER BY id") + assert is_binary(stamp_a) and is_binary(stamp_b) + end + end + + describe "foreign key topology" do + test "a self-referencing table", %{ctx: ctx} do + defres Node, "nodes" do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + + relationships do + belongs_to(:parent, Node) + end + + sqlite do + references do + reference(:parent, on_delete: :delete) + end + end + end + + defdomain([Node]) + generate(Domain, ctx) + migrate(ctx) + + sql( + "INSERT INTO nodes (id, name, parent_id) VALUES ('root', 'r', NULL), ('kid', 'k', 'root')" + ) + + defres Node, "nodes" do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + + relationships do + belongs_to(:parent, Node) + end + + sqlite do + references do + reference(:parent, on_delete: :delete) + end + end + end + + generate(Domain, ctx) + migrate(ctx) + + assert sql("SELECT id, parent_id FROM nodes ORDER BY id") == [ + ["kid", "root"], + ["root", nil] + ] + + assert fk_violations() == [] + # still a working cascade pointing at the table itself + sql("DELETE FROM nodes WHERE id = 'root'") + assert sql("SELECT id FROM nodes") == [] + end + + test "a new column with a foreign key can be rolled back", %{ctx: ctx} do + defres Owner, "owners" do + attributes do + uuid_primary_key(:id) + end + end + + defres Pet, "pets" do + attributes do + uuid_primary_key(:id) + end + end + + defdomain([Owner, Pet]) + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO pets (id) VALUES ('p')") + + defres Pet, "pets" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:owner, Owner) + end + end + + generate(Domain, ctx) + migrate(ctx) + assert [[_, _, "owners" | _]] = sql("PRAGMA foreign_key_list(pets)") + + rollback(ctx) + assert Enum.map(columns("pets"), & &1.name) == ["id"] + assert sql("SELECT id FROM pets") == [["p"]] + assert fk_violations() == [] + end + + test "a table created in the same migration that references the rebuilt one", %{ctx: ctx} do + defres Parent, "parents" do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + defdomain([Parent]) + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO parents (id, name) VALUES ('p', 'n')") + + defres Parent, "parents" do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + defres Child, "children" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:parent, Parent, allow_nil?: false) + end + + sqlite do + references do + reference(:parent, on_delete: :delete) + end + end + end + + defdomain([Parent, Child]) + generate(Domain, ctx) + migrate(ctx) + + sql("INSERT INTO children (id, parent_id) VALUES ('c', 'p')") + assert fk_violations() == [] + sql("DELETE FROM parents") + assert sql("SELECT id FROM children") == [] + end + end + + describe "deferrable foreign keys" do + test "a deferrable reference next to a rebuild says it is not applied", %{ctx: ctx} do + defres Parent, "parents" do + attributes do + uuid_primary_key(:id) + end + end + + defres Child, "children" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:parent, Parent) + end + end + + defdomain([Parent, Child]) + generate(Domain, ctx) + migrate(ctx) + + defres Child, "children" do + attributes do + uuid_primary_key(:id) + attribute(:note, :string, allow_nil?: false) + end + + relationships do + belongs_to(:parent, Parent) + end + + sqlite do + references do + reference(:parent, deferrable: :initially) + end + end + end + + generate(Domain, ctx) + migration = last_migration(ctx) + assert migration =~ "NOT APPLIED: the foreign key children_parent_id_fkey is deferrable" + migrate(ctx) + assert fk_violations() == [] + end + end + + # Two versions of the resource on "items": `name` required, then optional. + defmacrop items_v1 do + quote do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + end + end + + defmacrop items_v2 do + quote do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + end + end + + describe "how the migration is run" do + test "rolling back the rebuild with data that fits", %{ctx: ctx} do + items_v1() + generate(Domain, ctx) + migrate(ctx) + + items_v2() + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name) VALUES ('a', 'x')") + rollback(ctx) + + assert [%{name: "name", notnull: true}] = + Enum.filter(columns("items"), &(&1.name == "name")) + + assert sql("SELECT id, name FROM items") == [["a", "x"]] + assert versions() |> length() == 1 + end + end + + defp assert_raise_on_rollback(ctx, message) do + error = + try do + rollback(ctx) + flunk("expected the rollback to fail") + rescue + error in ExUnit.AssertionError -> reraise error, __STACKTRACE__ + error -> Exception.message(error) + end + + assert error =~ message + end + + describe "the generator" do + test "generating again after a rebuild migration finds nothing to do (codegen --check)", %{ + ctx: ctx + } do + items_v1() + generate(Domain, ctx) + migrate(ctx) + items_v2() + generate(Domain, ctx) + migrate(ctx) + + files = migrations(ctx) + assert generate(Domain, ctx) == files + assert generate(Domain, ctx, check: true) == files + end + + test "the formatted migration is valid and runs", %{ctx: ctx} do + items_v1() + generate(Domain, ctx, format: true) + migrate(ctx) + sql("INSERT INTO items (id, name) VALUES ('a', 'x')") + items_v2() + generate(Domain, ctx, format: true) + migrate(ctx) + assert sql("SELECT id, name FROM items") == [["a", "x"]] + end + + test "a column removed from the resource is kept (nullable) by the rebuild", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + attribute(:legacy, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name, legacy) VALUES ('a', 'x', 'old data')") + + items_v2() + generate(Domain, ctx) + migrate(ctx) + + assert sql("SELECT id, name, legacy FROM items") == [["a", "x", "old data"]] + assert [%{notnull: false}] = Enum.filter(columns("items"), &(&1.name == "legacy")) + + # a row written after: the resource no longer knows `legacy`, so it is NULL + sql("INSERT INTO items (id, name) VALUES ('b', 'y')") + # going back would need a value for the required column `legacy` + assert_raise_on_rollback(ctx, "NOT NULL constraint failed") + assert length(sql("SELECT id FROM items")) == 2 + end + end + + defp index_sql(table) do + sql( + "SELECT name, sql FROM sqlite_master WHERE type = 'index' AND tbl_name = ? AND sql IS NOT NULL ORDER BY name", + [ + table + ] + ) + end + + defmacrop indexed_items(nullable?) do + quote do + defitem do + sqlite do + custom_indexes do + index([:name], where: "name IS NOT NULL", name: "items_partial_index") + end + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: unquote(!nullable?), public?: true) + attribute(:code, :string, public?: true) + end + + identities do + identity(:unique_code, [:code]) + identity(:unique_live_name, [:name], where: expr(not is_nil(^ref(:code)))) + end + end + end + end + + defmacrop retyped_items(type, strict?) do + quote do + defitem do + sqlite do + strict?(unquote(strict?)) + end + + attributes do + uuid_primary_key(:id) + attribute(:amount, unquote(type)) + end + end + end + end + + describe "resource shapes" do + test "a composite primary key", %{ctx: ctx} do + defitem do + attributes do + attribute(:org, :string, primary_key?: true, allow_nil?: false) + attribute(:code, :string, primary_key?: true, allow_nil?: false) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (org, code, name) VALUES ('o', 'c', 'x')") + + defitem do + attributes do + attribute(:org, :string, primary_key?: true, allow_nil?: false) + attribute(:code, :string, primary_key?: true, allow_nil?: false) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + + assert sql("SELECT org, code, name FROM items") == [["o", "c", "x"]] + assert Enum.map(columns("items"), & &1.pk) |> Enum.sort() == [0, 1, 2] + + assert_raise Exqlite.Error, ~r/UNIQUE constraint failed/, fn -> + sql("INSERT INTO items (org, code, name) VALUES ('o', 'c', 'dup')") + end + end + + test "a primary key that changes is listed as such", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:code, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, code) VALUES ('a', 'c')") + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:code, :string, primary_key?: true, allow_nil?: false) + end + end + + generate(Domain, ctx) + assert last_migration(ctx) =~ "# - changing the primary key" + refute last_migration(ctx) =~ "Operation" + + migrate(ctx) + assert Enum.map(columns("items"), & &1.pk) |> Enum.sort() == [1, 2] + end + + test "identities with a where, custom indexes with where", + %{ctx: ctx} do + indexed_items(false) + generate(Domain, ctx) + migrate(ctx) + before = index_sql("items") + assert length(before) == 3 + + indexed_items(true) + generate(Domain, ctx) + migrate(ctx) + + assert index_sql("items") == before + end + + test "a table named like the temporary one is refused loudly, not overwritten", %{ctx: ctx} do + defres Other, "items_rebuild" do + attributes do + uuid_primary_key(:id) + end + end + + items_v1() + defdomain([Item, Other]) + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items_rebuild (id) VALUES ('keep me')") + + items_v2() + defdomain([Item, Other]) + generate(Domain, ctx) + assert migrate_error(ctx) =~ "already exists" + assert sql("SELECT id FROM items_rebuild") == [["keep me"]] + end + end + + describe "changing a column's type" do + defp retype(ctx, strict?) do + retyped_items(:string, strict?) + + generate(__MODULE__.Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, amount) VALUES ('a', '12'), ('b', 'twelve')") + + retyped_items(:integer, strict?) + + generate(__MODULE__.Domain, ctx) + end + + test "in a STRICT table a value that is not a number is loud", %{ctx: ctx} do + retype(ctx, true) + refute last_migration(ctx) =~ "not STRICT" + assert migrate_error(ctx) =~ "cannot store TEXT value in INTEGER column" + assert sql("SELECT id, amount FROM items ORDER BY id") == [["a", "12"], ["b", "twelve"]] + end + + test "in a table that is not STRICT it is copied as it is, text in an integer column", %{ + ctx: ctx + } do + retype(ctx, false) + assert last_migration(ctx) =~ "REVIEW: `amount` changes type in a table that is not STRICT" + migrate(ctx) + + assert sql("SELECT id, amount, typeof(amount) FROM items ORDER BY id") == [ + ["a", 12, "integer"], + ["b", "twelve", "text"] + ] + end + end + + describe "dropping columns" do + test "--drop-columns drops the column instead of keeping it", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + attribute(:legacy, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name, legacy) VALUES ('a', 'x', 'old')") + + items_v2() + generate(Domain, ctx, drop_columns: true) + refute last_migration(ctx) =~ "no longer in the resource" + migrate(ctx) + + assert Enum.map(columns("items"), & &1.name) == ["id", "name"] + end + end + + defmacrop filtered_items(nullable?) do + quote do + defitem do + sqlite do + base_filter_sql("name IS NOT NULL") + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: unquote(!nullable?), public?: true) + end + + identities do + identity(:unique_name, [:name]) + end + end + end + end + + describe "keys and filters" do + test "a resource's base filter on its identities", %{ctx: ctx} do + filtered_items(false) + generate(Domain, ctx) + migrate(ctx) + before = index_sql("items") + assert [[_, index_sql]] = before + assert index_sql =~ "WHERE" + + filtered_items(true) + generate(Domain, ctx) + migrate(ctx) + assert index_sql("items") == before + end + end + + describe "opting in through the repo's config" do + @tag repo_config: [rebuild_tables: true] + test "the repo's config turns it on for every run", %{ctx: ctx} do + domain = make_name_optional_from_required(ctx) + + generate(domain, ctx, rebuild_tables: nil) + + assert last_migration(ctx) =~ "rebuild_table :items" + end + + @tag repo_config: [rebuild_tables: true] + test "--no-rebuild-tables turns it off for one run", %{ctx: ctx} do + domain = make_name_optional_from_required(ctx) + + generate(domain, ctx, rebuild_tables: false) + + migration = last_migration(ctx) + refute migration =~ "rebuild_table" + assert migration =~ "modify :name, :text, null: true" + end + + defp make_name_optional_from_required(ctx) do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx, rebuild_tables: nil) + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + # the alias made by `defdomain` only lives in this function + Domain + end + end +end From 622458cafa107547a9a6d077bbb0d26903923bab Mon Sep 17 00:00:00 2001 From: Nassim Date: Fri, 9 Oct 2026 04:48:54 +0200 Subject: [PATCH 03/15] improvement: keep the custom statements of a table when it is rebuilt --- .../development/migrations-and-tasks.md | 68 +++++++++++++++++++ lib/migration_generator/operation.ex | 23 ++++++- test/table_rebuild_test.exs | 41 +++++++++++ 3 files changed, 130 insertions(+), 2 deletions(-) diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index 15dde46..6c44fdc 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -58,6 +58,74 @@ end The rebuild runs in one transaction with foreign keys off, and changes nothing if it fails. +The custom statements of the resource are dropped (their `down`) before the rebuild and recreated (their `up`) after it, so what they hold is lost. To keep it, add the copy to the generated migration, around the statement's `execute` lines. For a statement that creates a table: + +```elixir +custom_statements do + statement :items_history do + up "CREATE TABLE items_history (item_id TEXT)" + down "DROP TABLE items_history" + end +end +``` + +the generated migration drops and recreates `items_history` around the rebuild of `items`. The two lines marked `# added` keep the rows: + +```elixir +def up do + # SQLite cannot change `items` in place, so it is rebuilt ... Changes: + # - modify :name, :text, null: true + # - REVIEW: the custom statements :items_history are dropped (their `down`) before and ... + execute("CREATE TABLE items_history_backup AS SELECT * FROM items_history") # added + + execute(""" + DROP TABLE items_history + """) + + rebuild_table :items do + add :id, :uuid, null: false, primary_key: true + add :name, :text + end + + execute(""" + CREATE TABLE items_history (item_id TEXT) + """) + + execute("INSERT INTO items_history SELECT * FROM items_history_backup") # added + execute("DROP TABLE items_history_backup") # added +end +``` + +A full-text index needs no backup: it is rebuilt from its table by a command after the statement's `up`. + +```elixir +statement :items_fts do + up "CREATE VIRTUAL TABLE items_fts USING fts5(name, content='items', content_rowid='rowid')" + down "DROP TABLE items_fts" +end +``` + +```elixir +def up do + # SQLite cannot change `items` in place, so it is rebuilt ... Changes: + # ... + execute(""" + DROP TABLE items_fts + """) + + rebuild_table :items do + add :id, :uuid, null: false, primary_key: true + add :name, :text + end + + execute(""" + CREATE VIRTUAL TABLE items_fts USING fts5(name, content='items', content_rowid='rowid') + """) + + execute("INSERT INTO items_fts(items_fts) VALUES('rebuild')") # added +end +``` + ### Regenerating Migrations Often, you will run into a situation where you want to make a slight change to a resource after you've already generated and run migrations. If you are using git and would like to undo those changes, then regenerate the migrations, this script may prove useful: diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 025eab0..7c8b943 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -853,7 +853,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do to_columns: new.attributes ++ Enum.map(leftover, &as_nullable/1), renames: renames, reasons: reasons(changes), - notes: type_notes(old, new, changes) ++ leftover_notes(leftover) + notes: type_notes(old, new, changes) ++ leftover_notes(leftover) ++ statement_notes(old) }) end @@ -869,7 +869,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do to_columns: old.attributes, renames: Map.new(up_renames, fn {to, from} -> {from, to} end), reasons: ["undoing the rebuild above (rows in columns it added are not kept)"], - notes: [] + notes: statement_notes(new) }) end @@ -898,6 +898,9 @@ defmodule AshSqlite.MigrationGenerator.Operation do [ header(table, lines), + Enum.map(plan.from.custom_statements, fn statement -> + Operation.AddCustomStatement.down(%{statement: statement, table: table}) + end), "rebuild_table :#{as_atom(table)}#{options} do\n#{adds}\nend", Enum.map(plan.to.identities, fn identity -> Operation.AddUniqueIndex.up(%{ @@ -913,6 +916,9 @@ defmodule AshSqlite.MigrationGenerator.Operation do base_filter: plan.to.base_filter, multitenancy: plan.to.multitenancy }) + end), + Enum.map(plan.to.custom_statements, fn statement -> + Operation.AddCustomStatement.up(%{statement: statement, table: table}) end) ] |> List.flatten() @@ -1013,6 +1019,19 @@ defmodule AshSqlite.MigrationGenerator.Operation do end end + defp statement_notes(%{custom_statements: []}), do: [] + + defp statement_notes(%{custom_statements: statements}) do + names = Enum.map_join(statements, ", ", &inspect(&1.name)) + + [ + "REVIEW: the custom statements #{names} are dropped (their `down`) before and " <> + "recreated (their `up`) after the rebuild.\n" <> + "What they hold, such as the rows of a table or the contents of a full-text index, " <> + "is not kept: to keep it, copy it before and restore it after, in this migration" + ] + end + defp leftover_notes([]), do: [] defp leftover_notes(leftover) do diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index 623f61d..ff38931 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -756,6 +756,47 @@ defmodule AshSqlite.TableRebuildTest do end end + defmacrop trigger_items(nullable?) do + quote do + defitem do + sqlite do + custom_statements do + statement :items_audit do + up "CREATE TRIGGER items_audit AFTER INSERT ON items BEGIN INSERT INTO audit VALUES (new.id); END" + down "DROP TRIGGER items_audit" + end + end + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: unquote(!nullable?)) + end + end + end + end + + describe "custom statements on the rebuilt table" do + test "a trigger is dropped before the rebuild, recreated after, and still fires", %{ctx: ctx} do + sql("CREATE TABLE IF NOT EXISTS audit (msg TEXT)") + trigger_items(false) + generate(Domain, ctx) + migrate(ctx) + sql("CREATE TABLE IF NOT EXISTS audit (msg TEXT)") + sql("INSERT INTO items (id, name) VALUES ('a', 'x')") + assert sql("SELECT msg FROM audit") == [["a"]] + + trigger_items(true) + generate(Domain, ctx) + assert last_migration(ctx) =~ "REVIEW: the custom statements :items_audit" + migrate(ctx) + + sql("INSERT INTO items (id, name) VALUES ('b', 'y')") + assert sql("SELECT msg FROM audit ORDER BY msg") == [["a"], ["b"]] + assert [["items_audit"]] = sql("SELECT name FROM sqlite_master WHERE type = 'trigger'") + end + end + describe "dropping columns" do test "--drop-columns drops the column instead of keeping it", %{ctx: ctx} do defitem do From 2d329a33e8227c595b56867f4ee7fc93552ea475 Mon Sep 17 00:00:00 2001 From: Nassim Date: Fri, 9 Oct 2026 04:49:22 +0200 Subject: [PATCH 04/15] improvement: hint in a rebuild comment where a decision is needed --- .../development/migrations-and-tasks.md | 2 +- lib/migration_generator/operation.ex | 58 ++++++++++++- test/table_rebuild_test.exs | 82 +++++++++++++++++++ 3 files changed, 140 insertions(+), 2 deletions(-) diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index 6c44fdc..e129d54 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -56,7 +56,7 @@ def up do end ``` -The rebuild runs in one transaction with foreign keys off, and changes nothing if it fails. +The rebuild runs in one transaction with foreign keys off, and changes nothing if it fails. Columns both tables have are copied as they are. When the right value for the rows depends on your data, as when a column becomes required and has no default, a `REVIEW` comment says so and shows how to give them one. The custom statements of the resource are dropped (their `down`) before the rebuild and recreated (their `up`) after it, so what they hold is lost. To keep it, add the copy to the generated migration, around the statement's `execute` lines. For a statement that creates a table: diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 7c8b943..18bafff 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -852,6 +852,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do to: new, to_columns: new.attributes ++ Enum.map(leftover, &as_nullable/1), renames: renames, + direction: :up, reasons: reasons(changes), notes: type_notes(old, new, changes) ++ leftover_notes(leftover) ++ statement_notes(old) }) @@ -868,6 +869,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do to: old, to_columns: old.attributes, renames: Map.new(up_renames, fn {to, from} -> {from, to} end), + direction: :down, reasons: ["undoing the rebuild above (rows in columns it added are not kept)"], notes: statement_notes(new) }) @@ -876,7 +878,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do defp render(%{table: table} = plan) do columns = plan.to_columns - lines = plan.reasons ++ plan.notes + lines = plan.reasons ++ plan.notes ++ review_notes(columns, plan) options = [ @@ -984,6 +986,60 @@ defmodule AshSqlite.MigrationGenerator.Operation do defp backfill(_from, _to, _expression), do: nil + defp review_notes(_columns, %{direction: :down}), do: [] + + defp review_notes(columns, plan) do + for column <- columns, + note = review_note(column, plan) do + note + end + end + + defp review_note(%{allow_nil?: false} = column, plan) do + from_name = Map.get(plan.renames, column.source, column.source) + + case Enum.find(plan.from_columns, &(&1.source == from_name)) do + nil -> + if unfilled?(column), + do: review_new_column(plan.table, column.source) + + %{allow_nil?: true} -> + if default_sql(column.default) == nil, + do: review_required_column(plan.table, column.source, from_name) + + _ -> + nil + end + end + + defp review_note(_column, _plan), do: nil + + defp unfilled?(attribute), do: attribute.default == "nil" and !attribute.generated? + + defp review_required_column(table, name, from) do + replaces = if from != name, do: " (in place of its `#{atom_key(name)} #{inspect(from)}`)" + + """ + REVIEW: #{inspect(name)} becomes required and has no default, so a row without a value for it + stops this migration (nothing is changed). Give those rows one with the `copy:` option + of `rebuild_table`#{replaces}, for example + rebuild_table :#{as_atom(table)}, copy: [#{atom_key(name)} "COALESCE(#{from}, 'your value')"] do + To give new rows one too, add `migration_defaults(#{atom_key(name)} "\\"your value\\"")` to the + resource's `sqlite` block and regenerate.\ + """ + end + + defp review_new_column(table, name) do + """ + REVIEW: #{inspect(name)} is new, required and has no default, so if the table has rows this + migration stops (nothing is changed). Give the existing rows a value with the `copy:` + option of `rebuild_table`, for example + rebuild_table :#{as_atom(table)}, copy: [#{atom_key(name)} "'your value'"] do + or add `migration_defaults(#{atom_key(name)} "\\"your value\\"")` to the resource's `sqlite` + block and regenerate.\ + """ + end + defp renames(changes) do for %Operation.RenameAttribute{old_attribute: old, new_attribute: new} <- changes, into: %{}, diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index ff38931..5ea4217 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -897,4 +897,86 @@ defmodule AshSqlite.TableRebuildTest do Domain end end + + describe "the copy" do + defp edit_last_migration(ctx, from, to) do + file = ctx |> migrations() |> List.last() + contents = File.read!(file) + assert contents =~ from + File.write!(file, String.replace(contents, from, to, global: false)) + end + + test "a required column without a default: the comment's remedy gives the rows a value", + %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name) VALUES ('a', 'x'), ('b', NULL)") + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + assert last_migration(ctx) =~ "REVIEW: :name becomes required and has no default" + assert last_migration(ctx) =~ ~S|copy: [name: "COALESCE(name, 'your value')"]| + + # as it is, it stops, as the comment says + assert migrate_error(ctx) =~ "NOT NULL constraint failed" + + edit_last_migration( + ctx, + "rebuild_table :items do", + ~S|rebuild_table :items, copy: [name: "COALESCE(name, 'unnamed')"] do| + ) + + migrate(ctx) + assert sql("SELECT id, name FROM items ORDER BY id") == [["a", "x"], ["b", "unnamed"]] + end + + test "a new required column: the comment's remedy gives the existing rows a value", %{ + ctx: ctx + } do + defitem do + attributes do + uuid_primary_key(:id) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id) VALUES ('a')") + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:code, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + assert last_migration(ctx) =~ "REVIEW: :code is new, required and has no default" + assert last_migration(ctx) =~ ~S|copy: [code: "'your value'"]| + + assert migrate_error(ctx) =~ "NOT NULL constraint failed" + + edit_last_migration( + ctx, + "rebuild_table :items do", + ~S|rebuild_table :items, copy: [code: "'none'"] do| + ) + + migrate(ctx) + assert sql("SELECT id, code FROM items") == [["a", "none"]] + end + end end From 627d1b5cc5308583631107977f06832462491041 Mon Sep 17 00:00:00 2001 From: Nassim Date: Thu, 8 Oct 2026 22:17:55 +0200 Subject: [PATCH 05/15] improvement: rebuild a table while views and triggers mention it --- lib/migration.ex | 22 ++++++++++++++++++---- test/table_rebuild_test.exs | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/lib/migration.ex b/lib/migration.ex index 7482448..e17cd42 100644 --- a/lib/migration.ex +++ b/lib/migration.ex @@ -41,18 +41,23 @@ defmodule AshSqlite.Migration do end # `PRAGMA foreign_keys` does nothing inside a transaction, so it is set before Ecto opens one, - # on the connection it will use. The previous value is kept to put it back afterwards. + # on the connection it will use. The previous values are kept to put them back afterwards. @doc false def __before_transaction__(repo) do - Process.put(@pragmas_key, pragma(repo, "foreign_keys")) + Process.put(@pragmas_key, {pragma(repo, "foreign_keys"), pragma(repo, "legacy_alter_table")}) quiet!(repo, "PRAGMA foreign_keys = OFF") end + # `legacy_alter_table` is put back too: a rebuild that fails does not turn it off itself. @doc false def __after_transaction__(repo) do case Process.delete(@pragmas_key) do - nil -> :ok - foreign_keys -> quiet!(repo, "PRAGMA foreign_keys = #{foreign_keys}") + nil -> + :ok + + {foreign_keys, legacy_alter_table} -> + quiet!(repo, "PRAGMA legacy_alter_table = #{legacy_alter_table}") + quiet!(repo, "PRAGMA foreign_keys = #{foreign_keys}") end end @@ -70,6 +75,10 @@ defmodule AshSqlite.Migration do end end + @doc false + def __legacy_alter_table__(repo, value), + do: quiet!(repo, "PRAGMA legacy_alter_table = #{value}") + @doc """ Rebuilds `table` with the shape `block` describes, as the block of a `create table`. @@ -101,8 +110,13 @@ defmodule AshSqlite.Migration do # the columns of both tables are only known once the new one exists execute(fn -> AshSqlite.Migration.__copy_rows__(repo(), table, temporary, copy) end) + # While the old table is dropped, a view or trigger that mentions it points at nothing, and + # the rename that follows refuses. The legacy behaviour leaves them alone. It is not on for + # the rest of the migration, so that a `rename table` in it still updates what mentions it. + execute(fn -> AshSqlite.Migration.__legacy_alter_table__(repo(), 1) end) drop(table(table)) rename(table(temporary), to: table(table)) + execute(fn -> AshSqlite.Migration.__legacy_alter_table__(repo(), 0) end) end end diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index 5ea4217..91cdf46 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -278,6 +278,43 @@ defmodule AshSqlite.TableRebuildTest do end end + describe "things in the database that refer to the rebuilt table" do + test "a view and a trigger on another table that mention it survive", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("CREATE TABLE audit (msg TEXT)") + sql("CREATE TABLE other (id TEXT)") + sql("CREATE VIEW item_names AS SELECT name FROM items") + + sql( + "CREATE TRIGGER other_audit AFTER INSERT ON other BEGIN INSERT INTO audit SELECT name FROM items; END" + ) + + sql("INSERT INTO items (id, name) VALUES ('a', 'x')") + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + + assert sql("SELECT name FROM item_names") == [["x"]] + sql("INSERT INTO other (id) VALUES ('o')") + assert sql("SELECT msg FROM audit") == [["x"]] + end + end + describe "foreign key topology" do test "a self-referencing table", %{ctx: ctx} do defres Node, "nodes" do From ed86b8449ce10a8e22a434a6b27d5440cb61b589 Mon Sep 17 00:00:00 2001 From: Nassim Date: Thu, 8 Oct 2026 22:17:55 +0200 Subject: [PATCH 06/15] improvement: keep the AUTOINCREMENT sequence when a table is rebuilt --- lib/migration.ex | 31 +++++++++++++++++--- test/migration_test.exs | 57 +++++++++++++++++++++++++++++++++++++ test/table_rebuild_test.exs | 29 +++++++++++++++++++ 3 files changed, 113 insertions(+), 4 deletions(-) diff --git a/lib/migration.ex b/lib/migration.ex index e17cd42..a48c3a4 100644 --- a/lib/migration.ex +++ b/lib/migration.ex @@ -98,12 +98,12 @@ defmodule AshSqlite.Migration do defmacro rebuild_table(table, opts \\ [], do: block) do quote do table = unquote(table) - {copy, options} = Keyword.pop(unquote(opts), :copy, []) + {copy, opts} = Keyword.pop(unquote(opts), :copy, []) temporary = "#{table}_rebuild" execute(fn -> AshSqlite.Migration.__check_foreign_keys_off__(repo(), table) end) - create table(temporary, [primary_key: false] ++ options) do + create table(temporary, [primary_key: false] ++ opts) do unquote(block) end @@ -122,6 +122,8 @@ defmodule AshSqlite.Migration do @doc false def __copy_rows__(repo, table, temporary, copy) do + copy_sequence(repo, table, temporary) + old_columns = column_names(repo, table) new_columns = column_names(repo, temporary) @@ -154,6 +156,25 @@ defmodule AshSqlite.Migration do ) end + # An AUTOINCREMENT table keeps the highest id it ever handed out in `sqlite_sequence`, which + # dropping the table deletes: ids that were used and deleted would be handed out again. + defp copy_sequence(repo, table, temporary) do + %{rows: [[autoincrement]]} = + quiet!( + repo, + "SELECT count(*) FROM sqlite_master WHERE name = ? AND sql GLOB '*PRIMARY KEY AUTOINCREMENT*'", + [temporary] + ) + + if autoincrement > 0 do + query!( + repo, + "INSERT INTO sqlite_sequence (name, seq) SELECT ?, seq FROM sqlite_sequence WHERE name = ?", + [temporary, to_string(table)] + ) + end + end + defp copy_expression(copy, name, old_columns) do case Enum.find(copy, fn {new, _} -> to_string(new) == name end) do {_, from} when is_atom(from) -> quote_name(from) @@ -171,10 +192,12 @@ defmodule AshSqlite.Migration do # not `repo.query!/1`: ecto_libsql's repo has none # sobelow_skip ["SQL.Query"] - defp query!(repo, statement), do: Ecto.Adapters.SQL.query!(repo, statement) + defp query!(repo, statement, params \\ []), + do: Ecto.Adapters.SQL.query!(repo, statement, params) # sobelow_skip ["SQL.Query"] - defp quiet!(repo, statement), do: Ecto.Adapters.SQL.query!(repo, statement, [], log: false) + defp quiet!(repo, statement, params \\ []), + do: Ecto.Adapters.SQL.query!(repo, statement, params, log: false) defp pragma(repo, name) do %{rows: [[value]]} = quiet!(repo, "PRAGMA #{name}") diff --git a/test/migration_test.exs b/test/migration_test.exs index 8a80ed2..1c31f1e 100644 --- a/test/migration_test.exs +++ b/test/migration_test.exs @@ -207,6 +207,36 @@ defmodule AshSqlite.MigrationTest do assert sql("SELECT id FROM comments") == [["c"]] end + test "a table that stops being AUTOINCREMENT and becomes one again has one row", %{ctx: ctx} do + write_migration(ctx, 1, "create_things", """ + def up do + execute "CREATE TABLE things (id INTEGER PRIMARY KEY AUTOINCREMENT)" + execute "INSERT INTO things DEFAULT VALUES" + execute "INSERT INTO things DEFAULT VALUES" + end + + def down, do: :ok + """) + + write_migration(ctx, 2, "there_and_back", """ + def up do + rebuild_table :things do + add :id, :integer, primary_key: true + end + + rebuild_table :things do + add :id, :bigserial, primary_key: true + end + end + + def down, do: :ok + """) + + migrate(ctx) + + assert [["things", 2]] = sql("SELECT name, seq FROM sqlite_sequence") + end + describe "with ecto_libsql" do @describetag repo: AshSqlite.RebuildLibSqlRepo @@ -233,5 +263,32 @@ defmodule AshSqlite.MigrationTest do assert sql("SELECT id FROM comments") == [["c"]] assert sql("PRAGMA foreign_keys") == [[1]] end + + @tag repo_config: [pool_size: 1] + test "a rebuild works when the database has an AUTOINCREMENT table", %{ctx: ctx} do + write_migration(ctx, 1, "create_tables", """ + def up do + execute "CREATE TABLE counters (id INTEGER PRIMARY KEY AUTOINCREMENT)" + execute "CREATE TABLE plain (id INTEGER PRIMARY KEY, name TEXT NOT NULL)" + end + + def down, do: :ok + """) + + write_migration(ctx, 2, "name_optional", """ + def up do + rebuild_table :plain do + add :id, :integer, primary_key: true + add :name, :text + end + end + + def down, do: :ok + """) + + migrate(ctx) + + assert versions() == [1, 2] + end end end diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index 91cdf46..520174d 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -315,6 +315,35 @@ defmodule AshSqlite.TableRebuildTest do end end + describe "integer primary keys with AUTOINCREMENT" do + test "ids that were used and deleted are not handed out again", %{ctx: ctx} do + defitem do + attributes do + integer_primary_key(:id) + attribute(:name, :string, allow_nil?: false) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (name) VALUES ('a'), ('b'), ('c')") + sql("DELETE FROM items WHERE id = 3") + + defitem do + attributes do + integer_primary_key(:id) + attribute(:name, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + + sql("INSERT INTO items (name) VALUES ('d')") + assert sql("SELECT id, name FROM items ORDER BY id") == [[1, "a"], [2, "b"], [4, "d"]] + end + end + describe "foreign key topology" do test "a self-referencing table", %{ctx: ctx} do defres Node, "nodes" do From 00adad49b33d1e4c288366cbd06719c7b9ff8c10 Mon Sep 17 00:00:00 2001 From: Nassim Date: Thu, 8 Oct 2026 22:17:55 +0200 Subject: [PATCH 07/15] improvement: roll back dev migrations through Ecto.Migrator --- .../migration_generator.ex | 42 ++++--------------- test/table_rebuild_test.exs | 29 +++++++++++++ 2 files changed, 36 insertions(+), 35 deletions(-) diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index 9e357e9..acf0a85 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -405,41 +405,13 @@ defmodule AshSqlite.MigrationGenerator do end) with_repo_not_in_test(repo, fn repo -> - {repo, query, opts} = Ecto.Migration.SchemaMigration.versions(repo, [], nil) - - repo.transaction(fn -> - Ecto.Migration.SchemaMigration.ensure_schema_migrations_table!( - repo, - repo.config(), - [] - ) - - versions = repo.all(query, opts) - - dev_migrations - |> Enum.map(&extract_migration_info/1) - |> Enum.filter(& &1) - |> Enum.map(&load_migration!/1) - |> Enum.sort() - |> Enum.reverse() - |> Enum.filter(fn {version, _} -> - version in versions - end) - |> Enum.each(fn {version, mod} -> - Ecto.Migration.Runner.run( - repo, - [], - version, - mod, - :forward, - :down, - :down, - all: true - ) - - Ecto.Migration.SchemaMigration.down(repo, repo.config(), version, []) - end) - end) + dev_migrations + |> Enum.map(&extract_migration_info/1) + |> Enum.filter(& &1) + |> Enum.map(&load_migration!/1) + |> Enum.sort() + |> Enum.reverse() + |> Enum.each(fn {version, mod} -> Ecto.Migrator.down(repo, version, mod) end) end) # Remove dev migration files diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index 520174d..41ad7e8 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -863,6 +863,35 @@ defmodule AshSqlite.TableRebuildTest do end end + describe "dev migrations" do + test "dev migrations that rebuild can be replaced by the real migration", %{ctx: ctx} do + items_v1() + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, name) VALUES ('a', 'x')") + + items_v2() + generate(Domain, ctx, dev: true) + migrate(ctx) + assert [_, dev] = migrations_with_dev(ctx) + assert dev =~ "_dev.exs" + + # `ash.codegen name` rolls the dev migrations back and writes the real one + generate(Domain, ctx, name: "make_name_optional") + + assert [_, real] = migrations_with_dev(ctx) + refute real =~ "_dev.exs" + assert File.read!(real) =~ "use AshSqlite.Migration" + # the dev migration was rolled back: the column is required again until the real one runs + assert [%{notnull: true}] = Enum.filter(columns("items"), &(&1.name == "name")) + migrate(ctx) + assert [%{notnull: false}] = Enum.filter(columns("items"), &(&1.name == "name")) + assert sql("SELECT id, name FROM items") == [["a", "x"]] + end + end + + defp migrations_with_dev(ctx), do: Enum.sort(Path.wildcard("#{ctx.migration_path}/**/*.exs")) + describe "dropping columns" do test "--drop-columns drops the column instead of keeping it", %{ctx: ctx} do defitem do From d30d6827b8a3da45aa32292e19ea5af553d77a7f Mon Sep 17 00:00:00 2001 From: Nassim Date: Thu, 8 Oct 2026 22:17:55 +0200 Subject: [PATCH 08/15] improvement: also change an attribute that is renamed and changed --- .../development/migrations-and-tasks.md | 2 +- .../migration_generator.ex | 101 ++++++++++++++---- lib/migration_generator/operation.ex | 14 +++ .../tasks/ash_sqlite.generate_migrations.ex | 5 +- test/migration_generator_test.exs | 71 ++++++++++++ test/table_rebuild_test.exs | 65 +++++++++++ 6 files changed, 232 insertions(+), 26 deletions(-) diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index e129d54..6f79e8c 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -41,7 +41,7 @@ For more information on generating migrations, run `mix help ash_sqlite.generate SQLite can mostly add, rename and drop a column in place. Changing a column's type or default, adding or dropping a foreign key, changing a primary key and, as the adapters write it, making a column required or optional all need the table to be created again in its new shape, with the rows copied over. -By default the generator writes a statement that fails when the migration runs. With `--rebuild-tables`, or `rebuild_tables: true` in the repo's config, it writes a `rebuild_table` instead, which shows the table's new shape and what triggered it: +By default the generator writes a statement that fails when the migration runs, or leaves the change out (a renamed attribute that also changes). With `--rebuild-tables`, or `rebuild_tables: true` in the repo's config, it writes a `rebuild_table` instead, which shows the table's new shape and what triggered it: ```elixir def up do diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index acf0a85..ce9c7ef 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -1697,6 +1697,8 @@ defmodule AshSqlite.MigrationGenerator do defp rebuild_if_needed(operations, snapshot, old_snapshot, opts) do if !old_snapshot[:empty?] and Enum.any?(operations, &Operation.RebuildTable.requires_rebuild?/1) do + {omitted, operations} = Enum.split_with(operations, &match?(%Operation.Omitted{}, &1)) + if rebuild_tables?(opts, snapshot.repo) do [ %Operation.RebuildTable{ @@ -1707,7 +1709,13 @@ defmodule AshSqlite.MigrationGenerator do } ] else - hint_rebuild(operations, snapshot, opts) + hint_rebuild( + operations, + Enum.map(omitted, &Operation.Omitted.reason(&1, snapshot.multitenancy)), + snapshot, + opts + ) + operations end else @@ -1715,18 +1723,17 @@ defmodule AshSqlite.MigrationGenerator do end end - defp hint_rebuild(operations, snapshot, opts) do + defp hint_rebuild(operations, omitted_reasons, snapshot, opts) do unless opts.quiet do - bullets = - Enum.map_join( - Operation.RebuildTable.blocking_reasons(operations), - "\n", - fn {reason, blocks} -> " - #{reason} (#{blocked_when(blocks)})" end - ) + fails = + for {reason, blocks} <- Operation.RebuildTable.blocking_reasons(operations), + do: " - #{reason} (#{blocked_when(blocks)})" + + omitted_lines = for reason <- omitted_reasons, do: " - #{reason} (not in the migration)" warn(""" SQLite cannot make these changes to `#{snapshot.table}` in place: - #{bullets} + #{Enum.join(fails ++ omitted_lines, "\n")} To rebuild the table instead, #{rebuild_advice(opts)} """) end @@ -1829,18 +1836,48 @@ defmodule AshSqlite.MigrationGenerator do {attributes_to_add, attributes_to_remove, attributes_to_rename} = resolve_renames(snapshot.table, attributes_to_add, attributes_to_remove, opts) + old_names = Map.new(attributes_to_rename, fn {new, old} -> {new.source, old.source} end) + attributes_to_alter = snapshot.attributes |> Enum.map(fn attribute -> - {attribute, - Enum.find( - old_snapshot.attributes, - &(&1.source == attribute.source && - attributes_unequal?(&1, attribute, snapshot.repo, old_snapshot, snapshot)) - )} + old_name = Map.get(old_names, attribute.source, attribute.source) + old_attribute = Enum.find(old_snapshot.attributes, &(&1.source == old_name)) + + changed? = + old_attribute && + attributes_unequal?( + old_attribute, + attribute, + snapshot.repo, + old_snapshot, + snapshot, + old_name != attribute.source + ) + + {attribute, changed? && old_attribute} end) |> Enum.filter(&elem(&1, 1)) + # Only a rebuild can make the change of a renamed attribute, SQLite refuses the statement. + {renamed_and_changed, attributes_to_alter} = + if rebuild_tables?(opts, snapshot.repo) do + {[], attributes_to_alter} + else + Enum.split_with(attributes_to_alter, fn {new, old} -> new.source != old.source end) + end + + omitted = + for {new, old} <- renamed_and_changed, + do: %Operation.Omitted{ + table: snapshot.table, + operation: %Operation.AlterAttribute{ + table: snapshot.table, + new_attribute: new, + old_attribute: old + } + } + rename_attribute_events = Enum.map(attributes_to_rename, fn {new, old} -> %Operation.RenameAttribute{ @@ -1962,7 +1999,7 @@ defmodule AshSqlite.MigrationGenerator do end) add_attribute_events ++ - alter_attribute_events ++ remove_attribute_events ++ rename_attribute_events + alter_attribute_events ++ remove_attribute_events ++ rename_attribute_events ++ omitted end defp differently_deferrable?(%{references: %{deferrable: left}}, %{ @@ -1987,15 +2024,18 @@ defmodule AshSqlite.MigrationGenerator do # This exists to handle the fact that the remapping of the key name -> source caused attributes # to be considered unequal. We ignore things that only differ in that way using this function. - defp attributes_unequal?(left, right, repo, _old_snapshot, _new_snapshot) do - left = clean_for_equality(left, repo) - - right = clean_for_equality(right, repo) - - left != right + defp attributes_unequal?( + left, + right, + _repo, + _old_snapshot, + _new_snapshot, + renaming? + ) do + clean_for_equality(left, renaming?) != clean_for_equality(right, renaming?) end - defp clean_for_equality(attribute, _repo) do + defp clean_for_equality(attribute, ignore_names?) do cond do attribute[:source] -> Map.put(attribute, :name, attribute[:source]) @@ -2023,6 +2063,21 @@ defmodule AshSqlite.MigrationGenerator do attribute -> attribute end) + |> without_names(ignore_names?) + end + + defp without_names(attribute, false), do: attribute + + defp without_names(attribute, true) do + attribute = Map.drop(attribute, [:source, :name]) + + case attribute do + %{references: %{} = references} -> + %{attribute | references: Map.delete(references, :name)} + + attribute -> + attribute + end end defp add_ignore(%{references: references} = attribute) when is_map(references) do diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 18bafff..971fb92 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -821,6 +821,19 @@ defmodule AshSqlite.MigrationGenerator.Operation do end end + defmodule Omitted do + @moduledoc false + # A change SQLite cannot make in place, when tables are not rebuilt: only the hint knows of it. + defstruct [:operation, :table, :multitenancy, :old_multitenancy] + + def reason(%{operation: operation}, multitenancy) do + operation + |> Map.put(:multitenancy, multitenancy) + |> operation.__struct__.up() + |> String.trim() + end + end + defmodule RebuildTable do @moduledoc false # The operations of a table when one of them cannot be done in place (`requires_rebuild?/1`): @@ -835,6 +848,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do def requires_rebuild?(%Operation.DropForeignKey{}), do: true def requires_rebuild?(%Operation.AlterDeferrability{}), do: true def requires_rebuild?(%Operation.RemovePrimaryKey{}), do: true + def requires_rebuild?(%Operation.Omitted{}), do: true def requires_rebuild?(%Operation.AddAttribute{attribute: attribute}), do: attribute.allow_nil? == false and attribute.default == "nil" diff --git a/lib/mix/tasks/ash_sqlite.generate_migrations.ex b/lib/mix/tasks/ash_sqlite.generate_migrations.ex index 4827ae5..6be288b 100644 --- a/lib/mix/tasks/ash_sqlite.generate_migrations.ex +++ b/lib/mix/tasks/ash_sqlite.generate_migrations.ex @@ -52,8 +52,9 @@ defmodule Mix.Tasks.AshSqlite.GenerateMigrations do #### Rebuilding tables SQLite and its Ecto adapters cannot change a column's type, default or nullability, or a foreign key, - in place. By default the generator writes a statement that fails when the migration runs, and prints a hint - saying so. With `--rebuild-tables` it writes a `rebuild_table` instead, which creates the table + in place. By default the generator writes a statement that fails when the migration runs, or leaves + the change out (a renamed attribute that also changes), and prints a hint saying so. With + `--rebuild-tables` it writes a `rebuild_table` instead, which creates the table again in its new shape and copies the rows. See the "Migrations" guide. #### Conflicts/Multiple Resources diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index 52d44af..bcafad2 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -1927,5 +1927,76 @@ defmodule AshSqlite.MigrationGeneratorTest do assert up =~ "# - modify :parent_id, references(:parents" refute up =~ "dropping the foreign key" end + + test "a plain rename of a foreign key column stays a plain rename", ctx do + defresource Parent, "parents" do + attributes do + uuid_primary_key(:id) + end + end + + defresource Child, "children" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:parent, Parent, source_attribute: :owner_id) + end + end + + defdomain([Parent, Child]) + generate(Domain, ctx) + + defresource Child, "children" do + attributes do + uuid_primary_key(:id) + end + + relationships do + belongs_to(:parent, Parent, source_attribute: :guardian_id) + end + end + + send(self(), {:mix_shell_input, :yes?, true}) + send(self(), {:mix_shell_input, :prompt, "guardian_id"}) + generate(Domain, ctx) + migration = last_migration(ctx) + refute migration =~ "use AshSqlite.Migration" + assert migration =~ "rename table(:children), :owner_id, to: :guardian_id" + end + + test "without the option, a rename together with a change is only a rename", ctx do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:title, :string) + end + end + + defdomain([Item]) + generate(Domain, ctx, rebuild_tables: nil) + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:subject, :string, allow_nil?: false) + end + end + + send(self(), {:mix_shell_input, :yes?, true}) + generate(Domain, ctx, rebuild_tables: nil, quiet: false) + migration = last_migration(ctx) + + assert migration =~ "rename table(:items), :title, to: :subject" + refute migration =~ "rebuild_table" + refute migration =~ "modify" + + # the change that is left out is in the hint + assert_received {:mix_shell, :info, + ["SQLite cannot make these changes to `items`" <> _ = hint]} + + assert hint =~ " - modify :subject, :text, null: false (not in the migration)" + end end end diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index 41ad7e8..68f6f79 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -591,6 +591,37 @@ defmodule AshSqlite.TableRebuildTest do end describe "the generator" do + test "a rename together with a change is both a rename and a change", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:title, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, title) VALUES ('a', 'x')") + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:subject, :string, allow_nil?: false) + end + end + + send(self(), {:mix_shell_input, :yes?, true}) + send(self(), {:mix_shell_input, :prompt, "subject"}) + generate(Domain, ctx) + assert last_migration(ctx) =~ "use AshSqlite.Migration" + migrate(ctx) + + assert sql("SELECT id, subject FROM items") == [["a", "x"]] + + assert [%{name: "subject", notnull: true}] = + Enum.filter(columns("items"), &(&1.name == "subject")) + end + test "generating again after a rebuild migration finds nothing to do (codegen --check)", %{ ctx: ctx } do @@ -1073,5 +1104,39 @@ defmodule AshSqlite.TableRebuildTest do migrate(ctx) assert sql("SELECT id, code FROM items") == [["a", "none"]] end + + test "a renamed column that becomes required: the remedy replaces the rename", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:title, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, title) VALUES ('a', NULL)") + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:subject, :string, allow_nil?: false) + end + end + + send(self(), {:mix_shell_input, :yes?, true}) + generate(Domain, ctx) + assert last_migration(ctx) =~ ~S|copy: [subject: "COALESCE(title, 'your value')"]| + assert last_migration(ctx) =~ "in place of its `subject: :title`" + + edit_last_migration( + ctx, + "copy: [subject: :title]", + ~S|copy: [subject: "COALESCE(title, 'untitled')"]| + ) + + migrate(ctx) + assert sql("SELECT id, subject FROM items") == [["a", "untitled"]] + end end end From b8abbacdc3fee1a75288da64d1954dc242c048db Mon Sep 17 00:00:00 2001 From: Nassim Date: Thu, 8 Oct 2026 22:17:55 +0200 Subject: [PATCH 09/15] improvement: use an attribute default as its column default (opt-in) --- .../development/migrations-and-tasks.md | 4 + .../migration_generator.ex | 62 +++++++++++---- lib/migration_generator/operation.ex | 7 +- .../tasks/ash_sqlite.generate_migrations.ex | 4 + lib/repo.ex | 1 + test/migration_generator_test.exs | 67 +++++++++++++++- test/table_rebuild_test.exs | 76 +++++++++++++++++++ 7 files changed, 200 insertions(+), 21 deletions(-) diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index 6f79e8c..03a8871 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -126,6 +126,10 @@ def up do end ``` +The `default:` of an attribute (a number, string, boolean, atom, decimal, date or time) is then also the default of its column. SQLite cannot change a default in place either, so the first run rebuilds the tables whose attributes have one: `--dry-run` shows which. + +Snapshots keep the defaults, so a run without the option after one with it sees them as removed. If you use rebuilds, set `rebuild_tables: true` in the repo's config instead of passing the flag, so that every run agrees, `mix ash.codegen --check` in CI included. + ### Regenerating Migrations Often, you will run into a situation where you want to make a slight change to a resource after you've already generated and run migrations. If you are using git and would like to undo those changes, then regenerate the migrations, this script may prove useful: diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index ce9c7ef..ecbb642 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -36,7 +36,7 @@ defmodule AshSqlite.MigrationGenerator do Ash.DataLayer.data_layer(resource) == AshSqlite.DataLayer && AshSqlite.DataLayer.Info.migrate?(resource) end) - |> Enum.flat_map(&get_snapshots(&1, all_resources)) + |> Enum.flat_map(&get_snapshots(&1, all_resources, opts)) repos = snapshots @@ -2250,7 +2250,7 @@ defmodule AshSqlite.MigrationGenerator do defp pad(i) when i < 10, do: <> defp pad(i), do: to_string(i) - def get_snapshots(resource, all_resources) do + def get_snapshots(resource, all_resources, opts \\ %__MODULE__{}) do Code.ensure_compiled!(AshSqlite.DataLayer.Info.repo(resource)) if AshSqlite.DataLayer.Info.polymorphic?(resource) do @@ -2262,7 +2262,7 @@ defmodule AshSqlite.MigrationGenerator do |> Enum.uniq() |> Enum.map(fn relationship -> resource - |> do_snapshot(relationship.context[:data_layer][:table]) + |> do_snapshot(relationship.context[:data_layer][:table], opts) |> Map.update!(:identities, fn identities -> identity_index_names = AshSqlite.DataLayer.Info.identity_index_names(resource) @@ -2292,7 +2292,8 @@ defmodule AshSqlite.MigrationGenerator do default( source_attribute, relationship.destination, - AshSqlite.DataLayer.Info.repo(relationship.destination) + AshSqlite.DataLayer.Info.repo(relationship.destination), + opts ), deferrable: false, match_tenant?: false, @@ -2313,13 +2314,13 @@ defmodule AshSqlite.MigrationGenerator do end) end) else - [do_snapshot(resource, AshSqlite.DataLayer.Info.table(resource))] + [do_snapshot(resource, AshSqlite.DataLayer.Info.table(resource), opts)] end end - defp do_snapshot(resource, table) do + defp do_snapshot(resource, table, opts) do snapshot = %{ - attributes: attributes(resource, table), + attributes: attributes(resource, table, opts), identities: identities(resource), table: table || AshSqlite.DataLayer.Info.table(resource), custom_indexes: custom_indexes(resource), @@ -2373,7 +2374,7 @@ defmodule AshSqlite.MigrationGenerator do } end - defp attributes(resource, table) do + defp attributes(resource, table, opts) do repo = AshSqlite.DataLayer.Info.repo(resource) ignored = AshSqlite.DataLayer.Info.migration_ignore_attributes(resource) || [] @@ -2393,7 +2394,7 @@ defmodule AshSqlite.MigrationGenerator do ]) ) |> Enum.map(fn attribute -> - default = default(attribute, resource, repo) + default = default(attribute, resource, repo, opts) type = AshSqlite.DataLayer.Info.migration_types(resource)[attribute.name] || @@ -2705,23 +2706,24 @@ defmodule AshSqlite.MigrationGenerator do |> Enum.map(&Map.put(&1, :base_filter, AshSqlite.DataLayer.Info.base_filter_sql(resource))) end - defp default(%{name: name, default: default}, resource, _repo) when is_function(default) do + defp default(%{name: name, default: default}, resource, _repo, _opts) + when is_function(default) do configured_default(resource, name) || "nil" end - defp default(%{name: name, default: {_, _, _}}, resource, _), + defp default(%{name: name, default: {_, _, _}}, resource, _, _), do: configured_default(resource, name) || "nil" - defp default(%{name: name, default: nil}, resource, _), + defp default(%{name: name, default: nil}, resource, _, _), do: configured_default(resource, name) || "nil" - defp default(%{name: name, default: []}, resource, _), + defp default(%{name: name, default: []}, resource, _, _), do: configured_default(resource, name) || "[]" - defp default(%{name: name, default: default}, resource, _) when default == %{}, + defp default(%{name: name, default: default}, resource, _, _) when default == %{}, do: configured_default(resource, name) || "%{}" - defp default(%{name: name, default: value, type: type} = attr, resource, _) do + defp default(%{name: name, default: value, type: type} = attr, resource, repo, opts) do case configured_default(resource, name) do nil -> case migration_default(type, Map.get(attr, :constraints, []), value) do @@ -2729,7 +2731,7 @@ defmodule AshSqlite.MigrationGenerator do default :error -> - "nil" + literal_default(value, repo, opts) end default -> @@ -2737,6 +2739,34 @@ defmodule AshSqlite.MigrationGenerator do end end + defp literal_default(value, repo, opts) do + if rebuild_tables?(opts, repo), + do: literal_default(value), + else: "nil" + end + + defp literal_default(value) when is_number(value), do: to_string(value) + defp literal_default(value) when is_boolean(value), do: to_string(value) + defp literal_default(value) when is_atom(value), do: inspect(to_string(value)) + defp literal_default(value) when is_binary(value), do: inspect(value) + defp literal_default(%Decimal{} = value), do: inspect(to_string(value)) + + defp literal_default(%module{} = value) when module in [Date, Time, DateTime, NaiveDateTime], + do: ~s[fragment("'#{module.to_iso8601(value)}'")] + + defp literal_default(value) do + Logger.warning(""" + You have specified a default value that cannot be explicitly converted to a column default: + + `#{inspect(value)}` + + The default value in the migration will be set to `nil`. To set one, use + `migration_defaults` in the sqlite section of the resource. + """) + + "nil" + end + defp migration_default(type, constraints, value) do type = type diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 971fb92..21fe301 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -1038,8 +1038,8 @@ defmodule AshSqlite.MigrationGenerator.Operation do stops this migration (nothing is changed). Give those rows one with the `copy:` option of `rebuild_table`#{replaces}, for example rebuild_table :#{as_atom(table)}, copy: [#{atom_key(name)} "COALESCE(#{from}, 'your value')"] do - To give new rows one too, add `migration_defaults(#{atom_key(name)} "\\"your value\\"")` to the - resource's `sqlite` block and regenerate.\ + To give new rows one too, set a `default` on the attribute (or use `migration_defaults`) + and regenerate.\ """ end @@ -1049,8 +1049,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do migration stops (nothing is changed). Give the existing rows a value with the `copy:` option of `rebuild_table`, for example rebuild_table :#{as_atom(table)}, copy: [#{atom_key(name)} "'your value'"] do - or add `migration_defaults(#{atom_key(name)} "\\"your value\\"")` to the resource's `sqlite` - block and regenerate.\ + or set a `default` on the attribute (or use `migration_defaults`) and regenerate.\ """ end diff --git a/lib/mix/tasks/ash_sqlite.generate_migrations.ex b/lib/mix/tasks/ash_sqlite.generate_migrations.ex index 6be288b..4166cd0 100644 --- a/lib/mix/tasks/ash_sqlite.generate_migrations.ex +++ b/lib/mix/tasks/ash_sqlite.generate_migrations.ex @@ -57,6 +57,10 @@ defmodule Mix.Tasks.AshSqlite.GenerateMigrations do `--rebuild-tables` it writes a `rebuild_table` instead, which creates the table again in its new shape and copies the rows. See the "Migrations" guide. + The `default:` of an attribute that is a plain value (a number, string, boolean, atom, decimal, + date or time) then also becomes the default of its column. Set `rebuild_tables: true` in the + repo's config rather than passing the flag, so that every run agrees. See the "Migrations" guide. + #### Conflicts/Multiple Resources It will raise on conflicts that it can't resolve, like the same field with different diff --git a/lib/repo.ex b/lib/repo.ex index d25a464..aa6ef02 100644 --- a/lib/repo.ex +++ b/lib/repo.ex @@ -22,6 +22,7 @@ defmodule AshSqlite.Repo do - `:snapshots_path` - The path where the resource snapshots for the migration generator are stored. - `:rebuild_tables` - Set to `true` to rebuild tables SQLite cannot change in place, as if `--rebuild-tables` were passed to every run (`--no-rebuild-tables` turns it off for one run). + Prefer it to the flag: the defaults it applies are kept in the snapshots, so every run has to agree. """ @doc "Use this to inform the data layer about what extensions are installed" diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index bcafad2..c300270 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -8,7 +8,9 @@ defmodule AshSqlite.MigrationGeneratorTest do @moduletag :tmp_dir import ExUnit.CaptureLog - import AshSqlite.RebuildHelper, only: [generate: 2, generate: 3, last_migration: 1] + + import AshSqlite.RebuildHelper, + only: [generate: 2, generate: 3, last_migration: 1, migrations: 1] setup %{tmp_dir: tmp_dir} do current_shell = Mix.shell() @@ -1998,5 +2000,68 @@ defmodule AshSqlite.MigrationGeneratorTest do assert hint =~ " - modify :subject, :text, null: false (not in the migration)" end + + test "an attribute's default is not a column default when rebuilding is off", ctx do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:count, :integer, default: 0) + end + end + + defdomain([Item]) + generate(Domain, ctx, rebuild_tables: nil) + + refute last_migration(ctx) =~ "default:" + end + + test "a value that cannot be a column default is nil, with a warning", ctx do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:tags, {:array, :string}, default: ["a"]) + end + end + + defdomain([Item]) + log = capture_log(fn -> generate(Domain, ctx) end) + + assert log =~ "cannot be explicitly converted to a column default" + assert log =~ ~S|["a"]| + assert last_migration(ctx) =~ "add :tags, {:array, :text}" + end + + test "a migration_defaults entry still wins over an attribute's default", ctx do + defitem do + sqlite do + migration_defaults(title: "\"from migration_defaults\"") + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, default: "from the attribute") + end + end + + defdomain([Item]) + generate(Domain, ctx) + assert last_migration(ctx) =~ ~S|default: "from migration_defaults"| + end + + test "generating again after the defaults were applied finds nothing to do", ctx do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:count, :integer, default: 3) + end + end + + defdomain([Item]) + generate(Domain, ctx) + assert last_migration(ctx) =~ "default: 3" + + generate(Domain, ctx) + assert length(migrations(ctx)) == 1 + end end end diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index 68f6f79..a5b7beb 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -1139,4 +1139,80 @@ defmodule AshSqlite.TableRebuildTest do assert sql("SELECT id, subject FROM items") == [["a", "untitled"]] end end + + describe "default values" do + @describetag repo_config: [rebuild_tables: true] + + test "a new table gets the defaults of its attributes", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:title, :string, default: "it's untitled") + attribute(:count, :integer, default: 0) + attribute(:ratio, :float, default: 1.5) + attribute(:done, :boolean, default: false) + attribute(:price, :decimal, default: Decimal.new("9.50")) + attribute(:on, :date, default: ~D[2020-01-02]) + attribute(:at, :utc_datetime, default: ~U[2020-01-02 03:04:05Z]) + end + end + + generate(Domain, ctx) + migration = last_migration(ctx) + assert migration =~ ~S|add :title, :text, default: "it's untitled"| + assert migration =~ "add :count, :bigint, default: 0" + assert migration =~ "add :done, :boolean, default: false" + + migrate(ctx) + sql("INSERT INTO items (id) VALUES ('a')") + + assert [["a", "it's untitled", 0, 1.5, 0, price, "2020-01-02", at]] = + sql("SELECT id, title, count, ratio, done, price, \"on\", at FROM items") + + assert to_string(price) =~ "9.5" + # the text Ecto stores, so that comparing with a value it dumps finds the row + assert at == "2020-01-02T03:04:05Z" + end + + test "a default added to an existing attribute rebuilds the table to get it", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:title, :string) + end + end + + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, title) VALUES ('a', NULL)") + + defitem do + attributes do + uuid_primary_key(:id) + attribute(:title, :string, default: "untitled") + end + end + + generate(Domain, ctx) + assert last_migration(ctx) =~ ~S|add :title, :text, default: "untitled"| + migrate(ctx) + + sql("INSERT INTO items (id) VALUES ('b')") + # the rows that were there keep what they had + assert sql("SELECT id, title FROM items ORDER BY id") == [["a", nil], ["b", "untitled"]] + end + + test "--no-rebuild-tables turns them off for a run, whatever the config says", %{ctx: ctx} do + defitem do + attributes do + uuid_primary_key(:id) + attribute(:count, :integer, default: 0) + end + end + + generate(Domain, ctx, rebuild_tables: false) + + refute last_migration(ctx) =~ "default:" + end + end end From 91589b6604671429b1492c44c96d4190791830bf Mon Sep 17 00:00:00 2001 From: Nassim Date: Thu, 8 Oct 2026 22:17:55 +0200 Subject: [PATCH 10/15] improvement: rebuild a table that becomes STRICT, or stops being --- .../development/migrations-and-tasks.md | 4 +- .../migration_generator.ex | 15 ++++ lib/migration_generator/operation.ex | 18 +++++ .../tasks/ash_sqlite.generate_migrations.ex | 4 +- test/migration_generator_test.exs | 58 +++++++++++++++ test/table_rebuild_test.exs | 73 +++++++++++++++++++ 6 files changed, 168 insertions(+), 4 deletions(-) diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index 03a8871..e913664 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -39,9 +39,9 @@ For more information on generating migrations, run `mix help ash_sqlite.generate ### Changes SQLite cannot make in place -SQLite can mostly add, rename and drop a column in place. Changing a column's type or default, adding or dropping a foreign key, changing a primary key and, as the adapters write it, making a column required or optional all need the table to be created again in its new shape, with the rows copied over. +SQLite can mostly add, rename and drop a column in place. Changing a column's type or default, adding or dropping a foreign key, changing a primary key, making a table `STRICT` and, as the adapters write it, making a column required or optional all need the table to be created again in its new shape, with the rows copied over. -By default the generator writes a statement that fails when the migration runs, or leaves the change out (a renamed attribute that also changes). With `--rebuild-tables`, or `rebuild_tables: true` in the repo's config, it writes a `rebuild_table` instead, which shows the table's new shape and what triggered it: +By default the generator writes a statement that fails when the migration runs, or leaves the change out (a renamed attribute that also changes, a table that becomes `STRICT`). With `--rebuild-tables`, or `rebuild_tables: true` in the repo's config, it writes a `rebuild_table` instead, which shows the table's new shape and what triggered it: ```elixir def up do diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index ecbb642..475b9bf 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -1687,6 +1687,7 @@ defmodule AshSqlite.MigrationGenerator do |> Enum.concat() |> Enum.map(&Map.put(&1, :multitenancy, snapshot.multitenancy)) |> Enum.map(&Map.put(&1, :old_multitenancy, old_snapshot.multitenancy)) + |> add_strict_change(snapshot, old_snapshot, opts) |> rebuild_if_needed(snapshot, old_snapshot, opts) end @@ -1694,6 +1695,20 @@ defmodule AshSqlite.MigrationGenerator do defp rebuild_tables?(%{rebuild_tables: nil}, repo), do: repo.config()[:rebuild_tables] == true defp rebuild_tables?(%{rebuild_tables: rebuild_tables}, _repo), do: rebuild_tables + defp add_strict_change(operations, snapshot, old_snapshot, opts) do + from = !!old_snapshot[:strict?] + + if !old_snapshot[:empty?] and from != snapshot.strict? do + change = %Operation.AlterStrict{table: snapshot.table, from: from, to: snapshot.strict?} + + if rebuild_tables?(opts, snapshot.repo), + do: [change | operations], + else: [%Operation.Omitted{operation: change, table: snapshot.table} | operations] + else + operations + end + end + defp rebuild_if_needed(operations, snapshot, old_snapshot, opts) do if !old_snapshot[:empty?] and Enum.any?(operations, &Operation.RebuildTable.requires_rebuild?/1) do diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 21fe301..9b7202a 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -821,11 +821,26 @@ defmodule AshSqlite.MigrationGenerator.Operation do end end + defmodule AlterStrict do + @moduledoc false + # The table became STRICT, or stopped being. There is no statement for that: it takes a + # rebuild. + defstruct [:table, :from, :to] + + def reason(%{table: table, to: true}), do: "making `#{table}` a STRICT table" + def reason(%{table: table, to: false}), do: "`#{table}` is no longer a STRICT table" + end + defmodule Omitted do @moduledoc false # A change SQLite cannot make in place, when tables are not rebuilt: only the hint knows of it. defstruct [:operation, :table, :multitenancy, :old_multitenancy] + alias AshSqlite.MigrationGenerator.Operation + + def reason(%{operation: %Operation.AlterStrict{} = operation}, _multitenancy), + do: Operation.AlterStrict.reason(operation) + def reason(%{operation: operation}, multitenancy) do operation |> Map.put(:multitenancy, multitenancy) @@ -849,6 +864,7 @@ defmodule AshSqlite.MigrationGenerator.Operation do def requires_rebuild?(%Operation.AlterDeferrability{}), do: true def requires_rebuild?(%Operation.RemovePrimaryKey{}), do: true def requires_rebuild?(%Operation.Omitted{}), do: true + def requires_rebuild?(%Operation.AlterStrict{}), do: true def requires_rebuild?(%Operation.AddAttribute{attribute: attribute}), do: attribute.allow_nil? == false and attribute.default == "nil" @@ -1166,6 +1182,8 @@ defmodule AshSqlite.MigrationGenerator.Operation do "which SQLite refuses on a table with rows" end + defp reason(%Operation.AlterStrict{} = change), do: Operation.AlterStrict.reason(change) + defp reason(%Operation.DropForeignKey{direction: :down, attribute: %{references: reference}}), do: "dropping the foreign key #{reference.name} when rolling back" diff --git a/lib/mix/tasks/ash_sqlite.generate_migrations.ex b/lib/mix/tasks/ash_sqlite.generate_migrations.ex index 4166cd0..b0761a8 100644 --- a/lib/mix/tasks/ash_sqlite.generate_migrations.ex +++ b/lib/mix/tasks/ash_sqlite.generate_migrations.ex @@ -53,8 +53,8 @@ defmodule Mix.Tasks.AshSqlite.GenerateMigrations do SQLite and its Ecto adapters cannot change a column's type, default or nullability, or a foreign key, in place. By default the generator writes a statement that fails when the migration runs, or leaves - the change out (a renamed attribute that also changes), and prints a hint saying so. With - `--rebuild-tables` it writes a `rebuild_table` instead, which creates the table + the change out (a renamed attribute that also changes, a table that becomes `STRICT`), and prints a + hint saying so. With `--rebuild-tables` it writes a `rebuild_table` instead, which creates the table again in its new shape and copies the rows. See the "Migrations" guide. The `default:` of an attribute that is a plain value (a number, string, boolean, atom, decimal, diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index c300270..b1b4a5f 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -2063,5 +2063,63 @@ defmodule AshSqlite.MigrationGeneratorTest do generate(Domain, ctx) assert length(migrations(ctx)) == 1 end + + test "a snapshot from before there was strict? is a table that is not", ctx do + defitem do + sqlite do + strict?(false) + end + + attributes do + uuid_primary_key(:id) + end + end + + defdomain([Item]) + generate(Domain, ctx) + + for file <- Path.wildcard(Path.join(ctx.snapshot_path, "**/*.json")) do + snapshot = file |> File.read!() |> Jason.decode!() |> Map.delete("strict?") + File.write!(file, Jason.encode!(snapshot)) + end + + generate(Domain, ctx) + assert length(migrations(ctx)) == 1 + end + + test "without the option, a table that becomes STRICT is only in the hint", ctx do + defitem do + sqlite do + strict?(false) + end + + attributes do + uuid_primary_key(:id) + end + end + + defdomain([Item]) + generate(Domain, ctx, rebuild_tables: nil) + + defitem do + sqlite do + strict?(true) + end + + attributes do + uuid_primary_key(:id) + end + end + + generate(Domain, ctx, rebuild_tables: nil, quiet: false) + + assert length(migrations(ctx)) == 1 + + assert_received {:mix_shell, :info, + ["SQLite cannot make these changes to `items`" <> _ = hint]} + + assert hint =~ "making `items` a STRICT table" + assert hint =~ "(not in the migration)" + end end end diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index a5b7beb..e3268e3 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -1215,4 +1215,77 @@ defmodule AshSqlite.TableRebuildTest do refute last_migration(ctx) =~ "default:" end end + + describe "STRICT" do + defmacrop strict_items(strict?) do + quote do + defitem do + sqlite do + strict?(unquote(strict?)) + end + + attributes do + uuid_primary_key(:id) + attribute(:count, :integer) + attribute(:name, :string) + end + end + end + end + + defp table_sql, do: sql("SELECT sql FROM sqlite_master WHERE name = 'items'") |> hd() |> hd() + + test "making a table STRICT rebuilds it and keeps the rows", %{ctx: ctx} do + strict_items(false) + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, count, name) VALUES ('a', 1, 'x')") + refute table_sql() =~ "STRICT" + + strict_items(true) + generate(Domain, ctx) + assert last_migration(ctx) =~ "# - making `items` a STRICT table" + + assert last_migration(ctx) =~ ~S|rebuild_table :items, options: "STRICT" do| + + migrate(ctx) + assert table_sql() =~ "STRICT" + assert sql("SELECT id, count, name FROM items") == [["a", 1, "x"]] + end + + test "a value that does not fit its column stops it, and nothing is changed", %{ctx: ctx} do + strict_items(false) + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, count) VALUES ('a', 'many')") + versions_before = versions() + + strict_items(true) + generate(Domain, ctx) + + assert migrate_error(ctx) =~ "cannot store TEXT value in INTEGER column" + assert leftovers() == [] + assert versions() == versions_before + refute table_sql() =~ "STRICT" + assert sql("SELECT count FROM items") == [["many"]] + end + + test "making a table not STRICT, and rolling that back", %{ctx: ctx} do + strict_items(true) + generate(Domain, ctx) + migrate(ctx) + sql("INSERT INTO items (id, count) VALUES ('a', 1)") + assert table_sql() =~ "STRICT" + + strict_items(false) + generate(Domain, ctx) + assert last_migration(ctx) =~ "# - `items` is no longer a STRICT table" + migrate(ctx) + refute table_sql() =~ "STRICT" + + rollback(ctx) + assert table_sql() =~ "STRICT" + assert sql("SELECT id, count FROM items") == [["a", 1]] + end + end end From 3b867e39e9b7461b8e6000c2819ba241c3dbdd62 Mon Sep 17 00:00:00 2001 From: Nassim Date: Mon, 12 Oct 2026 15:03:49 +0200 Subject: [PATCH 11/15] chore: choose the ecto_sql to test against with ECTO_SQL_VERSION --- mix.exs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/mix.exs b/mix.exs index 1744ac8..3a928d2 100644 --- a/mix.exs +++ b/mix.exs @@ -151,10 +151,10 @@ defmodule AshSqlite.MixProject do # Run "mix help deps" to learn about dependencies. defp deps do [ - {:ecto_sql, "~> 3.13"}, + {:ecto_sql, ecto_sql_version("~> 3.13")}, {:ecto_sqlite3, "~> 0.12"}, {:ecto_libsql, "~> 0.9", optional: true}, - {:ecto, "~> 3.13"}, + {:ecto, "~> 3.13", override: System.get_env("ECTO_SQL_VERSION") == "main"}, {:jason, "~> 1.0"}, {:ash, ash_version("~> 3.33 and >= 3.33.2")}, {:ash_sql, ash_sql_version("~> 0.2 and >= 0.6.9")}, @@ -170,6 +170,21 @@ defmodule AshSqlite.MixProject do ] end + # `main` has migration callbacks that are not released yet. It asks for `ecto` from git, which + # the `ecto` entry above overrides to stay on Hex. + defp ecto_sql_version(default_version) do + case System.get_env("ECTO_SQL_VERSION") do + nil -> + default_version + + "main" -> + [git: "https://github.com/elixir-ecto/ecto_sql.git", override: true] + + version -> + "~> #{version}" + end + end + defp ash_version(default_version) do case System.get_env("ASH_VERSION") do nil -> From f3c09537f12198bc3721e107a13c443cef808705 Mon Sep 17 00:00:00 2001 From: Nassim Date: Mon, 12 Oct 2026 15:05:38 +0200 Subject: [PATCH 12/15] fixup! improvement: keep the custom statements of a table when it is rebuilt --- .../development/migrations-and-tasks.md | 40 +------------------ 1 file changed, 1 insertion(+), 39 deletions(-) diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index e913664..15afb05 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -58,45 +58,7 @@ end The rebuild runs in one transaction with foreign keys off, and changes nothing if it fails. Columns both tables have are copied as they are. When the right value for the rows depends on your data, as when a column becomes required and has no default, a `REVIEW` comment says so and shows how to give them one. -The custom statements of the resource are dropped (their `down`) before the rebuild and recreated (their `up`) after it, so what they hold is lost. To keep it, add the copy to the generated migration, around the statement's `execute` lines. For a statement that creates a table: - -```elixir -custom_statements do - statement :items_history do - up "CREATE TABLE items_history (item_id TEXT)" - down "DROP TABLE items_history" - end -end -``` - -the generated migration drops and recreates `items_history` around the rebuild of `items`. The two lines marked `# added` keep the rows: - -```elixir -def up do - # SQLite cannot change `items` in place, so it is rebuilt ... Changes: - # - modify :name, :text, null: true - # - REVIEW: the custom statements :items_history are dropped (their `down`) before and ... - execute("CREATE TABLE items_history_backup AS SELECT * FROM items_history") # added - - execute(""" - DROP TABLE items_history - """) - - rebuild_table :items do - add :id, :uuid, null: false, primary_key: true - add :name, :text - end - - execute(""" - CREATE TABLE items_history (item_id TEXT) - """) - - execute("INSERT INTO items_history SELECT * FROM items_history_backup") # added - execute("DROP TABLE items_history_backup") # added -end -``` - -A full-text index needs no backup: it is rebuilt from its table by a command after the statement's `up`. +The custom statements of the resource are dropped (their `down`) before the rebuild and recreated (their `up`) after it, so what they hold is lost. That suits what can be dropped and recreated without losing anything, such as a trigger, a view or an index, and a full-text index, which is rebuilt from its table. Data that exists nowhere else does not belong in a custom statement. The full-text index needs a command that you add to the generated migration, after the statement's `up`: ```elixir statement :items_fts do From 47f39897fa98d782efb2631a6eb258ce0452b159 Mon Sep 17 00:00:00 2001 From: Nassim Date: Mon, 12 Oct 2026 15:10:57 +0200 Subject: [PATCH 13/15] improvement: support an ecto_sql without the migration callbacks --- .../development/migrations-and-tasks.md | 2 + lib/migration.ex | 16 ++++-- .../migration_generator.ex | 25 +++++++-- .../tasks/ash_sqlite.generate_migrations.ex | 3 +- lib/repo.ex | 2 + test/migration_generator_test.exs | 31 +++++++++++ test/migration_test.exs | 1 + test/rebuild_unsupported_test.exs | 53 +++++++++++++++++++ test/test_helper.exs | 7 +++ 9 files changed, 133 insertions(+), 7 deletions(-) create mode 100644 test/rebuild_unsupported_test.exs diff --git a/documentation/topics/development/migrations-and-tasks.md b/documentation/topics/development/migrations-and-tasks.md index 15afb05..b101135 100644 --- a/documentation/topics/development/migrations-and-tasks.md +++ b/documentation/topics/development/migrations-and-tasks.md @@ -56,6 +56,8 @@ def up do end ``` +A rebuild needs an `ecto_sql` that runs the `before_transaction/1` and `after_transaction/1` migration callbacks, which is how foreign keys are switched off outside the transaction. Without them, asking for rebuilds raises, and the generator only prints its hint, without suggesting one. + The rebuild runs in one transaction with foreign keys off, and changes nothing if it fails. Columns both tables have are copied as they are. When the right value for the rows depends on your data, as when a column becomes required and has no default, a `REVIEW` comment says so and shows how to give them one. The custom statements of the resource are dropped (their `down`) before the rebuild and recreated (their `up`) after it, so what they hold is lost. That suits what can be dropped and recreated without losing anything, such as a trigger, a view or an index, and a full-text index, which is rebuilt from its table. Data that exists nowhere else does not belong in a custom statement. The full-text index needs a command that you add to the generated migration, after the statement's `up`: diff --git a/lib/migration.ex b/lib/migration.ex index a48c3a4..8a0a07e 100644 --- a/lib/migration.ex +++ b/lib/migration.ex @@ -23,6 +23,10 @@ defmodule AshSqlite.Migration do safe: foreign keys are off while it runs (dropping a table would otherwise run the `ON DELETE` actions of the tables that point to it) and they are checked before it commits. A rebuild either completes or changes nothing. + + Foreign keys can only be switched off by an `ecto_sql` that runs the `before_transaction/1` + and `after_transaction/1` migration callbacks. Without them, a rebuild refuses to run + unless the connection has foreign keys off already. """ @pragmas_key :ash_sqlite_rebuild_pragmas @@ -40,6 +44,12 @@ defmodule AshSqlite.Migration do end end + # An `ecto_sql` without the callbacks never calls the ones `use AshSqlite.Migration` adds. + @doc false + def rebuild_supported? do + {:before_transaction, 1} in Ecto.Migration.behaviour_info(:optional_callbacks) + end + # `PRAGMA foreign_keys` does nothing inside a transaction, so it is set before Ecto opens one, # on the connection it will use. The previous values are kept to put them back afterwards. @doc false @@ -69,9 +79,9 @@ defmodule AshSqlite.Migration do if pragma(repo, "foreign_keys") != 0 do raise "refusing to rebuild #{table}: foreign keys are on, so dropping it would run " <> "the ON DELETE actions of the tables that reference it. They are switched " <> - "off by the callbacks `use AshSqlite.Migration` adds, which Ecto only runs " <> - "when the migration runs in its own transaction: remove " <> - "`@disable_ddl_transaction true`." + "off by the callbacks `use AshSqlite.Migration` adds. Ecto runs them only when " <> + "the migration runs in its own transaction (remove `@disable_ddl_transaction " <> + "true`), and only in an `ecto_sql` that has them." end end diff --git a/lib/migration_generator/migration_generator.ex b/lib/migration_generator/migration_generator.ex index 475b9bf..6565bb7 100644 --- a/lib/migration_generator/migration_generator.ex +++ b/lib/migration_generator/migration_generator.ex @@ -1691,9 +1691,23 @@ defmodule AshSqlite.MigrationGenerator do |> rebuild_if_needed(snapshot, old_snapshot, opts) end + defp rebuild_tables?(opts, repo) do + rebuild_tables = rebuild_requested?(opts, repo) + + if rebuild_tables and !AshSqlite.Migration.rebuild_supported?() do + raise "rebuilding tables needs an `ecto_sql` that runs the `before_transaction/1` and " <> + "`after_transaction/1` migration callbacks, and the one installed does not. " <> + "Turn `rebuild_tables` off in the repo's config, or pass `--no-rebuild-tables`." + end + + rebuild_tables + end + # `--rebuild-tables` (or `--no-rebuild-tables`) for this run, else the repo's `rebuild_tables`. - defp rebuild_tables?(%{rebuild_tables: nil}, repo), do: repo.config()[:rebuild_tables] == true - defp rebuild_tables?(%{rebuild_tables: rebuild_tables}, _repo), do: rebuild_tables + defp rebuild_requested?(%{rebuild_tables: nil}, repo), + do: repo.config()[:rebuild_tables] == true + + defp rebuild_requested?(%{rebuild_tables: rebuild_tables}, _repo), do: rebuild_tables defp add_strict_change(operations, snapshot, old_snapshot, opts) do from = !!old_snapshot[:strict?] @@ -1746,10 +1760,15 @@ defmodule AshSqlite.MigrationGenerator do omitted_lines = for reason <- omitted_reasons, do: " - #{reason} (not in the migration)" + advice = + if AshSqlite.Migration.rebuild_supported?(), + do: "To rebuild the table instead, #{rebuild_advice(opts)}\n", + else: "" + warn(""" SQLite cannot make these changes to `#{snapshot.table}` in place: #{Enum.join(fails ++ omitted_lines, "\n")} - To rebuild the table instead, #{rebuild_advice(opts)} + #{advice}\ """) end end diff --git a/lib/mix/tasks/ash_sqlite.generate_migrations.ex b/lib/mix/tasks/ash_sqlite.generate_migrations.ex index b0761a8..d87efb5 100644 --- a/lib/mix/tasks/ash_sqlite.generate_migrations.ex +++ b/lib/mix/tasks/ash_sqlite.generate_migrations.ex @@ -15,7 +15,8 @@ defmodule Mix.Tasks.AshSqlite.GenerateMigrations do * `drop-columns` - whether or not to drop columns as attributes are removed. See below for more * `rebuild-tables` - rebuild a table when SQLite cannot make a change in place. Can be set for every run with `rebuild_tables: true` in the repo's config, and turned off for one run with - `--no-rebuild-tables`. See "Rebuilding tables" below + `--no-rebuild-tables`. It needs an `ecto_sql` with the `before_transaction/1` and + `after_transaction/1` migration callbacks. See "Rebuilding tables" below * `name` - names the generated migrations, prepending with the timestamp. The default is `migrate_resources_`, where `` is the count of migrations matching `*migrate_resources*` plus one. diff --git a/lib/repo.ex b/lib/repo.ex index aa6ef02..d783974 100644 --- a/lib/repo.ex +++ b/lib/repo.ex @@ -23,6 +23,8 @@ defmodule AshSqlite.Repo do - `:rebuild_tables` - Set to `true` to rebuild tables SQLite cannot change in place, as if `--rebuild-tables` were passed to every run (`--no-rebuild-tables` turns it off for one run). Prefer it to the flag: the defaults it applies are kept in the snapshots, so every run has to agree. + It needs an `ecto_sql` with the `before_transaction/1` and `after_transaction/1` migration + callbacks, and raises without them. """ @doc "Use this to inform the data layer about what extensions are installed" diff --git a/test/migration_generator_test.exs b/test/migration_generator_test.exs index b1b4a5f..be6b05f 100644 --- a/test/migration_generator_test.exs +++ b/test/migration_generator_test.exs @@ -1601,6 +1601,8 @@ defmodule AshSqlite.MigrationGeneratorTest do end describe "rebuilding tables" do + @describetag :rebuild + defmacrop defitem(do: body) do quote do defresource Item, "items" do @@ -2122,4 +2124,33 @@ defmodule AshSqlite.MigrationGeneratorTest do assert hint =~ "(not in the migration)" end end + + describe "without the migration callbacks of ecto_sql" do + @describetag :no_rebuild_support + + test "asking for rebuilds raises", ctx do + defitem do + attributes do + uuid_primary_key(:id) + end + end + + defdomain([Item]) + + assert_raise RuntimeError, ~r/needs an `ecto_sql` that runs/, fn -> + generate(Domain, ctx, rebuild_tables: true) + end + end + + test "the hint lists what SQLite cannot do and does not suggest a rebuild", ctx do + domain = name_required_then_optional(ctx) + generate(domain, ctx, rebuild_tables: nil, quiet: false) + + assert_received {:mix_shell, :info, + ["SQLite cannot make these changes to `items`" <> _ = hint]} + + assert hint =~ " - modify :name, :text, null: true (fails when the migration runs)" + refute hint =~ "rebuild" + end + end end diff --git a/test/migration_test.exs b/test/migration_test.exs index 1c31f1e..2c97464 100644 --- a/test/migration_test.exs +++ b/test/migration_test.exs @@ -8,6 +8,7 @@ defmodule AshSqlite.MigrationTest do """ use ExUnit.Case, async: false @moduletag :migration + @moduletag :rebuild @moduletag :tmp_dir import AshSqlite.RebuildHelper diff --git a/test/rebuild_unsupported_test.exs b/test/rebuild_unsupported_test.exs new file mode 100644 index 0000000..32cda51 --- /dev/null +++ b/test/rebuild_unsupported_test.exs @@ -0,0 +1,53 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.RebuildUnsupportedTest do + @moduledoc """ + What a rebuild does on an `ecto_sql` without the migration callbacks: it refuses. + """ + use ExUnit.Case, async: false + @moduletag :no_rebuild_support + @moduletag :tmp_dir + + import AshSqlite.RebuildHelper + + setup %{tmp_dir: tmp_dir}, do: %{ctx: start(tmp_dir)} + + test "a rebuild refuses while foreign keys are on, and the rows that point at the table stay", + %{ctx: ctx} do + write_migration(ctx, 1, "create_blog", """ + def up do + create table(:posts, primary_key: false) do + add :id, :text, primary_key: true + end + + create table(:comments, primary_key: false) do + add :id, :text, primary_key: true + add :post_id, references(:posts, type: :text, on_delete: :delete_all) + end + end + + def down, do: :ok + """) + + migrate(ctx) + sql("INSERT INTO posts (id) VALUES ('p')") + sql("INSERT INTO comments (id, post_id) VALUES ('c', 'p')") + + write_migration(ctx, 2, "rebuild_posts", """ + def up do + rebuild_table :posts do + add :id, :text, primary_key: true + add :title, :text + end + end + + def down, do: :ok + """) + + assert migrate_error(ctx) =~ "only in an `ecto_sql` that has them" + assert sql("SELECT id FROM comments") == [["c"]] + assert versions() == [1] + end +end diff --git a/test/test_helper.exs b/test/test_helper.exs index 30925ec..eff3b12 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -5,6 +5,13 @@ ExUnit.start() ExUnit.configure(stacktrace_depth: 100) +# A rebuild needs the migration callbacks of an `ecto_sql` that has them +if AshSqlite.Migration.rebuild_supported?() do + ExUnit.configure(exclude: [:no_rebuild_support]) +else + ExUnit.configure(exclude: [:rebuild]) +end + AshSqlite.TestRepo.start_link() AshSqlite.TransactionTestRepo.start_link() AshSqlite.DevTestRepo.start_link() From 224f0604d06dd420e17b339edfaafe1ae0118161 Mon Sep 17 00:00:00 2001 From: Nassim Date: Mon, 12 Oct 2026 16:52:25 +0200 Subject: [PATCH 14/15] fixup! improvement: add rebuild_table, a migration helper that rebuilds a table --- lib/migration.ex | 22 ++++++++++++++++++---- test/migration_test.exs | 26 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/lib/migration.ex b/lib/migration.ex index 8a0a07e..f9b173d 100644 --- a/lib/migration.ex +++ b/lib/migration.ex @@ -71,11 +71,16 @@ defmodule AshSqlite.Migration do end end + @doc false + def __check_can_rebuild__(repo, table) do + check_foreign_keys_off!(repo, table) + check_in_transaction!(repo, table) + end + # Ecto does not run the callbacks for a migration without a transaction # (`@disable_ddl_transaction true`), and dropping a table with foreign keys on deletes - # from the tables that reference it. So `rebuild_table/3` checks first that they are off. - @doc false - def __check_foreign_keys_off__(repo, table) do + # from the tables that reference it. + defp check_foreign_keys_off!(repo, table) do if pragma(repo, "foreign_keys") != 0 do raise "refusing to rebuild #{table}: foreign keys are on, so dropping it would run " <> "the ON DELETE actions of the tables that reference it. They are switched " <> @@ -85,6 +90,15 @@ defmodule AshSqlite.Migration do end end + # Without one nothing keeps other connections from writing during the rebuild, and a failure + # leaves the table half rebuilt. + defp check_in_transaction!(repo, table) do + unless repo.in_transaction?() do + raise "refusing to rebuild #{table}: the migration is not in a transaction, so a rebuild " <> + "could lose concurrent writes or fail halfway. Remove `@disable_ddl_transaction true`." + end + end + @doc false def __legacy_alter_table__(repo, value), do: quiet!(repo, "PRAGMA legacy_alter_table = #{value}") @@ -111,7 +125,7 @@ defmodule AshSqlite.Migration do {copy, opts} = Keyword.pop(unquote(opts), :copy, []) temporary = "#{table}_rebuild" - execute(fn -> AshSqlite.Migration.__check_foreign_keys_off__(repo(), table) end) + execute(fn -> AshSqlite.Migration.__check_can_rebuild__(repo(), table) end) create table(temporary, [primary_key: false] ++ opts) do unquote(block) diff --git a/test/migration_test.exs b/test/migration_test.exs index 2c97464..7290e05 100644 --- a/test/migration_test.exs +++ b/test/migration_test.exs @@ -208,6 +208,32 @@ defmodule AshSqlite.MigrationTest do assert sql("SELECT id FROM comments") == [["c"]] end + # a repo can have foreign keys off for every connection, which is all the check above looks at + @tag repo_config: [foreign_keys: :off, pool_size: 1] + test "a migration without a transaction refuses, with foreign keys off", %{ctx: ctx} do + blog(ctx) + + write_migration(ctx, 2, "no_transaction", """ + @disable_ddl_transaction true + + def up do + rebuild_table :posts do + add :id, :text, primary_key: true + add :title, :text + add :subtitle, :text + end + end + + def down, do: :ok + """) + + assert migrate_error(ctx) =~ + "refusing to rebuild posts: the migration is not in a transaction" + + assert versions() == [1] + assert sql("SELECT id FROM comments") == [["c"]] + end + test "a table that stops being AUTOINCREMENT and becomes one again has one row", %{ctx: ctx} do write_migration(ctx, 1, "create_things", """ def up do From 77d071aeb4902c2be452b369a56f28b8916d3198 Mon Sep 17 00:00:00 2001 From: Nassim Date: Wed, 14 Oct 2026 17:00:01 +0200 Subject: [PATCH 15/15] fixup! improvement: rebuild tables sqlite cannot alter in place (opt-in) --- lib/migration_generator/operation.ex | 9 ------ test/table_rebuild_test.exs | 47 ---------------------------- 2 files changed, 56 deletions(-) diff --git a/lib/migration_generator/operation.ex b/lib/migration_generator/operation.ex index 9b7202a..d1b2b04 100644 --- a/lib/migration_generator/operation.ex +++ b/lib/migration_generator/operation.ex @@ -861,7 +861,6 @@ defmodule AshSqlite.MigrationGenerator.Operation do def requires_rebuild?(%Operation.AlterAttribute{}), do: true def requires_rebuild?(%Operation.DropForeignKey{}), do: true - def requires_rebuild?(%Operation.AlterDeferrability{}), do: true def requires_rebuild?(%Operation.RemovePrimaryKey{}), do: true def requires_rebuild?(%Operation.Omitted{}), do: true def requires_rebuild?(%Operation.AlterStrict{}), do: true @@ -1192,14 +1191,6 @@ defmodule AshSqlite.MigrationGenerator.Operation do defp reason(%Operation.RemovePrimaryKey{}), do: "changing the primary key" - defp reason(%Operation.AlterDeferrability{references: %{name: name, deferrable: deferrable}}) - when deferrable not in [false, nil], - do: - "NOT APPLIED: the foreign key #{name} is deferrable " <> - "(SQLite migrations cannot create DEFERRABLE constraints)" - - defp reason(%Operation.AlterDeferrability{}), do: nil - defp quote_name(name), do: ~s("#{name}") defp default_sql(nil), do: nil diff --git a/test/table_rebuild_test.exs b/test/table_rebuild_test.exs index e3268e3..1eb3dd3 100644 --- a/test/table_rebuild_test.exs +++ b/test/table_rebuild_test.exs @@ -487,53 +487,6 @@ defmodule AshSqlite.TableRebuildTest do end end - describe "deferrable foreign keys" do - test "a deferrable reference next to a rebuild says it is not applied", %{ctx: ctx} do - defres Parent, "parents" do - attributes do - uuid_primary_key(:id) - end - end - - defres Child, "children" do - attributes do - uuid_primary_key(:id) - end - - relationships do - belongs_to(:parent, Parent) - end - end - - defdomain([Parent, Child]) - generate(Domain, ctx) - migrate(ctx) - - defres Child, "children" do - attributes do - uuid_primary_key(:id) - attribute(:note, :string, allow_nil?: false) - end - - relationships do - belongs_to(:parent, Parent) - end - - sqlite do - references do - reference(:parent, deferrable: :initially) - end - end - end - - generate(Domain, ctx) - migration = last_migration(ctx) - assert migration =~ "NOT APPLIED: the foreign key children_parent_id_fkey is deferrable" - migrate(ctx) - assert fk_violations() == [] - end - end - # Two versions of the resource on "items": `name` required, then optional. defmacrop items_v1 do quote do