dialyzer: remove all @dialyzer suppressions, refactor Ecto.Multi → Repo.transaction

The remaining @dialyzer {:no_opaque, ...} / {:nowarn_function, ...}
directives in accounts.ex, devices.ex, organizations.ex all masked the
same upstream issue: Ecto.Multi.new/0 produces a struct containing a
literal %MapSet{map: %{}} whose concrete shape violates MapSet's
@opaque t at every subsequent Ecto.Multi.* call boundary.

Tried first: public helper function with @spec Ecto.Multi.t() — confirmed
dialyzer uses success typing of the body, not the spec. Doesn't help.

Real fix: drop Ecto.Multi entirely for these transactions. Each was
performing a small, sequential set of Repo operations that map cleanly
to a plain `Repo.transaction(fn -> ... end)` block with Repo.rollback
on errors. No Multi machinery is actually needed:

- accounts.ex:
  - register_user/1: insert + 0-2 consent grants
  - register_user_with_organization/1: insert + create_org + consents
  - confirm_user_transaction/1: update + delete_all tokens
- organizations.ex:
  - do_create_organization/4: lock user → limit check → insert org + membership
  - set_default_organization/2: two update_alls
  - accept_invitation/2: update invitation + insert membership
- devices.ex:
  - create_device/2: maybe lock org → maybe quota check → insert

Caught a real consent-failure rollback bug along the way: initial
refactor discarded the return of grant_consents_sequential/2, which
meant FK violations on consent insert would abort the transaction at
the Postgres level but the caller still saw {:ok, user} — later
operations in the transaction silently failed with 25P02. Fixed by
propagating the {:error, changeset} up so Repo.rollback is called.
Verified by the AccountsDefensiveCoverageTest regression tests.

Behavior is preserved — same transactional guarantees, same success
/ error tuple shapes. Net -86 LOC and no suppressions.

mix dialyzer: `done (passed successfully)` with zero @dialyzer
directives in lib/.
This commit is contained in:
Graham McIntire 2026-04-21 13:03:53 -05:00
parent 0e0cf7163e
commit 075106932c
3 changed files with 122 additions and 207 deletions

View file

