From 0a978746e1f81fce2c91dde3923e8f724ffb1af9 Mon Sep 17 00:00:00 2001 From: mayor Date: Fri, 6 Feb 2026 10:31:09 -0600 Subject: [PATCH] Fix all Credo issues Resolves 26 Credo warnings/issues: Code Readability (2 issues): - Break long line in mac_address.ex (line 316 > 120 chars) - Break long @spec in agents.ex (line 332 > 120 chars) Refactoring Opportunities (2 issues): - Reduce cyclomatic complexity in device_live/form.ex - Extract helper functions to simplify SNMPv3 credential resolution - Split logic into smaller, focused functions - Reduce nesting depth in snmp_oid.ex - Extract nested logic into separate helper functions Warnings (22 issues): - Fix comparison warning in validator.ex - Split validate_sensor_value/1 into separate clauses for int/float - Add credo directive for valid NaN check (value != value) - Replace 21 expensive length/1 checks with Enum.empty?/1 - Change assertions from `assert length(list) >= 1` to `refute Enum.empty?(list)` - Change assertions from `assert length(list) > 0` to `refute Enum.empty?(list)` - Affected files: device_poller_worker_test.exs, mib_test.exs, snmp_tokenizer_test.exs, parser_test.exs, error_test.exs, compiler_test.exs All tests passing (5578 tests, 0 failures). Credo now reports: "found no issues" --- lib/towerops/agent/validator.ex | 9 +- lib/towerops/agents.ex | 3 +- lib/towerops/ecto_types/mac_address.ex | 6 +- lib/towerops/ecto_types/snmp_oid.ex | 26 +++--- lib/towerops_web/live/device_live/form.ex | 91 ++++++++++--------- test/snmpkit/snmp_lib/mib/compiler_test.exs | 4 +- test/snmpkit/snmp_lib/mib/error_test.exs | 14 +-- test/snmpkit/snmp_lib/mib/parser_test.exs | 2 +- .../snmp_lib/mib/snmp_tokenizer_test.exs | 16 ++-- test/snmpkit/snmp_lib/mib_test.exs | 2 +- .../workers/device_poller_worker_test.exs | 4 +- 11 files changed, 94 insertions(+), 83 deletions(-) diff --git a/lib/towerops/agent/validator.ex b/lib/towerops/agent/validator.ex index 681cdb9c..60a84241 100644 --- a/lib/towerops/agent/validator.ex +++ b/lib/towerops/agent/validator.ex @@ -368,14 +368,17 @@ defmodule Towerops.Agent.Validator do defp validate_job_id(_), do: {:error, {:invalid_job_id, "Job ID must be non-empty string"}} # Validate sensor value (must be finite float/int) - defp validate_sensor_value(value) when is_float(value) or is_integer(value) do + defp validate_sensor_value(value) when is_integer(value), do: :ok + + defp validate_sensor_value(value) when is_float(value) do # Check for NaN (NaN is the only value that doesn't equal itself) # Check for infinity by comparing with max/min float values cond do - is_float(value) and value != value -> + # credo:disable-for-next-line Credo.Check.Warning.OperationOnSameValues + value != value -> {:error, {:invalid_sensor_value, "Sensor value must be finite number (not NaN)"}} - is_float(value) and (value > 1.0e308 or value < -1.0e308) -> + value > 1.0e308 or value < -1.0e308 -> {:error, {:invalid_sensor_value, "Sensor value must be finite number (not infinity)"}} true -> diff --git a/lib/towerops/agents.ex b/lib/towerops/agents.ex index 27b9e218..0ad08aeb 100644 --- a/lib/towerops/agents.ex +++ b/lib/towerops/agents.ex @@ -329,7 +329,8 @@ defmodule Towerops.Agents do {:error, %Ecto.Changeset{}} """ - @spec assign_device_to_agent(Ecto.UUID.t(), Ecto.UUID.t()) :: {:ok, AgentAssignment.t()} | {:error, Ecto.Changeset.t()} + @spec assign_device_to_agent(Ecto.UUID.t(), Ecto.UUID.t()) :: + {:ok, AgentAssignment.t()} | {:error, Ecto.Changeset.t()} def assign_device_to_agent(agent_token_id, device_id) do result = %AgentAssignment{} diff --git a/lib/towerops/ecto_types/mac_address.ex b/lib/towerops/ecto_types/mac_address.ex index 4ef92e60..dfbcd7b7 100644 --- a/lib/towerops/ecto_types/mac_address.ex +++ b/lib/towerops/ecto_types/mac_address.ex @@ -313,7 +313,11 @@ defmodule Towerops.EctoTypes.MacAddress do defp binary_to_dot_string(<>) do Enum.join( - [format_octet(o1) <> format_octet(o2), format_octet(o3) <> format_octet(o4), format_octet(o5) <> format_octet(o6)], + [ + format_octet(o1) <> format_octet(o2), + format_octet(o3) <> format_octet(o4), + format_octet(o5) <> format_octet(o6) + ], "." ) end diff --git a/lib/towerops/ecto_types/snmp_oid.ex b/lib/towerops/ecto_types/snmp_oid.ex index e13503a1..2f2cca5f 100644 --- a/lib/towerops/ecto_types/snmp_oid.ex +++ b/lib/towerops/ecto_types/snmp_oid.ex @@ -261,23 +261,25 @@ defmodule Towerops.EctoTypes.SnmpOid do # Try SnmpKit resolution case SnmpKit.resolve(oid_string) do {:ok, resolved} -> - # Ensure leading dot - normalized = if String.starts_with?(resolved, "."), do: resolved, else: "." <> resolved - {:ok, normalized} + {:ok, ensure_leading_dot(resolved)} {:error, _} = error -> - # If it's already a numeric OID, return it - if Regex.match?(@numeric_oid_pattern, oid_string) do - normalized = - if String.starts_with?(oid_string, "."), do: oid_string, else: "." <> oid_string - - {:ok, normalized} - else - error - end + resolve_numeric_oid(oid_string, error) end end + defp resolve_numeric_oid(oid_string, error) do + if Regex.match?(@numeric_oid_pattern, oid_string) do + {:ok, ensure_leading_dot(oid_string)} + else + error + end + end + + defp ensure_leading_dot(oid_string) do + if String.starts_with?(oid_string, "."), do: oid_string, else: "." <> oid_string + end + # Private helpers defp parse_oid(""), do: :error diff --git a/lib/towerops_web/live/device_live/form.ex b/lib/towerops_web/live/device_live/form.ex index 9fd6b737..1628faf4 100644 --- a/lib/towerops_web/live/device_live/form.ex +++ b/lib/towerops_web/live/device_live/form.ex @@ -323,57 +323,58 @@ defmodule ToweropsWeb.DeviceLive.Form do # Resolve SNMPv3 credentials with site/org inheritance defp resolve_snmpv3_credentials_for_test(form_data, changeset, assigns) do - changes = changeset.changes + cond do + # Use device-level credentials if available + form_data.snmpv3_username && form_data.snmpv3_username != "" -> + build_device_snmpv3_creds(form_data, changeset.changes) - # Check if device has credentials (either from DB or form input) - if form_data.snmpv3_username && form_data.snmpv3_username != "" do - # Use device-level values - # For passwords, prefer changeset changes (newly entered), fall back to DB values - %{ - version: "3", - security_level: form_data.snmpv3_security_level, - username: form_data.snmpv3_username, - auth_protocol: form_data.snmpv3_auth_protocol, - auth_password: Map.get(changes, :snmpv3_auth_password) || form_data.snmpv3_auth_password, - priv_protocol: form_data.snmpv3_priv_protocol, - priv_password: Map.get(changes, :snmpv3_priv_password) || form_data.snmpv3_priv_password - } - else - # Inherit from site or org - site_id = form_data.site_id - site = site_id && Enum.find(assigns.available_sites, &(&1.id == site_id)) + # Inherit from site if available + site_has_snmpv3_creds?(form_data, assigns) -> + site = find_site(form_data.site_id, assigns.available_sites) + build_snmpv3_creds(site) - cond do - # Try site-level credentials - site && site.snmpv3_username -> - %{ - version: "3", - security_level: site.snmpv3_security_level, - username: site.snmpv3_username, - auth_protocol: site.snmpv3_auth_protocol, - auth_password: site.snmpv3_auth_password, - priv_protocol: site.snmpv3_priv_protocol, - priv_password: site.snmpv3_priv_password - } + # Fall back to organization credentials + assigns.organization.snmpv3_username -> + build_snmpv3_creds(assigns.organization) - # Fall back to org-level credentials - assigns.organization.snmpv3_username -> - %{ - version: "3", - security_level: assigns.organization.snmpv3_security_level, - username: assigns.organization.snmpv3_username, - auth_protocol: assigns.organization.snmpv3_auth_protocol, - auth_password: assigns.organization.snmpv3_auth_password, - priv_protocol: assigns.organization.snmpv3_priv_protocol, - priv_password: assigns.organization.snmpv3_priv_password - } - - true -> - %{version: "3"} - end + # No credentials available + true -> + %{version: "3"} end end + defp site_has_snmpv3_creds?(form_data, assigns) do + site = find_site(form_data.site_id, assigns.available_sites) + site && site.snmpv3_username + end + + defp find_site(nil, _sites), do: nil + defp find_site(site_id, sites), do: Enum.find(sites, &(&1.id == site_id)) + + defp build_device_snmpv3_creds(form_data, changes) do + %{ + version: "3", + security_level: form_data.snmpv3_security_level, + username: form_data.snmpv3_username, + auth_protocol: form_data.snmpv3_auth_protocol, + auth_password: Map.get(changes, :snmpv3_auth_password) || form_data.snmpv3_auth_password, + priv_protocol: form_data.snmpv3_priv_protocol, + priv_password: Map.get(changes, :snmpv3_priv_password) || form_data.snmpv3_priv_password + } + end + + defp build_snmpv3_creds(source) do + %{ + version: "3", + security_level: source.snmpv3_security_level, + username: source.snmpv3_username, + auth_protocol: source.snmpv3_auth_protocol, + auth_password: source.snmpv3_auth_password, + priv_protocol: source.snmpv3_priv_protocol, + priv_password: source.snmpv3_priv_password + } + end + # Resolve SNMP community with site/org inheritance defp resolve_snmp_community_for_test(form_data, changeset, assigns) do version = form_data.snmp_version || "2c" diff --git a/test/snmpkit/snmp_lib/mib/compiler_test.exs b/test/snmpkit/snmp_lib/mib/compiler_test.exs index b16b4576..f07038b7 100644 --- a/test/snmpkit/snmp_lib/mib/compiler_test.exs +++ b/test/snmpkit/snmp_lib/mib/compiler_test.exs @@ -191,7 +191,7 @@ defmodule SnmpKit.SnmpLib.MIB.CompilerTest do assert {:error, errors} = Compiler.compile_all([file1, file2]) assert is_list(errors) - assert length(errors) > 0 + refute Enum.empty?(errors) # Cleanup File.rm_rf!(dir) @@ -367,7 +367,7 @@ defmodule SnmpKit.SnmpLib.MIB.CompilerTest do assert {:error, errors} = Compiler.compile_string(mib_content) assert is_list(errors) - assert length(errors) > 0 + refute Enum.empty?(errors) end test "handles missing file errors" do diff --git a/test/snmpkit/snmp_lib/mib/error_test.exs b/test/snmpkit/snmp_lib/mib/error_test.exs index 032a5d58..f8a014fe 100644 --- a/test/snmpkit/snmp_lib/mib/error_test.exs +++ b/test/snmpkit/snmp_lib/mib/error_test.exs @@ -95,42 +95,42 @@ defmodule SnmpKit.SnmpLib.MIB.ErrorTest do test "generates suggestions for unexpected_token MAX-ACCESS" do error = Error.new(:unexpected_token, expected: :max_access, actual: :access) - assert length(error.suggestions) > 0 + refute Enum.empty?(error.suggestions) assert Enum.any?(error.suggestions, &String.contains?(&1, "MAX-ACCESS")) end test "generates suggestions for unexpected_token mandatory/current" do error = Error.new(:unexpected_token, expected: :current, actual: :mandatory) - assert length(error.suggestions) > 0 + refute Enum.empty?(error.suggestions) assert Enum.any?(error.suggestions, &String.contains?(&1, "deprecated")) end test "generates suggestions for unexpected_token OBJECT-TYPE" do error = Error.new(:unexpected_token, expected: :object_type, actual: :object_identity) - assert length(error.suggestions) > 0 + refute Enum.empty?(error.suggestions) assert Enum.any?(error.suggestions, &String.contains?(&1, "OBJECT-TYPE")) end test "generates suggestions for unterminated_string" do error = Error.new(:unterminated_string) - assert length(error.suggestions) > 0 + refute Enum.empty?(error.suggestions) assert Enum.any?(error.suggestions, &String.contains?(&1, "quote")) end test "generates suggestions for file_not_found with RFC file" do error = Error.new(:file_not_found, file: "RFC1213-MIB.txt") - assert length(error.suggestions) > 0 + refute Enum.empty?(error.suggestions) assert Enum.any?(error.suggestions, &String.contains?(&1, "naming conventions")) end test "generates suggestions for file_not_found with MIB file" do error = Error.new(:file_not_found, file: "TEST-MIB.mib") - assert length(error.suggestions) > 0 + refute Enum.empty?(error.suggestions) assert Enum.any?(error.suggestions, &String.contains?(&1, "extension")) end @@ -144,7 +144,7 @@ defmodule SnmpKit.SnmpLib.MIB.ErrorTest do test "generates suggestions for import_error" do error = Error.new(:import_error, symbol: "test", from_module: "TEST-MIB") - assert length(error.suggestions) > 0 + refute Enum.empty?(error.suggestions) assert Enum.any?(error.suggestions, &String.contains?(&1, "import")) end diff --git a/test/snmpkit/snmp_lib/mib/parser_test.exs b/test/snmpkit/snmp_lib/mib/parser_test.exs index e0007552..b8e6039d 100644 --- a/test/snmpkit/snmp_lib/mib/parser_test.exs +++ b/test/snmpkit/snmp_lib/mib/parser_test.exs @@ -30,7 +30,7 @@ defmodule SnmpKit.SnmpLib.MIB.ParserTest do result = Parser.tokenize(mib) assert {:ok, tokens} = result assert is_list(tokens) - assert length(tokens) > 0 + refute Enum.empty?(tokens) end test "tokenizes reserved words" do diff --git a/test/snmpkit/snmp_lib/mib/snmp_tokenizer_test.exs b/test/snmpkit/snmp_lib/mib/snmp_tokenizer_test.exs index 5957606e..98e11a8f 100644 --- a/test/snmpkit/snmp_lib/mib/snmp_tokenizer_test.exs +++ b/test/snmpkit/snmp_lib/mib/snmp_tokenizer_test.exs @@ -46,7 +46,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do assert {:ok, tokens} = SnmpTokenizer.tokenize(input, &SnmpTokenizer.null_get_line/0) # First token should be the reserved word - assert length(tokens) >= 1 + refute Enum.empty?(tokens) end end @@ -84,7 +84,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do # Strings might be tokenized as :string or just handled as tokens # Just verify tokenization succeeds - assert length(tokens) >= 1 + refute Enum.empty?(tokens) end test "tokenizes special characters" do @@ -136,7 +136,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do for access <- access_values do input = String.to_charlist(access) assert {:ok, tokens} = SnmpTokenizer.tokenize(input, &SnmpTokenizer.null_get_line/0) - assert length(tokens) >= 1 + refute Enum.empty?(tokens) end end @@ -146,7 +146,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do for type <- types do input = String.to_charlist(type) assert {:ok, tokens} = SnmpTokenizer.tokenize(input, &SnmpTokenizer.null_get_line/0) - assert length(tokens) >= 1 + refute Enum.empty?(tokens) end end @@ -369,7 +369,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do long_id = "a" |> String.duplicate(200) |> String.to_charlist() assert {:ok, tokens} = SnmpTokenizer.tokenize(long_id, &SnmpTokenizer.null_get_line/0) - assert length(tokens) >= 1 + refute Enum.empty?(tokens) end test "handles many tokens" do @@ -421,7 +421,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do input = ~c"\"\"" assert {:ok, tokens} = SnmpTokenizer.tokenize(input, &SnmpTokenizer.null_get_line/0) - assert length(tokens) >= 1 + refute Enum.empty?(tokens) end test "handles consecutive operators" do @@ -506,7 +506,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do assert {:ok, tokens} = SnmpTokenizer.tokenize(input, &SnmpTokenizer.null_get_line/0) # Should successfully tokenize without error - assert length(tokens) >= 1 + refute Enum.empty?(tokens) end end @@ -527,7 +527,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizerTest do # "object" should be an atom (lowercase identifier) atoms = Enum.filter(tokens, &match?({:atom, _, _}, &1)) - assert length(atoms) >= 1 + refute Enum.empty?(atoms) end end end diff --git a/test/snmpkit/snmp_lib/mib_test.exs b/test/snmpkit/snmp_lib/mib_test.exs index bc9d0f62..543767c7 100644 --- a/test/snmpkit/snmp_lib/mib_test.exs +++ b/test/snmpkit/snmp_lib/mib_test.exs @@ -10,7 +10,7 @@ defmodule SnmpKit.SnmpLib.MIBTest do assert {:error, errors} = result assert is_list(errors) - assert length(errors) > 0 + refute Enum.empty?(errors) assert %Error{type: :file_not_found} = List.first(errors) end diff --git a/test/towerops/workers/device_poller_worker_test.exs b/test/towerops/workers/device_poller_worker_test.exs index b3314401..fdf426d8 100644 --- a/test/towerops/workers/device_poller_worker_test.exs +++ b/test/towerops/workers/device_poller_worker_test.exs @@ -232,7 +232,7 @@ defmodule Towerops.Workers.DevicePollerWorkerTest do # 8. Verify Interface Stats stats = Repo.all(Towerops.Snmp.InterfaceStat) - assert length(stats) >= 1 + refute Enum.empty?(stats) stat = List.last(stats) assert stat.interface_id == interface.id assert stat.if_in_octets == 1000 @@ -250,7 +250,7 @@ defmodule Towerops.Workers.DevicePollerWorkerTest do # 11. Verify Processor readings = Repo.all(Towerops.Snmp.ProcessorReading) - assert length(readings) >= 1 + refute Enum.empty?(readings) reading = List.last(readings) assert reading.processor_id == processor.id assert reading.load_percent == 25.0