improvement: spike out simplifying multitenancy via moving config to repo

.formatter.exs

@@ -35,7 +35,6 @@ spark_locals_without_parens = [
   statement: 2,
   strict?: 1,
   table: 1,
-  tenant_repo: 1,
   unique: 1,
   unique_index_names: 1,
   up: 1,

documentation/dsls/DSL-AshSqlite.DataLayer.md

@@ -36,7 +36,6 @@ end
 | Name | Type | Default | Docs |
 |------|------|---------|------|
 | [`repo`](#sqlite-repo){: #sqlite-repo .spark-required} | `module \| (any, any -> any)` |  | The repo that will be used to fetch your data. See the `AshSqlite.Repo` documentation for more. Can also be a capture of a named function in another module that takes a resource and a type `:read \| :mutate` and returns the repo. An inline `fn` is not supported, because it cannot be called while the resource is compiling. |
-| [`tenant_repo`](#sqlite-tenant_repo){: #sqlite-tenant_repo } | `(any, any -> any) \| module` |  | Selects the repo instance a tenanted statement runs on. Required for `strategy :context`, where each tenant has its own database file. Either a function `(tenant, %{resource: resource, type: :read \| :mutate}) -> pid \| atom`, or a module implementing `AshSqlite.TenantRepo`. |
 | [`migrate?`](#sqlite-migrate?){: #sqlite-migrate? } | `boolean` | `true` | Whether or not to include this resource in the generated migrations with `mix ash.generate_migrations` |
 | [`migration_types`](#sqlite-migration_types){: #sqlite-migration_types } | `keyword` | `[]` | A keyword list of attribute names to the ecto migration type that should be used for that attribute. Only necessary if you need to override the defaults. |
 | [`migration_defaults`](#sqlite-migration_defaults){: #sqlite-migration_defaults } | `keyword` | `[]` | A keyword list of attribute names to the ecto migration default that should be used for that attribute. The string you use will be placed verbatim in the migration. Use fragments like `fragment(\\"now()\\")`, or for `nil`, use `\\"nil\\"`. |

lib/data_layer.ex

@@ -203,12 +203,6 @@ defmodule AshSqlite.DataLayer do
         doc:
           "The repo that will be used to fetch your data. See the `AshSqlite.Repo` documentation for more. Can also be a capture of a named function in another module that takes a resource and a type `:read | :mutate` and returns the repo. An inline `fn` is not supported, because it cannot be called while the resource is compiling."
       ],
-      tenant_repo: [
-        type:
-          {:spark_function_behaviour, AshSqlite.TenantRepo, {AshSqlite.TenantRepo.Function, 2}},
-        doc:
-          "Selects the repo instance a tenanted statement runs on. Required for `strategy :context`, where each tenant has its own database file. Either a function `(tenant, %{resource: resource, type: :read | :mutate}) -> pid | atom`, or a module implementing `AshSqlite.TenantRepo`."
-      ],
       migrate?: [
         type: :boolean,
         default: true,
@@ -553,7 +547,7 @@ defmodule AshSqlite.DataLayer do
   @doc """
   A no-op on the query. Each tenant has its own database file, so the tenant is
   applied by choosing the connection rather than by changing the query. The
-  resource's `tenant_repo` chooses it, once per statement.
+  repo's `c:AshSqlite.Repo.tenant_repo/2` chooses it, once per statement.
   """
   def set_tenant(_resource, query, _tenant) do
     {:ok, query}
@@ -2206,8 +2200,13 @@ defmodule AshSqlite.DataLayer do
 
   @impl true
   def in_transaction?(resource) do
-    repo = AshSqlite.DataLayer.Info.repo(resource, :mutate)
+    resource
+    |> AshSqlite.DataLayer.Info.repo(:mutate)
+    |> repo_in_transaction?()
+  end
 
+  # A tenant repo's default instance is usually never started, so it can't be asked.
+  defp repo_in_transaction?(repo) do
     case repo.get_dynamic_repo() do
       pid when is_pid(pid) -> repo.in_transaction?()
       name when is_atom(name) -> !is_nil(GenServer.whereis(name)) and repo.in_transaction?()
@@ -2229,16 +2228,13 @@ defmodule AshSqlite.DataLayer do
 
     if is_nil(tenant) and tenant_required?(resource) do
       raise ArgumentError, """
-      #{inspect(resource)} has a tenant_repo and `strategy :context`, but the \
-      transaction about to be opened for this action carried no tenant.
+      #{inspect(resource)} has `strategy :context`, but the transaction about to be \
+      opened for it carried no tenant.
 
-      A transaction is opened on one connection, and for a database-per-tenant \
-      layout that means one tenant's database. Ash forwards only the changeset's \
-      data layer context to this callback, so the tenant has to travel in it:
+      A transaction is opened on one connection, which here means one tenant's \
+      database. Pass the tenant:
 
-          Ash.Changeset.set_context(changeset, %{data_layer: %{tenant: tenant}})
-
-      A global change on the resource is the usual place to do that.
+          Ash.transaction(#{inspect(resource)}, fun, tenant: tenant)
       """
     end
 
@@ -2285,69 +2281,74 @@ 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)
-
+  #
+  # Ecto binds an instance per repo module, so a tenant repo serves only
+  # `strategy :context` resources. That makes anything else on the module during a
+  # tenant's transaction tenant work too. `VerifyTenantRepo` enforces this at compile
+  # time; the raises below are for a function `repo` that resolves differently at
+  # runtime.
   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 ->
-        fun.()
-
-      {module, opts} ->
-        repo = AshSqlite.DataLayer.Info.repo(resource, :mutate)
-
-        # Captured before the bind: if a transaction is already open it is open on
-        # *this* connection, so a statement that binds elsewhere leaves it.
-        enclosing = if in_transaction?(resource), do: repo.get_dynamic_repo()
-
-        context = %{resource: resource, type: type}
+    repo = AshSqlite.DataLayer.Info.repo(resource, type)
 
-        bind_tenant_repo(module, tenant, context, opts, fn ->
-          if enclosing && repo.get_dynamic_repo() != enclosing do
-            raise ArgumentError, """
-            #{inspect(resource)} tried to run a statement for tenant \
-            #{inspect(tenant)} inside a transaction open on another tenant's \
-            database. SQLite cannot commit across files atomically in WAL mode, so \
-            this statement would commit on its own and survive a rollback.
-
-            Open one transaction per tenant instead.
-            """
-          end
+    case {tenant_required?(resource), tenant_repo?(repo)} do
+      {false, false} ->
+        fun.()
 
-          fun.()
-        end)
-    end
-  end
+      {true, true} ->
+        bind_tenant_repo(resource, repo, tenant, type, fun)
 
-  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))
+      {true, false} ->
+        raise ArgumentError, """
+        #{inspect(resource)} has `strategy :context`, but its repo #{inspect(repo)} \
+        does not implement `c:AshSqlite.Repo.tenant_repo/2`.
+        """
 
-      try do
-        fun.()
-      after
-        repo.put_dynamic_repo(previous)
-      end
+      {false, true} ->
+        raise ArgumentError, """
+        #{inspect(resource)} does not have `strategy :context`, but its repo \
+        #{inspect(repo)} is a tenant repo. Put it on a repo module of its own.
+        """
     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
-    if tenant_required?(resource) do
+  defp bind_tenant_repo(resource, _repo, nil, _type, _fun) 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
+
+  defp bind_tenant_repo(resource, repo, tenant, type, fun) do
+    instance = repo.tenant_repo(tenant, %{resource: resource, type: type})
+
+    # An open transaction is open on the bound instance, so a statement for another
+    # one would leave it.
+    if repo_in_transaction?(repo) and repo.get_dynamic_repo() != instance 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.
+      #{inspect(resource)} tried to run a statement for tenant #{inspect(tenant)} \
+      inside a transaction open on another tenant's database. SQLite cannot commit \
+      across files atomically in WAL mode, so this statement would commit on its \
+      own and survive a rollback.
+
+      Open one transaction per tenant instead.
       """
     end
 
-    fun.()
+    previous = repo.put_dynamic_repo(instance)
+
+    try do
+      fun.()
+    after
+      repo.put_dynamic_repo(previous)
+    end
+  end
+
+  @doc false
+  def tenant_repo?(repo) do
+    Code.ensure_loaded?(repo) and function_exported?(repo, :tenant_repo, 2)
   end
 
   defp tenant_required?(resource) do

lib/data_layer/info.ex

@@ -66,17 +66,6 @@ defmodule AshSqlite.DataLayer.Info do
   defp resource_module(dsl_state),
     do: Spark.Dsl.Transformer.get_persisted(dsl_state, :module)
 
-  @doc """
-  The tenant repo for a resource as `{module, opts}`, or nil.
-
-  Only set for resources with context multitenancy. A function given to the
-  `tenant_repo` option is returned wrapped in a module implementing
-  `AshSqlite.TenantRepo`.
-  """
-  def tenant_repo(resource) do
-    Extension.get_opt(resource, [:sqlite], :tenant_repo, nil, true)
-  end
-
   @doc "The configured table for a resource"
   def table(resource) do
     Extension.get_opt(resource, [:sqlite], :table, nil, true)

lib/repo.ex

@@ -33,6 +33,24 @@ defmodule AshSqlite.Repo do
   @doc "Whether Ash may wrap write actions on this repo's resources in a transaction"
   @callback write_transactions?() :: boolean()
 
+  @doc """
+  The repo instance, a pid or a registered name, that `tenant`'s statements run on.
+
+  Implementing this makes the repo a tenant repo, for `strategy :context`: each
+  tenant has its own database file, and the tenant is applied by choosing the
+  instance. Every resource on a tenant repo must use `strategy :context`, so put
+  tables shared across tenants on a repo module of their own.
+
+  `context.type` is `:read` for queries and aggregates, and `:mutate` for writes and
+  for opening a transaction.
+  """
+  @callback tenant_repo(
+              tenant :: term(),
+              context :: %{resource: Ash.Resource.t(), type: :read | :mutate}
+            ) :: pid() | atom()
+
+  @optional_callbacks tenant_repo: 2
+
   @doc "The path where your migrations are stored"
   @callback migrations_path() :: String.t() | nil
   @doc "Allows overriding a given migration type for *all* fields, for example if you wanted to always use :timestamptz for :utc_datetime fields"

lib/verifiers/verify_tenant_repo.ex

@@ -11,29 +11,59 @@ defmodule AshSqlite.Verifiers.VerifyTenantRepo do
 
   @impl true
   def verify(dsl) do
-    if Ash.Resource.Info.multitenancy_strategy(dsl) == :context and
-         is_nil(Verifier.get_option(dsl, [:sqlite], :tenant_repo)) do
-      {:error,
-       DslError.exception(
-         module: Verifier.get_persisted(dsl, :module),
-         path: [:sqlite, :tenant_repo],
-         message: """
-         `strategy :context` needs a `tenant_repo`.
-
-         Each tenant has its own database file, so a tenanted statement has to be \
-         given the repo instance for its tenant. Only the application knows how to \
-         map a tenant to a repo instance.
-
-             sqlite do
-               repo MyApp.Repo
-               tenant_repo &MyApp.Tenants.repo/2
-             end
-
-         See `AshSqlite.TenantRepo`.\
-         """
-       )}
-    else
-      :ok
-    end
+    context? = Ash.Resource.Info.multitenancy_strategy(dsl) == :context
+
+    # A function `repo` can name a different module for reads and writes.
+    [:read, :mutate]
+    |> Enum.map(&AshSqlite.DataLayer.Info.repo(dsl, &1))
+    |> Enum.uniq()
+    |> Enum.filter(&match?({:module, _}, Code.ensure_compiled(&1)))
+    |> Enum.find_value(:ok, fn repo ->
+      case {context?, AshSqlite.DataLayer.tenant_repo?(repo)} do
+        {true, false} -> {:error, not_a_tenant_repo(dsl, repo)}
+        {false, true} -> {:error, not_context(dsl, repo)}
+        _ -> nil
+      end
+    end)
+  end
+
+  defp not_a_tenant_repo(dsl, repo) do
+    DslError.exception(
+      module: Verifier.get_persisted(dsl, :module),
+      path: [:sqlite, :repo],
+      message: """
+      `strategy :context` needs a tenant repo, but #{inspect(repo)} does not \
+      implement `c:AshSqlite.Repo.tenant_repo/2`.
+
+      Each tenant has its own database file, so a tenanted statement has to be \
+      given the repo instance for its tenant. Only the application knows how to map \
+      a tenant to a repo instance:
+
+          defmodule MyApp.TenantRepo do
+            use AshSqlite.Repo, otp_app: :my_app
+
+            def tenant_repo(tenant, _context), do: MyApp.Tenants.repo_for(tenant)
+          end
+      """
+    )
+  end
+
+  defp not_context(dsl, repo) do
+    DslError.exception(
+      module: Verifier.get_persisted(dsl, :module),
+      path: [:sqlite, :repo],
+      message: """
+      #{inspect(repo)} is a tenant repo, so every resource on it must use \
+      `strategy :context`.
+
+      Ecto binds a repo instance per repo module, not per resource. Inside a \
+      tenant's transaction this resource would run on that tenant's database. Put \
+      it on a repo module of its own:
+
+          sqlite do
+            repo MyApp.SharedRepo
+          end
+      """
+    )
   end
 end

mix.exs

@@ -119,9 +119,6 @@ defmodule AshSqlite.MixProject do
           AshSqlite.Repo,
           AshSqlite.DataLayer
         ],
-        Multitenancy: [
-          AshSqlite.TenantRepo
-        ],
         Utilities: [
           AshSqlite.ManualRelationship
         ],