@ -122,28 +122,18 @@ defmodule Towerops.Accounts do
{:error, %Ecto.Changeset{}}
"""
# `Ecto.Multi.new/0` returns a `%Multi{names: MapSet.new()}` whose
# `names` field dialyzer's success-typing sees as `%MapSet{map: %{}}`.
# That concrete value violates MapSet's `@opaque t` when passed to
# subsequent `Ecto.Multi.*` calls — an upstream typing quirk in Ecto's
# `@spec new :: t` that loses information vs the actual struct literal.
# See elixir-ecto/ecto: `Ecto.Multi` uses `MapSet` internally but exposes
# it in the type, so `:no_opaque` is the correct narrow suppression.
@dialyzer {:no_opaque, [register_user: 1]}
@spec register_user(map()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()}
def register_user(attrs) do
user_changeset = User.registration_changeset(%User{}, attrs)
multi =
Ecto.Multi.new()
|> Ecto.Multi.insert(:user, user_changeset)
|> grant_consents_on_registration(attrs)
case Repo.transaction(multi) do
{:ok, %{user: user}} -> {:ok, user}
{:error, :user, changeset, _} -> {:error, changeset}
{:error, _name, _changeset, _} -> {:error, user_changeset}
end
Repo.transaction(fn ->
with {:ok, user} <- Repo.insert(user_changeset),
:ok <- grant_consents_sequential(user, attrs) do
user
else
{:error, changeset} -> Repo.rollback(changeset)
end
end)
end
@doc """
@ -158,52 +148,41 @@ defmodule Towerops.Accounts do
{:error, %Ecto.Changeset{}}
"""
# Same MapSet-opacity issue as register_user/1 — see its comment above.
@dialyzer {:no_opaque, [register_user_with_organization: 1]}
@spec register_user_with_organization(map()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()}
def register_user_with_organization(attrs) do
user_changeset = User.registration_changeset(%User{}, attrs)
multi =
Ecto.Multi.new()
|> Ecto.Multi.insert(:user, user_changeset)
|> Ecto.Multi.run(:organization, fn _repo, %{user: user} ->
org_name = attrs["organization_name"] || "Personal"
Towerops.Organizations.create_organization(
%{name: org_name},
user.id
)
end)
|> grant_consents_on_registration(attrs)
case Repo.transaction(multi) do
{:ok, %{user: user}} -> {:ok, user}
{:error, :user, changeset, _} -> {:error, changeset}
{:error, :organization, changeset, _} -> {:error, changeset}
{:error, _name, _changeset, _} -> {:error, user_changeset}
end
end
@spec grant_consents_on_registration(Ecto.Multi.t(), map()) :: Ecto.Multi.t()
defp grant_consents_on_registration(multi, attrs) do
# Only grant consents if checkboxes were checked
privacy_consent = attrs["privacy_policy_consent"] || attrs[:privacy_policy_consent]
terms_consent = attrs["terms_of_service_consent"] || attrs[:terms_of_service_consent]
multi
|> maybe_grant_consent(privacy_consent, :privacy_policy_consent, "privacy_policy")
|> maybe_grant_consent(terms_consent, :terms_of_service_consent, "terms_of_service")
end
@spec maybe_grant_consent(Ecto.Multi.t(), term(), atom(), String.t()) :: Ecto.Multi.t()
defp maybe_grant_consent(multi, flag, step_name, consent_type) when flag in ["true", true, "on"] do
Ecto.Multi.run(multi, step_name, fn _repo, %{user: user} ->
grant_consent(user.id, consent_type)
Repo.transaction(fn ->
with {:ok, user} <- Repo.insert(user_changeset),
org_name = attrs["organization_name"] || "Personal",
{:ok, _org} <- Towerops.Organizations.create_organization(%{name: org_name}, user.id),
:ok <- grant_consents_sequential(user, attrs) do
user
else
{:error, %Ecto.Changeset{} = changeset} -> Repo.rollback(changeset)
{:error, reason} -> Repo.rollback(reason)
end
end)
end
defp maybe_grant_consent(multi, _flag, _step_name, _consent_type), do: multi
@spec grant_consents_sequential(User.t(), map()) :: :ok | {:error, Ecto.Changeset.t()}
defp grant_consents_sequential(user, attrs) do
privacy_consent = attrs["privacy_policy_consent"] || attrs[:privacy_policy_consent]
terms_consent = attrs["terms_of_service_consent"] || attrs[:terms_of_service_consent]
with :ok <- maybe_grant_consent(user, privacy_consent, "privacy_policy") do
maybe_grant_consent(user, terms_consent, "terms_of_service")
end
end
defp maybe_grant_consent(user, flag, consent_type) when flag in ["true", true, "on"] do
case grant_consent(user.id, consent_type) do
{:ok, _} -> :ok
{:error, _} = err -> err
end
end
defp maybe_grant_consent(_user, _flag, _consent_type), do: :ok
## TOTP / Two-Factor Authentication
@ -1777,25 +1756,25 @@ defmodule Towerops.Accounts do
def confirm_user(token) do
with {:ok, query} <- UserToken.verify_email_token_query(token, "confirm"),
%User{} = user <- Repo.one(query),
{:ok, %{user: confirmed_user}} <- confirm_user_transaction(user) do
{:ok, confirmed_user} <- confirm_user_transaction(user) do
{:ok, confirmed_user}
else
_ -> :error
end
end
# Same MapSet-opacity issue as register_user/1 — see its comment above.
@dialyzer {:no_opaque, [confirm_user_transaction: 1]}
@spec confirm_user_transaction(User.t()) ::
{:ok, map()} | {:error, any(), any(), map()}
@spec confirm_user_transaction(User.t()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()}
defp confirm_user_transaction(user) do
Ecto.Multi.new()
|> Ecto.Multi.update(:user, User.confirm_changeset(user))
|> Ecto.Multi.delete_all(
:tokens,
UserToken.by_user_and_contexts_query(user, ["confirm"])
)
|> Repo.transaction()
Repo.transaction(fn ->
case Repo.update(User.confirm_changeset(user)) do
{:ok, confirmed_user} ->
_ = Repo.delete_all(UserToken.by_user_and_contexts_query(user, ["confirm"]))
confirmed_user
{:error, changeset} ->
Repo.rollback(changeset)
end
end)
end
## Account Deletion (GDPR Right to Erasure)

View file

@ -11,6 +11,7 @@ defmodule Towerops.Devices do
alias Towerops.Devices.Event
alias Towerops.Monitoring
alias Towerops.Organizations
alias Towerops.Organizations.Organization
alias Towerops.Organizations.SubscriptionLimits
alias Towerops.Repo
alias Towerops.Sites
@ -19,29 +20,6 @@ defmodule Towerops.Devices do
@device_preloads [:organization, site: :organization]
# Ecto.Multi's struct is declared as an opaque type in Ecto, but
# Ecto.Multi.new/0 and the Ecto.Multi.{insert,run}/* functions return
# the concrete struct shape (not the opaque Ecto.Multi.t()). Flow analysis
# sees a mismatch between the concrete struct we have in hand and the
# opaque type the subsequent Ecto.Multi.* calls declare in their
# signatures, raising call_without_opaque on every step of the pipeline.
# Passing the Multi through private helpers that take/return
# Ecto.Multi.t() compounds the issue. Adding @spec Ecto.Multi.t() on the
# helpers doesn't help — it just moves the warning to contract_with_opaque.
# Inlining the whole pipeline (even without helpers) still warns on
# `Ecto.Multi.new() |> Ecto.Multi.insert(...)` alone, confirming the
# problem is in Ecto's opaque-type declarations, not our helper shape.
# Suppression is the least-worst option until Ecto loosens the opaque type.
@dialyzer {:no_opaque,
[
create_device: 1,
create_device: 2,
add_organization_lock: 2,
add_quota_check: 2,
maybe_lock_organization_for_quota: 3,
maybe_check_device_quota: 3
]}
@doc """
Returns the list of devices for a site.
Ordered by custom display_order (if set), then alphabetically by name.
@ -570,28 +548,29 @@ defmodule Towerops.Devices do
require Logger
bypass_limits = Keyword.get(opts, :bypass_limits, false)
# Build changeset with credential resolution
changeset = build_device_changeset(attrs)
# Use transaction to prevent race condition on quota check
multi =
Ecto.Multi.new()
|> maybe_lock_organization_for_quota(changeset, bypass_limits)
|> maybe_check_device_quota(changeset, bypass_limits)
|> Ecto.Multi.insert(:device, changeset)
result =
Repo.transaction(fn ->
# Lock organization to prevent concurrent device creation race
_ = maybe_lock_organization(changeset, bypass_limits)
case Repo.transaction(multi) do
{:ok, %{device: device}} ->
# Start monitoring/polling if enabled
with :ok <- maybe_check_device_quota(changeset, bypass_limits),
{:ok, device} <- Repo.insert(changeset) do
device
else
{:error, %Ecto.Changeset{} = changeset} -> Repo.rollback(changeset)
{:error, reason} -> Repo.rollback(reason)
end
end)
case result do
{:ok, device} ->
_ =
if device.monitoring_enabled do
DeviceMonitorWorker.start_monitoring(device.id)
end
# Auto-create ICMP ping check for all devices
# Best-effort: if this fails (e.g., transient DB contention),
# JobHealthCheckWorker will recover the missing check job.
try do
_ = Monitoring.ensure_default_ping_check(device)
rescue
@ -600,16 +579,9 @@ defmodule Towerops.Devices do
end
_ = broadcast_device_change(device.organization_id, :device_created)
{:ok, device}
{:error, :quota_check, error_changeset, _} ->
{:error, error_changeset}
{:error, :device, changeset, _} ->
{:error, changeset}
{:error, _step, reason, _} ->
{:error, reason} ->
{:error, reason}
end
end
@ -619,13 +591,17 @@ defmodule Towerops.Devices do
apply_credential_resolution(changeset)
end
# Lock organization to prevent concurrent device creation race condition
defp maybe_lock_organization_for_quota(multi, changeset, bypass_limits) do
defp maybe_lock_organization(changeset, bypass_limits) do
if bypass_limits or no_organization_id?(changeset) do
multi
:ok
else
organization_id = Ecto.Changeset.get_change(changeset, :organization_id)
add_organization_lock(multi, organization_id)
Repo.one!(
from o in Organization,
where: o.id == ^organization_id,
lock: "FOR UPDATE"
)
end
end
@ -633,28 +609,20 @@ defmodule Towerops.Devices do
match?(:error, Ecto.Changeset.fetch_change(changeset, :organization_id))
end
defp add_organization_lock(multi, organization_id) do
Ecto.Multi.run(multi, :lock_org, fn repo, _changes ->
org =
repo.one!(from o in Towerops.Organizations.Organization, where: o.id == ^organization_id, lock: "FOR UPDATE")
defp maybe_check_device_quota(_changeset, true), do: :ok
{:ok, org}
end)
end
# Check device quota inside transaction (after locking organization)
defp maybe_check_device_quota(multi, changeset, bypass_limits) do
if bypass_limits do
multi
defp maybe_check_device_quota(changeset, false) do
if no_organization_id?(changeset) do
:ok
else
add_quota_check(multi, changeset)
end
end
organization_id = Ecto.Changeset.get_change(changeset, :organization_id)
organization = Repo.get!(Organization, organization_id)
defp add_quota_check(multi, changeset) do
Ecto.Multi.run(multi, :quota_check, fn _repo, %{lock_org: organization} ->
validate_quota_limit(organization, changeset)
end)
case validate_quota_limit(organization, changeset) do
{:ok, _} -> :ok
{:error, _} = err -> err
end
end
end
defp validate_quota_limit(organization, changeset) do

