Skip to content

improvement: passing tenant in the transaction reason - #2917

Merged
zachdaniel merged 2 commits into
ash-project:mainfrom
C-Sinclair:improvement/tenant-in-transaction-reason
Sep 9, 2026
Merged

zachdaniel merged 2 commits into
ash-project:mainfrom
C-Sinclair:improvement/tenant-in-transaction-reason

Conversation

@C-Sinclair

@C-Sinclair C-Sinclair commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

A data layer that picks its connection per tenant needs the tenant in c:Ash.DataLayer.transaction/4. The reason Ash builds names the resource, the action and the actor, but not the tenant.

  • Ash.DataLayer.transaction_reason/0 gains optional(:tenant) => term() on every variant that already declares optional(:data_layer_context). The key is optional, so a data layer that ignores it is unaffected.
  • Ash.Changeset puts changeset.tenant on the reason in Ash.Changeset.with_hooks/3. That one site covers create, update and destroy.
  • Ash.Actions.Create.Bulk, Ash.Actions.Update.Bulk and Ash.Actions.Destroy.Bulk pass opts[:tenant] at both their outer and inner transactions. Ash.Actions.Read passes query.tenant.
  • Ash.Actions.Update.UpdateMany gains :tenant and :data_layer_context. It built the only reason that had neither.

The tenant sits beside :data_layer_context rather than inside it, so a data layer reads a named key instead of a convention:

def transaction(_resource, fun, _timeout, %{tenant: tenant}) do
  Repo.put_dynamic_repo(repo_for_tenant(tenant))
  Repo.transaction(fun)
end

Where this came from

ash_sqlite#224 adds strategy :context to AshSqlite, giving each tenant its own SQLite file. It carries AshSqlite.Changes.CarryTenant and AshSqlite.Transformers.CarryTenant only to put the tenant where transaction/4 can read it. @zachdaniel asked why that was necessary and suggested Ash set it in the transaction metadata. Both modules are deleted there once this lands.

With the transformer disabled, create, update, destroy, bulk_create and bulk_update all reach transaction/4 with data_layer_context: %{} and no tenant. Reads were the only path already carrying it, on query.

Not in this PR

No test asserts the new key. Of the built-in data layers only Ash.DataLayer.Mnesia implements transaction/4, and it does not read the reason, so a test needs a data layer that records what it is given. Ash.DataLayer.Ets answers false to :transact and implements neither rollback/2 nor in_transaction?/1, so wrapping it means writing a transaction implementation too. I can add such a module under test/support if you want the coverage here rather than in the data layer that consumes it.

A data layer that chooses its connection per tenant needs the tenant in
`c:Ash.DataLayer.transaction/4`. Ash calls that callback above the data layer, and
the reason it builds names the resource, the action and the actor, but not the
tenant. A database-per-tenant data layer therefore has nothing to select a
connection with, and has to smuggle the tenant into the changeset context with a
change of its own before the transaction opens.

`Ash.Changeset` already forwards `changeset.context[:data_layer]` to the callback
as `:data_layer_context`, so the delivery path exists. What is missing is that
nothing puts the tenant into it. This adds `:tenant` to the reason instead, beside
the context rather than inside it, so it is named rather than found:

  * `Ash.Changeset` sets `changeset.tenant`, which covers create, update and
    destroy.
  * `Ash.Actions.Create.Bulk`, `Ash.Actions.Update.Bulk` and
    `Ash.Actions.Destroy.Bulk` set `opts[:tenant]`, at both their outer and inner
    transactions.
  * `Ash.Actions.Update.UpdateMany` gains `:tenant` and `:data_layer_context`. It
    built the only reason with neither.
  * `Ash.Actions.Read` sets `query.tenant`. A read already carried its query, so
    the tenant was reachable there, and naming it keeps every reason uniform.

`Ash.DataLayer.transaction_reason/0` declares `optional(:tenant) => term()` on each
variant that already declares `optional(:data_layer_context)`. The key is optional,
so a data layer that ignores it is unaffected.

This also gives `Ash.transaction/2` somewhere to put a tenant, should it take one.
C-Sinclair added a commit to C-Sinclair/ash_sqlite that referenced this pull request Sep 8, 2026
Depends on ash-project/ash#2917, which adds `:tenant` to the transaction reason.
Until that merges, the nine tests that reach `transaction/4` through an Ash action
fail with "carried no tenant". Every other test passes.

