Merge pull request #610 from Th3-M4jor/bugfix-user-cache-not-casting-…

…before-saving-all-branches

Ensure user struct is casted properly before updating ets cache

lib/nostrum/cache/member_cache/ets.ex

@@ -12,6 +12,7 @@ defmodule Nostrum.Cache.MemberCache.ETS do
   @table_name :nostrum_guild_members
 
   alias Nostrum.Cache.MemberCache
+  alias Nostrum.Snowflake
   alias Nostrum.Struct.Guild
   alias Nostrum.Struct.Guild.Member
   alias Nostrum.Util
@@ -76,14 +77,22 @@ defmodule Nostrum.Cache.MemberCache.ETS do
   @impl MemberCache
   @spec update(Guild.id(), map()) :: {Guild.id(), Member.t() | nil, Member.t()}
   def update(guild_id, payload) do
-    new_member = Util.cast(payload, {:struct, Member})
+    # Force keys to be atoms before casting just to simplify finding the user_id
+    # because of the atom/string ambiguity issues from the gateway that Discord
+    # won't fix.
 
-    case :ets.lookup(@table_name, {guild_id, new_member.user_id}) do
+    member_payload = Map.new(payload, fn {k, v} -> {Util.maybe_to_atom(k), v} end)
+
+    member_id = Util.cast(member_payload[:user][:id], Snowflake)
+
+    case :ets.lookup(@table_name, {guild_id, member_id}) do
       [{key, old_member}] ->
+        new_member = Member.to_struct(member_payload, old_member)
         true = :ets.update_element(@table_name, key, {2, new_member})
         {guild_id, old_member, new_member}
 
       [] ->
+        new_member = Util.cast(member_payload, {:struct, Member})
         {guild_id, nil, new_member}
     end
   end

lib/nostrum/cache/member_cache/mnesia.ex

@@ -17,6 +17,7 @@ if Code.ensure_loaded?(:mnesia) do
     @behaviour Nostrum.Cache.MemberCache
 
     alias Nostrum.Cache.MemberCache
+    alias Nostrum.Snowflake
     alias Nostrum.Struct.Guild
     alias Nostrum.Struct.Guild.Member
     alias Nostrum.Util
@@ -76,18 +77,28 @@ if Code.ensure_loaded?(:mnesia) do
     @doc "Update the given member for the given guild in the cache."
     @spec update(Guild.id(), map()) :: {Guild.id(), Member.t() | nil, Member.t()}
     def update(guild_id, payload) do
-      new_member = Util.cast(payload, {:struct, Member})
-      key = {guild_id, new_member.user_id}
+      # Force keys to be atoms before casting just to simplify finding the user_id
+      # because of the atom/string ambiguity issues from the gateway that Discord
+      # won't fix.
 
-      old_member =
+      member_payload = Map.new(payload, fn {k, v} -> {Util.maybe_to_atom(k), v} end)
+
+      member_id = Util.cast(member_payload[:user][:id], Snowflake)
+
+      key = {guild_id, member_id}
+
+      {old_member, new_member} =
         :mnesia.activity(:sync_transaction, fn ->
           case :mnesia.read(@table_name, key, :write) do
             [{_tag, _key, _guild_id, _user_id, old_member} = entry] ->
+              new_member = Member.to_struct(member_payload, old_member)
+
               :mnesia.write(put_elem(entry, 4, new_member))
-              old_member
+              {old_member, new_member}
 
             [] ->
-              nil
+              new_member = Member.to_struct(member_payload)
+              {nil, new_member}
           end
         end)
 

lib/nostrum/cache/user_cache/ets.ex

@@ -11,6 +11,7 @@ defmodule Nostrum.Cache.UserCache.ETS do
 
   @table_name :nostrum_users
 
+  alias Nostrum.Snowflake
   alias Nostrum.Struct.User
   use Supervisor
 
@@ -51,24 +52,31 @@ defmodule Nostrum.Cache.UserCache.ETS do
   @doc "Update a user from upstream data."
   @spec update(map()) :: {User.t() | nil, User.t()}
   def update(info) do
-    converted = User.to_struct(info)
+    # We don't know if the user_id is an atom or a string here.
+    user_id =
+      (Map.get(info, :id) || Map.get(info, "id"))
+      |> Snowflake.cast!()
 
-    with {:ok, old_user} <- lookup(info.id),
-         new_user = Map.merge(old_user, info),
+    with {:ok, old_user} <- lookup(user_id),
+         new_user = User.to_struct(info, old_user),
          false <- old_user == new_user do
       :ets.insert(@table_name, {new_user.id, new_user})
       {old_user, new_user}
     else
       {:error, _} ->
         # User just came online, make sure to cache if possible
         # TODO: check for `:global_name` once fully rolled out?
