From aa0c6e6566adc99b1cc7884a8c60f780733fbb40 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Thu, 23 Apr 2026 13:45:55 -0500 Subject: [PATCH] refactor: pattern-match in DeviceCache and DatabaseMetrics + add tests - DeviceCache.pattern_matches?: replace a 3-branch `cond` with a two-clause `case` on `String.contains?(pattern, "?")`. The old `*` branch was dead code (it did the same exact-match compare as the fallback), so it's removed. - Telemetry.DatabaseMetrics.collect_postgres_metrics: dispatch on environment atom via multi-clause private functions instead of `if Application.get_env(...) == :test`. New tests: - DeviceCache wildcard/exact lookup paths exercised via a seeded cache. - MobileUserSocket connect/3 for x_headers/peer_data/empty info and rate-limit deny. - DatabaseMetrics pool-metrics emission captured via :telemetry handler and the :test-env short-circuit. --- lib/aprsme/device_cache.ex | 16 ++- lib/aprsme/telemetry/database_metrics.ex | 23 ++--- test/aprsme/device_cache_test.exs | 46 +++++++++ .../telemetry/database_metrics_test.exs | 59 +++++++++++ .../channels/mobile_user_socket_test.exs | 99 +++++++++++++++++++ 5 files changed, 220 insertions(+), 23 deletions(-) create mode 100644 test/aprsme/telemetry/database_metrics_test.exs create mode 100644 test/aprsme_web/channels/mobile_user_socket_test.exs diff --git a/lib/aprsme/device_cache.ex b/lib/aprsme/device_cache.ex index c65b011..9eab5e1 100644 --- a/lib/aprsme/device_cache.ex +++ b/lib/aprsme/device_cache.ex @@ -123,16 +123,14 @@ defmodule Aprsme.DeviceCache do end) end + # Patterns containing `?` wildcards route through regex; everything else is + # compared verbatim. The old `*` branch was dead code — it did the same + # exact-match compare as the fallback — so it's gone. defp pattern_matches?(pattern, identifier) do - cond do - String.contains?(pattern, "?") -> - matches_wildcard_pattern?(pattern, identifier) - - String.contains?(pattern, "*") -> - pattern == identifier - - true -> - pattern == identifier + if String.contains?(pattern, "?") do + matches_wildcard_pattern?(pattern, identifier) + else + pattern == identifier end end diff --git a/lib/aprsme/telemetry/database_metrics.ex b/lib/aprsme/telemetry/database_metrics.ex index 04d2246..14c7250 100644 --- a/lib/aprsme/telemetry/database_metrics.ex +++ b/lib/aprsme/telemetry/database_metrics.ex @@ -91,23 +91,18 @@ defmodule Aprsme.Telemetry.DatabaseMetrics do end def collect_postgres_metrics do - # Skip database metrics collection in test environment - if Application.get_env(:aprsme, :env) == :test do - :ok - else - do_collect_postgres_metrics() - end + do_collect_postgres_metrics(Application.get_env(:aprsme, :env)) end - defp do_collect_postgres_metrics do - # Check if Repo is started before collecting metrics - case Process.whereis(Aprsme.Repo) do - nil -> - # Repo not started yet, skip metrics collection silently - :ok + # Skip database metrics collection in the test environment — the extra + # pg_stat_* queries add test-suite noise and sometimes fail in CI sandboxes. + defp do_collect_postgres_metrics(:test), do: :ok - _pid -> - collect_database_metrics() + defp do_collect_postgres_metrics(_env) do + case Process.whereis(Aprsme.Repo) do + # Repo not started yet, skip metrics collection silently. + nil -> :ok + _pid -> collect_database_metrics() end end diff --git a/test/aprsme/device_cache_test.exs b/test/aprsme/device_cache_test.exs index c05de12..58e88fd 100644 --- a/test/aprsme/device_cache_test.exs +++ b/test/aprsme/device_cache_test.exs @@ -26,4 +26,50 @@ defmodule Aprsme.DeviceCacheTest do test "lookup_device/1 returns nil on cache miss without crashing when server is not running" do assert DeviceCache.lookup_device("TEST-DEVICE") == nil end + + test "lookup_device/1 returns nil for nil input" do + assert DeviceCache.lookup_device(nil) == nil + end + + describe "wildcard pattern matching via cache" do + setup do + # Seed the cache directly with a small device list so we can exercise + # the pattern-matching branches without running the GenServer. + devices = [ + %Aprsme.Devices{identifier: "APSK21", vendor: "Kenwood", model: "TH-D74"}, + %Aprsme.Devices{identifier: "APS???", vendor: "Kenwood", model: "Unknown"}, + %Aprsme.Devices{identifier: "EXACT", vendor: "Test", model: "Exact"} + ] + + Aprsme.Cache.put(:device_cache, :all_devices, devices) + + on_exit(fn -> Aprsme.Cache.put(:device_cache, :all_devices, []) end) + :ok + end + + test "exact identifier matches literally" do + assert %{vendor: "Test"} = DeviceCache.lookup_device("EXACT") + end + + test "returns nil for non-matching identifier" do + assert DeviceCache.lookup_device("DOES-NOT-EXIST") == nil + end + + test "prefers exact match when both wildcard and exact apply" do + # Both EXACT (literal) and APS??? would not both apply here; just verify + # that exact-string hits don't get confused with wildcard entries. + assert %{identifier: "EXACT"} = DeviceCache.lookup_device("EXACT") + end + + test "? wildcards match a single character" do + # APSK21 matches APS??? (three wildcards after APS) + assert %{identifier: _} = DeviceCache.lookup_device("APSK21") + end + + test "wildcard pattern does not match if length mismatches" do + # APS??? requires exactly 3 trailing chars; "APSXX" has only 2 and + # shouldn't match the wildcard entry (nor is there any other entry). + assert DeviceCache.lookup_device("APSXX") == nil + end + end end diff --git a/test/aprsme/telemetry/database_metrics_test.exs b/test/aprsme/telemetry/database_metrics_test.exs new file mode 100644 index 0000000..8de3223 --- /dev/null +++ b/test/aprsme/telemetry/database_metrics_test.exs @@ -0,0 +1,59 @@ +defmodule Aprsme.Telemetry.DatabaseMetricsTest do + # Attaches telemetry handlers at test-specific event names so each test is + # isolated, but we still keep async: false to avoid handler-name collisions + # in process-global state. + use ExUnit.Case, async: false + + alias Aprsme.Telemetry.DatabaseMetrics + + defp attach_collector(event, tag \\ to_string(System.unique_integer([:positive]))) do + test_pid = self() + handler_id = "test-#{tag}-#{Enum.join(event, ",")}" + + :telemetry.attach( + handler_id, + event, + fn ^event, measurements, metadata, _ -> + send(test_pid, {:telemetry, event, measurements, metadata}) + end, + nil + ) + + on_exit(fn -> :telemetry.detach(handler_id) end) + handler_id + end + + describe "collect_postgres_metrics/0 in :test env" do + test "is a no-op returning :ok" do + original = Application.get_env(:aprsme, :env) + Application.put_env(:aprsme, :env, :test) + + try do + assert DatabaseMetrics.collect_postgres_metrics() == :ok + after + Application.put_env(:aprsme, :env, original) + end + end + end + + describe "collect_db_pool_metrics/0" do + test "emits [:aprsme, :repo, :pool] telemetry with pool_size from config" do + attach_collector([:aprsme, :repo, :pool]) + DatabaseMetrics.collect_db_pool_metrics() + + assert_receive {:telemetry, [:aprsme, :repo, :pool], measurements, _}, 1_000 + assert Map.has_key?(measurements, :size) + assert Map.has_key?(measurements, :idle) + assert Map.has_key?(measurements, :busy) + assert Map.has_key?(measurements, :available) + assert Map.has_key?(measurements, :queue_length) + assert Map.has_key?(measurements, :total) + end + end + + describe "collect_pgbouncer_metrics/0" do + test "is a no-op returning :ok" do + assert DatabaseMetrics.collect_pgbouncer_metrics() == :ok + end + end +end diff --git a/test/aprsme_web/channels/mobile_user_socket_test.exs b/test/aprsme_web/channels/mobile_user_socket_test.exs new file mode 100644 index 0000000..b0e3da4 --- /dev/null +++ b/test/aprsme_web/channels/mobile_user_socket_test.exs @@ -0,0 +1,99 @@ +defmodule AprsmeWeb.MobileUserSocketTest do + # The rate limiter uses a process-wide ETS table; we're careful to use unique + # IPs so tests don't interfere with each other, but we still mark async:false + # to avoid clashing with anything else that flips the :aprsme, :env flag. + use ExUnit.Case, async: false + + alias AprsmeWeb.MobileUserSocket + + defp unique_ip, do: "203.0.113." <> to_string(System.unique_integer([:positive])) + + setup do + original_env = Application.get_env(:aprsme, :env) + on_exit(fn -> Application.put_env(:aprsme, :env, original_env) end) + :ok + end + + describe "connect/3 in :test env" do + test "always succeeds regardless of rate-limit key" do + Application.put_env(:aprsme, :env, :test) + socket = %Phoenix.Socket{assigns: %{}} + + connect_info = %{x_headers: [{"cf-connecting-ip", "198.51.100.1"}]} + + assert {:ok, result} = MobileUserSocket.connect(%{}, socket, connect_info) + assert result.assigns.peer_ip == "198.51.100.1" + end + + test "extracts IP from x-forwarded-for, splitting on comma" do + Application.put_env(:aprsme, :env, :test) + socket = %Phoenix.Socket{assigns: %{}} + + connect_info = %{ + x_headers: [{"x-forwarded-for", "198.51.100.5, 10.0.0.1"}] + } + + assert {:ok, result} = MobileUserSocket.connect(%{}, socket, connect_info) + assert result.assigns.peer_ip == "198.51.100.5" + end + + test "extracts IP from x-real-ip" do + Application.put_env(:aprsme, :env, :test) + socket = %Phoenix.Socket{assigns: %{}} + connect_info = %{x_headers: [{"x-real-ip", "198.51.100.7"}]} + assert {:ok, result} = MobileUserSocket.connect(%{}, socket, connect_info) + assert result.assigns.peer_ip == "198.51.100.7" + end + + test "falls back to 'unknown' when x_headers list contains no known IP header" do + Application.put_env(:aprsme, :env, :test) + socket = %Phoenix.Socket{assigns: %{}} + connect_info = %{x_headers: [{"user-agent", "curl"}]} + assert {:ok, result} = MobileUserSocket.connect(%{}, socket, connect_info) + assert result.assigns.peer_ip == "unknown" + end + + test "uses peer_data when no x_headers" do + Application.put_env(:aprsme, :env, :test) + socket = %Phoenix.Socket{assigns: %{}} + connect_info = %{peer_data: %{address: {127, 0, 0, 1}}} + assert {:ok, result} = MobileUserSocket.connect(%{}, socket, connect_info) + assert result.assigns.peer_ip == "127.0.0.1" + end + + test "falls back to 'unknown' for empty connect_info" do + Application.put_env(:aprsme, :env, :test) + socket = %Phoenix.Socket{assigns: %{}} + assert {:ok, result} = MobileUserSocket.connect(%{}, socket, %{}) + assert result.assigns.peer_ip == "unknown" + end + end + + describe "connect/3 in non-test env with rate limits" do + test "allows first connection from an IP" do + Application.put_env(:aprsme, :env, :prod) + socket = %Phoenix.Socket{assigns: %{}} + ip = unique_ip() + connect_info = %{x_headers: [{"cf-connecting-ip", ip}]} + + assert {:ok, _} = MobileUserSocket.connect(%{}, socket, connect_info) + end + + test "denies once the per-IP limit is exceeded" do + Application.put_env(:aprsme, :env, :prod) + socket = %Phoenix.Socket{assigns: %{}} + ip = unique_ip() + connect_info = %{x_headers: [{"cf-connecting-ip", ip}]} + + # The configured limit is 30 in-module; burn through it, then one more. + for _ <- 1..30, do: MobileUserSocket.connect(%{}, socket, connect_info) + assert :error == MobileUserSocket.connect(%{}, socket, connect_info) + end + end + + describe "id/1" do + test "always returns nil (anonymous socket)" do + assert MobileUserSocket.id(%Phoenix.Socket{assigns: %{}}) == nil + end + end +end