From cd06f437bb10f8cfc0b9e78490d4a0dcc624af8d Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Fri, 30 Jan 2026 16:47:11 -0600 Subject: [PATCH] fix my-data page --- CLAUDE.md | 208 +++++++++++++++++- lib/mix/tasks/geoip.import.ex | 2 +- lib/towerops/api_tokens.ex | 13 ++ lib/towerops/result.ex | 129 +++++++++++ lib/towerops_web/live/account_live/my_data.ex | 2 +- mix.exs | 4 +- 6 files changed, 354 insertions(+), 4 deletions(-) create mode 100644 lib/towerops/result.ex diff --git a/CLAUDE.md b/CLAUDE.md index 0deb4ba5..e6eb067e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -862,4 +862,210 @@ end) - Use `async: false` for tests with shared state (supervisor tests, integration tests) - never open the test coverage html files - remember when working in rust to always run cargo fmt before committing -- never try to use npm to install js things, use esbuild built in to phoenix \ No newline at end of file +- never try to use npm to install js things, use esbuild built in to phoenix + +## Dialyzer: Static Type Analysis + +### Overview + +Dialyzer is Elixir's static type analyzer that catches type errors, unreachable code, and contract violations. It uses "success typing" - only reporting errors it's certain about, avoiding false positives. + +**Key Commands:** +- `mix dialyzer` - Run Dialyzer analysis (builds PLT on first run) +- `mix dialyzer --format dialyzer` - Show detailed error locations +- `mix dialyzer --format ignore_file` - Generate `.dialyzer_ignore.exs` for gradual adoption +- `mix dialyzer --quiet-with-result` - Cleaner output during analysis + +**Configuration** (`mix.exs`): +```elixir +defp dialyzer do + [ + plt_add_deps: :apps_direct, # Faster incremental analysis + plt_file: {:no_warn, "priv/plts/dialyzer.plt"}, + flags: [:error_handling, :underspecs] + ] +end +``` + +### Type Specification Best Practices + +#### 1. Opaque Types - Hide Implementation Details + +Use `@opaque` to enforce data abstraction by hiding internal structure: + +```elixir +defmodule Token do + @opaque t :: %__MODULE__{ + value: String.t(), + expires_at: DateTime.t() + } + + defstruct [:value, :expires_at] + + @spec new(String.t(), DateTime.t()) :: t() + def new(value, expires_at), do: %__MODULE__{value: value, expires_at: expires_at} + + @spec valid?(t()) :: boolean() + def valid?(%__MODULE__{expires_at: exp}), do: DateTime.compare(DateTime.utc_now(), exp) == :lt +end + +# External modules can't access %Token{} internals directly +# They must use Token.new/2 and Token.valid?/1 +``` + +**Benefits:** +- Safe refactoring - internal changes don't break external code +- Enforces API boundaries between modules +- Prevents invalid data construction + +#### 2. Recursive Types - Self-Referential Structures + +Model tree-like or nested data with recursive types: + +```elixir +defmodule AST do + @type expr :: + {:const, integer()} + | {:add, expr(), expr()} + | {:multiply, expr(), expr()} + + @spec evaluate(expr()) :: integer() + def evaluate({:const, n}), do: n + def evaluate({:add, left, right}), do: evaluate(left) + evaluate(right) + def evaluate({:multiply, left, right}), do: evaluate(left) * evaluate(right) +end +``` + +#### 3. Generic Types - Parameterized Patterns + +Create reusable, type-safe wrappers: + +```elixir +defmodule Result do + @type t(ok, error) :: {:ok, ok} | {:error, error} + + @spec map(t(a, e), (a -> b)) :: t(b, e) when a: var, b: var, e: var + def map({:ok, value}, fun), do: {:ok, fun.(value)} + def map({:error, _} = err, _fun), do: err + + @spec map_error(t(o, a), (a -> b)) :: t(o, b) when o: var, a: var, b: var + def map_error({:ok, _} = ok, _fun), do: ok + def map_error({:error, err}, fun), do: {:error, fun.(err)} +end + +# Usage with specific types +@spec fetch_user(id :: String.t()) :: Result.t(User.t(), :not_found | :timeout) +``` + +### Common Warning Patterns + +#### 1. Invalid Contracts - Return Type Mismatch + +```elixir +# ❌ Wrong - spec doesn't match all return paths +@spec process(String.t()) :: {:ok, String.t()} +def process(""), do: {:error, :empty} # Dialyzer warning! +def process(str), do: {:ok, String.upcase(str)} + +# ✅ Correct - spec matches all possible returns +@spec process(String.t()) :: {:ok, String.t()} | {:error, :empty} +def process(""), do: {:error, :empty} +def process(str), do: {:ok, String.upcase(str)} +``` + +#### 2. Function Application Arguments - Type Alignment + +```elixir +# ❌ Wrong - passing nil to String.upcase/1 which expects String.t() +@spec format(String.t() | nil) :: String.t() +def format(nil), do: "N/A" +def format(name), do: String.upcase(name) # Dialyzer warning if name could be nil + +# ✅ Correct - guard ensures String.upcase/1 only receives String.t() +@spec format(String.t() | nil) :: String.t() +def format(nil), do: "N/A" +def format(name) when is_binary(name), do: String.upcase(name) +``` + +#### 3. Opaque Type Mismatches - External Manipulation + +```elixir +# ❌ Wrong - external module accessing opaque type internals +defmodule TokenConsumer do + def check_expiry(%Token{expires_at: exp}), do: exp # Dialyzer warning! +end + +# ✅ Correct - use public API +defmodule TokenConsumer do + def check_expiry(token), do: Token.valid?(token) +end +``` + +#### 4. Range Errors - Incomplete Specs + +```elixir +# ❌ Wrong - spec doesn't account for empty list +@spec sum(list(integer())) :: integer() +def sum([]), do: nil # Dialyzer warning: returns nil, not integer +def sum(list), do: Enum.sum(list) + +# ✅ Correct - spec includes nil case or use default +@spec sum(list(integer())) :: integer() | nil +def sum([]), do: nil +def sum(list), do: Enum.sum(list) + +# ✅ Better - eliminate nil by using default +@spec sum(list(integer())) :: integer() +def sum([]), do: 0 +def sum(list), do: Enum.sum(list) +``` + +### Practical Workflow + +#### Start Broad, Narrow Gradually + +Don't over-specify types prematurely. Start with broader types and refine as needed: + +```elixir +# Start broad +@spec parse(String.t()) :: map() + +# Narrow as structure becomes clear +@spec parse(String.t()) :: %{name: String.t(), age: integer()} + +# Finalize with custom type for reusability +@type user_params :: %{name: String.t(), age: integer()} +@spec parse(String.t()) :: user_params() +``` + +#### Use Dialyzer for Gradual Adoption + +When adding Dialyzer to legacy codebases: + +1. Generate ignore file: `mix dialyzer --format ignore_file` +2. Commit `.dialyzer_ignore.exs` to baseline +3. Fix warnings incrementally, removing from ignore file +4. Eventually delete ignore file when all issues resolved + +#### Combine with Runtime Type Checking + +Dialyzer is compile-time only. For runtime safety, consider TypeCheck or contracts: + +```elixir +# Dialyzer catches compile-time type errors +@spec divide(integer(), integer()) :: float() +def divide(a, b), do: a / b + +# Runtime check prevents division by zero +@spec divide_safe(integer(), integer()) :: {:ok, float()} | {:error, :division_by_zero} +def divide_safe(_a, 0), do: {:error, :division_by_zero} +def divide_safe(a, b), do: {:ok, a / b} +``` + +### Important Reminders + +- **Success Typing**: Dialyzer only reports errors it's certain about - comprehensive testing remains essential +- **Not a Proof System**: Dialyzer won't catch all bugs, just type-related ones +- **PLT Files**: Persist type lookup tables (`priv/plts/`) in gitignore - don't commit +- **Incremental Analysis**: Use `plt_add_deps: :apps_direct` for faster rebuilds +- **Never Suppress Valid Warnings**: Fix the root cause, don't silence Dialyzer with wrong specs \ No newline at end of file diff --git a/lib/mix/tasks/geoip.import.ex b/lib/mix/tasks/geoip.import.ex index d6aa1b84..79fac0f9 100644 --- a/lib/mix/tasks/geoip.import.ex +++ b/lib/mix/tasks/geoip.import.ex @@ -323,7 +323,7 @@ defmodule Mix.Tasks.Geoip.Import do entries = Enum.map(batch, fn block -> block - |> Map.put(:id, Ecto.UUID.generate()) + |> Map.put(:id, Ecto.UUID.bingenerate()) |> Map.put(:inserted_at, now) |> Map.put(:updated_at, now) end) diff --git a/lib/towerops/api_tokens.ex b/lib/towerops/api_tokens.ex index 6c673e5c..93589565 100644 --- a/lib/towerops/api_tokens.ex +++ b/lib/towerops/api_tokens.ex @@ -26,6 +26,7 @@ defmodule Towerops.ApiTokens do iex> create_api_token(%{name: ""}) {:error, %Ecto.Changeset{}} """ + @spec create_api_token(map()) :: {:ok, {ApiToken.t(), String.t()}} | {:error, Ecto.Changeset.t()} def create_api_token(attrs \\ %{}) do # Generate a random token raw_token = generate_token() @@ -52,6 +53,7 @@ defmodule Towerops.ApiTokens do iex> list_organization_api_tokens(org_id) [%ApiToken{}, ...] """ + @spec list_organization_api_tokens(binary()) :: [ApiToken.t()] def list_organization_api_tokens(organization_id) do ApiToken |> where([t], t.organization_id == ^organization_id) @@ -67,6 +69,7 @@ defmodule Towerops.ApiTokens do iex> list_user_api_tokens(user_id) [%ApiToken{}, ...] """ + @spec list_user_api_tokens(binary()) :: [ApiToken.t()] def list_user_api_tokens(user_id) do ApiToken |> where([t], t.user_id == ^user_id) @@ -86,6 +89,7 @@ defmodule Towerops.ApiTokens do iex> get_api_token!("nonexistent") ** (Ecto.NoResultsError) """ + @spec get_api_token!(binary()) :: ApiToken.t() def get_api_token!(id), do: Repo.get!(ApiToken, id) @doc """ @@ -103,6 +107,8 @@ defmodule Towerops.ApiTokens do iex> verify_token("invalid") {:error, :invalid_token} """ + @spec verify_token(String.t()) :: + {:ok, binary(), Towerops.Accounts.User.t() | nil} | {:error, :invalid_token} def verify_token(raw_token) do token_hash = hash_token(raw_token) @@ -136,6 +142,7 @@ defmodule Towerops.ApiTokens do iex> delete_api_token(api_token) {:error, %Ecto.Changeset{}} """ + @spec delete_api_token(ApiToken.t()) :: {:ok, ApiToken.t()} | {:error, Ecto.Changeset.t()} def delete_api_token(%ApiToken{} = api_token) do Repo.delete(api_token) end @@ -151,6 +158,7 @@ defmodule Towerops.ApiTokens do iex> update_api_token(api_token, %{name: nil}) {:error, %Ecto.Changeset{}} """ + @spec update_api_token(ApiToken.t(), map()) :: {:ok, ApiToken.t()} | {:error, Ecto.Changeset.t()} def update_api_token(%ApiToken{} = api_token, attrs) do api_token |> ApiToken.changeset(attrs) @@ -159,6 +167,7 @@ defmodule Towerops.ApiTokens do # Private functions + @spec generate_token() :: String.t() defp generate_token do # Generate a 32-byte random token and encode it as base64 # Prefix with "towerops_" for easy identification @@ -166,12 +175,14 @@ defmodule Towerops.ApiTokens do "towerops_" <> Base.url_encode64(random_bytes, padding: false) end + @spec hash_token(String.t()) :: String.t() defp hash_token(token) do :sha256 |> :crypto.hash(token) |> Base.encode16(case: :lower) end + @spec update_last_used(ApiToken.t()) :: :ok defp update_last_used(token) do timestamp = DateTime.truncate(DateTime.utc_now(), :second) @@ -186,12 +197,14 @@ defmodule Towerops.ApiTokens do end end + @spec update_token_timestamp(ApiToken.t(), DateTime.t()) :: {:ok, ApiToken.t()} | {:error, Ecto.Changeset.t()} defp update_token_timestamp(token, timestamp) do token |> Ecto.Changeset.change(last_used_at: timestamp) |> Repo.update() end + @spec spawn_async_update(ApiToken.t(), DateTime.t()) :: :ok defp spawn_async_update(token, timestamp) do parent = self() diff --git a/lib/towerops/result.ex b/lib/towerops/result.ex new file mode 100644 index 00000000..d53fa588 --- /dev/null +++ b/lib/towerops/result.ex @@ -0,0 +1,129 @@ +defmodule Towerops.Result do + @moduledoc """ + Generic result type for representing success or failure outcomes. + + This module provides a type-safe way to handle operations that can succeed or fail, + using Elixir's standard {:ok, value} | {:error, reason} pattern with enhanced type safety. + + ## Type Parameters + + - `ok_value`: The type of value returned on success + - `error_value`: The type of error returned on failure + + ## Examples + + # Basic usage with string success and atom error + @spec fetch_user(id :: String.t()) :: Result.t(User.t(), :not_found | :timeout) + def fetch_user(id) do + case Repo.get(User, id) do + nil -> {:error, :not_found} + user -> {:ok, user} + end + end + + # Using helper functions + Result.map({:ok, user}, &User.display_name/1) + # => {:ok, "John Doe"} + + Result.map_error({:error, :db_error}, fn _ -> :service_unavailable end) + # => {:error, :service_unavailable} + """ + + @type t(ok_value, error_value) :: {:ok, ok_value} | {:error, error_value} + + @type t(ok_value) :: t(ok_value, term()) + + @doc """ + Maps the success value using the given function. + Error values are passed through unchanged. + + ## Examples + + iex> Result.map({:ok, 5}, fn x -> x * 2 end) + {:ok, 10} + + iex> Result.map({:error, :not_found}, fn x -> x * 2 end) + {:error, :not_found} + """ + @spec map(t(a, e), (a -> b)) :: t(b, e) when a: var, b: var, e: var + def map({:ok, value}, fun) when is_function(fun, 1), do: {:ok, fun.(value)} + def map({:error, _} = err, fun) when is_function(fun, 1), do: err + + @doc """ + Maps the error value using the given function. + Success values are passed through unchanged. + + ## Examples + + iex> Result.map_error({:error, :timeout}, fn _ -> :network_error end) + {:error, :network_error} + + iex> Result.map_error({:ok, 42}, fn _ -> :network_error end) + {:ok, 42} + """ + @spec map_error(t(o, a), (a -> b)) :: t(o, b) when o: var, a: var, b: var + def map_error({:ok, _} = ok, fun) when is_function(fun, 1), do: ok + def map_error({:error, err}, fun) when is_function(fun, 1), do: {:error, fun.(err)} + + @doc """ + Chains together operations that return results. + If the first result is an error, returns it immediately. + Otherwise, applies the function to the success value. + + ## Examples + + iex> Result.and_then({:ok, user}, fn u -> {:ok, u.email} end) + {:ok, "user@example.com"} + + iex> Result.and_then({:error, :not_found}, fn u -> {:ok, u.email} end) + {:error, :not_found} + """ + @spec and_then(t(a, e), (a -> t(b, e))) :: t(b, e) when a: var, b: var, e: var + def and_then({:ok, value}, fun) when is_function(fun, 1), do: fun.(value) + def and_then({:error, _} = err, fun) when is_function(fun, 1), do: err + + @doc """ + Unwraps a result, returning the success value or a default on error. + + ## Examples + + iex> Result.unwrap_or({:ok, 42}, 0) + 42 + + iex> Result.unwrap_or({:error, :not_found}, 0) + 0 + """ + @spec unwrap_or(t(a, term()), a) :: a when a: var + def unwrap_or({:ok, value}, _default), do: value + def unwrap_or({:error, _}, default), do: default + + @doc """ + Checks if the result is a success. + + ## Examples + + iex> Result.ok?({:ok, 42}) + true + + iex> Result.ok?({:error, :not_found}) + false + """ + @spec ok?(t(any(), any())) :: boolean() + def ok?({:ok, _}), do: true + def ok?({:error, _}), do: false + + @doc """ + Checks if the result is an error. + + ## Examples + + iex> Result.error?({:error, :not_found}) + true + + iex> Result.error?({:ok, 42}) + false + """ + @spec error?(t(any(), any())) :: boolean() + def error?({:error, _}), do: true + def error?({:ok, _}), do: false +end diff --git a/lib/towerops_web/live/account_live/my_data.ex b/lib/towerops_web/live/account_live/my_data.ex index 2aac0632..b0332108 100644 --- a/lib/towerops_web/live/account_live/my_data.ex +++ b/lib/towerops_web/live/account_live/my_data.ex @@ -526,7 +526,7 @@ defmodule ToweropsWeb.AccountLive.MyData do {log.action}

- By: {log.actor_email || "System"} + By: {if log.superuser, do: log.superuser.email, else: "System"}

{Calendar.strftime(log.inserted_at, "%B %d, %Y at %I:%M %p")} diff --git a/mix.exs b/mix.exs index c9ee30c9..ddad9312 100644 --- a/mix.exs +++ b/mix.exs @@ -91,8 +91,10 @@ defmodule Towerops.MixProject do defp dialyzer do [ plt_file: {:no_warn, "priv/plts/dialyzer.plt"}, + # Faster incremental analysis + plt_add_deps: :apps_direct, plt_add_apps: [:mix, :ex_unit], - flags: [:unmatched_returns, :error_handling, :unknown], + flags: [:unmatched_returns, :error_handling, :underspecs, :unknown], ignore_warnings: ".dialyzer_ignore.exs" ] end