improvement: support strategy :context multitenancy via a tenant binder - #224
improvement: support strategy :context multitenancy via a tenant binder#224C-Sinclair wants to merge 5 commits into
strategy :context multitenancy via a tenant binder#224Conversation
…tions?`
`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 ash-project#91, and supersedes the work in ash-project#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 ash-project#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 ash-project#91 proposed and ash-project#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.
| # | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| defmodule AshSqlite.Test.TenantBinder do |
There was a problem hiding this comment.
I'm feeling like if we are offering this as a feature that we should just "do this for them". AshPostgres context multitenancy automatically manages schemas for you and the selection of schemas.
For example, we could use something like :persistent_term to manage repos globally, stopping/starting them as needed etc. In its current form it feels like we're offering context multitenancy but with a lot of manual management still left to the end user.
There was a problem hiding this comment.
Yeah I agree. It's something I was looking at in my experimental repo (ash_cell)
I'd love to have a great default exposed from AshSqlite instead!
My suggestion, if you're open to it: keep the tenant_binder + behaviour as an escape hatch. That way you expose the option for clients like Turso or Litestream etc to be implemented in the application layer. But we ship a default Binder as part of AshSqlite, that provides one connection/one db file per tenant, so zero code required to get that nice multitenancy behaviour!
:persistent_term is great, but I don't think it's the right shape here. Every put/erase copies the table and triggers a global GC scan across all processes to find references, activating and evicting tenants hits that costly write path. So the more tenant churn, the worse it gets.
Instead, we could just use a specific Registry, where connection processes are added and removed behind the scenes by the library.
I can take a look at extending this PR to include that implementation 👍
There was a problem hiding this comment.
Fair point. Will look to review the changes on Friday of this week, got a busy week ahead.
…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.
cfb0221 to
6c4d27f
Compare
…nder Closes the data layer half of ash-project#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.
6c4d27f to
b372245
Compare
| @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 |
There was a problem hiding this comment.
I opted for a follow on PR to provide a default engine for this. That way this PR stays focused on exposing the required extension points, and #225 can focus on implementation
There was a problem hiding this comment.
We'll therefore remove this Verifier once a default is available
| 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 |
There was a problem hiding this comment.
Aggregates need to be tested separately as they don't go via Ash.Actions.Read, so preparations nor around_transaction can touch them. Fortunately, tenant_binder is at the Datalayer ...layer.
|
|
||
| multitenancy do | ||
| strategy(:context) | ||
| global?(true) |
There was a problem hiding this comment.
Welcome to Footgun territory! 🦶🔫
This Resource will just reuse whatever Repo pid is bound, so a downstream application would need to take care with this multitenancy flag. Tempting to verify against it for now tbh.
global? needs some thinking about with how it will work with the database-per-tenant this change unlocks.
|
|
||
| assert bound("acme", fn -> global_titles() end) == ["acme's own"] | ||
| assert bound("globex", fn -> global_titles() end) == ["globex's own"] | ||
| end |
There was a problem hiding this comment.
This is the confusing behaviour
2 participants
Closes the data layer half of #127.
Important
Stacked on #223 — please review and merge that first. GitHub cannot express a stacked PR across forks, so the diff below includes #223's commits as well. The commit to review here is the last one.
This PR was previously much larger. It has been split: it now ships the seam and nothing behind it, and the managed runtime that used to be in here is a follow-up.
What
Turning on
strategy :contextcurrently fails at compile time withData 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, viaAshSql.dynamic_repo/3) invoke the result as a module, so passing an instance raisesArgumentError: 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 viaput_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/3is therefore a no-op on the query, and the tenant is deliberately not passed toAshSql.repo_opts/5— it reached Ecto as a table prefix and raisedSQLite3 does not support table prefixeson every write.The binder
tenant_bindernames a module implementingAshSqlite.TenantBinder, asked for a connection once per statement:There is no default binder in this PR, and
strategy :contextwithout one is refused — at compile time by a verifier, and at the first statement by the data layer, because a Spark verifier only warns.Why the data layer rather than the caller
Every entry point would otherwise have to call
put_dynamic_repo/1before Ash runs, and some cannot:Ash.count/2never entersAsh.Actions.Read, so no preparation oraround_transactionhook runs for it.atomic/3rather thanchange/3whenever it can build one statement, so a hook-installing change forcesrequire_atomic? false.Task.async, anAsh.loadfan-out, or a background job.bind/3also receives the resource and ausageof:read,:writeor: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. Binders that do not care can ignore it.Interaction with transactions (#223)
transaction/4never receives the tenant: Ash calls it above the data layer and the reason it builds does not name one.AshSqlite.Transformers.CarryTenantadds a change that puts it in the changeset context, implementingatomic/3as well aschange/3so 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?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.
Not in this PR
AshSqlite.MultiTenancy— registry, per-tenant connections, lazy migration, LRU residency, quarantine,all_tenants/1,rename/3— is the follow-up. That is the piece @zachdaniel asked for on the earlier version of this PR, and it is easier to review on its own.mix ash_sqlite.generate_migrationshas no tenant awareness, so the tenant migrations directory is yours to maintain. AshPostgres routes tenanted resources topriv/repo/tenant_migrations; matching that is not attempted here.mix ash_postgres.rollback --tenants. There is no counterpart, by choice — forward-only for now.Tests
mix compile --warnings-as-errorsclean,mix credo --strictclean,mix test192 passing.