ready for testing?

config/config.exs

@@ -6,3 +6,14 @@ config :nostrum,
 config :logger, :console, metadata: [:shard, :guild, :channel]
 
 if File.exists?("config/secret.exs"), do: import_config("secret.exs")
+
+if Mix.env() == :test do
+  config :nostrum,
+    # constrain the size of the message cache in tests
+    # to make it easier to test eviction
+    caches: [
+      message_cache_size_limit: 10,
+      message_cache_eviction_count: 4,
+      message_cache_table_name: :nostrum_messages_test
+    ]
+end

lib/nostrum/cache/message_cache/mnesia.ex

@@ -22,13 +22,21 @@ if Code.ensure_loaded?(:mnesia) do
     operations, so a full table scan + sort is required to find the oldest messages.
     """
 
-    @table_name :nostrum_messages
+    # allow us to override the table name for testing
+    # without accidentally overwriting the production table
+    @table_name Application.compile_env(
+                  :nostrum,
+                  [:caches, :message_cache_table_name],
+                  :nostrum_messages
+                )
     @record_name @table_name
 
-    @maximum_size :nostrum
-                  |> Application.compile_env([:caches, :message_cache_size_limit], 10_000)
-    @eviction_count :nostrum
-                    |> Application.compile_env([:caches, :message_cache_eviction_count], 100)
+    @maximum_size Application.compile_env(:nostrum, [:caches, :message_cache_size_limit], 10_000)
+    @eviction_count Application.compile_env(
+                      :nostrum,
+                      [:caches, :message_cache_eviction_count],
+                      100
+                    )
 
     @behaviour Nostrum.Cache.MessageCache
 
@@ -103,18 +111,8 @@ if Code.ensure_loaded?(:mnesia) do
         {@record_name, message.id, message.channel_id, message.author.id, message}
 
       writer = fn ->
-        size = :mnesia.table_info(@table_name, :size)
-
-        if size >= @maximum_size do
-          oldest_message_ids =
-            :nostrum_message_cache_qlc.sorted_by_age_with_limit(__MODULE__, @eviction_count)
-
-          Enum.each(oldest_message_ids, fn message_id ->
-            :mnesia.delete(@table_name, message_id, :write)
-          end)
-
-          :mnesia.write(record)
-        end
+        maybe_evict_records()
+        :mnesia.write(record)
       end
 
       {:atomic, :ok} = :mnesia.sync_transaction(writer)
@@ -136,7 +134,8 @@ if Code.ensure_loaded?(:mnesia) do
         case :mnesia.read(@table_name, id, :write) do
           [] ->
             # we don't have the old message, so we shouldn't
-            # save it in the cache
+            # save it in the cache as updates are not guaranteed
+            # to have the full message payload
             updated_message = Message.to_struct(atomized_payload)
             {nil, updated_message}
 
@@ -153,12 +152,13 @@ if Code.ensure_loaded?(:mnesia) do
     @doc "Removes a message from the cache."
     @spec delete(Channel.id(), Message.id()) :: Message.t() | :noop
     def delete(channel_id, message_id) do
-      key = {channel_id, message_id}
-
       :mnesia.activity(:sync_transaction, fn ->
-        case :mnesia.read(@table_name, key, :write) do
-          [{_tag, _key, _channel_id, _author_id, message}] ->
-            :mnesia.delete(@table_name, key, :write)
+        case :mnesia.read(@table_name, message_id, :write) do
+          # as a safety measure, we check the channel_id
+          # before deleting the message from the cache
+          # to prevent deleting messages from the wrong channel
+          [{_tag, _id, ^channel_id, _author_id, message}] ->
+            :mnesia.delete(@table_name, message_id, :write)
             message
 
           _ ->
@@ -168,18 +168,22 @@ if Code.ensure_loaded?(:mnesia) do
     end
 
     @impl MessageCache
-    @doc "Removes and returns a list of messages from the cache."
+    @doc """
+    Removes and returns a list of messages from the cache.
+    Messages not found in the cache will not be included in the returned list.
+    """
     @spec bulk_delete(Channel.id(), [Message.id()]) :: [Message.t()]
     def bulk_delete(channel_id, message_ids) do
-      Enum.map(message_ids, fn message_id ->
+      Enum.reduce(message_ids, [], fn message_id, list ->
         case delete(channel_id, message_id) do
           :noop ->
-            Message.to_struct(%{id: message_id, channel_id: channel_id})
+            list
 
           message ->
-            message
+            [message | list]
         end
       end)
+      |> Enum.reverse()
     end
 
     @impl MessageCache
@@ -208,5 +212,19 @@ if Code.ensure_loaded?(:mnesia) do
     def wrap_qlc(fun) do
       :mnesia.activity(:sync_transaction, fun)
     end
+
+    # assumes its called from within a transaction
+    defp maybe_evict_records do
+      size = :mnesia.table_info(@table_name, :size)
+
+      if size >= @maximum_size do
+        oldest_message_ids =
+          :nostrum_message_cache_qlc.sorted_by_age_with_limit(__MODULE__, @eviction_count)
+
+        Enum.each(oldest_message_ids, fn message_id ->
+          :mnesia.delete(@table_name, message_id, :write)
+        end)
+      end
+    end
   end
 end

lib/nostrum/cache/message_cache/noop.ex

@@ -37,11 +37,7 @@ defmodule Nostrum.Cache.MessageCache.Noop do
   def delete(_channel_id, _message_id), do: :noop
 
   @impl MessageCache
-  def bulk_delete(channel_id, message_ids) do
-    Enum.map(message_ids, fn message_id ->
-      Message.to_struct(%{id: message_id, channel_id: channel_id})
-    end)
-  end
+  def bulk_delete(_channel_id, _message_ids), do: []
 
   @impl MessageCache
   def channel_delete(_channel_id), do: :ok

lib/nostrum/util.ex

@@ -280,7 +280,8 @@ defmodule Nostrum.Util do
   def map_update_if_present(map, key, fun) do
     case map do
       %{^key => value} ->
-        fun.(value) |> Map.put(key, map)
+        new_value = fun.(value)
+        Map.put(map, key, new_value)
 
       _ ->
         map

test/nostrum/cache/message_cache/mnesia_additional_test.exs

@@ -0,0 +1,168 @@
+defmodule Nostrum.Cache.MessageCache.MnesiaAdditionalTest do
+  use ExUnit.Case
+
+  alias Nostrum.Cache.MessageCache.Mnesia, as: MessageCache
+  alias Nostrum.Struct.Message
+
+  @test_message %{
+    id: 1_234_567,
+    channel_id: 7_654_321,
+    author: %{
+      id: 12345,
+      username: "test",
+      avatar: nil,
+      bot: true,
+      mfa_enabled: nil,
+      verified: nil
+    },
+    content: "Hello, world!",
+    timestamp: "1970-01-01T00:00:00Z",
+    edited_timestamp: nil
+  }
+
+  @test_message_two %{
+    id: 7_654_321,
+    channel_id: 1_234_567,
+    author: %{
+      id: 54321,
+      username: "test two",
+      avatar: nil,
+      bot: false,
+      mfa_enabled: nil,
+      verified: nil
+    },
+    content: "Goodbye, world!",
+    timestamp: "2038-01-01T00:00:00Z",
+    edited_timestamp: nil,
+    embeds: [
+      %{
+        title: "Test Embed",
+        description: "This is a test embed",
+        url: "https://example.com",
+        timestamp: "2038-01-01T00:00:00Z",
+        color: 0x00FF00,
+        footer: %{
+          text: "Test Footer"
+        },
+        fields: [
+          %{
+            name: "Test Field",
+            value: "Test Value",
+            inline: false
+          }
+        ]
+      }
+    ]
+  }
+
+  setup do
+    on_exit(:cleanup, fn ->
+      try do
+        MessageCache.teardown()
+      rescue
+        e -> e
+      end
+    end)
+
+    [pid: start_supervised!(MessageCache)]
+  end
+
+  describe "create/1" do
+    test "evicts the messages with the lowest ids when it gets full" do
+      for id <- 1..11, do: MessageCache.create(Map.put(@test_message, :id, id))
+
+      # in tests, the cache is limited to 10 messages
+      # and we evict 4 messages when hitting the limit
+      # so the first 4 messages should be evicted
+
+      for id <- 1..4 do
+        assert MessageCache.get(id) == {:error, :not_found}
+      end
+
+      for id <- 5..11 do
+        assert {:ok, %Message{id: ^id}} = MessageCache.get(id)
+      end
+    end
+  end
+
+  describe "update/1" do
+    test "returns {old_message, updated_message} when the old message is found in the cache" do
+      expected_old_message = MessageCache.create(@test_message_two)
+
+      updated_payload = %{
+        id: @test_message_two.id,
+        content: "Hello, world!",
+        channel_id: @test_message_two.channel_id
+      }
+
+      {old_message, updated_message} = MessageCache.update(updated_payload)
+
+      assert old_message == expected_old_message
+      assert updated_message == %{old_message | content: "Hello, world!"}
+    end
+
+    test "does not save the updated message to the cache it was not there before" do
+      updated_payload = %{
+        id: 10_258_109_258_109_258_125,
+        content: "Hello, world!",
+        channel_id: 10_258_109_258_109_258_125
+      }
+
+      {old_message, updated_message} = MessageCache.update(updated_payload)
+
+      assert updated_message == Message.to_struct(updated_payload)
+      assert old_message == nil
+      assert MessageCache.get(10_258_109_258_109_258_125) == {:error, :not_found}
+    end
+  end
+
+  describe "get/1" do
+    test "returns {:ok, message} when the message is found in the cache" do
+      expected = MessageCache.create(@test_message)
+      assert {:ok, expected} == MessageCache.get(@test_message.id)
+    end
+  end
+
+  describe "delete/2" do
+    test "returns the deleted message when it is found in the cache" do
+      expected_message = MessageCache.create(@test_message)
+      assert expected_message == MessageCache.delete(@test_message.channel_id, @test_message.id)
+    end
+  end
+
+  describe "bulk_delete/2" do
+    test "returns the deleted messages when they are found in the cache" do
+      expected_messages = [
+        MessageCache.create(@test_message),
+        MessageCache.create(%{@test_message_two | channel_id: @test_message.channel_id})
+      ]
+
+      assert expected_messages ==
+               MessageCache.bulk_delete(@test_message.channel_id, [
+                 @test_message.id,
+                 @test_message_two.id
+               ])
+    end
+
+    test "does not include messages not found in the cache in the returned list" do
+      expected_message = MessageCache.create(@test_message)
+
+      assert [expected_message] ==
+               MessageCache.bulk_delete(@test_message.channel_id, [
+                 @test_message.id,
+                 @test_message_two.id
+               ])
+    end
+  end
+
+  describe "channel_delete/1" do
+    test "deletes all messages for the channel" do
+      MessageCache.create(@test_message)
+      MessageCache.create(%{@test_message_two | channel_id: @test_message.channel_id})
+
+      assert :ok == MessageCache.channel_delete(@test_message.channel_id)
+      assert {:error, :not_found} == MessageCache.get(@test_message.id)
+      assert {:error, :not_found} == MessageCache.get(@test_message_two.id)
+    end
+  end
+end