From 34fea670e4ec4614832ccd4f0372e8b0b1a707df Mon Sep 17 00:00:00 2001 From: The Major Date: Sat, 18 May 2024 21:00:15 +0000 Subject: [PATCH] ready for testing? --- config/config.exs | 11 ++ lib/nostrum/cache/message_cache/mnesia.ex | 72 +++++--- lib/nostrum/cache/message_cache/noop.ex | 6 +- lib/nostrum/util.ex | 3 +- .../message_cache/mnesia_additional_test.exs | 168 ++++++++++++++++++ .../nostrum/cache/message_cache_meta_test.exs | 117 ++++++++++++ test/nostrum/shard/dispatch_test.exs | 2 +- 7 files changed, 345 insertions(+), 34 deletions(-) create mode 100644 test/nostrum/cache/message_cache/mnesia_additional_test.exs create mode 100644 test/nostrum/cache/message_cache_meta_test.exs diff --git a/config/config.exs b/config/config.exs index da3677333..13bb8c8f5 100644 --- a/config/config.exs +++ b/config/config.exs @@ -8,3 +8,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 diff --git a/lib/nostrum/cache/message_cache/mnesia.ex b/lib/nostrum/cache/message_cache/mnesia.ex index 4d3509843..98aa66b8d 100644 --- a/lib/nostrum/cache/message_cache/mnesia.ex +++ b/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 diff --git a/lib/nostrum/cache/message_cache/noop.ex b/lib/nostrum/cache/message_cache/noop.ex index 3795b36d0..24a84cb45 100644 --- a/lib/nostrum/cache/message_cache/noop.ex +++ b/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 diff --git a/lib/nostrum/util.ex b/lib/nostrum/util.ex index bd3b52746..d983680f6 100644 --- a/lib/nostrum/util.ex +++ b/lib/nostrum/util.ex @@ -267,7 +267,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 diff --git a/test/nostrum/cache/message_cache/mnesia_additional_test.exs b/test/nostrum/cache/message_cache/mnesia_additional_test.exs new file mode 100644 index 000000000..083485fb5 --- /dev/null +++ b/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 diff --git a/test/nostrum/cache/message_cache_meta_test.exs b/test/nostrum/cache/message_cache_meta_test.exs new file mode 100644 index 000000000..ed882ec17 --- /dev/null +++ b/test/nostrum/cache/message_cache_meta_test.exs @@ -0,0 +1,117 @@ +defmodule Nostrum.Cache.MessageCacheMetaTest do + alias Nostrum.Cache.MessageCache + use ExUnit.Case, async: true + + @cache_modules [ + # Dispatcher + MessageCache, + # Implementation + MessageCache.Mnesia, + MessageCache.Noop + ] + + for cache <- @cache_modules do + defmodule :"#{cache}Test" do + alias Nostrum.Struct.Message + use ExUnit.Case + + # this is needed because otherwise we cannot access + # the cache in the tests + @cache cache + + @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: 0, + color: 0x00FF00, + footer: %{ + text: "Test Footer" + }, + fields: [ + %{ + name: "Test Field", + value: "Test Value", + inline: false + } + ] + } + ] + } + + setup do + on_exit(:cleanup, fn -> + try do + if function_exported?(@cache, :teardown, 0) do + apply(@cache, :teardown, []) + end + rescue + e -> e + end + end) + + [pid: start_supervised!(@cache)] + end + + describe "create/1" do + test "returns a struct of the created message" do + expected_one = Message.to_struct(@test_message) + assert expected_one == @cache.create(@test_message) + + expected_two = Message.to_struct(@test_message_two) + assert expected_two == @cache.create(@test_message_two) + end + end + + describe "update/1" do + test "returns {nil, updated_message} on and uncached message" do + expected = Message.to_struct(@test_message) + assert {nil, expected} == @cache.update(@test_message) + end + end + + describe "get/1" do + test "returns {:error, :not_found} on an uncached message" do + assert {:error, :not_found} == @cache.get(10_258_109_258_109_258_125) + end + end + + describe "delete/2" do + test "returns :noop on an uncached message" do + assert :noop == @cache.delete(10_258_109_258_109_258_125, 10_258_109_258_109_258_125) + end + end + end + end +end diff --git a/test/nostrum/shard/dispatch_test.exs b/test/nostrum/shard/dispatch_test.exs index 0b1ddf509..cb6499dc6 100644 --- a/test/nostrum/shard/dispatch_test.exs +++ b/test/nostrum/shard/dispatch_test.exs @@ -25,7 +25,7 @@ defmodule Nostrum.Shard.DispatchTest do {key, event, _} = Dispatch.handle_event(:MESSAGE_DELETE_BULK, payload, %{}) assert(^key = :MESSAGE_DELETE_BULK) - assert(^event = struct(MessageDeleteBulk, payload)) + assert(^event = struct(MessageDeleteBulk, Map.put(payload, :deleted_messages, []))) end end end