diff --git a/lib/supavisor/client_handler.ex b/lib/supavisor/client_handler.ex index ae2f63ac..af89a12b 100644 --- a/lib/supavisor/client_handler.ex +++ b/lib/supavisor/client_handler.ex @@ -259,7 +259,7 @@ defmodule Supavisor.ClientHandler do Logger.metadata(state: :exchange) sni_hostname = HandlerHelpers.try_get_sni(sock) - case Tenants.get_user_cache(type, user, tenant_or_alias, sni_hostname) do + case Tenants.fetch_user_cache(type, user, tenant_or_alias, sni_hostname) do {:ok, info} -> db_name = db_name || info.tenant.db_database diff --git a/lib/supavisor/tenants.ex b/lib/supavisor/tenants.ex index a404ba0a..6ed9c7dd 100644 --- a/lib/supavisor/tenants.ex +++ b/lib/supavisor/tenants.ex @@ -84,46 +84,45 @@ defmodule Supavisor.Tenants do def get_tenant(_, _), do: nil - @spec get_user_cache(:single | :cluster, String.t(), String.t() | nil, String.t() | nil) :: + @spec fetch_user_cache(:single | :cluster, String.t(), String.t() | nil, String.t() | nil) :: {:ok, map()} | {:error, any()} - def get_user_cache(type, user, external_id, sni_hostname) do + def fetch_user_cache(type, user, external_id, sni_hostname) do cache_key = {:user_cache, type, user, external_id, sni_hostname} - case Cachex.fetch(Supavisor.Cache, cache_key, fn _key -> - {:commit, {:cached, get_user(type, user, external_id, sni_hostname)}, - ttl: :timer.hours(24)} - end) do - {_, {:cached, value}} -> value - {_, {:cached, value}, _} -> value + case Cachex.fetch(Supavisor.Cache, cache_key, fn key -> build_user_cache(key) end) do + {:ok, {:cached, value}} -> {:ok, value} + {:commit, {:cached, value}, _} -> {:ok, value} + {:ignore, error} -> {:error, error} + end + end + + defp build_user_cache({_key, type, user, external_id, sni_hostname}) do + case fetch_user(type, user, external_id, sni_hostname) do + {:ok, value} -> {:commit, {:cached, value}, ttl: :timer.hours(24)} + {:error, reason} -> {:ignore, reason} end end - @spec get_user(atom(), String.t(), String.t() | nil, String.t() | nil) :: + @spec fetch_user(atom(), String.t(), String.t() | nil, String.t() | nil) :: {:ok, map()} | {:error, any()} - def get_user(_, _, nil, nil) do + def fetch_user(_, _, nil, nil) do {:error, "Either external_id or sni_hostname must be provided"} end - def get_user(:cluster, user, external_id, sni_hostname) do + def fetch_user(:cluster, user, external_id, sni_hostname) do query = - from(ct in ClusterTenants, - where: ct.cluster_alias == ^external_id and ct.active == true, - limit: 1 - ) + from(ct in ClusterTenants, where: ct.cluster_alias == ^external_id and ct.active == true) - case Repo.all(query) do - [%ClusterTenants{} = ct] -> - get_user(:single, user, ct.tenant_external_id, sni_hostname) + case Repo.one(query) do + %ClusterTenants{} = ct -> + fetch_user(:single, user, ct.tenant_external_id, sni_hostname) - [_ | _] -> - {:error, :multiple_results} - - _ -> + nil -> {:error, :not_found} end end - def get_user(:single, user, external_id, sni_hostname) do + def fetch_user(:single, user, external_id, sni_hostname) do query = build_user_query(user, external_id, sni_hostname) case Repo.all(query) do diff --git a/test/supavisor/tenants_test.exs b/test/supavisor/tenants_test.exs index f41bb860..0ff0353f 100644 --- a/test/supavisor/tenants_test.exs +++ b/test/supavisor/tenants_test.exs @@ -139,12 +139,73 @@ defmodule Supavisor.TenantsTest do assert %Ecto.Changeset{} = Tenants.change_tenant(tenant) end - test "get_user/4" do - _tenant = tenant_fixture() - assert {:error, :not_found} = Tenants.get_user(:single, "no_user", "no_tenant", "") + test "fetch_user_cache/4 returns a user from cache" do + tenant = + tenant_fixture(%{external_id: "user_cache_tenant", sni_hostname: "user.example.com"}) + + user = List.first(tenant.users) + + assert {:error, "Either external_id or sni_hostname must be provided"} == + Tenants.fetch_user_cache(:single, user.db_user, nil, nil) + end + + test "fetch_user/4 returns :multiple_results when more than one user matches" do + users_attrs = [ + %{ + "db_user" => "postgres", + "db_password" => "postgres", + "pool_size" => 3, + "mode_type" => "session" + }, + %{ + "db_user" => "postgres", + "db_password" => "postgres", + "pool_size" => 3, + "mode_type" => "transaction" + } + ] + + _tenant = tenant_fixture(%{users: users_attrs}) + + assert {:error, :multiple_results} = + Tenants.fetch_user(:single, "postgres", "dev_tenant", "") + end + + test "fetch_user/4 returns :not_found for missing user or tenant, and :ok for valid input" do + tenant = tenant_fixture() + user = List.first(tenant.users) + + assert {:error, :not_found} = Tenants.fetch_user(:single, "no_user", "no_tenant", "") + assert {:error, :not_found} = Tenants.fetch_user(:single, "postgres", nil, "") + + assert {:ok, %{user: fetched_user, tenant: fetched_tenant}} = + Tenants.fetch_user(:single, "postgres", "dev_tenant", "") + + assert fetched_user.id == user.id + assert fetched_tenant.id == tenant.id + end + + test "fetch_user/4 with :cluster mode returns :not_found for invalid tenant alias and :ok for valid alias" do + cluster_tenant_attrs = %{ + type: "write", + cluster_alias: "cluster_alias", + tenant_external_id: "dev_tenant", + active: true + } + + tenant = tenant_fixture() + user = List.first(tenant.users) + + _cluster = + cluster_fixture(%{alias: "cluster_alias", cluster_tenants: [cluster_tenant_attrs]}) + + assert {:error, :not_found} == Tenants.fetch_user(:cluster, "postgres", "cluster", "") + + assert {:ok, %{user: fetched_user, tenant: fetched_tenant}} = + Tenants.fetch_user(:cluster, "postgres", "cluster_alias", "") - assert {:ok, %{tenant: _, user: _}} = - Tenants.get_user(:single, "postgres", "dev_tenant", "") + assert fetched_user.id == user.id + assert fetched_tenant.id == tenant.id end test "update_tenant_ps/2 updates the tenant's default_parameter_status" do diff --git a/test/supavisor_web/controllers/tenant_controller_test.exs b/test/supavisor_web/controllers/tenant_controller_test.exs index 02672727..c0dfa6e9 100644 --- a/test/supavisor_web/controllers/tenant_controller_test.exs +++ b/test/supavisor_web/controllers/tenant_controller_test.exs @@ -180,7 +180,7 @@ defmodule SupavisorWeb.TenantControllerTest do end defp set_cache(external_id) do - Supavisor.Tenants.get_user_cache(:single, "user", external_id, nil) + Supavisor.Tenants.fetch_user_cache(:single, "user", external_id, nil) Supavisor.Tenants.get_tenant_cache(external_id, nil) end