From 93a1f2d430968ed0528c869a7285f520820354c1 Mon Sep 17 00:00:00 2001 From: Luis Araujo Date: Thu, 28 Aug 2025 12:30:55 +0100 Subject: [PATCH 1/2] fix: correct get_user_cache fuction and add tests --- lib/supavisor/client_handler.ex | 2 +- lib/supavisor/tenants.ex | 37 +++++----- test/supavisor/tenants_test.exs | 71 +++++++++++++++++-- .../controllers/tenant_controller_test.exs | 2 +- 4 files changed, 85 insertions(+), 27 deletions(-) diff --git a/lib/supavisor/client_handler.ex b/lib/supavisor/client_handler.ex index 03b3ddca..8988e6f2 100644 --- a/lib/supavisor/client_handler.ex +++ b/lib/supavisor/client_handler.ex @@ -252,7 +252,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 f8a8e05c..92cab711 100644 --- a/lib/supavisor/tenants.ex +++ b/lib/supavisor/tenants.ex @@ -73,46 +73,43 @@ 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)} + 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) do - {_, {:cached, value}} -> value - {_, {:cached, value}, _} -> value + {:ok, {:cached, value}} -> {:ok, value} + {:commit, {:cached, value}, _} -> {:ok, value} + {:ignore, error} -> {:error, error} 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) - - [_ | _] -> - {:error, :multiple_results} + case Repo.one(query) do + %ClusterTenants{} = ct -> + fetch_user(:single, user, ct.tenant_external_id, sni_hostname) _ -> {: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 c7063b8d..a6129e3c 100644 --- a/test/supavisor/tenants_test.exs +++ b/test/supavisor/tenants_test.exs @@ -73,12 +73,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"}) - assert {:ok, %{tenant: _, user: _}} = - Tenants.get_user(:single, "postgres", "dev_tenant", "") + 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 fetched_user.id == user.id + assert fetched_tenant.id == tenant.id end end 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 From a3bff3fec7c8bf24f2a46eee80bb7c55345f74d3 Mon Sep 17 00:00:00 2001 From: Luis Araujo Date: Thu, 28 Aug 2025 12:30:55 +0100 Subject: [PATCH 2/2] fix: correct get_user_cache fuction and add tests --- lib/supavisor/tenants.ex | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/supavisor/tenants.ex b/lib/supavisor/tenants.ex index 7a2f26cf..6ed9c7dd 100644 --- a/lib/supavisor/tenants.ex +++ b/lib/supavisor/tenants.ex @@ -89,18 +89,20 @@ defmodule Supavisor.Tenants 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 -> - 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) do + 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 fetch_user(atom(), String.t(), String.t() | nil, String.t() | nil) :: {:ok, map()} | {:error, any()} def fetch_user(_, _, nil, nil) do @@ -115,7 +117,7 @@ defmodule Supavisor.Tenants do %ClusterTenants{} = ct -> fetch_user(:single, user, ct.tenant_external_id, sni_hostname) - _ -> + nil -> {:error, :not_found} end end