add query_helpers tests and fix all credo issues (#56)

- add tests for QueryHelpers.sanitize_like/1 (%, _, \ escaping)
- fix nesting depth in store_check_result, handle_check_state_change,
  graphql check resolver, checks controller, and ping executor
- fix cyclomatic complexity in build_check_protobuf by extracting
  check_type_config/2 function clauses
- fix formatting in api_docs_html template

Reviewed-on: graham/towerops-web#56
This commit is contained in:
Graham McIntire 2026-03-17 10:43:57 -05:00 committed by graham
parent 1781aace98
commit 0c8bb35ef7
9 changed files with 208 additions and 169 deletions

View file

@ -1 +1 @@
/nix/store/4xlkmdd947h5qlhj2rmbyavq08grkz27-pre-commit-config.json
/nix/store/zx636v118rzzqzwafgrw2aybh7l0szrl-pre-commit-config.json

View file

@ -26,22 +26,7 @@ defmodule Towerops.Monitoring.Executors.PingExecutor do
count = config |> Map.get("count", @default_count) |> max(1) |> min(10)
with :ok <- validate_host(host) do
args = build_args(host, count, timeout_ms)
# Use timeout_ms plus buffer for the system call
system_timeout = timeout_ms + count * 1000 + 2000
case System.cmd("ping", args, stderr_to_stdout: true, timeout: system_timeout) do
{output, 0} ->
parse_output(output)
{output, _exit_code} ->
# Non-zero exit — try to parse anyway (partial loss still has stats)
case parse_output(output) do
{:ok, _, _} = success -> success
{:error, _} = error -> error
end
end
run_ping(host, count, timeout_ms)
end
rescue
e ->
@ -69,6 +54,22 @@ defmodule Towerops.Monitoring.Executors.PingExecutor do
def parse_output(_), do: {:error, "No ping output"}
defp run_ping(host, count, timeout_ms) do
args = build_args(host, count, timeout_ms)
system_timeout = timeout_ms + count * 1000 + 2000
case System.cmd("ping", args, stderr_to_stdout: true, timeout: system_timeout) do
{output, 0} ->
parse_output(output)
{output, _exit_code} ->
case parse_output(output) do
{:ok, _, _} = success -> success
{:error, _} = error -> error
end
end
end
defp validate_host(host) do
# Only allow valid hostnames and IP addresses — no shell metacharacters
if Regex.match?(~r/^[a-zA-Z0-9\.\-\:]+$/, host) do

View file

@ -901,56 +901,46 @@ defmodule ToweropsWeb.AgentChannel do
timeout_ms: check.timeout_ms
]
case check.check_type do
"http" ->
struct!(
CheckProto,
base_fields ++
[
http: %HttpCheckConfig{
url: config["url"] || "",
method: config["method"] || "GET",
expected_status: config["expected_status"] || 200,
verify_ssl: config["verify_ssl"] != false,
regex: config["regex"] || "",
follow_redirects: config["follow_redirects"] != false
}
]
)
"tcp" ->
struct!(
CheckProto,
base_fields ++
[
tcp: %TcpCheckConfig{
host: config["host"] || "",
port: config["port"] || 0,
send: config["send"] || "",
expect: config["expect"] || ""
}
]
)
"dns" ->
struct!(
CheckProto,
base_fields ++
[
dns: %DnsCheckConfig{
hostname: config["hostname"] || "",
server: config["server"] || "",
record_type: config["record_type"] || "A",
expected: config["expected"] || ""
}
]
)
_ ->
struct!(CheckProto, base_fields)
end
struct!(CheckProto, base_fields ++ check_type_config(check.check_type, config))
end
defp check_type_config("http", config) do
[
http: %HttpCheckConfig{
url: config["url"] || "",
method: config["method"] || "GET",
expected_status: config["expected_status"] || 200,
verify_ssl: config["verify_ssl"] != false,
regex: config["regex"] || "",
follow_redirects: config["follow_redirects"] != false
}
]
end
defp check_type_config("tcp", config) do
[
tcp: %TcpCheckConfig{
host: config["host"] || "",
port: config["port"] || 0,
send: config["send"] || "",
expect: config["expect"] || ""
}
]
end
defp check_type_config("dns", config) do
[
dns: %DnsCheckConfig{
hostname: config["hostname"] || "",
server: config["server"] || "",
record_type: config["record_type"] || "A",
expected: config["expected"] || ""
}
]
end
defp check_type_config(_, _config), do: []
@spec build_jobs_for_agent(Ecto.UUID.t()) :: [AgentJob.t()]
defp build_jobs_for_agent(agent_token_id) do
agent_token_id
@ -1544,76 +1534,63 @@ defmodule ToweropsWeb.AgentChannel do
organization_id = socket.assigns.organization_id
agent_token_id = socket.assigns.agent_token_id
case Monitoring.get_check(result.check_id) do
with check when not is_nil(check) <- Monitoring.get_check(result.check_id),
true <- check.organization_id == organization_id do
record_and_process_check_result(check, result, organization_id, agent_token_id)
else
nil ->
Logger.error("Check not found for check result: #{result.check_id}")
{:error, :check_not_found}
check ->
if check.organization_id == organization_id do
now = DateTime.truncate(DateTime.utc_now(), :second)
# Record check result
case Monitoring.create_check_result(%{
check_id: check.id,
organization_id: organization_id,
status: result.status,
output: result.output,
response_time_ms: result.response_time_ms,
checked_at: now,
agent_token_id: agent_token_id
}) do
{:ok, _} ->
# Update check state (soft/hard transitions)
old_state = check.current_state
{:ok, updated_check} = Monitoring.update_check_state(check, result.status, result.output)
# Handle alert creation/resolution on hard state changes
handle_check_state_change(old_state, updated_check, result.output)
# Broadcast for LiveView refresh
if check.device_id do
Phoenix.PubSub.broadcast(
Towerops.PubSub,
"device:#{check.device_id}",
{:monitoring_check_updated, check.device_id}
)
end
:ok
{:error, changeset} ->
Logger.error("Failed to store check result",
check_id: check.id,
errors: inspect(changeset.errors)
)
{:error, :storage_failed}
end
else
Logger.error("Check #{result.check_id} not in agent's organization")
{:error, :wrong_organization}
end
false ->
Logger.error("Check #{result.check_id} not in agent's organization")
{:error, :wrong_organization}
end
end
defp record_and_process_check_result(check, result, organization_id, agent_token_id) do
now = DateTime.truncate(DateTime.utc_now(), :second)
case Monitoring.create_check_result(%{
check_id: check.id,
organization_id: organization_id,
status: result.status,
output: result.output,
response_time_ms: result.response_time_ms,
checked_at: now,
agent_token_id: agent_token_id
}) do
{:ok, _} ->
old_state = check.current_state
{:ok, updated_check} = Monitoring.update_check_state(check, result.status, result.output)
handle_check_state_change(old_state, updated_check, result.output)
broadcast_check_update(check.device_id)
:ok
{:error, changeset} ->
Logger.error("Failed to store check result",
check_id: check.id,
errors: inspect(changeset.errors)
)
{:error, :storage_failed}
end
end
defp broadcast_check_update(nil), do: :ok
defp broadcast_check_update(device_id) do
Phoenix.PubSub.broadcast(
Towerops.PubSub,
"device:#{device_id}",
{:monitoring_check_updated, device_id}
)
end
defp handle_check_state_change(old_state, check, output) do
# State changed from OK to problem and is now hard state
if old_state == 0 and check.current_state in [1, 2] and check.current_state_type == "hard" do
if !Alerts.has_active_check_alert?(check.id) do
severity = if check.current_state == 1, do: 1, else: 2
Alerts.create_alert(%{
check_id: check.id,
device_id: check.device_id,
organization_id: check.organization_id,
alert_type: "check_#{check.check_type}",
severity: severity,
message: "Check '#{check.name}' is #{Monitoring.Check.state_label(check.current_state)}",
output: output,
triggered_at: DateTime.utc_now()
})
end
maybe_create_check_alert(check, output)
end
# State changed from problem to OK
@ -1622,6 +1599,23 @@ defmodule ToweropsWeb.AgentChannel do
end
end
defp maybe_create_check_alert(check, output) do
if !Alerts.has_active_check_alert?(check.id) do
severity = if check.current_state == 1, do: 1, else: 2
Alerts.create_alert(%{
check_id: check.id,
device_id: check.device_id,
organization_id: check.organization_id,
alert_type: "check_#{check.check_type}",
severity: severity,
message: "Check '#{check.name}' is #{Monitoring.Check.state_label(check.current_state)}",
output: output,
triggered_at: DateTime.utc_now()
})
end
end
defp store_lldp_neighbors(%LldpTopologyResult{} = result) do
now = DateTime.utc_now()
device_id = result.device_id

View file

@ -51,20 +51,7 @@ defmodule ToweropsWeb.Api.V1.ChecksController do
if check_type in @service_check_types do
attrs = Map.put(check_params, "organization_id", organization_id)
case Monitoring.create_check(attrs) do
{:ok, check} ->
if check.enabled, do: Monitoring.schedule_check(check)
conn
|> put_status(:created)
|> json(format_check(check))
{:error, %Ecto.Changeset{} = changeset} ->
conn
|> put_status(:unprocessable_entity)
|> json(%{errors: translate_errors(changeset)})
end
create_and_respond(conn, attrs)
else
conn
|> put_status(:bad_request)
@ -78,6 +65,22 @@ defmodule ToweropsWeb.Api.V1.ChecksController do
|> json(%{error: "Missing 'check' parameter"})
end
defp create_and_respond(conn, attrs) do
case Monitoring.create_check(attrs) do
{:ok, check} ->
if check.enabled, do: Monitoring.schedule_check(check)
conn
|> put_status(:created)
|> json(format_check(check))
{:error, %Ecto.Changeset{} = changeset} ->
conn
|> put_status(:unprocessable_entity)
|> json(%{errors: translate_errors(changeset)})
end
end
@doc """
GET /api/v1/checks/:id

View file

@ -3117,7 +3117,7 @@ curl -X POST https://towerops.net/api/v1/integrations/intg-uuid-1/test \\
<p class="mt-4 text-base text-gray-600 dark:text-gray-400">
Service checks monitor the availability and responsiveness of network services. Supported check types: HTTP, TCP, DNS, and ping. SNMP checks are managed internally and are not exposed via this API.
</p>
<!-- Check Model -->
<div class="mt-12">
<h3 class="text-xl font-semibold text-zinc-900 dark:text-white">The check model</h3>
@ -3216,7 +3216,7 @@ curl -X POST https://towerops.net/api/v1/integrations/intg-uuid-1/test \\
</div>
<hr class="my-12 border-gray-200 dark:border-white/10" />
<!-- List all checks -->
<div class="mt-12">
<div class="flex items-center gap-x-3">
@ -3302,7 +3302,7 @@ curl -G https://towerops.net/api/v1/checks \
</div>
<hr class="my-12 border-gray-200 dark:border-white/10" />
<!-- Create a check -->
<div class="mt-12">
<div class="flex items-center gap-x-3">
@ -3413,7 +3413,9 @@ curl https://towerops.net/api/v1/checks \
<div class="mt-4 overflow-hidden rounded-lg bg-gray-900 dark:bg-gray-950">
<div class="border-b border-zinc-800 px-4 py-2">
<p class="text-xs font-medium text-zinc-400">Response <span class="text-zinc-500">201 Created</span></p>
<p class="text-xs font-medium text-zinc-400">
Response <span class="text-zinc-500">201 Created</span>
</p>
</div>
<pre class="p-4 text-sm text-zinc-100 overflow-x-auto"><code><%= raw(~S"""
{
@ -3434,7 +3436,7 @@ curl https://towerops.net/api/v1/checks \
</div>
<hr class="my-12 border-gray-200 dark:border-white/10" />
<!-- Get a check -->
<div class="mt-12">
<div class="flex items-center gap-x-3">
@ -3442,7 +3444,9 @@ curl https://towerops.net/api/v1/checks \
<span class="inline-flex items-center rounded-md bg-emerald-50 px-2 py-1 text-xs font-medium text-emerald-700 ring-1 ring-inset ring-emerald-600/20 dark:bg-emerald-400/10 dark:text-emerald-400 dark:ring-emerald-400/30">
GET
</span>
<span class="text-sm font-mono text-gray-600 dark:text-gray-400">/api/v1/checks/:id</span>
<span class="text-sm font-mono text-gray-600 dark:text-gray-400">
/api/v1/checks/:id
</span>
</div>
<div class="mt-6 grid grid-cols-1 gap-x-12 gap-y-8 lg:grid-cols-2">
@ -3467,7 +3471,7 @@ curl https://towerops.net/api/v1/checks/7c9e6679-7425-40de-944b-e07fc1f90ae7 \
</div>
<hr class="my-12 border-gray-200 dark:border-white/10" />
<!-- Update a check -->
<div class="mt-12">
<div class="flex items-center gap-x-3">
@ -3475,7 +3479,9 @@ curl https://towerops.net/api/v1/checks/7c9e6679-7425-40de-944b-e07fc1f90ae7 \
<span class="inline-flex items-center rounded-md bg-amber-50 px-2 py-1 text-xs font-medium text-amber-700 ring-1 ring-inset ring-amber-600/20 dark:bg-amber-400/10 dark:text-amber-400 dark:ring-amber-400/30">
PATCH
</span>
<span class="text-sm font-mono text-gray-600 dark:text-gray-400">/api/v1/checks/:id</span>
<span class="text-sm font-mono text-gray-600 dark:text-gray-400">
/api/v1/checks/:id
</span>
</div>
<div class="mt-6 grid grid-cols-1 gap-x-12 gap-y-8 lg:grid-cols-2">
@ -3502,7 +3508,7 @@ curl -X PATCH https://towerops.net/api/v1/checks/7c9e6679-7425-40de-944b-e07fc1f
</div>
<hr class="my-12 border-gray-200 dark:border-white/10" />
<!-- Delete a check -->
<div class="mt-12">
<div class="flex items-center gap-x-3">
@ -3510,7 +3516,9 @@ curl -X PATCH https://towerops.net/api/v1/checks/7c9e6679-7425-40de-944b-e07fc1f
<span class="inline-flex items-center rounded-md bg-red-50 px-2 py-1 text-xs font-medium text-red-700 ring-1 ring-inset ring-red-600/20 dark:bg-red-400/10 dark:text-red-400 dark:ring-red-400/30">
DELETE
</span>
<span class="text-sm font-mono text-gray-600 dark:text-gray-400">/api/v1/checks/:id</span>
<span class="text-sm font-mono text-gray-600 dark:text-gray-400">
/api/v1/checks/:id
</span>
</div>
<div class="mt-6 grid grid-cols-1 gap-x-12 gap-y-8 lg:grid-cols-2">
@ -3547,7 +3555,7 @@ curl -X DELETE https://towerops.net/api/v1/checks/7c9e6679-7425-40de-944b-e07fc1
</section>
<hr class="my-16 border-gray-200 dark:border-white/10" />
<!-- Check Results & Metrics Resource -->
<section id="check-results" class="scroll-mt-24 mb-16">
<h2 class="text-3xl font-bold tracking-tight text-zinc-900 dark:text-white">

View file

@ -39,14 +39,7 @@ defmodule ToweropsWeb.GraphQL.Resolvers.Check do
check_type = Map.get(attrs, "check_type", "")
if check_type in @service_check_types do
case Monitoring.create_check(attrs) do
{:ok, check} ->
if check.enabled, do: Monitoring.schedule_check(check)
{:ok, check}
{:error, changeset} ->
{:error, Helpers.format_changeset_errors(changeset)}
end
create_and_schedule_check(attrs)
else
{:error, "Invalid check type '#{check_type}'. Must be one of: #{Enum.join(@service_check_types, ", ")}"}
end
@ -78,6 +71,17 @@ defmodule ToweropsWeb.GraphQL.Resolvers.Check do
def delete(_parent, _args, _resolution), do: Helpers.authentication_error()
defp create_and_schedule_check(attrs) do
case Monitoring.create_check(attrs) do
{:ok, check} ->
if check.enabled, do: Monitoring.schedule_check(check)
{:ok, check}
{:error, changeset} ->
{:error, Helpers.format_changeset_errors(changeset)}
end
end
defp fetch_org_check(id, org_id) do
case ScopedResource.fetch(Check, id, org_id) do
{:ok, check} -> {:ok, check}

View file

@ -0,0 +1,31 @@
defmodule Towerops.QueryHelpersTest do
use ExUnit.Case, async: true
alias Towerops.QueryHelpers
describe "sanitize_like/1" do
test "escapes percent wildcard" do
assert QueryHelpers.sanitize_like("100%") == "100\\%"
end
test "escapes underscore wildcard" do
assert QueryHelpers.sanitize_like("some_value") == "some\\_value"
end
test "escapes backslashes before other characters" do
assert QueryHelpers.sanitize_like("back\\slash") == "back\\\\slash"
end
test "escapes all special characters in combination" do
assert QueryHelpers.sanitize_like("100%_test\\end") == "100\\%\\_test\\\\end"
end
test "returns plain string unchanged" do
assert QueryHelpers.sanitize_like("hello world") == "hello world"
end
test "handles empty string" do
assert QueryHelpers.sanitize_like("") == ""
end
end
end

View file

@ -111,29 +111,27 @@ defmodule Towerops.Workers.SessionCleanupWorkerTest do
SessionCleanupWorker.perform(%Oban.Job{args: %{}})
end
test "handles sessions that expired exactly now" do
test "handles sessions that expire at boundary conditions" do
user = user_fixture()
{_token, user_token} = Accounts.generate_user_session_token_with_record(user)
now = DateTime.utc_now()
# Create a session that expires exactly now (should be considered expired)
expired_now =
# Create a session that expires 1 millisecond in the future
# This ensures it won't be deleted due to timing race conditions
# The query uses `expires_at < now`, so this should NOT be deleted
almost_expired =
insert_browser_session(%{
user_id: user.id,
user_token_id: user_token.id,
expires_at: now
expires_at: DateTime.add(DateTime.utc_now(), 1, :millisecond)
})
# Execute the worker
assert {:ok, %{sessions_deleted: count}} =
SessionCleanupWorker.perform(%Oban.Job{args: %{}})
# The session should be deleted (expires_at < now, not <=)
# Actually, checking the code: where([bs], bs.expires_at < ^now)
# So expires_at == now should NOT be deleted
# The session should NOT be deleted (expires_at is still in the future)
assert count == 0
assert Repo.get(BrowserSession, expired_now.id)
assert Repo.get(BrowserSession, almost_expired.id)
end
end

View file

@ -205,7 +205,7 @@ defmodule ToweropsWeb.Api.V1.PagerdutyWebhookControllerTest do
end
describe "incident.acknowledged event" do
test "acknowledges an active alert matching the dedup key", %{conn: conn, org: org, user: user} do
test "acknowledges an active alert matching the dedup key", %{conn: conn, org: org, user: _user} do
site = site_fixture(org.id)
{:ok, device} =