From c0f7f6d299100b46c87cc658fb1a9acc18ac71d6 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Fri, 21 Aug 2026 19:32:06 +0100 Subject: [PATCH 1/8] improvement: allow write transactions per resource via `write_transactions?` `can?(:transact)` has returned false since this data layer was split out of ash_postgres, and the transactions guide explains why: SQLite allows one write lock at a time, and a write attempted while another transaction holds that lock fails immediately rather than queueing. That is a reason to make transactions opt in, not a reason to leave them unimplemented. SQLite is fully ACID, and without a transaction a create whose `after_action` hook fails leaves its row behind with nothing to undo it. Adds `write_transactions?` to the `sqlite` section, defaulting to false so nothing changes for existing resources, and implements the callbacks it enables: * `transaction/4` opens write transactions as `BEGIN IMMEDIATE`. This is what makes `busy_timeout` effective. A deferred transaction takes no lock until its first write, so a read-then-write has to upgrade partway through -- and SQLite cannot make an upgrade wait, because the snapshot already read from may be stale by the time the lock frees. It fails immediately regardless of `busy_timeout`. `BEGIN IMMEDIATE` has nothing to upgrade. Read-only transactions stay deferred, since they never take the write lock. * `in_transaction?/1` answers rather than raising when the repo has no running process. `Ecto.Repo.in_transaction?/0` resolves the current dynamic repo through the registry and raises when it is absent, which happens whenever the repo was reached through `put_dynamic_repo/1` and started under no name of its own. Ash asks this before opening a transaction, so it has to be an answer. * `prefer_transaction_for_atomic_updates?/1` is false. An atomic update is a single statement and so already atomic; wrapping it would hold the one write lock across the surrounding work and buy nothing. Refs #91, and supersedes the work in #95. Two notes on what is deliberately not here. The option composes with functional repos rather than replacing them: the read/write pool split discussed in #91 is `repo fn _, :mutate -> WriteRepo; _, :read -> ReadRepo end` plus this flag on the write side, and the guide documents the pair. There is no verifier rejecting `transaction? true` on a resource that has not opted in, which #91 proposed and #95 implemented. It cannot work as specified: Ash *derives* `transaction? true` from the action rather than only taking it from the author -- an action using `manage_relationship` gets it automatically -- so the check fires on resources nobody annotated. Compiling it against this suite flags four existing test resources (Comment, Device, Manager, Post), none of which mentions `transaction?` at all, and the error asks the author to change an action they did not write. On a released version it would break existing applications at compile time on upgrade. Discoverability is handled in the guide instead. --- .formatter.exs | 3 +- documentation/dsls/DSL-AshSqlite.DataLayer.md | 1 + .../topics/about-ash-sqlite/transactions.md | 39 +++++++++++ lib/data_layer.ex | 60 ++++++++++++++++- lib/data_layer/info.ex | 5 ++ test/support/domain.ex | 1 + .../resources/transactional_account.ex | 40 +++++++++++ test/transaction_test.exs | 67 +++++++++++++++++++ 8 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 test/support/resources/transactional_account.ex create mode 100644 test/transaction_test.exs diff --git a/.formatter.exs b/.formatter.exs index 439399a..06ae737 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -39,7 +39,8 @@ spark_locals_without_parens = [ unique_index_names: 1, up: 1, using: 1, - where: 1 + where: 1, + write_transactions?: 1 ] [ diff --git a/documentation/dsls/DSL-AshSqlite.DataLayer.md b/documentation/dsls/DSL-AshSqlite.DataLayer.md index 184224b..0205a32 100644 --- a/documentation/dsls/DSL-AshSqlite.DataLayer.md +++ b/documentation/dsls/DSL-AshSqlite.DataLayer.md @@ -36,6 +36,7 @@ end | Name | Type | Default | Docs | |------|------|---------|------| | [`repo`](#sqlite-repo){: #sqlite-repo .spark-required} | `module \| (any, any -> any)` | | The repo that will be used to fetch your data. See the `AshSqlite.Repo` documentation for more. Can also be a function that takes a resource and a type `:read \| :mutate` and returns the repo. | +| [`write_transactions?`](#sqlite-write_transactions?){: #sqlite-write_transactions? } | `boolean` | `false` | Whether Ash may wrap this resource's write actions in a transaction. Off by default. SQLite allows a single write lock at a time and a contended write fails immediately rather than queueing, so transactions are only safe once the repo is configured for them. See the [transactions guide](/documentation/topics/about-ash-sqlite/transactions.md). With this on, write transactions are opened as `BEGIN IMMEDIATE` so that `busy_timeout` can do its job. | | [`migrate?`](#sqlite-migrate?){: #sqlite-migrate? } | `boolean` | `true` | Whether or not to include this resource in the generated migrations with `mix ash.generate_migrations` | | [`migration_types`](#sqlite-migration_types){: #sqlite-migration_types } | `keyword` | `[]` | A keyword list of attribute names to the ecto migration type that should be used for that attribute. Only necessary if you need to override the defaults. | | [`migration_defaults`](#sqlite-migration_defaults){: #sqlite-migration_defaults } | `keyword` | `[]` | A keyword list of attribute names to the ecto migration default that should be used for that attribute. The string you use will be placed verbatim in the migration. Use fragments like `fragment(\\"now()\\")`, or for `nil`, use `\\"nil\\"`. | diff --git a/documentation/topics/about-ash-sqlite/transactions.md b/documentation/topics/about-ash-sqlite/transactions.md index af4ac6f..f83e000 100644 --- a/documentation/topics/about-ash-sqlite/transactions.md +++ b/documentation/topics/about-ash-sqlite/transactions.md @@ -17,6 +17,45 @@ Because of this, **AshSqlite disables transaction support by default** (`can?(:transact)` returns `false`). Without extra configuration, Ash will not wrap actions in transactions when using the SQLite data layer. +## Enabling Transactions + +Transactions are opt in per resource, via `write_transactions?` in the `sqlite` +block: + +```elixir +sqlite do + table "accounts" + repo MyApp.Repo + write_transactions? true +end +``` + +Turning this on is worth it wherever an action does more than one thing. Without +a transaction, a create whose `after_action` hook fails leaves its record behind: +the insert already committed on its own, and there is nothing to undo it. With +one, the failure rolls the insert back. + +Read it as a statement about the *repo*, not just the resource — a resource only +transacts safely once the repo underneath it is configured as below. Leaving it +off is not a bug, and it stays the default so that existing applications are +unaffected. + +> ### Transactions are opened as IMMEDIATE {: .info} +> +> When a write transaction is opened, AshSqlite issues `BEGIN IMMEDIATE` rather +> than letting it default to deferred, whatever `default_transaction_mode` is set +> to. This is what makes `busy_timeout` effective for transactions that read +> before they write. +> +> A deferred transaction takes no lock until its first write, so a +> read-then-write has to *upgrade* to the write lock partway through. SQLite +> cannot make an upgrade wait: the snapshot the transaction already read from may +> be stale by the time the lock frees, so it fails immediately no matter how long +> `busy_timeout` is. `BEGIN IMMEDIATE` takes the lock up front, and has nothing to +> upgrade. +> +> Read-only transactions stay deferred, since they never take the write lock. + ## Enabling Reliable Concurrent Writes `ecto_sqlite3` exposes two knobs that together make concurrent writes behave more diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 567eca2..48a6c0a 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -203,6 +203,21 @@ defmodule AshSqlite.DataLayer do doc: "The repo that will be used to fetch your data. See the `AshSqlite.Repo` documentation for more. Can also be a function that takes a resource and a type `:read | :mutate` and returns the repo." ], + write_transactions?: [ + type: :boolean, + default: false, + doc: """ + Whether Ash may wrap this resource's write actions in a transaction. + + Off by default. SQLite allows a single write lock at a time and a + contended write fails immediately rather than queueing, so transactions + are only safe once the repo is configured for them. See the + [transactions guide](/documentation/topics/about-ash-sqlite/transactions.md). + + With this on, write transactions are opened as `BEGIN IMMEDIATE` so that + `busy_timeout` can do its job. + """ + ], migrate?: [ type: :boolean, default: true, @@ -447,7 +462,7 @@ defmodule AshSqlite.DataLayer do def can?(_, :destroy_query), do: true def can?(_, {:lock, _}), do: false - def can?(_, :transact), do: false + def can?(resource, :transact), do: AshSqlite.DataLayer.Info.write_transactions?(resource) def can?(_, :composite_primary_key), do: true def can?(_, {:atomic, :update}), do: true def can?(_, {:atomic, :upsert}), do: true @@ -2104,6 +2119,49 @@ defmodule AshSqlite.DataLayer do %{query | __ash_bindings__: new_ash_bindings} end + @impl true + def in_transaction?(resource) do + repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + + # Ash asks this before opening a transaction, and the answer has to be an + # answer rather than an exception. `Ecto.Repo.in_transaction?/0` resolves the + # current dynamic repo through the registry and raises when it is not there, + # which happens whenever the repo was reached through + # `c:Ecto.Repo.put_dynamic_repo/1` and started under no name of its own. No + # running repo means no open transaction. + case repo.get_dynamic_repo() do + pid when is_pid(pid) -> repo.in_transaction?() + name when is_atom(name) -> !is_nil(GenServer.whereis(name)) and repo.in_transaction?() + end + end + + # An atomic update is a single statement, so it is already atomic. Wrapping it + # would hold SQLite's one write lock across the surrounding work and buy nothing. + @impl true + def prefer_transaction_for_atomic_updates?(_resource), do: false + + @impl true + def transaction(resource, func, timeout \\ nil, reason \\ %{type: :custom, metadata: %{}}) do + repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + + # A read takes no write lock, so it stays deferred. Anything that may write + # takes the write lock up front, because a deferred transaction that reads and + # then writes has to *upgrade* its lock -- and SQLite cannot make an upgrade + # wait for `busy_timeout`, since the snapshot the transaction already read from + # may be stale by the time the lock frees. It fails immediately instead. + # `BEGIN IMMEDIATE` has nothing to upgrade, so `busy_timeout` applies. + mode = if reason[:type] == :read, do: :deferred, else: :immediate + + opts = + case timeout do + nil -> [mode: mode] + :infinity -> [mode: mode] + timeout -> [mode: mode, timeout: timeout] + end + + repo.transaction(func, opts) + end + @impl true def rollback(resource, term) do AshSqlite.DataLayer.Info.repo(resource, :mutate).rollback(term) diff --git a/lib/data_layer/info.ex b/lib/data_layer/info.ex index af5f92b..96507a8 100644 --- a/lib/data_layer/info.ex +++ b/lib/data_layer/info.ex @@ -18,6 +18,11 @@ defmodule AshSqlite.DataLayer.Info do end end + @doc "Whether Ash may wrap this resource's write actions in a transaction" + def write_transactions?(resource) do + Extension.get_opt(resource, [:sqlite], :write_transactions?, false, true) + end + @doc "The configured table for a resource" def table(resource) do Extension.get_opt(resource, [:sqlite], :table, nil, true) diff --git a/test/support/domain.ex b/test/support/domain.ex index d55e541..69c8860 100644 --- a/test/support/domain.ex +++ b/test/support/domain.ex @@ -17,6 +17,7 @@ defmodule AshSqlite.Test.Domain do resource(AshSqlite.Test.Profile) resource(AshSqlite.Test.User) resource(AshSqlite.Test.Account) + resource(AshSqlite.Test.TransactionalAccount) resource(AshSqlite.Test.Organization) resource(AshSqlite.Test.Manager) resource(AshSqlite.Test.Device) diff --git a/test/support/resources/transactional_account.ex b/test/support/resources/transactional_account.ex new file mode 100644 index 0000000..dd26980 --- /dev/null +++ b/test/support/resources/transactional_account.ex @@ -0,0 +1,40 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.TransactionalAccount do + @moduledoc """ + Shares the `accounts` table with `AshSqlite.Test.Account`, with transactions on. + + Sharing the table is the point: the two resources differ only in + `write_transactions?`, so a test can show the same failure either rolling back or + leaving its row behind. + """ + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + default_accept(:*) + defaults([:create, :read, :update, :destroy]) + + create :create_then_fail do + accept([:is_active]) + + change( + after_action(fn _changeset, _record, _context -> + {:error, Ash.Error.Changes.InvalidAttribute.exception(field: :is_active, message: "no")} + end) + ) + end + end + + attributes do + uuid_primary_key(:id) + attribute(:is_active, :boolean, public?: true) + end + + sqlite do + table("accounts") + repo(AshSqlite.TestRepo) + write_transactions?(true) + end +end diff --git a/test/transaction_test.exs b/test/transaction_test.exs new file mode 100644 index 0000000..1fbca08 --- /dev/null +++ b/test/transaction_test.exs @@ -0,0 +1,67 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.TransactionTest do + @moduledoc """ + Write transactions are off by default, and roll back when turned on. + + `AshSqlite.Test.Account` and `AshSqlite.Test.TransactionalAccount` share a table + and differ only in `write_transactions?`, which is what makes the pair worth + testing together: the contrast is the feature. + """ + use AshSqlite.RepoCase, async: false + + alias AshSqlite.Test.{Account, TransactionalAccount} + + require Ash.Query + + test "transactions are off unless the resource asks for them" do + refute Ash.DataLayer.data_layer_can?(Account, :transact) + assert Ash.DataLayer.data_layer_can?(TransactionalAccount, :transact) + end + + test "a failing multi-step action rolls back" do + assert {:error, _} = + TransactionalAccount + |> Ash.Changeset.for_create(:create_then_fail, %{is_active: true}) + |> Ash.create() + + assert [] = Ash.read!(TransactionalAccount) + end + + test "a succeeding action still commits" do + assert {:ok, account} = + TransactionalAccount + |> Ash.Changeset.for_create(:create, %{is_active: true}) + |> Ash.create() + + assert [%{id: id}] = Ash.read!(TransactionalAccount) + assert id == account.id + end + + test "without transactions the same failure leaves the row behind" do + # Not an endorsement, just the contrast: this is what every AshSqlite resource + # does today, and it is why the option is worth having. + assert {:error, _} = + Account + |> Ash.Changeset.for_create(:create, %{is_active: true}) + |> Ash.Changeset.after_action(fn _changeset, _record -> + {:error, Ash.Error.Changes.InvalidAttribute.exception(field: :is_active)} + end) + |> Ash.create() + + assert [_] = Ash.read!(Account) + end + + test "in_transaction?/1 answers rather than raising when no repo is bound" do + refute AshSqlite.DataLayer.in_transaction?(TransactionalAccount) + end + + test "reports being in a transaction from inside one" do + assert {:ok, true} = + AshSqlite.TestRepo.transaction(fn -> + AshSqlite.DataLayer.in_transaction?(TransactionalAccount) + end) + end +end From e059cb3da5e3329bad2988bc7cafeb4aeedcb7fb Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Mon, 24 Aug 2026 07:35:25 +0100 Subject: [PATCH 2/8] chore: require ash 3.32.1 so mutation actions report the transaction they get Ash now clears the derived `transaction? true` on a resource whose data layer cannot transact, so a resource without `write_transactions?` reflects `transaction? false` instead of naming a transaction that never opens. --- documentation/topics/about-ash-sqlite/transactions.md | 7 +++++++ mix.exs | 2 +- mix.lock | 4 ++-- test/transaction_test.exs | 7 +++++++ 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/documentation/topics/about-ash-sqlite/transactions.md b/documentation/topics/about-ash-sqlite/transactions.md index f83e000..6831920 100644 --- a/documentation/topics/about-ash-sqlite/transactions.md +++ b/documentation/topics/about-ash-sqlite/transactions.md @@ -35,6 +35,13 @@ a transaction, a create whose `after_action` hook fails leaves its record behind the insert already committed on its own, and there is nothing to undo it. With one, the failure rolls the insert back. +Ash derives `transaction? true` on create, update and destroy actions, and then +clears it again on a resource whose data layer cannot transact. So on a resource +that has not opted in, `Ash.Resource.Info.action(MyApp.Post, :create).transaction?` +reads `false` and says what will really happen, rather than naming a transaction +the data layer was never going to open. Turning `write_transactions?` on is what +lets that default stand. + Read it as a statement about the *repo*, not just the resource — a resource only transacts safely once the repo underneath it is configured as below. Leaving it off is not a bug, and it stays the default so that existing applications are diff --git a/mix.exs b/mix.exs index 68c0008..ba0118a 100644 --- a/mix.exs +++ b/mix.exs @@ -148,7 +148,7 @@ defmodule AshSqlite.MixProject do {:ecto_libsql, "~> 0.9", optional: true}, {:ecto, "~> 3.13"}, {:jason, "~> 1.0"}, - {:ash, ash_version("~> 3.19")}, + {:ash, ash_version("~> 3.19 and >= 3.32.1")}, {:ash_sql, ash_sql_version("~> 0.2 and >= 0.6.9")}, {:igniter, "~> 0.6 and >= 0.6.14", optional: true}, {:simple_sat, ">= 0.0.0", only: [:dev, :test]}, diff --git a/mix.lock b/mix.lock index 5e57c96..8f6697b 100644 --- a/mix.lock +++ b/mix.lock @@ -1,5 +1,5 @@ %{ - "ash": {:hex, :ash, "3.31.3", "b36672bff745eadd52265d8b0a303f98ba26a31173f4116ebd2df7be0cbafeff", [:mix], [{:crux, ">= 0.1.2 and < 1.0.0-0", [hex: :crux, repo: "hexpm", optional: false]}, {:decimal, "~> 2.0 or ~> 3.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:ecto, "~> 3.14", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8", [hex: :ets, repo: "hexpm", optional: false]}, {:igniter, ">= 0.6.29 and < 1.0.0-0", [hex: :igniter, repo: "hexpm", optional: true]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.2", [hex: :picosat_elixir, repo: "hexpm", optional: true]}, {:plug, ">= 0.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:reactor, "~> 1.0", [hex: :reactor, repo: "hexpm", optional: false]}, {:simple_sat, ">= 0.1.1 and < 1.0.0-0", [hex: :simple_sat, repo: "hexpm", optional: true]}, {:spark, ">= 2.6.0", [hex: :spark, repo: "hexpm", optional: false]}, {:splode, "~> 0.3", [hex: :splode, repo: "hexpm", optional: false]}, {:stream_data, "~> 1.0", [hex: :stream_data, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.1", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "4c105da2080475d114908c32e70970e69c857a9068a577a4e99dbf2801baa11e"}, + "ash": {:hex, :ash, "3.32.1", "6faf61c06c3212fbd7f1bcd2276d94773714a2e1494db1a37227d67ecb4fb4f8", [:mix], [{:crux, ">= 0.1.2 and < 1.0.0-0", [hex: :crux, repo: "hexpm", optional: false]}, {:decimal, "~> 2.0 or ~> 3.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:ecto, "~> 3.14", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8", [hex: :ets, repo: "hexpm", optional: false]}, {:igniter, ">= 0.6.29 and < 1.0.0-0", [hex: :igniter, repo: "hexpm", optional: true]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.2", [hex: :picosat_elixir, repo: "hexpm", optional: true]}, {:plug, ">= 0.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:reactor, "~> 1.0", [hex: :reactor, repo: "hexpm", optional: false]}, {:simple_sat, ">= 0.1.1 and < 1.0.0-0", [hex: :simple_sat, repo: "hexpm", optional: true]}, {:spark, ">= 2.6.0", [hex: :spark, repo: "hexpm", optional: false]}, {:splode, "~> 0.3", [hex: :splode, repo: "hexpm", optional: false]}, {:stream_data, "~> 1.0", [hex: :stream_data, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.1", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "b5de7127c1582be578efff3b959e775cafff3e82b4e8771323984064f514abb2"}, "ash_sql": {:hex, :ash_sql, "0.6.9", "618143050c39736580677629859d003f5ea84f9898944b1b43a0c593771f8863", [:mix], [{:ash, ">= 3.24.5 and < 4.0.0-0", [hex: :ash, repo: "hexpm", optional: false]}, {:ecto, ">= 3.13.4 and < 4.0.0-0", [hex: :ecto, repo: "hexpm", optional: false]}, {:ecto_sql, "~> 3.9", [hex: :ecto_sql, repo: "hexpm", optional: false]}], "hexpm", "be52eb90ee1e552c1469dcf84afc26acfc0c1a232897cada02bf19bf23a38cf1"}, "bunt": {:hex, :bunt, "1.0.0", "081c2c665f086849e6d57900292b3a161727ab40431219529f13c4ddcf3e7a44", [:mix], [], "hexpm", "dc5f86aa08a5f6fa6b8096f0735c4e76d54ae5c9fa2c143e5a1fc7c1cd9bb6b5"}, "cc_precompiler": {:hex, :cc_precompiler, "0.1.11", "8c844d0b9fb98a3edea067f94f616b3f6b29b959b6b3bf25fee94ffe34364768", [:mix], [{:elixir_make, "~> 0.7", [hex: :elixir_make, repo: "hexpm", optional: false]}], "hexpm", "3427232caf0835f94680e5bcf082408a70b48ad68a5f5c0b02a3bea9f3a075b9"}, @@ -41,7 +41,7 @@ "nimble_pool": {:hex, :nimble_pool, "1.1.0", "bf9c29fbdcba3564a8b800d1eeb5a3c58f36e1e11d7b7fb2e084a643f645f06b", [:mix], [], "hexpm", "af2e4e6b34197db81f7aad230c1118eac993acc0dae6bc83bac0126d4ae0813a"}, "owl": {:hex, :owl, "0.13.1", "1ec4a5dea170465f0e90c502c203079224516bc0cbd599281c8667b3c6ef8848", [:mix], [{:ucwidth, "~> 0.2", [hex: :ucwidth, repo: "hexpm", optional: true]}], "hexpm", "351e768af8f2edc575cdaab1a5a2f6d6381be591758a026c701c703145508a0c"}, "reactor": {:hex, :reactor, "1.0.6", "546a87255693bcee99451d022cc86927161cdb527f5216d4dcac2b31e08eb122", [:mix], [{:igniter, "~> 0.4", [hex: :igniter, repo: "hexpm", optional: true]}, {:iterex, "~> 0.1", [hex: :iterex, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:multigraph, "~> 0.16.1-mg.2", [hex: :multigraph, repo: "hexpm", optional: false]}, {:spark, ">= 2.3.3 and < 3.0.0-0", [hex: :spark, repo: "hexpm", optional: false]}, {:splode, "~> 0.2", [hex: :splode, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.2", [hex: :telemetry, repo: "hexpm", optional: false]}, {:yaml_elixir, "~> 2.11", [hex: :yaml_elixir, repo: "hexpm", optional: false]}, {:ymlr, "~> 5.0", [hex: :ymlr, repo: "hexpm", optional: false]}], "hexpm", "e3f8fd8e870c2b011316ca2ac422bb4bc710cac9f02c578f8526f7a6348d932b"}, - "req": {:hex, :req, "0.7.2", "364eae2e5f5c984f2dac6d71c07f8c8c89ce0bc49c4d746dacb7a306823020de", [:mix], [{:brotli, "~> 0.3.1", [hex: :brotli, repo: "hexpm", optional: true]}, {:finch, "~> 0.21", [hex: :finch, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:mime, "~> 2.0.6 or ~> 2.1", [hex: :mime, repo: "hexpm", optional: false]}, {:nimble_csv, "~> 1.0", [hex: :nimble_csv, repo: "hexpm", optional: true]}, {:plug, "~> 1.0", [hex: :plug, repo: "hexpm", optional: true]}], "hexpm", "c9cdfa276b05d8db2a27fda5d233e6858b764d47189d76cbb186e130a871ae0b"}, + "req": {:hex, :req, "0.7.3", "b141f1b465dabc5fb8ce67bd2f15a85fc80f6c75719910699560e32ce49f62ef", [:mix], [{:brotli, "~> 0.3.1", [hex: :brotli, repo: "hexpm", optional: true]}, {:finch, "~> 0.21", [hex: :finch, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:mime, "~> 2.0.6 or ~> 2.1", [hex: :mime, repo: "hexpm", optional: false]}, {:nimble_csv, "~> 1.0", [hex: :nimble_csv, repo: "hexpm", optional: true]}, {:plug, "~> 1.0", [hex: :plug, repo: "hexpm", optional: true]}], "hexpm", "73b303030dccc2b6d023ee5ada380825ab3a7cd3863aead493db09ec420ffdf2"}, "rewrite": {:hex, :rewrite, "1.3.0", "67448ba7975690b35ba7e7f35717efcce317dbd5963cb0577aa7325c1923121a", [:mix], [{:glob_ex, "~> 0.1", [hex: :glob_ex, repo: "hexpm", optional: false]}, {:sourceror, "~> 1.0", [hex: :sourceror, repo: "hexpm", optional: false]}, {:text_diff, "~> 0.1", [hex: :text_diff, repo: "hexpm", optional: false]}], "hexpm", "d111ac7ff3a58a802ef4f193bbd1831e00a9c57b33276e5068e8390a212714a5"}, "rustler_precompiled": {:hex, :rustler_precompiled, "0.9.0", "3a052eda09f3d2436364645cc1f13279cf95db310eb0c17b0d8f25484b233aa0", [:mix], [{:rustler, "~> 0.23", [hex: :rustler, repo: "hexpm", optional: true]}], "hexpm", "471d97315bd3bf7b64623418b3693eedd8e47de3d1cb79a0ac8f9da7d770d94c"}, "simple_sat": {:hex, :simple_sat, "0.1.4", "39baf72cdca14f93c0b6ce2b6418b72bbb67da98fa9ca4384e2f79bbc299899d", [:mix], [], "hexpm", "3569b68e346a5fd7154b8d14173ff8bcc829f2eb7b088c30c3f42a383443930b"}, diff --git a/test/transaction_test.exs b/test/transaction_test.exs index 1fbca08..bd2fa5b 100644 --- a/test/transaction_test.exs +++ b/test/transaction_test.exs @@ -21,6 +21,13 @@ defmodule AshSqlite.TransactionTest do assert Ash.DataLayer.data_layer_can?(TransactionalAccount, :transact) end + test "a mutation action reports the transaction it will actually get" do + # Ash derives `transaction? true` on mutations, then clears it on a resource + # whose data layer cannot transact, so reflection matches runtime behaviour. + refute Ash.Resource.Info.action(Account, :create).transaction? + assert Ash.Resource.Info.action(TransactionalAccount, :create).transaction? + end + test "a failing multi-step action rolls back" do assert {:error, _} = TransactionalAccount From 8d4cde28b6d705455022f09e665d35f1591c5973 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Mon, 24 Aug 2026 10:32:27 +0100 Subject: [PATCH 3/8] chore: trim redundant test commentary --- test/transaction_test.exs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/transaction_test.exs b/test/transaction_test.exs index bd2fa5b..e513411 100644 --- a/test/transaction_test.exs +++ b/test/transaction_test.exs @@ -14,16 +14,14 @@ defmodule AshSqlite.TransactionTest do alias AshSqlite.Test.{Account, TransactionalAccount} - require Ash.Query - test "transactions are off unless the resource asks for them" do refute Ash.DataLayer.data_layer_can?(Account, :transact) assert Ash.DataLayer.data_layer_can?(TransactionalAccount, :transact) end test "a mutation action reports the transaction it will actually get" do - # Ash derives `transaction? true` on mutations, then clears it on a resource - # whose data layer cannot transact, so reflection matches runtime behaviour. + # Ash derives `transaction? true` on mutations, then clears it again when the + # data layer cannot transact — neither resource says anything about it. refute Ash.Resource.Info.action(Account, :create).transaction? assert Ash.Resource.Info.action(TransactionalAccount, :create).transaction? end @@ -48,8 +46,6 @@ defmodule AshSqlite.TransactionTest do end test "without transactions the same failure leaves the row behind" do - # Not an endorsement, just the contrast: this is what every AshSqlite resource - # does today, and it is why the option is worth having. assert {:error, _} = Account |> Ash.Changeset.for_create(:create, %{is_active: true}) From b3722454a01829151506d74c52c6e1676458cd76 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Mon, 24 Aug 2026 15:29:51 +0100 Subject: [PATCH 4/8] improvement: support `strategy :context` multitenancy via a tenant binder Closes the data layer half of #127. Turning on `strategy :context` currently fails at compile time with `Data layer does not support multitenancy`. The documented workaround -- setting `%{data_layer: %{repo: ...}}` in a change and a preparation -- does not work. Both the read path (`repo.all/2`) and the write path (`repo.insert_all/3`, via `AshSql.dynamic_repo/3`) invoke the result *as a module*, so passing an instance raises `ArgumentError: Modules (the first argument of apply) must always be an atom`. That override selects between repo *modules*; database-per-tenant needs an *instance*, and binding an instance is Ecto's job via `put_dynamic_repo/1`. ## What context multitenancy means for SQLite One database file per tenant. There is no schema to prefix, so the generated SQL is identical for every tenant and isolation comes from which file the connection is attached to. `set_tenant/3` is therefore a no-op on the query, and the tenant is deliberately not passed to `AshSql.repo_opts/5` -- it reached Ecto as a table prefix and raised `SQLite3 does not support table prefixes` on every write. ## The binder `tenant_binder` names a module implementing `AshSqlite.TenantBinder`, which is asked for a connection once per statement: sqlite do table "posts" repo MyApp.Repo tenant_binder MyApp.TenantBinder end This PR ships the seam and nothing behind it -- there is no default binder, and `strategy :context` without one is refused. A managed runtime that supplies one is a follow-up, so that this can be reviewed on its own. ## Why the data layer rather than the caller Every entry point would otherwise have to call `put_dynamic_repo/1` before Ash runs, and some cannot: - `Ash.count/2` never enters `Ash.Actions.Read`, so no preparation or `around_transaction` hook runs for it. - Ash calls `atomic/3` rather than `change/3` whenever it can build one statement, so a hook-installing change forces `require_atomic? false`. - The binding is ambient, so it does not survive `Task.async`, an `Ash.load` fan-out, or a background job. `bind/3` also receives the resource and a `usage` of `:read`, `:write` or `:transaction`. Only the data layer can say which, and it is what lets a binder serve reads from a replica while writes go to the owner. ## Interaction with transactions `transaction/4` never receives the tenant: Ash calls it above the data layer and the reason it builds does not name one. `AshSqlite.Transformers.CarryTenant` adds a change that puts it in the changeset context, implementing `atomic/3` as well as `change/3` so it does not force actions off the atomic path. A transaction cannot span two tenants -- separate files on separate connections, and SQLite cannot commit atomically across databases in WAL mode even with `ATTACH`. A statement for another tenant inside an open transaction is refused rather than committing on its own and surviving the rollback around it. ## global? `global? true` is honoured: such a resource is not required to carry a tenant. Ecto binds per repo *module*, though, so a global resource sharing a repo with tenanted ones reads from whichever tenant the process last bound. The tests say so rather than leaving it to be discovered. ## Documentation Deliberately none in this PR. The guide is being written once the whole feature has landed, rather than in pieces that contradict each other. --- .formatter.exs | 1 + config/config.exs | 13 + documentation/dsls/DSL-AshSqlite.DataLayer.md | 1 + lib/changes/carry_tenant.ex | 30 ++ lib/data_layer.ex | 231 ++++++++++++- lib/data_layer/info.ex | 10 + lib/tenant_binder.ex | 27 ++ lib/transformers/carry_tenant.ex | 42 +++ lib/verifiers/verify_tenant_binder.ex | 39 +++ mix.exs | 3 + test/multitenancy_test.exs | 304 ++++++++++++++++++ test/support/domain.ex | 2 + test/support/resources/global_post.ex | 36 +++ test/support/resources/tenanted_post.ex | 36 +++ test/support/tenant_binder.ex | 42 +++ test/support/tenant_repo.ex | 10 + 16 files changed, 814 insertions(+), 13 deletions(-) create mode 100644 lib/changes/carry_tenant.ex create mode 100644 lib/tenant_binder.ex create mode 100644 lib/transformers/carry_tenant.ex create mode 100644 lib/verifiers/verify_tenant_binder.ex create mode 100644 test/multitenancy_test.exs create mode 100644 test/support/resources/global_post.ex create mode 100644 test/support/resources/tenanted_post.ex create mode 100644 test/support/tenant_binder.ex create mode 100644 test/support/tenant_repo.ex diff --git a/.formatter.exs b/.formatter.exs index 06ae737..157a11d 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -35,6 +35,7 @@ spark_locals_without_parens = [ statement: 2, strict?: 1, table: 1, + tenant_binder: 1, unique: 1, unique_index_names: 1, up: 1, diff --git a/config/config.exs b/config/config.exs index 7c2c12f..053f3ea 100644 --- a/config/config.exs +++ b/config/config.exs @@ -25,6 +25,11 @@ if Mix.env() == :test do config :ash, :validate_domain_resource_inclusion?, false config :ash, :validate_domain_config_inclusion?, false + # The sandbox runs every test inside a transaction, so Ash's warning about + # `before_transaction` hooks under an open transaction fires on tenanted actions + # that are perfectly fine outside the suite. + config :ash, :warn_on_transaction_hooks?, false + config :ash_sqlite, AshSqlite.TestRepo, database: Path.join(__DIR__, "../test/test.db"), pool_size: 1, @@ -39,6 +44,14 @@ if Mix.env() == :test do pool: Ecto.Adapters.SQL.Sandbox, migration_primary_key: [name: :id, type: :binary_id] + # No `database:` and no sandbox: the multitenancy tests start an instance of this + # per tenant, each against its own file, and bind it with `put_dynamic_repo/1`. + # Deliberately absent from `ecto_repos:` -- there is nothing to migrate centrally. + config :ash_sqlite, AshSqlite.TenantRepo, + pool_size: 1, + migration_lock: false, + migration_primary_key: [name: :id, type: :binary_id] + config :ash_sqlite, ecto_repos: [AshSqlite.TestRepo, AshSqlite.DevTestRepo], ash_domains: [ diff --git a/documentation/dsls/DSL-AshSqlite.DataLayer.md b/documentation/dsls/DSL-AshSqlite.DataLayer.md index 0205a32..ae722b5 100644 --- a/documentation/dsls/DSL-AshSqlite.DataLayer.md +++ b/documentation/dsls/DSL-AshSqlite.DataLayer.md @@ -37,6 +37,7 @@ end |------|------|---------|------| | [`repo`](#sqlite-repo){: #sqlite-repo .spark-required} | `module \| (any, any -> any)` | | The repo that will be used to fetch your data. See the `AshSqlite.Repo` documentation for more. Can also be a function that takes a resource and a type `:read \| :mutate` and returns the repo. | | [`write_transactions?`](#sqlite-write_transactions?){: #sqlite-write_transactions? } | `boolean` | `false` | Whether Ash may wrap this resource's write actions in a transaction. Off by default. SQLite allows a single write lock at a time and a contended write fails immediately rather than queueing, so transactions are only safe once the repo is configured for them. See the [transactions guide](/documentation/topics/about-ash-sqlite/transactions.md). With this on, write transactions are opened as `BEGIN IMMEDIATE` so that `busy_timeout` can do its job. | +| [`tenant_binder`](#sqlite-tenant_binder){: #sqlite-tenant_binder } | `module` | | A module that selects the connection a tenanted statement runs on. Required for database-per-tenant layouts, where the tenant is a database file rather than a query prefix. See `AshSqlite.TenantBinder`. | | [`migrate?`](#sqlite-migrate?){: #sqlite-migrate? } | `boolean` | `true` | Whether or not to include this resource in the generated migrations with `mix ash.generate_migrations` | | [`migration_types`](#sqlite-migration_types){: #sqlite-migration_types } | `keyword` | `[]` | A keyword list of attribute names to the ecto migration type that should be used for that attribute. Only necessary if you need to override the defaults. | | [`migration_defaults`](#sqlite-migration_defaults){: #sqlite-migration_defaults } | `keyword` | `[]` | A keyword list of attribute names to the ecto migration default that should be used for that attribute. The string you use will be placed verbatim in the migration. Use fragments like `fragment(\\"now()\\")`, or for `nil`, use `\\"nil\\"`. | diff --git a/lib/changes/carry_tenant.ex b/lib/changes/carry_tenant.ex new file mode 100644 index 0000000..e0194cb --- /dev/null +++ b/lib/changes/carry_tenant.ex @@ -0,0 +1,30 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Changes.CarryTenant do + @moduledoc """ + Puts the tenant in `context[:data_layer]`, where `AshSqlite.DataLayer.transaction/4` can reach it. + """ + use Ash.Resource.Change + + @impl true + def change(changeset, _opts, _context) do + # Also as a before_transaction hook, because a tenant given to `Ash.create/3` + # rather than to `Ash.Changeset.for_create/4` is applied after global changes + # have run -- and those hooks run on the changeset the transaction is opened + # with. + changeset + |> put_tenant() + |> Ash.Changeset.before_transaction(&put_tenant/1) + end + + defp put_tenant(%{tenant: nil} = changeset), do: changeset + + defp put_tenant(changeset) do + Ash.Changeset.set_context(changeset, %{data_layer: %{tenant: changeset.tenant}}) + end + + @impl true + def atomic(changeset, _opts, _context), do: {:ok, changeset} +end diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 48a6c0a..7c1f311 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -218,6 +218,11 @@ defmodule AshSqlite.DataLayer do `busy_timeout` can do its job. """ ], + tenant_binder: [ + type: {:behaviour, AshSqlite.TenantBinder}, + doc: + "A module that selects the connection a tenanted statement runs on. Required for database-per-tenant layouts, where the tenant is a database file rather than a query prefix. See `AshSqlite.TenantBinder`." + ], migrate?: [ type: :boolean, default: true, @@ -321,7 +326,11 @@ defmodule AshSqlite.DataLayer do transformers: [ AshSqlite.Transformers.ValidateReferences, AshSqlite.Transformers.VerifyRepo, - AshSqlite.Transformers.EnsureTableOrPolymorphic + AshSqlite.Transformers.EnsureTableOrPolymorphic, + AshSqlite.Transformers.CarryTenant + ], + verifiers: [ + AshSqlite.Verifiers.VerifyTenantBinder ] def migrate(args) do @@ -503,7 +512,7 @@ defmodule AshSqlite.DataLayer do def can?(_, :filter), do: true def can?(_, :limit), do: true def can?(_, :offset), do: true - def can?(_, :multitenancy), do: false + def can?(_, :multitenancy), do: true def can?(_, {:filter_relationship, %{manual: {module, _}}}) do Spark.implements_behaviour?(module, AshSqlite.ManualRelationship) @@ -528,6 +537,18 @@ defmodule AshSqlite.DataLayer do def can?(_, {:sort, _}), do: true def can?(_, _), do: false + @impl true + @doc """ + A no-op on the query: with one database per tenant there is no prefix to set. + + The connection is chosen per statement by the resource's `tenant_binder` + instead. Declaring the strategy still earns its keep — it makes Ash accept the + resource and enforce "a tenant is required unless `global?`". + """ + def set_tenant(_resource, query, _tenant) do + {:ok, query} + end + @impl true def limit(query, nil, _), do: {:ok, query} @@ -577,16 +598,22 @@ defmodule AshSqlite.DataLayer do @impl true def run_aggregate_query(query, aggregates, resource) do - AshSql.AggregateQuery.run_aggregate_query( - query, - aggregates, - resource, - AshSqlite.SqlImplementation - ) + bind_tenant(resource, query_tenant(query), :read, fn -> + AshSql.AggregateQuery.run_aggregate_query( + query, + aggregates, + resource, + AshSqlite.SqlImplementation + ) + end) end @impl true def run_query(query, resource) do + bind_tenant(resource, query_tenant(query), :read, fn -> do_run_query(query, resource) end) + end + + defp do_run_query(query, resource) do with_sort_applied = if query.__ash_bindings__[:sort_applied?] do {:ok, query} @@ -650,6 +677,14 @@ defmodule AshSqlite.DataLayer do @impl true def bulk_create(resource, stream, options) do + stream = Enum.to_list(stream) + + bind_tenant(resource, changesets_tenant(stream), :write, fn -> + do_bulk_create(resource, stream, options) + end) + end + + defp do_bulk_create(resource, stream, options) do # Cell-wise default values are not supported on INSERT statements by SQLite # This requires that we group changesets by what attributes are changing # And *omit* any defaults instead of using something like `(1, 2, DEFAULT)` @@ -658,7 +693,7 @@ defmodule AshSqlite.DataLayer do |> Enum.group_by(&Map.keys(&1.attributes)) |> Enum.reduce_while({:ok, []}, fn {_, changesets}, {:ok, acc} -> repo = AshSql.dynamic_repo(resource, AshSqlite.SqlImplementation, Enum.at(changesets, 0)) - opts = AshSql.repo_opts(repo, AshSqlite.SqlImplementation, nil, options[:tenant], resource) + opts = AshSql.repo_opts(repo, AshSqlite.SqlImplementation, nil, nil, resource) opts = if options.return_records? do @@ -1360,6 +1395,10 @@ defmodule AshSqlite.DataLayer do @impl true def upsert(resource, changeset, keys \\ nil) do + bind_tenant(resource, changeset.tenant, :write, fn -> do_upsert(resource, changeset, keys) end) + end + + defp do_upsert(resource, changeset, keys) do keys = keys || Ash.Resource.Info.primary_key(keys) touch_update_defaults? = @@ -1508,6 +1547,10 @@ defmodule AshSqlite.DataLayer do @impl true def update(resource, changeset) do + bind_tenant(resource, changeset.tenant, :write, fn -> do_update(resource, changeset) end) + end + + defp do_update(resource, changeset) do source = resolve_source(resource, changeset) query = @@ -1564,6 +1607,12 @@ defmodule AshSqlite.DataLayer do @impl true def destroy(resource, %{data: record} = changeset) do + bind_tenant(resource, changeset.tenant, :write, fn -> + do_destroy(resource, record, changeset) + end) + end + + defp do_destroy(resource, record, changeset) do source = resolve_source(resource, changeset) query = @@ -1607,7 +1656,7 @@ defmodule AshSqlite.DataLayer do repo, AshSqlite.SqlImplementation, changeset.timeout, - changeset.tenant, + nil, changeset.resource ) @@ -1638,6 +1687,12 @@ defmodule AshSqlite.DataLayer do @impl true def update_query(query, changeset, resource, options) do + bind_tenant(resource, changeset.tenant, :write, fn -> + do_update_query(query, changeset, resource, options) + end) + end + + defp do_update_query(query, changeset, resource, options) do repo = AshSql.dynamic_repo(resource, AshSqlite.SqlImplementation, changeset) ecto_changeset = @@ -1668,7 +1723,7 @@ defmodule AshSqlite.DataLayer do repo, AshSqlite.SqlImplementation, changeset.timeout, - changeset.tenant, + nil, changeset.resource ) @@ -1763,6 +1818,12 @@ defmodule AshSqlite.DataLayer do @impl true def destroy_query(query, changeset, resource, options) do + bind_tenant(resource, changeset.tenant, :write, fn -> + do_destroy_query(query, changeset, resource, options) + end) + end + + defp do_destroy_query(query, changeset, resource, options) do repo = AshSql.dynamic_repo(resource, AshSqlite.SqlImplementation, changeset) ecto_changeset = @@ -1794,7 +1855,7 @@ defmodule AshSqlite.DataLayer do repo, AshSqlite.SqlImplementation, changeset.timeout, - changeset.tenant, + nil, changeset.resource ) @@ -2144,6 +2205,25 @@ defmodule AshSqlite.DataLayer do def transaction(resource, func, timeout \\ nil, reason \\ %{type: :custom, metadata: %{}}) do repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + # Ash calls this above the data layer, so unlike every other callback there is + # no changeset here to read the tenant off. + tenant = reason_tenant(reason) + + if is_nil(tenant) and tenant_required?(resource) do + raise ArgumentError, """ + #{inspect(resource)} has a tenant_binder and `strategy :context`, but the \ + transaction about to be opened for this action carried no tenant. + + A transaction is opened on one connection, and for a database-per-tenant \ + layout that means one tenant's database. Ash forwards only the changeset's \ + data layer context to this callback, so the tenant has to travel in it: + + Ash.Changeset.set_context(changeset, %{data_layer: %{tenant: tenant}}) + + A global change on the resource is the usual place to do that. + """ + end + # A read takes no write lock, so it stays deferred. Anything that may write # takes the write lock up front, because a deferred transaction that reads and # then writes has to *upgrade* its lock -- and SQLite cannot make an upgrade @@ -2159,7 +2239,25 @@ defmodule AshSqlite.DataLayer do timeout -> [mode: mode, timeout: timeout] end - repo.transaction(func, opts) + bind_tenant(resource, tenant, :transaction, fn -> repo.transaction(func, opts) end) + end + + # Ash forwards no tenant of its own, so this digs it out of what it does + # forward. The shapes differ by path and all three are load-bearing: + # + # * the single-record paths pass `changeset.context[:data_layer]`, so a + # resource that puts the tenant there arrives as `%{tenant: t}` + # * the bulk paths pass the *whole* first changeset context, so the same + # value arrives one level down, under `:data_layer` + # * a read carries the query itself in its metadata, which already has the + # tenant on it -- so reads need nothing added to their context + defp reason_tenant(reason) do + context = reason[:data_layer_context] || %{} + + context[:tenant] || + get_in(context, [:data_layer, :tenant]) || + get_in(context, [:private, :tenant]) || + get_in(reason, [:metadata, :query, Access.key(:tenant)]) end @impl true @@ -2186,6 +2284,113 @@ defmodule AshSqlite.DataLayer do end end + # Wraps a statement in the resource's tenant binder, if it has one and this + # statement has a tenant. Every callback that issues SQL goes through here, which + # is the point: a caller cannot bind around a path it never sees, and two of the + # paths that matter -- aggregates and atomic writes -- give it nothing to bind + # around. + # + # `usage` is what this callback is: `:read`, `:write`, or `:transaction`. Only + # this module can say -- by the time a binder sees a statement the distinction is + # gone -- and a binder that caches, replicates, or routes reads separately from + # writes cannot be written without it. + defp bind_tenant(resource, nil, _usage, fun), do: unbound(resource, fun) + + defp bind_tenant(resource, tenant, usage, fun) do + case AshSqlite.DataLayer.Info.tenant_binder(resource) do + nil -> + without_binder(resource, tenant, fun) + + binder -> + repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + + # Captured before the bind: if a transaction is already open it is open on + # *this* connection, so a statement that binds elsewhere leaves it. + enclosing = if in_transaction?(resource), do: repo.get_dynamic_repo() + + binder.bind(tenant, [resource: resource, usage: usage], fn -> + if enclosing && repo.get_dynamic_repo() != enclosing do + # TODO: message wording. + raise ArgumentError, """ + #{inspect(resource)} tried to run a statement for tenant \ + #{inspect(tenant)} inside a transaction open on another tenant's \ + database. SQLite cannot commit across files atomically in WAL mode, so \ + this statement would commit on its own and survive a rollback. + + Open one transaction per tenant instead. + """ + end + + fun.() + end) + end + end + + # A tenanted resource whose data layer chooses connections per tenant, reached + # with no tenant, would run against whichever connection the process happens to + # hold. For a database-per-tenant layout that is another tenant's data, so it + # fails rather than guesses. Ash enforces "tenant required unless global?" itself; + # this catches the paths that bypass an action. + defp unbound(resource, fun) do + if tenant_required?(resource) do + # TODO: message wording. + raise ArgumentError, """ + #{inspect(resource)} has `strategy :context` but this statement carried no \ + tenant, so there is no connection to select. Pass a tenant, or set \ + `global? true` if this resource is genuinely shared. + """ + end + + fun.() + end + + # `strategy :context` and no binder is a configuration error rather than a + # statement to run unbound: the tenant was given, and nothing can act on it. The + # verifier says so at compile time, but only as a warning, so this is the guard + # that holds. + defp without_binder(resource, tenant, fun) do + if Ash.Resource.Info.multitenancy_strategy(resource) == :context do + raise ArgumentError, """ + #{inspect(resource)} has `strategy :context` and a tenant of \ + #{inspect(tenant)}, but no `tenant_binder` to select a connection with. + + sqlite do + tenant_binder MyApp.TenantBinder + end + + See `AshSqlite.TenantBinder`. + """ + end + + fun.() + end + + defp tenant_required?(resource) do + Ash.Resource.Info.multitenancy_strategy(resource) == :context && + !Ash.Resource.Info.multitenancy_global?(resource) + end + + defp query_tenant(%{__ash_bindings__: %{context: context}}) do + get_in(context, [:private, :tenant]) + end + + defp query_tenant(_), do: nil + + # A bulk operation is one statement per group, so every changeset in it has to + # agree on the tenant. Ash builds batches per action and per tenant, so a mixed + # batch is a bug elsewhere -- but this is the last place it could be caught + # before rows land in the wrong database. + defp changesets_tenant(changesets) do + changesets + |> Enum.map(& &1.tenant) + |> Enum.uniq() + |> case do + [] -> nil + [tenant] -> tenant + many -> raise ArgumentError, "bulk operation mixes tenants: #{inspect(many)}" + end + end + defp dynamic_repo(resource, %{__ash_bindings__: %{context: %{data_layer: %{repo: repo}}}}) do repo || AshSqlite.DataLayer.Info.repo(resource, :read) end diff --git a/lib/data_layer/info.ex b/lib/data_layer/info.ex index 96507a8..69050e7 100644 --- a/lib/data_layer/info.ex +++ b/lib/data_layer/info.ex @@ -23,6 +23,16 @@ defmodule AshSqlite.DataLayer.Info do Extension.get_opt(resource, [:sqlite], :write_transactions?, false, true) end + @doc """ + The tenant binder for a resource, or nil. + + A resource with no context multitenancy has none, and its statements run on + whatever connection the calling process already had. + """ + def tenant_binder(resource) do + Extension.get_opt(resource, [:sqlite], :tenant_binder, nil, true) + end + @doc "The configured table for a resource" def table(resource) do Extension.get_opt(resource, [:sqlite], :table, nil, true) diff --git a/lib/tenant_binder.ex b/lib/tenant_binder.ex new file mode 100644 index 0000000..295cd35 --- /dev/null +++ b/lib/tenant_binder.ex @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.TenantBinder do + @moduledoc """ + Chooses the connection a tenanted statement runs on. + + SQLite has no schemas, so `strategy :context` cannot be a query prefix: the SQL is + identical for every tenant and isolation comes from which file the connection is + attached to. + """ + + @typedoc """ + What the statement being bound is. + + * `:resource` — the resource the statement is for. + * `:usage` — `:read` for queries and aggregates, `:write` for creates, updates, + destroys, upserts and their bulk and atomic forms, and `:transaction` for the + callback that opens one. A `:transaction` may go on to contain either. + """ + @type opts :: [resource: Ash.Resource.t(), usage: :read | :write | :transaction] + + @doc "Runs `fun` with a connection selected for `tenant`, and returns its result." + @callback bind(tenant :: term(), opts :: opts(), fun :: (-> result)) :: result + when result: var +end diff --git a/lib/transformers/carry_tenant.ex b/lib/transformers/carry_tenant.ex new file mode 100644 index 0000000..10fbc40 --- /dev/null +++ b/lib/transformers/carry_tenant.ex @@ -0,0 +1,42 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Transformers.CarryTenant do + @moduledoc """ + Adds `AshSqlite.Changes.CarryTenant` to resources that need it. + """ + use Spark.Dsl.Transformer + + alias Spark.Dsl.Transformer + + @impl true + def after?(_), do: true + + @impl true + def transform(dsl) do + if needs_tenant?(dsl) and not carries_tenant?(dsl) do + {:ok, + Transformer.add_entity(dsl, [:changes], %Ash.Resource.Change{ + change: {AshSqlite.Changes.CarryTenant, []}, + on: [:create, :update, :destroy], + only_when_valid?: false, + where: [] + })} + else + {:ok, dsl} + end + end + + # Without this the tenant never reaches `transaction/4`: Ash calls it above the + # data layer, and nothing in the transaction reason names one. + defp needs_tenant?(dsl) do + Ash.Resource.Info.multitenancy_strategy(dsl) == :context + end + + defp carries_tenant?(dsl) do + dsl + |> Ash.Resource.Info.changes() + |> Enum.any?(&match?(%{change: {AshSqlite.Changes.CarryTenant, _}}, &1)) + end +end diff --git a/lib/verifiers/verify_tenant_binder.ex b/lib/verifiers/verify_tenant_binder.ex new file mode 100644 index 0000000..f4f59a6 --- /dev/null +++ b/lib/verifiers/verify_tenant_binder.ex @@ -0,0 +1,39 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Verifiers.VerifyTenantBinder do + @moduledoc false + use Spark.Dsl.Verifier + + alias Spark.Dsl.Verifier + alias Spark.Error.DslError + + @impl true + def verify(dsl) do + if Ash.Resource.Info.multitenancy_strategy(dsl) == :context and + is_nil(Verifier.get_option(dsl, [:sqlite], :tenant_binder)) do + {:error, + DslError.exception( + module: Verifier.get_persisted(dsl, :module), + path: [:sqlite, :tenant_binder], + message: """ + `strategy :context` needs a `tenant_binder`. + + SQLite has no schemas, so the tenant cannot be a query prefix: the SQL is \ + identical for every tenant and isolation comes from which file the \ + connection is attached to. Something has to choose that connection, and \ + only the application knows how. + + sqlite do + tenant_binder MyApp.TenantBinder + end + + See `AshSqlite.TenantBinder`.\ + """ + )} + else + :ok + end + end +end diff --git a/mix.exs b/mix.exs index ba0118a..4c3330d 100644 --- a/mix.exs +++ b/mix.exs @@ -118,6 +118,9 @@ defmodule AshSqlite.MixProject do AshSqlite.Repo, AshSqlite.DataLayer ], + Multitenancy: [ + AshSqlite.TenantBinder + ], Utilities: [ AshSqlite.ManualRelationship ], diff --git a/test/multitenancy_test.exs b/test/multitenancy_test.exs new file mode 100644 index 0000000..40a3f63 --- /dev/null +++ b/test/multitenancy_test.exs @@ -0,0 +1,304 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultitenancyTest do + @moduledoc """ + Context multitenancy against two real database files, checked by reading each file. + """ + use ExUnit.Case, async: false + + alias AshSqlite.Test.{GlobalPost, TenantBinder, TenantedPost} + + require Ash.Query + + setup do + dir = + Path.join( + System.tmp_dir!(), + "ash_sqlite_multitenancy_#{System.unique_integer([:positive])}" + ) + + File.mkdir_p!(dir) + on_exit(fn -> File.rm_rf!(dir) end) + + repos = + Map.new(["acme", "globex"], fn tenant -> + path = Path.join(dir, "#{tenant}.db") + {:ok, pid} = AshSqlite.TenantRepo.start_link(name: nil, database: path, pool_size: 1) + + Ecto.Adapters.SQL.query!( + pid, + "CREATE TABLE tenanted_posts (id TEXT PRIMARY KEY, title TEXT)", + [] + ) + + Ecto.Adapters.SQL.query!( + pid, + "CREATE TABLE global_posts (id TEXT PRIMARY KEY, title TEXT)", + [] + ) + + TenantBinder.register(tenant, pid) + {tenant, %{pid: pid, path: path}} + end) + + TenantBinder.reset_calls() + + %{repos: repos} + end + + # Goes to the file rather than back through Ash, so the isolation claim is checked + # against bytes on disk and not against the layer being tested. + defp titles_in_file(path) do + {:ok, db} = Exqlite.Sqlite3.open(path) + {:ok, stmt} = Exqlite.Sqlite3.prepare(db, "SELECT title FROM tenanted_posts ORDER BY title") + {:ok, rows} = Exqlite.Sqlite3.fetch_all(db, stmt) + :ok = Exqlite.Sqlite3.close(db) + List.flatten(rows) + end + + defp titles(tenant) do + TenantedPost |> Ash.read!(tenant: tenant) |> Enum.map(& &1.title) |> Enum.sort() + end + + defp create!(tenant, title) do + TenantedPost + |> Ash.Changeset.for_create(:create, %{title: title}, tenant: tenant) + |> Ash.create!() + end + + test "the data layer accepts context multitenancy" do + assert Ash.DataLayer.data_layer_can?(TenantedPost, :multitenancy) + end + + test "a resource that names a binder gets that one, not the default" do + assert AshSqlite.DataLayer.Info.tenant_binder(TenantedPost) == TenantBinder + end + + test "the named binder is what actually runs" do + TenantBinder.reset_calls() + create!("acme", "one") + + assert TenantBinder.calls() != [] + end + + test "a tenant given to Ash.create/3 rather than to the changeset still arrives" do + # The tenant is applied after global changes have run, so nothing has put it in + # the changeset context by the time the transaction opens. + post = + TenantedPost + |> Ash.Changeset.for_create(:create, %{title: "late tenant"}) + |> Ash.create!(tenant: "acme") + + assert post.title == "late tenant" + assert titles("acme") == ["late tenant"] + end + + test "each tenant's rows land in that tenant's own file", %{repos: repos} do + create!("acme", "acme one") + create!("acme", "acme two") + create!("globex", "globex one") + + assert titles_in_file(repos["acme"].path) == ["acme one", "acme two"] + assert titles_in_file(repos["globex"].path) == ["globex one"] + end + + test "a read only sees its own tenant" do + create!("acme", "acme one") + create!("globex", "globex one") + + assert ["acme one"] = TenantedPost |> Ash.read!(tenant: "acme") |> Enum.map(& &1.title) + assert ["globex one"] = TenantedPost |> Ash.read!(tenant: "globex") |> Enum.map(& &1.title) + end + + test "aggregates are bound, which a caller could not have wrapped" do + # `Ash.count/2` never enters `Ash.Actions.Read`, so a preparation or an + # around_transaction hook cannot reach it. Binding in the data layer does. + create!("acme", "acme one") + create!("globex", "globex one") + create!("globex", "globex two") + + assert Ash.count!(TenantedPost, tenant: "acme") == 1 + assert Ash.count!(TenantedPost, tenant: "globex") == 2 + end + + test "atomic updates are bound", %{repos: repos} do + # Ash builds these as one statement with no changeset for a caller to hook. + create!("acme", "before") + + TenantedPost + |> Ash.Query.filter(title == "before") + |> Ash.bulk_update!(:update, %{title: "after"}, tenant: "acme", strategy: :atomic) + + assert titles_in_file(repos["acme"].path) == ["after"] + end + + test "reads are reported to the binder as reads" do + create!("acme", "one") + TenantBinder.reset_calls() + + Ash.read!(TenantedPost, tenant: "acme") + + assert TenantBinder.calls() != [] + assert Enum.all?(TenantBinder.calls(), &match?({"acme", :read}, &1)) + end + + test "a write reports both the transaction and the write inside it" do + TenantBinder.reset_calls() + create!("acme", "two") + + usages = TenantBinder.calls() |> Enum.map(&elem(&1, 1)) |> Enum.uniq() + + assert :transaction in usages + assert :write in usages + end + + test "Ash refuses a tenantless query before it reaches the data layer" do + assert_raise Ash.Error.Invalid, ~r/require a tenant to be specified/, fn -> + Ash.read!(TenantedPost) + end + end + + test "and the data layer refuses one too, for the paths that bypass an action" do + assert_raise ArgumentError, ~r/carried no tenant/, fn -> + AshSqlite.DataLayer.transaction(TenantedPost, fn -> :unreachable end) + end + end + + test "a transaction commits to the tenant's own database", %{repos: repos} do + create!("acme", "in a transaction") + + assert titles_in_file(repos["acme"].path) == ["in a transaction"] + assert titles_in_file(repos["globex"].path) == [] + end + + test "a transaction refuses to reach into another tenant's database" do + # Wrapped by Ash, since the inner statement is a real action. + assert_raise Ash.Error.Unknown, ~r/open on another tenant's database/, fn -> + AshSqlite.DataLayer.transaction( + TenantedPost, + fn -> create!("globex", "wrong database") end, + nil, + %{type: :custom, metadata: %{}, data_layer_context: %{tenant: "acme"}} + ) + end + end + + describe "a context-multitenant resource with no binder" do + setup do + # Spark reports a verifier failure through `@after_verify`, which the parallel + # checker turns into a warning rather than an exception -- Ash's own + # multitenancy verifier included. So the module compiles, and the runtime is + # what actually stops it. + warnings = + ExUnit.CaptureIO.capture_io(:stderr, fn -> + Code.compile_string(""" + defmodule AshSqlite.Test.Unbindable do + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + defaults [:read] + end + + multitenancy do + strategy :context + end + + attributes do + uuid_primary_key :id + end + + sqlite do + table "unbindable" + repo AshSqlite.TenantRepo + end + end + """) + end) + + {:ok, warnings: warnings} + end + + test "is told so at compile time", %{warnings: warnings} do + assert warnings =~ "needs a `tenant_binder`" + end + + test "is refused at the first statement, rather than running unbound" do + assert_raise ArgumentError, ~r/no `tenant_binder` to select a connection with/, fn -> + AshSqlite.DataLayer.transaction( + AshSqlite.Test.Unbindable, + fn -> :unreachable end, + nil, + %{type: :custom, metadata: %{}, data_layer_context: %{tenant: "acme"}} + ) + end + end + end + + describe "a global? resource" do + test "is bound to the tenant it is given, like any other", %{repos: repos} do + GlobalPost + |> Ash.Changeset.for_create(:create, %{title: "for acme"}, tenant: "acme") + |> Ash.create!() + + assert global_titles_in_file(repos["acme"].path) == ["for acme"] + assert global_titles_in_file(repos["globex"].path) == [] + end + + test "is read without a tenant, where a tenanted resource is refused" do + assert_raise Ash.Error.Invalid, ~r/require a tenant to be specified/, fn -> + Ash.read!(TenantedPost) + end + + bound("acme", fn -> assert Ash.read!(GlobalPost) == [] end) + end + + test "reads whatever the process is bound to when given no tenant", %{repos: repos} do + # Ecto binds per repo *module*, so this is a sharp edge rather than a feature: + # a global resource sharing a repo with tenanted ones sees the last tenant bound. + insert_global(repos["acme"].pid, "acme's own") + insert_global(repos["globex"].pid, "globex's own") + + assert bound("acme", fn -> global_titles() end) == ["acme's own"] + assert bound("globex", fn -> global_titles() end) == ["globex's own"] + end + + test "is never asked of the binder when given no tenant" do + TenantBinder.reset_calls() + bound("acme", fn -> Ash.read!(GlobalPost) end) + + assert TenantBinder.calls() == [] + end + end + + defp bound(tenant, fun) do + previous = AshSqlite.TenantRepo.put_dynamic_repo(TenantBinder.repo_for(tenant)) + + try do + fun.() + after + AshSqlite.TenantRepo.put_dynamic_repo(previous) + end + end + + defp insert_global(pid, title) do + Ecto.Adapters.SQL.query!( + pid, + "INSERT INTO global_posts (id, title) VALUES (?, ?)", + [Ash.UUID.generate(), title] + ) + end + + defp global_titles do + GlobalPost |> Ash.read!() |> Enum.map(& &1.title) |> Enum.sort() + end + + defp global_titles_in_file(path) do + {:ok, db} = Exqlite.Sqlite3.open(path) + {:ok, stmt} = Exqlite.Sqlite3.prepare(db, "SELECT title FROM global_posts ORDER BY title") + {:ok, rows} = Exqlite.Sqlite3.fetch_all(db, stmt) + :ok = Exqlite.Sqlite3.close(db) + List.flatten(rows) + end +end diff --git a/test/support/domain.ex b/test/support/domain.ex index 69c8860..dfc68cb 100644 --- a/test/support/domain.ex +++ b/test/support/domain.ex @@ -18,6 +18,8 @@ defmodule AshSqlite.Test.Domain do resource(AshSqlite.Test.User) resource(AshSqlite.Test.Account) resource(AshSqlite.Test.TransactionalAccount) + resource(AshSqlite.Test.TenantedPost) + resource(AshSqlite.Test.GlobalPost) resource(AshSqlite.Test.Organization) resource(AshSqlite.Test.Manager) resource(AshSqlite.Test.Device) diff --git a/test/support/resources/global_post.ex b/test/support/resources/global_post.ex new file mode 100644 index 0000000..12e1cc7 --- /dev/null +++ b/test/support/resources/global_post.ex @@ -0,0 +1,36 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.GlobalPost do + @moduledoc """ + A `strategy :context` resource that Ash also allows without a tenant. + + Shares `AshSqlite.TenantRepo` with `AshSqlite.Test.TenantedPost`, which is what + makes it worth having: Ecto binds per repo *module*, so this resource sees + whatever tenant the calling process last bound. + """ + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + default_accept(:*) + defaults([:create, :read, :update, :destroy]) + end + + multitenancy do + strategy(:context) + global?(true) + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + sqlite do + table("global_posts") + repo(AshSqlite.TenantRepo) + tenant_binder(AshSqlite.Test.TenantBinder) + migrate?(false) + end +end diff --git a/test/support/resources/tenanted_post.ex b/test/support/resources/tenanted_post.ex new file mode 100644 index 0000000..9aa571b --- /dev/null +++ b/test/support/resources/tenanted_post.ex @@ -0,0 +1,36 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.TenantedPost do + @moduledoc """ + A `strategy :context` resource whose tenant is a database file. + + `migrate? false` because its table is created directly in each tenant's file by + the test setup — there is no one database for the generator to migrate, which is + the whole shape of database-per-tenant. + """ + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + default_accept(:*) + defaults([:create, :read, :update, :destroy]) + end + + multitenancy do + strategy(:context) + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + sqlite do + table("tenanted_posts") + repo(AshSqlite.TenantRepo) + tenant_binder(AshSqlite.Test.TenantBinder) + write_transactions?(true) + migrate?(false) + end +end diff --git a/test/support/tenant_binder.ex b/test/support/tenant_binder.ex new file mode 100644 index 0000000..13f9b9a --- /dev/null +++ b/test/support/tenant_binder.ex @@ -0,0 +1,42 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.TenantBinder do + @moduledoc """ + A binder that maps tenants to repo instances registered by the test. + """ + @behaviour AshSqlite.TenantBinder + + @doc "Registers the repo instance serving `tenant`." + def register(tenant, pid), do: Process.put({__MODULE__, :repo, tenant}, pid) + + @doc "The repo instance registered for `tenant`, or nil." + def repo_for(tenant), do: Process.get({__MODULE__, :repo, tenant}) + + @doc "Every `{tenant, usage}` this binder has been asked for, oldest first." + def calls, do: Enum.reverse(Process.get({__MODULE__, :calls}, [])) + + @doc "Forgets what has been recorded, leaving registrations in place." + def reset_calls, do: Process.delete({__MODULE__, :calls}) + + @impl true + def bind(tenant, opts, fun) do + Process.put({__MODULE__, :calls}, [ + {tenant, opts[:usage]} | Process.get({__MODULE__, :calls}, []) + ]) + + pid = + Process.get({__MODULE__, :repo, tenant}) || + raise ArgumentError, "no repo registered for tenant #{inspect(tenant)}" + + previous = AshSqlite.TenantRepo.get_dynamic_repo() + AshSqlite.TenantRepo.put_dynamic_repo(pid) + + try do + fun.() + after + AshSqlite.TenantRepo.put_dynamic_repo(previous) + end + end +end diff --git a/test/support/tenant_repo.ex b/test/support/tenant_repo.ex new file mode 100644 index 0000000..58bb9b9 --- /dev/null +++ b/test/support/tenant_repo.ex @@ -0,0 +1,10 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.TenantRepo do + @moduledoc """ + A repo used as a template, started per tenant as an anonymous instance rather than under its own name. + """ + use AshSqlite.Repo, otp_app: :ash_sqlite +end From 9887a4837cde5b4e56c6d09059e4e233b24b8583 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Mon, 24 Aug 2026 16:13:20 +0100 Subject: [PATCH 5/8] better code level comments --- config/config.exs | 7 ------- lib/changes/carry_tenant.ex | 4 ---- lib/data_layer.ex | 10 +--------- lib/transformers/carry_tenant.ex | 2 -- test/multitenancy_test.exs | 5 ----- 5 files changed, 1 insertion(+), 27 deletions(-) diff --git a/config/config.exs b/config/config.exs index 053f3ea..b927342 100644 --- a/config/config.exs +++ b/config/config.exs @@ -24,10 +24,6 @@ end if Mix.env() == :test do config :ash, :validate_domain_resource_inclusion?, false config :ash, :validate_domain_config_inclusion?, false - - # The sandbox runs every test inside a transaction, so Ash's warning about - # `before_transaction` hooks under an open transaction fires on tenanted actions - # that are perfectly fine outside the suite. config :ash, :warn_on_transaction_hooks?, false config :ash_sqlite, AshSqlite.TestRepo, @@ -44,9 +40,6 @@ if Mix.env() == :test do pool: Ecto.Adapters.SQL.Sandbox, migration_primary_key: [name: :id, type: :binary_id] - # No `database:` and no sandbox: the multitenancy tests start an instance of this - # per tenant, each against its own file, and bind it with `put_dynamic_repo/1`. - # Deliberately absent from `ecto_repos:` -- there is nothing to migrate centrally. config :ash_sqlite, AshSqlite.TenantRepo, pool_size: 1, migration_lock: false, diff --git a/lib/changes/carry_tenant.ex b/lib/changes/carry_tenant.ex index e0194cb..d0f603b 100644 --- a/lib/changes/carry_tenant.ex +++ b/lib/changes/carry_tenant.ex @@ -10,10 +10,6 @@ defmodule AshSqlite.Changes.CarryTenant do @impl true def change(changeset, _opts, _context) do - # Also as a before_transaction hook, because a tenant given to `Ash.create/3` - # rather than to `Ash.Changeset.for_create/4` is applied after global changes - # have run -- and those hooks run on the changeset the transaction is opened - # with. changeset |> put_tenant() |> Ash.Changeset.before_transaction(&put_tenant/1) diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 7c1f311..25cf958 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -540,10 +540,8 @@ defmodule AshSqlite.DataLayer do @impl true @doc """ A no-op on the query: with one database per tenant there is no prefix to set. - The connection is chosen per statement by the resource's `tenant_binder` - instead. Declaring the strategy still earns its keep — it makes Ash accept the - resource and enforce "a tenant is required unless `global?`". + instead. """ def set_tenant(_resource, query, _tenant) do {:ok, query} @@ -2310,7 +2308,6 @@ defmodule AshSqlite.DataLayer do binder.bind(tenant, [resource: resource, usage: usage], fn -> if enclosing && repo.get_dynamic_repo() != enclosing do - # TODO: message wording. raise ArgumentError, """ #{inspect(resource)} tried to run a statement for tenant \ #{inspect(tenant)} inside a transaction open on another tenant's \ @@ -2333,7 +2330,6 @@ defmodule AshSqlite.DataLayer do # this catches the paths that bypass an action. defp unbound(resource, fun) do if tenant_required?(resource) do - # TODO: message wording. raise ArgumentError, """ #{inspect(resource)} has `strategy :context` but this statement carried no \ tenant, so there is no connection to select. Pass a tenant, or set \ @@ -2376,10 +2372,6 @@ defmodule AshSqlite.DataLayer do defp query_tenant(_), do: nil - # A bulk operation is one statement per group, so every changeset in it has to - # agree on the tenant. Ash builds batches per action and per tenant, so a mixed - # batch is a bug elsewhere -- but this is the last place it could be caught - # before rows land in the wrong database. defp changesets_tenant(changesets) do changesets |> Enum.map(& &1.tenant) diff --git a/lib/transformers/carry_tenant.ex b/lib/transformers/carry_tenant.ex index 10fbc40..e91d59d 100644 --- a/lib/transformers/carry_tenant.ex +++ b/lib/transformers/carry_tenant.ex @@ -28,8 +28,6 @@ defmodule AshSqlite.Transformers.CarryTenant do end end - # Without this the tenant never reaches `transaction/4`: Ash calls it above the - # data layer, and nothing in the transaction reason names one. defp needs_tenant?(dsl) do Ash.Resource.Info.multitenancy_strategy(dsl) == :context end diff --git a/test/multitenancy_test.exs b/test/multitenancy_test.exs index 40a3f63..3108c3f 100644 --- a/test/multitenancy_test.exs +++ b/test/multitenancy_test.exs @@ -84,8 +84,6 @@ defmodule AshSqlite.MultitenancyTest do end test "a tenant given to Ash.create/3 rather than to the changeset still arrives" do - # The tenant is applied after global changes have run, so nothing has put it in - # the changeset context by the time the transaction opens. post = TenantedPost |> Ash.Changeset.for_create(:create, %{title: "late tenant"}) @@ -113,8 +111,6 @@ defmodule AshSqlite.MultitenancyTest do end test "aggregates are bound, which a caller could not have wrapped" do - # `Ash.count/2` never enters `Ash.Actions.Read`, so a preparation or an - # around_transaction hook cannot reach it. Binding in the data layer does. create!("acme", "acme one") create!("globex", "globex one") create!("globex", "globex two") @@ -124,7 +120,6 @@ defmodule AshSqlite.MultitenancyTest do end test "atomic updates are bound", %{repos: repos} do - # Ash builds these as one statement with no changeset for a caller to hook. create!("acme", "before") TenantedPost From 76866fcb831826ba7a2da202e04f9d63a95c2994 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Mon, 24 Aug 2026 15:36:30 +0100 Subject: [PATCH 6/8] improvement: manage tenant databases, so `strategy :context` works out of the box Stacked on the tenant binder PR, which ships the seam and nothing behind it. This supplies the runtime, so that `strategy :context` needs no binder of your own -- what @zachdaniel asked for on that PR: context multitenancy that manages its own storage rather than leaving it to the reader. children = [ MyApp.Repo, {AshSqlite.MultiTenancy, repo: MyApp.Repo, dir: "priv/tenants", migrations_path: "priv/repo/tenant_migrations"} ] `AshSqlite.MultiTenancy.Binder` becomes the default for a `strategy :context` resource, so the compile-time requirement to name one goes with it. Naming your own still overrides it, which is what keeps Turso, Litestream or a replica-aware router implementable outside this library. Named after `AshPostgres.MultiTenancy` rather than inventing a word for it. - `AshSqlite.MultiTenancy.Registry` -- one connection per tenant, and only one. - `AshSqlite.MultiTenancy.Connection` -- opens the file, migrates it, and refuses to serve until both have happened. - `AshSqlite.MultiTenancy.Manager` -- activation, an LRU bound on residency that will exceed itself rather than close a tenant mid-statement, quarantine for tenants that cannot open, and seal/unseal for draining a node. - `AshSqlite.MultiTenancy.Database` -- the tenant-to-filename mapping, escaped rather than sanitised so that it is injective: sanitising maps `a:b` and `a_b` onto one file, which puts two tenants in one database. - `AshSqlite.MultiTenancy.Migrations` -- compiles a migration directory once per boot rather than once per tenant. `all_tenants/1` answers what AshPostgres asks the application for through `Repo.all_tenants/0`; here it is derived, because a tenant is a file. Residents are unioned in, since SQLite creates the file on the first write. `rename/3` moves a tenant's database, sidecars included. Without it a renamed tenant keeps none of its data: the new name addresses a file that does not exist, and the next request quietly creates an empty one. The sidecars are not belt-and-braces -- a write is still in the WAL when the rename happens. Deliberately none, as with the PR below it. The guide covering both is a separate piece of work. --- lib/data_layer.ex | 26 +- lib/data_layer/info.ex | 19 +- lib/multi_tenancy.ex | 153 +++++++ lib/multi_tenancy/binder.ex | 18 + lib/multi_tenancy/binds.ex | 123 ++++++ lib/multi_tenancy/connection.ex | 175 ++++++++ lib/multi_tenancy/connection_supervisor.ex | 36 ++ lib/multi_tenancy/database.ex | 140 ++++++ lib/multi_tenancy/manager.ex | 367 ++++++++++++++++ lib/multi_tenancy/migrations.ex | 85 ++++ lib/multi_tenancy/registry.ex | 65 +++ lib/multi_tenancy/unavailable_error.ex | 20 + lib/tenant_binder.ex | 2 +- lib/verifiers/verify_tenant_binder.ex | 39 -- mix.exs | 4 +- test/multi_tenancy/binds_test.exs | 167 +++++++ test/multi_tenancy/connection_test.exs | 301 +++++++++++++ test/multi_tenancy/database_test.exs | 213 +++++++++ test/multi_tenancy/managed_test.exs | 231 ++++++++++ test/multi_tenancy/multi_tenancy_test.exs | 483 +++++++++++++++++++++ test/multi_tenancy/registry_test.exs | 115 +++++ test/multitenancy_test.exs | 51 --- test/support/domain.ex | 1 + test/support/managed_tenant_repo.ex | 11 + test/support/resources/managed_post.ex | 31 ++ 25 files changed, 2755 insertions(+), 121 deletions(-) create mode 100644 lib/multi_tenancy.ex create mode 100644 lib/multi_tenancy/binder.ex create mode 100644 lib/multi_tenancy/binds.ex create mode 100644 lib/multi_tenancy/connection.ex create mode 100644 lib/multi_tenancy/connection_supervisor.ex create mode 100644 lib/multi_tenancy/database.ex create mode 100644 lib/multi_tenancy/manager.ex create mode 100644 lib/multi_tenancy/migrations.ex create mode 100644 lib/multi_tenancy/registry.ex create mode 100644 lib/multi_tenancy/unavailable_error.ex delete mode 100644 lib/verifiers/verify_tenant_binder.ex create mode 100644 test/multi_tenancy/binds_test.exs create mode 100644 test/multi_tenancy/connection_test.exs create mode 100644 test/multi_tenancy/database_test.exs create mode 100644 test/multi_tenancy/managed_test.exs create mode 100644 test/multi_tenancy/multi_tenancy_test.exs create mode 100644 test/multi_tenancy/registry_test.exs create mode 100644 test/support/managed_tenant_repo.ex create mode 100644 test/support/resources/managed_post.ex diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 25cf958..7a640db 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -328,9 +328,6 @@ defmodule AshSqlite.DataLayer do AshSqlite.Transformers.VerifyRepo, AshSqlite.Transformers.EnsureTableOrPolymorphic, AshSqlite.Transformers.CarryTenant - ], - verifiers: [ - AshSqlite.Verifiers.VerifyTenantBinder ] def migrate(args) do @@ -2297,7 +2294,7 @@ defmodule AshSqlite.DataLayer do defp bind_tenant(resource, tenant, usage, fun) do case AshSqlite.DataLayer.Info.tenant_binder(resource) do nil -> - without_binder(resource, tenant, fun) + unbound(resource, fun) binder -> repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) @@ -2340,27 +2337,6 @@ defmodule AshSqlite.DataLayer do fun.() end - # `strategy :context` and no binder is a configuration error rather than a - # statement to run unbound: the tenant was given, and nothing can act on it. The - # verifier says so at compile time, but only as a warning, so this is the guard - # that holds. - defp without_binder(resource, tenant, fun) do - if Ash.Resource.Info.multitenancy_strategy(resource) == :context do - raise ArgumentError, """ - #{inspect(resource)} has `strategy :context` and a tenant of \ - #{inspect(tenant)}, but no `tenant_binder` to select a connection with. - - sqlite do - tenant_binder MyApp.TenantBinder - end - - See `AshSqlite.TenantBinder`. - """ - end - - fun.() - end - defp tenant_required?(resource) do Ash.Resource.Info.multitenancy_strategy(resource) == :context && !Ash.Resource.Info.multitenancy_global?(resource) diff --git a/lib/data_layer/info.ex b/lib/data_layer/info.ex index 69050e7..e35c863 100644 --- a/lib/data_layer/info.ex +++ b/lib/data_layer/info.ex @@ -24,13 +24,24 @@ defmodule AshSqlite.DataLayer.Info do end @doc """ - The tenant binder for a resource, or nil. + The tenant binder for a resource. - A resource with no context multitenancy has none, and its statements run on - whatever connection the calling process already had. + Defaults to `AshSqlite.MultiTenancy.Binder` for a resource with + `strategy :context`, so that database-per-tenant works without the application + supplying a runtime of its own. A resource with no context multitenancy has no + binder, and its statements run on whatever connection the calling process + already had. """ def tenant_binder(resource) do - Extension.get_opt(resource, [:sqlite], :tenant_binder, nil, true) + case Extension.get_opt(resource, [:sqlite], :tenant_binder, nil, true) do + nil -> + if Ash.Resource.Info.multitenancy_strategy(resource) == :context do + AshSqlite.MultiTenancy.Binder + end + + binder -> + binder + end end @doc "The configured table for a resource" diff --git a/lib/multi_tenancy.ex b/lib/multi_tenancy.ex new file mode 100644 index 0000000..b97320d --- /dev/null +++ b/lib/multi_tenancy.ex @@ -0,0 +1,153 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy do + @moduledoc """ + One SQLite database per tenant: choosing the file, opening it, migrating it, and closing it again. + """ + use Supervisor + + alias AshSqlite.MultiTenancy.Binds + alias AshSqlite.MultiTenancy.Connection + alias AshSqlite.MultiTenancy.Manager + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + @doc false + def child_spec(opts) do + %{ + id: {__MODULE__, Keyword.fetch!(opts, :repo)}, + start: {__MODULE__, :start_link, [opts]}, + type: :supervisor + } + end + + def start_link(opts) do + repo = Keyword.fetch!(opts, :repo) + Supervisor.start_link(__MODULE__, opts, name: Module.concat(repo, MultiTenancy)) + end + + @impl true + def init(opts) do + repo = Keyword.fetch!(opts, :repo) + _dir = Keyword.fetch!(opts, :dir) + verify_migrations_path!(Keyword.get(opts, :migrations_path)) + + children = [ + TenantRegistry.child_spec(repo), + Binds.child_spec(repo), + AshSqlite.MultiTenancy.ConnectionSupervisor.child_spec(repo), + Manager.child_spec(opts) + ] + + Supervisor.init(children, strategy: :one_for_one) + end + + @doc "The repo instance serving `tenant`, starting it if it is not resident." + @spec connection_for(module(), String.t()) :: {:ok, pid()} | {:error, term()} + def connection_for(repo, tenant) do + case TenantRegistry.lookup(repo, tenant) do + {:ok, _connection, repo_pid} when is_pid(repo_pid) -> + {:ok, repo_pid} + + # Registered, but its `init/1` has not published a repo yet, so this caller + # raced the activation. Asking the connection directly waits for it. + {:ok, connection, nil} -> + case Connection.repo_pid(connection) do + repo_pid when is_pid(repo_pid) -> {:ok, repo_pid} + _ -> Manager.activate(repo, tenant) + end + + :error -> + Manager.activate(repo, tenant) + end + end + + @doc "Runs `fun` with `tenant`'s database bound to the calling process." + @spec with_tenant(module(), String.t(), (-> result)) :: result when result: var + def with_tenant(repo, tenant, fun) when is_function(fun, 0) do + case connection_for(repo, tenant) do + {:ok, repo_pid} -> + bound(repo, tenant, repo_pid, fun) + + # Raising, not returning: an unbound statement here would run against whichever + # database the process already had, which is another tenant's data. + {:error, reason} -> + raise AshSqlite.MultiTenancy.UnavailableError, tenant: tenant, reason: reason + end + end + + defp bound(repo, tenant, repo_pid, fun) do + case Binds.bound(repo, tenant) do + :ok -> + previous = repo.put_dynamic_repo(repo_pid) + + try do + fun.() + after + repo.put_dynamic_repo(previous) + Binds.released(repo, tenant) + end + + # Being closed, so wait for it to land and bind whatever replaces it. + :closing -> + Process.sleep(1) + with_tenant(repo, tenant, fun) + end + end + + @doc "Closes a tenant's database, leaving the file on disk." + defdelegate close(repo, tenant, opts \\ []), to: Manager + + @doc "Closes a tenant's database and deletes it, WAL sidecars included." + defdelegate delete(repo, tenant), to: Manager + + @doc "Moves a tenant's database to another tenant's name, sidecars included." + defdelegate rename(repo, from, to), to: Manager + + @doc "Tenants holding an open connection right now." + @spec resident(module()) :: [String.t()] + defdelegate resident(repo), to: TenantRegistry + + @doc "Every tenant with a database on disk, plus any that are resident." + defdelegate all_tenants(repo), to: Manager + + @doc "Tenants that failed to activate, with the reason." + defdelegate quarantined(repo), to: Manager + + @doc "Clears a tenant's quarantine, so the next request tries again." + defdelegate release(repo, tenant), to: Manager + + @doc "Stops this node taking on new tenants." + defdelegate seal(repo), to: Manager + + @doc "Lets this node accept tenants again." + defdelegate unseal(repo), to: Manager + + @doc "Activates and migrates each tenant in turn, reporting what happened." + defdelegate migrate_all(repo, tenants \\ :all, opts \\ []), to: Manager + + @doc "Where a tenant's database is, whether or not it exists yet." + defdelegate path_for(repo, tenant), to: Manager + + @doc "The fleet configuration." + defdelegate config(repo), to: Manager + + # Checked at boot rather than at activation: a typo would otherwise surface in + # production as `no such table`, at whatever hour the first tenant woke up. + defp verify_migrations_path!(nil), do: :ok + + defp verify_migrations_path!(path) do + if File.dir?(path) do + :ok + else + raise ArgumentError, """ + :migrations_path #{inspect(path)} is not a directory. + + Tenant databases are migrated from it, and `Ecto.Migrator` treats a missing \ + directory as one containing no migrations -- so every tenant would open an \ + empty database and fail on its first query instead of here. + """ + end + end +end diff --git a/lib/multi_tenancy/binder.ex b/lib/multi_tenancy/binder.ex new file mode 100644 index 0000000..3a650d9 --- /dev/null +++ b/lib/multi_tenancy/binder.ex @@ -0,0 +1,18 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Binder do + @moduledoc """ + Implements `AshSqlite.TenantBinder` in terms of `AshSqlite.MultiTenancy`. The default for `strategy :context`. + """ + + @behaviour AshSqlite.TenantBinder + + @impl true + def bind(tenant, opts, fun) do + resource = Keyword.fetch!(opts, :resource) + repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + AshSqlite.MultiTenancy.with_tenant(repo, tenant, fun) + end +end diff --git a/lib/multi_tenancy/binds.ex b/lib/multi_tenancy/binds.ex new file mode 100644 index 0000000..02b9288 --- /dev/null +++ b/lib/multi_tenancy/binds.ex @@ -0,0 +1,123 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Binds do + @moduledoc """ + Counts who is using each tenant, so that closing one can be safe. + """ + use GenServer + + @doc false + def child_spec(repo) do + %{id: {__MODULE__, repo}, start: {__MODULE__, :start_link, [repo]}} + end + + @doc false + def start_link(repo), do: GenServer.start_link(__MODULE__, repo, name: name(repo)) + + @doc "The table serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantBinds) + + @doc "Records that the calling process has bound `tenant`." + @spec bound(module(), String.t()) :: :ok | :closing + def bound(repo, tenant) do + if closing?(repo, tenant) do + :closing + else + :ets.update_counter(name(repo), {:binds, tenant}, {2, 1}, {{:binds, tenant}, 0}) + :ok + end + end + + @doc "Records that the calling process has released `tenant`, and marks it used." + @spec released(module(), String.t()) :: :ok + def released(repo, tenant) do + # Clamped at zero: an unmatched release would make a busy tenant look evictable. + :ets.update_counter(name(repo), {:binds, tenant}, {2, -1, 0, 0}, {{:binds, tenant}, 0}) + touch(repo, tenant) + end + + @doc "How many processes are bound to `tenant`." + @spec count(module(), String.t()) :: non_neg_integer() + def count(repo, tenant) do + case :ets.lookup(name(repo), {:binds, tenant}) do + [{_, count}] -> count + [] -> 0 + end + end + + @doc "Marks `tenant` as used now." + @spec touch(module(), String.t()) :: :ok + def touch(repo, tenant) do + :ets.insert(name(repo), {{:used, tenant}, System.monotonic_time()}) + :ok + end + + @doc "When `tenant` was last used, in `System.monotonic_time/0` units." + @spec last_used(module(), String.t()) :: integer() | nil + def last_used(repo, tenant) do + case :ets.lookup(name(repo), {:used, tenant}) do + [{_, at}] -> at + [] -> nil + end + end + + @doc "The tenant among `candidates` that should be evicted, if any." + @spec least_recently_used(module(), [String.t()]) :: String.t() | nil + def least_recently_used(repo, candidates) do + candidates + |> Enum.reject(&(count(repo, &1) > 0 or closing?(repo, &1))) + |> Enum.min_by(&sort_key(repo, &1), fn -> nil end) + end + + # Never-used sorts first, and cannot be spelled as a sentinel timestamp: + # `System.monotonic_time/0` is normally negative, so 0 would read as recent. + defp sort_key(repo, tenant) do + case last_used(repo, tenant) do + nil -> {0, 0} + at -> {1, at} + end + end + + @doc "Marks `tenant` as closing, so that `bound/2` refuses it." + @spec begin_closing(module(), String.t()) :: :ok + def begin_closing(repo, tenant) do + :ets.insert(name(repo), {{:closing, tenant}, true}) + :ok + end + + @doc "Clears the closing mark for `tenant`." + @spec end_closing(module(), String.t()) :: :ok + def end_closing(repo, tenant) do + :ets.delete(name(repo), {:closing, tenant}) + :ok + end + + @doc "Whether `tenant` is being closed right now." + @spec closing?(module(), String.t()) :: boolean() + def closing?(repo, tenant), do: :ets.member(name(repo), {:closing, tenant}) + + @doc "Drops everything recorded about `tenant`." + @spec forget(module(), String.t()) :: :ok + def forget(repo, tenant) do + table = name(repo) + Enum.each([:binds, :used, :closing], &:ets.delete(table, {&1, tenant})) + :ok + end + + @impl true + def init(repo) do + # Public: binds happen in arbitrary caller processes, not in this one. + :ets.new(name(repo), [ + :named_table, + :public, + :set, + read_concurrency: true, + write_concurrency: true + ]) + + {:ok, repo} + end +end diff --git a/lib/multi_tenancy/connection.ex b/lib/multi_tenancy/connection.ex new file mode 100644 index 0000000..367c6be --- /dev/null +++ b/lib/multi_tenancy/connection.ex @@ -0,0 +1,175 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Connection do + @moduledoc """ + One tenant's database, owned by one process for as long as the tenant is resident. + """ + use GenServer, restart: :temporary + + require Logger + + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + defstruct [:tenant, :repo, :repo_pid, :path, :schema_version, :opened_at] + + @doc "Starts a connection for a tenant." + def start_link(opts) do + repo = Keyword.fetch!(opts, :repo) + tenant = Keyword.fetch!(opts, :tenant) + + GenServer.start_link(__MODULE__, opts, name: TenantRegistry.via(repo, tenant)) + end + + @doc "The repo instance pid for this tenant." + def repo_pid(connection), do: GenServer.call(connection, :repo_pid) + + @doc "What this connection is serving, for the fleet view." + def info(connection), do: GenServer.call(connection, :info) + + @doc "Stops the repo instance while leaving this process alive." + def stop_repo(connection), do: GenServer.call(connection, :stop_repo) + + @impl true + def init(opts) do + repo = Keyword.fetch!(opts, :repo) + tenant = Keyword.fetch!(opts, :tenant) + path = Keyword.fetch!(opts, :path) + key = Keyword.get(opts, :key) + + # The registry value is a repo pid, so this process must not outlive its repo. + Process.flag(:trap_exit, true) + File.mkdir_p!(Path.dirname(path)) + + migrations_path = Keyword.get(opts, :migrations_path) + + with :ok <- verify_key(key, Keyword.get(opts, :encrypted?, false)), + :ok <- verify_migrations_path(migrations_path), + {:ok, repo_pid} <- start_repo(repo, path, key, Keyword.get(opts, :repo_opts, [])), + {:ok, version} <- migrate(repo, repo_pid, tenant, migrations_path) do + :ok = TenantRegistry.publish(repo, tenant, repo_pid) + + {:ok, + %__MODULE__{ + tenant: tenant, + repo: repo, + repo_pid: repo_pid, + path: path, + schema_version: version, + opened_at: System.monotonic_time(:millisecond) + }} + else + {:error, reason} -> {:stop, reason} + end + end + + @impl true + def handle_call(:repo_pid, _from, state), do: {:reply, state.repo_pid, state} + + def handle_call(:info, _from, state) do + {:reply, Map.take(state, [:tenant, :path, :schema_version, :opened_at]), state} + end + + def handle_call(:stop_repo, _from, state) do + stop_repo_instance(state.repo_pid) + + # Cleared so that a second call, and `terminate/2`, are no-ops rather than a + # stop against a pid that is already gone. + {:reply, :ok, %{state | repo_pid: nil}} + end + + @impl true + def handle_info({:EXIT, repo_pid, reason}, %{repo_pid: repo_pid} = state) do + {:stop, {:repo_exited, reason}, %{state | repo_pid: nil}} + end + + def handle_info({:EXIT, _pid, _reason}, state), do: {:noreply, state} + + @impl true + def terminate(_reason, state), do: stop_repo_instance(state.repo_pid) + + defp start_repo(repo, path, key, extra) do + opts = + extra + # backoff_type: :stop — a wrong key or a corrupt file is not transient, and + # retrying turns a clear failure into a hang. + |> Keyword.merge(name: nil, database: path, backoff_type: :stop) + # One writer per file is all SQLite offers; at one, contention waits in the + # pool rather than reaching SQLite's write lock, which fails instead of waiting. + |> Keyword.put(:pool_size, 1) + |> maybe_put_key(key) + + case repo.start_link(opts) do + {:ok, repo_pid} -> {:ok, repo_pid} + {:error, {:already_started, repo_pid}} -> {:ok, repo_pid} + {:error, reason} -> {:error, {:cannot_open_database, reason}} + end + end + + defp maybe_put_key(opts, nil), do: opts + defp maybe_put_key(opts, key), do: Keyword.put(opts, :key, key) + + # SQLite will create a *plaintext* database for a tenant whose key is missing. + defp verify_key(nil, true), do: {:error, :no_key} + defp verify_key(_key, _encrypted?), do: :ok + + # `Ecto.Migrator` treats a missing directory as one with no migrations, which would + # leave every tenant empty and surface as `no such table` far from the cause. + defp verify_migrations_path(nil), do: :ok + + defp verify_migrations_path(path) do + if File.dir?(path), do: :ok, else: {:error, {:missing_migrations_path, path}} + end + + defp migrate(_repo, _repo_pid, _tenant, nil), do: {:ok, nil} + + defp migrate(repo, repo_pid, tenant, migrations_path) do + opts = [ + dynamic_repo: repo_pid, + # Registration already reduced this to one process per file, and with a pool of + # one there is no second connection for Ecto's lock to take. + migration_lock: false, + log: false, + log_migrations_sql: false + ] + + source = AshSqlite.MultiTenancy.Migrations.load!(migrations_path) + statuses = Ecto.Migrator.migrations(repo, source, opts) + + # Only run the migrator when something is pending: the common case is an + # up-to-date tenant being activated. + if Enum.any?(statuses, &match?({:down, _, _}, &1)) do + Ecto.Migrator.run(repo, source, :up, Keyword.put(opts, :all, true)) + end + + {:ok, current_version(statuses)} + rescue + exception -> + Logger.error(""" + migrating tenant #{inspect(tenant)} raised #{inspect(exception.__struct__)}: \ + #{Exception.message(exception)} + """) + + {:error, {:migration_failed, exception.__struct__}} + end + + # Every version in the directory, because anything pending has just been run. + defp current_version([]), do: nil + defp current_version(statuses), do: statuses |> Enum.map(&elem(&1, 1)) |> Enum.max() + + defp stop_repo_instance(nil), do: :ok + + defp stop_repo_instance(repo_pid) do + if Process.alive?(repo_pid) do + # A clean stop closes the connection, which checkpoints the WAL back into the + # database. A kill leaves it to be replayed by whoever opens the file next. + Supervisor.stop(repo_pid) + end + + :ok + catch + # The repo is already going down, which is the state we wanted. + :exit, _ -> :ok + end +end diff --git a/lib/multi_tenancy/connection_supervisor.ex b/lib/multi_tenancy/connection_supervisor.ex new file mode 100644 index 0000000..ba753a0 --- /dev/null +++ b/lib/multi_tenancy/connection_supervisor.ex @@ -0,0 +1,36 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.ConnectionSupervisor do + @moduledoc """ + Supervises the connection processes of one repo's tenants. + """ + + @doc false + def child_spec(repo) do + %{id: {__MODULE__, repo}, start: {__MODULE__, :start_link, [repo]}, type: :supervisor} + end + + @doc false + def start_link(repo) do + DynamicSupervisor.start_link(strategy: :one_for_one, name: name(repo)) + end + + @doc "The supervisor serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantConnectionSupervisor) + + @doc "Starts a connection, or reports the one already serving that tenant." + @spec start_connection(module(), keyword()) :: + {:ok, pid()} | {:error, {:already_started, pid()}} | {:error, term()} + def start_connection(repo, opts) do + DynamicSupervisor.start_child(name(repo), {AshSqlite.MultiTenancy.Connection, opts}) + end + + @doc "Stops a connection, letting it close its database cleanly." + @spec stop_connection(module(), pid()) :: :ok | {:error, :not_found} + def stop_connection(repo, connection) do + DynamicSupervisor.terminate_child(name(repo), connection) + end +end diff --git a/lib/multi_tenancy/database.ex b/lib/multi_tenancy/database.ex new file mode 100644 index 0000000..541b0c9 --- /dev/null +++ b/lib/multi_tenancy/database.ex @@ -0,0 +1,140 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Database do + @moduledoc """ + Turns a tenant into the name of its database file. The encoding is reversible, so no two tenants can name one file. + """ + + @unreserved ~c"abcdefghijklmnopqrstuvwxyz0123456789-_." + + # 255 is the usual per-name limit. `-shm` is the longest suffix a SQLite + # database adds to its own name, and `.db` is ours, so this is what is left. + @max_encoded_bytes 255 - byte_size(".db") - byte_size("-shm") + + @doc "Encodes `tenant` as a path-safe name, reversibly." + @spec encode(String.t()) :: String.t() + def encode(tenant) when is_binary(tenant) and byte_size(tenant) > 0 do + encoded = tenant |> :binary.bin_to_list() |> Enum.map_join(&encode_byte/1) + + if byte_size(encoded) > @max_encoded_bytes do + raise ArgumentError, """ + tenant #{inspect(tenant)} encodes to #{byte_size(encoded)} bytes, and a \ + database name can be at most #{@max_encoded_bytes}. + + Bytes outside #{inspect(to_string(@unreserved))} take three bytes each once \ + escaped, so a tenant of mostly-escaped bytes reaches the limit at about \ + #{div(@max_encoded_bytes, 3)} characters. Use a shorter identifier — a slug \ + or a UUID — as the tenant, and keep the display name in a column. + """ + end + + encoded + end + + def encode(tenant) when is_binary(tenant) do + raise ArgumentError, "the tenant of an AshSqlite resource cannot be an empty string" + end + + def encode(other) do + raise ArgumentError, """ + expected a binary tenant, got: #{inspect(other)} + + A tenant names a database file, and a non-binary tenant would have to be \ + stringified to do that. Doing so here would let two different tenants -- \ + #{inspect(other)} and #{inspect(to_string_safe(other))} -- name one file while \ + remaining two tenants to Ash, so their rows would share a database with \ + nothing raising. + + Convert it where you set the tenant instead. + """ + end + + @doc "Recovers the tenant from a name produced by `encode/1`." + @spec decode(String.t()) :: {:ok, String.t()} | :error + def decode(encoded) when is_binary(encoded), do: decode(encoded, []) + + @doc "The absolute path of `tenant`'s database inside `dir`." + @spec path(Path.t(), String.t()) :: Path.t() + def path(dir, tenant), do: Path.join(dir, encode(tenant) <> ".db") + + @doc "Every file that makes up `path`, including the ones SQLite adds." + @spec sidecars(Path.t()) :: [Path.t()] + def sidecars(path), do: [path, path <> "-wal", path <> "-shm"] + + @doc "The tenant a database file inside `dir` belongs to." + @spec tenant_from_path(Path.t(), Path.t()) :: {:ok, String.t()} | :error + def tenant_from_path(dir, path) do + with {:ok, relative} <- relative_to(dir, path), + {:ok, base} <- strip_extension(relative) do + decode(base) + end + end + + @doc "Every tenant with a database in `dir`, in no particular order." + @spec list(Path.t()) :: [String.t()] + def list(dir) do + case File.ls(dir) do + {:ok, entries} -> + for entry <- entries, + {:ok, tenant} <- [tenant_from_path(dir, Path.join(dir, entry))], + do: tenant + + {:error, _} -> + [] + end + end + + defp relative_to(dir, path) do + case Path.relative_to(path, dir) do + ^path -> :error + relative -> if Path.dirname(relative) == ".", do: {:ok, relative}, else: :error + end + end + + defp strip_extension(name) do + case Path.extname(name) do + ".db" -> {:ok, Path.rootname(name, ".db")} + _ -> :error + end + end + + defp encode_byte(byte) when byte in @unreserved, do: <> + + defp encode_byte(byte) do + <> = <> + <> + end + + # Lowercase, so that the encoded alphabet as a whole is lowercase and two + # encodings can never differ only by case. See the moduledoc. + defp hex(nibble) when nibble < 10, do: ?0 + nibble + defp hex(nibble), do: ?a + nibble - 10 + + defp decode(<<>>, []), do: :error + defp decode(<<>>, acc), do: {:ok, acc |> Enum.reverse() |> IO.iodata_to_binary()} + + defp decode(<>, acc) do + with {:ok, hi} <- unhex(hi), {:ok, lo} <- unhex(lo) do + decode(rest, [<> | acc]) + end + end + + defp decode(<>, _acc), do: :error + + defp decode(<>, acc) when byte in @unreserved, + do: decode(rest, [byte | acc]) + + defp decode(_, _acc), do: :error + + defp unhex(char) when char in ?0..?9, do: {:ok, char - ?0} + defp unhex(char) when char in ?a..?f, do: {:ok, char - ?a + 10} + defp unhex(_char), do: :error + + defp to_string_safe(term) do + to_string(term) + rescue + _ -> inspect(term) + end +end diff --git a/lib/multi_tenancy/manager.ex b/lib/multi_tenancy/manager.ex new file mode 100644 index 0000000..76ad1e5 --- /dev/null +++ b/lib/multi_tenancy/manager.ex @@ -0,0 +1,367 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Manager do + @moduledoc """ + Activates tenants, bounds how many stay resident, and quarantines the ones that cannot start. + """ + use GenServer + + require Logger + + alias AshSqlite.MultiTenancy.Binds + alias AshSqlite.MultiTenancy.ConnectionSupervisor + alias AshSqlite.MultiTenancy.Database + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + @default_max_resident 256 + + defstruct [ + :repo, + :dir, + :key_for, + :migrations_path, + :max_resident, + :repo_opts, + quarantined: %{}, + sealed?: false + ] + + @doc false + def child_spec(opts) do + %{id: {__MODULE__, Keyword.fetch!(opts, :repo)}, start: {__MODULE__, :start_link, [opts]}} + end + + @doc false + def start_link(opts) do + GenServer.start_link(__MODULE__, opts, name: name(Keyword.fetch!(opts, :repo))) + end + + @doc "The manager serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantManager) + + @doc "Returns the repo instance serving `tenant`, starting it if it is not resident." + @spec activate(module(), String.t()) :: {:ok, pid()} | {:error, term()} + def activate(repo, tenant), do: activate(repo, tenant, 50) + + defp activate(repo, tenant, attempts_left) do + case GenServer.call(name(repo), {:activate, tenant}, 30_000) do + {:error, :connection_died} -> retry_activate(repo, tenant, attempts_left) + other -> other + end + end + + # A registration is released when the registry handles the connection's DOWN, so a + # tenant whose connection just died is briefly impossible to start. Yield and retry. + defp retry_activate(_repo, _tenant, 0), do: {:error, :connection_died} + + defp retry_activate(repo, tenant, attempts_left) do + Process.sleep(1) + activate(repo, tenant, attempts_left - 1) + end + + @doc "Closes a tenant's database, leaving the file on disk." + @spec close(module(), String.t(), keyword()) :: :ok + def close(repo, tenant, opts \\ []) do + # A caller that had already marked the tenant closing wants it to stay that way + # afterwards -- `rename/3` holds the mark across the close *and* the file move. + held_open_by_caller? = Binds.closing?(repo, tenant) + Binds.begin_closing(repo, tenant) + + try do + unless Keyword.get(opts, :force, false) do + await_quiescence(repo, tenant, Keyword.get(opts, :grace_ms, 1_000)) + end + + case TenantRegistry.lookup(repo, tenant) do + {:ok, connection, _repo_pid} -> ConnectionSupervisor.stop_connection(repo, connection) + :error -> :ok + end + + Binds.forget(repo, tenant) + :ok + after + unless held_open_by_caller?, do: Binds.end_closing(repo, tenant) + end + end + + @doc "Closes a tenant and deletes its database, including the WAL sidecars." + @spec delete(module(), String.t()) :: {:ok, [Path.t()]} + def delete(repo, tenant) do + close(repo, tenant, force: true) + GenServer.call(name(repo), {:delete, tenant}) + end + + @doc """ + Moves a tenant's database to another tenant's name, sidecars included. + + A tenant here is a file, so a tenant renamed without this keeps none of its data: + the new name addresses a database that does not exist, and the next request + creates an empty one. Both names are held closed for the move, so nothing reopens + the file being moved or creates a database at the destination while it happens. + + Refuses rather than overwrites when the destination already has a database. + """ + @spec rename(module(), String.t(), String.t()) :: + :ok | {:error, :no_database | :target_exists | File.posix()} + def rename(_repo, tenant, tenant), do: :ok + + def rename(repo, from, to) do + Binds.begin_closing(repo, from) + Binds.begin_closing(repo, to) + + try do + close(repo, from) + GenServer.call(name(repo), {:rename, from, to}) + after + Binds.end_closing(repo, from) + Binds.end_closing(repo, to) + end + end + + @doc "Stops the node taking on new tenants. The first step of a drain." + @spec seal(module()) :: :ok + def seal(repo), do: GenServer.call(name(repo), :seal) + + @doc "Lets the node accept tenants again." + @spec unseal(module()) :: :ok + def unseal(repo), do: GenServer.call(name(repo), :unseal) + + @doc "Whether the node is refusing new tenants." + @spec sealed?(module()) :: boolean() + def sealed?(repo), do: GenServer.call(name(repo), :sealed?) + + @doc "Tenants that failed to activate, with the reason." + @spec quarantined(module()) :: %{String.t() => term()} + def quarantined(repo), do: GenServer.call(name(repo), :quarantined) + + @doc "Clears a tenant's quarantine, so the next request tries again." + @spec release(module(), String.t()) :: :ok + def release(repo, tenant), do: GenServer.call(name(repo), {:release, tenant}) + + @doc "The fleet configuration." + @spec config(module()) :: map() + def config(repo), do: GenServer.call(name(repo), :config) + + @doc "Where `tenant`'s database is, whether or not it exists yet." + @spec path_for(module(), String.t()) :: Path.t() + def path_for(repo, tenant), do: GenServer.call(name(repo), {:path_for, tenant}) + + @doc """ + Every tenant this node knows of: one with a database in the fleet's directory, + or one currently resident. + + Derived rather than asked of the application, unlike AshPostgres' + `all_tenants/0`, because a tenant here *is* a file. Residents are included + because SQLite creates the file on the first write, so a tenant activated a + moment ago may not have one yet. A tenant that has never been activated is + absent, which is also why it needs no migrating -- it is migrated when it is + first opened. + """ + @spec all_tenants(module()) :: [String.t()] + def all_tenants(repo) do + on_disk = repo |> config() |> Map.fetch!(:dir) |> Database.list() + + Enum.uniq(on_disk ++ TenantRegistry.resident(repo)) + end + + @doc "Activates each tenant in turn, migrating it, and reports what happened." + @spec migrate_all(module(), [String.t()] | :all, keyword()) :: + [{String.t(), {:ok, term()} | {:error, term()}}] + def migrate_all(repo, tenants \\ :all, opts \\ []) + + def migrate_all(repo, :all, opts), do: migrate_all(repo, all_tenants(repo), opts) + + def migrate_all(repo, tenants, opts) do + close_after? = Keyword.get(opts, :close_after?, true) + + Enum.map(tenants, fn tenant -> + result = + with {:ok, _repo_pid} <- activate(repo, tenant), + {:ok, connection, _} <- TenantRegistry.lookup(repo, tenant) do + version = AshSqlite.MultiTenancy.Connection.info(connection).schema_version + if close_after?, do: close(repo, tenant) + {:ok, version} + else + :error -> {:error, :connection_died} + {:error, reason} -> {:error, reason} + end + + {tenant, result} + end) + end + + @impl true + def init(opts) do + state = %__MODULE__{ + repo: Keyword.fetch!(opts, :repo), + dir: Keyword.fetch!(opts, :dir), + key_for: Keyword.get(opts, :key_for), + migrations_path: Keyword.get(opts, :migrations_path), + max_resident: Keyword.get(opts, :max_resident, @default_max_resident), + repo_opts: Keyword.get(opts, :repo_opts, []) + } + + File.mkdir_p!(state.dir) + {:ok, state} + end + + @impl true + def handle_call({:activate, _tenant}, _from, %{sealed?: true} = state) do + {:reply, {:error, :draining}, state} + end + + def handle_call({:activate, tenant}, _from, state) + when is_map_key(state.quarantined, tenant) do + {:reply, {:error, {:quarantined, state.quarantined[tenant]}}, state} + end + + def handle_call({:activate, tenant}, _from, state) do + case TenantRegistry.lookup(state.repo, tenant) do + {:ok, connection, repo_pid} -> + {:reply, {:ok, repo_pid || AshSqlite.MultiTenancy.Connection.repo_pid(connection)}, state} + + :error -> + state = evict_if_needed(state) + result = start(state, tenant) + {:reply, result, quarantine_on_failure(state, tenant, result)} + end + end + + def handle_call({:delete, tenant}, _from, state) do + base = Database.path(state.dir, tenant) + + # Closing checkpoints and may remove the sidecars itself, so existence is not + # stable between a check and a removal. Attempt each and report what went. + removed = for path <- Database.sidecars(base), File.rm(path) == :ok, do: path + + {:reply, {:ok, removed}, state} + end + + def handle_call({:rename, from, to}, _from, state) do + source = Database.path(state.dir, from) + target = Database.path(state.dir, to) + + cond do + not File.exists?(source) -> + {:reply, {:error, :no_database}, state} + + File.exists?(target) -> + {:reply, {:error, :target_exists}, state} + + true -> + {:reply, move(source, target), + %{state | quarantined: Map.delete(state.quarantined, from)}} + end + end + + def handle_call(:seal, _from, state), do: {:reply, :ok, %{state | sealed?: true}} + def handle_call(:unseal, _from, state), do: {:reply, :ok, %{state | sealed?: false}} + def handle_call(:sealed?, _from, state), do: {:reply, state.sealed?, state} + def handle_call(:quarantined, _from, state), do: {:reply, state.quarantined, state} + + def handle_call({:release, tenant}, _from, state) do + {:reply, :ok, %{state | quarantined: Map.delete(state.quarantined, tenant)}} + end + + def handle_call(:config, _from, state) do + {:reply, Map.take(state, [:repo, :dir, :migrations_path, :max_resident]), state} + end + + def handle_call({:path_for, tenant}, _from, state) do + {:reply, Database.path(state.dir, tenant), state} + end + + defp start(state, tenant) do + opts = [ + repo: state.repo, + tenant: tenant, + path: Database.path(state.dir, tenant), + key: key_for(state, tenant), + # `encrypted?` tells the connection "this fleet uses keys", so that a tenant + # whose key is missing fails instead of quietly opening a plaintext database. + encrypted?: not is_nil(state.key_for), + migrations_path: state.migrations_path, + repo_opts: state.repo_opts + ] + + case ConnectionSupervisor.start_connection(state.repo, opts) do + {:ok, connection} -> published(state, tenant, connection) + {:error, {:already_started, connection}} -> published(state, tenant, connection) + {:error, reason} -> {:error, reason} + end + end + + defp published(state, tenant, connection) do + if Process.alive?(connection) do + # Marked used at activation, or a tenant that is about to be queried would + # have no timestamp and sort first for eviction. + Binds.touch(state.repo, tenant) + + case TenantRegistry.lookup(state.repo, tenant) do + {:ok, _connection, repo_pid} when is_pid(repo_pid) -> {:ok, repo_pid} + _ -> {:error, :connection_died} + end + else + {:error, :connection_died} + end + end + + defp quarantine_on_failure(state, tenant, {:error, reason}) + when reason not in [:connection_died] do + %{state | quarantined: Map.put(state.quarantined, tenant, reason)} + end + + defp quarantine_on_failure(state, _tenant, _result), do: state + + defp key_for(%{key_for: nil}, _tenant), do: nil + defp key_for(%{key_for: fun}, tenant), do: fun.(tenant) + + defp evict_if_needed(state) do + if TenantRegistry.count(state.repo) >= state.max_resident do + case Binds.least_recently_used(state.repo, TenantRegistry.resident(state.repo)) do + nil -> + Logger.warning(""" + #{inspect(state.repo)} holds #{TenantRegistry.count(state.repo)} tenant \ + databases, at or above max_resident of #{state.max_resident}, and every \ + one of them is in use. Exceeding the limit rather than refusing the \ + tenant that is arriving. + """) + + tenant -> + # No wait: the candidate was chosen because nothing is bound to it. + close(state.repo, tenant, force: true) + end + end + + state + end + + # The sidecars only exist between a write and a checkpoint, but when they do they + # hold committed data the database file does not -- so a move that left them + # behind would lose the most recent writes. + defp move(source, target) do + with :ok <- File.rename(source, target) do + for {sidecar, renamed} <- Enum.zip(Database.sidecars(source), Database.sidecars(target)), + sidecar != source, + File.exists?(sidecar) do + File.rename(sidecar, renamed) + end + + :ok + end + end + + defp await_quiescence(_repo, _tenant, grace_ms) when grace_ms <= 0, do: :ok + + defp await_quiescence(repo, tenant, grace_ms) do + if Binds.count(repo, tenant) == 0 do + :ok + else + Process.sleep(1) + await_quiescence(repo, tenant, grace_ms - 1) + end + end +end diff --git a/lib/multi_tenancy/migrations.ex b/lib/multi_tenancy/migrations.ex new file mode 100644 index 0000000..06a2212 --- /dev/null +++ b/lib/multi_tenancy/migrations.ex @@ -0,0 +1,85 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Migrations do + @moduledoc """ + Compiles a directory of migrations once, rather than once per tenant. + """ + + @doc "The `{version, module}` pairs in `path`, compiling them if needed." + @spec load!(Path.t()) :: [{integer(), module()}] + def load!(path) do + # Keyed by a fingerprint of the directory, so adding a migration invalidates the + # cache without a restart. + fingerprint = fingerprint(path) + + case :persistent_term.get(key(path), nil) do + {^fingerprint, loaded} -> loaded + _ -> compile!(path, fingerprint) + end + end + + @doc "Forgets the compiled migrations for `path`. For tests." + @spec forget(Path.t()) :: :ok + def forget(path) do + :persistent_term.erase(key(path)) + :ok + end + + # `:persistent_term` because this is written once per directory per boot and read + # on every activation. Its cost is in writes, which is why residency is not stored + # here. + defp compile!(path, fingerprint) do + loaded = path |> files() |> Enum.map(&load_file!/1) |> Enum.sort() + :persistent_term.put(key(path), {fingerprint, loaded}) + loaded + end + + # Mirrors `Ecto.Migrator`'s own naming rules: `_.exs`, and a file + # ending in `.ex` is not a migration however much it looks like one. + defp files(path) do + [path, "**", "*.exs"] + |> Path.join() + |> Path.wildcard() + |> Enum.filter(&version_of(&1)) + end + + defp version_of(file) do + case Integer.parse(Path.rootname(Path.basename(file))) do + {version, "_" <> _name} -> version + _ -> nil + end + end + + defp load_file!(file) do + modules = file |> Code.compile_file() |> Enum.map(&elem(&1, 0)) + + case Enum.find(modules, &migration?/1) do + nil -> + raise Ecto.MigrationError, + "file #{Path.relative_to_cwd(file)} does not define an Ecto.Migration" + + module -> + {version_of(file), module} + end + end + + defp migration?(module) do + Code.ensure_loaded?(module) and function_exported?(module, :__migration__, 0) + end + + defp fingerprint(path) do + path + |> files() + |> Enum.map(fn file -> + case File.stat(file, time: :posix) do + {:ok, stat} -> {file, stat.size, stat.mtime} + {:error, reason} -> {file, reason} + end + end) + |> Enum.sort() + end + + defp key(path), do: {__MODULE__, Path.expand(path)} +end diff --git a/lib/multi_tenancy/registry.ex b/lib/multi_tenancy/registry.ex new file mode 100644 index 0000000..dd6efad --- /dev/null +++ b/lib/multi_tenancy/registry.ex @@ -0,0 +1,65 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Registry do + @moduledoc """ + Maps a tenant to the process holding its connection, and to that connection's repo instance. + """ + + @doc false + def child_spec(repo) do + %{id: {__MODULE__, repo}, start: {__MODULE__, :start_link, [repo]}, type: :supervisor} + end + + @doc false + def start_link(repo) do + Registry.start_link( + keys: :unique, + name: name(repo), + # Every tenanted statement takes a lookup, so reads are the hot path. + # Every tenanted statement does a lookup, so reads are the hot path and the + # partitions are what keep them off one ETS table. + partitions: System.schedulers_online() + ) + end + + @doc "The registry serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantRegistry) + + @doc "The name to start a tenant's connection process under." + @spec via(module(), String.t()) :: {:via, module(), {module(), String.t(), nil}} + def via(repo, tenant), do: {:via, Registry, {name(repo), tenant, nil}} + + @doc "Publishes the repo instance pid for `tenant`." + @spec publish(module(), String.t(), pid()) :: :ok + def publish(repo, tenant, repo_pid) do + Registry.update_value(name(repo), tenant, fn _ -> repo_pid end) + :ok + end + + @doc "The connection process and repo instance serving `tenant`." + @spec lookup(module(), String.t()) :: {:ok, pid(), pid() | nil} | :error + def lookup(repo, tenant) do + case Registry.lookup(name(repo), tenant) do + # An entry outlives its process until the partition handles the DOWN, so a + # hit is not proof of life and a dead pid must read as absent. + [{connection, repo_pid}] -> + if Process.alive?(connection), do: {:ok, connection, repo_pid}, else: :error + + [] -> + :error + end + end + + @doc "Tenants with a connection open on this node right now." + @spec resident(module()) :: [String.t()] + def resident(repo) do + Registry.select(name(repo), [{{:"$1", :_, :_}, [], [:"$1"]}]) + end + + @doc "How many tenants are resident." + @spec count(module()) :: non_neg_integer() + def count(repo), do: Registry.count(name(repo)) +end diff --git a/lib/multi_tenancy/unavailable_error.ex b/lib/multi_tenancy/unavailable_error.ex new file mode 100644 index 0000000..ddf3c21 --- /dev/null +++ b/lib/multi_tenancy/unavailable_error.ex @@ -0,0 +1,20 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.UnavailableError do + @moduledoc """ + Raised when a tenant's database cannot be reached, rather than letting a statement run unbound. + """ + defexception [:tenant, :reason] + + @impl true + def message(%{tenant: tenant, reason: reason}) do + """ + the database for tenant #{inspect(tenant)} is unavailable: #{inspect(reason)} + + No statement was run. Each tenant is its own SQLite file, so running without \ + one would have run against another tenant's data. + """ + end +end diff --git a/lib/tenant_binder.ex b/lib/tenant_binder.ex index 295cd35..c7dbe63 100644 --- a/lib/tenant_binder.ex +++ b/lib/tenant_binder.ex @@ -8,7 +8,7 @@ defmodule AshSqlite.TenantBinder do SQLite has no schemas, so `strategy :context` cannot be a query prefix: the SQL is identical for every tenant and isolation comes from which file the connection is - attached to. + attached to. Defaults to `AshSqlite.MultiTenancy.Binder`; name your own to replace it. """ @typedoc """ diff --git a/lib/verifiers/verify_tenant_binder.ex b/lib/verifiers/verify_tenant_binder.ex deleted file mode 100644 index f4f59a6..0000000 --- a/lib/verifiers/verify_tenant_binder.ex +++ /dev/null @@ -1,39 +0,0 @@ -# SPDX-FileCopyrightText: 2023 ash_sqlite contributors -# -# SPDX-License-Identifier: MIT - -defmodule AshSqlite.Verifiers.VerifyTenantBinder do - @moduledoc false - use Spark.Dsl.Verifier - - alias Spark.Dsl.Verifier - alias Spark.Error.DslError - - @impl true - def verify(dsl) do - if Ash.Resource.Info.multitenancy_strategy(dsl) == :context and - is_nil(Verifier.get_option(dsl, [:sqlite], :tenant_binder)) do - {:error, - DslError.exception( - module: Verifier.get_persisted(dsl, :module), - path: [:sqlite, :tenant_binder], - message: """ - `strategy :context` needs a `tenant_binder`. - - SQLite has no schemas, so the tenant cannot be a query prefix: the SQL is \ - identical for every tenant and isolation comes from which file the \ - connection is attached to. Something has to choose that connection, and \ - only the application knows how. - - sqlite do - tenant_binder MyApp.TenantBinder - end - - See `AshSqlite.TenantBinder`.\ - """ - )} - else - :ok - end - end -end diff --git a/mix.exs b/mix.exs index 4c3330d..b1819f6 100644 --- a/mix.exs +++ b/mix.exs @@ -119,7 +119,9 @@ defmodule AshSqlite.MixProject do AshSqlite.DataLayer ], Multitenancy: [ - AshSqlite.TenantBinder + AshSqlite.MultiTenancy, + AshSqlite.TenantBinder, + AshSqlite.MultiTenancy.UnavailableError ], Utilities: [ AshSqlite.ManualRelationship diff --git a/test/multi_tenancy/binds_test.exs b/test/multi_tenancy/binds_test.exs new file mode 100644 index 0000000..b0498b0 --- /dev/null +++ b/test/multi_tenancy/binds_test.exs @@ -0,0 +1,167 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.BindsTest do + @moduledoc """ + A bound tenant is never evictable, and a closing tenant accepts no new binds. + """ + use ExUnit.Case, async: true + + alias AshSqlite.MultiTenancy.Binds + + defmodule Repo do + @moduledoc false + end + + setup do + start_supervised!(Binds.child_spec(Repo)) + :ok + end + + describe "bound/2 and released/2" do + test "count the processes using a tenant" do + assert Binds.count(Repo, "acme") == 0 + + assert :ok = Binds.bound(Repo, "acme") + assert :ok = Binds.bound(Repo, "acme") + assert Binds.count(Repo, "acme") == 2 + + Binds.released(Repo, "acme") + assert Binds.count(Repo, "acme") == 1 + end + + test "count tenants separately" do + Binds.bound(Repo, "acme") + + assert Binds.count(Repo, "acme") == 1 + assert Binds.count(Repo, "globex") == 0 + end + + test "clamp at zero, so an unmatched release cannot make a busy tenant look idle" do + Binds.released(Repo, "acme") + Binds.released(Repo, "acme") + assert Binds.count(Repo, "acme") == 0 + + Binds.bound(Repo, "acme") + assert Binds.count(Repo, "acme") == 1 + end + + test "releasing marks the tenant used" do + refute Binds.last_used(Repo, "acme") + + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + + assert is_integer(Binds.last_used(Repo, "acme")) + end + end + + describe "closing" do + test "refuses new binds while it is marked" do + Binds.begin_closing(Repo, "acme") + + assert :closing = Binds.bound(Repo, "acme") + assert Binds.count(Repo, "acme") == 0 + end + + test "accepts binds again once cleared" do + Binds.begin_closing(Repo, "acme") + Binds.end_closing(Repo, "acme") + + assert :ok = Binds.bound(Repo, "acme") + end + + test "marks one tenant without affecting another" do + Binds.begin_closing(Repo, "acme") + + assert Binds.closing?(Repo, "acme") + refute Binds.closing?(Repo, "globex") + assert :ok = Binds.bound(Repo, "globex") + end + + test "is idempotent" do + Binds.begin_closing(Repo, "acme") + Binds.begin_closing(Repo, "acme") + Binds.end_closing(Repo, "acme") + + refute Binds.closing?(Repo, "acme") + end + end + + describe "least_recently_used/2" do + test "picks the tenant used longest ago, not the one started longest ago" do + for tenant <- ["acme", "globex", "initech"] do + Binds.bound(Repo, tenant) + Binds.released(Repo, tenant) + end + + # "acme" was started first but has just been used, so it must not be chosen. + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + + assert Binds.least_recently_used(Repo, ["acme", "globex", "initech"]) == "globex" + end + + test "never picks a tenant with work in flight" do + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + Binds.bound(Repo, "globex") + Binds.released(Repo, "globex") + + # The least recently used tenant is now busy, so the next one must be chosen. + Binds.bound(Repo, "acme") + + assert Binds.least_recently_used(Repo, ["acme", "globex"]) == "globex" + end + + test "never picks a tenant that is already closing" do + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + Binds.bound(Repo, "globex") + Binds.released(Repo, "globex") + Binds.begin_closing(Repo, "acme") + + assert Binds.least_recently_used(Repo, ["acme", "globex"]) == "globex" + end + + test "is nil when every candidate is in use, rather than choosing one anyway" do + Binds.bound(Repo, "acme") + Binds.bound(Repo, "globex") + + refute Binds.least_recently_used(Repo, ["acme", "globex"]) + end + + test "is nil for no candidates" do + refute Binds.least_recently_used(Repo, []) + end + + test "prefers a tenant never used over one used recently" do + Binds.bound(Repo, "globex") + Binds.released(Repo, "globex") + + assert Binds.least_recently_used(Repo, ["acme", "globex"]) == "acme" + end + end + + describe "forget/2" do + test "drops everything recorded about a tenant" do + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + Binds.begin_closing(Repo, "acme") + + Binds.forget(Repo, "acme") + + assert Binds.count(Repo, "acme") == 0 + refute Binds.last_used(Repo, "acme") + refute Binds.closing?(Repo, "acme") + end + + test "leaves other tenants alone" do + Binds.bound(Repo, "globex") + Binds.forget(Repo, "acme") + + assert Binds.count(Repo, "globex") == 1 + end + end +end diff --git a/test/multi_tenancy/connection_test.exs b/test/multi_tenancy/connection_test.exs new file mode 100644 index 0000000..c934666 --- /dev/null +++ b/test/multi_tenancy/connection_test.exs @@ -0,0 +1,301 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.ConnectionTest do + @moduledoc """ + A connection may only exist once its database is open and migrated, so most of this is refusal. + """ + use ExUnit.Case, async: false + + @moduletag :capture_log + + alias AshSqlite.MultiTenancy.Connection + alias AshSqlite.MultiTenancy.Database + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + @repo AshSqlite.TestRepo + + setup context do + start_supervised!(TenantRegistry.child_spec(@repo)) + + dir = Path.join(System.tmp_dir!(), "ash_sqlite_tenancy_#{:erlang.phash2(context.test)}") + File.rm_rf!(dir) + File.mkdir_p!(dir) + on_exit(fn -> File.rm_rf!(dir) end) + + {:ok, dir: dir} + end + + describe "starting" do + test "opens the tenant's database and publishes the repo instance", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + assert {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + assert is_pid(repo_pid) + + # The pool connects lazily, so the file appears on first use rather than at + # start. Anything that touches the database is enough. + assert {:ok, _} = query(connection, "select 1") + assert File.exists?(Database.path(dir, "acme")) + end + + test "a tenant whose name is not a filename still gets its own file", %{dir: dir} do + {:ok, colon} = start_connection(dir, "acme:2026-08") + {:ok, underscore} = start_connection(dir, "acme_2026-08") + + query!(colon, "create table only_in_colon (id integer)") + query!(underscore, "select 1") + + assert File.exists?(Path.join(dir, "acme~3a2026-08.db")) + assert File.exists?(Path.join(dir, "acme_2026-08.db")) + + # The point of the encoding: these are two databases, not one shared by two + # tenants whose names sanitise to the same thing. + assert {:error, _} = query(underscore, "select * from only_in_colon") + end + + test "refuses a second connection for the same tenant", %{dir: dir} do + {:ok, first} = start_connection(dir, "acme") + + assert {:error, {:already_started, ^first}} = start_connection(dir, "acme") + end + + test "info/1 reports what is being served", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + info = Connection.info(connection) + + assert info.tenant == "acme" + assert info.path == Database.path(dir, "acme") + assert is_integer(info.opened_at) + end + end + + describe "migrations" do + test "run before the tenant is available", %{dir: dir} do + migrations = migrations_dir(dir, [{20_260_101_000_000, "create_widgets", :create_widgets}]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: migrations) + + assert Connection.info(connection).schema_version == 20_260_101_000_000 + assert {:ok, %{rows: [[0]]}} = query(connection, "select count(*) from widgets") + end + + test "are not re-run for a tenant that is already up to date", %{dir: dir} do + migrations = migrations_dir(dir, [{20_260_101_000_000, "create_widgets", :create_widgets}]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: migrations) + query!(connection, "insert into widgets (name) values ('kept')") + stop_connection(connection) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: migrations) + + # A re-run would have raised on the existing table, and a fresh database + # would have lost the row. + assert {:ok, %{rows: [["kept"]]}} = query(connection, "select name from widgets") + assert Connection.info(connection).schema_version == 20_260_101_000_000 + end + + test "a tenant behind the others catches up on activation", %{dir: dir} do + first = migrations_dir(dir, [{20_260_101_000_000, "create_widgets", :create_widgets}]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: first) + stop_connection(connection) + + both = + migrations_dir(dir, [ + {20_260_101_000_000, "create_widgets", :create_widgets}, + {20_260_202_000_000, "add_colour", :add_colour} + ]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: both) + + assert Connection.info(connection).schema_version == 20_260_202_000_000 + assert {:ok, _} = query(connection, "select colour from widgets") + end + + test "a migrations path that is not a directory is a configuration error", %{dir: dir} do + Process.flag(:trap_exit, true) + missing = Path.join(dir, "nope") + + assert {:error, {:missing_migrations_path, ^missing}} = + start_connection(dir, "acme", migrations_path: missing) + + # Distinct from a migration that failed: this one is true of every tenant on + # the node, and quarantining one tenant over it would hide that. + assert :error = TenantRegistry.lookup(@repo, "acme") + refute File.exists?(Database.path(dir, "acme")) + end + + test "a migration that raises stops the connection rather than serving", %{dir: dir} do + Process.flag(:trap_exit, true) + migrations = migrations_dir(dir, [{20_260_101_000_000, "explode", :raise}]) + + assert {:error, {:migration_failed, _}} = + start_connection(dir, "acme", migrations_path: migrations) + + assert :error = TenantRegistry.lookup(@repo, "acme") + end + + test "no migrations path means no migration, not a failure", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + refute Connection.info(connection).schema_version + end + end + + describe "encryption" do + test "refuses to open a plaintext database when a key was expected", %{dir: dir} do + Process.flag(:trap_exit, true) + + assert {:error, :no_key} = start_connection(dir, "acme", encrypted?: true, key: nil) + refute File.exists?(Database.path(dir, "acme")) + end + + test "an unencrypted fleet opens without a key", %{dir: dir} do + assert {:ok, _} = start_connection(dir, "acme", encrypted?: false, key: nil) + end + end + + describe "lifetime" do + test "the connection stops when its repo instance dies", %{dir: dir} do + Process.flag(:trap_exit, true) + {:ok, connection} = start_connection(dir, "acme") + {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + + ref = Process.monitor(connection) + Process.exit(repo_pid, :kill) + + assert_receive {:DOWN, ^ref, :process, ^connection, {:repo_exited, _}}, 2_000 + assert :error = TenantRegistry.lookup(@repo, "acme") + end + + test "stop_repo/1 closes the database but keeps the process", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + + assert :ok = Connection.stop_repo(connection) + + refute Process.alive?(repo_pid) + assert Process.alive?(connection) + assert Connection.repo_pid(connection) == nil + end + + test "stop_repo/1 twice is harmless", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + assert :ok = Connection.stop_repo(connection) + assert :ok = Connection.stop_repo(connection) + end + + test "stopping the connection closes the repo instance", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + + stop_connection(connection) + + refute Process.alive?(repo_pid) + assert :error = TenantRegistry.lookup(@repo, "acme") + end + end + + # `AshSqlite.TestRepo` is configured with the sandbox pool, and a tenant instance + # inherits the repo module's application config. + defp start_connection(dir, tenant, opts \\ []) do + Connection.start_link( + Keyword.merge( + [ + repo: @repo, + tenant: tenant, + path: Database.path(dir, tenant), + repo_opts: [pool: DBConnection.ConnectionPool] + ], + opts + ) + ) + end + + defp stop_connection(connection) do + ref = Process.monitor(connection) + GenServer.stop(connection) + assert_receive {:DOWN, ^ref, :process, ^connection, _}, 2_000 + end + + defp query(connection, sql) do + repo_pid = Connection.repo_pid(connection) + previous = @repo.put_dynamic_repo(repo_pid) + + try do + # `@repo.query/1`, not `Ecto.Adapters.SQL.query(@repo, ...)`: the latter looks up + # the module's adapter meta and ignores the dynamic binding. + @repo.query(sql) + after + @repo.put_dynamic_repo(previous) + end + end + + defp query!(connection, sql) do + {:ok, result} = query(connection, sql) + result + end + + defp migrations_dir(dir, migrations) do + path = Path.join(dir, "migrations") + File.mkdir_p!(path) + + for {version, name, behaviour} <- migrations do + # A unique module per written file, so that rewriting a directory to add a + # later migration does not redefine the earlier one's module. + module = + Module.concat([ + Migrations, + :"M#{:erlang.phash2(dir)}", + :"V#{version}_#{System.unique_integer([:positive])}" + ]) + + File.write!(Path.join(path, "#{version}_#{name}.exs"), migration_source(module, behaviour)) + end + + path + end + + defp migration_source(module, :raise) do + """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def up, do: raise "no" + def down, do: :ok + end + """ + end + + defp migration_source(module, :create_widgets) do + """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + create table(:widgets, primary_key: false) do + add :name, :text + end + end + end + """ + end + + defp migration_source(module, :add_colour) do + """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + alter table(:widgets) do + add :colour, :text + end + end + end + """ + end +end diff --git a/test/multi_tenancy/database_test.exs b/test/multi_tenancy/database_test.exs new file mode 100644 index 0000000..8ca96b3 --- /dev/null +++ b/test/multi_tenancy/database_test.exs @@ -0,0 +1,213 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.DatabaseTest do + @moduledoc """ + A tenant becomes a filename, so this is where two tenants would come to share a database. + """ + use ExUnit.Case, async: true + + alias AshSqlite.MultiTenancy.Database + + @seed 20_260_822 + + describe "encode/1 and decode/1 round trip" do + test "every single byte survives" do + for byte <- 0..255 do + tenant = <> + assert {:ok, ^tenant} = Database.decode(Database.encode(tenant)) + end + end + + test "tenants people actually use survive" do + for tenant <- corpus() do + assert {:ok, ^tenant} = Database.decode(Database.encode(tenant)), + "did not round trip: #{inspect(tenant)}" + end + end + + test "generated tenants survive, and no two of them collide" do + tenants = generated_tenants() + + for tenant <- tenants do + assert {:ok, ^tenant} = Database.decode(Database.encode(tenant)), + "did not round trip: #{inspect(tenant)} (seed #{@seed})" + end + + encoded = Enum.map(tenants, &Database.encode/1) + + assert Enum.count(Enum.uniq(encoded)) == Enum.count(Enum.uniq(tenants)), + "two distinct tenants encoded to one name (seed #{@seed})" + end + end + + describe "collisions the obvious implementation would allow" do + test "punctuation is not flattened into the character that replaces it" do + refute Database.encode("a:b") == Database.encode("a_b") + refute Database.encode("a/b") == Database.encode("a_b") + refute Database.encode("a b") == Database.encode("a-b") + end + + test "an escape in the tenant does not alias an escape we produced" do + refute Database.encode("~3a") == Database.encode(":") + end + + test "encodings never differ only by case, so a case-insensitive filesystem is safe" do + for tenant <- corpus() ++ generated_tenants() do + encoded = Database.encode(tenant) + + assert String.downcase(encoded) == encoded, + "#{inspect(tenant)} encoded to #{inspect(encoded)}, which has case (seed #{@seed})" + end + end + + test "tenants differing only by case get different files" do + refute Database.encode("Acme") == Database.encode("acme") + assert Database.encode("Acme") == "~41cme" + end + end + + describe "encode/1 keeps the name inside the directory" do + test "no encoded name contains a path separator or a null" do + for tenant <- ["../../etc/passwd", "a/b", "a\\b", "..", ".", "/", <<0>>] do + encoded = Database.encode(tenant) + refute String.contains?(encoded, ["/", "\\", <<0>>]) + end + end + + test "a traversing tenant stays a single file in dir" do + path = Database.path("/tmp/tenants", "../../etc/passwd") + + assert path == "/tmp/tenants/..~2f..~2fetc~2fpasswd.db" + assert Path.dirname(Path.expand(path)) == "/tmp/tenants" + end + + test "dot-only tenants are ordinary filenames" do + assert Path.dirname(Path.expand(Database.path("/tmp/tenants", "."))) == "/tmp/tenants" + assert Path.dirname(Path.expand(Database.path("/tmp/tenants", ".."))) == "/tmp/tenants" + end + end + + describe "encode/1 refuses what it cannot name" do + test "a non-binary tenant, rather than stringifying it into a collision" do + assert_raise ArgumentError, ~r/expected a binary tenant/, fn -> Database.encode(:acme) end + assert_raise ArgumentError, ~r/expected a binary tenant/, fn -> Database.encode(1) end + assert_raise ArgumentError, ~r/expected a binary tenant/, fn -> Database.encode(nil) end + end + + test "an empty tenant" do + assert_raise ArgumentError, ~r/cannot be an empty string/, fn -> Database.encode("") end + end + + test "a tenant too long to be a filename, before the filesystem does" do + assert_raise ArgumentError, ~r/encodes to \d+ bytes/, fn -> + Database.encode(String.duplicate("a", 249)) + end + + # Escaped bytes cost three each, so the limit arrives three times sooner. + assert_raise ArgumentError, ~r/encodes to \d+ bytes/, fn -> + Database.encode(String.duplicate(":", 83)) + end + end + + test "the longest name that does fit is accepted" do + assert byte_size(Database.encode(String.duplicate("a", 248))) == 248 + end + end + + describe "decode/1 declines what encode/1 could not have produced" do + test "an unterminated or malformed escape" do + assert :error = Database.decode("acme~") + assert :error = Database.decode("acme~3") + assert :error = Database.decode("acme~zz") + assert :error = Database.decode("acme~3z") + end + + test "uppercase hex, which we never emit" do + assert :error = Database.decode("acme~3A") + end + + test "a byte that would have been escaped, appearing raw" do + assert :error = Database.decode("acme:2026") + assert :error = Database.decode("Acme") + assert :error = Database.decode("acme name") + end + + test "an empty name" do + assert :error = Database.decode("") + end + end + + describe "sidecars/1" do + test "names the WAL and shared-memory files alongside the database" do + assert Database.sidecars("/tmp/t/acme.db") == [ + "/tmp/t/acme.db", + "/tmp/t/acme.db-wal", + "/tmp/t/acme.db-shm" + ] + end + end + + describe "tenant_from_path/2" do + test "recovers the tenant of a database in the directory" do + path = Database.path("/tmp/tenants", "acme:2026-08") + assert {:ok, "acme:2026-08"} = Database.tenant_from_path("/tmp/tenants", path) + end + + test "declines a path outside the directory" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/other/acme.db") + end + + test "declines a path nested below the directory" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/tenants/sub/acme.db") + end + + test "declines a file that is not a database" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/tenants/acme.sqlite") + end + + test "declines a name no tenant could have produced" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/tenants/Backup.db") + end + end + + defp corpus do + [ + "acme", + "acme-corp", + "acme_corp", + "acme.corp", + "ACME", + "Acme", + "acme:2026-08", + "acme:billing", + "0191f3d0-4d1e-7c3a-9c9e-1b2c3d4e5f60", + "0191F3D0-4D1E-7C3A-9C9E-1B2C3D4E5F60", + "tenant 42", + "~", + "~3a", + "..", + ".", + "/", + "../../etc/passwd", + "日本語", + "emoji-🙂", + <<0>>, + <<255, 254, 253>>, + "a\nb", + "'; drop table users; --" + ] + end + + # Includes invalid UTF-8: a tenant is whatever the application put in `set_tenant/2`. + defp generated_tenants do + :rand.seed(:exsss, {@seed, @seed, @seed}) + + for _ <- 1..2_000 do + length = :rand.uniform(24) + for _ <- 1..length, into: <<>>, do: <<:rand.uniform(256) - 1>> + end + |> Enum.uniq() + end +end diff --git a/test/multi_tenancy/managed_test.exs b/test/multi_tenancy/managed_test.exs new file mode 100644 index 0000000..755eb7f --- /dev/null +++ b/test/multi_tenancy/managed_test.exs @@ -0,0 +1,231 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.ManagedTest do + @moduledoc """ + A `strategy :context` resource with nothing configured but `AshSqlite.MultiTenancy`, + driven through Ash and checked against the files on disk. + """ + use ExUnit.Case, async: false + + @moduletag :capture_log + + alias AshSqlite.ManagedTenantRepo + alias AshSqlite.MultiTenancy + alias AshSqlite.Test.ManagedPost + + require Ash.Query + + setup context do + dir = Path.join(System.tmp_dir!(), "ash_sqlite_managed_#{:erlang.phash2(context.test)}") + File.rm_rf!(dir) + on_exit(fn -> File.rm_rf!(dir) end) + + start_supervised!( + {MultiTenancy, + repo: ManagedTenantRepo, + dir: dir, + migrations_path: migrations(dir), + repo_opts: [pool: DBConnection.ConnectionPool]} + ) + + {:ok, dir: dir} + end + + describe "wiring" do + test "a context-multitenant resource with no binder gets the default" do + assert AshSqlite.DataLayer.Info.tenant_binder(ManagedPost) == AshSqlite.MultiTenancy.Binder + end + + test "a resource that names a binder keeps it" do + assert AshSqlite.DataLayer.Info.tenant_binder(AshSqlite.Test.TenantedPost) == + AshSqlite.Test.TenantBinder + end + + test "a resource with no context multitenancy has no binder" do + refute AshSqlite.DataLayer.Info.tenant_binder(AshSqlite.Test.Post) + end + end + + describe "reads and writes" do + test "a create lands in its own tenant's file", %{dir: dir} do + create!("acme", "acme one") + create!("acme", "acme two") + create!("globex", "globex one") + + assert titles_in_file(dir, "acme") == ["acme one", "acme two"] + assert titles_in_file(dir, "globex") == ["globex one"] + end + + test "a read only sees its own tenant" do + create!("acme", "acme one") + create!("globex", "globex one") + + assert titles("acme") == ["acme one"] + assert titles("globex") == ["globex one"] + end + + test "an update stays in its own tenant", %{dir: dir} do + post = create!("acme", "before") + create!("globex", "untouched") + + post |> Ash.Changeset.for_update(:update, %{title: "after"}) |> Ash.update!(tenant: "acme") + + assert titles_in_file(dir, "acme") == ["after"] + assert titles_in_file(dir, "globex") == ["untouched"] + end + + test "a destroy stays in its own tenant", %{dir: dir} do + post = create!("acme", "doomed") + create!("globex", "safe") + + Ash.destroy!(post, tenant: "acme") + + assert titles_in_file(dir, "acme") == [] + assert titles_in_file(dir, "globex") == ["safe"] + end + end + + describe "the paths a caller could not have wrapped" do + test "Ash.count/2 goes through the binder with the right tenant" do + create!("acme", "one") + create!("globex", "one") + create!("globex", "two") + + assert Ash.count!(ManagedPost, tenant: "acme") == 1 + assert Ash.count!(ManagedPost, tenant: "globex") == 2 + end + + test "an atomic bulk update stays in its own tenant", %{dir: dir} do + create!("acme", "before") + create!("globex", "before") + + ManagedPost + |> Ash.Query.filter(title == "before") + |> Ash.bulk_update!(:update, %{title: "after"}, tenant: "acme", strategy: :atomic) + + assert titles_in_file(dir, "acme") == ["after"] + assert titles_in_file(dir, "globex") == ["before"] + end + end + + describe "tenants" do + test "each tenant is a separate file, named as MultiTenancy says", %{dir: dir} do + create!("acme", "one") + + assert File.exists?(MultiTenancy.path_for(ManagedTenantRepo, "acme")) + assert MultiTenancy.path_for(ManagedTenantRepo, "acme") == Path.join(dir, "acme.db") + end + + test "a tenant met for the first time is migrated on activation", %{dir: dir} do + create!("acme", "one") + + # "globex" has never been seen, so its database does not exist yet and its + # table can only come from the migration running at activation. + refute File.exists?(Path.join(dir, "globex.db")) + assert create!("globex", "one").title == "one" + assert titles_in_file(dir, "globex") == ["one"] + end + + test "a tenant reopened after being closed keeps its rows" do + create!("acme", "durable") + :ok = MultiTenancy.close(ManagedTenantRepo, "acme") + + assert MultiTenancy.resident(ManagedTenantRepo) == [] + assert titles("acme") == ["durable"] + end + + test "tenants whose names would sanitise alike stay separate", %{dir: dir} do + create!("acme:eu", "colon") + + assert titles("acme_eu") == [] + assert titles_in_file(dir, "acme:eu") == ["colon"] + end + end + + describe "refusals" do + test "Ash refuses a tenantless query before the data layer sees it" do + assert_raise Ash.Error.Invalid, ~r/require a tenant to be specified/, fn -> + Ash.read!(ManagedPost) + end + end + + test "the data layer refuses one on the paths that bypass an action" do + assert_raise ArgumentError, ~r/carried no tenant/, fn -> + AshSqlite.DataLayer.transaction(ManagedPost, fn -> :unreachable end) + end + end + end + + describe "transactions" do + test "a failed action rolls its write back", %{dir: dir} do + assert_raise RuntimeError, fn -> + AshSqlite.DataLayer.transaction( + ManagedPost, + fn -> + create!("acme", "rolled back") + raise "no" + end, + nil, + %{type: :custom, metadata: %{}, data_layer_context: %{tenant: "acme"}} + ) + end + + assert titles_in_file(dir, "acme") == [] + end + + test "a transaction refuses to reach into another tenant's database" do + assert_raise Ash.Error.Unknown, ~r/open on another tenant's database/, fn -> + AshSqlite.DataLayer.transaction( + ManagedPost, + fn -> create!("globex", "wrong database") end, + nil, + %{type: :custom, metadata: %{}, data_layer_context: %{tenant: "acme"}} + ) + end + end + end + + defp create!(tenant, title) do + ManagedPost + |> Ash.Changeset.for_create(:create, %{title: title}) + |> Ash.create!(tenant: tenant) + end + + defp titles(tenant) do + ManagedPost |> Ash.read!(tenant: tenant) |> Enum.map(& &1.title) |> Enum.sort() + end + + # Read with Exqlite rather than through Ash, so isolation is checked against the + # bytes on disk and not against the thing under test. + defp titles_in_file(dir, tenant) do + path = Path.join(dir, AshSqlite.MultiTenancy.Database.encode(tenant) <> ".db") + {:ok, db} = Exqlite.Sqlite3.open(path) + {:ok, stmt} = Exqlite.Sqlite3.prepare(db, "SELECT title FROM managed_posts ORDER BY title") + {:ok, rows} = Exqlite.Sqlite3.fetch_all(db, stmt) + :ok = Exqlite.Sqlite3.close(db) + List.flatten(rows) + end + + defp migrations(dir) do + path = Path.join(dir, "migrations") + File.mkdir_p!(path) + module = Module.concat([ManagedMigrations, :"M#{:erlang.phash2(dir)}"]) + + File.write!(Path.join(path, "20260101000000_create_managed_posts.exs"), """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + create table(:managed_posts, primary_key: false) do + add :id, :text, primary_key: true + add :title, :text + end + end + end + """) + + path + end +end diff --git a/test/multi_tenancy/multi_tenancy_test.exs b/test/multi_tenancy/multi_tenancy_test.exs new file mode 100644 index 0000000..4b65aa8 --- /dev/null +++ b/test/multi_tenancy/multi_tenancy_test.exs @@ -0,0 +1,483 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancyTest do + @moduledoc """ + The runtime end to end, against real database files. + """ + use ExUnit.Case, async: false + + @moduletag :capture_log + + alias AshSqlite.MultiTenancy + alias AshSqlite.MultiTenancy.Database + + @repo AshSqlite.TestRepo + + setup context do + dir = Path.join(System.tmp_dir!(), "ash_sqlite_tenancy_#{:erlang.phash2(context.test)}") + File.rm_rf!(dir) + on_exit(fn -> File.rm_rf!(dir) end) + + {:ok, dir: dir} + end + + describe "connection_for/2" do + test "activates a tenant that is not resident", %{dir: dir} do + start_tenancy(dir) + + assert {:ok, repo_pid} = MultiTenancy.connection_for(@repo, "acme") + assert is_pid(repo_pid) + assert MultiTenancy.resident(@repo) == ["acme"] + end + + test "returns the same connection for a resident tenant", %{dir: dir} do + start_tenancy(dir) + + assert {:ok, first} = MultiTenancy.connection_for(@repo, "acme") + assert {:ok, ^first} = MultiTenancy.connection_for(@repo, "acme") + end + + test "gives each tenant its own connection", %{dir: dir} do + start_tenancy(dir) + + {:ok, acme} = MultiTenancy.connection_for(@repo, "acme") + {:ok, globex} = MultiTenancy.connection_for(@repo, "globex") + + refute acme == globex + end + end + + describe "isolation" do + test "a row written for one tenant is not visible to another", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('acme only')") + end) + + names = + MultiTenancy.with_tenant(@repo, "globex", fn -> + @repo.query!("select name from widgets").rows + end) + + assert names == [] + + assert MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("select name from widgets").rows + end) == [["acme only"]] + end + + test "tenants whose names would sanitise alike get separate databases", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme:eu", fn -> + @repo.query!("insert into widgets (name) values ('colon')") + end) + + assert MultiTenancy.with_tenant(@repo, "acme_eu", fn -> + @repo.query!("select name from widgets").rows + end) == [] + end + end + + describe "with_tenant/3" do + test "restores the previous binding afterwards", %{dir: dir} do + start_tenancy(dir) + before = @repo.get_dynamic_repo() + + MultiTenancy.with_tenant(@repo, "acme", fn -> :ok end) + + assert @repo.get_dynamic_repo() == before + end + + test "restores the previous binding when the function raises", %{dir: dir} do + start_tenancy(dir) + before = @repo.get_dynamic_repo() + + assert_raise RuntimeError, fn -> + MultiTenancy.with_tenant(@repo, "acme", fn -> raise "boom" end) + end + + assert @repo.get_dynamic_repo() == before + end + + test "raises rather than running against whatever was bound", %{dir: dir} do + start_tenancy(dir, key_for: fn _tenant -> nil end) + + assert_raise AshSqlite.MultiTenancy.UnavailableError, ~r/unavailable/, fn -> + MultiTenancy.with_tenant(@repo, "acme", fn -> :never end) + end + end + end + + describe "residency" do + test "closes the least recently used tenant beyond max_resident", %{dir: dir} do + start_tenancy(dir, max_resident: 2) + + {:ok, _} = MultiTenancy.connection_for(@repo, "first") + {:ok, _} = MultiTenancy.connection_for(@repo, "second") + {:ok, _} = MultiTenancy.connection_for(@repo, "third") + + assert Enum.sort(MultiTenancy.resident(@repo)) == ["second", "third"] + end + + test "never closes a tenant with a statement in flight", %{dir: dir} do + start_tenancy(dir, max_resident: 2) + + {:ok, _} = MultiTenancy.connection_for(@repo, "first") + {:ok, _} = MultiTenancy.connection_for(@repo, "second") + + # "first" is the least recently used, and is held bound for the duration. + holder = hold("first") + + {:ok, _} = MultiTenancy.connection_for(@repo, "third") + + assert "first" in MultiTenancy.resident(@repo) + refute "second" in MultiTenancy.resident(@repo) + + release(holder) + end + + test "exceeds the limit rather than refusing when every tenant is in use", %{dir: dir} do + start_tenancy(dir, max_resident: 1) + + {:ok, _} = MultiTenancy.connection_for(@repo, "first") + holder = hold("first") + + assert {:ok, _} = MultiTenancy.connection_for(@repo, "second") + assert Enum.sort(MultiTenancy.resident(@repo)) == ["first", "second"] + + release(holder) + end + + test "a closed tenant keeps its data and reopens on demand", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('durable')") + end) + + :ok = MultiTenancy.close(@repo, "acme") + assert MultiTenancy.resident(@repo) == [] + assert File.exists?(Database.path(dir, "acme")) + + assert MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("select name from widgets").rows + end) == [["durable"]] + end + end + + describe "delete/2" do + test "removes the database and its sidecars", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('gone')") + end) + + assert {:ok, removed} = MultiTenancy.delete(@repo, "acme") + + assert Database.path(dir, "acme") in removed + refute File.exists?(Database.path(dir, "acme")) + refute File.exists?(Database.path(dir, "acme") <> "-wal") + end + end + + describe "rename/3" do + test "carries the tenant's data to the new name", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "carried") + + # The write is still in the WAL rather than the database file, so this fails + # unless the sidecars move with it. + assert File.exists?(Database.path(dir, "acme") <> "-wal") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme-renamed") + + assert read("acme-renamed") == [["carried"]] + refute File.exists?(Database.path(dir, "acme")) + assert File.exists?(Database.path(dir, "acme-renamed")) + end + + test "closes a resident tenant before moving its file", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme-renamed") + assert MultiTenancy.resident(@repo) == [] + end + + test "renames a tenant whose name is not a filename", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme:eu", "colon") + + assert :ok = MultiTenancy.rename(@repo, "acme:eu", "acme:us") + + assert read("acme:us") == [["colon"]] + assert MultiTenancy.all_tenants(@repo) == ["acme:us"] + end + + test "refuses to overwrite a tenant that already has a database", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "keep me") + write("globex", "do not clobber") + + assert {:error, :target_exists} = MultiTenancy.rename(@repo, "acme", "globex") + + assert read("globex") == [["do not clobber"]] + assert read("acme") == [["keep me"]] + end + + test "refuses a tenant with no database, rather than reporting success", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + assert {:error, :no_database} = MultiTenancy.rename(@repo, "never-seen", "renamed") + end + + test "renaming a tenant to itself leaves it alone", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "untouched") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme") + assert read("acme") == [["untouched"]] + end + + test "the new name serves reads and writes afterwards", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "before") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme-renamed") + write("acme-renamed", "after") + + assert read("acme-renamed") == [["after"], ["before"]] + end + end + + describe "quarantine" do + test "a tenant that cannot open is refused without retrying", %{dir: dir} do + start_tenancy(dir, key_for: fn _tenant -> nil end) + + assert {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + assert {:error, {:quarantined, :no_key}} = MultiTenancy.connection_for(@repo, "acme") + assert MultiTenancy.quarantined(@repo) == %{"acme" => :no_key} + end + + test "release/2 lets the next request try again", %{dir: dir} do + start_tenancy(dir, key_for: fn _tenant -> nil end) + + {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + :ok = MultiTenancy.release(@repo, "acme") + + assert MultiTenancy.quarantined(@repo) == %{} + # Still broken, so it fails the same way rather than being quarantined-stale. + assert {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + end + + test "quarantines one tenant without affecting another", %{dir: dir} do + start_tenancy(dir, key_for: fn tenant -> if tenant == "acme", do: nil, else: "unused" end) + + assert {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + assert Map.keys(MultiTenancy.quarantined(@repo)) == ["acme"] + end + end + + describe "seal/1" do + test "refuses new tenants, and unseal/1 allows them again", %{dir: dir} do + start_tenancy(dir) + {:ok, resident} = MultiTenancy.connection_for(@repo, "acme") + + :ok = MultiTenancy.seal(@repo) + + assert {:error, :draining} = MultiTenancy.connection_for(@repo, "globex") + # An already-resident tenant keeps serving, so work in flight can finish. + assert {:ok, ^resident} = MultiTenancy.connection_for(@repo, "acme") + + :ok = MultiTenancy.unseal(@repo) + assert {:ok, _} = MultiTenancy.connection_for(@repo, "globex") + end + end + + describe "all_tenants/1" do + test "is empty for a fleet nobody has used yet", %{dir: dir} do + start_tenancy(dir) + + assert MultiTenancy.all_tenants(@repo) == [] + end + + test "lists a tenant whether it is resident or only on disk", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + {:ok, _} = MultiTenancy.connection_for(@repo, "globex") + :ok = MultiTenancy.close(@repo, "globex") + + assert MultiTenancy.resident(@repo) == ["acme"] + assert Enum.sort(MultiTenancy.all_tenants(@repo)) == ["acme", "globex"] + end + + test "lists a resident tenant that has not written its file yet", %{dir: dir} do + start_tenancy(dir) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + + assert MultiTenancy.all_tenants(@repo) == ["acme"] + end + + test "recovers from disk a tenant whose name is not a filename", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme:eu/1") + :ok = MultiTenancy.close(@repo, "acme:eu/1") + + assert MultiTenancy.all_tenants(@repo) == ["acme:eu/1"] + end + + test "counts a tenant once, not once per file it owns", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('one')") + end) + + assert Enum.any?(File.ls!(dir), &String.ends_with?(&1, "-wal")) + assert MultiTenancy.all_tenants(@repo) == ["acme"] + end + + test "ignores files no tenant could have produced", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + :ok = MultiTenancy.close(@repo, "acme") + File.write!(Path.join(dir, "notes.txt"), "") + File.write!(Path.join(dir, "Backup.db"), "") + + assert MultiTenancy.all_tenants(@repo) == ["acme"] + end + end + + describe "migrate_all/3" do + test "migrates each tenant and closes it again", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + results = MultiTenancy.migrate_all(@repo, ["acme", "globex"]) + + assert results == [ + {"acme", {:ok, 20_260_101_000_000}}, + {"globex", {:ok, 20_260_101_000_000}} + ] + + assert MultiTenancy.resident(@repo) == [] + assert File.exists?(Database.path(dir, "acme")) + assert File.exists?(Database.path(dir, "globex")) + end + + test "migrates every tenant on disk when given none", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + {:ok, _} = MultiTenancy.connection_for(@repo, "globex") + + assert [{"acme", {:ok, _}}, {"globex", {:ok, _}}] = + @repo |> MultiTenancy.migrate_all() |> Enum.sort() + end + + test "reports the tenants that failed rather than stopping", %{dir: dir} do + start_tenancy(dir, key_for: fn tenant -> if tenant == "acme", do: nil, else: "unused" end) + + assert [{"acme", {:error, :no_key}}, {"globex", _}] = + MultiTenancy.migrate_all(@repo, ["acme", "globex"]) + end + end + + describe "path_for/2" do + test "names the file a tenant would use", %{dir: dir} do + start_tenancy(dir) + + assert MultiTenancy.path_for(@repo, "acme:eu") == Path.join(dir, "acme~3aeu.db") + end + end + + describe "configuration" do + test "a migrations_path that is not a directory fails at boot", %{dir: dir} do + Process.flag(:trap_exit, true) + + assert {:error, {%ArgumentError{message: message}, _stacktrace}} = + MultiTenancy.start_link( + repo: @repo, + dir: dir, + migrations_path: Path.join(dir, "nope") + ) + + assert message =~ "is not a directory" + end + end + + defp write(tenant, name) do + MultiTenancy.with_tenant(@repo, tenant, fn -> + @repo.query!("insert into widgets (name) values ('#{name}')") + end) + end + + defp read(tenant) do + MultiTenancy.with_tenant(@repo, tenant, fn -> + @repo.query!("select name from widgets order by name").rows + end) + end + + defp start_tenancy(dir, opts \\ []) do + opts = + Keyword.merge( + [ + repo: @repo, + dir: dir, + # A tenant instance inherits the repo module's config, which here names the + # sandbox pool -- one connection shared between processes. + repo_opts: [pool: DBConnection.ConnectionPool] + ], + opts + ) + + start_supervised!({MultiTenancy, opts}) + end + + defp migrations(dir) do + path = Path.join(dir, "migrations") + File.mkdir_p!(path) + module = Module.concat([TenancyMigrations, :"M#{:erlang.phash2(dir)}"]) + + File.write!(Path.join(path, "20260101000000_create_widgets.exs"), """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + create table(:widgets, primary_key: false) do + add :name, :text + end + end + end + """) + + path + end + + # Holds a tenant bound in another process until told to stop, so that eviction + # has something genuinely in use to skip. + # Holds a tenant bound in another process, so eviction has something in use to skip. + defp hold(tenant) do + test = self() + + spawn_link(fn -> + MultiTenancy.with_tenant(@repo, tenant, fn -> + send(test, {:holding, self()}) + receive do: (:release -> :ok) + end) + end) + + receive do + {:holding, pid} -> pid + after + 1_000 -> flunk("holder never bound #{tenant}") + end + end + + defp release(holder) do + send(holder, :release) + end +end diff --git a/test/multi_tenancy/registry_test.exs b/test/multi_tenancy/registry_test.exs new file mode 100644 index 0000000..99d44d0 --- /dev/null +++ b/test/multi_tenancy/registry_test.exs @@ -0,0 +1,115 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.RegistryTest do + @moduledoc """ + Registration is the mutual exclusion, and a lookup must never hand out a dead process. + """ + use ExUnit.Case, async: true + + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + defmodule Repo do + @moduledoc false + end + + setup do + start_supervised!(TenantRegistry.child_spec(Repo)) + :ok + end + + describe "via/2" do + test "registers a process under the tenant" do + connection = start_connection("acme") + + assert {:ok, ^connection, nil} = TenantRegistry.lookup(Repo, "acme") + end + + test "refuses a second process for the same tenant, without running its init" do + first = start_connection("acme") + + assert {:error, {:already_started, ^first}} = + Agent.start(fn -> raise "init must not run" end, + name: TenantRegistry.via(Repo, "acme") + ) + end + + test "keeps tenants that differ only by case apart" do + lower = start_connection("acme") + upper = start_connection("ACME") + + refute lower == upper + assert {:ok, ^lower, _} = TenantRegistry.lookup(Repo, "acme") + assert {:ok, ^upper, _} = TenantRegistry.lookup(Repo, "ACME") + end + end + + describe "publish/3" do + test "makes the repo pid available to the next lookup" do + connection = start_connection("acme") + repo_pid = publish_self(connection, "acme") + + assert {:ok, ^connection, ^repo_pid} = TenantRegistry.lookup(Repo, "acme") + end + + test "a connection that has not published yet is found, with no repo" do + connection = start_connection("acme") + + assert {:ok, ^connection, nil} = TenantRegistry.lookup(Repo, "acme") + end + end + + describe "lookup/2" do + test "is :error for a tenant with no connection" do + assert :error = TenantRegistry.lookup(Repo, "nobody") + end + + test "is :error for an entry whose process has died" do + connection = start_connection("acme") + publish_self(connection, "acme") + + ref = Process.monitor(connection) + Process.exit(connection, :kill) + assert_receive {:DOWN, ^ref, :process, ^connection, :killed}, 2_000 + + # The entry may not have been reaped yet -- that is the point. A dead + # connection must read as absent rather than as a pid to bind. + assert :error = TenantRegistry.lookup(Repo, "acme") + end + end + + describe "resident/1 and count/1" do + test "list the tenants with a connection" do + start_connection("acme") + start_connection("globex") + + assert Enum.sort(TenantRegistry.resident(Repo)) == ["acme", "globex"] + assert TenantRegistry.count(Repo) == 2 + end + + test "are empty for a registry nobody has used" do + assert TenantRegistry.resident(Repo) == [] + assert TenantRegistry.count(Repo) == 0 + end + end + + describe "name/1" do + test "is derived from the repo, so two repos do not share a namespace" do + refute TenantRegistry.name(Repo) == + TenantRegistry.name(AshSqlite.MultiTenancy.BindsTest.Repo) + end + end + + defp start_connection(tenant) do + {:ok, pid} = Agent.start(fn -> :ok end, name: TenantRegistry.via(Repo, tenant)) + pid + end + + defp publish_self(connection, tenant) do + Agent.get(connection, fn _ -> + :ok = TenantRegistry.publish(Repo, tenant, self()) + self() + end) + end +end diff --git a/test/multitenancy_test.exs b/test/multitenancy_test.exs index 3108c3f..0417145 100644 --- a/test/multitenancy_test.exs +++ b/test/multitenancy_test.exs @@ -180,57 +180,6 @@ defmodule AshSqlite.MultitenancyTest do end end - describe "a context-multitenant resource with no binder" do - setup do - # Spark reports a verifier failure through `@after_verify`, which the parallel - # checker turns into a warning rather than an exception -- Ash's own - # multitenancy verifier included. So the module compiles, and the runtime is - # what actually stops it. - warnings = - ExUnit.CaptureIO.capture_io(:stderr, fn -> - Code.compile_string(""" - defmodule AshSqlite.Test.Unbindable do - use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer - - actions do - defaults [:read] - end - - multitenancy do - strategy :context - end - - attributes do - uuid_primary_key :id - end - - sqlite do - table "unbindable" - repo AshSqlite.TenantRepo - end - end - """) - end) - - {:ok, warnings: warnings} - end - - test "is told so at compile time", %{warnings: warnings} do - assert warnings =~ "needs a `tenant_binder`" - end - - test "is refused at the first statement, rather than running unbound" do - assert_raise ArgumentError, ~r/no `tenant_binder` to select a connection with/, fn -> - AshSqlite.DataLayer.transaction( - AshSqlite.Test.Unbindable, - fn -> :unreachable end, - nil, - %{type: :custom, metadata: %{}, data_layer_context: %{tenant: "acme"}} - ) - end - end - end - describe "a global? resource" do test "is bound to the tenant it is given, like any other", %{repos: repos} do GlobalPost diff --git a/test/support/domain.ex b/test/support/domain.ex index dfc68cb..5424209 100644 --- a/test/support/domain.ex +++ b/test/support/domain.ex @@ -20,6 +20,7 @@ defmodule AshSqlite.Test.Domain do resource(AshSqlite.Test.TransactionalAccount) resource(AshSqlite.Test.TenantedPost) resource(AshSqlite.Test.GlobalPost) + resource(AshSqlite.Test.ManagedPost) resource(AshSqlite.Test.Organization) resource(AshSqlite.Test.Manager) resource(AshSqlite.Test.Device) diff --git a/test/support/managed_tenant_repo.ex b/test/support/managed_tenant_repo.ex new file mode 100644 index 0000000..f317366 --- /dev/null +++ b/test/support/managed_tenant_repo.ex @@ -0,0 +1,11 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.ManagedTenantRepo do + @moduledoc """ + The repo whose tenants `AshSqlite.MultiTenancy` manages, kept apart from + `AshSqlite.TenantRepo` so the two binders cannot interfere. + """ + use AshSqlite.Repo, otp_app: :ash_sqlite +end diff --git a/test/support/resources/managed_post.ex b/test/support/resources/managed_post.ex new file mode 100644 index 0000000..0bd5450 --- /dev/null +++ b/test/support/resources/managed_post.ex @@ -0,0 +1,31 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.ManagedPost do + @moduledoc """ + A `strategy :context` resource with no `tenant_binder`, so it gets the default. + """ + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + default_accept(:*) + defaults([:create, :read, :update, :destroy]) + end + + multitenancy do + strategy(:context) + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + sqlite do + table("managed_posts") + repo(AshSqlite.ManagedTenantRepo) + write_transactions?(true) + migrate?(false) + end +end From 1cac62b8c993ef11e79a005ea7bffa10fcd5472d Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Wed, 26 Aug 2026 02:10:03 +0100 Subject: [PATCH 7/8] fix: hold tenants closed across file operations, and refuse busy closes Four defects in the tenant runtime, each found by probing rather than reading, and each with the test that fails without the fix. ## A tenant was not held closed across its own rename or delete `Binds.forget/2` deleted the closing mark along with the bind count, so the `held_open_by_caller?` guard in `close/3` was defeated by the line above it. `rename/3` and `delete/2` both document that they hold a tenant closed across the file operation; neither did, from `close/3`'s return until their own `after`. Measured, for rename: a write addressed to `acme` in that window commits into `globex`'s database. For delete it is worse -- an activation there leaves the tenant *resident* across the unlink, serving reads and accepting writes against an inode with no directory entry, all of it discarded when the connection closes. `all_tenants/1` unions residents, so `migrate_all/3` would then migrate a tenant that no longer exists. `forget/2` now drops only the binds and the last-used mark. The closing mark is lifecycle state owned by whoever took it, and `close/3` already clears its own. `delete/2` now takes a mark and holds it across the close and the unlink, as `rename/3` does -- it never took one at all, so the `forget/2` change alone left it broken. ## `close/3` closed a tenant with a statement in flight, silently `await_quiescence/3` counted down and then closed regardless. `rename/3` calls `close/3` without `force`, so a busy tenant had its file moved out from under a live statement, which keeps the old inode and commits into the destination. `close/3` now returns `{:error, :busy}`; `force: true` closes regardless, which is what eviction and `delete/2` want once they know nothing is bound. `rename/3` propagates it. `migrate_all/3` treats it as success and leaves the tenant resident -- `close_after?` frees residency, it is not part of migrating -- and forwards `:grace_ms`. `grace_ms` is a deadline rather than a loop count. `Process.sleep(1)` sleeps at least a millisecond, so it meant between one and several times what it said. ## A bind could be lost to an eviction that had already chosen its candidate `Binds.bound/2` checked the closing mark and then incremented, so a bind could slip between an eviction's choice of candidate and its close. It now increments first and backs out if it lost: the two are separate ETS objects and no single operation covers both, but publishing the increment before the check means a closer can never read zero for a bind that goes on to proceed, which is the direction that loses data. That only holds because eviction cooperates, so `evict_if_needed/1` marks its candidate closing *before* re-reading the count, and backs out if a bind got in. Neither half is sufficient alone. ## A split read/mutate repo silently unbound every read `put_dynamic_repo/1` binds one repo *module*. The default binder binds the mutate repo, while reads resolve `repo(resource, :read)` -- so a `repo` function returning different modules issued its reads unbound, against whatever database that module was configured with. `VerifyRepo` returns early for any function repo, so nothing checked it. `VerifyTenantRepo` refuses the split, but only for a resource using the default binder. A binder of its own is told whether each statement is a `:read` or a `:write`, so routing reads to a replica is something only it can do correctly, and forbidding that outright would be wrong. ## Also `connection_for/2` on a repo with no fleet running raised `unknown registry: MyApp.Repo.TenantRegistry`, naming an implementation detail instead of the omission. It now names what is missing and shows the child spec, while reraising untouched anything from a fleet that is actually running. Tests promoted from the probes that found these: concurrent activation of one cold tenant sharing a connection, statements surviving eviction churn under contention, isolation holding across that churn, a tenant reused after a delete, and the Ash-level create/read/update/destroy and bulk-create paths, which had no coverage through Ash at all. --- lib/data_layer.ex | 3 +- lib/multi_tenancy.ex | 48 +++- lib/multi_tenancy/binds.ex | 26 ++- lib/multi_tenancy/manager.ex | 106 ++++++--- lib/transformers/verify_tenant_repo.ex | 53 +++++ test/multi_tenancy/binds_test.exs | 69 +++++- test/multi_tenancy/managed_test.exs | 46 ++++ test/multi_tenancy/multi_tenancy_test.exs | 264 ++++++++++++++++++++++ test/multi_tenancy/tenant_repo_test.exs | 144 ++++++++++++ 9 files changed, 723 insertions(+), 36 deletions(-) create mode 100644 lib/transformers/verify_tenant_repo.ex create mode 100644 test/multi_tenancy/tenant_repo_test.exs diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 7a640db..7a8df66 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -327,7 +327,8 @@ defmodule AshSqlite.DataLayer do AshSqlite.Transformers.ValidateReferences, AshSqlite.Transformers.VerifyRepo, AshSqlite.Transformers.EnsureTableOrPolymorphic, - AshSqlite.Transformers.CarryTenant + AshSqlite.Transformers.CarryTenant, + AshSqlite.Transformers.VerifyTenantRepo ] def migrate(args) do diff --git a/lib/multi_tenancy.ex b/lib/multi_tenancy.ex index b97320d..20331b1 100644 --- a/lib/multi_tenancy.ex +++ b/lib/multi_tenancy.ex @@ -46,6 +46,19 @@ defmodule AshSqlite.MultiTenancy do @doc "The repo instance serving `tenant`, starting it if it is not resident." @spec connection_for(module(), String.t()) :: {:ok, pid()} | {:error, term()} def connection_for(repo, tenant) do + do_connection_for(repo, tenant) + rescue + # `Registry.lookup/2` raises on a registry that was never started, and the + # message names an internal registry rather than the thing that is missing. + # Rescuing costs nothing on the path that works. + exception in ArgumentError -> + if started?(repo), do: reraise(exception, __STACKTRACE__), else: not_started!(repo) + catch + :exit, {:noproc, _} = reason -> + if started?(repo), do: exit(reason), else: not_started!(repo) + end + + defp do_connection_for(repo, tenant) do case TenantRegistry.lookup(repo, tenant) do {:ok, _connection, repo_pid} when is_pid(repo_pid) -> {:ok, repo_pid} @@ -63,6 +76,34 @@ defmodule AshSqlite.MultiTenancy do end end + defp started?(repo), do: is_pid(Process.whereis(Module.concat(repo, MultiTenancy))) + + # Reached when a resource has `strategy :context` but nothing is managing its + # tenants. Raised rather than left as `unknown registry: MyApp.Repo.TenantRegistry`, + # which names an implementation detail instead of the omission. + defp not_started!(repo) do + raise """ + #{inspect(repo)} has no tenant fleet running, so there is no database to select \ + for this tenant. + + A resource with `strategy :context` gets `AshSqlite.MultiTenancy.Binder` as its \ + tenant binder, which asks `AshSqlite.MultiTenancy` for a connection. Add it to \ + your supervision tree, after the repo: + + children = [ + #{inspect(repo)}, + {AshSqlite.MultiTenancy, + repo: #{inspect(repo)}, + dir: "priv/tenants", + migrations_path: "priv/repo/tenant_migrations"} + ] + + If this repo's tenants are managed elsewhere -- Turso, Litestream, a router of \ + your own -- name that module as the resource's `tenant_binder` instead, and this \ + one is not needed. + """ + end + @doc "Runs `fun` with `tenant`'s database bound to the calling process." @spec with_tenant(module(), String.t(), (-> result)) :: result when result: var def with_tenant(repo, tenant, fun) when is_function(fun, 0) do @@ -96,7 +137,12 @@ defmodule AshSqlite.MultiTenancy do end end - @doc "Closes a tenant's database, leaving the file on disk." + @doc """ + Closes a tenant's database, leaving the file on disk. + + Waits `:grace_ms` (1000 by default) for statements in flight, and reports + `{:error, :busy}` rather than closing under one. `force: true` closes regardless. + """ defdelegate close(repo, tenant, opts \\ []), to: Manager @doc "Closes a tenant's database and deletes it, WAL sidecars included." diff --git a/lib/multi_tenancy/binds.ex b/lib/multi_tenancy/binds.ex index 02b9288..9365067 100644 --- a/lib/multi_tenancy/binds.ex +++ b/lib/multi_tenancy/binds.ex @@ -20,13 +20,24 @@ defmodule AshSqlite.MultiTenancy.Binds do @spec name(module()) :: module() def name(repo), do: Module.concat(repo, TenantBinds) - @doc "Records that the calling process has bound `tenant`." + @doc """ + Records that the calling process has bound `tenant`. + + Increments *before* checking the closing mark, and backs the increment out if the + tenant turns out to be closing. The two are separate ETS objects, so no single + operation covers both -- but this order is still safe, because the increment is + published before the check. A closer can therefore never read a count of zero for + a bind that goes on to proceed, which is the direction that loses data. The other + direction only costs a closer one more pass around its grace period. + """ @spec bound(module(), String.t()) :: :ok | :closing def bound(repo, tenant) do + :ets.update_counter(name(repo), {:binds, tenant}, {2, 1}, {{:binds, tenant}, 0}) + if closing?(repo, tenant) do + :ets.update_counter(name(repo), {:binds, tenant}, {2, -1, 0, 0}) :closing else - :ets.update_counter(name(repo), {:binds, tenant}, {2, 1}, {{:binds, tenant}, 0}) :ok end end @@ -99,11 +110,18 @@ defmodule AshSqlite.MultiTenancy.Binds do @spec closing?(module(), String.t()) :: boolean() def closing?(repo, tenant), do: :ets.member(name(repo), {:closing, tenant}) - @doc "Drops everything recorded about `tenant`." + @doc """ + Drops what is recorded about how `tenant` has been used. + + Deliberately not the closing mark. That is lifecycle state owned by whoever took + it, and `close/3` is called *inside* `rename/3` and `delete/2` while they hold one + across a file operation -- clearing it here would reopen the window they took it + to close. + """ @spec forget(module(), String.t()) :: :ok def forget(repo, tenant) do table = name(repo) - Enum.each([:binds, :used, :closing], &:ets.delete(table, {&1, tenant})) + Enum.each([:binds, :used], &:ets.delete(table, {&1, tenant})) :ok end diff --git a/lib/multi_tenancy/manager.ex b/lib/multi_tenancy/manager.ex index 76ad1e5..d2455ea 100644 --- a/lib/multi_tenancy/manager.ex +++ b/lib/multi_tenancy/manager.ex @@ -62,36 +62,58 @@ defmodule AshSqlite.MultiTenancy.Manager do activate(repo, tenant, attempts_left - 1) end - @doc "Closes a tenant's database, leaving the file on disk." - @spec close(module(), String.t(), keyword()) :: :ok + @doc """ + Closes a tenant's database, leaving the file on disk. + + Waits up to `:grace_ms` for statements in flight to finish, and reports + `{:error, :busy}` if they do not. Closing anyway would pull the connection out + from under them -- and for `rename/3`, move the file too -- so the caller is told + rather than left to find out. `force: true` closes regardless, which is what + eviction and `delete/2` want once they know nothing is bound. + """ + @spec close(module(), String.t(), keyword()) :: :ok | {:error, :busy} def close(repo, tenant, opts \\ []) do # A caller that had already marked the tenant closing wants it to stay that way # afterwards -- `rename/3` holds the mark across the close *and* the file move. held_open_by_caller? = Binds.closing?(repo, tenant) + force? = Keyword.get(opts, :force, false) Binds.begin_closing(repo, tenant) try do - unless Keyword.get(opts, :force, false) do - await_quiescence(repo, tenant, Keyword.get(opts, :grace_ms, 1_000)) - end + if force? or quiesced?(repo, tenant, Keyword.get(opts, :grace_ms, 1_000)) do + case TenantRegistry.lookup(repo, tenant) do + {:ok, connection, _repo_pid} -> ConnectionSupervisor.stop_connection(repo, connection) + :error -> :ok + end - case TenantRegistry.lookup(repo, tenant) do - {:ok, connection, _repo_pid} -> ConnectionSupervisor.stop_connection(repo, connection) - :error -> :ok + Binds.forget(repo, tenant) + :ok + else + {:error, :busy} end - - Binds.forget(repo, tenant) - :ok after unless held_open_by_caller?, do: Binds.end_closing(repo, tenant) end end - @doc "Closes a tenant and deletes its database, including the WAL sidecars." + @doc """ + Closes a tenant and deletes its database, including the WAL sidecars. + + The tenant is held closed across the close *and* the unlink, as in `rename/3`. A + request arriving in between would otherwise reopen the database and go on serving + it from the unlinked inode, accepting writes that are discarded when the + connection finally closes. + """ @spec delete(module(), String.t()) :: {:ok, [Path.t()]} def delete(repo, tenant) do - close(repo, tenant, force: true) - GenServer.call(name(repo), {:delete, tenant}) + Binds.begin_closing(repo, tenant) + + try do + close(repo, tenant, force: true) + GenServer.call(name(repo), {:delete, tenant}) + after + Binds.end_closing(repo, tenant) + end end @doc """ @@ -105,7 +127,7 @@ defmodule AshSqlite.MultiTenancy.Manager do Refuses rather than overwrites when the destination already has a database. """ @spec rename(module(), String.t(), String.t()) :: - :ok | {:error, :no_database | :target_exists | File.posix()} + :ok | {:error, :busy | :no_database | :target_exists | File.posix()} def rename(_repo, tenant, tenant), do: :ok def rename(repo, from, to) do @@ -113,8 +135,11 @@ defmodule AshSqlite.MultiTenancy.Manager do Binds.begin_closing(repo, to) try do - close(repo, from) - GenServer.call(name(repo), {:rename, from, to}) + # Not moved while something is still reading or writing it: the statement in + # flight would keep the old inode and commit into the destination's database. + with :ok <- close(repo, from) do + GenServer.call(name(repo), {:rename, from, to}) + end after Binds.end_closing(repo, from) Binds.end_closing(repo, to) @@ -182,7 +207,12 @@ defmodule AshSqlite.MultiTenancy.Manager do with {:ok, _repo_pid} <- activate(repo, tenant), {:ok, connection, _} <- TenantRegistry.lookup(repo, tenant) do version = AshSqlite.MultiTenancy.Connection.info(connection).schema_version - if close_after?, do: close(repo, tenant) + + # `close_after?` frees residency, it is not part of migrating. A tenant + # serving traffic stays open rather than turning a migrated tenant into a + # reported failure. + if close_after?, do: close(repo, tenant, Keyword.take(opts, [:grace_ms])) + {:ok, version} else :error -> {:error, :connection_died} @@ -331,14 +361,32 @@ defmodule AshSqlite.MultiTenancy.Manager do """) tenant -> - # No wait: the candidate was chosen because nothing is bound to it. - close(state.repo, tenant, force: true) + evict(state.repo, tenant) end end state end + # The candidate was chosen because nothing was bound to it, which was true when it + # was chosen. Marking it closing *before* re-reading the count is what makes it + # still true: `Binds.bound/2` publishes its increment before reading the mark, so + # after the mark is set a count of zero means no bind can still arrive. A bind that + # got in first is left alone and the limit is exceeded instead. + defp evict(repo, tenant) do + Binds.begin_closing(repo, tenant) + + try do + if Binds.count(repo, tenant) == 0 do + close(repo, tenant, force: true) + else + :ok + end + after + Binds.end_closing(repo, tenant) + end + end + # The sidecars only exist between a write and a checkpoint, but when they do they # hold committed data the database file does not -- so a move that left them # behind would lose the most recent writes. @@ -354,14 +402,18 @@ defmodule AshSqlite.MultiTenancy.Manager do end end - defp await_quiescence(_repo, _tenant, grace_ms) when grace_ms <= 0, do: :ok + # A deadline rather than a loop count: `Process.sleep(1)` sleeps *at least* a + # millisecond, so counting iterations made `grace_ms` mean something between one + # and several times what it said, depending on how loaded the scheduler was. + defp quiesced?(repo, tenant, grace_ms) do + await_quiescence(repo, tenant, System.monotonic_time(:millisecond) + grace_ms) + end - defp await_quiescence(repo, tenant, grace_ms) do - if Binds.count(repo, tenant) == 0 do - :ok - else - Process.sleep(1) - await_quiescence(repo, tenant, grace_ms - 1) + defp await_quiescence(repo, tenant, deadline) do + cond do + Binds.count(repo, tenant) == 0 -> true + System.monotonic_time(:millisecond) >= deadline -> false + true -> Process.sleep(1) && await_quiescence(repo, tenant, deadline) end end end diff --git a/lib/transformers/verify_tenant_repo.ex b/lib/transformers/verify_tenant_repo.ex new file mode 100644 index 0000000..537b804 --- /dev/null +++ b/lib/transformers/verify_tenant_repo.ex @@ -0,0 +1,53 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Transformers.VerifyTenantRepo do + @moduledoc false + use Spark.Dsl.Transformer + + alias Spark.Dsl.Transformer + + # `after_compile?`, as `VerifyRepo` is: Spark stores a `repo` function as a + # generated function *on the resource*, so there is nothing to call until the + # module exists. + def after_compile?, do: true + + # `put_dynamic_repo/1` binds per repo *module*, so a binder can only bind one of + # them. `AshSqlite.MultiTenancy.Binder` binds the mutate repo; a resource whose + # read repo is a different module would issue its reads on a module nothing bound, + # against that module's own configured database -- which for a database-per-tenant + # layout is every tenant's rows at once, with nothing raising. + # + # Only checked for the default binder. A binder of its own sees `usage` and can + # bind a read replica separately, which is the case this would otherwise forbid. + def transform(dsl) do + with true <- Ash.Resource.Info.multitenancy_strategy(dsl) == :context, + nil <- Transformer.get_option(dsl, [:sqlite], :tenant_binder), + fun when is_function(fun, 2) <- Transformer.get_option(dsl, [:sqlite], :repo) do + resource = Transformer.get_persisted(dsl, :module) + verify(dsl, resource, fun.(resource, :read), fun.(resource, :mutate)) + else + _ -> {:ok, dsl} + end + end + + defp verify(dsl, _resource, repo, repo), do: {:ok, dsl} + + defp verify(_dsl, resource, read, mutate) do + {:error, + """ + #{inspect(resource)} has `strategy :context` and a `repo` function returning \ + #{inspect(read)} for :read and #{inspect(mutate)} for :mutate. + + A tenant binder selects a connection with `Ecto.Repo.put_dynamic_repo/1`, which \ + binds one repo *module*. The default binder binds the mutate repo, so reads \ + would be issued on #{inspect(read)} unbound -- against whatever database that \ + module was configured with, rather than this tenant's. + + Either return one module for both, or name a `tenant_binder` of your own. A \ + binder is told whether each statement is a `:read` or a `:write`, so binding a \ + read replica separately is something only it can do correctly. + """} + end +end diff --git a/test/multi_tenancy/binds_test.exs b/test/multi_tenancy/binds_test.exs index b0498b0..0c3b7f5 100644 --- a/test/multi_tenancy/binds_test.exs +++ b/test/multi_tenancy/binds_test.exs @@ -57,6 +57,59 @@ defmodule AshSqlite.MultiTenancy.BindsTest do end end + describe "bound/2 against a concurrent close" do + # The bind and the closing check are two ETS operations, so the order matters. + # Incrementing first means a closer can never read a count of zero for a binder + # that goes on to proceed; checking first would let one slip between the check + # and the increment and lose its statement to the close. + test "a bind is visible to a closer before it is acted on" do + parent = self() + + task = + Task.async(fn -> + assert Binds.bound(Repo, "acme") == :ok + send(parent, :bound) + receive do: (:release -> Binds.released(Repo, "acme")) + end) + + assert_receive :bound + assert Binds.count(Repo, "acme") == 1 + + send(task.pid, :release) + Task.await(task) + end + + test "a bind that loses the race leaves the count where it found it" do + Binds.begin_closing(Repo, "acme") + + assert Binds.bound(Repo, "acme") == :closing + assert Binds.count(Repo, "acme") == 0 + end + + test "many binds racing a close either all hold or all back out" do + outcomes = + 1..100 + |> Task.async_stream( + fn i -> + if i == 50, do: Binds.begin_closing(Repo, "acme") + + case Binds.bound(Repo, "acme") do + :ok -> Binds.released(Repo, "acme") && :held + :closing -> :backed_out + end + end, + max_concurrency: 20 + ) + |> Enum.map(fn {:ok, outcome} -> outcome end) + + # Whatever the interleaving, every bind is matched by a release or a back-out, + # so nothing is left counted against a tenant nobody is using. + assert Binds.count(Repo, "acme") == 0 + assert :backed_out in outcomes + assert Binds.closing?(Repo, "acme") + end + end + describe "closing" do test "refuses new binds while it is marked" do Binds.begin_closing(Repo, "acme") @@ -145,16 +198,26 @@ defmodule AshSqlite.MultiTenancy.BindsTest do end describe "forget/2" do - test "drops everything recorded about a tenant" do + test "drops the binds and the last-used mark" do Binds.bound(Repo, "acme") Binds.released(Repo, "acme") - Binds.begin_closing(Repo, "acme") Binds.forget(Repo, "acme") assert Binds.count(Repo, "acme") == 0 refute Binds.last_used(Repo, "acme") - refute Binds.closing?(Repo, "acme") + end + + # `rename/3` and `delete/2` hold a tenant closed across a file operation, and + # call `close/3` -- which forgets the tenant -- in the middle of it. Clearing the + # mark here would reopen the window they took it to close. + test "leaves a closing mark for whoever took it" do + Binds.bound(Repo, "acme") + Binds.begin_closing(Repo, "acme") + + Binds.forget(Repo, "acme") + + assert Binds.closing?(Repo, "acme") end test "leaves other tenants alone" do diff --git a/test/multi_tenancy/managed_test.exs b/test/multi_tenancy/managed_test.exs index 755eb7f..6853b32 100644 --- a/test/multi_tenancy/managed_test.exs +++ b/test/multi_tenancy/managed_test.exs @@ -87,6 +87,52 @@ defmodule AshSqlite.MultiTenancy.ManagedTest do end end + describe "the whole action surface, per tenant" do + test "create, read, update and destroy all stay in one tenant's file", %{dir: dir} do + post = create!("acme", "one") + create!("globex", "untouched") + + updated = + post + |> Ash.Changeset.for_update(:update, %{title: "two"}, tenant: "acme") + |> Ash.update!() + + assert updated.title == "two" + assert titles_in_file(dir, "acme") == ["two"] + + Ash.destroy!(updated, tenant: "acme") + + assert titles_in_file(dir, "acme") == [] + assert titles_in_file(dir, "globex") == ["untouched"] + end + + # The bulk paths take a different route to the tenant than the single-record + # ones -- they carry the whole changeset context rather than its data layer half. + test "a bulk create lands in its own tenant's file", %{dir: dir} do + result = + Ash.bulk_create!([%{title: "b1"}, %{title: "b2"}], ManagedPost, :create, + tenant: "acme", + return_records?: true + ) + + assert length(result.records) == 2 + assert titles_in_file(dir, "acme") == ["b1", "b2"] + end + + test "a bulk create mixing tenants is refused rather than split" do + assert_raise ArgumentError, ~r/mixes tenants/, fn -> + AshSqlite.DataLayer.bulk_create( + ManagedPost, + [ + Ash.Changeset.for_create(ManagedPost, :create, %{title: "a"}, tenant: "acme"), + Ash.Changeset.for_create(ManagedPost, :create, %{title: "b"}, tenant: "globex") + ], + %{return_records?: false, action: :create} + ) + end + end + end + describe "the paths a caller could not have wrapped" do test "Ash.count/2 goes through the binder with the right tenant" do create!("acme", "one") diff --git a/test/multi_tenancy/multi_tenancy_test.exs b/test/multi_tenancy/multi_tenancy_test.exs index 4b65aa8..1eb4ae2 100644 --- a/test/multi_tenancy/multi_tenancy_test.exs +++ b/test/multi_tenancy/multi_tenancy_test.exs @@ -11,6 +11,7 @@ defmodule AshSqlite.MultiTenancyTest do @moduletag :capture_log alias AshSqlite.MultiTenancy + alias AshSqlite.MultiTenancy.Binds alias AshSqlite.MultiTenancy.Database @repo AshSqlite.TestRepo @@ -39,6 +40,22 @@ defmodule AshSqlite.MultiTenancyTest do assert {:ok, ^first} = MultiTenancy.connection_for(@repo, "acme") end + # Every caller of a cold tenant races the same activation. One connection per + # database is the whole invariant -- two would be two writers on one file. + test "concurrent first requests for one cold tenant share one connection", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + results = + 1..50 + |> Task.async_stream(fn _ -> MultiTenancy.connection_for(@repo, "acme") end, + max_concurrency: 50 + ) + |> Enum.map(fn {:ok, result} -> result end) + + assert Enum.all?(results, &match?({:ok, _}, &1)), inspect(Enum.uniq(results)) + assert results |> Enum.map(fn {:ok, pid} -> pid end) |> Enum.uniq() |> length() == 1 + end + test "gives each tenant its own connection", %{dir: dir} do start_tenancy(dir) @@ -49,6 +66,39 @@ defmodule AshSqlite.MultiTenancyTest do end end + describe "a fleet that was never started" do + test "connection_for/2 names what is missing, not an internal registry" do + assert_raise RuntimeError, fn -> MultiTenancy.connection_for(@repo, "acme") end + + message = + try do + MultiTenancy.connection_for(@repo, "acme") + rescue + e -> Exception.message(e) + end + + assert message =~ "has no tenant fleet running" + assert message =~ "{AshSqlite.MultiTenancy," + assert message =~ "tenant_binder" + refute message =~ "unknown registry" + end + + test "with_tenant/3 says the same thing" do + assert_raise RuntimeError, ~r/has no tenant fleet running/, fn -> + MultiTenancy.with_tenant(@repo, "acme", fn -> :never end) + end + end + + # A started fleet asked about a tenant it cannot serve must still report that, + # rather than being mistaken for a missing supervision tree. + test "a real error from a started fleet is not rewritten", %{dir: dir} do + start_tenancy(dir) + + assert :ok = MultiTenancy.seal(@repo) + assert {:error, :draining} = MultiTenancy.connection_for(@repo, "acme") + end + end + describe "isolation" do test "a row written for one tenant is not visible to another", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -152,6 +202,90 @@ defmodule AshSqlite.MultiTenancyTest do release(holder) end + # Eviction picks a tenant nothing is bound to and then closes it, and the two + # steps are not one atomic act. A request that arrives in between must not lose + # its statement, so the whole cycle is driven under contention rather than in + # the sequential order the tests above use. + test "statements survive eviction churn under contention", %{dir: dir} do + start_tenancy(dir, max_resident: 1, migrations_path: migrations(dir)) + + outcomes = + 1..40 + |> Task.async_stream( + fn i -> + tenant = "t#{rem(i, 3)}" + + try do + write(tenant, "row #{i}") + read(tenant) + :ok + rescue + exception -> {:error, Exception.message(exception)} + catch + kind, reason -> {kind, reason} + end + end, + max_concurrency: 8, + timeout: 30_000 + ) + |> Enum.map(fn {:ok, outcome} -> outcome end) + + assert Enum.reject(outcomes, &(&1 == :ok)) == [] + end + + test "eviction churn does not move rows between tenants", %{dir: dir} do + start_tenancy(dir, max_resident: 1, migrations_path: migrations(dir)) + + for i <- 1..10 do + write("acme", "acme #{i}") + write("globex", "globex #{i}") + end + + acme = List.flatten(read("acme")) + globex = List.flatten(read("globex")) + + assert length(acme) == 10 + assert length(globex) == 10 + assert Enum.all?(acme, &String.starts_with?(&1, "acme")) + assert Enum.all?(globex, &String.starts_with?(&1, "globex")) + end + + test "close/3 refuses a tenant with a statement in flight", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert {:error, :busy} = MultiTenancy.close(@repo, "acme", grace_ms: 20) + assert MultiTenancy.resident(@repo) == ["acme"] + + release(holder) + end + + test "close/3 with force: true closes it regardless", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert :ok = MultiTenancy.close(@repo, "acme", force: true) + assert MultiTenancy.resident(@repo) == [] + + release(holder) + end + + test "close/3 waits for a statement to finish rather than refusing at once", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + Task.start(fn -> + Process.sleep(20) + release(holder) + end) + + assert :ok = MultiTenancy.close(@repo, "acme", grace_ms: 2_000) + end + test "a closed tenant keeps its data and reopens on demand", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -183,6 +317,103 @@ defmodule AshSqlite.MultiTenancyTest do refute File.exists?(Database.path(dir, "acme")) refute File.exists?(Database.path(dir, "acme") <> "-wal") end + + # A deleted tenant is one that has never been seen, not one that is broken: the + # next request must get a fresh, migrated database rather than a quarantine or + # the rows that were just removed. + test "a tenant requested again after a delete gets a fresh migrated database", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "gone") + + assert {:ok, _removed} = MultiTenancy.delete(@repo, "acme") + assert MultiTenancy.quarantined(@repo) == %{} + + write("acme", "new") + + assert read("acme") == [["new"]] + end + end + + describe "holding a tenant closed" do + # `rename/3` and `delete/2` both mark a tenant closing, close it, and then move + # or unlink its file. The mark is what stops a request arriving in between and + # opening the very file that is about to be moved, so `close/3` must leave a + # mark its caller took. + test "close/3 leaves a closing mark its caller was already holding", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "held") + + Binds.begin_closing(@repo, "acme") + MultiTenancy.close(@repo, "acme") + + assert Binds.closing?(@repo, "acme") + assert Binds.bound(@repo, "acme") == :closing + end + + test "close/3 clears a mark it took itself", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "not held") + + MultiTenancy.close(@repo, "acme") + + refute Binds.closing?(@repo, "acme") + assert Binds.bound(@repo, "acme") == :ok + end + + # The window `rename/3` opens between its own close and its file move: a bind + # arriving here must wait for the move rather than opening the source file. Run + # as `rename/3` runs it, because the bug is in the sequence and not in one call. + test "nothing can bind the source name mid-rename", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "original") + + Binds.begin_closing(@repo, "acme") + Binds.begin_closing(@repo, "acme-renamed") + + MultiTenancy.close(@repo, "acme") + + assert Binds.bound(@repo, "acme") == :closing, + "a write addressed to acme here opens the file rename/3 is moving, " <> + "so it commits into acme-renamed's database instead" + + assert Binds.bound(@repo, "acme-renamed") == :closing + + Binds.end_closing(@repo, "acme") + Binds.end_closing(@repo, "acme-renamed") + end + + # `delete/2`'s window is worse than rename's: a bind here leaves a connection + # open on the inode `delete/2` is about to unlink, and it goes on serving reads + # and accepting writes against a database with no directory entry. + test "nothing can bind a tenant mid-delete", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "doomed") + + Binds.begin_closing(@repo, "acme") + MultiTenancy.close(@repo, "acme", force: true) + + assert Binds.bound(@repo, "acme") == :closing, + "a request arriving between delete/2's close and its unlink reopens " <> + "the database, which then survives the delete and keeps serving" + + Binds.end_closing(@repo, "acme") + end + + test "delete/2 leaves no connection open on a database it removed", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "doomed") + + assert {:ok, _removed} = MultiTenancy.delete(@repo, "acme") + + # A tenant may legitimately be resident again already -- a request after the + # delete recreates the file. What must never hold is residency with no file. + for tenant <- MultiTenancy.resident(@repo) do + assert File.exists?(Database.path(dir, tenant)), + "#{tenant} is resident but its database is gone, so it is serving " <> + "an unlinked inode whose writes are discarded on close" + end + end end describe "rename/3" do @@ -219,6 +450,22 @@ defmodule AshSqlite.MultiTenancyTest do assert MultiTenancy.all_tenants(@repo) == ["acme:us"] end + # A statement in flight holds the old inode, so a move under it would commit + # into the destination's database. Refused, as the close it rests on is. + test "refuses a tenant with a statement in flight, leaving the file alone", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert {:error, :busy} = MultiTenancy.rename(@repo, "acme", "acme-renamed") + + assert File.exists?(Database.path(dir, "acme")) + refute File.exists?(Database.path(dir, "acme-renamed")) + + release(holder) + end + test "refuses to overwrite a tenant that already has a database", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) write("acme", "keep me") @@ -378,6 +625,23 @@ defmodule AshSqlite.MultiTenancyTest do @repo |> MultiTenancy.migrate_all() |> Enum.sort() end + # `close_after?` frees residency; it is not part of migrating. A tenant serving + # traffic must not be reported as a migration failure, and must not be closed + # out from under the traffic either. + test "a tenant in use is migrated, left resident, and still reported ok", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert [{"acme", {:ok, 20_260_101_000_000}}] = + MultiTenancy.migrate_all(@repo, ["acme"], grace_ms: 20) + + assert MultiTenancy.resident(@repo) == ["acme"] + + release(holder) + end + test "reports the tenants that failed rather than stopping", %{dir: dir} do start_tenancy(dir, key_for: fn tenant -> if tenant == "acme", do: nil, else: "unused" end) diff --git a/test/multi_tenancy/tenant_repo_test.exs b/test/multi_tenancy/tenant_repo_test.exs new file mode 100644 index 0000000..bb0cdc9 --- /dev/null +++ b/test/multi_tenancy/tenant_repo_test.exs @@ -0,0 +1,144 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.TenantRepoTest do + @moduledoc """ + A `repo` function returning different modules for `:read` and `:mutate` cannot be + served by a binder, because `put_dynamic_repo/1` binds one module at a time. + """ + use ExUnit.Case, async: false + + import ExUnit.CaptureIO + + defp resource(name, body) do + quote do + defmodule unquote(name) do + use Ash.Resource, + domain: nil, + validate_domain_inclusion?: false, + data_layer: AshSqlite.DataLayer + + actions do + defaults([:read]) + end + + attributes do + uuid_primary_key(:id) + end + + unquote(body) + end + end + end + + # Raised from the resource's own verification, which is what fails `mix compile`. + test "a split read/mutate repo is refused for a context-multitenant resource" do + message = + capture_io(:stderr, fn -> + try do + Code.eval_quoted( + resource( + SplitRepoPost, + quote do + multitenancy(do: strategy(:context)) + + sqlite do + table("split_repo_posts") + migrate?(false) + + repo(fn _resource, type -> + case type do + :read -> AshSqlite.DevTestRepo + :mutate -> AshSqlite.TestRepo + end + end) + end + end + ) + ) + rescue + _ -> :raised + end + end) + + assert message =~ "AshSqlite.DevTestRepo for :read" + assert message =~ "AshSqlite.TestRepo for :mutate" + assert message =~ "put_dynamic_repo" + assert message =~ "tenant_binder" + end + + test "a repo function returning one module for both is accepted" do + Code.eval_quoted( + resource( + SameRepoPost, + quote do + multitenancy(do: strategy(:context)) + + sqlite do + table("same_repo_posts") + migrate?(false) + repo(fn _resource, _type -> AshSqlite.TestRepo end) + end + end + ) + ) + + assert AshSqlite.DataLayer.Info.repo(SameRepoPost, :read) == AshSqlite.TestRepo + assert AshSqlite.DataLayer.Info.repo(SameRepoPost, :mutate) == AshSqlite.TestRepo + end + + # The case the check would otherwise forbid: a binder of its own is told whether + # each statement is a read or a write, so it can bind a replica for reads. + test "a split is allowed when the resource names its own binder" do + Code.eval_quoted( + resource( + SplitWithBinderPost, + quote do + multitenancy(do: strategy(:context)) + + sqlite do + table("split_with_binder_posts") + migrate?(false) + tenant_binder(AshSqlite.Test.TenantBinder) + + repo(fn _resource, type -> + case type do + :read -> AshSqlite.DevTestRepo + :mutate -> AshSqlite.TestRepo + end + end) + end + end + ) + ) + + assert AshSqlite.DataLayer.Info.tenant_binder(SplitWithBinderPost) == + AshSqlite.Test.TenantBinder + end + + # Nothing binds a resource with no context multitenancy, so a split is its own + # business there and has been supported for as long as the `repo` option has. + test "a split is allowed on a resource with no context multitenancy" do + Code.eval_quoted( + resource( + SplitGlobalPost, + quote do + sqlite do + table("split_global_posts") + migrate?(false) + + repo(fn _resource, type -> + case type do + :read -> AshSqlite.DevTestRepo + :mutate -> AshSqlite.TestRepo + end + end) + end + end + ) + ) + + assert AshSqlite.DataLayer.Info.repo(SplitGlobalPost, :read) == AshSqlite.DevTestRepo + end +end From 2a658e24fcaf29434751be890af915b4913d4907 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Wed, 26 Aug 2026 23:35:13 +0100 Subject: [PATCH 8/8] improvement: serve `global? true` from one shared database rather than the caller's binding `global?` means one copy of the rows. For a schema-based data layer that falls out for free -- the global table sits in a schema the connection a tenanted statement already holds can reach. One SQLite database per tenant has no such connection, so it has to be chosen, and nothing chose it. Two halves were wrong, and both are fixed here: * A tenantless statement ran on whatever the calling process had bound. A `global?` resource sharing a repo module with tenanted ones therefore read whichever tenant was bound last, and which one depended on what that process had done before. * A statement *with* a tenant was bound to it, so a write with `tenant: "acme"` landed in acme's file and every tenant accumulated its own copy of a table that is supposed to have exactly one. A `global?` resource now binds its repo module's own named instance, explicitly, and ignores the tenant. Which database holds the global rows follows from `repo`: sharing the tenanted module puts them in that module's configured database, naming another module puts them there. Neither depends on the caller. ## Why not a tenant binder callback The obvious shape was an optional `bind_global/2` on `AshSqlite.TenantBinder`, letting a binder pick the connection for a tenantless statement. It is the wrong seam. A binder picks a connection *instance* within one repo module; shared rows are not another instance of a tenant's database but another database, which Ash and Ecto already address as a repo module and resolve through `AshSqlite.DataLayer.Info.repo/2`. Routing by binding would have put connection selection in two places with no rule for which wins, and would still have needed the answer `repo` already gives. The binder contract is unchanged and no binder needs updating. ## Why the shared repo is checked at runtime A `global?` resource needs a repo module started under its own name *and* holding a `database:`. A repo module serving only tenants needs neither -- it is reached through `Ecto.Repo.put_dynamic_repo/1` -- so adding `global? true` to a resource on one is an easy mistake, and both halves are checked because neither implies the other: * Unstarted, the statement fails with Ecto's own "could not lookup Ecto repo", which names the repo but not the reason a `global?` resource wanted it. * Started with no `database:` -- which Ecto allows -- the statement waits out the pool timeout and then reports that requests are arriving faster than they can be served. Measured at roughly six seconds, and nothing in it points at the missing configuration. Neither check can be a transformer. A repo's `database:` is very often set in `config/runtime.exs`, which is the recommended shape for a release, and a transformer runs long before that file is evaluated -- so it would reject exactly the configuration it should accept. They run instead when a global statement first needs the connection, where the answer is knowable. Nothing changes for a repo module with no `global?` resource on it. `AshSqlite.ManagedTenantRepo` is configured with nothing whatsoever -- no `database:`, no pool, no name -- and stays that way, with a test that says so. ## Tests `AshSqlite.TenantRepo` gains a database and is started under its own name -- which is what a shared database is here -- while deliberately continuing to serve the tenanted resources, so the tests are pointed at the footgun this fixes: a global resource sharing a repo module with tenanted ones, still reading one copy. `AshSqlite.Test.UnstartedGlobalPost` covers both ways to have no shared database, on a repo module that has neither a name nor a database of its own. The three tests that pinned the old behaviour are replaced rather than adjusted. They asserted that a tenantless read followed the process binding and that a tenanted write landed in the tenant's file; both are now wrong on purpose. --- .gitignore | 4 + config/config.exs | 5 + lib/data_layer.ex | 93 ++++++++++++- test/multi_tenancy/managed_test.exs | 17 +++ test/multitenancy_test.exs | 129 ++++++++++++++---- test/support/domain.ex | 1 + .../resources/unstarted_global_post.ex | 35 +++++ test/test_helper.exs | 10 ++ 8 files changed, 269 insertions(+), 25 deletions(-) create mode 100644 test/support/resources/unstarted_global_post.ex diff --git a/.gitignore b/.gitignore index 65c6920..09c09d1 100644 --- a/.gitignore +++ b/.gitignore @@ -38,4 +38,8 @@ test/test.db-wal test/dev_test.db test/dev_test.db-shm test/dev_test.db-wal + +test/tenant_shared.db +test/tenant_shared.db-shm +test/tenant_shared.db-wal notes/ diff --git a/config/config.exs b/config/config.exs index b927342..c90a0d5 100644 --- a/config/config.exs +++ b/config/config.exs @@ -40,7 +40,12 @@ if Mix.env() == :test do pool: Ecto.Adapters.SQL.Sandbox, migration_primary_key: [name: :id, type: :binary_id] + # A real database, and started under its own name in `test_helper.exs`. This is the + # database a `global? true` resource on this module uses: one copy of its rows, + # reached without a tenant binding. config :ash_sqlite, AshSqlite.TenantRepo, + database: Path.join(__DIR__, "../test/tenant_shared.db"), + pool: DBConnection.ConnectionPool, pool_size: 1, migration_lock: false, migration_primary_key: [name: :id, type: :binary_id] diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 7a8df66..b60c1f6 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -2290,9 +2290,98 @@ defmodule AshSqlite.DataLayer do # this module can say -- by the time a binder sees a statement the distinction is # gone -- and a binder that caches, replicates, or routes reads separately from # writes cannot be written without it. - defp bind_tenant(resource, nil, _usage, fun), do: unbound(resource, fun) - defp bind_tenant(resource, tenant, usage, fun) do + cond do + global?(resource) -> bind_global(resource, fun) + is_nil(tenant) -> unbound(resource, fun) + true -> bind_to_tenant(resource, tenant, usage, fun) + end + end + + # A `global?` resource holds one copy of its rows, not one per tenant. No tenant + # selects its database, so the binder is never asked and a tenant passed to it is + # ignored rather than honoured -- honouring it is what gave every tenant its own + # copy of a table that is supposed to have exactly one. + # + # It binds the resource's repo module to its *own* named instance, explicitly. The + # alternative is to leave the process binding alone, which is what made this a + # footgun: a global resource sharing a repo module with tenanted ones read whichever + # tenant happened to be bound last. Which database the global rows live in follows + # from `repo` -- the tenanted module's own configured database if it shares one, + # another module's if it names one -- and in neither case from the caller. + defp bind_global(resource, fun) do + repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + verify_shared_repo!(resource, repo) + previous = repo.get_dynamic_repo() + + if previous == repo do + fun.() + else + repo.put_dynamic_repo(repo) + + try do + fun.() + after + repo.put_dynamic_repo(previous) + end + end + end + + # Checked here rather than by a transformer, because it cannot be known at compile + # time. A repo's `database:` is very often set in `config/runtime.exs` -- that is + # the recommended shape for a release -- and a transformer runs long before that + # file is evaluated, so it would reject exactly the configuration it should accept. + # Ecto's own error for this names the repo but not the reason a `global?` resource + # wanted it, which is the part worth saying. + defp verify_shared_repo!(resource, repo) do + cond do + is_nil(Process.whereis(repo)) -> + raise ArgumentError, shared_repo_error(resource, repo, "is not running") + + is_nil(repo.config()[:database]) -> + raise ArgumentError, shared_repo_error(resource, repo, "has no `database:` set") + + true -> + :ok + end + end + + # Both halves are checked, because neither implies the other and the failure + # without them is unrecognisable. A repo module serving only tenants is reached + # through `Ecto.Repo.put_dynamic_repo/1`, so it needs no name and no database of + # its own -- and it starts happily without either. A global statement on one then + # waits out the pool timeout and reports that requests are arriving faster than + # they can be served, which is not what went wrong. + defp shared_repo_error(resource, repo, problem) do + """ + #{inspect(resource)} has `strategy :context` with `global? true`, so its rows \ + live in one shared database rather than one per tenant -- #{inspect(repo)}'s own, \ + under its own name. That repo #{problem}. + + A repo module used only for tenants needs neither, which is what makes this easy \ + to arrive at by adding `global? true` to a resource on one. Give it a database \ + and start it: + + config :my_app, #{inspect(repo)}, database: "priv/shared.db" + + children = [ + #{inspect(repo)}, + {AshSqlite.MultiTenancy, repo: #{inspect(repo)}, dir: "priv/tenants"} + ] + + Or put the shared tables on a repo module of their own and name it in this \ + resource's `repo`. What will not work is neither: there is no tenant to fall \ + back to, and falling back to one would put a shared table in a single tenant's \ + database. + """ + end + + defp global?(resource) do + Ash.Resource.Info.multitenancy_strategy(resource) == :context && + Ash.Resource.Info.multitenancy_global?(resource) + end + + defp bind_to_tenant(resource, tenant, usage, fun) do case AshSqlite.DataLayer.Info.tenant_binder(resource) do nil -> unbound(resource, fun) diff --git a/test/multi_tenancy/managed_test.exs b/test/multi_tenancy/managed_test.exs index 6853b32..edaf2b7 100644 --- a/test/multi_tenancy/managed_test.exs +++ b/test/multi_tenancy/managed_test.exs @@ -48,6 +48,23 @@ defmodule AshSqlite.MultiTenancy.ManagedTest do end end + describe "a repo module that serves only tenants" do + # The shape a `global?` resource needs a database for, and the shape that needs + # none. `AshSqlite.ManagedTenantRepo` is configured with nothing at all -- no + # `database:`, no pool, no name -- because every connection it serves is a tenant + # reached through `Ecto.Repo.put_dynamic_repo/1`. Nothing here requires more, and + # the check that a shared database exists is reached only by `global? true`. + test "needs no database of its own, and no name", %{dir: dir} do + assert is_nil(Application.get_env(:ash_sqlite, ManagedTenantRepo)) + assert is_nil(Process.whereis(ManagedTenantRepo)) + + create!("acme", "no shared database needed") + + assert titles_in_file(dir, "acme") == ["no shared database needed"] + assert titles("acme") == ["no shared database needed"] + end + end + describe "reads and writes" do test "a create lands in its own tenant's file", %{dir: dir} do create!("acme", "acme one") diff --git a/test/multitenancy_test.exs b/test/multitenancy_test.exs index 0417145..a53dedf 100644 --- a/test/multitenancy_test.exs +++ b/test/multitenancy_test.exs @@ -45,6 +45,10 @@ defmodule AshSqlite.MultitenancyTest do TenantBinder.reset_calls() + # The tenant files are made per test, but the shared database is not: it is the + # repo module's own, and it outlives every test that writes to it. + Ecto.Adapters.SQL.query!(AshSqlite.TenantRepo, "DELETE FROM global_posts", []) + %{repos: repos} end @@ -181,39 +185,119 @@ defmodule AshSqlite.MultitenancyTest do end describe "a global? resource" do - test "is bound to the tenant it is given, like any other", %{repos: repos} do + # One copy of the rows, as `global?` means for a schema-based data layer. The + # tenant is ignored rather than honoured: honouring it gave every tenant its own + # copy of a table that is supposed to have exactly one. + test "a write goes to the shared database, not the tenant's file", %{repos: repos} do GlobalPost - |> Ash.Changeset.for_create(:create, %{title: "for acme"}, tenant: "acme") + |> Ash.Changeset.for_create(:create, %{title: "shared"}, tenant: "acme") |> Ash.create!() - assert global_titles_in_file(repos["acme"].path) == ["for acme"] + assert shared_global_titles() == ["shared"] + assert global_titles_in_file(repos["acme"].path) == [] assert global_titles_in_file(repos["globex"].path) == [] end + test "every tenant sees the same rows" do + GlobalPost + |> Ash.Changeset.for_create(:create, %{title: "one copy"}, tenant: "acme") + |> Ash.create!() + + assert global_titles("acme") == ["one copy"] + assert global_titles("globex") == ["one copy"] + assert Ash.read!(GlobalPost) |> Enum.map(& &1.title) == ["one copy"] + end + + # The footgun this replaces: the resource shares a repo module with tenanted + # ones, so leaving the process binding alone made it read whichever tenant was + # bound last. It now binds the module's own instance explicitly. + test "is unaffected by a tenant bound on the same repo module" do + GlobalPost + |> Ash.Changeset.for_create(:create, %{title: "shared"}, tenant: "acme") + |> Ash.create!() + + assert bound("acme", fn -> Ash.read!(GlobalPost) |> Enum.map(& &1.title) end) == + ["shared"] + + assert bound("globex", fn -> Ash.read!(GlobalPost) |> Enum.map(& &1.title) end) == + ["shared"] + end + test "is read without a tenant, where a tenanted resource is refused" do assert_raise Ash.Error.Invalid, ~r/require a tenant to be specified/, fn -> Ash.read!(TenantedPost) end - bound("acme", fn -> assert Ash.read!(GlobalPost) == [] end) + assert Ash.read!(GlobalPost) == [] end - test "reads whatever the process is bound to when given no tenant", %{repos: repos} do - # Ecto binds per repo *module*, so this is a sharp edge rather than a feature: - # a global resource sharing a repo with tenanted ones sees the last tenant bound. - insert_global(repos["acme"].pid, "acme's own") - insert_global(repos["globex"].pid, "globex's own") + test "is never asked of the binder, with or without a tenant" do + TenantBinder.reset_calls() - assert bound("acme", fn -> global_titles() end) == ["acme's own"] - assert bound("globex", fn -> global_titles() end) == ["globex's own"] - end + GlobalPost + |> Ash.Changeset.for_create(:create, %{title: "shared"}, tenant: "acme") + |> Ash.create!() - test "is never asked of the binder when given no tenant" do - TenantBinder.reset_calls() - bound("acme", fn -> Ash.read!(GlobalPost) end) + Ash.read!(GlobalPost) assert TenantBinder.calls() == [] end + + # The shape this is easy to arrive at: adding `global? true` to a resource on a + # repo module that only ever served tenants. Such a module is reached entirely + # through `put_dynamic_repo/1`, so it has no named process and no shared database + # to hold the one copy. + test "says so when the shared repo has no instance of its own" do + message = + try do + Ash.read!(AshSqlite.Test.UnstartedGlobalPost) + rescue + error -> Exception.message(error) + end + + assert message =~ "one shared database rather than one per tenant" + assert message =~ "AshSqlite.ManagedTenantRepo" + assert message =~ ~s(database: "priv/shared.db") + refute message =~ "could not lookup Ecto repo" + end + + # Started under its own name but with no database: the other way to have no + # shared database, and the one whose native failure is unrecognisable -- the + # statement waits out the pool timeout and then reports that requests are + # arriving faster than they can be served. + test "says so when the shared repo is named but has no database" do + {:ok, pid} = AshSqlite.ManagedTenantRepo.start_link() + + # A repo with no database cannot keep a connection up, so it may already be on + # its way down by the time this runs. + on_exit(fn -> + try do + if Process.alive?(pid), do: Supervisor.stop(pid) + catch + :exit, _ -> :ok + end + end) + + assert is_nil(AshSqlite.ManagedTenantRepo.config()[:database]) + + message = + try do + Ash.read!(AshSqlite.Test.UnstartedGlobalPost) + rescue + error -> Exception.message(error) + end + + assert message =~ "has no `database:` set" + refute message =~ "connection not available" + end + + test "restores the caller's binding afterwards" do + bound("acme", fn -> + before = AshSqlite.TenantRepo.get_dynamic_repo() + Ash.read!(GlobalPost) + assert AshSqlite.TenantRepo.get_dynamic_repo() == before + end) + end end defp bound(tenant, fun) do @@ -226,16 +310,15 @@ defmodule AshSqlite.MultitenancyTest do end end - defp insert_global(pid, title) do - Ecto.Adapters.SQL.query!( - pid, - "INSERT INTO global_posts (id, title) VALUES (?, ?)", - [Ash.UUID.generate(), title] - ) + defp shared_global_titles do + AshSqlite.TenantRepo + |> Ecto.Adapters.SQL.query!("SELECT title FROM global_posts ORDER BY title", []) + |> Map.fetch!(:rows) + |> List.flatten() end - defp global_titles do - GlobalPost |> Ash.read!() |> Enum.map(& &1.title) |> Enum.sort() + defp global_titles(tenant) do + GlobalPost |> Ash.read!(tenant: tenant) |> Enum.map(& &1.title) |> Enum.sort() end defp global_titles_in_file(path) do diff --git a/test/support/domain.ex b/test/support/domain.ex index 5424209..c8b67f6 100644 --- a/test/support/domain.ex +++ b/test/support/domain.ex @@ -20,6 +20,7 @@ defmodule AshSqlite.Test.Domain do resource(AshSqlite.Test.TransactionalAccount) resource(AshSqlite.Test.TenantedPost) resource(AshSqlite.Test.GlobalPost) + resource(AshSqlite.Test.UnstartedGlobalPost) resource(AshSqlite.Test.ManagedPost) resource(AshSqlite.Test.Organization) resource(AshSqlite.Test.Manager) diff --git a/test/support/resources/unstarted_global_post.ex b/test/support/resources/unstarted_global_post.ex new file mode 100644 index 0000000..172425d --- /dev/null +++ b/test/support/resources/unstarted_global_post.ex @@ -0,0 +1,35 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.UnstartedGlobalPost do + @moduledoc """ + A `global? true` resource on a repo module that has no instance of its own. + + The shape that is easy to arrive at by accident: `AshSqlite.ManagedTenantRepo` + serves its tenants entirely through `Ecto.Repo.put_dynamic_repo/1`, so it is never + started under its own name and has no shared database to hold one copy of anything. + """ + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + default_accept(:*) + defaults([:create, :read]) + end + + multitenancy do + strategy(:context) + global?(true) + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + sqlite do + table("unstarted_global_posts") + repo(AshSqlite.ManagedTenantRepo) + migrate?(false) + end +end diff --git a/test/test_helper.exs b/test/test_helper.exs index 8941098..824de16 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -8,5 +8,15 @@ ExUnit.configure(stacktrace_depth: 100) AshSqlite.TestRepo.start_link() AshSqlite.DevTestRepo.start_link() +# Named, so that a `global? true` resource on this module has an instance of its own +# to bind -- the shared database, as opposed to any tenant's. +AshSqlite.TenantRepo.start_link() + +Ecto.Adapters.SQL.query!( + AshSqlite.TenantRepo, + "CREATE TABLE IF NOT EXISTS global_posts (id TEXT PRIMARY KEY, title TEXT)", + [] +) + Ecto.Adapters.SQL.Sandbox.mode(AshSqlite.TestRepo, :manual) Ecto.Adapters.SQL.Sandbox.mode(AshSqlite.DevTestRepo, :manual)