Repository: Kraigie/nostrum
Pull request #365 (2021-11-25 12:55:15)

Converting ChannelCache to be pluggable
Work in progress PR resolving issue #355.

We made the adjustments to transform ChannelCache to be pluggable just as asked and created tests for this.

The PR is in Work in Progress because we need to update the Docs on the files created.

:)

jchristgit: Hi @panoramix360 and @gbottari, thanks for your contribution!

This looks great so far! I will give it a complete review once you consider it ready.

As for your question here:

> We noticed that there isn't any tests for ChannelCache so we are creating these but we had some problems with an ETS table that is needed for ChannelCache.get/1 that is only created on the Application.setup_ets_tables/0.

Due to the callbacks implementing the supervisor behaviour (just like in `Nostrum.Cache.GuildCache.ETS`), you should be able to skip the separate `Application.setup_ets_tables()` call, as the correct table should be created by `init`.
gbottari: Hi @jchristgit, thanks for your kind words!

I guess the problem we ran into is that `ChannelCache.ETS.lookup()` actually relies on `GuildCache.select_by()` behind the scenes. `select_by()` in turn, uses `Mapping.ChannelGuild.get_guild()` which relies on table `:channel_guild_map`. The only place table `:channel_guild_map` is initialized is in `Application.setup_ets_tables()`.

As we see it right now the only way to test ChannelCache is to initialize `:channel_guild_map` table somehow or maybe work around this dependency somehow.
jchristgit: Hi @gbottari. Of course, I completely missed that, sorry! Yeah, in that case, the current startup logic is perfectly fine. 👍 
panoramix360: Hi @jchristgit, we finished the PR :)

Waiting for your approval.

:+1: 

Patch:
diff --git a/lib/nostrum/application.ex b/lib/nostrum/application.ex
index b4e591b4..a186a665 100644
--- a/lib/nostrum/application.ex
+++ b/lib/nostrum/application.ex
@@ -29,7 +29,6 @@ defmodule Nostrum.Application do
     :ets.new(:gateway_url, [:set, :public, :named_table])
     :ets.new(:unavailable_guilds, [:set, :public, :named_table])
     :ets.new(:users, [:set, :public, :named_table])
-    :ets.new(:channels, [:set, :public, :named_table])
     :ets.new(:guild_shard_map, [:set, :public, :named_table])
     :ets.new(:channel_guild_map, [:set, :public, :named_table])
   end
diff --git a/lib/nostrum/cache/cache_supervisor.ex b/lib/nostrum/cache/cache_supervisor.ex
index 4a9a6110..ecb279f4 100644
--- a/lib/nostrum/cache/cache_supervisor.ex
+++ b/lib/nostrum/cache/cache_supervisor.ex
@@ -22,6 +22,7 @@ defmodule Nostrum.Cache.CacheSupervisor do
       Nostrum.Cache.Me,
       # Uses the configured cache implementations.
       Nostrum.Cache.GuildCache,
+      Nostrum.Cache.ChannelCache,
       Nostrum.Cache.PresenceCache
     ]
 
diff --git a/lib/nostrum/cache/channel_cache.ex b/lib/nostrum/cache/channel_cache.ex
index 28a7327e..7f796758 100644
--- a/lib/nostrum/cache/channel_cache.ex
+++ b/lib/nostrum/cache/channel_cache.ex
@@ -1,24 +1,66 @@
 defmodule Nostrum.Cache.ChannelCache do