`reason_tenant/1` becomes `reason[:tenant]`. It previously looked in three places,
because no path named the tenant and each forwarded something different: the
single-record paths a data layer context, the bulk paths a whole changeset
context, and a read its query.

`AshSqlite.Changes.CarryTenant` and `AshSqlite.Transformers.CarryTenant` are
deleted. They existed only to put the tenant somewhere `transaction/4` could reach
it, which is what @zachdaniel objected to, and Ash naming it removes the reason for
them to exist.

`without_binder/3` goes too. It raised when a `strategy :context` resource had no
binder, which `AshSqlite.Verifiers.VerifyTenantBinder` already fails the compile
for. Its branch stays as `fun.()`, because a resource with multitenancy this data
layer does not resolve to a connection, such as `strategy :attribute`, reaches
that clause with a tenant and nothing to bind.
@C-Sinclair C-Sinclair changed the title improvement: name the tenant in the transaction reason improvement: passing tenant in the transaction reason Sep 8, 2026
@C-Sinclair
C-Sinclair marked this pull request as ready for review September 8, 2026 14:00
type: :bulk_update,
metadata: %{resource: resource, action: action.name},
tenant: opts[:tenant],
data_layer_context: opts[:data_layer_context] || %{}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were also missing data_layer_context for Update Many, so I've added it here as well

@zachdaniel

Copy link
Copy Markdown
Contributor

I think we will also need to add the tenant option to Ash.transact and Ash.transaction

  @transaction_opts_schema [
    timeout: [
      type: :timeout,
      doc: """
      The time in milliseconds (as an integer) to wait for the transaction to finish or `:infinity` to wait indefinitely.

      If not specified then default behaviour is adapter specific - for `Ecto`-based data layers it will be `15_000`.
      """
    ],
    return_notifications?: [
      type: :boolean,
      default: false,
      doc: """
      Use this if you want to manually handle sending notifications.

      If true the returned tuple will contain notifications list as the last element.

      To send notifications use `Ash.Notifier.notify(notifications)`. It sends any notifications that can be sent, and returns the rest.
      """
    ]
  ]

and thread it through.

…easons

`Ash.transact/3` and `Ash.transaction/3` build the `:custom` transaction reason
and took no tenant, so a data layer that chooses its connection per tenant got
nothing from a hand-written transaction. `@transaction_opts_schema` gains
`:tenant`, typed `{:protocol, Ash.ToTenant}` like every other tenant option, and
both functions put it on the reason.

`Ash.Actions.Action.run/4` builds the `:generic` reason for a generic action with
`transaction? true`. It named the actor and the data layer context but not the
tenant, so it now sets `input.tenant`.

`Ash.DataLayer.transaction_reason/0` declares `optional(:tenant) => term()` on the
`:custom` variant and on the catch-all variant. The six variants that declare
`optional(:data_layer_context)` already had it.
@zachdaniel
zachdaniel merged commit f1602c4 into ash-project:main Sep 9, 2026
48 of 51 checks passed
@zachdaniel

Copy link
Copy Markdown
Contributor

🚀 Thank you for your contribution! 🚀

C-Sinclair added a commit to C-Sinclair/ash_sqlite that referenced this pull request Sep 9, 2026
ash-project/ash#2917 merged as f1602c4 and puts `:tenant` on the transaction
reason, which `AshSqlite.DataLayer.transaction/4` reads. It is not in a release
yet: v3.33.1 was cut the day before it merged.

The nine tests that reach `transaction/4` through an Ash action now pass, and
the whole suite is green at 203.

`ash_version/1` still takes ASH_VERSION, so `local` and a version number keep
working. Restore `ash_version("~> 3.34")` once a release carries the change,
as 4400623 did after 627d7c5.
C-Sinclair added a commit to C-Sinclair/ash_sqlite that referenced this pull request Sep 18, 2026
Depends on ash-project/ash#2917, which adds `:tenant` to the transaction reason.
Until that merges, the nine tests that reach `transaction/4` through an Ash action
fail with "carried no tenant". Every other test passes.