-        if Enum.all?([:username, :discriminator], &Map.has_key?(info, &1)),
-          do: :ets.insert(@table_name, {info.id, info})
+
+        converted = User.to_struct(info)
+
+        if Enum.all?([:username, :discriminator], &is_map_key(info, &1)),
+          do: :ets.insert(@table_name, {converted.id, converted})
 
         {nil, converted}
 
       true ->
-        {nil, converted}
+        {:ok, old_user} = lookup(user_id)
+        {old_user, old_user}
     end
   end
 

lib/nostrum/cache/user_cache/mnesia.ex

@@ -13,6 +13,7 @@ if Code.ensure_loaded?(:mnesia) do
     @behaviour Nostrum.Cache.UserCache
 
     alias Nostrum.Cache.UserCache
+    alias Nostrum.Snowflake
     alias Nostrum.Struct.User
     use Supervisor
 
@@ -80,24 +81,26 @@ if Code.ensure_loaded?(:mnesia) do
     end
 
     @impl UserCache
-    @doc "Update the given member for the given guild in the cache."
+    @doc "Update a User if it exists in the cache."
     @spec update(map()) :: {User.t() | nil, User.t()}
     def update(payload) do
-      new_user = User.to_struct(payload)
+      # We don't know if the user_id is an atom or a string here.
+      user_id =
+        (Map.get(payload, :id) || Map.get(payload, "id"))
+        |> Snowflake.cast!()
 
-      old_user =
-        :mnesia.activity(:sync_transaction, fn ->
-          case :mnesia.read(@table_name, new_user.id, :write) do
-            [{_tag, _id, old_user} = entry] ->
-              :mnesia.write(put_elem(entry, 2, new_user))
-              old_user
+      :mnesia.activity(:sync_transaction, fn ->
+        case :mnesia.read(@table_name, user_id, :write) do
+          [{_tag, _id, old_user} = entry] ->
+            new_user = User.to_struct(payload, old_user)
 
-            [] ->
-              nil
-          end
-        end)
+            :mnesia.write(put_elem(entry, 2, new_user))
+            {old_user, new_user}
 
-      {old_user, new_user}
+          [] ->
+            {nil, User.to_struct(payload)}
+        end
+      end)
     end
 
     @impl UserCache

lib/nostrum/struct/guild/member.ex

@@ -308,4 +308,21 @@ defmodule Nostrum.Struct.Guild.Member do
 
     struct(__MODULE__, new)
   end
+
+  @doc false
+  @spec to_struct(map(), nil | __MODULE__.t()) :: __MODULE__.t()
+  def to_struct(map, nil), do: to_struct(map)
+
+  def to_struct(map, old_user) do
+    new =
+      map
+      |> Map.new(fn {k, v} -> {Util.maybe_to_atom(k), v} end)
+      |> Util.map_update_if_present(:roles, &Util.cast(&1, {:list, Snowflake}))
+      |> Util.map_update_if_present(:communication_disabled_until, &Util.maybe_to_datetime/1)
+      |> Util.map_update_if_present(:premium_since, &Util.maybe_to_datetime/1)
+      |> Util.map_update_if_present(:joined_at, &Util.maybe_to_unixtime/1)
+      |> Map.put(:user_id, Util.cast(map[:user][:id], Snowflake))
+
+    struct(old_user, new)
+  end
 end

lib/nostrum/struct/user.ex

@@ -177,4 +177,17 @@ defmodule Nostrum.Struct.User do
 
     struct(__MODULE__, new)
   end
+
+  @doc false
+  @spec to_struct(map(), nil | __MODULE__.t()) :: __MODULE__.t()
+  def to_struct(map, nil), do: to_struct(map)
+
+  def to_struct(map, old_user) do
+    new =
+      map
+      |> Map.new(fn {k, v} -> {Util.maybe_to_atom(k), v} end)
+      |> Map.update(:id, nil, &Util.cast(&1, Snowflake))
+
+    struct(old_user, new)
+  end
 end

test/nostrum/cache/user_cache_meta_test.exs

@@ -89,7 +89,7 @@ defmodule Nostrum.Cache.UserCacheMetaTest do
 
       describe "update/1" do
         test "returns `{nil, after}` on uncached user" do
-          payload = %{id: 8_284_967_893_178_597_859_421}
+          payload = %{id: 8_284_967_893_178_597}
           expected = {nil, User.to_struct(payload)}
           assert ^expected = @cache.update(payload)
         end