+  @default_cache_implementation Nostrum.Cache.ChannelCache.ETS
   @moduledoc """
-  Cache for channels.
-
-  The ETS table name associated with the Channel Cache is `:channels`. Besides the
-  methods provided below you can call any other ETS methods on the table.
-
-  ## Example
-  ```elixir
-  info = :ets.info(:channels)
-  [..., heir: :none, name: :channels, size: 1, ...]
-  size = info[:size]
-  1
-  ```
+  Cache behaviour & dispatcher for channels.
+
+  You can call the functions provided by this module independent of which cache
+  is configured, and it will dispatch to the configured cache implementation.
+  The user-facing functions for reading the cache can be found in the "Reading
+  the cache" section.
+
+  By default, #{@default_cache_implementation} will be used for caching channels.
+  You can override this in the `:caches` option of the `:nostrum` application
+  by setting the `:channels` field to a different module implementing the
+  `Nostrum.Cache.ChannelCache` behaviour. Any module below
+  `Nostrum.Cache.ChannelCache` can be used as a cache.
+
+  ## Writing your own channel cache
+
+  As with the other caches, the channel cache API consists of two parts:
+
+  - The functions that the user calls, currently only `get/1` and `get!/1`
+
+  - The functions that nostrum calls, such as `c:create/1` or `c:update/1`.
+  These **do not create any objects in the Discord API**, they are purely
+  created to update the cached data from data that Discord sends us. If you
+  want to create objects on Discord, use the functions exposed by `Nostrum.Api`
+  instead.
+
+  You need to implement both of them for nostrum to work with your custom
+  cache. **You also need to implement `Supervisor` callbacks**, which will
+  start your cache as a child under `Nostrum.Cache.CacheSupervisor`: As an
+  example, the `Nostrum.Cache.ChannelCache.ETS` implementation uses this to to
+  set up its ETS table it uses for caching. See the callbacks section for every
+  nostrum-related callback you need to implement.
+
+  The "upstream data" wording in this module references the fact that the
+  data that the channel cache (and other caches) retrieves represents the raw
+  data we receive from the upstream connection, no attempt is made by nostrum
+  to sanitize the data before it enters the cache. Caching implementations
+  need to cast the data to the resulting type themselves. A possible future
+  improvement would be moving the data casting into this module before the
+  backing cache implementation is called.
   """
 
-  alias Nostrum.Cache.GuildCache
   alias Nostrum.Struct.Channel
-  alias Nostrum.Util
 
-  import Nostrum.Snowflake, only: [is_snowflake: 1]
+  @configured_cache :nostrum
+                    |> Application.compile_env(:caches, %{})
+                    |> Map.get(:channels, @default_cache_implementation)
+
+  @typedoc "Specifies the reason for why a lookup operation has failed."
+  @type reason :: :channel_not_found
+
+  ## Supervisor callbacks
+  # These set up the backing cache.
+  @doc false
+  defdelegate init(init_arg), to: @configured_cache
+  @doc false
+  defdelegate start_link(init_arg), to: @configured_cache
+  @doc false
+  defdelegate child_spec(opts), to: @configured_cache
+
+  ## Behaviour specification
 
   @doc ~S"""
   Retrieves a channel from the cache.
@@ -39,78 +81,47 @@ defmodule Nostrum.Cache.ChannelCache do
   end
   ```
   """
-  @spec get(Channel.id() | Nostrum.Struct.Message.t()) :: {:error, atom} | {:ok, Channel.t()}
-  def get(%Nostrum.Struct.Message{channel_id: channel_id}), do: get(channel_id)
-
-  def get(id) when is_snowflake(id) do
-    case lookup(id) do
-      {:ok, channel} -> {:ok, convert(channel)}
-      error -> error
-    end
-  end
+  @doc section: :reading
+  @callback get(Channel.id() | Nostrum.Struct.Message.t()) ::
+              {:error, reason} | {:ok, Channel.t()} | {:error, reason}
 
-  @doc ~S"""
-  Same as `get/1`, but raises `Nostrum.Error.CacheError` in case of a failure.
+  @doc """
+  Same as `get/1`, but raises `Nostrum.Error.CacheError` in case of failure.
   """
-  @spec get!(Channel.id() | Nostrum.Struct.Message.t()) :: no_return | Channel.t()
-  def get!(%Nostrum.Struct.Message{channel_id: channel_id}), do: get!(channel_id)
-  def get!(id) when is_snowflake(id), do: id |> get |> Util.bangify_find(id, __MODULE__)
+  @doc section: :reading
+  @callback get!(Channel.id() | Nostrum.Struct.Message.t()) :: no_return | Channel.t()
 
-  @doc false
-  @spec create(map) :: Channel.t()
-  def create(channel) do
-    :ets.insert(:channels, {channel.id, channel})
-    convert(channel)
-  end
+  # Functions called from nostrum.
+  @doc "Create a channel in the cache."
+  @callback create(map) :: Channel.t()
 
-  @doc false
-  @spec update(Channel.t()) :: :noop | {Channel.t(), Channel.t()}
-  def update(channel) do
-    case lookup(channel.id) do
-      {:ok, old_channel} ->
-        {convert(old_channel), convert(channel)}
-
-      _ ->
-        :noop
-    end
-  end
+  @doc """
+  Update a channel from upstream data.
 
