From c0f7f6d299100b46c87cc658fb1a9acc18ac71d6 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Fri, 21 Aug 2026 19:32:06 +0100 Subject: [PATCH 1/5] 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/5] 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/5] 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/5] 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/5] 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