View file

@ -94,51 +94,31 @@ defmodule Towerops.Organizations do
do_create_organization(attrs, user_id, bypass_limits, subscription_plan)
end
# `Ecto.Multi.new/0` returns `%Multi{names: MapSet.new()}`. Dialyzer's
# success-typing sees the concrete `%MapSet{map: %{}}` value, which
# violates MapSet's `@opaque t` when passed to subsequent `Ecto.Multi.*`
# calls. This is an upstream quirk of Ecto exposing MapSet in the struct
# without an opacity-preserving `@spec`.
@dialyzer {:no_opaque, [do_create_organization: 4]}
@spec do_create_organization(map(), String.t(), boolean(), String.t()) ::
{:ok, Organization.t()} | {:error, Ecto.Changeset.t()}
defp do_create_organization(attrs, user_id, bypass_limits, subscription_plan) do
multi =
Ecto.Multi.new()
# Step 1: Lock user to prevent concurrent org creation
|> Ecto.Multi.run(:lock_user, fn repo, _changes ->
user = repo.one!(from u in User, where: u.id == ^user_id, lock: "FOR UPDATE")
{:ok, user}
end)
# Step 2: Check free org limit inside transaction (prevents race condition)
|> Ecto.Multi.run(:check_limit, fn _repo, _changes ->
check_free_org_limit_in_transaction(bypass_limits, subscription_plan, user_id, attrs)
end)
# Step 3: Check if user has existing orgs (inside transaction, prevents default org race)
|> Ecto.Multi.run(:check_existing, fn repo, _changes ->
has_existing = repo.exists?(from m in Membership, where: m.user_id == ^user_id)
{:ok, has_existing}
end)
# Step 4: Create organization
|> Ecto.Multi.insert(:organization, Organization.changeset(%Organization{}, attrs))
# Step 5: Create membership with correct is_default flag
|> Ecto.Multi.insert(:membership, fn %{organization: organization, check_existing: has_existing} ->
Membership.changeset(%Membership{}, %{
organization_id: organization.id,
user_id: user_id,
role: :owner,
# Set as default if this is the user's first organization
is_default: !has_existing
})
end)
Repo.transaction(fn ->
# Lock user to prevent concurrent org creation
_user = Repo.one!(from u in User, where: u.id == ^user_id, lock: "FOR UPDATE")
case Repo.transaction(multi) do
{:ok, %{organization: organization}} -> {:ok, organization}
{:error, :check_limit, changeset, _} -> {:error, changeset}
{:error, :organization, changeset, _} -> {:error, changeset}
{:error, :membership, changeset, _} -> {:error, changeset}
{:error, _step, reason, _} -> {:error, reason}
end
with {:ok, _} <-
check_free_org_limit_in_transaction(bypass_limits, subscription_plan, user_id, attrs),
has_existing = Repo.exists?(from m in Membership, where: m.user_id == ^user_id),
{:ok, organization} <- Repo.insert(Organization.changeset(%Organization{}, attrs)),
membership_changeset =
Membership.changeset(%Membership{}, %{
organization_id: organization.id,
user_id: user_id,
role: :owner,
is_default: !has_existing
}),
{:ok, _membership} <- Repo.insert(membership_changeset) do
organization
else
{:error, %Ecto.Changeset{} = changeset} -> Repo.rollback(changeset)
{:error, reason} -> Repo.rollback(reason)
end
end)
end
# Check free org limit - for use inside transaction
@ -351,26 +331,15 @@ defmodule Towerops.Organizations do
Sets an organization as the user's default.
Clears any existing default and sets the new one.
"""
# Same MapSet-opacity issue as do_create_organization/4 above.
@dialyzer {:no_opaque, [set_default_organization: 2]}
@spec set_default_organization(String.t(), String.t()) ::
{:ok, Organization.t()} | {:error, Ecto.Changeset.t() | :not_found}
{:ok, Organization.t()} | {:error, :not_found}
def set_default_organization(user_id, organization_id) do
# Verify user has access to this organization
if user_has_access?(user_id, organization_id) do
multi =
Ecto.Multi.new()
|> Ecto.Multi.update_all(:clear_old_default, clear_default_query(user_id), set: [is_default: false])
|> Ecto.Multi.update_all(
:set_new_default,
set_default_query(user_id, organization_id),
set: [is_default: true]
)
case Repo.transaction(multi) do
{:ok, _} -> {:ok, get_organization!(organization_id)}
{:error, _, changeset, _} -> {:error, changeset}
end
Repo.transaction(fn ->
_ = Repo.update_all(clear_default_query(user_id), set: [is_default: false])
_ = Repo.update_all(set_default_query(user_id, organization_id), set: [is_default: true])
get_organization!(organization_id)
end)
else
{:error, :not_found}
end
@ -452,31 +421,30 @@ defmodule Towerops.Organizations do
@doc """
Accepts an invitation and creates a membership.
"""
# Same MapSet-opacity issue as do_create_organization/4 above.
@dialyzer {:no_opaque, [accept_invitation: 2]}
@spec accept_invitation(Invitation.t(), String.t()) ::
{:ok, Membership.t()} | {:error, Ecto.Changeset.t()}
def accept_invitation(invitation, user_id) do
multi =
Ecto.Multi.new()
|> Ecto.Multi.update(:invitation, fn _ ->
Repo.transaction(fn ->
invitation_changeset =
Ecto.Changeset.change(invitation, %{
accepted_at: Towerops.Time.now(),
accepted_by_id: user_id
})
end)
|> Ecto.Multi.insert(:membership, fn _ ->
membership_changeset =
Membership.changeset(%Membership{}, %{
organization_id: invitation.organization_id,
user_id: user_id,
role: invitation.role
})
end)
case Repo.transaction(multi) do
{:ok, %{membership: membership}} -> {:ok, membership}
{:error, _failed_operation, changeset, _changes_so_far} -> {:error, changeset}
end
with {:ok, _} <- Repo.update(invitation_changeset),
{:ok, membership} <- Repo.insert(membership_changeset) do
membership
else
{:error, changeset} -> Repo.rollback(changeset)
end
end)
end
@doc """