-  @doc false
-  @spec delete(Channel.id()) :: :noop | Channel.t()
-  def delete(id) do
-    case lookup(id) do
-      {:ok, channel} ->
-        :ets.delete(:channels, id)
-        convert(channel)
-
-      _ ->
-        :noop
-    end
-  end
+  Return the original channel before the update, and the updated channel.
+  """
+  @callback update(Channel.t()) :: :noop | {Channel.t(), Channel.t()}
 
-  @doc false
-  @spec lookup(Channel.id()) :: {:error, :channel_not_found} | {:ok, map}
-  def lookup(id) do
-    case :ets.lookup(:channels, id) do
-      [] ->
-        [channel_id: id]
-        |> GuildCache.select_by(fn %{channels: channels} ->
-          Map.get(channels, id, {:error, :id_not_found})
-        end)
-        |> case do
-          {:error, :id_not_found} ->
-            {:error, :channel_not_found}
-
-          res ->
-            res
-        end
-
-      [{^id, channel}] ->
-        {:ok, channel}
-    end
-  end
+  @doc """
+  Delete a channel from the cache.
 
-  @doc false
-  def convert(%{__struct__: _} = struct), do: struct
-  def convert(map), do: Channel.to_struct(map)
+  Return the old channel if it was cached, or `nil` otherwise.
+  """
+  @callback delete(Channel.id()) :: :noop | Channel.t()
+
+  @doc """
+  Lookup a channel from the cache by ID.
+
+  Return channel_not_found if not found.
+  """
+  @callback lookup(Channel.id()) :: {:error, reason} | {:ok, map}
+
+  # Dispatching logic
+
+  defdelegate get(channel_id), to: @configured_cache
+  defdelegate get!(channel_id), to: @configured_cache
+  defdelegate create(map), to: @configured_cache
+  defdelegate update(channel), to: @configured_cache
+  defdelegate delete(channel_id), to: @configured_cache
+  defdelegate lookup(channel_id), to: @configured_cache
 end
diff --git a/lib/nostrum/cache/channel_cache/ets.ex b/lib/nostrum/cache/channel_cache/ets.ex
new file mode 100644
index 00000000..b7c89582
--- /dev/null
+++ b/lib/nostrum/cache/channel_cache/ets.ex
@@ -0,0 +1,126 @@
+defmodule Nostrum.Cache.ChannelCache.ETS do
+  @table_name :nostrum_channels
+  @moduledoc """
+  An ETS-based cache for channels.
+
+  The supervisor defined by this module will set up the ETS table associated
+  with it.
+
+  The default table name under which channels are cached is `#{@table_name}`.
+  In addition to the cache behaviour implementations provided by this module,
+  you can also call regular ETS table methods on it, such as `:ets.info`.
+
+  Note that users should not call the functions not related to this specific
+  implementation of the cache directly. Instead, call the functions of
+  `Nostrum.Cache.ChannelCache` directly, which will dispatch to the configured
+  cache.
+  """
+  @moduledoc since: "0.5"
+
+  @behaviour Nostrum.Cache.ChannelCache
+
+  alias Nostrum.Cache.ChannelCache
+  alias Nostrum.Cache.GuildCache
+  alias Nostrum.Struct.Channel
+  alias Nostrum.Util
+  import Nostrum.Snowflake, only: [is_snowflake: 1]
+  use Supervisor
+
+  @doc "Start the supervisor."
+  def start_link(init_arg) do
+    Supervisor.start_link(__MODULE__, init_arg, name: __MODULE__)
+  end
+
+  @doc "Set up the cache's ETS table."
+  @impl Supervisor
+  def init(_init_arg) do
+    :ets.new(tabname(), [:set, :public, :named_table])
+    Supervisor.init([], strategy: :one_for_one)
+  end
+
+  @doc "Retrieve the ETS table name used for the cache."
+  @spec tabname :: atom()
+  def tabname, do: @table_name
+
+  # IMPLEMENTATION
+  @doc "Retrieve a channel from the cache by ID."
+  @impl ChannelCache
+  @spec get(Channel.id() | Nostrum.Struct.Message.t()) ::
+          {:ok, Channel.t()} | {:error, ChannelCache.reason()}
+  def get(%Nostrum.Struct.Message{channel_id: channel_id}), do: get(channel_id)
+
+  def get(id) when is_snowflake(id) do
+    case lookup(id) do
+      {:ok, channel} -> {:ok, convert(channel)}
+      error -> error
+    end
+  end
+
+  @doc "Same as `get/1`, but raises `Nostrum.Error.CacheError` in case of a failure."
+  @impl ChannelCache
+  @spec get!(Channel.id() | Nostrum.Struct.Message.t()) :: no_return | Channel.t()
+  def get!(%Nostrum.Struct.Message{channel_id: channel_id}), do: get!(channel_id)
+  def get!(id) when is_snowflake(id), do: id |> get |> Util.bangify_find(id, __MODULE__)
+
+  @doc "Converts and creates the given map as a channel in the cache."
+  @impl ChannelCache
+  @spec create(map) :: Channel.t()
+  def create(channel) do
+    :ets.insert(tabname(), {channel.id, channel})
+    convert(channel)
+  end
+
+  @doc "Update the given channel in the cache."
+  @impl ChannelCache
+  @spec update(Channel.t()) :: :noop | {Channel.t(), Channel.t()}
+  def update(channel) do
+    case lookup(channel.id) do
+      {:ok, old_channel} ->
+        {convert(old_channel), convert(channel)}
+
+      _ ->
+        :noop
+    end
+  end
+
+  @doc "Delete the channel from the cache by ID."
+  @impl ChannelCache
+  @spec delete(Channel.id()) :: :noop | Channel.t()
+  def delete(id) do
+    case lookup(id) do
+      {:ok, channel} ->
+        :ets.delete(tabname(), id)
+        convert(channel)
+
+      _ ->
+        :noop
+    end
+  end
+
+  @doc "Lookup a channel from the cache by ID and if it does not exist try to get from GuildCache with select_by"
+  @impl ChannelCache
+  @spec lookup(Channel.id()) :: {:error, :channel_not_found} | {:ok, map}
+  def lookup(id) do
+    case :ets.lookup(tabname(), id) do
+      [] ->
+        [channel_id: id]
+        |> GuildCache.select_by(fn %{channels: channels} ->
+          Map.get(channels, id, {:error, :id_not_found})
+        end)
+        |> case do
+          {:error, :id_not_found} ->
+            {:error, :channel_not_found}
+
+          res ->
+            res
+        end
+
+      [{^id, channel}] ->
+        {:ok, channel}
+    end
+  end
+
+  # Converts a map into a Channel
+  defp convert(%{__struct__: _} = struct), do: struct
+  defp convert(map), do: Channel.to_struct(map)
+end


Test patch:
diff --git a/test/nostrum/cache/channel_cache_test.exs b/test/nostrum/cache/channel_cache_test.exs
new file mode 100644
index 00000000..6496e44a
--- /dev/null
+++ b/test/nostrum/cache/channel_cache_test.exs
@@ -0,0 +1,80 @@
+defmodule Nostrum.Cache.ChannelCacheTest do
+  alias Nostrum.Struct.Channel
+  alias Nostrum.Application
+  use ExUnit.Case
+
+  @cache_modules [
+    # Implementations
+    Nostrum.Cache.ChannelCache.ETS
+  ]
+
+  for cache <- @cache_modules do
+    defmodule :"#{cache}Test" do
+      use ExUnit.Case
+      # this is needed because otherwise we cannot access
+      # the cache in the tests
+      @cache cache
+      @test_channel %{
+        id: 91231,
+        name: "Joe's Grumblings"
+      }
+
+      doctest @cache
+
+      describe "with an empty cache" do
+        setup do
+          Application.setup_ets_tables()
+          [pid: start_supervised!(@cache)]
+        end
+
+        test "get/1 returns channel not found" do
+          assert @cache.get(123) == {:error, :channel_not_found}
+        end
+
+        test "create/1 returns true" do
+          expected = Channel.to_struct(@test_channel)
+          assert @cache.create(@test_channel) == expected
+        end
+
+        test "update/1 with a non-existing channel returns :noop" do
+          assert @cache.update(@test_channel) == :noop
+        end
+      end
+
+      describe "with cached channel" do
+        setup do
+          Application.setup_ets_tables()
+          pid = start_supervised!(@cache)
+          @cache.create(@test_channel)
+          [pid: pid]
+        end
+
+        test "get/1 returns channel" do
+          expected = Channel.to_struct(@test_channel)
+          assert @cache.get(@test_channel.id) == {:ok, expected}
+        end
+
+        test "update/1 updates a channel" do
+          updated_channel = Map.put(@test_channel, :name, "Craig's Grumbling")
+          expected = Channel.to_struct(updated_channel)
+
+          {old, new} = @cache.update(updated_channel)
+          old_name = @test_channel.name
+
+          assert old_name == old.name
+          assert expected == new
+        end
+
+        test "delete/1 deletes a channel the channel deleted" do
+          deleted = @cache.delete(@test_channel.id)
+          expected = Channel.to_struct(@test_channel)
+          assert deleted == expected
+          assert @cache.get(@test_channel.id) == {:error, :channel_not_found}
+
+          # double delete:
+          assert :noop == @cache.delete(@test_channel.id)
+        end
+      end
+    end
+  end
+end