From 304a8d4164b210275bf85adf1bae1fcfc7fc5459 Mon Sep 17 00:00:00 2001 From: Alek Ratzloff Date: Sat, 13 Jun 2020 17:13:05 -0400 Subject: [PATCH] WIP: Move Config.all_channels/0 and Config.channel_modules/1 logic into State; add State.add_loaded_module/2 The problem: When we're going through the list of modules to send messages to based on the channels they're a part of, it was being done so through the config. Since the config doesn't (and shouldn't) list all of the core modules that get included, any core modules that were loaded and running under the ModuleSupervisor would not get included in the router's attempt to send messages to a module. Now, the Config.all_channels and Config.channel_modules functions live in State, and State has a new "add_loaded_module" function where loaded modules are registered. The aforementioned moved functions will use this as the "source of truth" when deciding where to send messages for modules to handle. Signed-off-by: Alek Ratzloff --- lib/config.ex | 11 ------- lib/contrib/fortune.ex | 4 +-- lib/core/welcome.ex | 11 +++++++ lib/irc.ex | 5 ---- lib/module.ex | 41 ++++++++++++------------- lib/module_supervisor.ex | 8 +++++ lib/router.ex | 13 ++++---- lib/state.ex | 58 +++++++++++++++++++++++++++++++++-- lib/supervisor.ex | 2 +- test/config_test.exs | 58 ----------------------------------- test/irc/msg_test.exs | 2 +- test/state_test.exs | 65 ++++++++++++++++++++++++++++++++++++++++ 12 files changed, 172 insertions(+), 106 deletions(-) create mode 100644 lib/core/welcome.ex delete mode 100644 test/config_test.exs create mode 100644 test/state_test.exs diff --git a/lib/config.ex b/lib/config.ex index 91faf7e..f4d1682 100644 --- a/lib/config.ex +++ b/lib/config.ex @@ -25,17 +25,6 @@ defmodule Omnibot.Config do |> MapSet.to_list() end - @doc ~S""" - Gets a list of all `{module, mod_cfg}` pairs from the given configuration - that are listening to the given channel. - """ - def channel_modules(cfg, channel) do - cfg.modules - |> Enum.filter(fn {_, cfg} -> - cfg[:channels] == :all or Enum.member?(cfg[:channels] || [], channel) - end) - end - def msg_prefix(cfg) do %Msg.Prefix { nick: cfg.nick, diff --git a/lib/contrib/fortune.ex b/lib/contrib/fortune.ex index 2a8b107..c66118b 100644 --- a/lib/contrib/fortune.ex +++ b/lib/contrib/fortune.ex @@ -20,12 +20,12 @@ defmodule Omnibot.Contrib.Fortune do command "!fortune", [to] do fortune = Enum.random(@fortunes) reply = "#{to}: #{fortune}" - Irc.send_to(channel, reply) + Irc.send_to(irc, channel, reply) end command "!fortune" do fortune = Enum.random(@fortunes) reply = "#{nick}: #{fortune}" - Irc.send_to(channel, reply) + Irc.send_to(irc, channel, reply) end end diff --git a/lib/core/welcome.ex b/lib/core/welcome.ex new file mode 100644 index 0000000..383536b --- /dev/null +++ b/lib/core/welcome.ex @@ -0,0 +1,11 @@ +defmodule Omnibot.Core.Welcome do + use Omnibot.Module + + require Logger + + @impl true + def on_msg(irc, _) do + Logger.info("Syncing channels") + Irc.sync_channels(irc) + end +end diff --git a/lib/irc.ex b/lib/irc.ex index b6b7ec6..404da00 100644 --- a/lib/irc.ex +++ b/lib/irc.ex @@ -22,16 +22,12 @@ defmodule Omnibot.Irc do def send_msg(irc, command, param), do: send_msg(irc, command, [param]) - def send_to(channel, text), do: send_to(__MODULE__, channel, text) def send_to(irc, channel, text), do: send_msg(irc, "PRIVMSG", [channel, text]) - def join(channel), do: join(__MODULE__, channel) def join(irc, channel), do: send_msg(irc, "JOIN", channel) - def part(channel), do: part(__MODULE__, channel) def part(irc, channel), do: send_msg(irc, "PART", channel) - def sync_channels(), do: sync_channels(__MODULE__) def sync_channels(irc), do: GenServer.cast(irc, :sync_channels) ## Server callbacks @@ -45,7 +41,6 @@ defmodule Omnibot.Irc do :gen_tcp.connect(to_charlist(cfg.server), cfg.port, [:binary, active: false, packet: :line]) # Wait for first message - #{:ok, _} = :gen_tcp.recv(socket, 0) send_msg(self(), "NICK", cfg.nick) send_msg(self(), "USER", [cfg.user, "0", "*", cfg.real]) :inet.setopts(socket, [active: true]) diff --git a/lib/module.ex b/lib/module.ex index 54dbf9c..c72db29 100644 --- a/lib/module.ex +++ b/lib/module.ex @@ -3,19 +3,19 @@ defmodule Omnibot.Module do defmacro __before_compile__(_env) do quote generated: true do @impl true - def on_channel_msg(_channel, _nick, _line), do: nil + def on_channel_msg(_irc, _channel, _nick, _line), do: nil @impl true - def on_channel_msg(_channel, _nick, _cmd, _params), do: nil + def on_channel_msg(_irc, _channel, _nick, _cmd, _params), do: nil @impl true - def on_join(_channel, _nick), do: nil + def on_join(_irc, _channel, _nick), do: nil @impl true - def on_part(_channel, _nick), do: nil + def on_part(_irc, _channel, _nick), do: nil @impl true - def on_kick(_channel, _nick), do: nil + def on_kick(_irc, _channel, _nick), do: nil end end end @@ -38,13 +38,13 @@ defmodule Omnibot.Module do end @impl Module - def on_msg(msg) do + def on_msg(irc, msg) do # TODO - instead of using a router for modules, consider using a PubSub with a Registry: # https://hexdocs.pm/elixir/master/Registry.html#module-using-as-a-pubsub - route_msg(msg) + route_msg(irc, msg) end - defp route_msg(msg) do + defp route_msg(irc, msg) do nick = msg.prefix.nick case String.upcase(msg.command) do @@ -53,21 +53,21 @@ defmodule Omnibot.Module do line = Enum.join(params, " ") case String.split(line, " ") do - [cmd | params] -> on_channel_msg(channel, nick, cmd, params) - _ -> on_channel_msg(channel, nick, line) + [cmd | params] -> on_channel_msg(irc, channel, nick, cmd, params) + _ -> on_channel_msg(irc, channel, nick, line) end "JOIN" -> [channel | _] = msg.params - on_join(channel, nick) + on_join(irc, channel, nick) "PART" -> [channel | _] = msg.params - on_part(channel, nick) + on_part(irc, channel, nick) "KICK" -> [channel | _] = msg.params - on_kick(channel, nick) + on_kick(irc, channel, nick) _ -> nil @@ -80,22 +80,23 @@ defmodule Omnibot.Module do end end - @callback on_msg(msg :: %Omnibot.Irc.Msg{}) :: any - @callback on_channel_msg(channel :: String.t(), nick :: String.t(), line :: String.t()) :: any + @callback on_msg(irc :: pid(), msg :: %Omnibot.Irc.Msg{}) :: any + @callback on_channel_msg(irc :: pid(), channel :: String.t(), nick :: String.t(), line :: String.t()) :: any @callback on_channel_msg( + irc :: pid(), channel :: String.t(), nick :: String.t(), cmd :: String.t(), params :: [String.t()] ) :: any - @callback on_join(channel :: String.t(), nick :: String.t()) :: any - @callback on_part(channel :: String.t(), nick :: String.t()) :: any - @callback on_kick(channel :: String.t(), nick :: String.t()) :: any + @callback on_join(irc :: pid(), channel :: String.t(), nick :: String.t()) :: any + @callback on_part(irc :: pid(), channel :: String.t(), nick :: String.t()) :: any + @callback on_kick(irc :: pid(), channel :: String.t(), nick :: String.t()) :: any defmacro command(cmd, opts) do quote generated: true do @impl Omnibot.Module - def on_channel_msg(var!(channel), var!(nick), unquote(cmd), var!(params)) do + def on_channel_msg(var!(irc), var!(channel), var!(nick), unquote(cmd), var!(params)) do unquote(opts[:do]) end end @@ -115,7 +116,7 @@ defmodule Omnibot.Module do quote generated: true do @impl Omnibot.Module - def on_channel_msg(var!(channel), var!(nick), unquote(cmd), unquote(params)) do + def on_channel_msg(var!(irc), var!(channel), var!(nick), unquote(cmd), unquote(params)) do unquote(opts[:do]) end end diff --git a/lib/module_supervisor.ex b/lib/module_supervisor.ex index ca508b1..7258104 100644 --- a/lib/module_supervisor.ex +++ b/lib/module_supervisor.ex @@ -3,6 +3,7 @@ defmodule Omnibot.ModuleSupervisor do use Supervisor require Logger + alias Omnibot.State def start_link(opts \\ []) do Supervisor.start_link(__MODULE__, opts[:cfg], opts) @@ -12,6 +13,10 @@ defmodule Omnibot.ModuleSupervisor do def init(cfg) do compile_files(cfg.module_paths || []) + # These are modules that need to be loaded for core functionality of the bot + #{Omnibot.Core.Welcome, cfg: [channels: :all]}, + #{Omnibot.Core.Join, cfg: [channels: :all]}, + # Map the modules in the configuration to the children children = for mod <- cfg.modules do @@ -21,6 +26,9 @@ defmodule Omnibot.ModuleSupervisor do end end + # Add each child to the "loaded modules" list in the State + Enum.each(children, fn module -> State.add_loaded_module(module) end) + Supervisor.init(children, strategy: :one_for_one) end diff --git a/lib/router.ex b/lib/router.ex index 599ed4a..f148882 100644 --- a/lib/router.ex +++ b/lib/router.ex @@ -1,12 +1,13 @@ defmodule Omnibot.Router do require Logger - alias Omnibot.{Config, Irc, Irc.Msg, State} + alias Omnibot.{Irc.Msg, State} - def route(_irc, msg) do - channel = Msg.channel(msg) - State.cfg() - |> Config.channel_modules(channel) - |> Enum.each(fn {module, _} -> module.on_msg(msg) end) + def route(irc, msg) do + #channel = Msg.channel(msg) + channel = IO.inspect(Msg.channel(msg)) + IO.inspect(State.channel_modules(channel)) + State.channel_modules(channel) + |> Enum.each(fn {module, _} -> module.on_msg(irc, msg) end) end #def handle(_irc, :privmsg, msg) do diff --git a/lib/state.ex b/lib/state.ex index dfec1ab..361af6b 100644 --- a/lib/state.ex +++ b/lib/state.ex @@ -2,7 +2,7 @@ defmodule Omnibot.State do use GenServer @enforce_keys [:cfg] - defstruct [:cfg, channels: MapSet.new()] + defstruct [:cfg, channels: MapSet.new(), module_map: %{}] ## Client API @@ -21,6 +21,21 @@ defmodule Omnibot.State do GenServer.call(state, :cfg) end + @doc "Adds a loaded module to the default state." + def add_loaded_module(module), do: add_loaded_module(__MODULE__, module) + + @doc "Adds a loaded module to the given state." + def add_loaded_module(state, {module, cfg}), do: GenServer.cast(state, {:add_loaded_module, {module, cfg}}) + + @doc "Adds a loaded module to the given state." + def add_loaded_module(state, module), do: add_loaded_module(state, {module, []}) + + @doc "Gets all loaded modules from the default state." + def loaded_modules(), do: loaded_modules(__MODULE__) + + @doc "Gets all loaded modules from the given state." + def loaded_modules(state), do: GenServer.call(state, :loaded_modules) + @doc "Gets all channels that the bot is present in from the default State process." def channels(), do: channels(__MODULE__) @@ -45,6 +60,34 @@ defmodule Omnibot.State do GenServer.cast(state, {:remove_channel, channel}) end + def all_channels(), do: all_channels(__MODULE__) + + def all_channels(state) do + loaded_modules(state) |> Enum.flat_map( + fn {_, cfg} -> + case cfg[:channels] do + :all -> [] + nil -> [] + channels -> channels + end + end) + |> MapSet.new() + |> MapSet.to_list() + end + + def channel_modules(channel), do: channel_modules(__MODULE__, channel) + + @doc ~S""" + Gets a list of all `{module, mod_cfg}` from the given State that are both + loaded, and listening to the given channel. + """ + def channel_modules(state, channel) do + loaded_modules(state) |> Enum.filter( + fn {_, cfg} -> + cfg[:channels] == :all or Enum.member?(cfg[:channels] || [], channel) + end) + end + ## Server API @impl true @@ -61,7 +104,18 @@ defmodule Omnibot.State do def handle_call(:channels, _from, state) do {:reply, state.channels, state} end - + + @impl true + def handle_call(:loaded_modules, _from, state) do + {:reply, state.module_map, state} + end + + @impl true + def handle_cast({:add_loaded_module, {module, cfg}}, state) do + state = %{state | module_map: Map.put(state.module_map, module, cfg)} + {:noreply, state} + end + @impl true def handle_cast({:add_channel, channel}, state) do {:noreply, %{state | channels: state.channels |> MapSet.put(channel)}} diff --git a/lib/supervisor.ex b/lib/supervisor.ex index ce80002..3db8bca 100644 --- a/lib/supervisor.ex +++ b/lib/supervisor.ex @@ -15,8 +15,8 @@ defmodule Omnibot.Supervisor do children = [ {Task.Supervisor, name: Omnibot.RouterSupervisor, strategy: :one_for_one}, {Omnibot.State, cfg: cfg, name: Omnibot.State}, + {Omnibot.ModuleSupervisor, cfg: cfg, name: Omnibot.ModuleSupervisor}, {Omnibot.Irc, name: Omnibot.Irc}, - {Omnibot.ModuleSupervisor, cfg: cfg, name: Omnibot.ModuleSupervisor} ] # TODO : how to handle config reloading? diff --git a/test/config_test.exs b/test/config_test.exs deleted file mode 100644 index d467a86..0000000 --- a/test/config_test.exs +++ /dev/null @@ -1,58 +0,0 @@ -defmodule ConfigTest do - use ExUnit.Case - - alias Omnibot.Config - - test "config all_channels works correctly" do - cfg = %Config { - server: "test", - modules: [ - {Test, channels: ["#foo", "#bar"]}, - {Test, channels: ["#foo"]}, - {Test, channels: ["#bar"]}, - {Test, channels: ["#baz"]}, - {Test, channels: :all}, - ] - } - - channels = Config.all_channels(cfg) - - assert length(channels) == 3 - assert Enum.member?(channels, "#foo") - assert Enum.member?(channels, "#bar") - assert Enum.member?(channels, "#baz") - end - - test "config channel_modules works correctly" do - cfg = %Config { - server: "test", - modules: [ - {FooBar, channels: ["#foo", "#bar"]}, - {Foo, channels: ["#foo"]}, - {Bar, channels: ["#bar"]}, - {Baz, channels: ["#baz"]}, - {All, channels: :all}, - ] - } - - modules = Config.channel_modules(cfg, "#foo") - |> Enum.map(fn {module, _} -> module end) - assert length(modules) == 3 - assert Enum.member?(modules, FooBar) - assert Enum.member?(modules, Foo) - assert Enum.member?(modules, All) - - modules = Config.channel_modules(cfg, "#bar") - |> Enum.map(fn {module, _} -> module end) - assert length(modules) == 3 - assert Enum.member?(modules, FooBar) - assert Enum.member?(modules, Bar) - assert Enum.member?(modules, All) - - modules = Config.channel_modules(cfg, "#baz") - |> Enum.map(fn {module, _} -> module end) - assert length(modules) == 2 - assert Enum.member?(modules, Baz) - assert Enum.member?(modules, All) - end -end diff --git a/test/irc/msg_test.exs b/test/irc/msg_test.exs index 849cca1..a758a77 100644 --- a/test/irc/msg_test.exs +++ b/test/irc/msg_test.exs @@ -1,4 +1,4 @@ -alias Omnibot.{Irc, Irc.Msg} +alias Omnibot.Irc.Msg defmodule Omnibot.MsgTest do diff --git a/test/state_test.exs b/test/state_test.exs new file mode 100644 index 0000000..e92094b --- /dev/null +++ b/test/state_test.exs @@ -0,0 +1,65 @@ +defmodule StateTest do + use ExUnit.Case + + alias Omnibot.State + + setup do + state = start_supervised!(State) + {:ok, state: state} + end + + test "state channel_modules works correctly", %{state: state} do + modules = [ + {FooBar, channels: ["#foo", "#bar"]}, + {Foo, channels: ["#foo"]}, + {Bar, channels: ["#bar"]}, + {Baz, channels: ["#baz"]}, + {All, channels: :all}, + ] + + modules |> Enum.each(fn module -> State.add_loaded_module(state, module) end) + + modules = State.channel_modules(state, "#foo") + |> Enum.map(fn {module, _} -> module end) + assert length(modules) == 3 + assert Enum.member?(modules, FooBar) + assert Enum.member?(modules, Foo) + assert Enum.member?(modules, All) + + modules = State.channel_modules(state, "#bar") + |> Enum.map(fn {module, _} -> module end) + assert length(modules) == 3 + assert Enum.member?(modules, FooBar) + assert Enum.member?(modules, Bar) + assert Enum.member?(modules, All) + + modules = State.channel_modules(state, "#baz") + |> Enum.map(fn {module, _} -> module end) + assert length(modules) == 2 + assert Enum.member?(modules, Baz) + assert Enum.member?(modules, All) + + modules = State.channel_modules(state, nil) + |> Enum.map(fn {module, _} -> module end) + assert length(modules) == 1 + assert Enum.member?(modules, All) + end + + test "state all_channels works correctly", %{state: state} do + modules = [ + {FooBar, channels: ["#foo", "#bar"]}, + {Foo, channels: ["#foo"]}, + {Bar, channels: ["#bar"]}, + {Baz, channels: ["#baz"]}, + {All, channels: :all}, + ] + + modules |> Enum.each(fn module -> State.add_loaded_module(state, module) end) + channels = State.all_channels(state) + + assert length(channels) == 3 + assert Enum.member?(channels, "#foo") + assert Enum.member?(channels, "#bar") + assert Enum.member?(channels, "#baz") + end +end