Repository navigation
improvement: passing tenant in the transaction reason - #2917
Merged
zachdaniel merged 2 commits intoSep 9, 2026
Merged
zachdaniel merged 2 commits into
zachdaniel merged 2 commits into
Conversation
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
marked this pull request as ready for review
September 8, 2026 14:00
C-Sinclair
commented
Sep 8, 2026
| type: :bulk_update, | ||
| metadata: %{resource: resource, action: action.name}, | ||
| tenant: opts[:tenant], | ||
| data_layer_context: opts[:data_layer_context] || %{} |
Contributor
Author
There was a problem hiding this comment.
We were also missing data_layer_context for Update Many, so I've added it here as well
Contributor
|
I think we will also need to add the @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.
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`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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/0gainsoptional(:tenant) => term()on every variant that already declaresoptional(:data_layer_context). The key is optional, so a data layer that ignores it is unaffected.Ash.Changesetputschangeset.tenanton the reason inAsh.Changeset.with_hooks/3. That one site covers create, update and destroy.Ash.Actions.Create.Bulk,Ash.Actions.Update.BulkandAsh.Actions.Destroy.Bulkpassopts[:tenant]at both their outer and inner transactions.Ash.Actions.Readpassesquery.tenant.Ash.Actions.Update.UpdateManygains:tenantand:data_layer_context. It built the only reason that had neither.The tenant sits beside
:data_layer_contextrather than inside it, so a data layer reads a named key instead of a convention:Where this came from
ash_sqlite#224 adds
strategy :contextto AshSqlite, giving each tenant its own SQLite file. It carriesAshSqlite.Changes.CarryTenantandAshSqlite.Transformers.CarryTenantonly to put the tenant wheretransaction/4can 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_createandbulk_updateall reachtransaction/4withdata_layer_context: %{}and no tenant. Reads were the only path already carrying it, onquery.Not in this PR
No test asserts the new key. Of the built-in data layers only
Ash.DataLayer.Mnesiaimplementstransaction/4, and it does not read the reason, so a test needs a data layer that records what it is given.Ash.DataLayer.Etsanswersfalseto:transactand implements neitherrollback/2norin_transaction?/1, so wrapping it means writing a transaction implementation too. I can add such a module undertest/supportif you want the coverage here rather than in the data layer that consumes it.