diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 798f7cc..bedfc60 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -61,3 +61,15 @@ jobs: run: mix compile --warnings-as-errors - name: Run Tests run: mix test + + format: + runs-on: ubuntu-24.04 + name: Format + steps: + - uses: actions/checkout@v4 + - uses: erlef/setup-beam@v1 + with: + otp-version: "28" + elixir-version: "1.20" + - name: Check formatting + run: mix format --check-formatted diff --git a/CHANGELOG.md b/CHANGELOG.md index ea519aa..8e3ee13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,22 @@ - **[BREAKING]** Updated the minimum required version of Elixir to `1.16` - Fix type warnings emitted by `use Storex.Store` on Elixir 1.18 and newer +- **[BREAKING]** A `FunctionClauseError` raised inside a mutation that *did* match is no longer reported to the client as `No mutation matching ...`. Only the store's own `mutation/5` failing to match produces that error; anything else propagates with its original stacktrace, which stops the store process and the socket with it +- `terminate/3` is no longer skipped when the store module happens not to be loaded yet. `Storex.Store.__terminate__/4` gated the call on `function_exported?/3`, which answers `false` for an unloaded module, so whether the callback ran depended on the code server rather than on the store +- Housekeeping: dropped the unused `import Supervisor.Spec, warn: false` from `Storex.start/2` (deprecated since Elixir 1.5, and the `warn: false` was hiding it) and the `registry: Storex.Registry.ETS` key from `config/config.exs` (that module was removed in 0.4.0 and nothing read the key). The project is now formatted, and CI checks it +- `Storex.Registry` reads (`get_store/2`, `get_store_pid/2`, `get_store_instances/1`, `session_stores/1`) now run in the calling process against the `:protected` ETS table instead of a `GenServer.call`. The registry process was a global serialisation point — every mutation performs at least one lookup — and the cost grew with the number of connections. Measured at 500 lookups per reader: 98.3ms against 16.2ms with 64 concurrent readers, 335.8ms against 56.2ms with 256. Writes and the `:DOWN` cleanup still go through the process +- **[BREAKING]** `Storex.mutate/3` and `Storex.mutate/4` return `:ok`. They used to return whatever the comprehension in `Storex.PG.broadcast/1` produced, which was the internal broadcast envelope repeated once per node (`[broadcast: {:mutate, "Store", "reload", []}]`) +- Removed the unreachable `{:error, _}` branch in `Storex.PG.broadcast/1`. `:pg.get_members/2` always returns a list; the error tuple was `:pg2`'s contract, and `:pg2` support went away in this release +- The state a store reports on join is now read by asking the store process (`handle_call(:get_state, ...)` on the generated `Server`) instead of `:sys.get_state/1`, a debug function that was reaching into the process's internal state shape from the outside on every join. If the process is gone by the time the join reads it, the client now gets an error frame instead of the connection exiting +- A client could kill its connection process with a well-formed `error` frame: `Storex.Message.cast/1` accepted the shape but `Storex.Socket.message_handle/2` had no clause for it. `error` frames only travel server to client, so the shape is no longer accepted and the frame is refused with `1007` like any other unknown type +- `Storex.Handler.Plug` closed a malformed payload with a bare `1007`, passing the reason as the process exit reason instead of the close payload. It now sends `1007` with the reason, matching `Storex.Handler.Cowboy` +- Removed dead code: `Storex.Registry.session_pid/1` (no callers, and its `:ets.match/2` pattern was a 4-tuple against 5-tuple records, so it never matched anything) and `Storex.Handler.Cowboy.websocket_init/3` (a cowboy 1.x callback, unreachable on cowboy 2.x) +- **[SECURITY]** The SSR/HTTP path now runs the same store-name checks as the WebSocket path. `Storex.HTTP` carried its own copy of the resolution logic with only `Module.safe_concat/1`, missing `Code.ensure_compiled/1` and the `Storex.Store` behaviour check, so `GET /storex?store=Any.Module¶ms=%7B%7D` called `init/2` on any loaded module whose name resolved and serialised the result back to the caller. Both transports now share `Storex.Store.resolve/1` +- **[BREAKING]** **[SECURITY]** `Storex.Handler.Cowboy` no longer accepts `:binary` frames. It decoded them with `:erlang.binary_to_term/1`, which creates atoms out of bytes the client controls — a single 25-byte frame is enough — and passed the resulting term straight to `Storex.Socket.message_handle/2`, bypassing the `Storex.Message.cast/1` allowlist every other entry point goes through. Binary frames are now closed with `1003` +- `Storex.Handler.Plug` had no clause for `:binary` frames at all, so one raised `FunctionClauseError` and closed the connection with `1011`. It now closes with `1003` like the cowboy handler +- **[SECURITY]** A `mutation` frame is now resolved against the session the server assigned to the connection, not the `session` field carried by the frame. Any client could previously mutate — and read the resulting diff of — any other session's store by naming its session id. Mutating other sessions on purpose is what `Storex.mutate/3` and `Storex.mutate/4` are for +- A `mutation` for a store the session has not joined now returns an error to the client instead of exiting the connection process +- Store processes are now registered through a `Registry` keyed by `{session, store}` instead of being named `:"#{session}_#{store}"`. Session ids are unique per connection, so the old naming created one permanent atom per session-store pair and could exhaust the atom table on a long-running node - Removed the `:pg2` fallback, unreachable since OTP 24 - Updated dependencies diff --git a/config/config.exs b/config/config.exs index 990dd39..1af521d 100644 --- a/config/config.exs +++ b/config/config.exs @@ -1,8 +1,6 @@ import Config -config :storex, - session_id_library: Nanoid, - registry: Storex.Registry.ETS +config :storex, session_id_library: Nanoid if Mix.env() == :test do import_config "test.exs" diff --git a/lib/storex.ex b/lib/storex.ex index 724c3b0..f981f92 100644 --- a/lib/storex.ex +++ b/lib/storex.ex @@ -3,12 +3,11 @@ defmodule Storex do @doc false def start(_type, _args) do - import Supervisor.Spec, warn: false - children = [ %{id: :pg, start: {:pg, :start_link, [Storex.PG]}}, {Storex.PG, []}, {Storex.Registry, []}, + {Registry, keys: :unique, name: Storex.StoreRegistry}, {Storex.Supervisor, []} ] diff --git a/lib/storex/handler/cowboy.ex b/lib/storex/handler/cowboy.ex index ce37da0..f9e36f3 100644 --- a/lib/storex/handler/cowboy.ex +++ b/lib/storex/handler/cowboy.ex @@ -9,10 +9,6 @@ defmodule Storex.Handler.Cowboy do {:cowboy_websocket, request, %{session: session, pid: request.pid}} end - def websocket_init(_type, req, _opts) do - {:ok, req, %{status: "inactive"}} - end - def terminate(_reason, _req, %{session: session}) do Storex.Registry.session_stores(session) |> Enum.each(fn {store, _, session, _, _} -> @@ -26,14 +22,13 @@ defmodule Storex.Handler.Cowboy do :ok end - def websocket_handle({:binary, frame}, state) do - try do - :erlang.binary_to_term(frame) - |> Socket.message_handle(state) - |> map_response() - rescue - ArgumentError -> {:reply, {:close, 1007, "Payload is malformed."}, state} - end + # Binary frames are not part of the protocol: the client only ever sends text. + # They used to be decoded with `:erlang.binary_to_term/1`, which creates atoms + # out of bytes the client controls and handed the resulting term straight to + # `Storex.Socket.message_handle/2`, bypassing the `Storex.Message.cast/1` + # allowlist that every other entry point goes through. + def websocket_handle({:binary, _frame}, state) do + {:reply, {:close, 1003, "Binary frames are not supported."}, state} end def websocket_handle({:text, frame}, state) do diff --git a/lib/storex/handler/plug.ex b/lib/storex/handler/plug.ex index 2a71293..45c8de9 100644 --- a/lib/storex/handler/plug.ex +++ b/lib/storex/handler/plug.ex @@ -30,10 +30,16 @@ defmodule Storex.Handler.Plug do |> map_response() else {:error, _} -> - {:stop, "Payload is malformed.", 1007, state} + {:stop, :normal, {1007, "Payload is malformed."}, state} end end + # See the note in `Storex.Handler.Cowboy`. Without this clause a binary frame + # raises `FunctionClauseError` here and takes the connection down with a 1011. + def handle_in({_message, [opcode: :binary]}, state) do + {:stop, :normal, {1003, "Binary frames are not supported."}, state} + end + def handle_info({:mutate, store, mutation, data}, %{session: session} = state) do %{ type: "mutation", diff --git a/lib/storex/http.ex b/lib/storex/http.ex index 4ce594e..6772f0d 100644 --- a/lib/storex/http.ex +++ b/lib/storex/http.ex @@ -1,6 +1,6 @@ defmodule Storex.HTTP do def init_store(store, params) do - with {:store, {:ok, store_module}} <- {:store, store |> get_module()}, + with {:store, {:ok, store_module}} <- {:store, Storex.Store.resolve(store)}, {:params, {:ok, params}} <- {:params, params |> get_params()}, {:state, {:ok, result}} <- {:state, get_state(store_module, params)} do {:ok, @@ -17,7 +17,7 @@ defmodule Storex.HTTP do type: "error", session: "SSR", store: store, - error: "Store '#{inspect(store)}' is not defined or can't be compiled." + error: "Store '#{store}' is not defined or can't be compiled." }} {:state, {:error, message}} -> @@ -40,35 +40,20 @@ defmodule Storex.HTTP do end end - defp get_module(store) do - try do - module = Module.safe_concat([store]) - {:ok, module} - rescue - ArgumentError -> {:error, :not_exists} - end - end - defp get_params(params) do params |> Jason.decode() end + # The SSR path is init-only, so the key a store may return is dropped. Using + # the shared dispatcher keeps the accepted return values, and the error raised + # for anything else, identical to the websocket path. defp get_state(module, params) do module - |> apply(:init, ["SSR", params]) - |> result() - end - - defp result({:ok, state}) do - {:ok, state} - end - - defp result({:ok, state, _}) do - {:ok, state} - end - - defp result({:error, error_message}) do - {:error, error_message} + |> Storex.Store.__init__("SSR", params) + |> case do + {:ok, state, _key} -> {:ok, state} + {:error, reason} -> {:error, reason} + end end end diff --git a/lib/storex/message.ex b/lib/storex/message.ex index fc77fe2..be60485 100644 --- a/lib/storex/message.ex +++ b/lib/storex/message.ex @@ -37,17 +37,10 @@ defmodule Storex.Message do }} end - def cast(%{ - "type" => "error", - "store" => store, - "data" => data, - "request" => request, - "session" => session - }) do - {:ok, - %__MODULE__{type: "error", store: store, data: data, request: request, session: session}} - end - + # `error` frames only ever travel server to client, and are built as plain maps + # in `Storex.Socket`. Casting one here made it past the allowlist and then hit + # `Storex.Socket.message_handle/2`, which has no clause for it, so a client + # could kill its connection process with a well-formed frame. def cast(_) do {:error, "Unknown message type"} end diff --git a/lib/storex/pg.ex b/lib/storex/pg.ex index b87722b..99da40d 100644 --- a/lib/storex/pg.ex +++ b/lib/storex/pg.ex @@ -14,17 +14,15 @@ defmodule Storex.PG do {:ok, @name} end + # `:pg.get_members/2` always returns a list — the `{:error, _}` clause this used + # to carry was `:pg2`'s contract, and `:pg2` is gone. `Enum.each/2` rather than + # a comprehension so the return value is `:ok`: `send/2` returns the message, + # so `Storex.mutate/3` used to hand back the internal broadcast envelope once + # per node. def broadcast(payload) do - :pg.get_members(Storex.PG, @name) - |> case do - {:error, _} -> - :error - - pids -> - for pid <- pids do - send(pid, {:broadcast, payload}) - end - end + Storex.PG + |> :pg.get_members(@name) + |> Enum.each(&send(&1, {:broadcast, payload})) end @impl true diff --git a/lib/storex/registry.ex b/lib/storex/registry.ex index 60e5e87..0e3bd7f 100644 --- a/lib/storex/registry.ex +++ b/lib/storex/registry.ex @@ -15,10 +15,6 @@ defmodule Storex.Registry do {:ok, %{}} end - def session_pid(session) do - GenServer.call(@registry, {:session_pid, session}) - end - def register_store(store, store_pid, session, session_pid, key) do GenServer.call(@registry, {:register_store, store, store_pid, session, session_pid, key}) end @@ -27,28 +23,35 @@ defmodule Storex.Registry do GenServer.call(@registry, {:unregister_store, store, session}) end + # The table is `:protected`: only the owning process writes, but every process + # reads. So reads run in the caller. Routing them through this GenServer made + # it a global serialisation point for the whole library — every mutation does + # at least one lookup — and the cost scales with the number of connections. + # Measured, 500 lookups per reader: with 64 concurrent readers, 98.3ms through + # the GenServer against 16.2ms reading directly; with 256, 335.8ms against + # 56.2ms. Writes and the `:DOWN` cleanup stay in the process. def get_store(store, session) do - GenServer.call(@registry, {:get_store, store, session}) + :ets.match_object(@registry, {store, :"$1", session, :_, :_}) + |> case do + [] -> :undefined + [object | _tail] -> object + end end def get_store_pid(store, session) do - GenServer.call(@registry, {:get_store_pid, store, session}) + :ets.match(@registry, {store, :"$1", session, :_, :_}) + |> case do + [] -> :undefined + [[pid] | _tail] -> pid + end end def get_store_instances(query) do - GenServer.call(@registry, {:get_store_instances, query}) + :ets.match_object(@registry, query) end def session_stores(session) do - GenServer.call(@registry, {:session_stores, session}) - end - - def handle_call({:session_pid, session}, _from, state) do - :ets.match(@registry, {:_, session, :_, :"$1"}) - |> case do - [] -> {:reply, :undefined, state} - [[pid] | _tail] -> {:reply, pid, state} - end + :ets.match_object(@registry, {:_, :_, session, :_, :_}) end def handle_call({:register_store, store, store_pid, session, session_pid, key}, _from, state) do @@ -62,37 +65,9 @@ defmodule Storex.Registry do {:reply, result, state} end - def handle_call({:get_store, store, session}, _from, state) do - :ets.match_object(@registry, {store, :"$1", session, :_, :_}) - |> case do - [] -> {:reply, :undefined, state} - [object | _tail] -> {:reply, object, state} - end - end - - def handle_call({:get_store_pid, store, session}, _from, state) do - :ets.match(@registry, {store, :"$1", session, :_, :_}) - |> case do - [] -> {:reply, :undefined, state} - [[pid] | _tail] -> {:reply, pid, state} - end - end - - def handle_call({:get_store_instances, query}, _from, state) do - instances = :ets.match_object(@registry, query) - - {:reply, instances, state} - end - - def handle_call({:session_stores, session}, _from, state) do - stores = :ets.match_object(@registry, {:_, :_, session, :_, :_}) - - {:reply, stores, state} - end - - def handle_info({:DOWN, _ref, :process, pid, _reason}, _state) do + def handle_info({:DOWN, _ref, :process, pid, _reason}, state) do :ets.match_delete(@registry, {:_, pid, :_, :_, :_}) - {:noreply, :ok} + {:noreply, state} end end diff --git a/lib/storex/socket.ex b/lib/storex/socket.ex index de0b635..66823a5 100644 --- a/lib/storex/socket.ex +++ b/lib/storex/socket.ex @@ -21,12 +21,12 @@ defmodule Storex.Socket do end def message_handle(%{type: "join"} = message, state) do - with {:get_module, {:ok, _}} <- {:get_module, get_store_module(message.store)}, + with {:get_module, {:ok, _}} <- {:get_module, Storex.Store.resolve(message.store)}, {:add_store, {:ok, _}} <- {:add_store, - Storex.Supervisor.add_store(message.store, state.session, state.pid, message.data)} do - store_state = Storex.Supervisor.get_store_state(state.session, message.store) - + Storex.Supervisor.add_store(message.store, state.session, state.pid, message.data)}, + {:store_state, {:ok, store_state}} <- + {:store_state, Storex.Supervisor.get_store_state(state.session, message.store)} do message = Map.put(message, :data, store_state) |> Map.put(:session, state.session) @@ -34,7 +34,7 @@ defmodule Storex.Socket do {:text, message, state} else - {:add_store, {:error, error_message}} -> + {step, {:error, error_message}} when step in [:add_store, :store_state] -> %{ type: "error", session: state.session, @@ -50,10 +50,15 @@ defmodule Storex.Socket do end end - def message_handle(%{type: "mutation", session: session, store: store} = message, state) do + # The `session` carried by the frame is never used to resolve the store. It is + # supplied by the client, so trusting it would let any socket mutate any other + # session's store. Only `state.session`, generated by the handler for this + # connection, addresses a store. Mutating another session deliberately is what + # `Storex.mutate/3` and `Storex.mutate/4` are for. + def message_handle(%{type: "mutation", store: store} = message, %{session: session} = state) do Storex.Supervisor.mutate_store( - message.session, - message.store, + session, + store, message.data.name, message.data.data ) @@ -89,27 +94,4 @@ defmodule Storex.Socket do |> Jason.encode!() |> (&{:text, &1, state}).() end - - defp safe_concat(store) do - try do - module = Module.safe_concat([store]) - {:ok, module} - rescue - ArgumentError -> {:error, :not_exists} - end - end - - defp get_store_module(store) do - with {:ok, module} <- safe_concat(store), - {:module, module} <- Code.ensure_compiled(module), - true <- - Storex.Store in (module.module_info(:attributes) - |> Keyword.get_values(:behaviour) - |> List.flatten()) do - {:ok, module} - else - false -> {:error, :not_store} - _ -> {:error, :not_exists} - end - end end diff --git a/lib/storex/store.ex b/lib/storex/store.ex index 4cb765d..9f53bf3 100644 --- a/lib/storex/store.ex +++ b/lib/storex/store.ex @@ -20,9 +20,40 @@ defmodule Storex.Store do @doc """ Called when store session ends. """ - @callback terminate(session_id :: binary(), params :: %{binary() => any()}, state :: any()) :: any() + @callback terminate(session_id :: binary(), params :: %{binary() => any()}, state :: any()) :: + any() @optional_callbacks terminate: 3 + @doc false + # Resolves the store module from the name a client sent. All three checks + # belong together: `Module.safe_concat/1` refuses to create new atoms, + # `Code.ensure_compiled/1` refuses names that do not resolve to a real module, + # and the behaviour check refuses modules that are not stores. Dropping any of + # them lets client input reach arbitrary modules, which is what the SSR path + # did for as long as it carried its own copy of only the first check. + def resolve(store) do + with {:ok, module} <- safe_concat(store), + {:module, module} <- Code.ensure_compiled(module), + true <- storex_store?(module) do + {:ok, module} + else + false -> {:error, :not_store} + _ -> {:error, :not_exists} + end + end + + defp safe_concat(store) do + {:ok, Module.safe_concat([store])} + rescue + ArgumentError -> {:error, :not_exists} + end + + defp storex_store?(module) do + __MODULE__ in (module.module_info(:attributes) + |> Keyword.get_values(:behaviour) + |> List.flatten()) + end + @doc false def __init__(store, session, params) do apply(store, :init, [session, params]) @@ -60,15 +91,29 @@ defmodule Storex.Store do "Return value of mutation should be {:reply, message, state}, {:noreply, state} or {:error, error}"} end rescue - FunctionClauseError -> - {:error, - "No mutation matching #{inspect(name)} with data #{inspect(data)} in store #{inspect(store)}"} + error in FunctionClauseError -> + if unmatched_mutation?(error, store) do + {:error, + "No mutation matching #{inspect(name)} with data #{inspect(data)} in store #{inspect(store)}"} + else + reraise error, __STACKTRACE__ + end end end + # Only the store's own `mutation/5` failing to match means "no such mutation". + # Any other `FunctionClauseError` was raised deeper inside a mutation that did + # match, and has to keep its original stacktrace. + defp unmatched_mutation?(%FunctionClauseError{} = error, store) do + error.module == store and error.function == :mutation and error.arity == 5 + end + @doc false + # `function_exported?/3` answers `false` for a module that is not loaded yet, + # so the callback would be skipped silently. `ensure_loaded?/1` first makes the + # answer depend on the store, not on what the code server happens to hold. def __terminate__(store, session, params, state) do - if :erlang.function_exported(store, :terminate, 3) do + if Code.ensure_loaded?(store) and function_exported?(store, :terminate, 3) do apply(store, :terminate, [session, params, state]) end end @@ -114,6 +159,10 @@ defmodule Storex.Store do {:stop, :normal, state} end + def handle_call(:get_state, _, state) do + {:reply, state.state, state} + end + def handle_call({name, data}, _, state) do Storex.Store.__mutation__(@store, name, data, state.session, state.params, state.state) |> case do diff --git a/lib/storex/supervisor.ex b/lib/storex/supervisor.ex index dac27d7..2a77ead 100644 --- a/lib/storex/supervisor.ex +++ b/lib/storex/supervisor.ex @@ -15,8 +15,18 @@ defmodule Storex.Supervisor do ) end + @doc false + # The name a store process registers under. It is deliberately a `:via` tuple + # and not an atom: session ids are unique per connection, so naming processes + # `:"#{session}_#{store}"` created one permanent atom per session-store pair + # and eventually exhausted the atom table on a long-running node. + # + # Nothing looks a store up by this name — `Storex.Registry` maps to the pid + # for that. It exists so that starting the same `{session, store}` twice + # fails with `{:error, {:already_started, pid}}` instead of silently + # producing a second process. def name(session, store) do - String.to_atom("#{session}_#{store}") + {:via, Registry, {Storex.StoreRegistry, {session, store}}} end def add_store(store, session, session_pid, params \\ %{}) do @@ -46,15 +56,29 @@ defmodule Storex.Supervisor do end end + # `:sys.get_state/1` is a debug function, and using it here meant reading the + # generated `Server`'s internal state shape from the outside, on the join path. + # The process answers for its own state instead. def get_store_state(session, store) do Storex.Registry.get_store_pid(store, session) - |> :sys.get_state() - |> Map.get(:state) + |> case do + :undefined -> + {:error, "Store '#{store}' is not joined in this session."} + + pid -> + {:ok, GenServer.call(pid, :get_state)} + end end def mutate_store(session, store, name, data) do Storex.Registry.get_store_pid(store, session) - |> GenServer.call({name, data}) + |> case do + :undefined -> + {:error, "Store '#{store}' is not joined in this session."} + + pid -> + GenServer.call(pid, {name, data}) + end end def remove_store(session, store) do diff --git a/test/fixtures/not_a_store.ex b/test/fixtures/not_a_store.ex new file mode 100644 index 0000000..e4da980 --- /dev/null +++ b/test/fixtures/not_a_store.ex @@ -0,0 +1,10 @@ +defmodule StorexTest.NotAStore do + @moduledoc """ + A plain module that exports `init/2` but does not declare the `Storex.Store` + behaviour. The SSR path used to resolve and call it. + """ + + def init(_session, _params) do + raise "init/2 must not be called on a module that is not a Storex.Store" + end +end diff --git a/test/fixtures/stores/invalid_mutation.ex b/test/fixtures/stores/invalid_mutation.ex index 9725bbc..fde9374 100644 --- a/test/fixtures/stores/invalid_mutation.ex +++ b/test/fixtures/stores/invalid_mutation.ex @@ -12,4 +12,11 @@ defmodule StorexTest.Store.InvalidMutation do def mutation("error", _data, _session_id, _params, _state) do {:error, "Not allowed"} end + + # Matches, then raises a FunctionClauseError of its own from further down. + def mutation("raise", data, _session_id, _params, _state) do + {:noreply, %{counter: only_zero(data)}} + end + + defp only_zero(0), do: 0 end diff --git a/test/fixtures/stores/terminating.ex b/test/fixtures/stores/terminating.ex new file mode 100644 index 0000000..baf5b2c --- /dev/null +++ b/test/fixtures/stores/terminating.ex @@ -0,0 +1,24 @@ +defmodule StorexTest.Store.Terminating do + @moduledoc """ + Implements the optional `terminate/3` callback and reports it to the pid found + under the `"reporter"` param. The pid is kept out of the state so the state + stays JSON encodable. + """ + + use Storex.Store + + def init(_session, _params) do + {:ok, %{counter: 0}} + end + + def mutation("increase", _data, _session_id, _params, state) do + {:noreply, %{state | counter: state.counter + 1}} + end + + def terminate(session, params, state) do + case Map.get(params, "reporter") do + nil -> :ok + pid -> send(pid, {:terminated, session, params, state}) + end + end +end diff --git a/test/storex/diff_test.exs b/test/storex/diff_test.exs index 77120aa..7a45d68 100644 --- a/test/storex/diff_test.exs +++ b/test/storex/diff_test.exs @@ -43,6 +43,18 @@ defmodule StorexTest.Diff do assert Enum.member?(diff, %{a: "u", p: [:age], t: 10}) end + test "diff struct against a plain map" do + diff = Storex.Diff.check(%Struct{name: "A", age: 1}, %{name: "B", age: 1}) + + assert diff == [%{a: "u", p: [:name], t: "B"}] + end + + test "diff plain map against a struct" do + diff = Storex.Diff.check(%{name: "A", age: 1}, %Struct{name: "B", age: 1}) + + assert diff == [%{a: "u", p: [:name], t: "B"}] + end + test "diff DateTime" do assert [%{a: "u", p: [], t: "2000-02-29 23:10:00+01:00 CET Europe/Warsaw"}] = Storex.Diff.check( diff --git a/test/storex/handler/cowboy_test.exs b/test/storex/handler/cowboy_test.exs index 9219533..10de2a8 100644 --- a/test/storex/handler/cowboy_test.exs +++ b/test/storex/handler/cowboy_test.exs @@ -174,6 +174,122 @@ defmodule StorexTest.Handler.Cowboy do end end + describe "unknown message types" do + test "an error frame is refused with 1007", context do + client = tcp_client(context) + http1_handshake(client) + + # `error` frames only travel server to client. This one used to pass + # `Storex.Message.cast/1` and then crash `message_handle/2`. + send_text_frame(client, """ + { + "type": "error", + "store": "StorexTest.Store.Counter", + "data": null, + "request": "#{random_string()}", + "session": "#{random_string()}" + } + """) + + assert recv_connection_close_frame(client) == + {:ok, <<1007::16, "Payload is malformed."::binary>>} + end + end + + describe "binary frames" do + test "are rejected with 1003", context do + client = tcp_client(context) + http1_handshake(client) + + send_binary_frame(client, :erlang.term_to_binary(%{type: "ping", request: "r"})) + + assert recv_connection_close_frame(client) == + {:ok, <<1003::16, "Binary frames are not supported."::binary>>} + end + + test "cannot create atoms", context do + client = tcp_client(context) + http1_handshake(client) + + name = "storex_binary_frame_probe_#{System.unique_integer([:positive])}" + + # External term format for an atom that does not exist in this VM yet. It + # is built by hand because `:erlang.term_to_binary/1` would create the atom + # here first, in the test process. + send_binary_frame(client, <<131, 118, byte_size(name)::16, name::binary>>) + + assert recv_connection_close_frame(client) == + {:ok, <<1003::16, "Binary frames are not supported."::binary>>} + + assert_raise ArgumentError, fn -> String.to_existing_atom(name) end + end + end + + describe "keepalive" do + test "a ping is answered with a pong carrying the same request id", context do + client = tcp_client(context) + http1_handshake(client) + + request = random_string() + + send_text_frame(client, """ + { + "type": "ping", + "request": "#{request}" + } + """) + + {:ok, result} = recv_text_frame(client) + + assert %{type: "pong", request: ^request} = Jason.decode!(result, keys: :atoms) + end + + test "a ping does not need a store to be joined", context do + client = tcp_client(context) + http1_handshake(client) + + send_text_frame(client, ~s({"type": "ping", "request": "#{random_string()}"})) + + assert {:ok, _} = recv_text_frame(client) + end + end + + describe "session cleanup" do + test "closing the connection stops the session's stores", context do + client = tcp_client(context) + http1_handshake(client) + + send_text_frame(client, """ + { + "type": "join", + "store": "StorexTest.Store.Counter", + "data": {}, + "request": "#{random_string()}" + } + """) + + {:ok, result} = recv_text_frame(client) + assert %{session: session} = Jason.decode!(result, keys: :atoms) + + store_pid = Storex.Registry.get_store_pid("StorexTest.Store.Counter", session) + assert is_pid(store_pid) + + :gen_tcp.close(client) + + assert Enum.reduce_while(1..200, false, fn _, _ -> + if Storex.Registry.session_stores(session) == [] do + {:halt, true} + else + Process.sleep(10) + {:cont, false} + end + end), + "the session's registry rows were not cleaned up" + + refute Process.alive?(store_pid) + end + end + # Simple WebSocket client def tcp_client(context) do diff --git a/test/storex/handler/plug_test.exs b/test/storex/handler/plug_test.exs index d4617cb..41d0d66 100644 --- a/test/storex/handler/plug_test.exs +++ b/test/storex/handler/plug_test.exs @@ -178,6 +178,122 @@ defmodule StorexTest.Handler.Plug do end end + describe "unknown message types" do + test "an error frame is refused with 1007", context do + client = tcp_client(context) + http1_handshake(client, Storex.Handler.Plug) + + # `error` frames only travel server to client. This one used to pass + # `Storex.Message.cast/1` and then crash `message_handle/2`. + send_text_frame(client, """ + { + "type": "error", + "store": "StorexTest.Store.Counter", + "data": null, + "request": "#{random_string()}", + "session": "#{random_string()}" + } + """) + + assert recv_connection_close_frame(client) == + {:ok, <<1007::16, "Payload is malformed."::binary>>} + end + end + + describe "binary frames" do + test "are rejected with 1003", context do + client = tcp_client(context) + http1_handshake(client, Storex.Handler.Plug) + + send_binary_frame(client, :erlang.term_to_binary(%{type: "ping", request: "r"})) + + assert recv_connection_close_frame(client) == + {:ok, <<1003::16, "Binary frames are not supported."::binary>>} + end + + test "cannot create atoms", context do + client = tcp_client(context) + http1_handshake(client, Storex.Handler.Plug) + + name = "storex_binary_frame_probe_#{System.unique_integer([:positive])}" + + # External term format for an atom that does not exist in this VM yet. It + # is built by hand because `:erlang.term_to_binary/1` would create the atom + # here first, in the test process. + send_binary_frame(client, <<131, 118, byte_size(name)::16, name::binary>>) + + assert recv_connection_close_frame(client) == + {:ok, <<1003::16, "Binary frames are not supported."::binary>>} + + assert_raise ArgumentError, fn -> String.to_existing_atom(name) end + end + end + + describe "keepalive" do + test "a ping is answered with a pong carrying the same request id", context do + client = tcp_client(context) + http1_handshake(client, Storex.Handler.Plug) + + request = random_string() + + send_text_frame(client, """ + { + "type": "ping", + "request": "#{request}" + } + """) + + {:ok, result} = recv_text_frame(client) + + assert %{type: "pong", request: ^request} = Jason.decode!(result, keys: :atoms) + end + + test "a ping does not need a store to be joined", context do + client = tcp_client(context) + http1_handshake(client, Storex.Handler.Plug) + + send_text_frame(client, ~s({"type": "ping", "request": "#{random_string()}"})) + + assert {:ok, _} = recv_text_frame(client) + end + end + + describe "session cleanup" do + test "closing the connection stops the session's stores", context do + client = tcp_client(context) + http1_handshake(client, Storex.Handler.Plug) + + send_text_frame(client, """ + { + "type": "join", + "store": "StorexTest.Store.Counter", + "data": {}, + "request": "#{random_string()}" + } + """) + + {:ok, result} = recv_text_frame(client) + assert %{session: session} = Jason.decode!(result, keys: :atoms) + + store_pid = Storex.Registry.get_store_pid("StorexTest.Store.Counter", session) + assert is_pid(store_pid) + + :gen_tcp.close(client) + + assert Enum.reduce_while(1..200, false, fn _, _ -> + if Storex.Registry.session_stores(session) == [] do + {:halt, true} + else + Process.sleep(10) + {:cont, false} + end + end), + "the session's registry rows were not cleaned up" + + refute Process.alive?(store_pid) + end + end + # Simple WebSocket client def tcp_client(context) do diff --git a/test/storex/http_test.exs b/test/storex/http_test.exs new file mode 100644 index 0000000..8ab7548 --- /dev/null +++ b/test/storex/http_test.exs @@ -0,0 +1,50 @@ +defmodule StorexTest.HTTPTest do + use ExUnit.Case + + describe "init_store/2" do + test "returns the initial state of a store" do + assert {:ok, + %{ + type: "join", + session: "SSR", + store: "StorexTest.Store.Counter", + data: %{counter: 0} + }} = Storex.HTTP.init_store("StorexTest.Store.Counter", "{}") + end + + test "params are forwarded to the store" do + assert {:ok, %{data: "custom"}} = + Storex.HTTP.init_store("StorexTest.Store.Text", ~s({"initial_value": "custom"})) + end + + test "a module that is not a store is refused without being called" do + # StorexTest.NotAStore exports init/2 and raises if it is ever reached. + assert {:error, + %{ + type: "error", + session: "SSR", + store: "StorexTest.NotAStore", + error: "Store 'StorexTest.NotAStore' is not defined or can't be compiled." + }} = Storex.HTTP.init_store("StorexTest.NotAStore", "{}") + end + + test "a name that does not resolve to a module is refused" do + assert {:error, %{type: "error", error: error}} = + Storex.HTTP.init_store("StorexTest.Store.NotExisting", "{}") + + assert error == + "Store 'StorexTest.Store.NotExisting' is not defined or can't be compiled." + end + + test "an {:error, reason} from the store is passed through" do + assert {:error, %{type: "error", error: "Unauthorized"}} = + Storex.HTTP.init_store("StorexTest.Store.ErrorInit", "{}") + end + + test "an unsupported return value from the store raises" do + assert_raise RuntimeError, + "Return value of store init should be {:ok, state}, {:ok, state, key} or {:error, reason}", + fn -> Storex.HTTP.init_store("StorexTest.Store.InvalidInit", "{}") end + end + end +end diff --git a/test/storex/message_test.exs b/test/storex/message_test.exs new file mode 100644 index 0000000..a0c36b8 --- /dev/null +++ b/test/storex/message_test.exs @@ -0,0 +1,85 @@ +defmodule StorexTest.MessageTest do + use ExUnit.Case + + describe "cast/1" do + test "casts a ping" do + assert Storex.Message.cast(%{"type" => "ping", "request" => "r"}) == + {:ok, %Storex.Message{type: "ping", request: "r"}} + end + + test "casts a join" do + assert Storex.Message.cast(%{ + "type" => "join", + "store" => "Store", + "data" => %{}, + "request" => "r" + }) == + {:ok, %Storex.Message{type: "join", store: "Store", data: %{}, request: "r"}} + end + + test "casts a join carrying a session" do + assert Storex.Message.cast(%{ + "type" => "join", + "store" => "Store", + "data" => %{}, + "request" => "r", + "session" => "s" + }) == + {:ok, + %Storex.Message{ + type: "join", + store: "Store", + data: %{}, + request: "r", + session: "s" + }} + end + + test "casts a mutation" do + assert Storex.Message.cast(%{ + "type" => "mutation", + "store" => "Store", + "data" => %{"name" => "increase", "data" => []}, + "request" => "r", + "session" => "s" + }) == + {:ok, + %Storex.Message{ + type: "mutation", + store: "Store", + data: %{name: "increase", data: []}, + request: "r", + session: "s" + }} + end + + test "refuses an error frame, which only travels server to client" do + assert Storex.Message.cast(%{ + "type" => "error", + "store" => "Store", + "data" => nil, + "request" => "r", + "session" => "s" + }) == {:error, "Unknown message type"} + end + + test "refuses a mutation without a name" do + assert Storex.Message.cast(%{ + "type" => "mutation", + "store" => "Store", + "data" => %{}, + "request" => "r", + "session" => "s" + }) == {:error, "Unknown message type"} + end + + test "refuses an unknown type" do + assert Storex.Message.cast(%{"type" => "whatever", "request" => "r"}) == + {:error, "Unknown message type"} + end + + test "refuses a payload that is not a message" do + assert Storex.Message.cast(%{}) == {:error, "Unknown message type"} + end + end +end diff --git a/test/storex/registry_test.exs b/test/storex/registry_test.exs new file mode 100644 index 0000000..922fd0a --- /dev/null +++ b/test/storex/registry_test.exs @@ -0,0 +1,92 @@ +defmodule StorexTest.RegistryTest do + use ExUnit.Case, async: false + + @registry :storex_registry + + defp eventually(check, attempts \\ 100) do + Enum.reduce_while(1..attempts, false, fn _, _ -> + if check.() do + {:halt, true} + else + Process.sleep(10) + {:cont, false} + end + end) + end + + setup do + session = "session-#{System.unique_integer([:positive])}" + {:ok, _} = Storex.Registry.register_store("Store", self(), session, self(), "key") + on_exit(fn -> Storex.Registry.unregister_store("Store", session) end) + %{session: session} + end + + test "get_store/2 returns the whole row", %{session: session} do + assert Storex.Registry.get_store("Store", session) == + {"Store", self(), session, self(), "key"} + end + + test "get_store_pid/2 returns the store pid", %{session: session} do + assert Storex.Registry.get_store_pid("Store", session) == self() + end + + test "session_stores/1 returns every row of a session", %{session: session} do + assert Storex.Registry.session_stores(session) == + [{"Store", self(), session, self(), "key"}] + end + + test "get_store_instances/1 matches on the given query", %{session: session} do + assert Storex.Registry.get_store_instances({"Store", :_, :_, :_, "key"}) + |> Enum.any?(fn + {_, _, ^session, _, _} -> true + _ -> false + end) + end + + test "a miss is :undefined", %{session: session} do + assert Storex.Registry.get_store("Never.Joined", session) == :undefined + assert Storex.Registry.get_store_pid("Never.Joined", session) == :undefined + end + + test "a row is dropped when the store process dies" do + session = "session-#{System.unique_integer([:positive])}" + store_pid = spawn(fn -> Process.sleep(:infinity) end) + + {:ok, _} = Storex.Registry.register_store("Dying", store_pid, session, self(), nil) + assert Storex.Registry.get_store_pid("Dying", session) == store_pid + + Process.exit(store_pid, :kill) + + # The registry monitors the store, so the row goes when the process does. + assert eventually(fn -> Storex.Registry.get_store_pid("Dying", session) == :undefined end) + assert Storex.Registry.session_stores(session) == [] + end + + test "only the dead process's rows are dropped", %{session: session} do + other = "session-#{System.unique_integer([:positive])}" + store_pid = spawn(fn -> Process.sleep(:infinity) end) + + {:ok, _} = Storex.Registry.register_store("Dying", store_pid, other, self(), nil) + Process.exit(store_pid, :kill) + + assert eventually(fn -> Storex.Registry.get_store_pid("Dying", other) == :undefined end) + assert Storex.Registry.get_store_pid("Store", session) == self() + end + + test "reads do not go through the registry process", %{session: session} do + # With the owner suspended, anything that needs a GenServer round-trip + # blocks. Reads run in the calling process, so they still answer. + :sys.suspend(@registry) + + try do + assert Storex.Registry.get_store_pid("Store", session) == self() + assert Storex.Registry.get_store("Store", session) |> elem(0) == "Store" + assert Storex.Registry.session_stores(session) != [] + + # A write still needs the process, so it times out while it is suspended. + assert catch_exit(GenServer.call(@registry, {:unregister_store, "Store", session}, 100)) + after + :sys.resume(@registry) + end + end +end diff --git a/test/storex/socket_test.exs b/test/storex/socket_test.exs new file mode 100644 index 0000000..0903843 --- /dev/null +++ b/test/storex/socket_test.exs @@ -0,0 +1,100 @@ +defmodule StorexTest.SocketTest do + use ExUnit.Case + + @store "StorexTest.Store.Counter" + + defp session, do: "session-#{System.unique_integer([:positive])}" + + defp joined_session do + session = session() + {:ok, _key} = Storex.Supervisor.add_store(@store, session, self(), %{}) + on_exit(fn -> Storex.Supervisor.remove_store(session, @store) end) + session + end + + defp mutation(session, name) do + %Storex.Message{ + type: "mutation", + store: @store, + session: session, + data: %{name: name, data: []}, + request: "request-id" + } + end + + describe "mutation" do + test "the session in the frame cannot address another session's store" do + attacker = joined_session() + victim = joined_session() + + # The frame claims the victim's session; the socket belongs to the attacker. + message = mutation(victim, "increase") + state = %{session: attacker, pid: self()} + + assert {:text, response, ^state} = Storex.Socket.message_handle(message, state) + + assert Storex.Supervisor.get_store_state(victim, @store) == {:ok, %{counter: 0}} + assert Storex.Supervisor.get_store_state(attacker, @store) == {:ok, %{counter: 1}} + + assert %{"session" => ^attacker, "diff" => [%{"p" => ["counter"], "t" => 1}]} = + Jason.decode!(response) + end + + test "the response always echoes the session of the socket" do + session = joined_session() + state = %{session: session, pid: self()} + + assert {:text, response, ^state} = + Storex.Socket.message_handle(mutation("some-other-session", "increase"), state) + + assert %{"session" => ^session} = Jason.decode!(response) + end + + test "a store the session has not joined returns an error instead of crashing" do + session = session() + state = %{session: session, pid: self()} + + assert {:text, response, ^state} = + Storex.Socket.message_handle(mutation(session, "increase"), state) + + assert %{ + "type" => "error", + "session" => ^session, + "store" => @store, + "error" => error, + "request" => "request-id" + } = Jason.decode!(response) + + assert error == "Store 'StorexTest.Store.Counter' is not joined in this session." + end + + test "a reply from the store is passed through" do + session = joined_session() + state = %{session: session, pid: self()} + + assert {:text, response, ^state} = + Storex.Socket.message_handle(mutation(session, "decrease"), state) + + assert %{"message" => "decreased", "diff" => [%{"p" => ["counter"], "t" => -1}]} = + Jason.decode!(response) + end + + test "a mutation pushed by Storex.mutate/3 is handled" do + session = joined_session() + state = %{session: session, pid: self()} + + # The shape the handlers build in handle_info/2: a plain map, no request id. + pushed = %{ + type: "mutation", + session: session, + store: @store, + data: %{data: [], name: "increase"} + } + + assert {:text, response, ^state} = Storex.Socket.message_handle(pushed, state) + + assert %{"request" => nil, "diff" => [%{"p" => ["counter"], "t" => 1}]} = + Jason.decode!(response) + end + end +end diff --git a/test/storex/store_test.exs b/test/storex/store_test.exs index 5a30786..bf0ed89 100644 --- a/test/storex/store_test.exs +++ b/test/storex/store_test.exs @@ -6,8 +6,28 @@ defmodule StorexTest.StoreTest do alias StorexTest.Store.InvalidInit alias StorexTest.Store.InvalidMutation alias StorexTest.Store.KeyInit + alias StorexTest.Store.Terminating alias StorexTest.Store.Text + describe "resolve/1" do + test "resolves a module that declares the behaviour" do + assert Storex.Store.resolve("StorexTest.Store.Counter") == {:ok, Counter} + end + + test "refuses a module that does not declare the behaviour" do + assert Storex.Store.resolve("StorexTest.NotAStore") == {:error, :not_store} + end + + test "refuses a name that does not resolve to a module" do + assert Storex.Store.resolve("StorexTest.Store.NotExisting") == {:error, :not_exists} + end + + test "refuses a name that is not an existing atom" do + assert Storex.Store.resolve("Never.Compiled.#{System.unique_integer([:positive])}") == + {:error, :not_exists} + end + end + describe "init dispatch" do test "{:ok, state} is normalized with a nil key" do assert Storex.Store.__init__(Counter, "session", %{}) == {:ok, %{counter: 0}, nil} @@ -60,6 +80,43 @@ defmodule StorexTest.StoreTest do {:error, "No mutation matching \"unknown\" with data [1] in store StorexTest.Store.Counter"} end + + test "a FunctionClauseError raised inside a matching mutation is not swallowed" do + error = + assert_raise FunctionClauseError, fn -> + Storex.Store.__mutation__(InvalidMutation, "raise", 1, "session", %{}, %{}) + end + + assert error.function == :only_zero + assert error.arity == 1 + end + + test "the original stacktrace of a raise inside a mutation is preserved" do + stacktrace = + try do + Storex.Store.__mutation__(InvalidMutation, "raise", 1, "session", %{}, %{}) + rescue + _ -> __STACKTRACE__ + end + + assert [{InvalidMutation, :only_zero, [1], _location} | _rest] = stacktrace + end + end + + describe "terminate dispatch" do + test "the callback is invoked with the session, params and state" do + params = %{"reporter" => self()} + + assert Storex.Store.__terminate__(Terminating, "session", params, %{counter: 3}) == + {:terminated, "session", params, %{counter: 3}} + + assert_received {:terminated, "session", ^params, %{counter: 3}} + end + + test "a store that does not implement it is a no-op" do + refute function_exported?(Counter, :terminate, 3) + assert Storex.Store.__terminate__(Counter, "session", %{}, %{counter: 0}) == nil + end end describe "store server" do @@ -78,9 +135,8 @@ defmodule StorexTest.StoreTest do assert {:ok, nil} = Storex.Supervisor.add_store("StorexTest.Store.Counter", session, self(), %{}) - assert Storex.Supervisor.get_store_state(session, "StorexTest.Store.Counter") == %{ - counter: 0 - } + assert Storex.Supervisor.get_store_state(session, "StorexTest.Store.Counter") == + {:ok, %{counter: 0}} end test "starts a store returning {:ok, state, key}", %{session: session} do @@ -93,6 +149,18 @@ defmodule StorexTest.StoreTest do Storex.Supervisor.add_store("StorexTest.Store.ErrorInit", session, self(), %{}) end + test "stopping a store runs terminate/3 with the current state", %{session: session} do + store = "StorexTest.Store.Terminating" + params = %{"reporter" => self()} + + {:ok, _} = Storex.Supervisor.add_store(store, session, self(), params) + {:ok, _} = Storex.Supervisor.mutate_store(session, store, "increase", []) + + Storex.Supervisor.remove_store(session, store) + + assert_receive {:terminated, ^session, ^params, %{counter: 1}}, 1000 + end + test "mutating through the server returns the state diff", %{session: session} do {:ok, _} = Storex.Supervisor.add_store("StorexTest.Store.Counter", session, self(), %{}) diff --git a/test/storex/storex_test.exs b/test/storex/storex_test.exs index b8b7ff6..595dd49 100644 --- a/test/storex/storex_test.exs +++ b/test/storex/storex_test.exs @@ -53,7 +53,7 @@ defmodule StorexTest do test "get store", %{session: session, store: store, pid: pid} do assert {:ok, _pid} = Storex.Supervisor.add_store(store, session, pid, %{}) - assert %{counter: 0} = Storex.Supervisor.get_store_state(session, store) + assert {:ok, %{counter: 0}} = Storex.Supervisor.get_store_state(session, store) Storex.Supervisor.remove_store(session, store) end @@ -64,7 +64,55 @@ defmodule StorexTest do assert_receive :ok - assert %{counter: 1} = Storex.Supervisor.get_store_state(session, store) + assert {:ok, %{counter: 1}} = Storex.Supervisor.get_store_state(session, store) + end + + test "reaches every session of the store", %{session: session, store: store, pid: pid} do + # The point of mutate/3 is the fan-out. One session proves nothing. + other_session = Application.get_env(:storex, :session_id_library, Nanoid).generate() + + {:ok, other_pid} = + GenServer.start_link(FakeWebsocketServer, [self(), other_session], + name: {:global, other_session} + ) + + assert {:ok, _} = Storex.Supervisor.add_store(store, session, pid, %{}) + assert {:ok, _} = Storex.Supervisor.add_store(store, other_session, other_pid, %{}) + + Storex.mutate(store, "increase", []) + + assert_receive :ok + assert_receive :ok + + assert {:ok, %{counter: 1}} = Storex.Supervisor.get_store_state(session, store) + assert {:ok, %{counter: 1}} = Storex.Supervisor.get_store_state(other_session, store) + + Storex.Supervisor.remove_store(other_session, store) + end + + test "does not reach a session that joined a different store", %{ + session: session, + store: store, + pid: pid + } do + other_session = Application.get_env(:storex, :session_id_library, Nanoid).generate() + + {:ok, other_pid} = + GenServer.start_link(FakeWebsocketServer, [self(), other_session], + name: {:global, other_session} + ) + + assert {:ok, _} = Storex.Supervisor.add_store(store, session, pid, %{}) + + assert {:ok, _} = + Storex.Supervisor.add_store("StorexTest.Store.Text", other_session, other_pid, %{}) + + Storex.mutate(store, "increase", []) + + assert_receive :ok + refute_receive :ok, 200 + + Storex.Supervisor.remove_store(other_session, "StorexTest.Store.Text") end test "mutate store in cluster", %{session: session, store: store, pid: pid} do @@ -83,7 +131,7 @@ defmodule StorexTest do assert_receive :ok - assert %{counter: 1} = Storex.Supervisor.get_store_state(session, store) + assert {:ok, %{counter: 1}} = Storex.Supervisor.get_store_state(session, store) end end @@ -117,7 +165,7 @@ defmodule StorexTest do assert_receive :ok - assert %{counter: 1} = Storex.Supervisor.get_store_state(session, store) + assert {:ok, %{counter: 1}} = Storex.Supervisor.get_store_state(session, store) end test "don't mutate store for invalid key", %{session: session, store: store, pid: pid} do @@ -127,7 +175,7 @@ defmodule StorexTest do refute_receive :ok - assert %{counter: 0} = Storex.Supervisor.get_store_state(session, store) + assert {:ok, %{counter: 0}} = Storex.Supervisor.get_store_state(session, store) end test "don't mutate store for invalid key in cluster", %{ @@ -150,7 +198,7 @@ defmodule StorexTest do refute_receive :ok - assert %{counter: 0} = Storex.Supervisor.get_store_state(session, store) + assert {:ok, %{counter: 0}} = Storex.Supervisor.get_store_state(session, store) end end @@ -166,4 +214,14 @@ defmodule StorexTest do assert {:error, "Unauthorized"} = Storex.Supervisor.add_store(store, session, self(), %{}) end end + + describe "mutate return value" do + test "mutate/3 returns :ok" do + assert Storex.mutate("StorexTest.Store.Counter", "increase", []) == :ok + end + + test "mutate/4 returns :ok" do + assert Storex.mutate("key", "StorexTest.Store.Counter", "increase", []) == :ok + end + end end diff --git a/test/storex/supervisor_test.exs b/test/storex/supervisor_test.exs new file mode 100644 index 0000000..116730a --- /dev/null +++ b/test/storex/supervisor_test.exs @@ -0,0 +1,109 @@ +defmodule StorexTest.SupervisorTest do + use ExUnit.Case + + @store "StorexTest.Store.Counter" + + defp session, do: "session-#{System.unique_integer([:positive])}" + + defp start_store(session) do + {:ok, _key} = Storex.Supervisor.add_store(@store, session, self(), %{}) + on_exit(fn -> Storex.Supervisor.remove_store(session, @store) end) + session + end + + describe "store process naming" do + test "a store is registered under a {session, store} key, not an atom" do + session = start_store(session()) + + assert [{pid, nil}] = Registry.lookup(Storex.StoreRegistry, {session, @store}) + assert pid == Storex.Registry.get_store_pid(@store, session) + end + + test "starting stores does not create atoms" do + # Warm up so that first-call code paths are not counted. + for _ <- 1..5, do: start_store(session()) + + sessions = for _ <- 1..200, do: session() + + atoms_before = :erlang.system_info(:atom_count) + for session <- sessions, do: start_store(session) + atoms_after = :erlang.system_info(:atom_count) + + assert atoms_after - atoms_before == 0 + end + + test "joining a store twice reuses the running one", %{} do + session = start_store(session()) + pid = Storex.Registry.get_store_pid(@store, session) + + # The second add_store/4 takes the registry branch and starts nothing. + assert {:ok, nil} = Storex.Supervisor.add_store(@store, session, self(), %{}) + + assert Storex.Registry.get_store_pid(@store, session) == pid + assert length(Storex.Registry.session_stores(session)) == 1 + end + + test "joining a store twice returns the key from the first join" do + store = "StorexTest.Store.KeyInit" + session = session() + + assert {:ok, "user_id"} = Storex.Supervisor.add_store(store, session, self(), %{}) + assert {:ok, "user_id"} = Storex.Supervisor.add_store(store, session, self(), %{}) + + on_exit(fn -> Storex.Supervisor.remove_store(session, store) end) + end + + test "the same session and store cannot be started twice" do + session = start_store(session()) + + spec = %{ + id: StorexTest.Store.Counter.Server, + start: + {StorexTest.Store.Counter.Server, :start_link, + [[session: session, store: @store, params: %{}]]}, + restart: :transient + } + + assert {:error, {:already_started, pid}} = + DynamicSupervisor.start_child(Storex.Supervisor, spec) + + assert pid == Storex.Registry.get_store_pid(@store, session) + end + + test "reading the state of a store that was never started returns an error" do + assert Storex.Supervisor.get_store_state(session(), @store) == + {:error, "Store 'StorexTest.Store.Counter' is not joined in this session."} + end + + test "the store answers for its own state, without :sys.get_state/1" do + session = start_store(session()) + pid = Storex.Registry.get_store_pid(@store, session) + + assert GenServer.call(pid, :get_state) == %{counter: 0} + assert Storex.Supervisor.get_store_state(session, @store) == {:ok, %{counter: 0}} + end + + test "mutating a store that was never started returns an error" do + assert Storex.Supervisor.mutate_store(session(), @store, "increase", []) == + {:error, "Store 'StorexTest.Store.Counter' is not joined in this session."} + end + + test "the name is released when the store stops" do + session = start_store(session()) + + assert [{_pid, nil}] = Registry.lookup(Storex.StoreRegistry, {session, @store}) + + Storex.Supervisor.remove_store(session, @store) + + # The cast is asynchronous, so wait for the Registry to drop the entry. + Enum.reduce_while(1..100, nil, fn _, _ -> + case Registry.lookup(Storex.StoreRegistry, {session, @store}) do + [] -> {:halt, :ok} + _ -> Process.sleep(10) && {:cont, nil} + end + end) + + assert Registry.lookup(Storex.StoreRegistry, {session, @store}) == [] + end + end +end