`reason_tenant/1` becomes `reason[:tenant]`. It previously looked in three places,
because no path named the tenant and each forwarded something different: the
single-record paths a data layer context, the bulk paths a whole changeset
context, and a read its query.

`AshSqlite.Changes.CarryTenant` and `AshSqlite.Transformers.CarryTenant` are
deleted. They existed only to put the tenant somewhere `transaction/4` could reach
it, which is what @zachdaniel objected to, and Ash naming it removes the reason for
them to exist.

`without_binder/3` goes too. It raised when a `strategy :context` resource had no
binder, which `AshSqlite.Verifiers.VerifyTenantBinder` already fails the compile
for. Its branch stays as `fun.()`, because a resource with multitenancy this data
layer does not resolve to a connection, such as `strategy :attribute`, reaches
that clause with a tenant and nothing to bind.
C-Sinclair added a commit to C-Sinclair/ash_sqlite that referenced this pull request Sep 18, 2026
ash-project/ash#2917 puts `:tenant` on the transaction reason, which
`AshSqlite.DataLayer.transaction/4` reads through `reason_tenant/1`. It shipped
in ash v3.33.2, so `~> 3.33` alone would resolve to v3.33.0 or v3.33.1 and the
nine tests that reach `transaction/4` through an Ash action would fail.

This replaces the temporary git dependency on ash that this branch carried while
the change was unreleased.
C-Sinclair added a commit to C-Sinclair/ash_sqlite that referenced this pull request Sep 18, 2026
Depends on ash-project/ash#2917, which adds `:tenant` to the transaction reason.
Until that merges, the nine tests that reach `transaction/4` through an Ash action
fail with "carried no tenant". Every other test passes.

`reason_tenant/1` becomes `reason[:tenant]`. It previously looked in three places,
because no path named the tenant and each forwarded something different: the
single-record paths a data layer context, the bulk paths a whole changeset
context, and a read its query.

`AshSqlite.Changes.CarryTenant` and `AshSqlite.Transformers.CarryTenant` are
deleted. They existed only to put the tenant somewhere `transaction/4` could reach
it, which is what @zachdaniel objected to, and Ash naming it removes the reason for
them to exist.

`without_binder/3` goes too. It raised when a `strategy :context` resource had no
binder, which `AshSqlite.Verifiers.VerifyTenantBinder` already fails the compile
for. Its branch stays as `fun.()`, because a resource with multitenancy this data
layer does not resolve to a connection, such as `strategy :attribute`, reaches
that clause with a tenant and nothing to bind.
C-Sinclair added a commit to C-Sinclair/ash_sqlite that referenced this pull request Sep 18, 2026
ash-project/ash#2917 puts `:tenant` on the transaction reason, which
`AshSqlite.DataLayer.transaction/4` reads through `reason_tenant/1`. It shipped
in ash v3.33.2, so `~> 3.33` alone would resolve to v3.33.0 or v3.33.1 and the
nine tests that reach `transaction/4` through an Ash action would fail.

