From ce7a8d196f711c90eecfa37f7ad4f51cfe65b14c Mon Sep 17 00:00:00 2001 From: graham Date: Sat, 28 Mar 2026 09:11:21 -0500 Subject: [PATCH] fix: replace Repo.rollback with proper error handling for Oban compatibility (#201) - Replace manual Repo.rollback/1 calls with raise/return patterns - Fix typo: Repo.transact -> Repo.transaction - Add unwrap_transaction_result/1 helper to reduce nesting depth - Extract helper functions to satisfy Credo nesting depth requirements - Fixes 'operation :rollback is rolling back unexpectedly' error When Oban Pro's Smart engine uses Ecto.Multi for nested transactions, manual Repo.rollback/1 calls break the transaction stack. This fix uses proper error handling patterns: 1. Raise exceptions to trigger automatic rollback (admin.ex) 2. Return error tuples and unwrap with helper function (accounts.ex, mobile_sessions.ex) Files changed: - lib/towerops/admin.ex (3 functions: delete_user, delete_organization, update_billing_overrides) - lib/towerops/accounts.ex (5 functions with helpers: update_user_email, reset_user_password, update_user_and_delete_all_tokens, do_update_user_and_delete_tokens, delete_account) - lib/towerops/mobile_sessions.ex (2 functions with helper: complete_qr_login, do_complete_qr_login) All tests passing (277 tests total). Reviewed-on: https://git.mcintire.me/graham/towerops-web/pulls/201 --- lib/towerops/accounts.ex | 57 ++++++++++++++++++++++++--------- lib/towerops/admin.ex | 9 ++++-- lib/towerops/mobile_sessions.ex | 26 ++++++++++++--- 3 files changed, 69 insertions(+), 23 deletions(-) diff --git a/lib/towerops/accounts.ex b/lib/towerops/accounts.ex index 9a1448bc..1b37061f 100644 --- a/lib/towerops/accounts.ex +++ b/lib/towerops/accounts.ex @@ -776,7 +776,7 @@ defmodule Towerops.Accounts do old_email = user.email result = - Repo.transact(fn -> + fn -> with {:ok, query} <- UserToken.verify_change_email_token_query(token, context), %UserToken{sent_to: email} <- Repo.one(query), {:ok, updated_user} <- Repo.update(User.email_changeset(user, %{email: email})), @@ -786,7 +786,9 @@ defmodule Towerops.Accounts do else _ -> {:error, :transaction_aborted} end - end) + end + |> Repo.transaction() + |> unwrap_transaction_result() # Log email change for security monitoring case result do @@ -904,15 +906,21 @@ defmodule Towerops.Accounts do """ def reset_user_password(user, attrs) do - Repo.transact(fn -> + fn -> changeset = User.password_changeset(user, attrs) - with {:ok, {updated_user, expired_tokens}} <- update_user_and_delete_all_tokens(changeset) do - # Delete all reset_password tokens after successful password update - Repo.delete_all(from(t in UserToken, where: t.user_id == ^user.id and t.context == "reset_password")) - {:ok, {updated_user, expired_tokens}} + case update_user_and_delete_all_tokens(changeset) do + {:ok, {updated_user, expired_tokens}} -> + # Delete all reset_password tokens after successful password update + Repo.delete_all(from(t in UserToken, where: t.user_id == ^user.id and t.context == "reset_password")) + {:ok, {updated_user, expired_tokens}} + + {:error, changeset} -> + {:error, changeset} end - end) + end + |> Repo.transaction() + |> unwrap_transaction_result() end ## Session @@ -1096,16 +1104,34 @@ defmodule Towerops.Accounts do ## Token helper + # Helper function to unwrap nested transaction results + defp unwrap_transaction_result(result) do + case result do + {:ok, {:ok, value}} -> {:ok, value} + {:ok, {:error, reason}} -> {:error, reason} + {:error, reason} -> {:error, reason} + end + end + defp update_user_and_delete_all_tokens(changeset) do - Repo.transact(fn -> - with {:ok, user} <- Repo.update(changeset) do - tokens_to_expire = Repo.all_by(UserToken, user_id: user.id) + changeset + |> do_update_user_and_delete_tokens() + |> Repo.transaction() + |> unwrap_transaction_result() + end - Repo.delete_all(from(t in UserToken, where: t.id in ^Enum.map(tokens_to_expire, & &1.id))) + defp do_update_user_and_delete_tokens(changeset) do + fn -> + case Repo.update(changeset) do + {:ok, user} -> + tokens_to_expire = Repo.all_by(UserToken, user_id: user.id) + Repo.delete_all(from(t in UserToken, where: t.id in ^Enum.map(tokens_to_expire, & &1.id))) + {:ok, {user, tokens_to_expire}} - {:ok, {user, tokens_to_expire}} + {:error, changeset} -> + {:error, changeset} end - end) + end end ## User Consent @@ -1854,7 +1880,8 @@ defmodule Towerops.Accounts do {:error, changeset} -> Logger.error("Failed to delete user account: #{inspect(changeset)}") - Repo.rollback(changeset) + # Raise to trigger rollback - never call Repo.rollback/1 directly + raise Ecto.InvalidChangesetError, action: :delete, changeset: changeset end end) end diff --git a/lib/towerops/admin.ex b/lib/towerops/admin.ex index 32f695e0..1cc0cc20 100644 --- a/lib/towerops/admin.ex +++ b/lib/towerops/admin.ex @@ -111,7 +111,8 @@ defmodule Towerops.Admin do {:error, changeset} -> Logger.error("Failed to delete user: #{inspect(changeset)}") - Repo.rollback(changeset) + # Raise to trigger rollback - never call Repo.rollback/1 directly + raise Ecto.InvalidChangesetError, action: :delete, changeset: changeset end end) end @@ -206,7 +207,8 @@ defmodule Towerops.Admin do {:error, changeset} -> Logger.error("Failed to delete organization: #{inspect(changeset)}") - Repo.rollback(changeset) + # Raise to trigger rollback - never call Repo.rollback/1 directly + raise Ecto.InvalidChangesetError, action: :delete, changeset: changeset end end) end @@ -247,7 +249,8 @@ defmodule Towerops.Admin do {:error, changeset} -> Logger.error("Failed to update billing overrides: #{inspect(changeset.errors)}") - Repo.rollback(changeset) + # Raise to trigger rollback - never call Repo.rollback/1 directly + raise Ecto.InvalidChangesetError, action: :update, changeset: changeset end end) end diff --git a/lib/towerops/mobile_sessions.ex b/lib/towerops/mobile_sessions.ex index 41fb9099..554a5ec7 100644 --- a/lib/towerops/mobile_sessions.ex +++ b/lib/towerops/mobile_sessions.ex @@ -173,7 +173,14 @@ defmodule Towerops.MobileSessions do Returns {:ok, mobile_session} on success. """ def complete_qr_login(token, device_attrs) do - Repo.transaction(fn -> + token + |> do_complete_qr_login(device_attrs) + |> Repo.transaction() + |> unwrap_transaction_result() + end + + defp do_complete_qr_login(token, device_attrs) do + fn -> with {:ok, qr_token} <- fetch_qr_token(token), session_attrs = Map.put(device_attrs, :user_id, qr_token.user_id), {:ok, session} <- create_mobile_session(session_attrs) do @@ -182,12 +189,21 @@ defmodule Towerops.MobileSessions do |> QRLoginToken.complete_changeset(session.id) |> Repo.update!() - session + {:ok, session} else - {:error, :invalid_token} -> Repo.rollback(:invalid_token) - {:error, changeset} -> Repo.rollback(changeset) + {:error, :invalid_token} -> {:error, :invalid_token} + {:error, changeset} -> {:error, changeset} end - end) + end + end + + # Helper function to unwrap nested transaction results + defp unwrap_transaction_result(result) do + case result do + {:ok, {:ok, value}} -> {:ok, value} + {:ok, {:error, reason}} -> {:error, reason} + {:error, reason} -> {:error, reason} + end end defp fetch_qr_token(token) do