From 1f0102fe3b4eb7893692b63a192eb4018827309c Mon Sep 17 00:00:00 2001 From: Ethan Swan Date: Wed, 4 Feb 2026 21:24:54 -0600 Subject: [PATCH] Add withRLSAction helper for clean ServerActionResult early returns Introduces withRLSAction, a variant of withRLS that allows transaction callbacks to return ServerActionResult directly. This enables clean `return error(...)` early returns inside transactions while maintaining RLS context and transactional atomicity. Key changes: - Add withRLSAction helper with automatic rollback on error results - Refactor 9 functions across 4 files to use the new pattern: - competition-members.ts: removeCompetitionMember, updateMemberRole, getEligibleMembers, addCompetitionMemberById - props.ts: createProp, resolveProp - competitions.ts: updateCompetition - forecasts.ts: createForecast, updateForecast - Consolidate multi-step operations into single atomic transactions - Eliminate awkward throw/catch string-prefix error handling pattern Closes #105 Co-Authored-By: Claude Opus 4.5 --- lib/db-helpers.test.ts | 37 ++++ lib/db-helpers.ts | 59 ++++++ lib/db_actions/competition-members.ts | 256 +++++++++++++------------- lib/db_actions/competitions.ts | 148 ++++++++------- lib/db_actions/forecasts.ts | 90 +++++---- lib/db_actions/forecasts.unit.test.ts | 21 ++- lib/db_actions/props.test.ts | 23 +-- lib/db_actions/props.ts | 91 +++++---- 8 files changed, 406 insertions(+), 319 deletions(-) diff --git a/lib/db-helpers.test.ts b/lib/db-helpers.test.ts index 1ce4ee69..bdbbde3e 100644 --- a/lib/db-helpers.test.ts +++ b/lib/db-helpers.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { db } from "@/lib/database"; +import { success } from "@/lib/server-action-result"; // Mock the database module vi.mock("@/lib/database", () => ({ @@ -47,3 +48,39 @@ describe("withRLS", () => { ); }); }); + +describe("withRLSAction", () => { + const mockExecute = vi.fn(); + + beforeEach(() => { + vi.clearAllMocks(); + + vi.mocked(db.transaction).mockReturnValue({ + execute: mockExecute, + } as any); + }); + + it("should start a transaction", async () => { + const { withRLSAction } = await import("./db-helpers"); + + mockExecute.mockRejectedValue(new Error("Expected test error")); + + try { + await withRLSAction(123, async () => success("result")); + } catch { + // Expected - sql template compilation fails in test environment + } + + expect(db.transaction).toHaveBeenCalled(); + }); + + it("should propagate transaction-level errors", async () => { + const { withRLSAction } = await import("./db-helpers"); + + mockExecute.mockRejectedValue(new Error("Connection refused")); + + await expect( + withRLSAction(123, async () => success("result")), + ).rejects.toThrow("Connection refused"); + }); +}); diff --git a/lib/db-helpers.ts b/lib/db-helpers.ts index 76134097..c31fa180 100644 --- a/lib/db-helpers.ts +++ b/lib/db-helpers.ts @@ -1,6 +1,7 @@ import { Database } from "@/types/db_types"; import { db } from "@/lib/database"; import { Transaction, sql } from "kysely"; +import type { ServerActionResult } from "@/lib/server-action-result"; /** * Executes a database operation within a transaction with Row Level Security (RLS) context. @@ -30,3 +31,61 @@ export async function withRLS( return fn(trx); }); } + +/** + * Sentinel error used to trigger transaction rollback when the callback + * returns a ServerActionResult with success: false. + */ +class RollbackWithResult extends Error { + constructor(public result: ServerActionResult) { + super("rollback"); + } +} + +/** + * Like `withRLS`, but the callback returns a `ServerActionResult` directly. + * + * This allows clean early `return error(...)` inside the transaction while + * preserving RLS context and transactional atomicity. If the callback returns + * an error result, the transaction is automatically rolled back. + * + * @example + * const result = await withRLSAction(currentUser.id, async (trx) => { + * const membership = await trx.selectFrom("competition_members") + * .select("role") + * .where("competition_id", "=", competitionId) + * .where("user_id", "=", currentUser.id) + * .executeTakeFirst(); + * + * if (membership?.role !== "admin") { + * return error("Only admins can do this", ERROR_CODES.UNAUTHORIZED); + * } + * + * const inserted = await trx.insertInto("competition_members").values(...).returningAll().executeTakeFirstOrThrow(); + * return success(inserted); + * }); + */ +export async function withRLSAction( + userId: number | undefined, + fn: (trx: Transaction) => Promise>, +): Promise> { + try { + return await db.transaction().execute(async (trx) => { + await trx.executeQuery( + sql`SELECT set_config('app.current_user_id', ${userId}, true);`.compile( + db, + ), + ); + const result = await fn(trx); + if (!result.success) { + throw new RollbackWithResult(result); + } + return result; + }); + } catch (err) { + if (err instanceof RollbackWithResult) { + return err.result as ServerActionResult; + } + throw err; + } +} diff --git a/lib/db_actions/competition-members.ts b/lib/db_actions/competition-members.ts index 55f4b5b5..f14b83e2 100644 --- a/lib/db_actions/competition-members.ts +++ b/lib/db_actions/competition-members.ts @@ -1,10 +1,8 @@ "use server"; -import { db } from "@/lib/database"; -import { withRLS } from "@/lib/db-helpers"; +import { withRLS, withRLSAction } from "@/lib/db-helpers"; import { CompetitionMember, - NewCompetitionMember, VCompetitionMember, } from "@/types/db_types"; import { getUserFromCookies } from "../get-user"; @@ -145,7 +143,7 @@ export async function removeCompetitionMember({ return error("You must be logged in", ERROR_CODES.UNAUTHORIZED); } - await withRLS(currentUser.id, async (trx) => { + const result = await withRLSAction(currentUser.id, async (trx) => { // Check if current user is an admin of this competition const currentUserMembership = await trx .selectFrom("competition_members") @@ -160,7 +158,10 @@ export async function removeCompetitionMember({ currentUserId: currentUser.id, currentRole: currentUserMembership?.role, }); - throw new Error("UNAUTHORIZED: Only competition admins can remove members"); + return error( + "Only competition admins can remove members", + ERROR_CODES.UNAUTHORIZED, + ); } // If removing self, check that they're not the last admin @@ -173,7 +174,10 @@ export async function removeCompetitionMember({ .executeTakeFirst(); if (Number(adminCount?.count) <= 1) { - throw new Error("VALIDATION: Cannot remove the last admin from a competition"); + return error( + "Cannot remove the last admin from a competition", + ERROR_CODES.VALIDATION_ERROR, + ); } } @@ -184,33 +188,29 @@ export async function removeCompetitionMember({ .executeTakeFirst(); if (Number(deleteResult.numDeletedRows) === 0) { - throw new Error("NOT_FOUND: Member not found in this competition"); + return error( + "Member not found in this competition", + ERROR_CODES.NOT_FOUND, + ); } - }); - const duration = Date.now() - startTime; - logger.info("Competition member removed successfully", { - operation: "removeCompetitionMember", - table: "competition_members", - competitionId, - removedUserId: userId, - duration, + return success(undefined); }); - revalidatePath(`/competitions/${competitionId}`); + if (result.success) { + const duration = Date.now() - startTime; + logger.info("Competition member removed successfully", { + operation: "removeCompetitionMember", + table: "competition_members", + competitionId, + removedUserId: userId, + duration, + }); + revalidatePath(`/competitions/${competitionId}`); + } - return success(undefined); + return result; } catch (err) { - const errMessage = (err as Error).message; - if (errMessage.startsWith("UNAUTHORIZED:")) { - return error(errMessage.replace("UNAUTHORIZED: ", ""), ERROR_CODES.UNAUTHORIZED); - } - if (errMessage.startsWith("VALIDATION:")) { - return error(errMessage.replace("VALIDATION: ", ""), ERROR_CODES.VALIDATION_ERROR); - } - if (errMessage.startsWith("NOT_FOUND:")) { - return error(errMessage.replace("NOT_FOUND: ", ""), ERROR_CODES.NOT_FOUND); - } const duration = Date.now() - startTime; logger.error("Failed to remove competition member", err as Error, { operation: "removeCompetitionMember", @@ -251,7 +251,7 @@ export async function updateMemberRole({ return error("You must be logged in", ERROR_CODES.UNAUTHORIZED); } - await withRLS(currentUser.id, async (trx) => { + const result = await withRLSAction(currentUser.id, async (trx) => { // Check if current user is an admin of this competition const currentUserMembership = await trx .selectFrom("competition_members") @@ -266,7 +266,10 @@ export async function updateMemberRole({ currentUserId: currentUser.id, currentRole: currentUserMembership?.role, }); - throw new Error("UNAUTHORIZED: Only competition admins can update roles"); + return error( + "Only competition admins can update roles", + ERROR_CODES.UNAUTHORIZED, + ); } // Get the member's current role @@ -278,7 +281,10 @@ export async function updateMemberRole({ .executeTakeFirst(); if (!targetMembership) { - throw new Error("NOT_FOUND: Member not found in this competition"); + return error( + "Member not found in this competition", + ERROR_CODES.NOT_FOUND, + ); } // If demoting an admin, check that they're not the last admin @@ -291,7 +297,10 @@ export async function updateMemberRole({ .executeTakeFirst(); if (Number(adminCount?.count) <= 1) { - throw new Error("VALIDATION: Cannot demote the last admin"); + return error( + "Cannot demote the last admin", + ERROR_CODES.VALIDATION_ERROR, + ); } } @@ -301,32 +310,25 @@ export async function updateMemberRole({ .where("competition_id", "=", competitionId) .where("user_id", "=", userId) .execute(); - }); - const duration = Date.now() - startTime; - logger.info("Competition member role updated successfully", { - operation: "updateMemberRole", - table: "competition_members", - competitionId, - userId, - newRole, - duration, + return success(undefined); }); - revalidatePath(`/competitions/${competitionId}`); + if (result.success) { + const duration = Date.now() - startTime; + logger.info("Competition member role updated successfully", { + operation: "updateMemberRole", + table: "competition_members", + competitionId, + userId, + newRole, + duration, + }); + revalidatePath(`/competitions/${competitionId}`); + } - return success(undefined); + return result; } catch (err) { - const errMessage = (err as Error).message; - if (errMessage.startsWith("UNAUTHORIZED:")) { - return error(errMessage.replace("UNAUTHORIZED: ", ""), ERROR_CODES.UNAUTHORIZED); - } - if (errMessage.startsWith("VALIDATION:")) { - return error(errMessage.replace("VALIDATION: ", ""), ERROR_CODES.VALIDATION_ERROR); - } - if (errMessage.startsWith("NOT_FOUND:")) { - return error(errMessage.replace("NOT_FOUND: ", ""), ERROR_CODES.NOT_FOUND); - } const duration = Date.now() - startTime; logger.error("Failed to update member role", err as Error, { operation: "updateMemberRole", @@ -406,50 +408,52 @@ export async function getEligibleMembers( return error("You must be logged in", ERROR_CODES.UNAUTHORIZED); } - // Only competition admins (or system admins) can view eligible members - if (!currentUser.is_admin) { - const membership = await withRLS(currentUser.id, async (trx) => - trx + const result = await withRLSAction(currentUser.id, async (trx) => { + // Only competition admins (or system admins) can view eligible members + if (!currentUser.is_admin) { + const membership = await trx .selectFrom("competition_members") .select("role") .where("competition_id", "=", competitionId) .where("user_id", "=", currentUser.id) - .executeTakeFirst(), - ); + .executeTakeFirst(); - if (membership?.role !== "admin") { - return error( - "Only competition admins can view eligible members", - ERROR_CODES.UNAUTHORIZED, - ); + if (membership?.role !== "admin") { + return error( + "Only competition admins can view eligible members", + ERROR_CODES.UNAUTHORIZED, + ); + } } - } - const users = await withRLS(currentUser.id, async (trx) => { // Get IDs of existing members const existingMemberIds = trx .selectFrom("competition_members") .select("user_id") .where("competition_id", "=", competitionId); - return trx + const users = await trx .selectFrom("users") .select(["id", "name", "username"]) .where("deactivated_at", "is", null) .where("id", "not in", existingMemberIds) .orderBy("name", "asc") .execute(); - }); - const duration = Date.now() - startTime; - logger.debug(`Retrieved ${users.length} eligible members`, { - operation: "getEligibleMembers", - table: "users", - competitionId, - duration, + return success(users); }); - return success(users); + if (result.success) { + const duration = Date.now() - startTime; + logger.debug(`Retrieved ${result.data.length} eligible members`, { + operation: "getEligibleMembers", + table: "users", + competitionId, + duration, + }); + } + + return result; } catch (err) { const duration = Date.now() - startTime; logger.error("Failed to get eligible members", err as Error, { @@ -489,85 +493,81 @@ export async function addCompetitionMemberById({ return error("You must be logged in", ERROR_CODES.UNAUTHORIZED); } - // Only competition admins (or system admins) can add members - if (!currentUser.is_admin) { - const currentUserMembership = await withRLS(currentUser.id, async (trx) => - trx + const result = await withRLSAction(currentUser.id, async (trx) => { + // Only competition admins (or system admins) can add members + if (!currentUser.is_admin) { + const currentUserMembership = await trx .selectFrom("competition_members") .select("role") .where("competition_id", "=", competitionId) .where("user_id", "=", currentUser.id) - .executeTakeFirst(), - ); + .executeTakeFirst(); - if (currentUserMembership?.role !== "admin") { - return error( - "Only competition admins can add members", - ERROR_CODES.UNAUTHORIZED, - ); + if (currentUserMembership?.role !== "admin") { + return error( + "Only competition admins can add members", + ERROR_CODES.UNAUTHORIZED, + ); + } } - } - // Verify the target user exists and is active - const userToAdd = await withRLS(currentUser.id, async (trx) => - trx + // Verify the target user exists and is active + const userToAdd = await trx .selectFrom("users") .select("id") .where("id", "=", userId) .where("deactivated_at", "is", null) - .executeTakeFirst(), - ); + .executeTakeFirst(); - if (!userToAdd) { - return error( - "No active user found with that ID", - ERROR_CODES.NOT_FOUND, - ); - } + if (!userToAdd) { + return error( + "No active user found with that ID", + ERROR_CODES.NOT_FOUND, + ); + } - // Check if user is already a member - const existingMembership = await withRLS(currentUser.id, async (trx) => - trx + // Check if user is already a member + const existingMembership = await trx .selectFrom("competition_members") .select("id") .where("competition_id", "=", competitionId) .where("user_id", "=", userId) - .executeTakeFirst(), - ); - - if (existingMembership) { - return error( - "User is already a member of this competition", - ERROR_CODES.VALIDATION_ERROR, - ); - } + .executeTakeFirst(); - const newMember: NewCompetitionMember = { - competition_id: competitionId, - user_id: userId, - role, - }; + if (existingMembership) { + return error( + "User is already a member of this competition", + ERROR_CODES.VALIDATION_ERROR, + ); + } - const inserted = await withRLS(currentUser.id, async (trx) => { - return trx + const inserted = await trx .insertInto("competition_members") - .values(newMember) + .values({ + competition_id: competitionId, + user_id: userId, + role, + }) .returningAll() .executeTakeFirstOrThrow(); - }); - const duration = Date.now() - startTime; - logger.info("Competition member added successfully", { - operation: "addCompetitionMemberById", - table: "competition_members", - competitionId, - addedUserId: inserted.user_id, - role, - duration, + return success(inserted); }); - revalidatePath(`/competitions/${competitionId}`); - return success(inserted); + if (result.success) { + const duration = Date.now() - startTime; + logger.info("Competition member added successfully", { + operation: "addCompetitionMemberById", + table: "competition_members", + competitionId, + addedUserId: result.data.user_id, + role, + duration, + }); + revalidatePath(`/competitions/${competitionId}`); + } + + return result; } catch (err) { const duration = Date.now() - startTime; logger.error("Failed to add competition member by ID", err as Error, { diff --git a/lib/db_actions/competitions.ts b/lib/db_actions/competitions.ts index c91b6f70..46d4773f 100644 --- a/lib/db_actions/competitions.ts +++ b/lib/db_actions/competitions.ts @@ -1,7 +1,6 @@ "use server"; -import { db } from "@/lib/database"; -import { withRLS } from "@/lib/db-helpers"; +import { withRLS, withRLSAction } from "@/lib/db-helpers"; import { Competition, CompetitionUpdate, @@ -156,103 +155,102 @@ export async function updateCompetition({ return error("You must be logged in", ERROR_CODES.UNAUTHORIZED); } - // System admins can update any competition. - // Competition admins can update their own private competitions. - if (!currentUser.is_admin) { - const membership = await withRLS(currentUser.id, async (trx) => - trx + // Prevent changing is_private — conversions are not supported yet. + if ("is_private" in competition) { + delete (competition as Record)["is_private"]; + } + + const result = await withRLSAction(currentUser.id, async (trx) => { + // System admins can update any competition. + // Competition admins can update their own private competitions. + if (!currentUser.is_admin) { + const membership = await trx .selectFrom("competition_members") .select("role") .where("competition_id", "=", id) .where("user_id", "=", currentUser.id) - .executeTakeFirst(), - ); + .executeTakeFirst(); - const comp = await withRLS(currentUser.id, async (trx) => - trx + const comp = await trx .selectFrom("competitions") .select("is_private") .where("id", "=", id) - .executeTakeFirst(), - ); + .executeTakeFirst(); - if (!comp?.is_private || membership?.role !== "admin") { - logger.warn("Unauthorized attempt to update competition", { - competitionId: id, - currentUserId: currentUser.id, - }); - return error( - "Only admins can update competitions", - ERROR_CODES.UNAUTHORIZED, - ); + if (!comp?.is_private || membership?.role !== "admin") { + logger.warn("Unauthorized attempt to update competition", { + competitionId: id, + currentUserId: currentUser.id, + }); + return error( + "Only admins can update competitions", + ERROR_CODES.UNAUTHORIZED, + ); + } } - } - // Prevent changing is_private — conversions are not supported yet. - if ("is_private" in competition) { - delete (competition as Record)["is_private"]; - } - - // If any of the date fields are being changed, validate ordering using the - // new values overlaid on the existing row. - if ( - "forecasts_open_date" in competition || - "forecasts_close_date" in competition || - "end_date" in competition - ) { - const existing = await withRLS(currentUser.id, async (trx) => - trx + // If any of the date fields are being changed, validate ordering using the + // new values overlaid on the existing row. + if ( + "forecasts_open_date" in competition || + "forecasts_close_date" in competition || + "end_date" in competition + ) { + const existing = await trx .selectFrom("competitions") .select(["forecasts_open_date", "forecasts_close_date", "end_date"]) .where("id", "=", id) - .executeTakeFirst(), - ); - if (!existing) { - return error("Competition not found", ERROR_CODES.NOT_FOUND); - } - const openDate = - "forecasts_open_date" in competition && - competition.forecasts_open_date !== undefined - ? competition.forecasts_open_date - : existing.forecasts_open_date; - const closeDate = - "forecasts_close_date" in competition && - competition.forecasts_close_date !== undefined - ? competition.forecasts_close_date - : existing.forecasts_close_date; - const endDate = - "end_date" in competition && competition.end_date !== undefined - ? competition.end_date - : existing.end_date; - const validationResult = validateCompetitionDates({ - forecasts_open_date: openDate, - forecasts_close_date: closeDate, - end_date: endDate, - }); - if (!validationResult.success) { - return validationResult; + .executeTakeFirst(); + if (!existing) { + return error("Competition not found", ERROR_CODES.NOT_FOUND); + } + const openDate = + "forecasts_open_date" in competition && + competition.forecasts_open_date !== undefined + ? competition.forecasts_open_date + : existing.forecasts_open_date; + const closeDate = + "forecasts_close_date" in competition && + competition.forecasts_close_date !== undefined + ? competition.forecasts_close_date + : existing.forecasts_close_date; + const endDate = + "end_date" in competition && competition.end_date !== undefined + ? competition.end_date + : existing.end_date; + const validationResult = validateCompetitionDates({ + forecasts_open_date: openDate, + forecasts_close_date: closeDate, + end_date: endDate, + }); + if (!validationResult.success) { + return validationResult; + } } - } - await withRLS(currentUser.id, async (trx) => { await trx .updateTable("competitions") .set(competition) .where("id", "=", id) .execute(); - }); - const duration = Date.now() - startTime; - logger.info("Competition updated successfully", { - operation: "updateCompetition", - table: "competitions", - competitionId: id, - duration, + return success(undefined); }); - revalidatePath("/competitions"); - revalidatePath("/admin/competitions"); - return success(undefined); + if (result.success) { + const duration = Date.now() - startTime; + logger.info("Competition updated successfully", { + operation: "updateCompetition", + table: "competitions", + competitionId: id, + duration, + }); + + revalidatePath("/competitions"); + revalidatePath("/admin/competitions"); + } + + return result; } catch (err) { const duration = Date.now() - startTime; logger.error("Failed to update competition", err as Error, { diff --git a/lib/db_actions/forecasts.ts b/lib/db_actions/forecasts.ts index 36c60b40..d116697f 100644 --- a/lib/db_actions/forecasts.ts +++ b/lib/db_actions/forecasts.ts @@ -11,7 +11,7 @@ import { error, ERROR_CODES, } from "@/lib/server-action-result"; -import { withRLS } from "@/lib/db-helpers"; +import { withRLS, withRLSAction } from "@/lib/db-helpers"; export type VForecastsOrderByExpression = OrderByExpression< Database, @@ -120,7 +120,7 @@ export async function createForecast({ const startTime = Date.now(); try { // Check that the competition hasn't ended, then insert the record. - const id = await withRLS(currentUser?.id, async (trx) => { + const result = await withRLSAction(currentUser?.id, async (trx) => { const prop = await trx .selectFrom("v_props") .where("prop_id", "=", forecast.prop_id) @@ -132,7 +132,10 @@ export async function createForecast({ propId: forecast.prop_id, dueDate: closeDate.toISOString(), }); - throw new Error("Cannot create forecasts past the due date"); + return error( + "Cannot create forecasts past the due date", + ERROR_CODES.VALIDATION_ERROR, + ); } const { id } = await trx @@ -140,22 +143,25 @@ export async function createForecast({ .values(forecast) .returning("id") .executeTakeFirstOrThrow(); - return id; + return success(id); }); - const duration = Date.now() - startTime; - logger.info("Forecast created successfully", { - operation: "createForecast", - table: "forecasts", - forecastId: id, - propId: forecast.prop_id, - userId: forecast.user_id, - duration, - }); + if (result.success) { + const duration = Date.now() - startTime; + logger.info("Forecast created successfully", { + operation: "createForecast", + table: "forecasts", + forecastId: result.data, + propId: forecast.prop_id, + userId: forecast.user_id, + duration, + }); - revalidatePath("/competitions"); - revalidatePath("/standalone/forecasts"); - return success(id); + revalidatePath("/competitions"); + revalidatePath("/standalone/forecasts"); + } + + return result; } catch (err) { const duration = Date.now() - startTime; logger.error("Failed to create forecast", err as Error, { @@ -165,13 +171,6 @@ export async function createForecast({ userId: forecast.user_id, duration, }); - // Check if it's the validation error we threw - if ( - err instanceof Error && - err.message === "Cannot create forecasts past the due date" - ) { - return error(err.message, ERROR_CODES.VALIDATION_ERROR); - } return error("Failed to create forecast", ERROR_CODES.DATABASE_ERROR); } } @@ -203,7 +202,7 @@ export async function updateForecast({ } // Check that the competition hasn't ended, then update the record. - await withRLS(currentUser?.id, async (trx) => { + const result = await withRLSAction(currentUser?.id, async (trx) => { // Check close date via v_forecasts (which joins with competition data) const forecastRecord = await trx .selectFrom("v_forecasts") @@ -212,7 +211,7 @@ export async function updateForecast({ .executeTakeFirst(); if (!forecastRecord) { - throw new Error("Forecast not found"); + return error("Forecast not found", ERROR_CODES.VALIDATION_ERROR); } const closeDate = forecastRecord.competition_forecasts_close_date; @@ -221,7 +220,10 @@ export async function updateForecast({ forecastId: id, dueDate: closeDate.toISOString(), }); - throw new Error("Cannot update forecasts past the due date"); + return error( + "Cannot update forecasts past the due date", + ERROR_CODES.VALIDATION_ERROR, + ); } await trx @@ -229,20 +231,25 @@ export async function updateForecast({ .set(forecast) .where("id", "=", id) .execute(); - }); - const duration = Date.now() - startTime; - logger.debug("Forecast updated successfully", { - operation: "updateForecast", - table: "forecasts", - forecastId: id, - currentUserId: currentUser?.id, - duration, + return success(undefined); }); - revalidatePath("/competitions"); - revalidatePath("/standalone/forecasts"); - return success(undefined); + if (result.success) { + const duration = Date.now() - startTime; + logger.debug("Forecast updated successfully", { + operation: "updateForecast", + table: "forecasts", + forecastId: id, + currentUserId: currentUser?.id, + duration, + }); + + revalidatePath("/competitions"); + revalidatePath("/standalone/forecasts"); + } + + return result; } catch (err) { const duration = Date.now() - startTime; logger.error("Failed to update forecast", err as Error, { @@ -251,15 +258,6 @@ export async function updateForecast({ forecastId: id, duration, }); - // Check if it's a validation error we threw - if (err instanceof Error) { - if ( - err.message === "Cannot update forecasts past the due date" || - err.message === "Forecast not found" - ) { - return error(err.message, ERROR_CODES.VALIDATION_ERROR); - } - } return error("Failed to update forecast", ERROR_CODES.DATABASE_ERROR); } } diff --git a/lib/db_actions/forecasts.unit.test.ts b/lib/db_actions/forecasts.unit.test.ts index 53fe885a..0e91317c 100644 --- a/lib/db_actions/forecasts.unit.test.ts +++ b/lib/db_actions/forecasts.unit.test.ts @@ -9,6 +9,7 @@ vi.mock("@/lib/get-user", () => ({ vi.mock("@/lib/db-helpers", () => ({ withRLS: vi.fn(), + withRLSAction: vi.fn(), })); vi.mock("next/cache", () => ({ @@ -46,7 +47,7 @@ describe("Forecasts Unit Tests", () => { executeTakeFirstOrThrow: vi.fn().mockResolvedValue({ id: 42 }), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -71,7 +72,7 @@ describe("Forecasts Unit Tests", () => { .mockResolvedValue({ competition_forecasts_close_date: pastDate }), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -101,7 +102,7 @@ describe("Forecasts Unit Tests", () => { executeTakeFirstOrThrow: vi.fn().mockResolvedValue({ id: 99 }), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -113,7 +114,7 @@ describe("Forecasts Unit Tests", () => { }); it("should handle database errors gracefully", async () => { - vi.mocked(dbHelpers.withRLS).mockRejectedValue( + vi.mocked(dbHelpers.withRLSAction).mockRejectedValue( new Error("Database connection failed"), ); @@ -142,7 +143,7 @@ describe("Forecasts Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -164,7 +165,7 @@ describe("Forecasts Unit Tests", () => { executeTakeFirst: vi.fn().mockResolvedValue(null), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -191,7 +192,7 @@ describe("Forecasts Unit Tests", () => { .mockResolvedValue({ competition_forecasts_close_date: pastDate }), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -219,7 +220,7 @@ describe("Forecasts Unit Tests", () => { expect(result.code).toBe("UNAUTHORIZED"); } // Should not even call withRLS - expect(dbHelpers.withRLS).not.toHaveBeenCalled(); + expect(dbHelpers.withRLSAction).not.toHaveBeenCalled(); }); it("should reject update with no forecast field", async () => { @@ -232,7 +233,7 @@ describe("Forecasts Unit Tests", () => { if (!result.success) { expect(result.error).toBe("Unauthorized"); } - expect(dbHelpers.withRLS).not.toHaveBeenCalled(); + expect(dbHelpers.withRLSAction).not.toHaveBeenCalled(); }); it("should reject update with empty object", async () => { @@ -248,7 +249,7 @@ describe("Forecasts Unit Tests", () => { }); it("should handle database errors gracefully", async () => { - vi.mocked(dbHelpers.withRLS).mockRejectedValue( + vi.mocked(dbHelpers.withRLSAction).mockRejectedValue( new Error("Connection timeout"), ); diff --git a/lib/db_actions/props.test.ts b/lib/db_actions/props.test.ts index c9a69f65..4ded93ab 100644 --- a/lib/db_actions/props.test.ts +++ b/lib/db_actions/props.test.ts @@ -9,6 +9,7 @@ vi.mock("@/lib/get-user", () => ({ vi.mock("@/lib/db-helpers", () => ({ withRLS: vi.fn(), + withRLSAction: vi.fn(), })); vi.mock("next/cache", () => ({ @@ -161,7 +162,7 @@ describe("Props Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -186,7 +187,7 @@ describe("Props Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -214,7 +215,7 @@ describe("Props Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -323,7 +324,7 @@ describe("Props Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -366,7 +367,7 @@ describe("Props Unit Tests", () => { }), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -409,7 +410,7 @@ describe("Props Unit Tests", () => { }), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -455,7 +456,7 @@ describe("Props Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -486,7 +487,7 @@ describe("Props Unit Tests", () => { executeTakeFirst: vi.fn().mockResolvedValue(null), // competition not found }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -579,7 +580,7 @@ describe("Props Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -605,7 +606,7 @@ describe("Props Unit Tests", () => { executeTakeFirst: vi.fn().mockResolvedValue({ resolution: true }), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); @@ -638,7 +639,7 @@ describe("Props Unit Tests", () => { execute: vi.fn().mockResolvedValue(undefined), }; - vi.mocked(dbHelpers.withRLS).mockImplementation(async (userId, fn) => { + vi.mocked(dbHelpers.withRLSAction).mockImplementation(async (userId, fn) => { return fn(mockTrx as any); }); diff --git a/lib/db_actions/props.ts b/lib/db_actions/props.ts index 401e3893..81becae2 100644 --- a/lib/db_actions/props.ts +++ b/lib/db_actions/props.ts @@ -10,7 +10,7 @@ import { validationError, } from "@/lib/server-action-result"; import { logger } from "@/lib/logger"; -import { withRLS } from "@/lib/db-helpers"; +import { withRLS, withRLSAction } from "@/lib/db-helpers"; export async function getPropById( propId: number, @@ -195,7 +195,7 @@ export async function resolveProp({ const startTime = Date.now(); try { - await withRLS(currentUser?.id, async (trx) => { + const result = await withRLSAction(currentUser?.id, async (trx) => { // first check that this prop doesn't already have a resolution const existingResolution = await trx .selectFrom("resolutions") @@ -208,7 +208,10 @@ export async function resolveProp({ existingResolution: existingResolution.resolution, overwrite, }); - throw new Error(`Proposition ${propId} already has a resolution`); + return error( + `Proposition ${propId} already has a resolution`, + ERROR_CODES.VALIDATION_ERROR, + ); } if (existingResolution) { @@ -230,19 +233,23 @@ export async function resolveProp({ await trx.insertInto("resolutions").values(record).execute(); logger.debug("Created new resolution", { propId, resolution }); } - }); - const duration = Date.now() - startTime; - logger.info("Prop resolved successfully", { - operation: "resolveProp", - table: "resolutions", - propId, - resolution, - duration, + return success(undefined); }); - revalidatePath("/props"); - return success(undefined); + if (result.success) { + const duration = Date.now() - startTime; + logger.info("Prop resolved successfully", { + operation: "resolveProp", + table: "resolutions", + propId, + resolution, + duration, + }); + revalidatePath("/props"); + } + + return result; } catch (err) { const duration = Date.now() - startTime; logger.error("Failed to resolve prop", err as Error, { @@ -251,13 +258,6 @@ export async function resolveProp({ propId, duration, }); - // Check if it's the validation error we threw - if ( - err instanceof Error && - err.message === `Proposition ${propId} already has a resolution` - ) { - return error(err.message, ERROR_CODES.VALIDATION_ERROR); - } return error("Failed to resolve prop", ERROR_CODES.DATABASE_ERROR); } } @@ -432,7 +432,7 @@ export async function createProp({ ); } - await withRLS(currentUser?.id, async (trx) => { + const result = await withRLSAction(currentUser?.id, async (trx) => { // For competition props, verify the user is a competition admin if (prop.competition_id) { const competition = await trx @@ -442,7 +442,7 @@ export async function createProp({ .executeTakeFirst(); if (!competition) { - throw new Error("NOT_FOUND: Competition not found"); + return error("Competition not found", ERROR_CODES.NOT_FOUND); } // For private competitions, only admins can create props @@ -455,28 +455,35 @@ export async function createProp({ .executeTakeFirst(); if (membership?.role !== "admin") { - throw new Error("UNAUTHORIZED: Only competition admins can create props"); + return error( + "Only competition admins can create props", + ERROR_CODES.UNAUTHORIZED, + ); } } } await trx.insertInto("props").values(prop).execute(); + return success(undefined); }); - const duration = Date.now() - startTime; - logger.info("Prop created successfully", { - operation: "createProp", - table: "props", - categoryId: prop.category_id, - textLength: prop.text?.length, - duration, - }); + if (result.success) { + const duration = Date.now() - startTime; + logger.info("Prop created successfully", { + operation: "createProp", + table: "props", + categoryId: prop.category_id, + textLength: prop.text?.length, + duration, + }); - revalidatePath("/props"); - if (prop.competition_id) { - revalidatePath(`/competitions/${prop.competition_id}`); + revalidatePath("/props"); + if (prop.competition_id) { + revalidatePath(`/competitions/${prop.competition_id}`); + } } - return success(undefined); + + return result; } catch (err) { const duration = Date.now() - startTime; logger.error("Failed to create prop", err as Error, { @@ -488,20 +495,6 @@ export async function createProp({ }); const errorMessage = (err as Error).message; - - // Handle specific error types thrown from within the transaction - if (errorMessage.startsWith("UNAUTHORIZED:")) { - return error( - errorMessage.replace("UNAUTHORIZED: ", ""), - ERROR_CODES.UNAUTHORIZED, - ); - } - if (errorMessage.startsWith("NOT_FOUND:")) { - return error( - errorMessage.replace("NOT_FOUND: ", ""), - ERROR_CODES.NOT_FOUND, - ); - } if (errorMessage.includes("duplicate")) { return error( "A proposition with this text already exists",