This replaces the temporary git dependency on ash that this branch carried while
the change was unreleased.
zachdaniel pushed a commit to ash-project/ash_sqlite that referenced this pull request Oct 10, 2026
…po (#224)

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`.

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.

`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.

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.

`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? 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.

Deliberately none in this PR. The guide is being written once the whole feature
has landed, rather than in pieces that contradict each other.

* better code level comments

* improvement: serve `global? true` from one shared database rather than the caller's binding

`global?` means one copy of the rows. For a schema-based data layer that falls
out for free -- the global table sits in a schema the connection a tenanted
statement already holds can reach. One SQLite database per tenant has no such
connection, so it has to be chosen, and nothing chose it.

Two halves were wrong, and both are fixed here:

  * A tenantless statement ran on whatever the calling process had bound. A
    `global?` resource sharing a repo module with tenanted ones therefore read
    whichever tenant was bound last, and which one depended on what that process
    had done before.
  * A statement *with* a tenant was bound to it, so a write with `tenant: "acme"`
    landed in acme's file and every tenant accumulated its own copy of a table
    that is supposed to have exactly one.

A `global?` resource now binds its repo module's own named instance, explicitly,
and ignores the tenant. Which database holds the global rows follows from `repo`:
sharing the tenanted module puts them in that module's configured database,
naming another module puts them there. Neither depends on the caller.

## Why not a tenant binder callback

The obvious shape was an optional `bind_global/2` on `AshSqlite.TenantBinder`,
letting a binder pick the connection for a tenantless statement. It is the wrong
seam. A binder picks a connection *instance* within one repo module; shared rows
are not another instance of a tenant's database but another database, which Ash
and Ecto already address as a repo module and resolve through
`AshSqlite.DataLayer.Info.repo/2`. Routing by binding would have put connection
selection in two places with no rule for which wins, and would still have needed
the answer `repo` already gives.

The binder contract is unchanged and no binder needs updating.

## Why the shared repo is checked at runtime

A `global?` resource needs a repo module started under its own name *and* holding a
`database:`. A repo module serving only tenants needs neither -- it is reached
through `Ecto.Repo.put_dynamic_repo/1` -- so adding `global? true` to a resource on
one is an easy mistake, and both halves are checked because neither implies the
other:

  * Unstarted, the statement fails with Ecto's own "could not lookup Ecto repo",
    which names the repo but not the reason a `global?` resource wanted it.
  * Started with no `database:` -- which Ecto allows -- the statement waits out the
    pool timeout and then reports that requests are arriving faster than they can be
    served. Measured at roughly six seconds, and nothing in it points at the
    missing configuration.

Neither check can be a transformer. A repo's `database:` is very often set in
`config/runtime.exs`, which is the recommended shape for a release, and a
transformer runs long before that file is evaluated -- so it would reject exactly
the configuration it should accept. They run instead when a global statement first
needs the connection, where the answer is knowable.

Nothing changes for a repo module with no `global?` resource on it.
`AshSqlite.UnconfiguredRepo` is configured with nothing whatsoever -- no
`database:`, no pool, no name -- and stays that way, with a test that says so.

## Tests

`AshSqlite.TenantRepo` gains a database and is started under its own name --
which is what a shared database is here -- while deliberately continuing to serve
the tenanted resources, so the tests are pointed at the footgun this fixes: a
global resource sharing a repo module with tenanted ones, still reading one copy.

`AshSqlite.Test.UnstartedGlobalPost` covers both ways to have no shared database,
on a repo module that has neither a name nor a database of its own.

The three tests that pinned the old behaviour are replaced rather than adjusted.
They asserted that a tenantless read followed the process binding and that a
tenanted write landed in the tenant's file; both are now wrong on purpose.

* chore: trim inline commentary

The rationale behind these decisions belongs in the commit messages, where it
already is in full, rather than repeated in four-to-eight line blocks beside the
code. Kept to one or two lines each: what is not obvious from reading the
function, and nothing about alternatives considered or how it was measured.

Comment-only; no behaviour change.

* chore: describe the tenant binder without reference to other data layers

The `tenant_binder` option, `AshSqlite.TenantBinder`, `set_tenant/3` and the
verifier all explained themselves by contrast with a schema-based data layer --
"no schemas", "rather than a query prefix". A reader of ash_sqlite does not need
the comparison, so each one now says what SQLite does and what the option is for.

The same sentence appeared in four places. `AshSqlite.DataLayer.Info.tenant_binder/1`
loses its second paragraph as well, which said what the `nil` in the first already
says. The DSL cheat sheet is regenerated from the option's new doc.

* improvement: refuse `global? true` with `strategy :context` rather than guess

`global?` allows a resource to be used with or without a tenant. Each tenant has
its own database file here, so a statement carrying no tenant names no database,
and there is no shared connection to fall back to.

The previous commit chose one reading: a `global? true` resource bound its repo
module's own named instance and ignored the tenant. That is not the reading a
schema-based data layer implements, where such a resource writes to the public
schema as well as the tenant's. Rather than settle the question inside this PR,
`AshSqlite.Verifiers.VerifyGlobalMultitenancy` refuses the combination, and the
error names the alternative: shared tables on a resource with no multitenancy,
pointing at a repo module of its own.

What this removes from the data layer: `bind_global/2`, `verify_shared_repo!/2`
and `shared_repo_error/3`, the `global?` branch of `bind_tenant/4`, and the
`multitenancy_global?/1` term in `tenant_required?/1`, which no `:context`
resource can satisfy any more. `AshSqlite.TenantRepo` goes back to having no
`database:` and no named instance, because only a `global?` resource needed
either.

`unbound/2` no longer suggests `global? true` in its error, and the comment on
`without_binder/3` no longer claims a Spark verifier only warns. A `DslError`
from a verifier is reported by `Module.ParallelChecker`, so `mix compile` fails;
that guard is reached only by a resource built at runtime.

* chore: take a bulk operation's tenant from its first changeset

Ash does not send mixed tenants into one bulk operation, so `changesets_tenant/1`
does not need to check for them. It mapped every changeset's tenant, took the
unique set and raised on more than one. Now it matches the first changeset.

The raise it drops was never reachable through Ash, and no test covered it.

* improvement: read the tenant from the transaction reason

Depends on ash-project/ash#2917, which adds `:tenant` to the transaction reason.
Until that merges, the nine tests that reach `transaction/4` through an Ash action
fail with "carried no tenant". Every other test passes.

`reason_tenant/1` becomes `reason[:tenant]`. It previously looked in three places,
because no path named the tenant and each forwarded something different: the
single-record paths a data layer context, the bulk paths a whole changeset
context, and a read its query.

`AshSqlite.Changes.CarryTenant` and `AshSqlite.Transformers.CarryTenant` are
deleted. They existed only to put the tenant somewhere `transaction/4` could reach
it, which is what @zachdaniel objected to, and Ash naming it removes the reason for
them to exist.

`without_binder/3` goes too. It raised when a `strategy :context` resource had no
binder, which `AshSqlite.Verifiers.VerifyTenantBinder` already fails the compile
for. Its branch stays as `fun.()`, because a resource with multitenancy this data
layer does not resolve to a connection, such as `strategy :attribute`, reaches
that clause with a tenant and nothing to bind.

* chore: require ash 3.33.2 for the tenant on the transaction reason

ash-project/ash#2917 puts `:tenant` on the transaction reason, which
`AshSqlite.DataLayer.transaction/4` reads through `reason_tenant/1`. It shipped
in ash v3.33.2, so `~> 3.33` alone would resolve to v3.33.0 or v3.33.1 and the
nine tests that reach `transaction/4` through an Ash action would fail.

This replaces the temporary git dependency on ash that this branch carried while
the change was unreleased.

* chore: drop the stale transformers left by the merge from main

- Remove the `transformers:` list from `AshSqlite.DataLayer`. Upstream moved `ValidateReferences`, `VerifyRepo` and `EnsureTableOrPolymorphic` to verifiers, and the merge kept both lists, so test compilation failed on the missing transformer modules.
- Define `min_sqlite_version/0` in the four test repos, with the value the fallback in `AshSqlite.Repo.BeforeCompile` already assumes. Without it the fallback warns, and `mix compile --warnings-as-errors` fails.
- Lock `ash_sql` 0.8.1, which carries the list aggregate sort fix that `test/join_subquery_sort_test.exs` and `test/uniq_aggregate_sort_test.exs` exercise.

* improvement: name the per-tenant connection option `tenant_repo`

- Rename the `tenant_binder` option to `tenant_repo`, so it sits next to `repo` and reads as the same kind of thing: `repo` names the module, `tenant_repo` names the instance for a tenant.
- `tenant_repo` takes a function `(tenant, %{resource: resource, type: :read | :mutate}) -> pid | atom`, or a module implementing `AshSqlite.TenantRepo`. `type` uses the values the `repo` option is called with, so a transaction is reported as `:mutate`.
- The data layer binds the returned instance with `put_dynamic_repo/1` around each statement and restores the previous binding after. A module can implement the optional `with_repo/4` callback to bind it itself, for work that has to bracket the statement.
- `AshSqlite.TenantBinder` becomes `AshSqlite.TenantRepo`, `VerifyTenantBinder` becomes `VerifyTenantRepo`, and `AshSqlite.DataLayer.Info.tenant_binder/1` becomes `tenant_repo/1`, which returns `{module, opts}`.
- The test repo previously named `AshSqlite.TenantRepo` is now `AshSqlite.TenantTestRepo`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants