Replace conditionals with functional pattern matching

Refactor cond/if/nested-case blocks into idiomatic multi-clause
functions across permissions, organizations, credential resolver,
SNMP client, and monitoring modules. Extract shared log_snmp_error
helper and dispatch_snmp for SNMP adapter routing.
This commit is contained in:
Graham McIntire 2026-02-10 17:11:37 -06:00
parent 4d73e77f3a
commit 138151ade6
No known key found for this signature in database
5 changed files with 161 additions and 303 deletions

View file

@ -60,30 +60,9 @@ defmodule Towerops.Devices.CredentialResolver do
def resolve_snmp_credentials(changeset, organization, site) do
# If user explicitly set community, keep it and mark as "device"
# Otherwise resolve from site → org and track source
case_result =
case get_field(changeset, :snmp_community) do
nil ->
# Resolve from hierarchy
{community, source} = resolve_with_source(site, organization, :snmp_community)
changeset
|> put_change(:snmp_community, community)
|> put_change(:snmp_community_source, source)
"" ->
# Empty string means inherit from site/org
{community, source} = resolve_with_source(site, organization, :snmp_community)
changeset
|> put_change(:snmp_community, community)
|> put_change(:snmp_community_source, source)
_value ->
# User set a value, mark as device-specific
put_change(changeset, :snmp_community_source, "device")
end
case_result
changeset
|> get_field(:snmp_community)
|> resolve_snmp_community(changeset, organization, site)
|> resolve_snmp_version(organization, site)
|> resolve_snmp_port(organization, site)
end
@ -121,36 +100,9 @@ defmodule Towerops.Devices.CredentialResolver do
@spec resolve_snmpv3_credentials(Ecto.Changeset.t(), Organization.t(), Site.t() | nil) ::
Ecto.Changeset.t()
def resolve_snmpv3_credentials(changeset, organization, site) do
# Only resolve if SNMP version is "3"
version = get_field(changeset, :snmp_version)
if version == "3" do
case get_field(changeset, :snmpv3_username) do
nil ->
# Resolve all SNMPv3 fields from hierarchy
{username, source} = resolve_with_source(site, organization, :snmpv3_username)
security_level = resolve_value(site, organization, :snmpv3_security_level)
auth_protocol = resolve_value(site, organization, :snmpv3_auth_protocol) || "SHA-256"
auth_password = resolve_value(site, organization, :snmpv3_auth_password)
priv_protocol = resolve_value(site, organization, :snmpv3_priv_protocol) || "AES"
priv_password = resolve_value(site, organization, :snmpv3_priv_password)
changeset
|> put_change(:snmpv3_username, username)
|> put_change(:snmpv3_security_level, security_level)
|> put_change(:snmpv3_auth_protocol, auth_protocol)
|> put_change(:snmpv3_auth_password, auth_password)
|> put_change(:snmpv3_priv_protocol, priv_protocol)
|> put_change(:snmpv3_priv_password, priv_password)
|> put_change(:snmpv3_credential_source, source)
_value ->
# User set credentials, mark as device-specific
put_change(changeset, :snmpv3_credential_source, "device")
end
else
changeset
end
changeset
|> get_field(:snmp_version)
|> do_resolve_snmpv3(changeset, organization, site)
end
@doc """
@ -194,26 +146,70 @@ defmodule Towerops.Devices.CredentialResolver do
# Private helpers
# Resolves a field from site → org, returns {value, source}
defp resolve_with_source(site, organization, field) do
cond do
site && Map.get(site, field) != nil ->
{Map.get(site, field), "site"}
# Resolves SNMP community from field value, collapsing nil and "" (both mean "inherit")
defp resolve_snmp_community(value, changeset, organization, site) when value in [nil, ""] do
{community, source} = resolve_with_source(site, organization, :snmp_community)
Map.get(organization, field) != nil ->
{Map.get(organization, field), "organization"}
changeset
|> put_change(:snmp_community, community)
|> put_change(:snmp_community_source, source)
end
true ->
{nil, "organization"}
defp resolve_snmp_community(_value, changeset, _organization, _site) do
put_change(changeset, :snmp_community_source, "device")
end
# Only resolve SNMPv3 fields when version is "3"
defp do_resolve_snmpv3("3", changeset, organization, site) do
case get_field(changeset, :snmpv3_username) do
nil ->
{username, source} = resolve_with_source(site, organization, :snmpv3_username)
security_level = resolve_value(site, organization, :snmpv3_security_level)
auth_protocol = resolve_value(site, organization, :snmpv3_auth_protocol) || "SHA-256"
auth_password = resolve_value(site, organization, :snmpv3_auth_password)
priv_protocol = resolve_value(site, organization, :snmpv3_priv_protocol) || "AES"
priv_password = resolve_value(site, organization, :snmpv3_priv_password)
changeset
|> put_change(:snmpv3_username, username)
|> put_change(:snmpv3_security_level, security_level)
|> put_change(:snmpv3_auth_protocol, auth_protocol)
|> put_change(:snmpv3_auth_password, auth_password)
|> put_change(:snmpv3_priv_protocol, priv_protocol)
|> put_change(:snmpv3_priv_password, priv_password)
|> put_change(:snmpv3_credential_source, source)
_value ->
put_change(changeset, :snmpv3_credential_source, "device")
end
end
defp do_resolve_snmpv3(_version, changeset, _organization, _site), do: changeset
# Resolves a field from site → org, returns {value, source}
defp resolve_with_source(%Site{} = site, organization, field) do
case Map.get(site, field) do
nil -> resolve_with_source(nil, organization, field)
value -> {value, "site"}
end
end
defp resolve_with_source(_site, %Organization{} = organization, field) do
case Map.get(organization, field) do
nil -> {nil, "organization"}
value -> {value, "organization"}
end
end
defp resolve_with_source(_site, _organization, _field), do: {nil, "organization"}
# Resolves a field from site → org, returns just the value
defp resolve_value(site, organization, field) do
if site && Map.get(site, field) != nil do
Map.get(site, field)
else
Map.get(organization, field)
defp resolve_value(%Site{} = site, organization, field) do
case Map.get(site, field) do
nil -> resolve_value(nil, organization, field)
value -> value
end
end
defp resolve_value(_site, organization, field), do: Map.get(organization, field)
end

View file

@ -72,12 +72,7 @@ defmodule Towerops.Monitoring do
{:ok, %{rows: rows} = result} when rows != [] ->
result
{:ok, _empty_result} ->
# Continuous aggregate may not have recent data yet, fall back to raw query
get_hourly_stats_raw(device_id, start_time, end_time)
{:error, _} ->
# Fallback to raw query if continuous aggregate doesn't exist
_empty_or_error ->
get_hourly_stats_raw(device_id, start_time, end_time)
end
end
@ -137,12 +132,7 @@ defmodule Towerops.Monitoring do
{:ok, %{rows: rows} = result} when rows != [] ->
result
{:ok, _empty_result} ->
# Continuous aggregate may not have recent data yet, fall back to raw query
get_daily_stats_raw(device_id, start_time, end_time)
{:error, _} ->
# Fallback to raw query if continuous aggregate doesn't exist
_empty_or_error ->
get_daily_stats_raw(device_id, start_time, end_time)
end
end
@ -195,16 +185,7 @@ defmodule Towerops.Monitoring do
{:ok, %{rows: [[%Decimal{} = uptime]]}} ->
uptime |> Decimal.to_float() |> Float.round(2)
{:ok, %{rows: [[nil]]}} ->
# Continuous aggregate may not have recent data yet, fall back to raw query
get_uptime_percentage_raw(device_id, start_time)
{:ok, %{rows: []}} ->
# Continuous aggregate may not have recent data yet, fall back to raw query
get_uptime_percentage_raw(device_id, start_time)
{:error, _} ->
# Fallback to raw query if continuous aggregate doesn't exist
_empty_or_error ->
get_uptime_percentage_raw(device_id, start_time)
end
end

View file

@ -78,23 +78,22 @@ defmodule Towerops.Organizations do
end
end
defp check_free_org_limit(bypass_limits, subscription_plan, user_id, attrs) do
if bypass_limits or subscription_plan != "free" do
defp check_free_org_limit(true, _plan, _user_id, _attrs), do: :ok
defp check_free_org_limit(_bypass, plan, _user_id, _attrs) when plan != "free", do: :ok
defp check_free_org_limit(false, "free", user_id, attrs) do
if SubscriptionLimits.can_create_free_organization?(user_id) do
:ok
else
if SubscriptionLimits.can_create_free_organization?(user_id) do
:ok
else
changeset =
%Organization{}
|> Organization.changeset(attrs)
|> Ecto.Changeset.add_error(
:base,
"You already have a free organization. Upgrade your existing organization to create additional ones."
)
changeset =
%Organization{}
|> Organization.changeset(attrs)
|> Ecto.Changeset.add_error(
:base,
"You already have a free organization. Upgrade your existing organization to create additional ones."
)
{:error, changeset}
end
{:error, changeset}
end
end

View file

@ -43,22 +43,7 @@ defmodule Towerops.Snmp.Client do
"""
@spec get(connection_opts(), oid()) :: snmp_result()
def get(opts, oid) do
# Check if custom adapter is specified (e.g., Replay adapter)
case Keyword.get(opts, :adapter) do
nil ->
# No adapter - check if Phoenix SNMP is disabled
if phoenix_snmp_disabled() do
log_disabled_call("get", opts, oid)
{:error, :phoenix_snmp_disabled}
else
# Default behavior - use SnmpKit
do_get_with_snmpkit(opts, oid)
end
adapter ->
# Custom adapter - delegate directly (bypass Phoenix SNMP check)
adapter.get(opts, oid)
end
dispatch_snmp(:get, opts, oid, fn -> do_get_with_snmpkit(opts, oid) end)
end
# Original get implementation using SnmpKit
@ -74,30 +59,7 @@ defmodule Towerops.Snmp.Client do
{:ok, extract_snmp_value(value)}
{:error, reason} = error ->
# Only log warnings for actual communication errors, not missing OIDs
case reason do
:no_such_object ->
Logger.debug("SNMP object not found for #{target}: #{inspect(oid)}")
:no_such_instance ->
Logger.debug("SNMP instance not found for #{target}: #{inspect(oid)}")
:no_such_name ->
Logger.debug("SNMP name not found for #{target}: #{inspect(oid)}")
:end_of_mib_view ->
Logger.debug("End of MIB view reached for #{target}: #{inspect(oid)}")
_ ->
# Actual errors like timeout, network issues, etc.
timeout = Keyword.get(opts, :timeout, @default_timeout)
version = Keyword.fetch!(opts, :version)
Logger.warning(
"SNMP GET failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(oid)}: #{inspect(reason)}"
)
end
log_snmp_error("GET", reason, target, oid, opts)
error
end
end
@ -186,29 +148,7 @@ defmodule Towerops.Snmp.Client do
{:ok, %{oid: next_oid, value: value}}
{:error, reason} = error ->
# Only log warnings for actual communication errors
case reason do
:no_such_object ->
Logger.debug("SNMP GET-NEXT: object not found for #{target} at #{inspect(oid)}")
:no_such_instance ->
Logger.debug("SNMP GET-NEXT: instance not found for #{target} at #{inspect(oid)}")
:no_such_name ->
Logger.debug("SNMP GET-NEXT: name not found for #{target} at #{inspect(oid)}")
:end_of_mib_view ->
Logger.debug("SNMP GET-NEXT: end of MIB view for #{target} at #{inspect(oid)}")
_ ->
timeout = Keyword.get(opts, :timeout, @default_timeout)
version = Keyword.fetch!(opts, :version)
Logger.warning(
"SNMP GET-NEXT failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(oid)}: #{inspect(reason)}"
)
end
log_snmp_error("GET-NEXT", reason, target, oid, opts)
error
end
end
@ -227,32 +167,9 @@ defmodule Towerops.Snmp.Client do
"""
@spec walk(connection_opts(), oid()) :: {:ok, %{String.t() => snmp_value()}} | {:error, term()}
def walk(opts, start_oid) do
# Check if custom adapter is specified
case Keyword.get(opts, :adapter) do
nil ->
# No adapter - check if Phoenix SNMP is disabled
if phoenix_snmp_disabled() do
log_disabled_call("walk", opts, start_oid)
{:error, :phoenix_snmp_disabled}
else
# Default behavior - use SnmpKit
do_walk_with_snmpkit(opts, start_oid)
end
adapter ->
# Custom adapter - delegate and convert to map format (bypass Phoenix SNMP check)
convert_adapter_walk_to_map(adapter.walk(opts, start_oid))
end
dispatch_snmp(:walk, opts, start_oid, fn -> do_walk_with_snmpkit(opts, start_oid) end)
end
# Convert adapter's list result to map format (for backward compatibility)
defp convert_adapter_walk_to_map({:ok, results}) when is_list(results) do
walked_data = Map.new(results, fn {oid, value} -> {oid, value} end)
{:ok, walked_data}
end
defp convert_adapter_walk_to_map(error), do: error
# Original walk implementation using SnmpKit
defp do_walk_with_snmpkit(opts, start_oid) do
target = build_target(opts)
@ -263,39 +180,13 @@ defmodule Towerops.Snmp.Client do
case snmp_adapter().walk(target, resolved_oid, snmp_opts) do
{:ok, results} when is_list(results) ->
# snmpkit returns a list of maps, convert to OID -> value map
walked_data =
Map.new(results, fn %{oid: oid, value: value} -> {oid, extract_snmp_value(value)} end)
{:ok, walked_data}
{:error, reason} = error ->
# Only log warnings for actual communication errors
case reason do
:no_such_object ->
Logger.debug("SNMP WALK: object not found for #{target} at #{inspect(start_oid)}")
:no_such_instance ->
Logger.debug("SNMP WALK: instance not found for #{target} at #{inspect(start_oid)}")
:no_such_name ->
Logger.debug("SNMP WALK: name not found for #{target} at #{inspect(start_oid)}")
:end_of_mib_view ->
Logger.debug("SNMP WALK: end of MIB view for #{target} at #{inspect(start_oid)}")
:not_found ->
Logger.debug("SNMP WALK: MIB not found for #{target} at #{inspect(start_oid)}")
_ ->
timeout = Keyword.get(opts, :timeout, @default_timeout)
version = Keyword.fetch!(opts, :version)
Logger.warning(
"SNMP WALK failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(start_oid)}: #{inspect(reason)}"
)
end
log_snmp_error("WALK", reason, target, start_oid, opts)
error
end
end
@ -331,29 +222,7 @@ defmodule Towerops.Snmp.Client do
{:ok, bulk_data}
{:error, reason} = error ->
# Only log warnings for actual communication errors
case reason do
:no_such_object ->
Logger.debug("SNMP GET-BULK: object not found for #{target} at #{inspect(start_oid)}")
:no_such_instance ->
Logger.debug("SNMP GET-BULK: instance not found for #{target} at #{inspect(start_oid)}")
:no_such_name ->
Logger.debug("SNMP GET-BULK: name not found for #{target} at #{inspect(start_oid)}")
:end_of_mib_view ->
Logger.debug("SNMP GET-BULK: end of MIB view for #{target} at #{inspect(start_oid)}")
_ ->
timeout = Keyword.get(opts, :timeout, @default_timeout)
version = Keyword.fetch!(opts, :version)
Logger.warning(
"SNMP GET-BULK failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(start_oid)}: #{inspect(reason)}"
)
end
log_snmp_error("GET-BULK", reason, target, start_oid, opts)
error
end
end
@ -371,24 +240,14 @@ defmodule Towerops.Snmp.Client do
"""
@spec test_connection(connection_opts()) :: {:ok, String.t()} | {:error, term()}
def test_connection(opts) do
# Check if custom adapter is specified
adapter = Keyword.get(opts, :adapter)
# get/2 already handles adapter dispatch and disabled check
case get(opts, "1.3.6.1.2.1.1.3.0") do
{:ok, _uptime} ->
{:ok, "Connection successful"}
if adapter == nil and phoenix_snmp_disabled() do
# No adapter and Phoenix SNMP is disabled
log_disabled_call("test_connection", opts, "sysUpTime.0")
{:error, :phoenix_snmp_disabled}
else
# Try to get sysUpTime (1.3.6.1.2.1.1.3.0) as a connectivity test
# This will use the adapter if one is specified, or SnmpKit if not
case get(opts, "1.3.6.1.2.1.1.3.0") do
{:ok, _uptime} ->
{:ok, "Connection successful"}
{:error, reason} = error ->
Logger.warning("SNMP connection test failed: #{inspect(reason)}")
error
end
{:error, reason} = error ->
Logger.warning("SNMP connection test failed: #{inspect(reason)}")
error
end
end
@ -400,15 +259,52 @@ defmodule Towerops.Snmp.Client do
Always returns false in test environment.
"""
@spec phoenix_snmp_disabled() :: boolean()
def phoenix_snmp_disabled do
# Only disable in non-test environments (allow tests to run normally)
if Application.get_env(:towerops, :env) == :test do
false
else
Application.get_env(:towerops, :disable_phoenix_snmp, true)
def phoenix_snmp_disabled, do: do_phoenix_snmp_disabled(Application.get_env(:towerops, :env))
defp do_phoenix_snmp_disabled(:test), do: false
defp do_phoenix_snmp_disabled(_env), do: Application.get_env(:towerops, :disable_phoenix_snmp, true)
# Dispatches SNMP operations: custom adapter → disabled check → default implementation
defp dispatch_snmp(operation, opts, oid, default_fn) do
case Keyword.get(opts, :adapter) do
nil ->
if phoenix_snmp_disabled() do
log_disabled_call(Atom.to_string(operation), opts, oid)
{:error, :phoenix_snmp_disabled}
else
default_fn.()
end
adapter when operation == :walk ->
opts |> adapter.walk(oid) |> convert_adapter_walk_to_map()
adapter ->
adapter.get(opts, oid)
end
end
# Convert adapter's list result to map format (for backward compatibility)
defp convert_adapter_walk_to_map({:ok, results}) when is_list(results) do
{:ok, Map.new(results, fn {oid, value} -> {oid, value} end)}
end
defp convert_adapter_walk_to_map(error), do: error
# Shared SNMP error logging with pattern-matched clauses for known vs unknown errors
defp log_snmp_error(operation, reason, target, oid, _opts)
when reason in [:no_such_object, :no_such_instance, :no_such_name, :end_of_mib_view, :not_found] do
Logger.debug("SNMP #{operation}: #{reason} for #{target} at #{inspect(oid)}")
end
defp log_snmp_error(operation, reason, target, oid, opts) do
timeout = Keyword.get(opts, :timeout, @default_timeout)
version = Keyword.fetch!(opts, :version)
Logger.warning(
"SNMP #{operation} failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(oid)}: #{inspect(reason)}"
)
end
defp log_disabled_call(operation, opts, oid_or_param) do
ip = Keyword.get(opts, :ip, "unknown")
version = Keyword.get(opts, :version, "unknown")

View file

@ -64,20 +64,7 @@ defmodule ToweropsWeb.Permissions do
end
def can?(%Scope{} = scope, action, resource) do
cond do
# Superusers can do everything
Scope.superuser?(scope) ->
true
# No organization context - cannot perform action
is_nil(scope.organization) ->
false
# Check organization membership role permissions
true ->
membership = get_membership(scope)
Policy.can?(membership, action, resource)
end
if Scope.superuser?(scope), do: true, else: do_can?(scope, action, resource)
end
def can?(nil, _action, _resource), do: false
@ -100,17 +87,7 @@ defmodule ToweropsWeb.Permissions do
end
def owner?(%Scope{} = scope) do
cond do
Scope.superuser?(scope) ->
true
is_nil(scope.organization) ->
false
true ->
membership = get_membership(scope)
membership && membership.role == :owner
end
if Scope.superuser?(scope), do: true, else: do_owner?(scope)
end
def owner?(nil), do: false
@ -133,27 +110,36 @@ defmodule ToweropsWeb.Permissions do
end
def admin?(%Scope{} = scope) do
cond do
Scope.superuser?(scope) ->
true
is_nil(scope.organization) ->
false
true ->
membership = get_membership(scope)
membership && membership.role in [:owner, :admin]
end
if Scope.superuser?(scope), do: true, else: do_admin?(scope)
end
def admin?(nil), do: false
# Private helper to extract membership from scope
# Pattern-matched private helpers for permission checks
defp do_can?(%Scope{organization: nil}, _action, _resource), do: false
defp do_can?(%Scope{} = scope, action, resource) do
scope |> get_membership() |> Policy.can?(action, resource)
end
defp do_owner?(%Scope{organization: nil}), do: false
defp do_owner?(%Scope{} = scope) do
membership = get_membership(scope)
membership && membership.role == :owner
end
defp do_admin?(%Scope{organization: nil}), do: false
defp do_admin?(%Scope{} = scope) do
membership = get_membership(scope)
membership && membership.role in [:owner, :admin]
end
defp get_membership(%Scope{organization: org, user: user}) when not is_nil(org) and not is_nil(user) do
# Organization should be preloaded with memberships
if Ecto.assoc_loaded?(org.memberships) do
Enum.find(org.memberships, &(&1.user_id == user.id))
# If memberships not preloaded, return nil (permission check will fail safely)
end
end