fix: correct get_user_cache fuction and add tests

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
 

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

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

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