fix: refuse a statement for a resource without tenant_repo while its repo is bound to a tenant

lib/data_layer.ex

@@ -2285,14 +2285,15 @@ 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
       # No tenant repo and a tenant means multitenancy this data layer does not
       # 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,31 +2323,76 @@ 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
+
+        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
 
   # 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 \
       tenant, so there is no database file to select. Pass a tenant.
       """
     end
 
+    refuse_tenant_binding!(resource, type)
     fun.()
   end
 

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,147 @@ 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"
+      assert message =~ "Ash.Changeset.after_transaction/3"
+    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 "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
+    } 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 =

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)

test/support/resources/shared_note.ex

@@ -0,0 +1,26 @@
+# SPDX-FileCopyrightText: 2023 ash_sqlite contributors <https://github.com/ash-project/ash_sqlite/graphs/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