From e6ba5c0513b6a9c394201519aa68ce1c883b49fd Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Tue, 13 Oct 2026 17:31:02 +0200 Subject: [PATCH 1/2] fix: refuse a statement for a resource without tenant_repo while its repo is bound to a tenant `bind_tenant_repo/5` binds a tenant instance with `put_dynamic_repo/1`, and Ecto keeps that binding per repo module per process. A resource with no `tenant_repo` on the same repo module, run inside a tenant transaction, ran on the tenant database. `unbound/3` and the `nil` branch of `with_tenant_repo/4` now raise `ArgumentError` while the data layer holds that binding. --- lib/data_layer.ex | 63 ++++++++++--- test/multitenancy_test.exs | 129 +++++++++++++++++++++++++- test/support/domain.ex | 1 + test/support/resources/shared_note.ex | 26 ++++++ 4 files changed, 207 insertions(+), 12 deletions(-) create mode 100644 test/support/resources/shared_note.ex diff --git a/lib/data_layer.ex b/lib/data_layer.ex index f065a7a..49c7013 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -2285,7 +2285,7 @@ defmodule AshSqlite.DataLayer do # Every callback that issues SQL goes through here, because a caller cannot bind # around a path it never sees -- aggregates and atomic writes give it nothing to # wrap. `type` is passed on so a tenant repo can route reads and writes differently. - defp with_tenant_repo(resource, nil, _type, fun), do: unbound(resource, fun) + defp with_tenant_repo(resource, nil, type, fun), do: unbound(resource, type, fun) defp with_tenant_repo(resource, tenant, type, fun) do case AshSqlite.DataLayer.Info.tenant_repo(resource) do @@ -2293,6 +2293,7 @@ defmodule AshSqlite.DataLayer do # resolve to a connection, such as `strategy :attribute`. `VerifyTenantRepo` # covers `strategy :context` at compile time. nil -> + refuse_tenant_binding!(resource, type) fun.() {module, opts} -> @@ -2322,24 +2323,63 @@ defmodule AshSqlite.DataLayer do end defp bind_tenant_repo(module, tenant, context, opts, fun) do - if Code.ensure_loaded?(module) and function_exported?(module, :with_repo, 4) do - module.with_repo(tenant, context, opts, fun) - else - repo = AshSqlite.DataLayer.Info.repo(context.resource, context.type) - previous = repo.put_dynamic_repo(module.repo(tenant, context, opts)) + repo = AshSqlite.DataLayer.Info.repo(context.resource, context.type) + previous_binding = Process.put({__MODULE__, :tenant_binding, repo}, tenant) - try do - fun.() - after - repo.put_dynamic_repo(previous) + try do + if Code.ensure_loaded?(module) and function_exported?(module, :with_repo, 4) do + module.with_repo(tenant, context, opts, fun) + else + previous = repo.put_dynamic_repo(module.repo(tenant, context, opts)) + + try do + fun.() + after + repo.put_dynamic_repo(previous) + end end + after + restore_tenant_binding(repo, previous_binding) + end + end + + defp restore_tenant_binding(repo, nil), do: Process.delete({__MODULE__, :tenant_binding, repo}) + + defp restore_tenant_binding(repo, tenant), + do: Process.put({__MODULE__, :tenant_binding, repo}, tenant) + + # Ecto keeps one dynamic repo per repo module, not per resource, so a resource with + # no tenant repo that shares the module would run on the bound tenant's database. + # Falling back to the default instance would commit outside the tenant's + # transaction, so this refuses instead. Only this data layer's own binding is + # recorded, which leaves a caller's `put_dynamic_repo/1` alone. + defp refuse_tenant_binding!(resource, type) do + repo = AshSqlite.DataLayer.Info.repo(resource, type) + + case Process.get({__MODULE__, :tenant_binding, repo}) do + nil -> + :ok + + tenant -> + raise ArgumentError, """ + #{inspect(resource)} has no tenant_repo, but its repo #{inspect(repo)} is \ + bound to tenant #{inspect(tenant)}'s database by another resource's \ + tenant_repo. Ecto binds a repo module per process, not per resource, so \ + this statement would run on that tenant's database. + + A resource shared across tenants needs its own repo module: + + sqlite do + repo MyApp.SharedRepo + end + """ end end # Without a tenant there is nothing to select a database, so this fails rather # than run against whichever connection the process happens to hold. Ash already # enforces this for actions; this catches the paths that bypass one. - defp unbound(resource, fun) do + defp unbound(resource, type, fun) do if tenant_required?(resource) do raise ArgumentError, """ #{inspect(resource)} has `strategy :context` but this statement carried no \ @@ -2347,6 +2387,7 @@ defmodule AshSqlite.DataLayer do """ end + refuse_tenant_binding!(resource, type) fun.() end diff --git a/test/multitenancy_test.exs b/test/multitenancy_test.exs index 4b6a1ab..6aa0d9e 100644 --- a/test/multitenancy_test.exs +++ b/test/multitenancy_test.exs @@ -10,7 +10,14 @@ defmodule AshSqlite.MultitenancyTest do import ExUnit.CaptureIO - alias AshSqlite.Test.{BracketedTenantPost, BracketingTenantRepo, TenantedPost, TenantRepos} + alias AshSqlite.Test.{ + BracketedTenantPost, + BracketingTenantRepo, + Post, + SharedNote, + TenantedPost, + TenantRepos + } require Ash.Query @@ -192,6 +199,126 @@ defmodule AshSqlite.MultitenancyTest do end end + describe "a resource without a tenant repo on the same repo module" do + setup %{repos: repos} do + dir = Path.dirname(repos["acme"].path) + shared_path = Path.join(dir, "shared.db") + + start_supervised!({AshSqlite.TenantTestRepo, database: shared_path, pool_size: 1}) + + for repo <- [AshSqlite.TenantTestRepo, repos["acme"].pid, repos["globex"].pid] do + Ecto.Adapters.SQL.query!( + repo, + "CREATE TABLE shared_notes (id TEXT PRIMARY KEY, body TEXT)", + [] + ) + end + + %{shared_path: shared_path} + end + + defp bodies_in_file(path) do + {:ok, db} = Exqlite.Sqlite3.open(path) + {:ok, stmt} = Exqlite.Sqlite3.prepare(db, "SELECT body FROM shared_notes ORDER BY body") + {:ok, rows} = Exqlite.Sqlite3.fetch_all(db, stmt) + :ok = Exqlite.Sqlite3.close(db) + List.flatten(rows) + end + + defp create_note!(body) do + SharedNote + |> Ash.Changeset.for_create(:create, %{body: body}) + |> Ash.create!() + end + + defp in_acme_transaction(fun) do + AshSqlite.DataLayer.transaction( + TenantedPost, + fun, + nil, + %{type: :custom, metadata: %{}, tenant: "acme"} + ) + end + + test "is refused when written from a tenanted action's hook", %{ + repos: repos, + shared_path: shared_path + } do + assert_raise Ash.Error.Unknown, ~r/AshSqlite.Test.SharedNote.*"acme"/s, fn -> + TenantedPost + |> Ash.Changeset.for_create(:create, %{title: "with a note"}, tenant: "acme") + |> Ash.Changeset.after_action(fn _changeset, post -> + create_note!("from the hook") + {:ok, post} + end) + |> Ash.create!() + end + + assert bodies_in_file(repos["acme"].path) == [] + assert bodies_in_file(shared_path) == [] + assert titles_in_file(repos["acme"].path) == [] + end + + test "is refused when read inside a tenant's transaction", %{shared_path: shared_path} do + create_note!("shared one") + assert bodies_in_file(shared_path) == ["shared one"] + + error = + assert_raise Ash.Error.Unknown, fn -> + in_acme_transaction(fn -> Ash.read!(SharedNote) end) + end + + message = Exception.message(error) + assert message =~ "AshSqlite.Test.SharedNote" + assert message =~ ~s("acme") + assert message =~ "AshSqlite.TenantTestRepo" + assert message =~ "repo MyApp.SharedRepo" + end + + test "lands in the shared file outside any transaction", %{ + repos: repos, + shared_path: shared_path + } do + create_note!("shared one") + + assert bodies_in_file(shared_path) == ["shared one"] + assert bodies_in_file(repos["acme"].path) == [] + assert ["shared one"] = SharedNote |> Ash.read!() |> Enum.map(& &1.body) + end + + test "on a different repo module is unaffected inside a tenant's transaction" do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(AshSqlite.TestRepo) + + in_acme_transaction(fn -> + create!("acme", "tenant post") + + Post + |> Ash.Changeset.for_create(:create, %{title: "untenanted post"}) + |> Ash.create!() + end) + + assert ["untenanted post"] = Post |> Ash.read!() |> Enum.map(& &1.title) + assert titles("acme") == ["tenant post"] + end + + test "follows a binding the caller made with put_dynamic_repo", %{ + repos: repos, + shared_path: shared_path + } do + previous = AshSqlite.TenantTestRepo.put_dynamic_repo(repos["acme"].pid) + + try do + create_note!("explicitly bound") + assert ["explicitly bound"] = SharedNote |> Ash.read!() |> Enum.map(& &1.body) + after + AshSqlite.TenantTestRepo.put_dynamic_repo(previous) + end + + assert bodies_in_file(repos["acme"].path) == ["explicitly bound"] + assert bodies_in_file(shared_path) == [] + end + end + describe "global? true" do test "is refused at compile time, rather than guessed at" do message = diff --git a/test/support/domain.ex b/test/support/domain.ex index fe47b36..4e07b03 100644 --- a/test/support/domain.ex +++ b/test/support/domain.ex @@ -22,6 +22,7 @@ defmodule AshSqlite.Test.Domain do resource(AshSqlite.Test.NamedFnRepoAccount) resource(AshSqlite.Test.TenantedPost) resource(AshSqlite.Test.BracketedTenantPost) + resource(AshSqlite.Test.SharedNote) resource(AshSqlite.Test.Organization) resource(AshSqlite.Test.Manager) resource(AshSqlite.Test.Device) diff --git a/test/support/resources/shared_note.ex b/test/support/resources/shared_note.ex new file mode 100644 index 0000000..f87325a --- /dev/null +++ b/test/support/resources/shared_note.ex @@ -0,0 +1,26 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.SharedNote do + @moduledoc """ + A resource with no multitenancy on the same repo module as `AshSqlite.Test.TenantedPost`. + """ + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + default_accept(:*) + defaults([:create, :read]) + end + + attributes do + uuid_primary_key(:id) + attribute(:body, :string, public?: true) + end + + sqlite do + table("shared_notes") + repo(AshSqlite.TenantTestRepo) + migrate?(false) + end +end From 402a2e5bf3b2ef4766cde87a634da730e3fa4597 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Tue, 13 Oct 2026 18:11:38 +0200 Subject: [PATCH 2/2] fix: name after_transaction/3 in the shared-resource refusal The refusal now says that a resource on its own repo module commits in its own transaction, which a rollback of the tenant transaction does not undo. A new test rolls back a tenant transaction after a `Post` write on `AshSqlite.TestRepo` and finds the `Post` row kept. --- lib/data_layer.ex | 5 +++++ test/multitenancy_test.exs | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/lib/data_layer.ex b/lib/data_layer.ex index 49c7013..01be2e2 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -2372,6 +2372,11 @@ defmodule AshSqlite.DataLayer do sqlite do repo MyApp.SharedRepo end + + Its writes then commit in their own transaction, not the tenant's, so a \ + later rollback of the tenant's transaction does not undo them. To write \ + it only once the tenant's data has committed, write it from an \ + `Ash.Changeset.after_transaction/3` hook on the outermost action. """ end end diff --git a/test/multitenancy_test.exs b/test/multitenancy_test.exs index 6aa0d9e..56e036d 100644 --- a/test/multitenancy_test.exs +++ b/test/multitenancy_test.exs @@ -273,6 +273,7 @@ defmodule AshSqlite.MultitenancyTest do assert message =~ ~s("acme") assert message =~ "AshSqlite.TenantTestRepo" assert message =~ "repo MyApp.SharedRepo" + assert message =~ "Ash.Changeset.after_transaction/3" end test "lands in the shared file outside any transaction", %{ @@ -301,6 +302,26 @@ defmodule AshSqlite.MultitenancyTest do assert titles("acme") == ["tenant post"] end + test "on a different repo module commits on its own, and survives the tenant's rollback", %{ + repos: repos + } do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(AshSqlite.TestRepo) + + assert {:error, :rolled_back} = + in_acme_transaction(fn -> + create!("acme", "tenant post") + + Post + |> Ash.Changeset.for_create(:create, %{title: "untenanted post"}) + |> Ash.create!() + + AshSqlite.DataLayer.rollback(TenantedPost, :rolled_back) + end) + + assert ["untenanted post"] = Post |> Ash.read!() |> Enum.map(& &1.title) + assert titles_in_file(repos["acme"].path) == [] + end + test "follows a binding the caller made with put_dynamic_repo", %{ repos: repos, shared_path: shared_path