From bbc82b2b6088ae94ef98ed1024e21b0cb712e7d7 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 14:47:12 -0700 Subject: [PATCH 1/8] refactor(orchestration): move the shared error contract out of lib/workflows OrchestrationErrorCode and statusForOrchestrationError are the contract every lib/[resource]/orchestration module returns against, but they lived inside the workflows module, so resource-neutral code (lib/folders) already had to import from a workflow path. Moved to lib/core/orchestration/types. Adds a 'locked' class mapping to 423. Both tables and workflows have a lock that forbids a mutation, and each caller was translating that to a status itself. Co-Authored-By: Claude Opus 5 --- apps/sim/app/api/tools/deployments/deploy/route.ts | 2 +- apps/sim/app/api/tools/deployments/promote/route.ts | 2 +- apps/sim/app/api/v1/workflows/[id]/deploy/route.ts | 2 +- apps/sim/app/api/v1/workflows/[id]/rollback/route.ts | 2 +- apps/sim/app/api/workflows/[id]/deploy/route.ts | 2 +- .../app/api/workflows/[id]/deployments/[version]/route.ts | 2 +- .../lib/{workflows => core}/orchestration/types.test.ts | 2 +- apps/sim/lib/{workflows => core}/orchestration/types.ts | 8 +++++--- apps/sim/lib/folders/lifecycle.ts | 2 +- apps/sim/lib/folders/status.ts | 2 +- apps/sim/lib/workflows/orchestration/deploy.ts | 2 +- apps/sim/lib/workflows/orchestration/folder-lifecycle.ts | 2 +- .../sim/lib/workflows/orchestration/workflow-lifecycle.ts | 2 +- 13 files changed, 17 insertions(+), 15 deletions(-) rename apps/sim/lib/{workflows => core}/orchestration/types.test.ts (82%) rename apps/sim/lib/{workflows => core}/orchestration/types.ts (52%) diff --git a/apps/sim/app/api/tools/deployments/deploy/route.ts b/apps/sim/app/api/tools/deployments/deploy/route.ts index 5f35c91da86..0795475b549 100644 --- a/apps/sim/app/api/tools/deployments/deploy/route.ts +++ b/apps/sim/app/api/tools/deployments/deploy/route.ts @@ -3,10 +3,10 @@ import { assertWorkflowMutable, WorkflowLockedError } from '@sim/platform-authz/ import { type NextRequest, NextResponse } from 'next/server' import { deploymentsDeployContract } from '@/lib/api/contracts/tools/deployments' import { getValidationErrorMessage, parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { performFullDeploy } from '@/lib/workflows/orchestration' -import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types' import { authenticateDeploymentToolRequest, authorizeDeploymentWorkflow, diff --git a/apps/sim/app/api/tools/deployments/promote/route.ts b/apps/sim/app/api/tools/deployments/promote/route.ts index a126c3dbd57..523a5630a32 100644 --- a/apps/sim/app/api/tools/deployments/promote/route.ts +++ b/apps/sim/app/api/tools/deployments/promote/route.ts @@ -3,10 +3,10 @@ import { assertWorkflowMutable, WorkflowLockedError } from '@sim/platform-authz/ import { type NextRequest, NextResponse } from 'next/server' import { deploymentsPromoteContract } from '@/lib/api/contracts/tools/deployments' import { getValidationErrorMessage, parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { performActivateVersion } from '@/lib/workflows/orchestration' -import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types' import { authenticateDeploymentToolRequest, authorizeDeploymentWorkflow, diff --git a/apps/sim/app/api/v1/workflows/[id]/deploy/route.ts b/apps/sim/app/api/v1/workflows/[id]/deploy/route.ts index 7068239e134..304d69f63d8 100644 --- a/apps/sim/app/api/v1/workflows/[id]/deploy/route.ts +++ b/apps/sim/app/api/v1/workflows/[id]/deploy/route.ts @@ -8,11 +8,11 @@ import { v1UndeployWorkflowContract, } from '@/lib/api/contracts/v1/workflows' import { parseOptionalJsonBody, parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { captureServerEvent } from '@/lib/posthog/server' import { performFullDeploy, performFullUndeploy } from '@/lib/workflows/orchestration' -import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types' import { createApiResponse, getUserLimits } from '@/app/api/v1/logs/meta' import { checkRateLimit, diff --git a/apps/sim/app/api/v1/workflows/[id]/rollback/route.ts b/apps/sim/app/api/v1/workflows/[id]/rollback/route.ts index a0779babf51..d015ba4b024 100644 --- a/apps/sim/app/api/v1/workflows/[id]/rollback/route.ts +++ b/apps/sim/app/api/v1/workflows/[id]/rollback/route.ts @@ -7,10 +7,10 @@ import { v1RollbackWorkflowContract, } from '@/lib/api/contracts/v1/workflows' import { parseOptionalJsonBody, parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { performActivateVersion } from '@/lib/workflows/orchestration' -import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types' import { findPreviousDeploymentVersion } from '@/lib/workflows/persistence/utils' import { createApiResponse, getUserLimits } from '@/app/api/v1/logs/meta' import { diff --git a/apps/sim/app/api/workflows/[id]/deploy/route.ts b/apps/sim/app/api/workflows/[id]/deploy/route.ts index 4c7e1027161..4fb34919b76 100644 --- a/apps/sim/app/api/workflows/[id]/deploy/route.ts +++ b/apps/sim/app/api/workflows/[id]/deploy/route.ts @@ -7,6 +7,7 @@ import { eq } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { updatePublicApiContract } from '@/lib/api/contracts/deployments' import { parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { captureServerEvent } from '@/lib/posthog/server' @@ -15,7 +16,6 @@ import { performFullDeploy, performFullUndeploy, } from '@/lib/workflows/orchestration' -import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types' import { validateWorkflowPermissions } from '@/lib/workflows/utils' import { checkNeedsRedeployment, diff --git a/apps/sim/app/api/workflows/[id]/deployments/[version]/route.ts b/apps/sim/app/api/workflows/[id]/deployments/[version]/route.ts index d3c3337e62f..5d5300ec13d 100644 --- a/apps/sim/app/api/workflows/[id]/deployments/[version]/route.ts +++ b/apps/sim/app/api/workflows/[id]/deployments/[version]/route.ts @@ -4,10 +4,10 @@ import { and, eq } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { updateDeploymentVersionMetadataContract } from '@/lib/api/contracts/deployments' import { getValidationErrorMessage, parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { performActivateVersion } from '@/lib/workflows/orchestration' -import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types' import { getWorkflowDeploymentVersion, updateDeploymentVersionMetadata, diff --git a/apps/sim/lib/workflows/orchestration/types.test.ts b/apps/sim/lib/core/orchestration/types.test.ts similarity index 82% rename from apps/sim/lib/workflows/orchestration/types.test.ts rename to apps/sim/lib/core/orchestration/types.test.ts index 26be2502be4..af8104841a1 100644 --- a/apps/sim/lib/workflows/orchestration/types.test.ts +++ b/apps/sim/lib/core/orchestration/types.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' describe('statusForOrchestrationError', () => { it.each([ diff --git a/apps/sim/lib/workflows/orchestration/types.ts b/apps/sim/lib/core/orchestration/types.ts similarity index 52% rename from apps/sim/lib/workflows/orchestration/types.ts rename to apps/sim/lib/core/orchestration/types.ts index 70c715ffb2c..da6df20b0ba 100644 --- a/apps/sim/lib/workflows/orchestration/types.ts +++ b/apps/sim/lib/core/orchestration/types.ts @@ -1,12 +1,14 @@ -export type OrchestrationErrorCode = 'validation' | 'not_found' | 'conflict' | 'internal' +export type OrchestrationErrorCode = 'validation' | 'not_found' | 'conflict' | 'locked' | 'internal' /** - * Maps an orchestration error code to its HTTP status. Shared by every route - * surface (UI, v1, tool routes) so deployment errors map identically. + * Transport-neutral failure classes returned by every `lib/[resource]/orchestration` + * module, so the UI routes, the public API, and the copilot tools map the same + * failure to the same status. */ export function statusForOrchestrationError(code: OrchestrationErrorCode | undefined): number { if (code === 'validation') return 400 if (code === 'not_found') return 404 if (code === 'conflict') return 409 + if (code === 'locked') return 423 return 500 } diff --git a/apps/sim/lib/folders/lifecycle.ts b/apps/sim/lib/folders/lifecycle.ts index af8e002be0f..970283bb0b2 100644 --- a/apps/sim/lib/folders/lifecycle.ts +++ b/apps/sim/lib/folders/lifecycle.ts @@ -6,6 +6,7 @@ import { getPostgresErrorCode } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { and, eq, isNull, min } from 'drizzle-orm' import type { FolderCascadeCountsApi, FolderResourceType } from '@/lib/api/contracts/folders' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' import type { DbOrTx } from '@/lib/db/types' import { archiveFolderCascade, @@ -19,7 +20,6 @@ import { folderResourceConfig } from '@/lib/folders/config' import { deduplicateFolderName } from '@/lib/folders/naming' import { wouldCreateFolderCycle } from '@/lib/folders/queries' import type { FolderMutationErrorCode } from '@/lib/folders/status' -import type { OrchestrationErrorCode } from '@/lib/workflows/orchestration/types' const logger = createLogger('FolderLifecycle') diff --git a/apps/sim/lib/folders/status.ts b/apps/sim/lib/folders/status.ts index ddea423bd0b..2cf6e89037b 100644 --- a/apps/sim/lib/folders/status.ts +++ b/apps/sim/lib/folders/status.ts @@ -1,4 +1,4 @@ -import type { OrchestrationErrorCode } from '@/lib/workflows/orchestration/types' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' /** * Folder mutations can fail for one reason the shared orchestration vocabulary has no word diff --git a/apps/sim/lib/workflows/orchestration/deploy.ts b/apps/sim/lib/workflows/orchestration/deploy.ts index 8a9f49f4a14..69ce7adac44 100644 --- a/apps/sim/lib/workflows/orchestration/deploy.ts +++ b/apps/sim/lib/workflows/orchestration/deploy.ts @@ -7,6 +7,7 @@ import { getErrorMessage } from '@sim/utils/errors' import { and, eq } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { env } from '@/lib/core/config/env' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { getSocketServerUrl } from '@/lib/core/utils/urls' import { captureServerEvent } from '@/lib/posthog/server' @@ -26,7 +27,6 @@ import { notifySocketDeploymentChanged, processWorkflowDeploymentOutboxEvent, } from '@/lib/workflows/deployment-outbox' -import type { OrchestrationErrorCode } from '@/lib/workflows/orchestration/types' import { getWorkflowDeploymentStatus, prepareWorkflowDeployment, diff --git a/apps/sim/lib/workflows/orchestration/folder-lifecycle.ts b/apps/sim/lib/workflows/orchestration/folder-lifecycle.ts index e4903974aa4..bc1c12e8c73 100644 --- a/apps/sim/lib/workflows/orchestration/folder-lifecycle.ts +++ b/apps/sim/lib/workflows/orchestration/folder-lifecycle.ts @@ -1,8 +1,8 @@ import type { folder as folderTable } from '@sim/db/schema' import type { FolderResourceType } from '@/lib/api/contracts/folders' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' import { createFolder, deleteFolder, restoreFolder, updateFolder } from '@/lib/folders/lifecycle' import type { FolderMutationErrorCode } from '@/lib/folders/status' -import type { OrchestrationErrorCode } from '@/lib/workflows/orchestration/types' /** * Workflow-bound entry points into the generic folder engine in `lib/folders/lifecycle.ts`. diff --git a/apps/sim/lib/workflows/orchestration/workflow-lifecycle.ts b/apps/sim/lib/workflows/orchestration/workflow-lifecycle.ts index 9ccee7d5171..07588af4460 100644 --- a/apps/sim/lib/workflows/orchestration/workflow-lifecycle.ts +++ b/apps/sim/lib/workflows/orchestration/workflow-lifecycle.ts @@ -6,11 +6,11 @@ import { isFolderInWorkspace } from '@sim/platform-authz/workflow' import { toError } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { and, eq, isNull, min, ne } from 'drizzle-orm' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { captureServerEvent } from '@/lib/posthog/server' import { buildDefaultWorkflowArtifacts } from '@/lib/workflows/defaults' import { archiveWorkflow, restoreWorkflow } from '@/lib/workflows/lifecycle' -import type { OrchestrationErrorCode } from '@/lib/workflows/orchestration/types' import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils' import { deduplicateWorkflowName } from '@/lib/workflows/utils' From 9736288815c2308d518cb703be719517c6126f31 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 14:47:40 -0700 Subject: [PATCH 2/8] refactor(tables): make lib/table/orchestration the single implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Column update was implemented four times — the UI route, v1, v2, and the copilot table tool — each calling the same column services but owning its own guards, error mapping, and audit. The copies had drifted, and the drift was the bug: v2 was missing both guards, only the copilot copy minted stable option ids, and only v1/v2 audited. performUpdateTableColumn, performDeleteTable, and performDeleteTableRow now own that logic; all ten call sites reduce to auth, parse, call, render. The guards are asserted once in lib/table/orchestration rather than four times against four routes. Behavior this consolidates, previously true on only some paths: - The typeChanging guard. updateColumnType early-returns on an unchanged type and drops any options sent with it, so restating the current type alongside new options silently discarded them. v2 had no guard at all and, since its contract shares v1's body schema, accepted options and ignored them. - The select-unique guard. Each write is its own locked transaction, so a rename or type change paired with a constraint write that is going to fail commits first and then throws, half-applying the schema change. - Stable select-option ids. Cells reference the option id, so an edit that re-sends an option by name has to reuse it or every cell holding it is orphaned. Only the copilot path did this; normalizeSelectOptionsInput moves to lib/table/select-options and now covers every caller. It preserves a supplied id, so it is a no-op for the fully-formed options the HTTP contracts accept. - required forwarded into the type and options writes, so a conversion validates against the constraint the same request is setting. - An audit on every successful update. The UI route and the copilot tool emitted none. - Single-row delete through the row service. v2 did a raw db.delete, skipping assertRowDelete and deleteOrderedRow, so a delete-locked table returned 200 and the row-count bookkeeping never ran. - The delete actor handed to deleteTable, which audits only when a row was actually archived. v1 and v2 omitted it and audited themselves outside that check, emitting TABLE_DELETED for a no-op delete of an archived table. Failure classes come back as OrchestrationErrorCode; v2 renders them through a new v2ErrorForOrchestration, mirroring statusForOrchestrationError on the v1 and UI surfaces, so a given failure maps to the same status everywhere. Co-Authored-By: Claude Opus 5 --- .../app/api/table/[tableId]/columns/route.ts | 116 ++-------- apps/sim/app/api/table/[tableId]/route.ts | 19 +- .../api/table/[tableId]/rows/[rowId]/route.ts | 12 +- .../api/v1/tables/[tableId]/columns/route.ts | 129 ++---------- apps/sim/app/api/v1/tables/[tableId]/route.ts | 24 +-- .../v1/tables/[tableId]/rows/[rowId]/route.ts | 14 +- apps/sim/app/api/v2/lib/response.ts | 22 ++ .../v2/tables/[tableId]/columns/route.test.ts | 105 ++++++++++ .../api/v2/tables/[tableId]/columns/route.ts | 83 ++------ .../app/api/v2/tables/[tableId]/route.test.ts | 109 ++++++++++ apps/sim/app/api/v2/tables/[tableId]/route.ts | 24 +-- .../[tableId]/rows/[rowId]/route.test.ts | 99 +++++++++ .../v2/tables/[tableId]/rows/[rowId]/route.ts | 27 ++- apps/sim/app/api/v2/tables/utils.ts | 11 + .../tools/server/table/user-table.test.ts | 59 +----- .../copilot/tools/server/table/user-table.ts | 123 ++--------- .../lib/table/orchestration/columns.test.ts | 177 ++++++++++++++++ apps/sim/lib/table/orchestration/columns.ts | 198 ++++++++++++++++++ apps/sim/lib/table/orchestration/index.ts | 67 +----- apps/sim/lib/table/orchestration/restore.ts | 63 ++++++ .../lib/table/orchestration/tables.test.ts | 78 +++++++ apps/sim/lib/table/orchestration/tables.ts | 96 +++++++++ apps/sim/lib/table/select-options.test.ts | 59 ++++++ apps/sim/lib/table/select-options.ts | 33 +++ 24 files changed, 1180 insertions(+), 567 deletions(-) create mode 100644 apps/sim/app/api/v2/tables/[tableId]/columns/route.test.ts create mode 100644 apps/sim/app/api/v2/tables/[tableId]/route.test.ts create mode 100644 apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts create mode 100644 apps/sim/lib/table/orchestration/columns.test.ts create mode 100644 apps/sim/lib/table/orchestration/columns.ts create mode 100644 apps/sim/lib/table/orchestration/restore.ts create mode 100644 apps/sim/lib/table/orchestration/tables.test.ts create mode 100644 apps/sim/lib/table/orchestration/tables.ts create mode 100644 apps/sim/lib/table/select-options.test.ts create mode 100644 apps/sim/lib/table/select-options.ts diff --git a/apps/sim/app/api/table/[tableId]/columns/route.ts b/apps/sim/app/api/table/[tableId]/columns/route.ts index 227c1422b04..acf6754bc88 100644 --- a/apps/sim/app/api/table/[tableId]/columns/route.ts +++ b/apps/sim/app/api/table/[tableId]/columns/route.ts @@ -8,17 +8,11 @@ import { import { parseRequest } from '@/lib/api/server' import { isZodError, validationErrorResponse } from '@/lib/api/server/validation' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { - addTableColumn, - deleteColumn, - renameColumn, - updateColumnConstraints, - updateColumnOptions, - updateColumnType, -} from '@/lib/table' -import { columnMatchesRef } from '@/lib/table/column-keys' +import { addTableColumn, deleteColumn } from '@/lib/table' +import { performUpdateTableColumn } from '@/lib/table/orchestration' import { accessError, checkAccess, @@ -117,111 +111,31 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - const { updates } = validated - let updatedTable = null - - if (updates.name) { - updatedTable = await renameColumn( - { tableId, oldName: validated.columnName, newName: updates.name }, - requestId - ) - } - - // A payload that repeats the current type must not go through - // `updateColumnType` — it early-returns on an unchanged type and would drop - // any `options` alongside it. Only a real type change routes there; an - // unchanged type with options routes to the options-only update. - const currentColumn = table.schema.columns.find((c) => - columnMatchesRef(c, validated.columnName) - ) - const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type - - // Every write below is its own locked transaction, so any of them paired - // with a constraint write that is going to fail commits and then errors. - // Gate on the type the column ENDS UP with, not on whether the type is - // changing: an options-only update on an existing select column carries the - // same hazard as a conversion does. - const resultingType = updates.type ?? currentColumn?.type - if (updates.unique === true && resultingType === 'select') { - return NextResponse.json({ error: 'Cannot set a select column as unique' }, { status: 400 }) - } - - if (typeChanging) { - updatedTable = await updateColumnType( - { - tableId, - columnName: updates.name ?? validated.columnName, - newType: updates.type as NonNullable, - ...(updates.options !== undefined ? { options: updates.options } : {}), - ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), - // Forwarded so the conversion validates against the constraint this - // same request is about to set, not the column's current one. - ...(updates.required !== undefined ? { required: updates.required } : {}), - }, - requestId - ) - } else if (updates.options !== undefined || updates.multiple !== undefined) { - updatedTable = await updateColumnOptions( - { - tableId, - columnName: updates.name ?? validated.columnName, - options: updates.options ?? currentColumn?.options ?? [], - ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), - // Forwarded so the removal guard validates against the constraint this - // same request is about to set, not the column's current one. - ...(updates.required !== undefined ? { required: updates.required } : {}), - }, - requestId - ) - } - - if (updates.required !== undefined || updates.unique !== undefined) { - updatedTable = await updateColumnConstraints( - { - tableId, - columnName: updates.name ?? validated.columnName, - ...(updates.required !== undefined ? { required: updates.required } : {}), - ...(updates.unique !== undefined ? { unique: updates.unique } : {}), - }, - requestId + const outcome = await performUpdateTableColumn({ + table, + columnName: validated.columnName, + userId: authResult.userId, + updates: validated.updates, + requestId, + }) + if (!outcome.success || !outcome.table) { + return NextResponse.json( + { error: outcome.error ?? 'Failed to update column' }, + { status: statusForOrchestrationError(outcome.errorCode) } ) } - if (!updatedTable) { - return NextResponse.json({ error: 'No updates specified' }, { status: 400 }) - } - return NextResponse.json({ success: true, data: { - columns: updatedTable.schema.columns.map(normalizeColumn), + columns: outcome.table.schema.columns.map(normalizeColumn), }, }) } catch (error) { - const lockError = tableLockErrorResponse(error) - if (lockError) return lockError if (isZodError(error)) { return validationErrorResponse(error, 'Invalid request data') } - const msg = rootErrorMessage(error) - if (msg.includes('not found') || msg.includes('Table not found')) { - return NextResponse.json({ error: msg }, { status: 404 }) - } - if ( - msg.includes('already exists') || - msg.includes('Cannot delete the last column') || - msg.includes('Cannot set column') || - msg.includes('Cannot set unique column') || - msg.includes('Invalid column') || - msg.includes('exceeds maximum') || - msg.includes('incompatible') || - msg.includes('duplicate') || - msg.includes('option') - ) { - return NextResponse.json({ error: msg }, { status: 400 }) - } - logger.error(`[${requestId}] Error updating column in table ${tableId}:`, error) return NextResponse.json({ error: 'Failed to update column' }, { status: 500 }) } diff --git a/apps/sim/app/api/table/[tableId]/route.ts b/apps/sim/app/api/table/[tableId]/route.ts index 03eaa4c7243..0c428056826 100644 --- a/apps/sim/app/api/table/[tableId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/route.ts @@ -5,12 +5,11 @@ import { getTableQuerySchema, updateTableContract } from '@/lib/api/contracts/ta import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' import { isFeatureEnabled } from '@/lib/core/config/feature-flags' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { findActiveFolder } from '@/lib/folders/queries' -import { captureServerEvent } from '@/lib/posthog/server' import { - deleteTable, getTableById, moveTableToFolder, renameTable, @@ -19,6 +18,7 @@ import { updateTableLocks, } from '@/lib/table' import { getWorkspaceTableLimits } from '@/lib/table/billing' +import { performDeleteTable } from '@/lib/table/orchestration' import { TABLE_LOCK_FLAGS, TABLE_LOCK_KINDS } from '@/lib/table/types' import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils' import { @@ -268,14 +268,13 @@ export const DELETE = withRouteHandler( return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - await deleteTable(tableId, requestId, authResult.userId) - - captureServerEvent( - authResult.userId, - 'table_deleted', - { table_id: tableId, workspace_id: table.workspaceId }, - { groups: { workspace: table.workspaceId } } - ) + const outcome = await performDeleteTable({ table, userId: authResult.userId, requestId }) + if (!outcome.success) { + return NextResponse.json( + { error: outcome.error ?? 'Failed to delete table' }, + { status: statusForOrchestrationError(outcome.errorCode) } + ) + } return NextResponse.json({ success: true, diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts index 82ec048ca84..da7852c4569 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts @@ -11,10 +11,12 @@ import { } from '@/lib/api/contracts/tables' import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import type { RowData, TableSchema } from '@/lib/table' -import { deleteRow, updateRow } from '@/lib/table' +import { updateRow } from '@/lib/table' +import { performDeleteTableRow } from '@/lib/table/orchestration' import { rowWireTranslators } from '@/app/api/table/row-wire' import { accessError, @@ -212,7 +214,13 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - await deleteRow(table, rowId, requestId) + const outcome = await performDeleteTableRow({ table, rowId, requestId }) + if (!outcome.success) { + return NextResponse.json( + { error: outcome.error ?? 'Failed to delete row' }, + { status: statusForOrchestrationError(outcome.errorCode) } + ) + } return NextResponse.json({ success: true, diff --git a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts index f1751ee2120..65522340765 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts @@ -7,17 +7,11 @@ import { v1UpdateTableColumnContract, } from '@/lib/api/contracts/v1/tables' import { parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { - addTableColumn, - deleteColumn, - renameColumn, - updateColumnConstraints, - updateColumnOptions, - updateColumnType, -} from '@/lib/table' -import { columnMatchesRef } from '@/lib/table/column-keys' +import { addTableColumn, deleteColumn } from '@/lib/table' +import { performUpdateTableColumn } from '@/lib/table/orchestration' import { accessError, checkAccess, @@ -151,123 +145,30 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - const { updates } = validated - let updatedTable = null - - if (updates.name) { - updatedTable = await renameColumn( - { tableId, oldName: validated.columnName, newName: updates.name }, - requestId - ) - } - - // A payload that repeats the current type must not go through - // `updateColumnType` — it early-returns on an unchanged type and would drop - // any `options` alongside it. Only a real type change routes there; an - // unchanged type with options routes to the options-only update. - const currentColumn = table.schema.columns.find((c) => - columnMatchesRef(c, validated.columnName) - ) - const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type - - // Every write below is its own locked transaction, so any of them paired - // with a constraint write that is going to fail commits and then errors. - // Gate on the type the column ENDS UP with, not on whether the type is - // changing: an options-only update on an existing select column carries the - // same hazard as a conversion does. - const resultingType = updates.type ?? currentColumn?.type - if (updates.unique === true && resultingType === 'select') { - return NextResponse.json({ error: 'Cannot set a select column as unique' }, { status: 400 }) - } - - if (typeChanging) { - updatedTable = await updateColumnType( - { - tableId, - columnName: updates.name ?? validated.columnName, - newType: updates.type as NonNullable, - ...(updates.options !== undefined ? { options: updates.options } : {}), - ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), - // Forwarded so the conversion validates against the constraint this - // same request is about to set, not the column's current one. - ...(updates.required !== undefined ? { required: updates.required } : {}), - }, - requestId - ) - } else if (updates.options !== undefined || updates.multiple !== undefined) { - updatedTable = await updateColumnOptions( - { - tableId, - columnName: updates.name ?? validated.columnName, - options: updates.options ?? currentColumn?.options ?? [], - ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), - // Forwarded so the removal guard validates against the constraint this - // same request is about to set, not the column's current one. - ...(updates.required !== undefined ? { required: updates.required } : {}), - }, - requestId - ) - } - - if (updates.required !== undefined || updates.unique !== undefined) { - updatedTable = await updateColumnConstraints( - { - tableId, - columnName: updates.name ?? validated.columnName, - ...(updates.required !== undefined ? { required: updates.required } : {}), - ...(updates.unique !== undefined ? { unique: updates.unique } : {}), - }, - requestId + const outcome = await performUpdateTableColumn({ + table, + columnName: validated.columnName, + userId, + updates: validated.updates, + requestId, + }) + if (!outcome.success || !outcome.table) { + return NextResponse.json( + { error: outcome.error ?? 'Failed to update column' }, + { status: statusForOrchestrationError(outcome.errorCode) } ) } - if (!updatedTable) { - return NextResponse.json({ error: 'No updates specified' }, { status: 400 }) - } - - recordAudit({ - workspaceId: validated.workspaceId, - actorId: userId, - action: AuditAction.TABLE_UPDATED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: table.name, - description: `Updated column "${validated.columnName}" in table "${table.name}"`, - metadata: { columnName: validated.columnName, updates }, - request, - }) - return NextResponse.json({ success: true, data: { - columns: updatedTable.schema.columns.map(normalizeColumn), + columns: outcome.table.schema.columns.map(normalizeColumn), }, }) } catch (error) { - const lockError = tableLockErrorResponse(error) - if (lockError) return lockError const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - if (error instanceof Error) { - const msg = error.message - if (msg.includes('not found') || msg.includes('Table not found')) { - return NextResponse.json({ error: msg }, { status: 404 }) - } - if ( - msg.includes('already exists') || - msg.includes('Cannot delete the last column') || - msg.includes('Cannot set column') || - msg.includes('Invalid column') || - msg.includes('exceeds maximum') || - msg.includes('incompatible') || - msg.includes('duplicate') || - msg.includes('option') - ) { - return NextResponse.json({ error: msg }, { status: 400 }) - } - } - logger.error(`[${requestId}] Error updating column in table:`, error) return NextResponse.json({ error: 'Failed to update column' }, { status: 500 }) } diff --git a/apps/sim/app/api/v1/tables/[tableId]/route.ts b/apps/sim/app/api/v1/tables/[tableId]/route.ts index c06492d02b7..1849c2e162d 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/route.ts @@ -1,11 +1,12 @@ -import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { type NextRequest, NextResponse } from 'next/server' import { v1DeleteTableContract, v1GetTableContract } from '@/lib/api/contracts/v1/tables' import { parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { deleteTable, type TableSchema } from '@/lib/table' +import type { TableSchema } from '@/lib/table' +import { performDeleteTable } from '@/lib/table/orchestration' import { accessError, checkAccess, @@ -139,18 +140,13 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - await deleteTable(tableId, requestId) - - recordAudit({ - workspaceId, - actorId: userId, - action: AuditAction.TABLE_DELETED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: result.table.name, - description: `Archived table "${result.table.name}"`, - request, - }) + const outcome = await performDeleteTable({ table: result.table, userId, requestId }) + if (!outcome.success) { + return NextResponse.json( + { error: outcome.error ?? 'Failed to delete table' }, + { status: statusForOrchestrationError(outcome.errorCode) } + ) + } return NextResponse.json({ success: true, diff --git a/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts index 5fee4f3d03b..1d237b7be0f 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts @@ -10,12 +10,14 @@ import { v1UpdateTableRowContract, } from '@/lib/api/contracts/v1/tables' import { parseRequest } from '@/lib/api/server' +import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import type { RowData, TableSchema } from '@/lib/table' -import { deleteRow, updateRow } from '@/lib/table' +import { updateRow } from '@/lib/table' import { namedRowMapper } from '@/lib/table/cell-format' import { buildIdByName, rowDataNameToId } from '@/lib/table/column-keys' +import { performDeleteTableRow } from '@/lib/table/orchestration' import { accessError, checkAccess, tableLockErrorResponse } from '@/app/api/table/utils' import { checkRateLimit, @@ -238,9 +240,13 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - // Route through the service (not a raw `db.delete`) so the delete lock is - // enforced — the raw path would return 200 on a locked table. - await deleteRow(result.table, rowId, requestId) + const outcome = await performDeleteTableRow({ table: result.table, rowId, requestId }) + if (!outcome.success) { + return NextResponse.json( + { error: outcome.error ?? 'Failed to delete row' }, + { status: statusForOrchestrationError(outcome.errorCode) } + ) + } return NextResponse.json({ success: true, diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index bd326d1218d..7ae333cc5bd 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -1,6 +1,7 @@ import { NextResponse } from 'next/server' import type { ZodError } from 'zod' import { getValidationErrorMessage, serializeZodIssues } from '@/lib/api/server' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' import type { RateLimitResult, WorkspaceAccessError } from '@/app/api/v1/middleware' /** @@ -155,3 +156,24 @@ export function decodeCursor>(cursor: string): T | n return null } } + +const V2_CODE_BY_ORCHESTRATION_ERROR: Record = { + validation: 'BAD_REQUEST', + not_found: 'NOT_FOUND', + conflict: 'CONFLICT', + locked: 'LOCKED', + internal: 'INTERNAL_ERROR', +} + +/** + * Renders a `lib/[resource]/orchestration` failure in the v2 envelope, so every + * v2 route maps a given failure class to the same status without restating the + * mapping. Mirrors `statusForOrchestrationError` for the v1/UI surfaces. + */ +export function v2ErrorForOrchestration( + code: OrchestrationErrorCode | undefined, + message: string +): NextResponse { + const v2Code = code ? V2_CODE_BY_ORCHESTRATION_ERROR[code] : 'INTERNAL_ERROR' + return v2Error(v2Code, v2Code === 'INTERNAL_ERROR' ? 'Internal server error' : message) +} diff --git a/apps/sim/app/api/v2/tables/[tableId]/columns/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/columns/route.test.ts new file mode 100644 index 00000000000..a7d6235dca0 --- /dev/null +++ b/apps/sim/app/api/v2/tables/[tableId]/columns/route.test.ts @@ -0,0 +1,105 @@ +/** + * @vitest-environment node + * + * v2 column update wiring: the route authenticates, scopes, delegates to the + * orchestration function, and maps its failure classes onto the v2 envelope. + * The guards themselves are covered in lib/table/orchestration/columns.test.ts. + */ +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockCheckRateLimit, mockResolveWorkspaceScope, mockCheckAccess, mockPerformUpdate } = + vi.hoisted(() => ({ + mockCheckRateLimit: vi.fn(), + mockResolveWorkspaceScope: vi.fn(), + mockCheckAccess: vi.fn(), + mockPerformUpdate: vi.fn(), + })) + +vi.mock('@/app/api/v1/middleware', () => ({ + checkRateLimit: mockCheckRateLimit, + resolveWorkspaceScope: mockResolveWorkspaceScope, +})) + +vi.mock('@/app/api/table/utils', () => ({ + checkAccess: mockCheckAccess, + normalizeColumn: (col: Record) => col, +})) + +vi.mock('@/lib/table', () => ({ addTableColumn: vi.fn(), deleteColumn: vi.fn() })) + +vi.mock('@/lib/table/orchestration', () => ({ + performUpdateTableColumn: mockPerformUpdate, +})) + +vi.mock('@/app/api/v2/lib/gate', () => ({ + v2ApiGateError: vi.fn().mockResolvedValue(null), +})) + +import { PATCH } from '@/app/api/v2/tables/[tableId]/columns/route' + +const COLUMN = { id: 'col-1', name: 'Status', type: 'text' } +const TABLE = { id: 'table-1', name: 'Tasks', workspaceId: 'ws-1', schema: { columns: [COLUMN] } } + +function patch(updates: Record = { name: 'State' }) { + const req = new NextRequest('http://localhost:3000/api/v2/tables/table-1/columns', { + method: 'PATCH', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ workspaceId: 'ws-1', columnName: 'Status', updates }), + }) + return PATCH(req, { params: Promise.resolve({ tableId: 'table-1' }) }) +} + +describe('PATCH /api/v2/tables/[tableId]/columns', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCheckRateLimit.mockResolvedValue({ + allowed: true, + userId: 'user-1', + keyType: 'workspace', + workspaceId: 'ws-1', + limit: 100, + remaining: 99, + resetAt: new Date('2026-01-01T01:00:00Z'), + }) + mockResolveWorkspaceScope.mockResolvedValue(null) + mockCheckAccess.mockResolvedValue({ ok: true, table: TABLE }) + mockPerformUpdate.mockResolvedValue({ success: true, table: TABLE }) + }) + + it('delegates to the orchestration function with the resolved table and actor', async () => { + const res = await patch() + + expect(res.status).toBe(200) + expect((await res.json()).data).toEqual({ columns: [COLUMN] }) + expect(mockPerformUpdate).toHaveBeenCalledWith( + expect.objectContaining({ table: TABLE, columnName: 'Status', userId: 'user-1' }) + ) + }) + + it.each([ + ['validation', 400, 'BAD_REQUEST'], + ['not_found', 404, 'NOT_FOUND'], + ['locked', 423, 'LOCKED'], + ])('maps a %s failure to %i', async (errorCode, status, code) => { + mockPerformUpdate.mockResolvedValue({ success: false, errorCode, error: 'nope' }) + + const res = await patch() + + expect(res.status).toBe(status) + expect((await res.json()).error.code).toBe(code) + }) + + it('does not leak an internal failure message', async () => { + mockPerformUpdate.mockResolvedValue({ + success: false, + errorCode: 'internal', + error: 'connection string leaked', + }) + + const res = await patch() + + expect(res.status).toBe(500) + expect(await res.text()).not.toContain('connection string') + }) +}) diff --git a/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts index c7c1e538600..ce9085e588e 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts @@ -10,19 +10,15 @@ import { import { isZodError, parseRequest } from '@/lib/api/server' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { - addTableColumn, - deleteColumn, - renameColumn, - updateColumnConstraints, - updateColumnType, -} from '@/lib/table' +import { addTableColumn, deleteColumn } from '@/lib/table' +import { performUpdateTableColumn } from '@/lib/table/orchestration' import { checkAccess, normalizeColumn } from '@/app/api/table/utils' import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware' import { v2ApiGateError } from '@/app/api/v2/lib/gate' import { v2Data, v2Error, + v2ErrorForOrchestration, v2RateLimitError, v2ValidationError, v2WorkspaceAccessError, @@ -136,73 +132,20 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu return v2Error('NOT_FOUND', 'Table not found') } - const { updates } = validated - let updatedTable = null - - if (updates.name) { - updatedTable = await renameColumn( - { tableId, oldName: validated.columnName, newName: updates.name }, - requestId - ) - } - - if (updates.type) { - updatedTable = await updateColumnType( - { tableId, columnName: updates.name ?? validated.columnName, newType: updates.type }, - requestId - ) - } - - if (updates.required !== undefined || updates.unique !== undefined) { - updatedTable = await updateColumnConstraints( - { - tableId, - columnName: updates.name ?? validated.columnName, - ...(updates.required !== undefined ? { required: updates.required } : {}), - ...(updates.unique !== undefined ? { unique: updates.unique } : {}), - }, - requestId - ) - } - - if (!updatedTable) { - return v2Error('BAD_REQUEST', 'No updates specified') - } - - recordAudit({ - workspaceId: validated.workspaceId, - actorId: userId, - action: AuditAction.TABLE_UPDATED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: table.name, - description: `Updated column "${validated.columnName}" in table "${table.name}"`, - metadata: { columnName: validated.columnName, updates }, - request, + const outcome = await performUpdateTableColumn({ + table, + columnName: validated.columnName, + userId, + updates: validated.updates, + requestId, }) + if (!outcome.success || !outcome.table) { + return v2ErrorForOrchestration(outcome.errorCode, outcome.error ?? 'Failed to update column') + } - return v2Data({ columns: updatedTable.schema.columns.map(normalizeColumn) }, { rateLimit }) + return v2Data({ columns: outcome.table.schema.columns.map(normalizeColumn) }, { rateLimit }) } catch (error) { if (isZodError(error)) return v2ValidationError(error) - - if (error instanceof Error) { - const msg = error.message - if (msg.includes('not found') || msg.includes('Table not found')) { - return v2Error('NOT_FOUND', msg) - } - if ( - msg.includes('already exists') || - msg.includes('Cannot delete the last column') || - msg.includes('Cannot set column') || - msg.includes('Invalid column') || - msg.includes('exceeds maximum') || - msg.includes('incompatible') || - msg.includes('duplicate') - ) { - return v2Error('BAD_REQUEST', msg) - } - } - logger.error(`[${requestId}] Error updating column in table`, { error: getErrorMessage(error, 'Unknown error'), }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/route.test.ts new file mode 100644 index 00000000000..43210d8a8e8 --- /dev/null +++ b/apps/sim/app/api/v2/tables/[tableId]/route.test.ts @@ -0,0 +1,109 @@ +/** + * @vitest-environment node + * + * Public v2 table delete: the actor is handed to the service so the audit is + * emitted there — and only for a delete that actually archived a row. + */ +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + mockCheckRateLimit, + mockResolveWorkspaceScope, + mockCheckAccess, + mockPerformDeleteTable, + mockRecordAudit, +} = vi.hoisted(() => ({ + mockCheckRateLimit: vi.fn(), + mockResolveWorkspaceScope: vi.fn(), + mockCheckAccess: vi.fn(), + mockPerformDeleteTable: vi.fn(), + mockRecordAudit: vi.fn(), +})) + +vi.mock('@sim/audit', () => ({ + AuditAction: { TABLE_DELETED: 'table.deleted', TABLE_UPDATED: 'table.updated' }, + AuditResourceType: { TABLE: 'table' }, + recordAudit: mockRecordAudit, +})) + +vi.mock('@/app/api/v1/middleware', () => ({ + checkRateLimit: mockCheckRateLimit, + resolveWorkspaceScope: mockResolveWorkspaceScope, +})) + +vi.mock('@/app/api/table/utils', () => ({ + checkAccess: mockCheckAccess, + normalizeColumn: (col: Record) => col, + rootErrorMessage: (error: unknown) => String(error), + rowWriteErrorResponse: () => null, +})) + +vi.mock('@/lib/table', () => ({ + updateTable: vi.fn(), + getTableById: vi.fn(), + updateRow: vi.fn(), + rowDataNameToId: vi.fn(), + buildIdByName: vi.fn(), +})) + +vi.mock('@/lib/table/orchestration', () => ({ performDeleteTable: mockPerformDeleteTable })) + +vi.mock('@/app/api/v2/lib/gate', () => ({ + v2ApiGateError: vi.fn().mockResolvedValue(null), +})) + +import { DELETE } from '@/app/api/v2/tables/[tableId]/route' + +const TABLE = { id: 'table-1', name: 'Tasks', workspaceId: 'ws-1', schema: { columns: [] } } + +function callDelete() { + const req = new NextRequest('http://localhost:3000/api/v2/tables/table-1?workspaceId=ws-1', { + method: 'DELETE', + }) + return DELETE(req, { params: Promise.resolve({ tableId: 'table-1' }) }) +} + +describe('DELETE /api/v2/tables/[tableId]', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCheckRateLimit.mockResolvedValue({ + allowed: true, + userId: 'user-1', + keyType: 'workspace', + workspaceId: 'ws-1', + limit: 100, + remaining: 99, + resetAt: new Date('2026-01-01T01:00:00Z'), + }) + mockResolveWorkspaceScope.mockResolvedValue(null) + mockCheckAccess.mockResolvedValue({ ok: true, table: TABLE }) + }) + + it('delegates to the orchestration function with the resolved table and actor', async () => { + mockPerformDeleteTable.mockResolvedValue({ success: true }) + + const res = await callDelete() + + expect(res.status).toBe(200) + expect(mockPerformDeleteTable).toHaveBeenCalledWith( + expect.objectContaining({ table: TABLE, userId: 'user-1' }) + ) + // The route no longer audits: doing so out here fired TABLE_DELETED even + // when the delete was a no-op on an already-archived table. + expect(mockRecordAudit).not.toHaveBeenCalled() + }) + + it('returns 423 LOCKED for a delete-locked table instead of a 500', async () => { + mockPerformDeleteTable.mockResolvedValue({ + success: false, + errorCode: 'locked', + error: 'Table is locked', + }) + + const res = await callDelete() + + expect(res.status).toBe(423) + expect((await res.json()).error.code).toBe('LOCKED') + }) +}) diff --git a/apps/sim/app/api/v2/tables/[tableId]/route.ts b/apps/sim/app/api/v2/tables/[tableId]/route.ts index 03e97d590fa..dfb54287e80 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/route.ts @@ -1,4 +1,3 @@ -import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import type { NextRequest } from 'next/server' @@ -6,18 +5,19 @@ import { v2DeleteTableContract, v2GetTableContract } from '@/lib/api/contracts/v import { parseRequest } from '@/lib/api/server' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { deleteTable } from '@/lib/table' +import { performDeleteTable } from '@/lib/table/orchestration' import { checkAccess } from '@/app/api/table/utils' import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware' import { v2ApiGateError } from '@/app/api/v2/lib/gate' import { v2Data, v2Error, + v2ErrorForOrchestration, v2RateLimitError, v2ValidationError, v2WorkspaceAccessError, } from '@/app/api/v2/lib/response' -import { toApiTable, v2TableAccessError } from '@/app/api/v2/tables/utils' +import { toApiTable, v2TableAccessError, v2TableLockError } from '@/app/api/v2/tables/utils' const logger = createLogger('V2TableDetailAPI') @@ -100,21 +100,15 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab return v2Error('NOT_FOUND', 'Table not found') } - await deleteTable(tableId, requestId) - - recordAudit({ - workspaceId, - actorId: userId, - action: AuditAction.TABLE_DELETED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: result.table.name, - description: `Archived table "${result.table.name}"`, - request, - }) + const outcome = await performDeleteTable({ table: result.table, userId, requestId }) + if (!outcome.success) { + return v2ErrorForOrchestration(outcome.errorCode, outcome.error ?? 'Failed to delete table') + } return v2Data({ id: tableId }, { rateLimit }) } catch (error) { + const lockError = v2TableLockError(error) + if (lockError) return lockError logger.error(`[${requestId}] Error deleting table`, { error: getErrorMessage(error, 'Unknown error'), }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts new file mode 100644 index 00000000000..2139216449a --- /dev/null +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.test.ts @@ -0,0 +1,99 @@ +/** + * @vitest-environment node + * + * Public v2 single-row delete: goes through the row service so the delete lock + * and row-count bookkeeping are enforced, and renders lock/not-found in the v2 + * error envelope. + */ +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockCheckRateLimit, mockResolveWorkspaceScope, mockCheckAccess, mockPerformDeleteRow } = + vi.hoisted(() => ({ + mockCheckRateLimit: vi.fn(), + mockResolveWorkspaceScope: vi.fn(), + mockCheckAccess: vi.fn(), + mockPerformDeleteRow: vi.fn(), + })) + +vi.mock('@/app/api/v1/middleware', () => ({ + checkRateLimit: mockCheckRateLimit, + resolveWorkspaceScope: mockResolveWorkspaceScope, +})) + +vi.mock('@/app/api/table/utils', () => ({ + checkAccess: mockCheckAccess, + normalizeColumn: (col: Record) => col, + rootErrorMessage: (error: unknown) => String(error), + rowWriteErrorResponse: () => null, +})) + +vi.mock('@/lib/table', () => ({ + updateTable: vi.fn(), + getTableById: vi.fn(), + updateRow: vi.fn(), + rowDataNameToId: vi.fn(), + buildIdByName: vi.fn(), +})) + +vi.mock('@/lib/table/orchestration', () => ({ performDeleteTableRow: mockPerformDeleteRow })) + +vi.mock('@/app/api/v2/lib/gate', () => ({ + v2ApiGateError: vi.fn().mockResolvedValue(null), +})) + +import { DELETE } from '@/app/api/v2/tables/[tableId]/rows/[rowId]/route' + +const TABLE = { id: 'table-1', workspaceId: 'ws-1', schema: { columns: [] } } + +function callDelete() { + const req = new NextRequest( + 'http://localhost:3000/api/v2/tables/table-1/rows/row-1?workspaceId=ws-1', + { method: 'DELETE' } + ) + return DELETE(req, { params: Promise.resolve({ tableId: 'table-1', rowId: 'row-1' }) }) +} + +describe('DELETE /api/v2/tables/[tableId]/rows/[rowId]', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCheckRateLimit.mockResolvedValue({ + allowed: true, + userId: 'user-1', + keyType: 'workspace', + workspaceId: 'ws-1', + limit: 100, + remaining: 99, + resetAt: new Date('2026-01-01T01:00:00Z'), + }) + mockResolveWorkspaceScope.mockResolvedValue(null) + mockCheckAccess.mockResolvedValue({ ok: true, table: TABLE }) + }) + + it('delegates to the orchestration function rather than deleting inline', async () => { + mockPerformDeleteRow.mockResolvedValue({ success: true }) + + const res = await callDelete() + + expect(res.status).toBe(200) + expect((await res.json()).data).toEqual({ deletedCount: 1, deletedRowIds: ['row-1'] }) + // The orchestration function routes through the row service, which applies + // the delete lock and the row-count decrement; the raw delete this replaced + // skipped both. + expect(mockPerformDeleteRow).toHaveBeenCalledWith( + expect.objectContaining({ table: TABLE, rowId: 'row-1' }) + ) + }) + + it.each([ + ['locked', 423, 'LOCKED'], + ['not_found', 404, 'NOT_FOUND'], + ])('maps a %s failure to %i', async (errorCode, status, code) => { + mockPerformDeleteRow.mockResolvedValue({ success: false, errorCode, error: 'nope' }) + + const res = await callDelete() + + expect(res.status).toBe(status) + expect((await res.json()).error.code).toBe(code) + }) +}) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts index f11cf9b2c74..7fe976ec553 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts @@ -15,17 +15,19 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import type { RowData, TableSchema } from '@/lib/table' import { buildIdByName, rowDataNameToId, updateRow } from '@/lib/table' import { namedRowMapper } from '@/lib/table/cell-format' +import { performDeleteTableRow } from '@/lib/table/orchestration' import { checkAccess } from '@/app/api/table/utils' import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware' import { v2ApiGateError } from '@/app/api/v2/lib/gate' import { v2Data, v2Error, + v2ErrorForOrchestration, v2RateLimitError, v2ValidationError, v2WorkspaceAccessError, } from '@/app/api/v2/lib/response' -import { toApiRow, v2TableAccessError } from '@/app/api/v2/tables/utils' +import { toApiRow, v2TableAccessError, v2TableLockError } from '@/app/api/v2/tables/utils' const logger = createLogger('V2TableRowAPI') @@ -214,22 +216,19 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row return v2Error('NOT_FOUND', 'Table not found') } - const [deletedRow] = await db - .delete(userTableRows) - .where( - and( - eq(userTableRows.id, rowId), - eq(userTableRows.tableId, tableId), - eq(userTableRows.workspaceId, workspaceId) - ) - ) - .returning({ id: userTableRows.id }) - - if (!deletedRow) return v2Error('NOT_FOUND', 'Row not found') + const outcome = await performDeleteTableRow({ table: result.table, rowId, requestId }) + if (!outcome.success) { + return v2ErrorForOrchestration(outcome.errorCode, outcome.error ?? 'Failed to delete row') + } // v2 mirrors the bulk delete shape: always returns `deletedRowIds`. - return v2Data({ deletedCount: 1, deletedRowIds: [deletedRow.id] }, { rateLimit }) + return v2Data({ deletedCount: 1, deletedRowIds: [rowId] }, { rateLimit }) } catch (error) { + const lockError = v2TableLockError(error) + if (lockError) return lockError + if (error instanceof Error && error.message === 'Row not found') { + return v2Error('NOT_FOUND', 'Row not found') + } logger.error(`[${requestId}] Error deleting row`, { error: getErrorMessage(error, 'Unknown error'), }) diff --git a/apps/sim/app/api/v2/tables/utils.ts b/apps/sim/app/api/v2/tables/utils.ts index 00a9510feb8..8d662be3d4a 100644 --- a/apps/sim/app/api/v2/tables/utils.ts +++ b/apps/sim/app/api/v2/tables/utils.ts @@ -1,5 +1,6 @@ import type { NextResponse } from 'next/server' import type { RowData, TableDefinition, TablePredicate, TableSchema } from '@/lib/table' +import { TableLockedError } from '@/lib/table/mutation-locks' import { predicateToFilter } from '@/lib/table/query-builder/converters' import { validatePredicateShape, @@ -95,6 +96,16 @@ export function v2TableAccessError(result: { ok: false; status: 404 | 403 }): Ne : v2Error('FORBIDDEN', 'Access denied') } +/** + * Maps a delete/write rejected by a table lock to the v2 `LOCKED` envelope, + * mirroring v1's {@link tableLockErrorResponse}. Returns `null` for anything + * else so the caller falls through to its own classification. + */ +export function v2TableLockError(error: unknown): NextResponse | null { + if (error instanceof TableLockedError) return v2Error('LOCKED', error.message) + return null +} + /** * Maps a known user-facing row-write failure (schema/size/unique/limit) to a v2 * `BAD_REQUEST`, reusing v1's {@link rowWriteErrorResponse} classifier as the diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.test.ts b/apps/sim/lib/copilot/tools/server/table/user-table.test.ts index adebd05bd7a..512401f65ea 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.test.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.test.ts @@ -137,10 +137,7 @@ vi.mock('@/lib/table/billing', () => ({ getWorkspaceTableLimits: mockGetWorkspaceTableLimits, })) -import { - normalizeSelectOptionsInput, - userTableServerTool, -} from '@/lib/copilot/tools/server/table/user-table' +import { userTableServerTool } from '@/lib/copilot/tools/server/table/user-table' import { encodeCursor } from '@/lib/table/rows/cursor' function buildTable(overrides: Partial = {}): TableDefinition { @@ -172,60 +169,6 @@ async function flushDetached(): Promise { await Promise.resolve() } -describe('normalizeSelectOptionsInput', () => { - it('generates a stable id for a bare-name string option', () => { - const [opt] = normalizeSelectOptionsInput(['Open']) ?? [] - expect(opt.name).toBe('Open') - expect(typeof opt.id).toBe('string') - expect(opt.id.length).toBeGreaterThan(0) - }) - - it('generates an id for an object option without one', () => { - const [opt] = normalizeSelectOptionsInput([{ name: 'Closed' }]) ?? [] - expect(opt.name).toBe('Closed') - expect(opt.id.length).toBeGreaterThan(0) - }) - - it('preserves an explicitly supplied id', () => { - const result = normalizeSelectOptionsInput([{ id: 'opt_keep', name: 'Open' }]) - expect(result).toEqual([{ id: 'opt_keep', name: 'Open' }]) - }) - - it('reuses the id of an existing option with the same name', () => { - // The agent re-sends options as bare names on every edit. Minting fresh ids - // would orphan every cell holding them — silently clearing the column. - const existing = [ - { id: 'opt_low', name: 'Low' }, - { id: 'opt_high', name: 'High' }, - ] - const result = normalizeSelectOptionsInput(['Low', 'Medium', 'High'], existing) ?? [] - - expect(result[0]).toEqual({ id: 'opt_low', name: 'Low' }) - expect(result[2]).toEqual({ id: 'opt_high', name: 'High' }) - // Only the genuinely new option gets a fresh id. - expect(result[1].name).toBe('Medium') - expect(result[1].id).not.toBe('opt_low') - expect(result[1].id).not.toBe('opt_high') - }) - - it('matches an existing option name case-insensitively', () => { - const result = normalizeSelectOptionsInput(['open'], [{ id: 'opt_open', name: 'Open' }]) ?? [] - expect(result[0].id).toBe('opt_open') - expect(result[0].name).toBe('open') - }) - - it('mints a fresh id when there is no existing column to match against', () => { - const result = normalizeSelectOptionsInput(['Open']) ?? [] - expect(result[0].id.length).toBeGreaterThan(0) - expect(result[0].name).toBe('Open') - }) - - it('returns undefined for a non-array (validation rejects it downstream)', () => { - expect(normalizeSelectOptionsInput(undefined)).toBeUndefined() - expect(normalizeSelectOptionsInput('Open')).toBeUndefined() - }) -}) - describe('userTableServerTool.import_file', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.ts b/apps/sim/lib/copilot/tools/server/table/user-table.ts index 9f66adcb9da..705dcfbf01a 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.ts @@ -1,7 +1,7 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { getErrorMessage, toError } from '@sim/utils/errors' -import { generateId, generateShortId } from '@sim/utils/id' +import { generateId } from '@sim/utils/id' import { UserTable } from '@/lib/copilot/generated/tool-catalog-v1' import { assertServerToolNotAborted, @@ -27,26 +27,19 @@ import { validateMapping, } from '@/lib/table' import { namedRowMapper } from '@/lib/table/cell-format' -import { - buildIdByName, - columnMatchesRef, - rowDataNameToId, - sortSpecNamesToIds, -} from '@/lib/table/column-keys' +import { buildIdByName, rowDataNameToId, sortSpecNamesToIds } from '@/lib/table/column-keys' import { columnTypeForLeaf, deriveOutputColumnName } from '@/lib/table/column-naming' import { addTableColumn, deleteColumn, deleteColumns, renameColumn, - updateColumnConstraints, - updateColumnOptions, - updateColumnType, } from '@/lib/table/columns/service' import { markTableDeleteFailed, runTableDelete } from '@/lib/table/delete-runner' import { runTableImport, type TableImportPayload } from '@/lib/table/import-runner' import { markTableJobRunning, releaseJobClaim } from '@/lib/table/jobs/service' import { assertRowDelete, assertRowUpdate, patchColumnIds } from '@/lib/table/mutation-locks' +import { performUpdateTableColumn } from '@/lib/table/orchestration' import { predicateToFilter } from '@/lib/table/query-builder/converters' import { validatePredicate, validateSortSpec } from '@/lib/table/query-builder/validate' import { assertCursorSortBinding, decodeCursor } from '@/lib/table/rows/cursor' @@ -63,13 +56,13 @@ import { updateRow, updateRowsByFilter, } from '@/lib/table/rows/service' +import { normalizeSelectOptionsInput } from '@/lib/table/select-options' import { predicateToStorage } from '@/lib/table/select-values' import { createTable, deleteTable, getTableById, renameTable } from '@/lib/table/service' import type { ColumnDefinition, Filter, RowData, - SelectOption, SortSpec, TableDefinition, TableDeleteJobPayload, @@ -331,30 +324,6 @@ function limitError(limit: unknown): string | null { * cell data survives the update. Non-array input returns `undefined`, letting * downstream validation reject a malformed / missing option set. */ -export function normalizeSelectOptionsInput( - raw: unknown, - existing: SelectOption[] = [] -): SelectOption[] | undefined { - if (!Array.isArray(raw)) return undefined - // Cells reference the option id, so an edit that re-sends the same option by - // name must reuse its id — minting a fresh one would orphan every cell - // holding it, silently clearing the column. - const idByName = new Map() - for (const option of existing) { - const key = option.name.toLowerCase() - if (!idByName.has(key)) idByName.set(key, option.id) - } - const resolveId = (name: string): string => idByName.get(name.toLowerCase()) ?? generateShortId() - - return raw.map((entry) => { - if (typeof entry === 'string') return { id: resolveId(entry), name: entry } - const e = (entry ?? {}) as { id?: unknown; name?: unknown } - const name = typeof e.name === 'string' ? e.name : String(e.name ?? '') - const id = typeof e.id === 'string' && e.id.length > 0 ? e.id : resolveId(name) - return { id, name } - }) -} - /** Rewrites every `select` column's options in an agent-authored create schema. */ function normalizeSchemaSelectColumns(schema: TableSchema): TableSchema { if (!schema || !Array.isArray(schema.columns)) return schema @@ -1664,87 +1633,39 @@ export const userTableServerTool: BaseServerTool message: 'At least one of newType, unique, options, or multiple must be provided', } } - const tableForUpdate = await getTableById(args.tableId) - if (!tableForUpdate || tableForUpdate.workspaceId !== workspaceId) { - return { success: false, message: `Table not found: ${args.tableId}` } - } - const requestId = generateId().slice(0, 8) - // The agent authors options by name; mint ids here, reusing the id of - // any option whose name already exists so its cells survive the edit. - const currentColumn = tableForUpdate.schema.columns.find((c) => - columnMatchesRef(c, colName) - ) - const existingOptions = currentColumn?.options ?? [] - const options = normalizeSelectOptionsInput(rawOptions, existingOptions) - // An agent restating the current type alongside new options must not - // go through `updateColumnType` — it early-returns on an unchanged - // type and would drop them. Mirrors the HTTP columns route. - const typeChanging = newType !== undefined && newType !== currentColumn?.type - let result: TableDefinition | undefined if (newType !== undefined && !(COLUMN_TYPES as readonly string[]).includes(newType)) { return { success: false, message: `Invalid column type "${newType}". Must be one of: ${COLUMN_TYPES.join(', ')}`, } } - // Each write below is its own locked transaction, so pairing any of - // them with a constraint write that is going to fail commits and then - // errors. Gate on the type the column ENDS UP with — an options-only - // update on an existing select column carries the same hazard as a - // conversion. Same guard the HTTP column routes apply. - const resultingType = newType ?? currentColumn?.type - if (uniqFlag === true && resultingType === 'select') { - return { - success: false, - message: `Cannot set column "${colName}" as unique: select columns cannot be unique.`, - } - } - if (typeChanging) { - assertNotAborted() - result = await updateColumnType( - { - tableId: args.tableId, - columnName: colName, - newType: newType as (typeof COLUMN_TYPES)[number], - options, - multiple, - }, - requestId - ) - } else if (options !== undefined || multiple !== undefined) { - // Editing an existing select column's option set / mode without a - // type change. `multiple` alone is a valid update — the catalog - // documents it as independent — so fall back to the column's current - // options rather than demanding the caller resend the whole list. - const nextOptions = options ?? existingOptions - if (nextOptions.length === 0) { - return { - success: false, - message: `Column "${colName}" is not a select column. Pass newType: "select" with options to convert it.`, - } - } - assertNotAborted() - result = await updateColumnOptions( - { tableId: args.tableId, columnName: colName, options: nextOptions, multiple }, - requestId - ) + const tableForUpdate = await getTableById(args.tableId) + if (!tableForUpdate || tableForUpdate.workspaceId !== workspaceId) { + return { success: false, message: `Table not found: ${args.tableId}` } } - if (uniqFlag !== undefined) { - assertNotAborted() - result = await updateColumnConstraints( - { tableId: args.tableId, columnName: colName, unique: uniqFlag }, - requestId - ) + assertNotAborted() + const outcome = await performUpdateTableColumn({ + table: tableForUpdate, + columnName: colName, + userId: context.userId, + updates: { + ...(newType !== undefined ? { type: newType as (typeof COLUMN_TYPES)[number] } : {}), + ...(uniqFlag !== undefined ? { unique: uniqFlag } : {}), + ...(rawOptions !== undefined ? { options: rawOptions } : {}), + ...(multiple !== undefined ? { multiple } : {}), + }, + }) + if (!outcome.success) { + return { success: false, message: outcome.error ?? 'Failed to update column' } } return { success: true, message: `Updated column "${colName}"`, // A payload that only restates the current type is a no-op; still // report the live schema rather than an undefined one. - data: { schema: (result ?? tableForUpdate).schema }, + data: { schema: (outcome.table ?? tableForUpdate).schema }, } } - case 'rename': { if (!args.tableId) { return { success: false, message: 'Table ID is required' } diff --git a/apps/sim/lib/table/orchestration/columns.test.ts b/apps/sim/lib/table/orchestration/columns.test.ts new file mode 100644 index 00000000000..2798b0ed5c7 --- /dev/null +++ b/apps/sim/lib/table/orchestration/columns.test.ts @@ -0,0 +1,177 @@ +/** + * @vitest-environment node + * + * The column-update guards. These used to live in four callers (UI route, v1, + * v2, copilot tool) and had drifted apart; they are asserted here once. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TableDefinition } from '@/lib/table/types' + +const { + mockRenameColumn, + mockUpdateColumnType, + mockUpdateColumnOptions, + mockUpdateColumnConstraints, + mockRecordAudit, +} = vi.hoisted(() => ({ + mockRenameColumn: vi.fn(), + mockUpdateColumnType: vi.fn(), + mockUpdateColumnOptions: vi.fn(), + mockUpdateColumnConstraints: vi.fn(), + mockRecordAudit: vi.fn(), +})) + +vi.mock('@sim/audit', () => ({ + AuditAction: { TABLE_UPDATED: 'table.updated' }, + AuditResourceType: { TABLE: 'table' }, + recordAudit: mockRecordAudit, +})) + +vi.mock('@/lib/table/columns/service', () => ({ + renameColumn: mockRenameColumn, + updateColumnConstraints: mockUpdateColumnConstraints, + updateColumnOptions: mockUpdateColumnOptions, + updateColumnType: mockUpdateColumnType, +})) + +import { TableLockedError } from '@/lib/table/mutation-locks' +import { performUpdateTableColumn } from '@/lib/table/orchestration/columns' + +const SELECT_COLUMN = { + id: 'col-1', + name: 'Status', + type: 'select' as const, + options: [{ id: 'opt_open', name: 'Open' }], +} +const TEXT_COLUMN = { id: 'col-2', name: 'Priority', type: 'text' as const } + +const TABLE = { + id: 'table-1', + name: 'Tasks', + workspaceId: 'ws-1', + schema: { columns: [SELECT_COLUMN, TEXT_COLUMN] }, +} as unknown as TableDefinition + +const UPDATED = { schema: { columns: [SELECT_COLUMN] } } as unknown as TableDefinition + +function run(updates: Record, columnName = 'Status') { + return performUpdateTableColumn({ + table: TABLE, + columnName, + userId: 'user-1', + updates, + requestId: 'req-1', + }) +} + +describe('performUpdateTableColumn', () => { + beforeEach(() => { + vi.clearAllMocks() + mockRenameColumn.mockResolvedValue(UPDATED) + mockUpdateColumnType.mockResolvedValue(UPDATED) + mockUpdateColumnOptions.mockResolvedValue(UPDATED) + mockUpdateColumnConstraints.mockResolvedValue(UPDATED) + }) + + it('refuses to make a select column unique before writing anything', async () => { + // Each write is its own locked transaction, so an un-gated constraint write + // commits the earlier writes and then throws, half-applying the change. + const result = await run({ unique: true }) + + expect(result).toMatchObject({ success: false, errorCode: 'validation' }) + expect(mockUpdateColumnConstraints).not.toHaveBeenCalled() + }) + + it('refuses a conversion to select that is also made unique', async () => { + const result = await run({ type: 'select', options: ['Done'], unique: true }, 'Priority') + + expect(result.errorCode).toBe('validation') + expect(mockUpdateColumnType).not.toHaveBeenCalled() + }) + + it('routes an unchanged type with options to the options update', async () => { + // updateColumnType early-returns on an unchanged type and would drop them. + await run({ type: 'select', options: ['Open', 'Closed'] }) + + expect(mockUpdateColumnType).not.toHaveBeenCalled() + expect(mockUpdateColumnOptions).toHaveBeenCalledWith( + expect.objectContaining({ columnName: 'Status' }), + 'req-1' + ) + }) + + it('reuses the id of an option resent by name so its cells survive', async () => { + await run({ options: ['Open', 'Blocked'] }) + + const [{ options }] = mockUpdateColumnOptions.mock.calls[0] + expect(options[0]).toEqual({ id: 'opt_open', name: 'Open' }) + expect(options[1].id).not.toBe('opt_open') + }) + + it('carries options and required through a real type change', async () => { + await run({ type: 'select', options: ['Done'], required: true }, 'Priority') + + expect(mockUpdateColumnOptions).not.toHaveBeenCalled() + expect(mockUpdateColumnType).toHaveBeenCalledWith( + expect.objectContaining({ newType: 'select', required: true }), + 'req-1' + ) + }) + + it('applies a rename before the later writes and targets the new name', async () => { + await run({ name: 'State', required: true }) + + expect(mockRenameColumn).toHaveBeenCalledWith( + { tableId: 'table-1', oldName: 'Status', newName: 'State' }, + 'req-1' + ) + expect(mockUpdateColumnConstraints).toHaveBeenCalledWith( + expect.objectContaining({ columnName: 'State' }), + 'req-1' + ) + }) + + it('rejects an options edit on a column that is not a select', async () => { + const result = await run({ multiple: true }, 'Priority') + + expect(result.errorCode).toBe('validation') + expect(mockUpdateColumnOptions).not.toHaveBeenCalled() + }) + + it('reports an empty payload as a validation failure', async () => { + const result = await run({}) + + expect(result).toMatchObject({ success: false, errorCode: 'validation' }) + expect(mockRecordAudit).not.toHaveBeenCalled() + }) + + it('classifies a table lock as locked and does not audit', async () => { + mockUpdateColumnConstraints.mockRejectedValue(new TableLockedError('update')) + + const result = await run({ required: true }) + + expect(result.errorCode).toBe('locked') + expect(mockRecordAudit).not.toHaveBeenCalled() + }) + + it('classifies a caller-fixable service error as validation', async () => { + mockUpdateColumnConstraints.mockRejectedValue(new Error('Column "State" already exists')) + + expect((await run({ required: true })).errorCode).toBe('validation') + }) + + it('classifies a missing column as not_found', async () => { + mockUpdateColumnConstraints.mockRejectedValue(new Error('Column "Nope" not found')) + + expect((await run({ required: true })).errorCode).toBe('not_found') + }) + + it('audits a successful update on every caller', async () => { + // The UI route and the copilot tool previously emitted no audit at all. + await run({ required: true }) + + expect(mockRecordAudit).toHaveBeenCalledWith( + expect.objectContaining({ workspaceId: 'ws-1', actorId: 'user-1', resourceId: 'table-1' }) + ) + }) +}) diff --git a/apps/sim/lib/table/orchestration/columns.ts b/apps/sim/lib/table/orchestration/columns.ts new file mode 100644 index 00000000000..15c87cce90e --- /dev/null +++ b/apps/sim/lib/table/orchestration/columns.ts @@ -0,0 +1,198 @@ +import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' +import { createLogger } from '@sim/logger' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import { generateRequestId } from '@/lib/core/utils/request' +import { columnMatchesRef } from '@/lib/table/column-keys' +import { + renameColumn, + updateColumnConstraints, + updateColumnOptions, + updateColumnType, +} from '@/lib/table/columns/service' +import { TableLockedError } from '@/lib/table/mutation-locks' +import { normalizeSelectOptionsInput } from '@/lib/table/select-options' +import type { ColumnType, SelectOption, TableDefinition } from '@/lib/table/types' + +const logger = createLogger('TableColumnOrchestration') + +export interface PerformUpdateTableColumnParams { + table: TableDefinition + columnName: string + userId: string + updates: { + name?: string + type?: ColumnType + required?: boolean + unique?: boolean + /** Accepts `{id,name}` pairs or bare names; ids are minted/reused as needed. */ + options?: unknown + multiple?: boolean + } + requestId?: string +} + +export interface PerformUpdateTableColumnResult { + success: boolean + error?: string + errorCode?: OrchestrationErrorCode + table?: TableDefinition +} + +/** + * Messages the column services raise for caller-fixable problems. They are + * thrown as plain `Error`s, so the classification lives here rather than being + * re-derived by each route. + */ +const VALIDATION_MESSAGE_FRAGMENTS = [ + 'already exists', + 'Cannot delete the last column', + 'Cannot set column', + 'Cannot set unique column', + 'Invalid column', + 'exceeds maximum', + 'incompatible', + 'duplicate', + 'option', +] as const + +function classify(error: unknown): PerformUpdateTableColumnResult { + if (error instanceof TableLockedError) { + return { success: false, error: error.message, errorCode: 'locked' } + } + if (error instanceof Error) { + const message = error.message + if (message.includes('not found') || message.includes('Table not found')) { + return { success: false, error: message, errorCode: 'not_found' } + } + if (VALIDATION_MESSAGE_FRAGMENTS.some((fragment) => message.includes(fragment))) { + return { success: false, error: message, errorCode: 'validation' } + } + } + return { success: false, error: 'Failed to update column', errorCode: 'internal' } +} + +/** + * Applies a column update — rename, type conversion, option-set edit, and + * constraint change — as the single implementation behind the UI route, the v1 + * and v2 public APIs, and the copilot table tool. + * + * Each underlying write is its own locked transaction, so the ordering and the + * two guards below are load-bearing: they exist to stop a partially-applied + * schema change, and previously lived in (and drifted between) four callers. + * The caller owns authentication and workspace scoping; by the time this runs, + * `table` is a table the actor may write. + */ +export async function performUpdateTableColumn( + params: PerformUpdateTableColumnParams +): Promise { + const { table, columnName, userId, updates } = params + const requestId = params.requestId ?? generateRequestId() + const tableId = table.id + + const currentColumn = table.schema.columns.find((c) => columnMatchesRef(c, columnName)) + const existingOptions: SelectOption[] = currentColumn?.options ?? [] + const options = normalizeSelectOptionsInput(updates.options, existingOptions) + + // A payload that repeats the current type must not go through + // `updateColumnType` — it early-returns on an unchanged type and would drop + // any options alongside it. Only a real type change routes there; an + // unchanged type with options routes to the options-only update. + const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type + + // Gate on the type the column ENDS UP with, not on whether the type is + // changing: an options-only update on an existing select column carries the + // same hazard as a conversion does. + const resultingType = updates.type ?? currentColumn?.type + if (updates.unique === true && resultingType === 'select') { + return { + success: false, + error: `Cannot set column "${columnName}" as unique: select columns cannot be unique.`, + errorCode: 'validation', + } + } + + const targetName = updates.name ?? columnName + let updated: TableDefinition | undefined + + try { + if (updates.name) { + updated = await renameColumn( + { tableId, oldName: columnName, newName: updates.name }, + requestId + ) + } + + if (typeChanging) { + updated = await updateColumnType( + { + tableId, + columnName: targetName, + newType: updates.type as ColumnType, + ...(options !== undefined ? { options } : {}), + ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), + // Forwarded so the conversion validates against the constraint this + // same request is about to set, not the column's current one. + ...(updates.required !== undefined ? { required: updates.required } : {}), + }, + requestId + ) + } else if (options !== undefined || updates.multiple !== undefined) { + // `multiple` alone is a valid update, so fall back to the column's current + // options rather than demanding the caller resend the whole list. + const nextOptions = options ?? existingOptions + if (nextOptions.length === 0) { + return { + success: false, + error: `Column "${columnName}" is not a select column. Pass type "select" with options to convert it.`, + errorCode: 'validation', + } + } + updated = await updateColumnOptions( + { + tableId, + columnName: targetName, + options: nextOptions, + ...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}), + // Forwarded so the removal guard validates against the constraint this + // same request is about to set, not the column's current one. + ...(updates.required !== undefined ? { required: updates.required } : {}), + }, + requestId + ) + } + + if (updates.required !== undefined || updates.unique !== undefined) { + updated = await updateColumnConstraints( + { + tableId, + columnName: targetName, + ...(updates.required !== undefined ? { required: updates.required } : {}), + ...(updates.unique !== undefined ? { unique: updates.unique } : {}), + }, + requestId + ) + } + } catch (error) { + logger.error(`[${requestId}] Failed to update column "${columnName}" on table ${tableId}`, { + error, + }) + return classify(error) + } + + if (!updated) { + return { success: false, error: 'No updates specified', errorCode: 'validation' } + } + + recordAudit({ + workspaceId: table.workspaceId, + actorId: userId, + action: AuditAction.TABLE_UPDATED, + resourceType: AuditResourceType.TABLE, + resourceId: tableId, + resourceName: table.name, + description: `Updated column "${columnName}" in table "${table.name}"`, + metadata: { columnName, updates }, + }) + + return { success: true, table: updated } +} diff --git a/apps/sim/lib/table/orchestration/index.ts b/apps/sim/lib/table/orchestration/index.ts index af29da6aced..8710edc1896 100644 --- a/apps/sim/lib/table/orchestration/index.ts +++ b/apps/sim/lib/table/orchestration/index.ts @@ -1,64 +1,3 @@ -import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' -import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' -import { generateRequestId } from '@/lib/core/utils/request' -import { getTableById, restoreTable, TableConflictError } from '@/lib/table/service' -import type { TableDefinition } from '@/lib/table/types' - -const logger = createLogger('TableOrchestration') - -export type TableOrchestrationErrorCode = 'not_found' | 'validation' | 'conflict' | 'internal' - -export interface PerformRestoreTableParams { - tableId: string - userId: string - requestId?: string -} - -export interface PerformRestoreTableResult { - success: boolean - error?: string - errorCode?: TableOrchestrationErrorCode - table?: TableDefinition -} - -export async function performRestoreTable( - params: PerformRestoreTableParams -): Promise { - const { tableId, userId } = params - const requestId = params.requestId ?? generateRequestId() - - const archivedTable = await getTableById(tableId, { includeArchived: true }) - if (!archivedTable) { - return { success: false, error: 'Table not found', errorCode: 'not_found' } - } - - try { - await restoreTable(tableId, requestId) - const table = (await getTableById(tableId)) ?? archivedTable - - logger.info(`[${requestId}] Restored table ${tableId}`) - - recordAudit({ - workspaceId: archivedTable.workspaceId, - actorId: userId, - action: AuditAction.TABLE_RESTORED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: table.name, - description: `Restored table "${table.name}"`, - metadata: { - tableName: table.name, - workspaceId: table.workspaceId, - }, - }) - - return { success: true, table } - } catch (error) { - logger.error(`[${requestId}] Failed to restore table ${tableId}`, { error }) - if (error instanceof TableConflictError) { - return { success: false, error: error.message, errorCode: 'conflict' } - } - return { success: false, error: toError(error).message, errorCode: 'internal' } - } -} +export { performUpdateTableColumn } from './columns' +export { performRestoreTable } from './restore' +export { performDeleteTable, performDeleteTableRow } from './tables' diff --git a/apps/sim/lib/table/orchestration/restore.ts b/apps/sim/lib/table/orchestration/restore.ts new file mode 100644 index 00000000000..1aff4de5da2 --- /dev/null +++ b/apps/sim/lib/table/orchestration/restore.ts @@ -0,0 +1,63 @@ +import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' +import { createLogger } from '@sim/logger' +import { toError } from '@sim/utils/errors' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import { generateRequestId } from '@/lib/core/utils/request' +import { getTableById, restoreTable, TableConflictError } from '@/lib/table/service' +import type { TableDefinition } from '@/lib/table/types' + +const logger = createLogger('TableOrchestration') + +export interface PerformRestoreTableParams { + tableId: string + userId: string + requestId?: string +} + +export interface PerformRestoreTableResult { + success: boolean + error?: string + errorCode?: OrchestrationErrorCode + table?: TableDefinition +} + +export async function performRestoreTable( + params: PerformRestoreTableParams +): Promise { + const { tableId, userId } = params + const requestId = params.requestId ?? generateRequestId() + + const archivedTable = await getTableById(tableId, { includeArchived: true }) + if (!archivedTable) { + return { success: false, error: 'Table not found', errorCode: 'not_found' } + } + + try { + await restoreTable(tableId, requestId) + const table = (await getTableById(tableId)) ?? archivedTable + + logger.info(`[${requestId}] Restored table ${tableId}`) + + recordAudit({ + workspaceId: archivedTable.workspaceId, + actorId: userId, + action: AuditAction.TABLE_RESTORED, + resourceType: AuditResourceType.TABLE, + resourceId: tableId, + resourceName: table.name, + description: `Restored table "${table.name}"`, + metadata: { + tableName: table.name, + workspaceId: table.workspaceId, + }, + }) + + return { success: true, table } + } catch (error) { + logger.error(`[${requestId}] Failed to restore table ${tableId}`, { error }) + if (error instanceof TableConflictError) { + return { success: false, error: error.message, errorCode: 'conflict' } + } + return { success: false, error: toError(error).message, errorCode: 'internal' } + } +} diff --git a/apps/sim/lib/table/orchestration/tables.test.ts b/apps/sim/lib/table/orchestration/tables.test.ts new file mode 100644 index 00000000000..3d82deea684 --- /dev/null +++ b/apps/sim/lib/table/orchestration/tables.test.ts @@ -0,0 +1,78 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TableDefinition } from '@/lib/table/types' + +const { mockDeleteTable, mockDeleteRow, mockCaptureServerEvent } = vi.hoisted(() => ({ + mockDeleteTable: vi.fn(), + mockDeleteRow: vi.fn(), + mockCaptureServerEvent: vi.fn(), +})) + +vi.mock('@/lib/table/service', () => ({ deleteTable: mockDeleteTable })) +vi.mock('@/lib/table/rows/service', () => ({ deleteRow: mockDeleteRow })) +vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: mockCaptureServerEvent })) + +import { TableLockedError } from '@/lib/table/mutation-locks' +import { performDeleteTable, performDeleteTableRow } from '@/lib/table/orchestration/tables' + +const TABLE = { id: 'table-1', name: 'Tasks', workspaceId: 'ws-1' } as unknown as TableDefinition + +describe('performDeleteTable', () => { + beforeEach(() => vi.clearAllMocks()) + + it('hands the actor to the service so it owns the audit', async () => { + // deleteTable audits only when a row was actually archived AND an actor is + // given. Callers that omitted the actor and audited themselves emitted + // TABLE_DELETED for a no-op delete of an already-archived table. + mockDeleteTable.mockResolvedValue(undefined) + + const result = await performDeleteTable({ table: TABLE, userId: 'user-1', requestId: 'req-1' }) + + expect(result.success).toBe(true) + expect(mockDeleteTable).toHaveBeenCalledWith('table-1', 'req-1', 'user-1') + expect(mockCaptureServerEvent).toHaveBeenCalledWith( + 'user-1', + 'table_deleted', + expect.objectContaining({ table_id: 'table-1' }), + expect.anything() + ) + }) + + it('classifies a delete lock as locked and emits no telemetry', async () => { + mockDeleteTable.mockRejectedValue(new TableLockedError('delete')) + + const result = await performDeleteTable({ table: TABLE, userId: 'user-1' }) + + expect(result).toMatchObject({ success: false, errorCode: 'locked' }) + expect(mockCaptureServerEvent).not.toHaveBeenCalled() + }) +}) + +describe('performDeleteTableRow', () => { + beforeEach(() => vi.clearAllMocks()) + + it('deletes through the row service so the lock and bookkeeping apply', async () => { + mockDeleteRow.mockResolvedValue(undefined) + + const result = await performDeleteTableRow({ table: TABLE, rowId: 'row-1', requestId: 'req-1' }) + + expect(result.success).toBe(true) + expect(mockDeleteRow).toHaveBeenCalledWith(TABLE, 'row-1', 'req-1') + }) + + it('classifies a delete lock as locked', async () => { + mockDeleteRow.mockRejectedValue(new TableLockedError('delete')) + + expect((await performDeleteTableRow({ table: TABLE, rowId: 'row-1' })).errorCode).toBe('locked') + }) + + it('classifies a missing row as not_found', async () => { + mockDeleteRow.mockRejectedValue(new Error('Row not found')) + + expect((await performDeleteTableRow({ table: TABLE, rowId: 'row-1' })).errorCode).toBe( + 'not_found' + ) + }) +}) diff --git a/apps/sim/lib/table/orchestration/tables.ts b/apps/sim/lib/table/orchestration/tables.ts new file mode 100644 index 00000000000..344a00a3db3 --- /dev/null +++ b/apps/sim/lib/table/orchestration/tables.ts @@ -0,0 +1,96 @@ +import { createLogger } from '@sim/logger' +import { toError } from '@sim/utils/errors' +import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import { generateRequestId } from '@/lib/core/utils/request' +import { captureServerEvent } from '@/lib/posthog/server' +import { TableLockedError } from '@/lib/table/mutation-locks' +import { deleteRow } from '@/lib/table/rows/service' +import { deleteTable } from '@/lib/table/service' +import type { TableDefinition } from '@/lib/table/types' + +const logger = createLogger('TableOrchestration') + +export interface PerformDeleteTableParams { + table: TableDefinition + userId: string + requestId?: string +} + +export interface PerformDeleteTableResult { + success: boolean + error?: string + errorCode?: OrchestrationErrorCode +} + +/** + * Archives a table on behalf of `userId`. + * + * `deleteTable` records the audit itself, but only when a row was actually + * archived AND an actor is supplied — omitting the actor is how the rollback + * callers opt out. Passing it here is what keeps a no-op delete of an + * already-archived table from emitting a `TABLE_DELETED` event, which is + * exactly what the callers that hand-rolled their own audit used to do. + */ +export async function performDeleteTable( + params: PerformDeleteTableParams +): Promise { + const { table, userId } = params + const requestId = params.requestId ?? generateRequestId() + + try { + await deleteTable(table.id, requestId, userId) + } catch (error) { + if (error instanceof TableLockedError) { + return { success: false, error: error.message, errorCode: 'locked' } + } + logger.error(`[${requestId}] Failed to delete table ${table.id}`, { error }) + return { success: false, error: toError(error).message, errorCode: 'internal' } + } + + captureServerEvent( + userId, + 'table_deleted', + { table_id: table.id, workspace_id: table.workspaceId }, + { groups: { workspace: table.workspaceId } } + ) + + return { success: true } +} + +export interface PerformDeleteTableRowParams { + table: TableDefinition + rowId: string + requestId?: string +} + +export interface PerformDeleteTableRowResult { + success: boolean + error?: string + errorCode?: OrchestrationErrorCode +} + +/** + * Deletes a single row through the row service, so the delete lock is enforced + * and the row-count and ordering bookkeeping runs. A raw `db.delete` skips both + * and returns success on a locked table. + */ +export async function performDeleteTableRow( + params: PerformDeleteTableRowParams +): Promise { + const { table, rowId } = params + const requestId = params.requestId ?? generateRequestId() + + try { + await deleteRow(table, rowId, requestId) + return { success: true } + } catch (error) { + if (error instanceof TableLockedError) { + return { success: false, error: error.message, errorCode: 'locked' } + } + if (error instanceof Error && error.message === 'Row not found') { + return { success: false, error: 'Row not found', errorCode: 'not_found' } + } + logger.error(`[${requestId}] Failed to delete row ${rowId} from table ${table.id}`, { error }) + return { success: false, error: toError(error).message, errorCode: 'internal' } + } +} diff --git a/apps/sim/lib/table/select-options.test.ts b/apps/sim/lib/table/select-options.test.ts new file mode 100644 index 00000000000..36841dc9926 --- /dev/null +++ b/apps/sim/lib/table/select-options.test.ts @@ -0,0 +1,59 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { normalizeSelectOptionsInput } from '@/lib/table/select-options' + +describe('normalizeSelectOptionsInput', () => { + it('generates a stable id for a bare-name string option', () => { + const [opt] = normalizeSelectOptionsInput(['Open']) ?? [] + expect(opt.name).toBe('Open') + expect(typeof opt.id).toBe('string') + expect(opt.id.length).toBeGreaterThan(0) + }) + + it('generates an id for an object option without one', () => { + const [opt] = normalizeSelectOptionsInput([{ name: 'Closed' }]) ?? [] + expect(opt.name).toBe('Closed') + expect(opt.id.length).toBeGreaterThan(0) + }) + + it('preserves an explicitly supplied id', () => { + const result = normalizeSelectOptionsInput([{ id: 'opt_keep', name: 'Open' }]) + expect(result).toEqual([{ id: 'opt_keep', name: 'Open' }]) + }) + + it('reuses the id of an existing option with the same name', () => { + // The agent re-sends options as bare names on every edit. Minting fresh ids + // would orphan every cell holding them — silently clearing the column. + const existing = [ + { id: 'opt_low', name: 'Low' }, + { id: 'opt_high', name: 'High' }, + ] + const result = normalizeSelectOptionsInput(['Low', 'Medium', 'High'], existing) ?? [] + + expect(result[0]).toEqual({ id: 'opt_low', name: 'Low' }) + expect(result[2]).toEqual({ id: 'opt_high', name: 'High' }) + // Only the genuinely new option gets a fresh id. + expect(result[1].name).toBe('Medium') + expect(result[1].id).not.toBe('opt_low') + expect(result[1].id).not.toBe('opt_high') + }) + + it('matches an existing option name case-insensitively', () => { + const result = normalizeSelectOptionsInput(['open'], [{ id: 'opt_open', name: 'Open' }]) ?? [] + expect(result[0].id).toBe('opt_open') + expect(result[0].name).toBe('open') + }) + + it('mints a fresh id when there is no existing column to match against', () => { + const result = normalizeSelectOptionsInput(['Open']) ?? [] + expect(result[0].id.length).toBeGreaterThan(0) + expect(result[0].name).toBe('Open') + }) + + it('returns undefined for a non-array (validation rejects it downstream)', () => { + expect(normalizeSelectOptionsInput(undefined)).toBeUndefined() + expect(normalizeSelectOptionsInput('Open')).toBeUndefined() + }) +}) diff --git a/apps/sim/lib/table/select-options.ts b/apps/sim/lib/table/select-options.ts new file mode 100644 index 00000000000..90202ade8d5 --- /dev/null +++ b/apps/sim/lib/table/select-options.ts @@ -0,0 +1,33 @@ +import { generateShortId } from '@sim/utils/id' +import type { SelectOption } from '@/lib/table/types' + +/** + * Normalizes caller-supplied select options to `{ id, name }` pairs. + * + * Cells reference the option id, so an edit that re-sends an option by name + * must reuse the id it already has — minting a fresh one would orphan every + * cell holding it, silently clearing the column. A caller that already supplies + * an id keeps it, which makes this a no-op for the fully-formed options the + * HTTP contracts accept and a repair for the name-only options agents author. + */ +export function normalizeSelectOptionsInput( + raw: unknown, + existing: SelectOption[] = [] +): SelectOption[] | undefined { + if (!Array.isArray(raw)) return undefined + + const idByName = new Map() + for (const option of existing) { + const key = option.name.toLowerCase() + if (!idByName.has(key)) idByName.set(key, option.id) + } + const resolveId = (name: string): string => idByName.get(name.toLowerCase()) ?? generateShortId() + + return raw.map((entry) => { + if (typeof entry === 'string') return { id: resolveId(entry), name: entry } + const e = (entry ?? {}) as { id?: unknown; name?: unknown } + const name = typeof e.name === 'string' ? e.name : String(e.name ?? '') + const id = typeof e.id === 'string' && e.id.length > 0 ? e.id : resolveId(name) + return { id, name } + }) +} From 13ab9eaa452772f453b24e10f980f5dd11b4cf67 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 16:05:39 -0700 Subject: [PATCH 3/8] test(tables): bind the column-update tests to the orchestration function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The base's route tests assert which column service each payload reaches — the behavior that now lives in performUpdateTableColumn. They mocked the `@/lib/table` barrel; the orchestration module imports the service directly, so they mock that too and keep asserting the same thing through the extracted implementation. The orchestration tests move onto the base's semantics: writes address the stable column id, a rename rides inside the write it accompanies rather than running first, and the currency guards replace the non-select options guard the service now owns. Co-Authored-By: Claude Opus 5 --- .../api/table/[tableId]/columns/route.test.ts | 7 ++++ .../lib/table/orchestration/columns.test.ts | 40 ++++++++++++++----- apps/sim/lib/table/orchestration/columns.ts | 10 ++++- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/apps/sim/app/api/table/[tableId]/columns/route.test.ts b/apps/sim/app/api/table/[tableId]/columns/route.test.ts index 4ac282861cd..eb9a0faaf01 100644 --- a/apps/sim/app/api/table/[tableId]/columns/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/columns/route.test.ts @@ -42,6 +42,13 @@ vi.mock('@/lib/table', () => ({ updateColumnOptions: mockUpdateColumnOptions, updateColumnType: mockUpdateColumnType, })) +vi.mock('@/lib/table/columns/service', () => ({ + renameColumn: mockRenameColumn, + updateColumnConstraints: mockUpdateColumnConstraints, + updateColumnCurrency: mockUpdateColumnCurrency, + updateColumnOptions: mockUpdateColumnOptions, + updateColumnType: mockUpdateColumnType, +})) vi.mock('@/app/api/table/utils', () => ({ accessError: () => new Response('denied', { status: 403 }), checkAccess: mockCheckAccess, diff --git a/apps/sim/lib/table/orchestration/columns.test.ts b/apps/sim/lib/table/orchestration/columns.test.ts index 2798b0ed5c7..31ac72bb160 100644 --- a/apps/sim/lib/table/orchestration/columns.test.ts +++ b/apps/sim/lib/table/orchestration/columns.test.ts @@ -12,12 +12,14 @@ const { mockUpdateColumnType, mockUpdateColumnOptions, mockUpdateColumnConstraints, + mockUpdateColumnCurrency, mockRecordAudit, } = vi.hoisted(() => ({ mockRenameColumn: vi.fn(), mockUpdateColumnType: vi.fn(), mockUpdateColumnOptions: vi.fn(), mockUpdateColumnConstraints: vi.fn(), + mockUpdateColumnCurrency: vi.fn(), mockRecordAudit: vi.fn(), })) @@ -30,6 +32,7 @@ vi.mock('@sim/audit', () => ({ vi.mock('@/lib/table/columns/service', () => ({ renameColumn: mockRenameColumn, updateColumnConstraints: mockUpdateColumnConstraints, + updateColumnCurrency: mockUpdateColumnCurrency, updateColumnOptions: mockUpdateColumnOptions, updateColumnType: mockUpdateColumnType, })) @@ -71,6 +74,7 @@ describe('performUpdateTableColumn', () => { mockUpdateColumnType.mockResolvedValue(UPDATED) mockUpdateColumnOptions.mockResolvedValue(UPDATED) mockUpdateColumnConstraints.mockResolvedValue(UPDATED) + mockUpdateColumnCurrency.mockResolvedValue(UPDATED) }) it('refuses to make a select column unique before writing anything', async () => { @@ -94,8 +98,9 @@ describe('performUpdateTableColumn', () => { await run({ type: 'select', options: ['Open', 'Closed'] }) expect(mockUpdateColumnType).not.toHaveBeenCalled() + // Addressed by stable id so a rename folded into the write can't break it. expect(mockUpdateColumnOptions).toHaveBeenCalledWith( - expect.objectContaining({ columnName: 'Status' }), + expect.objectContaining({ columnName: 'col-1' }), 'req-1' ) }) @@ -118,24 +123,41 @@ describe('performUpdateTableColumn', () => { ) }) - it('applies a rename before the later writes and targets the new name', async () => { + it('folds a rename into the write it rides on rather than running it separately', async () => { + // A rename is metadata-only, so folding it into the last write's transaction + // is what stops a combined request committing one half and failing the other. await run({ name: 'State', required: true }) - expect(mockRenameColumn).toHaveBeenCalledWith( - { tableId: 'table-1', oldName: 'Status', newName: 'State' }, + expect(mockRenameColumn).not.toHaveBeenCalled() + expect(mockUpdateColumnConstraints).toHaveBeenCalledWith( + expect.objectContaining({ columnName: 'col-1', newName: 'State' }), 'req-1' ) - expect(mockUpdateColumnConstraints).toHaveBeenCalledWith( - expect.objectContaining({ columnName: 'State' }), + }) + + it('runs a rename standalone when there is no write to ride on', async () => { + mockRenameColumn.mockResolvedValue(UPDATED) + + await run({ name: 'State' }) + + expect(mockRenameColumn).toHaveBeenCalledWith( + { tableId: 'table-1', oldName: 'col-1', newName: 'State' }, 'req-1' ) }) - it('rejects an options edit on a column that is not a select', async () => { - const result = await run({ multiple: true }, 'Priority') + it('rejects setting a currency code on a non-currency column', async () => { + const result = await run({ currencyCode: 'USD' }) + + expect(result).toMatchObject({ success: false, errorCode: 'validation' }) + expect(mockUpdateColumnCurrency).not.toHaveBeenCalled() + }) + + it('rejects an unsupported currency code before any write', async () => { + const result = await run({ type: 'currency', currencyCode: 'XX' }, 'Priority') expect(result.errorCode).toBe('validation') - expect(mockUpdateColumnOptions).not.toHaveBeenCalled() + expect(mockUpdateColumnType).not.toHaveBeenCalled() }) it('reports an empty payload as a validation failure', async () => { diff --git a/apps/sim/lib/table/orchestration/columns.ts b/apps/sim/lib/table/orchestration/columns.ts index 8fa31da6e8a..3fb96a73c71 100644 --- a/apps/sim/lib/table/orchestration/columns.ts +++ b/apps/sim/lib/table/orchestration/columns.ts @@ -160,7 +160,10 @@ export async function performUpdateTableColumn( ) { return fail(`Column "${updates.name}" already exists`, 'validation') } - if (currentColumn.workflowGroupId && (updates.required !== undefined || updates.unique !== undefined)) { + if ( + currentColumn.workflowGroupId && + (updates.required !== undefined || updates.unique !== undefined) + ) { return fail( `Cannot change constraints on workflow-output column "${currentColumn.name}". Constraints aren't applicable to columns whose values come from workflow execution.`, 'validation' @@ -241,7 +244,10 @@ export async function performUpdateTableColumn( // stops a combined request from committing one half and then failing. Only // a rename with nothing to ride on runs standalone. if (updates.name && !updated) { - updated = await renameColumn({ tableId, oldName: columnRef, newName: updates.name }, requestId) + updated = await renameColumn( + { tableId, oldName: columnRef, newName: updates.name }, + requestId + ) } } catch (error) { logger.error(`[${requestId}] Failed to update column "${columnName}" on table ${tableId}`, { From f3dd1fed43bdc9998fd9ff748ebbb7062df995ba Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 16:06:16 -0700 Subject: [PATCH 4/8] chore(copilot): drop the column-type import the delegation made dead Co-Authored-By: Claude Opus 5 --- apps/sim/lib/copilot/tools/server/table/user-table.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.ts b/apps/sim/lib/copilot/tools/server/table/user-table.ts index c3c1998d8ce..ad3716ec55c 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.ts @@ -29,7 +29,6 @@ import { import { namedRowMapper } from '@/lib/table/cell-format' import { buildIdByName, rowDataNameToId, sortSpecNamesToIds } from '@/lib/table/column-keys' import { columnTypeForLeaf, deriveOutputColumnName } from '@/lib/table/column-naming' -import { columnTypeById } from '@/lib/table/column-types' import { addTableColumn, deleteColumn, From fc0e11935a7f97d14bf5cd56f35c88fd4a50ca94 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 16:33:30 -0700 Subject: [PATCH 5/8] refactor(tables): move the audit log out of the table service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lib/table/service.ts` wrote its own audit rows, so whether an operation was audited depended on which function a caller reached for rather than on a user having performed it. That is what let v1 and v2 audit a no-op delete, and what made `deleteTable`'s optional `actingUserId` double as an audit opt-out flag. Worse, most sites fell back to `actingUserId ?? createdBy`, so an unattributed call was logged against the table's *creator*. The copilot `mv` path passed no actor at all: renaming someone else's table recorded them as the renamer. Audit now lives in the orchestration functions — performDeleteTable, performRenameTable, performMoveTableToFolder, performUpdateTableLocks — and the services just write. Internal callers (folder cascade, import rollback) keep calling the service and are silent by construction rather than by remembering to omit an argument. Two services now return what the audit needs: `deleteTable` reports whether it actually archived a row, so a repeat delete logs nothing; `updateTableLocks` returns the before/after locks, since only the locked write can observe the transition its description names. Co-Authored-By: Claude Opus 5 --- .../sim/app/api/table/[tableId]/route.test.ts | 20 +- apps/sim/app/api/table/[tableId]/route.ts | 71 ++++--- .../lib/copilot/tools/handlers/vfs-mutate.ts | 24 ++- .../copilot/tools/server/table/user-table.ts | 31 +++- apps/sim/lib/folders/config.ts | 2 +- apps/sim/lib/table/orchestration/index.ts | 8 +- .../lib/table/orchestration/tables.test.ts | 47 +++-- apps/sim/lib/table/orchestration/tables.ts | 173 +++++++++++++++++- apps/sim/lib/table/service.ts | 102 ++--------- 9 files changed, 329 insertions(+), 149 deletions(-) diff --git a/apps/sim/app/api/table/[tableId]/route.test.ts b/apps/sim/app/api/table/[tableId]/route.test.ts index 7f1f48243c7..2396ba13a21 100644 --- a/apps/sim/app/api/table/[tableId]/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/route.test.ts @@ -33,6 +33,13 @@ vi.mock('@/lib/table', () => ({ updateTableLocks: mockUpdateTableLocks, TableConflictError: class extends Error {}, })) +vi.mock('@/lib/table/service', () => ({ + deleteTable: mockDeleteTable, + getTableById: mockGetTableById, + moveTableToFolder: mockMoveTableToFolder, + renameTable: mockRenameTable, + updateTableLocks: mockUpdateTableLocks, +})) vi.mock('@/lib/table/billing', () => ({ getWorkspaceTableLimits: mockGetLimits })) vi.mock('@/lib/folders/queries', () => ({ findActiveFolder: mockFindActiveFolder })) vi.mock('@/lib/core/config/feature-flags', () => ({ isFeatureEnabled: vi.fn() })) @@ -77,6 +84,13 @@ const routeContext = { params: Promise.resolve({ tableId: 'tbl_1' }) } describe('PATCH /api/table/[tableId] folder moves', () => { beforeEach(() => { vi.clearAllMocks() + mockMoveTableToFolder.mockResolvedValue({ name: 'Table' }) + mockRenameTable.mockResolvedValue({ id: 'tbl_1', name: 'Table' }) + mockDeleteTable.mockResolvedValue({ archived: { name: 'Table', workspaceId: 'workspace-1' } }) + mockUpdateTableLocks.mockResolvedValue({ + table: { ...TABLE, locks: {} }, + previousLocks: {}, + }) hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ success: true, userId: 'user-1', @@ -99,8 +113,7 @@ describe('PATCH /api/table/[tableId] folder moves', () => { 'tbl_1', 'workspace-1', 'folder-1', - expect.any(String), - 'user-1' + expect.any(String) ) }) @@ -118,8 +131,7 @@ describe('PATCH /api/table/[tableId] folder moves', () => { 'tbl_1', 'workspace-1', null, - expect.any(String), - 'user-1' + expect.any(String) ) }) diff --git a/apps/sim/app/api/table/[tableId]/route.ts b/apps/sim/app/api/table/[tableId]/route.ts index 0c428056826..4644f635cb4 100644 --- a/apps/sim/app/api/table/[tableId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/route.ts @@ -9,16 +9,14 @@ import { statusForOrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { findActiveFolder } from '@/lib/folders/queries' -import { - getTableById, - moveTableToFolder, - renameTable, - TableConflictError, - type TableSchema, - updateTableLocks, -} from '@/lib/table' +import { getTableById, TableConflictError, type TableSchema } from '@/lib/table' import { getWorkspaceTableLimits } from '@/lib/table/billing' -import { performDeleteTable } from '@/lib/table/orchestration' +import { + performDeleteTable, + performMoveTableToFolder, + performRenameTable, + performUpdateTableLocks, +} from '@/lib/table/orchestration' import { TABLE_LOCK_FLAGS, TABLE_LOCK_KINDS } from '@/lib/table/types' import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils' import { @@ -180,11 +178,34 @@ export const PATCH = withRouteHandler( { status: 403 } ) } - await updateTableLocks(tableId, validated.locks, authResult.userId, requestId, request) + const lockOutcome = await performUpdateTableLocks({ + tableId, + partial: validated.locks, + userId: authResult.userId, + requestId, + request, + }) + if (!lockOutcome.success) { + return NextResponse.json( + { error: lockOutcome.error ?? 'Failed to update table locks' }, + { status: statusForOrchestrationError(lockOutcome.errorCode) } + ) + } } if (validated.name !== undefined) { - await renameTable(tableId, validated.name, requestId, authResult.userId) + const renameOutcome = await performRenameTable({ + table, + newName: validated.name, + userId: authResult.userId, + requestId, + }) + if (!renameOutcome.success) { + return NextResponse.json( + { error: renameOutcome.error ?? 'Failed to rename table' }, + { status: statusForOrchestrationError(renameOutcome.errorCode) } + ) + } } if (validated.folderId !== undefined) { @@ -196,21 +217,21 @@ export const PATCH = withRouteHandler( ) { return NextResponse.json({ error: 'Folder not found in this workspace' }, { status: 404 }) } - try { - await moveTableToFolder( - tableId, - table.workspaceId, - validated.folderId, - requestId, - authResult.userId + // The move re-asserts workspace and active state, so a miss means the table was + // archived between `checkAccess` and the write. That is a 404, not a server fault. + const moveOutcome = await performMoveTableToFolder({ + table, + folderId: validated.folderId, + userId: authResult.userId, + requestId, + }) + if (!moveOutcome.success) { + return NextResponse.json( + { + error: moveOutcome.errorCode === 'not_found' ? 'Table not found' : moveOutcome.error, + }, + { status: statusForOrchestrationError(moveOutcome.errorCode) } ) - } catch (moveError) { - // The move re-asserts workspace and active state, so a miss means the table was - // archived between `checkAccess` and the write. That is a 404, not a server fault. - if (moveError instanceof Error && moveError.message.endsWith('not found')) { - return NextResponse.json({ error: 'Table not found' }, { status: 404 }) - } - throw moveError } } diff --git a/apps/sim/lib/copilot/tools/handlers/vfs-mutate.ts b/apps/sim/lib/copilot/tools/handlers/vfs-mutate.ts index e5b7ffefda4..602d71664cb 100644 --- a/apps/sim/lib/copilot/tools/handlers/vfs-mutate.ts +++ b/apps/sim/lib/copilot/tools/handlers/vfs-mutate.ts @@ -22,7 +22,8 @@ import { getKnowledgeBases, updateKnowledgeBase, } from '@/lib/knowledge/service' -import { deleteTable, listTables, renameTable } from '@/lib/table/service' +import { performDeleteTable, performRenameTable } from '@/lib/table/orchestration' +import { listTables } from '@/lib/table/service' import { ensureWorkspaceFileFolderPath, findWorkspaceFileFolderIdByPath, @@ -759,11 +760,19 @@ async function renameFlatResource( return { success: false, error: `Table not found at ${sources[0]}` } } assertMutationNotAborted(context) - const renamed = await renameTable(match.id, newName, generateRequestId()) + const renameOutcome = await performRenameTable({ + table: match, + newName, + userId: context.userId, + requestId: generateRequestId(), + }) + if (!renameOutcome.success) { + return { success: false, error: renameOutcome.error ?? 'Failed to rename table' } + } return buildResult(verb, [ { from: sources[0], - to: `tables/${normalizeVfsSegment(renamed.name)}`, + to: `tables/${normalizeVfsSegment(newName)}`, kind, id: match.id, }, @@ -1018,7 +1027,14 @@ async function removeTablePath( ) if (!match) return { from: path, kind: 'table', error: `Table not found at ${path}` } - await deleteTable(match.id, generateRequestId(), context.userId) + const outcome = await performDeleteTable({ + table: match, + userId: context.userId, + requestId: generateRequestId(), + }) + if (!outcome.success) { + return { from: path, kind: 'table', error: outcome.error ?? 'Failed to archive table' } + } logger.info('Archived table via rm', { tableId: match.id, workspaceId }) return { from: path, kind: 'table', id: match.id } } diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.ts b/apps/sim/lib/copilot/tools/server/table/user-table.ts index ad3716ec55c..3a9ca23f525 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.ts @@ -40,7 +40,11 @@ import { markTableDeleteFailed, runTableDelete } from '@/lib/table/delete-runner import { runTableImport, type TableImportPayload } from '@/lib/table/import-runner' import { markTableJobRunning, releaseJobClaim } from '@/lib/table/jobs/service' import { assertRowDelete, assertRowUpdate, patchColumnIds } from '@/lib/table/mutation-locks' -import { performUpdateTableColumn } from '@/lib/table/orchestration' +import { + performDeleteTable, + performRenameTable, + performUpdateTableColumn, +} from '@/lib/table/orchestration' import { predicateToFilter } from '@/lib/table/query-builder/converters' import { validatePredicate, validateSortSpec } from '@/lib/table/query-builder/validate' import { assertCursorSortBinding, decodeCursor } from '@/lib/table/rows/cursor' @@ -59,7 +63,7 @@ import { } from '@/lib/table/rows/service' import { normalizeSelectOptionsInput } from '@/lib/table/select-options' import { predicateToStorage } from '@/lib/table/select-values' -import { createTable, deleteTable, getTableById, renameTable } from '@/lib/table/service' +import { createTable, deleteTable, getTableById } from '@/lib/table/service' import type { ColumnDefinition, Filter, @@ -490,7 +494,14 @@ export const userTableServerTool: BaseServerTool const requestId = generateId().slice(0, 8) assertNotAborted() - await deleteTable(tableId, requestId, context.userId) + const deleteOutcome = await performDeleteTable({ + table, + userId: context.userId, + requestId, + }) + if (!deleteOutcome.success) { + return { success: false, message: deleteOutcome.error ?? 'Failed to delete table' } + } captureServerEvent( context.userId, 'table_deleted', @@ -1703,12 +1714,20 @@ export const userTableServerTool: BaseServerTool const requestId = generateId().slice(0, 8) assertNotAborted() - const renamed = await renameTable(args.tableId, newName, requestId, context.userId) + const renameOutcome = await performRenameTable({ + table, + newName, + userId: context.userId, + requestId, + }) + if (!renameOutcome.success) { + return { success: false, message: renameOutcome.error ?? 'Failed to rename table' } + } return { success: true, - message: `Renamed table to "${renamed.name}"`, - data: { table: { id: renamed.id, name: renamed.name } }, + message: `Renamed table to "${newName}"`, + data: { table: { id: args.tableId, name: newName } }, } } diff --git a/apps/sim/lib/folders/config.ts b/apps/sim/lib/folders/config.ts index 4c6b3d6333e..5c69755b75e 100644 --- a/apps/sim/lib/folders/config.ts +++ b/apps/sim/lib/folders/config.ts @@ -304,7 +304,7 @@ async function archiveTableChildren(context: CascadeChildrenContext): Promise ({ - mockDeleteTable: vi.fn(), - mockDeleteRow: vi.fn(), - mockCaptureServerEvent: vi.fn(), +const { mockDeleteTable, mockDeleteRow, mockCaptureServerEvent, mockRecordAudit } = vi.hoisted( + () => ({ + mockDeleteTable: vi.fn(), + mockDeleteRow: vi.fn(), + mockCaptureServerEvent: vi.fn(), + mockRecordAudit: vi.fn(), + }) +) + +vi.mock('@sim/audit', () => ({ + AuditAction: { TABLE_DELETED: 'table.deleted', TABLE_UPDATED: 'table.updated' }, + AuditResourceType: { TABLE: 'table' }, + recordAudit: mockRecordAudit, })) -vi.mock('@/lib/table/service', () => ({ deleteTable: mockDeleteTable })) +vi.mock('@/lib/table/service', () => ({ + deleteTable: mockDeleteTable, + moveTableToFolder: vi.fn(), + renameTable: vi.fn(), + updateTableLocks: vi.fn(), +})) vi.mock('@/lib/table/rows/service', () => ({ deleteRow: mockDeleteRow })) vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: mockCaptureServerEvent })) @@ -22,16 +36,18 @@ const TABLE = { id: 'table-1', name: 'Tasks', workspaceId: 'ws-1' } as unknown a describe('performDeleteTable', () => { beforeEach(() => vi.clearAllMocks()) - it('hands the actor to the service so it owns the audit', async () => { - // deleteTable audits only when a row was actually archived AND an actor is - // given. Callers that omitted the actor and audited themselves emitted - // TABLE_DELETED for a no-op delete of an already-archived table. - mockDeleteTable.mockResolvedValue(undefined) + it('audits a genuine archive against the acting user', async () => { + mockDeleteTable.mockResolvedValue({ archived: { name: 'Tasks', workspaceId: 'ws-1' } }) const result = await performDeleteTable({ table: TABLE, userId: 'user-1', requestId: 'req-1' }) expect(result.success).toBe(true) - expect(mockDeleteTable).toHaveBeenCalledWith('table-1', 'req-1', 'user-1') + // The service no longer takes an actor — auditing follows from a user + // performing the operation, not from which function the caller reached for. + expect(mockDeleteTable).toHaveBeenCalledWith('table-1', 'req-1') + expect(mockRecordAudit).toHaveBeenCalledWith( + expect.objectContaining({ actorId: 'user-1', resourceId: 'table-1' }) + ) expect(mockCaptureServerEvent).toHaveBeenCalledWith( 'user-1', 'table_deleted', @@ -40,6 +56,15 @@ describe('performDeleteTable', () => { ) }) + it('does not audit a repeat delete of an already-archived table', async () => { + mockDeleteTable.mockResolvedValue({ archived: null }) + + const result = await performDeleteTable({ table: TABLE, userId: 'user-1' }) + + expect(result.success).toBe(true) + expect(mockRecordAudit).not.toHaveBeenCalled() + }) + it('classifies a delete lock as locked and emits no telemetry', async () => { mockDeleteTable.mockRejectedValue(new TableLockedError('delete')) diff --git a/apps/sim/lib/table/orchestration/tables.ts b/apps/sim/lib/table/orchestration/tables.ts index 344a00a3db3..7f7b1ab054a 100644 --- a/apps/sim/lib/table/orchestration/tables.ts +++ b/apps/sim/lib/table/orchestration/tables.ts @@ -1,3 +1,4 @@ +import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' @@ -5,8 +6,13 @@ import { generateRequestId } from '@/lib/core/utils/request' import { captureServerEvent } from '@/lib/posthog/server' import { TableLockedError } from '@/lib/table/mutation-locks' import { deleteRow } from '@/lib/table/rows/service' -import { deleteTable } from '@/lib/table/service' -import type { TableDefinition } from '@/lib/table/types' +import { deleteTable, moveTableToFolder, renameTable, updateTableLocks } from '@/lib/table/service' +import { + TABLE_LOCK_FLAGS, + TABLE_LOCK_KINDS, + type TableDefinition, + type TableLocks, +} from '@/lib/table/types' const logger = createLogger('TableOrchestration') @@ -25,11 +31,11 @@ export interface PerformDeleteTableResult { /** * Archives a table on behalf of `userId`. * - * `deleteTable` records the audit itself, but only when a row was actually - * archived AND an actor is supplied — omitting the actor is how the rollback - * callers opt out. Passing it here is what keeps a no-op delete of an - * already-archived table from emitting a `TABLE_DELETED` event, which is - * exactly what the callers that hand-rolled their own audit used to do. + * The audit lives here rather than in `deleteTable` so that auditing follows + * from "a user performed this operation", not from which function a caller + * reached for. The rollback and cleanup paths call the service directly and + * are silent by construction, and a repeat delete of an already-archived table + * logs nothing because the service reports that it archived no row. */ export async function performDeleteTable( params: PerformDeleteTableParams @@ -37,8 +43,9 @@ export async function performDeleteTable( const { table, userId } = params const requestId = params.requestId ?? generateRequestId() + let archived: { name: string; workspaceId: string | null } | null try { - await deleteTable(table.id, requestId, userId) + ;({ archived } = await deleteTable(table.id, requestId)) } catch (error) { if (error instanceof TableLockedError) { return { success: false, error: error.message, errorCode: 'locked' } @@ -47,6 +54,18 @@ export async function performDeleteTable( return { success: false, error: toError(error).message, errorCode: 'internal' } } + if (archived) { + recordAudit({ + workspaceId: archived.workspaceId, + actorId: userId, + action: AuditAction.TABLE_DELETED, + resourceType: AuditResourceType.TABLE, + resourceId: table.id, + resourceName: archived.name, + description: `Archived table "${archived.name}"`, + }) + } + captureServerEvent( userId, 'table_deleted', @@ -94,3 +113,141 @@ export async function performDeleteTableRow( return { success: false, error: toError(error).message, errorCode: 'internal' } } } + +export interface PerformRenameTableParams { + table: TableDefinition + newName: string + userId: string + requestId?: string +} + +export interface PerformTableMutationResult { + success: boolean + error?: string + errorCode?: OrchestrationErrorCode + table?: TableDefinition +} + +function classifyTableMutation(error: unknown, requestId: string, tableId: string) { + if (error instanceof TableLockedError) { + return { success: false as const, error: error.message, errorCode: 'locked' as const } + } + const message = toError(error).message + if (message.includes('not found')) { + return { success: false as const, error: message, errorCode: 'not_found' as const } + } + if (message.includes('Invalid') || message.includes('already exists')) { + return { success: false as const, error: message, errorCode: 'validation' as const } + } + logger.error(`[${requestId}] Table mutation failed for ${tableId}`, { error }) + return { success: false as const, error: message, errorCode: 'internal' as const } +} + +/** Renames a table and records the rename against `userId`. */ +export async function performRenameTable( + params: PerformRenameTableParams +): Promise { + const { table, newName, userId } = params + const requestId = params.requestId ?? generateRequestId() + + try { + const renamed = await renameTable(table.id, newName, requestId) + recordAudit({ + workspaceId: table.workspaceId, + actorId: userId, + action: AuditAction.TABLE_UPDATED, + resourceType: AuditResourceType.TABLE, + resourceId: table.id, + resourceName: renamed.name, + description: `Renamed table to "${renamed.name}"`, + metadata: { op: 'rename', previousName: table.name }, + }) + return { success: true } + } catch (error) { + return classifyTableMutation(error, requestId, table.id) + } +} + +export interface PerformMoveTableParams { + table: TableDefinition + folderId: string | null + userId: string + requestId?: string +} + +/** Moves a table between folders (or to the workspace root). */ +export async function performMoveTableToFolder( + params: PerformMoveTableParams +): Promise { + const { table, folderId, userId } = params + const requestId = params.requestId ?? generateRequestId() + if (!table.workspaceId) { + return { success: false, error: 'Table is not in a workspace', errorCode: 'validation' } + } + + try { + const { name } = await moveTableToFolder(table.id, table.workspaceId, folderId, requestId) + recordAudit({ + workspaceId: table.workspaceId, + actorId: userId, + action: AuditAction.TABLE_UPDATED, + resourceType: AuditResourceType.TABLE, + resourceId: table.id, + resourceName: name, + description: folderId + ? `Moved table "${name}" into a folder` + : `Moved table "${name}" to the workspace root`, + metadata: { op: 'move', folderId }, + }) + return { success: true } + } catch (error) { + return classifyTableMutation(error, requestId, table.id) + } +} + +export interface PerformUpdateTableLocksParams { + tableId: string + partial: Partial + userId: string + requestId?: string + /** Forwarded to the audit record for IP / user-agent capture. */ + request?: { headers: { get(name: string): string | null } } +} + +/** + * Applies a lock change and names the transitions in the audit description, so + * the audit list answers "who locked my production table" without expanding + * metadata. The before/after state comes back from the service because only the + * locked write can observe it. + */ +export async function performUpdateTableLocks( + params: PerformUpdateTableLocksParams +): Promise { + const { tableId, partial, userId, request } = params + const requestId = params.requestId ?? generateRequestId() + + try { + const { table, previousLocks } = await updateTableLocks(tableId, partial, requestId) + const flipped = TABLE_LOCK_KINDS.filter( + (kind) => previousLocks[TABLE_LOCK_FLAGS[kind]] !== table.locks[TABLE_LOCK_FLAGS[kind]] + ) + recordAudit({ + workspaceId: table.workspaceId, + actorId: userId, + action: AuditAction.TABLE_UPDATED, + resourceType: AuditResourceType.TABLE, + resourceId: tableId, + resourceName: table.name, + description: flipped.length + ? `Table locks changed: ${flipped + .map((kind) => `${kind} ${table.locks[TABLE_LOCK_FLAGS[kind]] ? 'locked' : 'unlocked'}`) + .join(', ')}` + : 'Updated table locks (no change)', + metadata: { op: 'update_locks', before: previousLocks, after: table.locks }, + ...(request ? { request } : {}), + }) + return { success: true, table } + } catch (error) { + return classifyTableMutation(error, requestId, tableId) + } +} diff --git a/apps/sim/lib/table/service.ts b/apps/sim/lib/table/service.ts index 5d23ec56931..0af610c0d52 100644 --- a/apps/sim/lib/table/service.ts +++ b/apps/sim/lib/table/service.ts @@ -28,8 +28,6 @@ import type { DbTransaction } from '@/lib/table/planner' import { setTableTxTimeouts } from '@/lib/table/tx' import { type CreateTableData, - TABLE_LOCK_FLAGS, - TABLE_LOCK_KINDS, type TableDefinition, type TableLocks, type TableMetadata, @@ -572,8 +570,7 @@ export function auditTableColumnsAdded( export async function renameTable( tableId: string, newName: string, - requestId: string, - actingUserId?: string + requestId: string ): Promise<{ id: string; name: string }> { const nameValidation = validateTableName(newName) if (!nameValidation.valid) { @@ -586,31 +583,12 @@ export async function renameTable( .update(userTableDefinitions) .set({ name: newName, updatedAt: now }) .where(eq(userTableDefinitions.id, tableId)) - .returning({ - id: userTableDefinitions.id, - createdBy: userTableDefinitions.createdBy, - workspaceId: userTableDefinitions.workspaceId, - }) + .returning({ id: userTableDefinitions.id }) if (result.length === 0) { throw new Error(`Table ${tableId} not found`) } - const { createdBy, workspaceId } = result[0] - const renameActorId = actingUserId ?? createdBy - if (renameActorId) { - recordAudit({ - workspaceId: workspaceId ?? null, - actorId: renameActorId, - action: AuditAction.TABLE_UPDATED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: newName, - description: `Renamed table to "${newName}"`, - metadata: { op: 'rename' }, - }) - } - logger.info(`[${requestId}] Renamed table ${tableId} to "${newName}"`) return { id: tableId, name: newName } } catch (error: unknown) { @@ -636,9 +614,8 @@ export async function moveTableToFolder( tableId: string, workspaceId: string, folderId: string | null, - requestId: string, - actingUserId?: string -): Promise { + requestId: string +): Promise<{ name: string }> { const updates: Partial = { folderId, updatedAt: new Date(), @@ -669,24 +646,10 @@ export async function moveTableToFolder( throw new Error(`Table ${tableId} not found`) } - const { name, createdBy } = result[0] - const actorId = actingUserId ?? createdBy - if (actorId) { - recordAudit({ - workspaceId, - actorId, - action: AuditAction.TABLE_UPDATED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: name, - description: folderId - ? `Moved table "${name}" into a folder` - : `Moved table "${name}" to the workspace root`, - metadata: { op: 'move', folderId }, - }) - } + const { name } = result[0] logger.info(`[${requestId}] Moved table ${tableId} to folder ${folderId ?? 'root'}`) + return { name } } /** @@ -703,11 +666,8 @@ export async function moveTableToFolder( export async function updateTableLocks( tableId: string, partial: Partial, - actingUserId: string, - requestId: string, - /** Forwarded to the audit record for IP / user-agent capture. */ - request?: { headers: { get(name: string): string | null } } -): Promise { + requestId: string +): Promise<{ table: TableDefinition; previousLocks: TableLocks }> { let previousLocks: TableLocks = UNLOCKED_TABLE_LOCKS const updated = await withLockedTable(tableId, async (table, trx) => { previousLocks = table.locks @@ -720,36 +680,12 @@ export async function updateTableLocks( return { ...table, locks: nextLocks, updatedAt: now } }) - // Name the transitions in the description so the audit list is readable - // without expanding metadata — "who locked my production table" is the - // question this feature exists to answer. - const flipped = TABLE_LOCK_KINDS.filter( - (kind) => previousLocks[TABLE_LOCK_FLAGS[kind]] !== updated.locks[TABLE_LOCK_FLAGS[kind]] - ) - const description = flipped.length - ? `Table locks changed: ${flipped - .map((kind) => `${kind} ${updated.locks[TABLE_LOCK_FLAGS[kind]] ? 'locked' : 'unlocked'}`) - .join(', ')}` - : 'Updated table locks (no change)' - - recordAudit({ - workspaceId: updated.workspaceId, - actorId: actingUserId, - action: AuditAction.TABLE_UPDATED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: updated.name, - description, - metadata: { op: 'update_locks', before: previousLocks, after: updated.locks }, - ...(request ? { request } : {}), - }) - await appendTableEvent({ kind: 'definition', tableId, reason: 'locks' }).catch((error) => { logger.warn(`[${requestId}] Failed to emit lock-change event for table ${tableId}`, { error }) }) logger.info(`[${requestId}] Updated locks for table ${tableId}`) - return updated + return { table: updated, previousLocks } } /** @@ -830,9 +766,8 @@ export async function updateTableMetadata( export async function deleteTable( tableId: string, requestId: string, - actingUserId?: string, options?: { archivedAt?: Date } -): Promise { +): Promise<{ archived: { name: string; workspaceId: string | null } | null }> { const now = options?.archivedAt ?? new Date() // Archiving destroys access to every row, so it is gated on the delete lock. // The guard is inline in the WHERE (atomic — no separate read, no TOCTOU); @@ -874,21 +809,10 @@ export async function deleteTable( } // Otherwise the table is missing or already archived — a silent no-op, as before. } - // Audit only genuine user deletes — rollback callers omit `actingUserId`. The - // caller emits the `table_deleted` PostHog event, so it is not duplicated here. - if (deleted && actingUserId) { - recordAudit({ - workspaceId: deleted.workspaceId ?? null, - actorId: actingUserId, - action: AuditAction.TABLE_DELETED, - resourceType: AuditResourceType.TABLE, - resourceId: tableId, - resourceName: deleted.name, - description: `Archived table "${deleted.name}"`, - }) - } - logger.info(`[${requestId}] Archived table ${tableId}`) + // Null when the table was missing or already archived — a silent no-op. The + // caller audits only a genuine archive, so a repeat delete logs nothing. + return { archived: deleted ? { name: deleted.name, workspaceId: deleted.workspaceId } : null } } /** From ad29585ff8b621be11c17290563098cd6bbc3938 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 17:04:55 -0700 Subject: [PATCH 6/8] fix(tables): restore audit provenance and conflict status in orchestration Moving the audits into the orchestration functions dropped three things the routes had been carrying, and added one the orchestration now owns twice. - The v1 and v2 column-update routes passed `request` to `recordAudit`, so their audit rows recorded the caller's IP and user-agent. The orchestration function had no way to receive it. Every table orchestration function now takes an optional `OrchestrationRequestContext` and every HTTP route forwards it; the copilot and VFS callers, which have no request, omit it. - `classifyTableMutation` matched `TableConflictError` on "already exists" appearing in the message and reported it as `validation`, turning the UI route's 409 on a duplicate table rename into a 400. It now matches the type, the way `performRestoreTable` already did. - `captureServerEvent` ran on every delete while the audit was gated on a row actually being archived, so a repeat delete of an archived table still reported `table_deleted`. Both now hang off the same evidence. - The copilot delete path kept its own `captureServerEvent` from when the service did not emit one, double-counting every copilot table delete. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YGzbVDZpe2dEALbu2BUU8a --- .../app/api/table/[tableId]/columns/route.ts | 1 + apps/sim/app/api/table/[tableId]/route.ts | 9 ++- .../api/v1/tables/[tableId]/columns/route.ts | 1 + apps/sim/app/api/v1/tables/[tableId]/route.ts | 2 +- .../api/v2/tables/[tableId]/columns/route.ts | 1 + apps/sim/app/api/v2/tables/[tableId]/route.ts | 2 +- .../copilot/tools/server/table/user-table.ts | 7 --- apps/sim/lib/core/orchestration/types.ts | 9 +++ apps/sim/lib/table/orchestration/columns.ts | 10 ++- .../lib/table/orchestration/tables.test.ts | 61 +++++++++++++++---- apps/sim/lib/table/orchestration/tables.ts | 55 ++++++++++++----- 11 files changed, 121 insertions(+), 37 deletions(-) diff --git a/apps/sim/app/api/table/[tableId]/columns/route.ts b/apps/sim/app/api/table/[tableId]/columns/route.ts index acf6754bc88..f55c4a0bc52 100644 --- a/apps/sim/app/api/table/[tableId]/columns/route.ts +++ b/apps/sim/app/api/table/[tableId]/columns/route.ts @@ -117,6 +117,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu userId: authResult.userId, updates: validated.updates, requestId, + request, }) if (!outcome.success || !outcome.table) { return NextResponse.json( diff --git a/apps/sim/app/api/table/[tableId]/route.ts b/apps/sim/app/api/table/[tableId]/route.ts index 4644f635cb4..7d75286cf83 100644 --- a/apps/sim/app/api/table/[tableId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/route.ts @@ -199,6 +199,7 @@ export const PATCH = withRouteHandler( newName: validated.name, userId: authResult.userId, requestId, + request, }) if (!renameOutcome.success) { return NextResponse.json( @@ -224,6 +225,7 @@ export const PATCH = withRouteHandler( folderId: validated.folderId, userId: authResult.userId, requestId, + request, }) if (!moveOutcome.success) { return NextResponse.json( @@ -289,7 +291,12 @@ export const DELETE = withRouteHandler( return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - const outcome = await performDeleteTable({ table, userId: authResult.userId, requestId }) + const outcome = await performDeleteTable({ + table, + userId: authResult.userId, + requestId, + request, + }) if (!outcome.success) { return NextResponse.json( { error: outcome.error ?? 'Failed to delete table' }, diff --git a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts index 65522340765..52296284215 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts @@ -151,6 +151,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu userId, updates: validated.updates, requestId, + request, }) if (!outcome.success || !outcome.table) { return NextResponse.json( diff --git a/apps/sim/app/api/v1/tables/[tableId]/route.ts b/apps/sim/app/api/v1/tables/[tableId]/route.ts index 1849c2e162d..149bc674651 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/route.ts @@ -140,7 +140,7 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) } - const outcome = await performDeleteTable({ table: result.table, userId, requestId }) + const outcome = await performDeleteTable({ table: result.table, userId, requestId, request }) if (!outcome.success) { return NextResponse.json( { error: outcome.error ?? 'Failed to delete table' }, diff --git a/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts index ce9085e588e..c5833176b15 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts @@ -138,6 +138,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu userId, updates: validated.updates, requestId, + request, }) if (!outcome.success || !outcome.table) { return v2ErrorForOrchestration(outcome.errorCode, outcome.error ?? 'Failed to update column') diff --git a/apps/sim/app/api/v2/tables/[tableId]/route.ts b/apps/sim/app/api/v2/tables/[tableId]/route.ts index dfb54287e80..55e2d792f8f 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/route.ts @@ -100,7 +100,7 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab return v2Error('NOT_FOUND', 'Table not found') } - const outcome = await performDeleteTable({ table: result.table, userId, requestId }) + const outcome = await performDeleteTable({ table: result.table, userId, requestId, request }) if (!outcome.success) { return v2ErrorForOrchestration(outcome.errorCode, outcome.error ?? 'Failed to delete table') } diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.ts b/apps/sim/lib/copilot/tools/server/table/user-table.ts index 3a9ca23f525..23950ca3bbf 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.ts @@ -10,7 +10,6 @@ import { } from '@/lib/copilot/tools/server/base-tool' import { isTriggerDevEnabled } from '@/lib/core/config/env-flags' import { runDetached } from '@/lib/core/utils/background' -import { captureServerEvent } from '@/lib/posthog/server' import { buildAutoMapping, COLUMN_TYPES, @@ -502,12 +501,6 @@ export const userTableServerTool: BaseServerTool if (!deleteOutcome.success) { return { success: false, message: deleteOutcome.error ?? 'Failed to delete table' } } - captureServerEvent( - context.userId, - 'table_deleted', - { table_id: tableId, workspace_id: workspaceId }, - { groups: { workspace: workspaceId } } - ) deleted.push(tableId) } diff --git a/apps/sim/lib/core/orchestration/types.ts b/apps/sim/lib/core/orchestration/types.ts index da6df20b0ba..7b6c4687d44 100644 --- a/apps/sim/lib/core/orchestration/types.ts +++ b/apps/sim/lib/core/orchestration/types.ts @@ -12,3 +12,12 @@ export function statusForOrchestrationError(code: OrchestrationErrorCode | undef if (code === 'locked') return 423 return 500 } + +/** + * The slice of an HTTP request the audit log reads for client IP and user-agent + * capture. Optional on every orchestration function so the non-HTTP callers — + * copilot tools, background jobs — can omit what they do not have. + */ +export interface OrchestrationRequestContext { + headers: { get(name: string): string | null } +} diff --git a/apps/sim/lib/table/orchestration/columns.ts b/apps/sim/lib/table/orchestration/columns.ts index 3fb96a73c71..4c1b82d3165 100644 --- a/apps/sim/lib/table/orchestration/columns.ts +++ b/apps/sim/lib/table/orchestration/columns.ts @@ -1,6 +1,9 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' -import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import type { + OrchestrationErrorCode, + OrchestrationRequestContext, +} from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys' import { columnTypeById } from '@/lib/table/column-types' @@ -33,6 +36,8 @@ export interface PerformUpdateTableColumnParams { currencyCode?: string } requestId?: string + /** Forwarded to the audit record for IP / user-agent capture. */ + request?: OrchestrationRequestContext } export interface PerformUpdateTableColumnResult { @@ -96,7 +101,7 @@ function fail(error: string, errorCode: OrchestrationErrorCode): PerformUpdateTa export async function performUpdateTableColumn( params: PerformUpdateTableColumnParams ): Promise { - const { table, columnName, userId, updates } = params + const { table, columnName, userId, updates, request } = params const requestId = params.requestId ?? generateRequestId() const tableId = table.id @@ -269,6 +274,7 @@ export async function performUpdateTableColumn( resourceName: table.name, description: `Updated column "${columnName}" in table "${table.name}"`, metadata: { columnName, updates }, + ...(request ? { request } : {}), }) return { success: true, table: updated } diff --git a/apps/sim/lib/table/orchestration/tables.test.ts b/apps/sim/lib/table/orchestration/tables.test.ts index f2a702432db..055f07afd85 100644 --- a/apps/sim/lib/table/orchestration/tables.test.ts +++ b/apps/sim/lib/table/orchestration/tables.test.ts @@ -4,14 +4,26 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { TableDefinition } from '@/lib/table/types' -const { mockDeleteTable, mockDeleteRow, mockCaptureServerEvent, mockRecordAudit } = vi.hoisted( - () => ({ - mockDeleteTable: vi.fn(), - mockDeleteRow: vi.fn(), - mockCaptureServerEvent: vi.fn(), - mockRecordAudit: vi.fn(), - }) -) +const { + mockDeleteTable, + mockDeleteRow, + mockRenameTable, + mockCaptureServerEvent, + mockRecordAudit, + MockTableConflictError, +} = vi.hoisted(() => ({ + mockDeleteTable: vi.fn(), + mockDeleteRow: vi.fn(), + mockRenameTable: vi.fn(), + mockCaptureServerEvent: vi.fn(), + mockRecordAudit: vi.fn(), + MockTableConflictError: class extends Error { + readonly code = 'TABLE_EXISTS' as const + constructor(name: string) { + super(`A table named "${name}" already exists in this workspace`) + } + }, +})) vi.mock('@sim/audit', () => ({ AuditAction: { TABLE_DELETED: 'table.deleted', TABLE_UPDATED: 'table.updated' }, @@ -22,14 +34,19 @@ vi.mock('@sim/audit', () => ({ vi.mock('@/lib/table/service', () => ({ deleteTable: mockDeleteTable, moveTableToFolder: vi.fn(), - renameTable: vi.fn(), + renameTable: mockRenameTable, updateTableLocks: vi.fn(), + TableConflictError: MockTableConflictError, })) vi.mock('@/lib/table/rows/service', () => ({ deleteRow: mockDeleteRow })) vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: mockCaptureServerEvent })) import { TableLockedError } from '@/lib/table/mutation-locks' -import { performDeleteTable, performDeleteTableRow } from '@/lib/table/orchestration/tables' +import { + performDeleteTable, + performDeleteTableRow, + performRenameTable, +} from '@/lib/table/orchestration/tables' const TABLE = { id: 'table-1', name: 'Tasks', workspaceId: 'ws-1' } as unknown as TableDefinition @@ -56,13 +73,23 @@ describe('performDeleteTable', () => { ) }) - it('does not audit a repeat delete of an already-archived table', async () => { + it('carries request provenance into the audit row', async () => { + mockDeleteTable.mockResolvedValue({ archived: { name: 'Tasks', workspaceId: 'ws-1' } }) + const request = new Request('https://sim.ai', { headers: { 'user-agent': 'curl/8' } }) + + await performDeleteTable({ table: TABLE, userId: 'user-1', request }) + + expect(mockRecordAudit).toHaveBeenCalledWith(expect.objectContaining({ request })) + }) + + it('neither audits nor reports a repeat delete of an already-archived table', async () => { mockDeleteTable.mockResolvedValue({ archived: null }) const result = await performDeleteTable({ table: TABLE, userId: 'user-1' }) expect(result.success).toBe(true) expect(mockRecordAudit).not.toHaveBeenCalled() + expect(mockCaptureServerEvent).not.toHaveBeenCalled() }) it('classifies a delete lock as locked and emits no telemetry', async () => { @@ -75,6 +102,18 @@ describe('performDeleteTable', () => { }) }) +describe('performRenameTable', () => { + beforeEach(() => vi.clearAllMocks()) + + it('classifies a name collision as a conflict, not bad input', async () => { + mockRenameTable.mockRejectedValue(new MockTableConflictError('Tasks')) + + const result = await performRenameTable({ table: TABLE, newName: 'Tasks', userId: 'user-1' }) + + expect(result).toMatchObject({ success: false, errorCode: 'conflict' }) + }) +}) + describe('performDeleteTableRow', () => { beforeEach(() => vi.clearAllMocks()) diff --git a/apps/sim/lib/table/orchestration/tables.ts b/apps/sim/lib/table/orchestration/tables.ts index 7f7b1ab054a..2c89fa19941 100644 --- a/apps/sim/lib/table/orchestration/tables.ts +++ b/apps/sim/lib/table/orchestration/tables.ts @@ -1,12 +1,21 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' -import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import type { + OrchestrationErrorCode, + OrchestrationRequestContext, +} from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { captureServerEvent } from '@/lib/posthog/server' import { TableLockedError } from '@/lib/table/mutation-locks' import { deleteRow } from '@/lib/table/rows/service' -import { deleteTable, moveTableToFolder, renameTable, updateTableLocks } from '@/lib/table/service' +import { + deleteTable, + moveTableToFolder, + renameTable, + TableConflictError, + updateTableLocks, +} from '@/lib/table/service' import { TABLE_LOCK_FLAGS, TABLE_LOCK_KINDS, @@ -20,6 +29,8 @@ export interface PerformDeleteTableParams { table: TableDefinition userId: string requestId?: string + /** Forwarded to the audit record for IP / user-agent capture. */ + request?: OrchestrationRequestContext } export interface PerformDeleteTableResult { @@ -40,7 +51,7 @@ export interface PerformDeleteTableResult { export async function performDeleteTable( params: PerformDeleteTableParams ): Promise { - const { table, userId } = params + const { table, userId, request } = params const requestId = params.requestId ?? generateRequestId() let archived: { name: string; workspaceId: string | null } | null @@ -54,6 +65,9 @@ export async function performDeleteTable( return { success: false, error: toError(error).message, errorCode: 'internal' } } + // Both the audit and the analytics event describe an archive that happened, so + // both hang off the same evidence that one did. A repeat delete of an + // already-archived table succeeds and records nothing. if (archived) { recordAudit({ workspaceId: archived.workspaceId, @@ -63,16 +77,16 @@ export async function performDeleteTable( resourceId: table.id, resourceName: archived.name, description: `Archived table "${archived.name}"`, + ...(request ? { request } : {}), }) + captureServerEvent( + userId, + 'table_deleted', + { table_id: table.id, workspace_id: table.workspaceId }, + { groups: { workspace: table.workspaceId } } + ) } - captureServerEvent( - userId, - 'table_deleted', - { table_id: table.id, workspace_id: table.workspaceId }, - { groups: { workspace: table.workspaceId } } - ) - return { success: true } } @@ -119,6 +133,8 @@ export interface PerformRenameTableParams { newName: string userId: string requestId?: string + /** Forwarded to the audit record for IP / user-agent capture. */ + request?: OrchestrationRequestContext } export interface PerformTableMutationResult { @@ -132,11 +148,18 @@ function classifyTableMutation(error: unknown, requestId: string, tableId: strin if (error instanceof TableLockedError) { return { success: false as const, error: error.message, errorCode: 'locked' as const } } + // A name collision is a conflict, not bad input — the same class + // `performRestoreTable` reports, and the 409 the UI route returned before it + // delegated. Matched on the type, not on "already exists" appearing in the + // message, so a rewording cannot silently demote it to a 400. + if (error instanceof TableConflictError) { + return { success: false as const, error: error.message, errorCode: 'conflict' as const } + } const message = toError(error).message if (message.includes('not found')) { return { success: false as const, error: message, errorCode: 'not_found' as const } } - if (message.includes('Invalid') || message.includes('already exists')) { + if (message.includes('Invalid')) { return { success: false as const, error: message, errorCode: 'validation' as const } } logger.error(`[${requestId}] Table mutation failed for ${tableId}`, { error }) @@ -147,7 +170,7 @@ function classifyTableMutation(error: unknown, requestId: string, tableId: strin export async function performRenameTable( params: PerformRenameTableParams ): Promise { - const { table, newName, userId } = params + const { table, newName, userId, request } = params const requestId = params.requestId ?? generateRequestId() try { @@ -161,6 +184,7 @@ export async function performRenameTable( resourceName: renamed.name, description: `Renamed table to "${renamed.name}"`, metadata: { op: 'rename', previousName: table.name }, + ...(request ? { request } : {}), }) return { success: true } } catch (error) { @@ -173,13 +197,15 @@ export interface PerformMoveTableParams { folderId: string | null userId: string requestId?: string + /** Forwarded to the audit record for IP / user-agent capture. */ + request?: OrchestrationRequestContext } /** Moves a table between folders (or to the workspace root). */ export async function performMoveTableToFolder( params: PerformMoveTableParams ): Promise { - const { table, folderId, userId } = params + const { table, folderId, userId, request } = params const requestId = params.requestId ?? generateRequestId() if (!table.workspaceId) { return { success: false, error: 'Table is not in a workspace', errorCode: 'validation' } @@ -198,6 +224,7 @@ export async function performMoveTableToFolder( ? `Moved table "${name}" into a folder` : `Moved table "${name}" to the workspace root`, metadata: { op: 'move', folderId }, + ...(request ? { request } : {}), }) return { success: true } } catch (error) { @@ -211,7 +238,7 @@ export interface PerformUpdateTableLocksParams { userId: string requestId?: string /** Forwarded to the audit record for IP / user-agent capture. */ - request?: { headers: { get(name: string): string | null } } + request?: OrchestrationRequestContext } /** From 0ee4499a71d2fed517896923f693fee00090cec6 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 17:12:46 -0700 Subject: [PATCH 7/8] fix(tables): say which type a no-op column update restated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A copilot `update_column` payload whose only content was the column's current type used to return success with the live schema, while the v1, v2, and UI routes rejected the same payload with "No updates specified". Delegating to `performUpdateTableColumn` unified them onto the routes' rejection — correct, but the message tells the caller its request was empty when it named a type. The orchestration function now reports the same thing `updateColumnType` reports when it loses this race concurrently: the column is already that type, re-issue without the type change. An empty payload still reads "No updates specified". Drops the copilot's `outcome.table ?? tableForUpdate` fallback with it — the comment described the no-op that can no longer reach that line, and a success always carries a table. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YGzbVDZpe2dEALbu2BUU8a --- apps/sim/lib/copilot/tools/server/table/user-table.ts | 6 ++---- apps/sim/lib/table/orchestration/columns.test.ts | 9 +++++++++ apps/sim/lib/table/orchestration/columns.ts | 9 +++++++++ 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/apps/sim/lib/copilot/tools/server/table/user-table.ts b/apps/sim/lib/copilot/tools/server/table/user-table.ts index 23950ca3bbf..93b0062c7b6 100644 --- a/apps/sim/lib/copilot/tools/server/table/user-table.ts +++ b/apps/sim/lib/copilot/tools/server/table/user-table.ts @@ -1677,15 +1677,13 @@ export const userTableServerTool: BaseServerTool ...(currencyCode !== undefined ? { currencyCode } : {}), }, }) - if (!outcome.success) { + if (!outcome.success || !outcome.table) { return { success: false, message: outcome.error ?? 'Failed to update column' } } return { success: true, message: `Updated column "${colName}"`, - // A payload that only restates the current type is a no-op; still - // report the live schema rather than an undefined one. - data: { schema: (outcome.table ?? tableForUpdate).schema }, + data: { schema: outcome.table.schema }, } } case 'rename': { diff --git a/apps/sim/lib/table/orchestration/columns.test.ts b/apps/sim/lib/table/orchestration/columns.test.ts index 31ac72bb160..c7dde28a8db 100644 --- a/apps/sim/lib/table/orchestration/columns.test.ts +++ b/apps/sim/lib/table/orchestration/columns.test.ts @@ -164,9 +164,18 @@ describe('performUpdateTableColumn', () => { const result = await run({}) expect(result).toMatchObject({ success: false, errorCode: 'validation' }) + expect(result.error).toBe('No updates specified') expect(mockRecordAudit).not.toHaveBeenCalled() }) + it("names the type when a payload only restates the column's current type", async () => { + const result = await run({ type: 'select' }) + + expect(result).toMatchObject({ success: false, errorCode: 'validation' }) + expect(result.error).toContain('is already type "select"') + expect(mockUpdateColumnType).not.toHaveBeenCalled() + }) + it('classifies a table lock as locked and does not audit', async () => { mockUpdateColumnConstraints.mockRejectedValue(new TableLockedError('update')) diff --git a/apps/sim/lib/table/orchestration/columns.ts b/apps/sim/lib/table/orchestration/columns.ts index 4c1b82d3165..b928f76a1ac 100644 --- a/apps/sim/lib/table/orchestration/columns.ts +++ b/apps/sim/lib/table/orchestration/columns.ts @@ -262,6 +262,15 @@ export async function performUpdateTableColumn( } if (!updated) { + // A payload whose only content is the type the column already has names a + // change and asks for nothing. Say which, the way `updateColumnType` does + // when it loses the same race, rather than claiming the request was empty. + if (updates.type !== undefined) { + return fail( + `Column "${currentColumn.name}" is already type "${currentColumn.type}"; re-issue the request without a type change.`, + 'validation' + ) + } return fail('No updates specified', 'validation') } From d44961048e5a7859ca86009308cd7386bcc7147a Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 31 Jul 2026 17:49:12 -0700 Subject: [PATCH 8/8] refactor(tables): classify failures by type instead of by message text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The table module decided HTTP statuses by searching error messages for phrases. `VALIDATION_MESSAGE_FRAGMENTS` and `ROW_WRITE_ERROR_PATTERNS` held 32 substrings between them, and fifteen more lists were inlined in routes — 83 matchers over 17 files, each its own copy of the guesswork and already drifted apart. It made message wording load-bearing: `TableRowLimitError`'s own doc comment noted that its text had to contain "row limit" for a route to answer 400, and adding "already exists" to a rename message silently demoted a 409 to a 400 (the bug fixed one commit ago, by adding another special case). Services now throw `OrchestrationError`, which carries the transport-neutral `OrchestrationErrorCode` the layers above already speak. Classification is one `instanceof` in `orchestrationErrorResponse` (UI + v1) and `v2CaughtOrchestrationError` (v2). Every pattern list is gone. Wording is free to change; an unclassified error still becomes a generic 500, which is what an unexpected fault should be. `asOrchestrationError` walks the `cause` chain rather than testing the caught value directly: drizzle wraps a throw raised inside a transaction callback in a `DrizzleQueryError` whose own message is the failed SQL, so a bare `instanceof` would drop every failure raised inside `withLockedTable`. That is the same reason `rootErrorMessage` had to dig for a root cause before. Three throws stay bare `Error` deliberately — `Table ID mismatch`, `Workspace ID mismatch`, and `Failed to build upsert conflict predicate` are internal invariants no consumer classified, and they keep falling through to a 500. `Insufficient capacity` was in the pattern list with no producer anywhere in the codebase. Status changes, all deliberate: - `'forbidden'` joins the code union so the table-row-limit ceiling keeps its 403; without it this refactor would have flattened it to 400. - import-async's table-limit rejection: 400 -> 403, matching the two other create routes it had drifted from. - Renaming a table to an invalid name: 500 -> 400. `validateTableName` messages don't contain "Invalid", so no matcher ever caught them. - Restoring a table that isn't archived, or into an archived workspace: 500 -> 400. - A duplicate *column* name stays `validation`/400 rather than becoming a 409 like a duplicate table name. Both v1 and the orchestration have always answered 400 for it; changing a published status is not this refactor's job. The twelve tests that changed were asserting the substring mechanism itself, constructing plain `Error`s with magic strings. They now assert the real contract, plus new cases pinning that identical wording carrying no classification stays internal and keeps its message off the wire. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YGzbVDZpe2dEALbu2BUU8a --- .../api/table/[tableId]/columns/route.test.ts | 6 +- .../api/table/[tableId]/columns/run/route.ts | 12 +- .../api/table/[tableId]/import/route.test.ts | 14 +- .../app/api/table/[tableId]/import/route.ts | 43 ++---- .../api/table/[tableId]/rows/[rowId]/route.ts | 14 +- apps/sim/app/api/table/import-async/route.ts | 10 +- .../app/api/table/import-csv/route.test.ts | 21 ++- apps/sim/app/api/table/import-csv/route.ts | 23 +--- apps/sim/app/api/table/route.ts | 16 +-- apps/sim/app/api/table/utils.test.ts | 24 +++- apps/sim/app/api/table/utils.ts | 64 ++++----- .../api/v1/tables/[tableId]/columns/route.ts | 29 +---- .../v1/tables/[tableId]/rows/[rowId]/route.ts | 30 ++--- .../v1/tables/[tableId]/rows/upsert/route.ts | 23 ++-- apps/sim/app/api/v1/tables/route.ts | 16 +-- apps/sim/app/api/v2/lib/response.ts | 14 +- .../api/v2/tables/[tableId]/columns/route.ts | 21 +-- .../v2/tables/[tableId]/rows/[rowId]/route.ts | 22 +--- .../v2/tables/[tableId]/rows/upsert/route.ts | 17 +-- apps/sim/app/api/v2/tables/route.ts | 15 +-- apps/sim/lib/core/orchestration/types.ts | 53 +++++++- apps/sim/lib/table/billing.ts | 10 +- apps/sim/lib/table/columns/service.ts | 123 ++++++++++++------ apps/sim/lib/table/import-data.ts | 14 +- apps/sim/lib/table/import.ts | 19 ++- .../lib/table/orchestration/columns.test.ts | 22 +++- apps/sim/lib/table/orchestration/columns.ts | 36 +---- .../lib/table/orchestration/tables.test.ts | 46 +++---- apps/sim/lib/table/orchestration/tables.ts | 46 +++---- apps/sim/lib/table/rows/service.ts | 113 +++++++++++----- apps/sim/lib/table/service.ts | 57 +++++--- apps/sim/lib/table/workflow-columns.ts | 6 +- 32 files changed, 532 insertions(+), 447 deletions(-) diff --git a/apps/sim/app/api/table/[tableId]/columns/route.test.ts b/apps/sim/app/api/table/[tableId]/columns/route.test.ts index eb9a0faaf01..e8497a0fa04 100644 --- a/apps/sim/app/api/table/[tableId]/columns/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/columns/route.test.ts @@ -57,6 +57,7 @@ vi.mock('@/app/api/table/utils', () => ({ tableLockErrorResponse: () => null, })) +import { OrchestrationError } from '@/lib/core/orchestration/types' import { PATCH } from '@/app/api/table/[tableId]/columns/route' const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111' @@ -166,7 +167,10 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => { // Stands in for the race the guards cannot close: the column stopped being // a currency between the snapshot the guards read and this write. mockUpdateColumnCurrency.mockRejectedValue( - new Error('Cannot set currency on column "amount" of type "string"') + new OrchestrationError( + 'validation', + 'Cannot set currency on column "amount" of type "string"' + ) ) const response = await patch({ name: 'renamed', currencyCode: 'USD' }) diff --git a/apps/sim/app/api/table/[tableId]/columns/run/route.ts b/apps/sim/app/api/table/[tableId]/columns/run/route.ts index 824ce73ddb4..7a047120f75 100644 --- a/apps/sim/app/api/table/[tableId]/columns/run/route.ts +++ b/apps/sim/app/api/table/[tableId]/columns/run/route.ts @@ -8,7 +8,12 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { TableQueryValidationError } from '@/lib/table/errors' import { toLegacyFilter } from '@/lib/table/query-builder/converters' import { runWorkflowColumn } from '@/lib/table/workflow-columns' -import { accessError, checkAccess, tableFilterError } from '@/app/api/table/utils' +import { + accessError, + checkAccess, + orchestrationErrorResponse, + tableFilterError, +} from '@/app/api/table/utils' const logger = createLogger('TableRunColumnAPI') @@ -66,9 +71,8 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro if (error instanceof TableQueryValidationError) { return NextResponse.json({ error: error.message }, { status: 400 }) } - if (error instanceof Error && error.message === 'Invalid workspace ID') { - return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 }) - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`run-column failed:`, error) return NextResponse.json({ error: 'Failed to run columns' }, { status: 500 }) } diff --git a/apps/sim/app/api/table/[tableId]/import/route.test.ts b/apps/sim/app/api/table/[tableId]/import/route.test.ts index baf8c313a4f..a2689295725 100644 --- a/apps/sim/app/api/table/[tableId]/import/route.test.ts +++ b/apps/sim/app/api/table/[tableId]/import/route.test.ts @@ -79,6 +79,7 @@ vi.mock('@/lib/table/billing', () => ({ limit >= 0 && current + added > limit, })) +import { OrchestrationError } from '@/lib/core/orchestration/types' import { TableLockedError } from '@/lib/table/mutation-locks' import { POST } from '@/app/api/table/[tableId]/import/route' @@ -372,7 +373,10 @@ describe('POST /api/table/[tableId]/import', () => { it('surfaces unique violations from importAppendRows as 400', async () => { mockImportAppendRows.mockRejectedValueOnce( - new Error('Row 1: Column "name" must be unique. Value "Alice" already exists in row row_xxx') + new OrchestrationError( + 'validation', + 'Row 1: Column "name" must be unique. Value "Alice" already exists in row row_xxx' + ) ) const response = await callPost( createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' }) @@ -516,7 +520,9 @@ describe('POST /api/table/[tableId]/import', () => { }) it('surfaces column-creation failures from importAppendRows as 400', async () => { - mockImportAppendRows.mockRejectedValueOnce(new Error('Column "email" already exists')) + mockImportAppendRows.mockRejectedValueOnce( + new OrchestrationError('validation', 'Column "email" already exists') + ) const response = await callPost( createFormData(createCsvFile('name,age,email\nAlice,30,a@x.io'), { mode: 'append', @@ -529,7 +535,9 @@ describe('POST /api/table/[tableId]/import', () => { }) it('surfaces row insert failures without success when schema was mutated', async () => { - mockImportAppendRows.mockRejectedValueOnce(new Error('must be unique')) + mockImportAppendRows.mockRejectedValueOnce( + new OrchestrationError('validation', 'must be unique') + ) const response = await callPost( createFormData(createCsvFile('name,age,email\nAlice,30,a@x.io'), { mode: 'append', diff --git a/apps/sim/app/api/table/[tableId]/import/route.ts b/apps/sim/app/api/table/[tableId]/import/route.ts index 7a9802442cc..0b6cf78a319 100644 --- a/apps/sim/app/api/table/[tableId]/import/route.ts +++ b/apps/sim/app/api/table/[tableId]/import/route.ts @@ -14,6 +14,7 @@ import { import { ianaTimezoneSchema } from '@/lib/api/contracts/user' import { getValidationErrorMessage } from '@/lib/api/server' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' +import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import { isMultipartError, readMultipart } from '@/lib/core/utils/multipart' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' @@ -349,21 +350,13 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro createdColumns: additions.length, error: message, }) - const isClientError = - message.includes('row limit') || - message.includes('Insufficient capacity') || - message.includes('Schema validation') || - message.includes('must be unique') || - message.includes('Row size exceeds') || - message.includes('already exists') || - message.includes('Invalid column name') || - /^Row \d+:/.test(message) + const classified = asOrchestrationError(err) return NextResponse.json( { - error: isClientError ? message : 'Failed to import CSV', + error: classified ? classified.message : 'Failed to import CSV', data: { insertedCount: 0 }, }, - { status: isClientError ? 400 : 500 } + { status: classified ? statusForOrchestrationError(classified.code) : 500 } ) } } @@ -400,17 +393,12 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro }, }) } catch (err) { - const message = toError(err).message - const isClientError = - message.includes('row limit') || - message.includes('Schema validation') || - message.includes('must be unique') || - message.includes('Row size exceeds') || - message.includes('already exists') || - message.includes('Invalid column name') || - /^Row \d+:/.test(message) - if (isClientError) { - return NextResponse.json({ error: message }, { status: 400 }) + const classified = asOrchestrationError(err) + if (classified) { + return NextResponse.json( + { error: classified.message }, + { status: statusForOrchestrationError(classified.code) } + ) } throw err } @@ -419,17 +407,12 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro if (lockError) return lockError if (isMultipartError(error)) return multipartErrorResponse(error) - const message = toError(error).message logger.error(`[${requestId}] CSV import into existing table failed:`, error) - const isClientError = - message.includes('CSV file has no') || - message.includes('already exists') || - message.includes('Invalid column name') - + const classified = asOrchestrationError(error) return NextResponse.json( - { error: isClientError ? message : 'Failed to import CSV' }, - { status: isClientError ? 400 : 500 } + { error: classified ? classified.message : 'Failed to import CSV' }, + { status: classified ? statusForOrchestrationError(classified.code) : 500 } ) } finally { fileStream?.destroy() diff --git a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts index da7852c4569..6f4636d9aa1 100644 --- a/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts @@ -1,7 +1,6 @@ import { db } from '@sim/db' import { userTableRows } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' import { and, eq } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { @@ -21,7 +20,7 @@ import { rowWireTranslators } from '@/app/api/table/row-wire' import { accessError, checkAccess, - rootErrorMessage, + orchestrationErrorResponse, rowWriteErrorResponse, tableLockErrorResponse, } from '@/app/api/table/utils' @@ -175,10 +174,6 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: RowR }, }) } catch (error) { - if (rootErrorMessage(error) === 'Row not found') { - return NextResponse.json({ error: 'Row not found' }, { status: 404 }) - } - const response = rowWriteErrorResponse(error) if (response) return response @@ -233,11 +228,8 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row const lockError = tableLockErrorResponse(error) if (lockError) return lockError - const errorMessage = toError(error).message - - if (errorMessage === 'Row not found') { - return NextResponse.json({ error: errorMessage }, { status: 404 }) - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error deleting row:`, error) return NextResponse.json({ error: 'Failed to delete row' }, { status: 500 }) diff --git a/apps/sim/app/api/table/import-async/route.ts b/apps/sim/app/api/table/import-async/route.ts index 57d879c1f32..04039178db7 100644 --- a/apps/sim/app/api/table/import-async/route.ts +++ b/apps/sim/app/api/table/import-async/route.ts @@ -18,11 +18,11 @@ import { releaseJobClaim, sanitizeName, TABLE_LIMITS, - TableConflictError, } from '@/lib/table' import { runTableImport, type TableImportPayload } from '@/lib/table/import-runner' import { getUserSettings } from '@/lib/users/queries' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' +import { orchestrationErrorResponse } from '@/app/api/table/utils' const logger = createLogger('TableImportAsync') @@ -101,12 +101,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => { requestId ) } catch (error) { - if (error instanceof TableConflictError) { - return NextResponse.json({ error: error.message }, { status: 409 }) - } - if (error instanceof Error && error.message.includes('maximum table limit')) { - return NextResponse.json({ error: error.message }, { status: 400 }) - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified throw error } diff --git a/apps/sim/app/api/table/import-csv/route.test.ts b/apps/sim/app/api/table/import-csv/route.test.ts index b85e1ccb01b..a9722924755 100644 --- a/apps/sim/app/api/table/import-csv/route.test.ts +++ b/apps/sim/app/api/table/import-csv/route.test.ts @@ -2,7 +2,6 @@ * @vitest-environment node */ import { hybridAuthMockFns, permissionsMock, permissionsMockFns } from '@sim/testing' -import { getErrorMessage } from '@sim/utils/errors' import type { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -32,6 +31,9 @@ vi.mock('@/lib/table/rows/service', () => ({ vi.mock('@/lib/table/billing', () => ({ getWorkspaceTableLimits: mockGetLimits })) vi.mock('@/app/api/table/utils', async () => { const { NextResponse } = await import('next/server') + const { asOrchestrationError, statusForOrchestrationError } = await import( + '@/lib/core/orchestration/types' + ) return { normalizeColumn: (column: unknown) => column, csvProxyBodyCapResponse: () => null, @@ -40,16 +42,20 @@ vi.mock('@/app/api/table/utils', async () => { { error: error.message }, { status: error.code === 'FILE_TOO_LARGE' ? 413 : 400 } ), - rowWriteErrorResponse: (error: unknown) => { - const message = getErrorMessage(error) - return message.includes('row limit') - ? NextResponse.json({ error: message }, { status: 400 }) + orchestrationErrorResponse: (error: unknown) => { + const classified = asOrchestrationError(error) + return classified + ? NextResponse.json( + { error: classified.message }, + { status: statusForOrchestrationError(classified.code) } + ) : null }, } }) vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) +import { OrchestrationError } from '@/lib/core/orchestration/types' import { POST } from '@/app/api/table/import-csv/route' type Part = @@ -184,7 +190,10 @@ describe('POST /api/table/import-csv', () => { it('returns 400 with the reason when an insert exceeds the plan row limit', async () => { mockBatchInsertRows.mockRejectedValueOnce( - new Error('This table has reached its row limit (1,000 rows) on your current plan.') + new OrchestrationError( + 'validation', + 'This table has reached its row limit (1,000 rows) on your current plan.' + ) ) const response = await POST(makeRequest(uploadParts(csvWithRows(250)))) const data = await response.json() diff --git a/apps/sim/app/api/table/import-csv/route.ts b/apps/sim/app/api/table/import-csv/route.ts index 9ca0381fe90..f84f457e820 100644 --- a/apps/sim/app/api/table/import-csv/route.ts +++ b/apps/sim/app/api/table/import-csv/route.ts @@ -1,6 +1,5 @@ import type { Readable } from 'node:stream' import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { type NextRequest, NextResponse } from 'next/server' import { csvExtensionSchema, csvImportFormSchema } from '@/lib/api/contracts/tables' @@ -34,7 +33,7 @@ import { csvProxyBodyCapResponse, multipartErrorResponse, normalizeColumn, - rowWriteErrorResponse, + orchestrationErrorResponse, } from '@/app/api/table/utils' const logger = createLogger('TableImportCSV') @@ -250,22 +249,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => { logger.error(`[${requestId}] CSV import failed:`, error) - // Row-write failures (e.g. the plan row-limit check) map to a 400 with the real reason. - const rowWriteError = rowWriteErrorResponse(error) - if (rowWriteError) return rowWriteError + // Every caller-fixable failure on this path — the plan row-limit check, the + // schema and CSV-shape validation, a name collision — arrives classified. + const classified = orchestrationErrorResponse(error) + if (classified) return classified - const message = toError(error).message - const isClientError = - message.includes('maximum table limit') || - message.includes('CSV file has no') || - message.includes('Invalid table name') || - message.includes('Invalid schema') || - message.includes('already exists') - - return NextResponse.json( - { error: isClientError ? message : 'Failed to import CSV' }, - { status: isClientError ? 400 : 500 } - ) + return NextResponse.json({ error: 'Failed to import CSV' }, { status: 500 }) } finally { fileStream?.destroy() } diff --git a/apps/sim/app/api/table/route.ts b/apps/sim/app/api/table/route.ts index 2522cddb7c6..28714885cb5 100644 --- a/apps/sim/app/api/table/route.ts +++ b/apps/sim/app/api/table/route.ts @@ -16,7 +16,7 @@ import { type TableScope, } from '@/lib/table' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' -import { normalizeColumn } from '@/app/api/table/utils' +import { normalizeColumn, orchestrationErrorResponse } from '@/app/api/table/utils' const logger = createLogger('TableAPI') @@ -153,18 +153,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => { }, }) } catch (error) { - if (error instanceof Error) { - if (error.message.includes('maximum table limit')) { - return NextResponse.json({ error: error.message }, { status: 403 }) - } - if ( - error.message.includes('Invalid table name') || - error.message.includes('Invalid schema') || - error.message.includes('already exists') - ) { - return NextResponse.json({ error: error.message }, { status: 400 }) - } - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error creating table:`, error) return NextResponse.json({ error: 'Failed to create table' }, { status: 500 }) diff --git a/apps/sim/app/api/table/utils.test.ts b/apps/sim/app/api/table/utils.test.ts index 99d0ce0c5a5..fa7b57ac6dd 100644 --- a/apps/sim/app/api/table/utils.test.ts +++ b/apps/sim/app/api/table/utils.test.ts @@ -2,6 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { TableRowLimitError } from '@/lib/table/billing' import type { ColumnDefinition } from '@/lib/table/types' import { rootErrorMessage, rowWriteErrorResponse, tableFilterError } from '@/app/api/table/utils' @@ -38,15 +39,30 @@ describe('rowWriteErrorResponse', () => { ) }) - it('passes known validation messages through as 400', async () => { - const response = rowWriteErrorResponse(new Error('Value for column "email" must be unique')) + it('passes a classified validation failure through as 400', async () => { + const response = rowWriteErrorResponse( + new OrchestrationError('validation', 'Value for column "email" must be unique') + ) expect(response?.status).toBe(400) const body = await response?.json() expect(body.error).toBe('Value for column "email" must be unique') }) - it('matches per-row batch validation messages', () => { - expect(rowWriteErrorResponse(new Error('Row 3: name is required'))?.status).toBe(400) + it('answers the code the failure carries, not one derived from its wording', () => { + expect( + rowWriteErrorResponse(new OrchestrationError('not_found', 'Row not found'))?.status + ).toBe(404) + // The phrase that used to force a 400 no longer decides anything. + expect( + rowWriteErrorResponse(new OrchestrationError('conflict', 'Row 3: must be unique'))?.status + ).toBe(409) + }) + + it('unwraps a classified failure drizzle wrapped in a query error', () => { + expect( + rowWriteErrorResponse(wrapLikeDrizzle(new OrchestrationError('validation', 'Row 3: bad'))) + ?.status + ).toBe(400) }) it('returns null for unknown errors so callers keep their generic 500', () => { diff --git a/apps/sim/app/api/table/utils.ts b/apps/sim/app/api/table/utils.ts index ceb399556c4..805d2b16206 100644 --- a/apps/sim/app/api/table/utils.ts +++ b/apps/sim/app/api/table/utils.ts @@ -8,6 +8,7 @@ import { updateTableColumnBodySchema, } from '@/lib/api/contracts/tables' import { isFeatureEnabled } from '@/lib/core/config/feature-flags' +import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import type { MultipartError } from '@/lib/core/utils/multipart' import type { ColumnDefinition, Filter, TableDefinition, TablePredicate } from '@/lib/table' import { buildFilterClause, getTableById, TableQueryValidationError } from '@/lib/table' @@ -42,8 +43,9 @@ export async function tablesV2GateError( * Maps a {@link TableLockedError} thrown by the service layer to a 423 response * carrying `{ error, lock }`; returns `null` for any other error so the caller * falls through to its existing handling. Call this as the FIRST statement of a - * table route's catch block — otherwise `rowWriteErrorResponse` (and the other - * substring funnels) turn the lock error into a generic 500. + * table route's catch block — `TableLockedError` is an `HttpError`, not an + * `OrchestrationError`, so nothing else classifies it and it would otherwise + * reach the route's generic 500. * * The body deliberately omits a `details` array: the client's `isValidationError` * treats any `ApiClientError` with array-valued `details` as a field-validation @@ -106,48 +108,36 @@ export function rootErrorMessage(error: unknown): string { } /** - * Known user-facing row-write failures (service validation + the best-effort - * plan row-limit check). Anything outside this list stays a generic 500 — - * unknown errors can carry SQL/internals that don't belong in a toast. - */ -const ROW_WRITE_ERROR_PATTERNS = [ - 'row limit', - 'Insufficient capacity', - 'Schema validation', - 'must be unique', - 'must be valid', - 'must be string', - 'must be number', - 'must be boolean', - 'unique column', - 'Unique constraint violation', - 'Row size exceeds', - 'conflictTarget', - 'Upsert requires', - 'Rows not found', - 'Filter is required', -] as const - -/** - * Maps a known user-facing row-write failure to a 400 carrying the real message - * (so client toasts can show the actual reason); `null` when the error is - * unrecognized and the caller should log it and return its generic 500. + * Maps a classified domain failure to its status, carrying the real message so + * client toasts can show the actual reason; `null` when the error carries no + * classification and the caller should log it and return its own generic 500 — + * an unrecognized error can hold SQL/internals that don't belong in a toast. + * + * This is the whole classification story for the UI and v1 table routes. It + * replaced per-route lists of message substrings, which decided a status by + * searching prose and so silently changed one whenever a message was reworded. */ -export function rowWriteErrorResponse(error: unknown): NextResponse | null { - // A lock violation is a 423, not a 400/500 — check before the pattern match, - // which would otherwise let it fall through to the caller's generic 500. +export function orchestrationErrorResponse(error: unknown): NextResponse | null { + // A lock violation is a 423, and `TableLockedError` is an `HttpError` rather + // than an `OrchestrationError`, so it needs its own check first. const lockResponse = tableLockErrorResponse(error) if (lockResponse) return lockResponse - const message = rootErrorMessage(error) - - if (ROW_WRITE_ERROR_PATTERNS.some((p) => message.includes(p)) || /^Row .+?:/.test(message)) { - return NextResponse.json({ error: message }, { status: 400 }) - } + const classified = asOrchestrationError(error) + if (!classified) return null - return null + return NextResponse.json( + { error: classified.message }, + { status: statusForOrchestrationError(classified.code) } + ) } +/** + * {@link orchestrationErrorResponse} under the name the row-write routes call + * it by. Row writes have no classification rules of their own any more. + */ +export const rowWriteErrorResponse = orchestrationErrorResponse + /** * Next.js buffers the request body for the proxy and silently truncates it past this * size (`experimental.proxyClientMaxBodySize`, default 10MB). The synchronous CSV diff --git a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts index 52296284215..78dca2d367f 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts @@ -16,6 +16,7 @@ import { accessError, checkAccess, normalizeColumn, + orchestrationErrorResponse, tableLockErrorResponse, } from '@/app/api/table/utils' import { @@ -92,22 +93,8 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Colum const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - if (error instanceof Error) { - // Same caller-error set the internal columns route maps — an invalid - // select option set is a bad request, not a server fault. - if ( - error.message.includes('already exists') || - error.message.includes('maximum column') || - error.message.includes('Invalid column') || - error.message.includes('exceeds maximum') || - error.message.includes('option') - ) { - return NextResponse.json({ error: error.message }, { status: 400 }) - } - if (error.message === 'Table not found') { - return NextResponse.json({ error: error.message }, { status: 404 }) - } - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error adding column to table:`, error) return NextResponse.json({ error: 'Failed to add column' }, { status: 500 }) @@ -236,14 +223,8 @@ export const DELETE = withRouteHandler( const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - if (error instanceof Error) { - if (error.message.includes('not found') || error.message === 'Table not found') { - return NextResponse.json({ error: error.message }, { status: 404 }) - } - if (error.message.includes('Cannot delete') || error.message.includes('last column')) { - return NextResponse.json({ error: error.message }, { status: 400 }) - } - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error deleting column from table:`, error) return NextResponse.json({ error: 'Failed to delete column' }, { status: 500 }) diff --git a/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts index 1d237b7be0f..dbe768ee830 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/rows/[rowId]/route.ts @@ -1,7 +1,6 @@ import { db } from '@sim/db' import { userTableRows } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' import { and, eq } from 'drizzle-orm' import { type NextRequest, NextResponse } from 'next/server' import { @@ -18,7 +17,12 @@ import { updateRow } from '@/lib/table' import { namedRowMapper } from '@/lib/table/cell-format' import { buildIdByName, rowDataNameToId } from '@/lib/table/column-keys' import { performDeleteTableRow } from '@/lib/table/orchestration' -import { accessError, checkAccess, tableLockErrorResponse } from '@/app/api/table/utils' +import { + accessError, + checkAccess, + orchestrationErrorResponse, + tableLockErrorResponse, +} from '@/app/api/table/utils' import { checkRateLimit, checkWorkspaceScope, @@ -190,21 +194,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: RowR const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - const errorMessage = toError(error).message - - if (errorMessage === 'Row not found') { - return NextResponse.json({ error: errorMessage }, { status: 404 }) - } - - if ( - errorMessage.includes('Row size exceeds') || - errorMessage.includes('Schema validation') || - errorMessage.includes('must be unique') || - errorMessage.includes('Unique constraint violation') || - errorMessage.includes('Cannot set unique column') - ) { - return NextResponse.json({ error: errorMessage }, { status: 400 }) - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error updating row:`, error) return NextResponse.json({ error: 'Failed to update row' }, { status: 500 }) @@ -258,9 +249,8 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row } catch (error) { const lockError = tableLockErrorResponse(error) if (lockError) return lockError - if (error instanceof Error && error.message === 'Row not found') { - return NextResponse.json({ error: 'Row not found' }, { status: 404 }) - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error deleting row:`, error) return NextResponse.json({ error: 'Failed to delete row' }, { status: 500 }) } diff --git a/apps/sim/app/api/v1/tables/[tableId]/rows/upsert/route.ts b/apps/sim/app/api/v1/tables/[tableId]/rows/upsert/route.ts index bf4a00df91b..a32f17a9c8e 100644 --- a/apps/sim/app/api/v1/tables/[tableId]/rows/upsert/route.ts +++ b/apps/sim/app/api/v1/tables/[tableId]/rows/upsert/route.ts @@ -1,5 +1,4 @@ import { createLogger } from '@sim/logger' -import { toError } from '@sim/utils/errors' import { type NextRequest, NextResponse } from 'next/server' import { v1UpsertTableRowContract } from '@/lib/api/contracts/v1/tables' import { parseRequest } from '@/lib/api/server' @@ -9,7 +8,12 @@ import type { RowData, TableSchema } from '@/lib/table' import { upsertRow } from '@/lib/table' import { namedRowMapper } from '@/lib/table/cell-format' import { buildIdByName, rowDataNameToId } from '@/lib/table/column-keys' -import { accessError, checkAccess, tableLockErrorResponse } from '@/app/api/table/utils' +import { + accessError, + checkAccess, + orchestrationErrorResponse, + tableLockErrorResponse, +} from '@/app/api/table/utils' import { checkRateLimit, checkWorkspaceScope, @@ -101,19 +105,8 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Upser const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - const errorMessage = toError(error).message - - if ( - errorMessage.includes('unique column') || - errorMessage.includes('Unique constraint violation') || - errorMessage.includes('conflictTarget') || - errorMessage.includes('row limit') || - errorMessage.includes('Schema validation') || - errorMessage.includes('Upsert requires') || - errorMessage.includes('Row size exceeds') - ) { - return NextResponse.json({ error: errorMessage }, { status: 400 }) - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error upserting row:`, error) return NextResponse.json({ error: 'Failed to upsert row' }, { status: 500 }) diff --git a/apps/sim/app/api/v1/tables/route.ts b/apps/sim/app/api/v1/tables/route.ts index 82bc6618247..6213fd59053 100644 --- a/apps/sim/app/api/v1/tables/route.ts +++ b/apps/sim/app/api/v1/tables/route.ts @@ -6,7 +6,7 @@ import { parseRequest } from '@/lib/api/server' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { createTable, getWorkspaceTableLimits, listTables, type TableSchema } from '@/lib/table' -import { normalizeColumn } from '@/app/api/table/utils' +import { normalizeColumn, orchestrationErrorResponse } from '@/app/api/table/utils' import { checkRateLimit, createRateLimitResponse, @@ -171,18 +171,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => { const validationResponse = v1ValidationErrorResponseFromError(error) if (validationResponse) return validationResponse - if (error instanceof Error) { - if (error.message.includes('maximum table limit')) { - return NextResponse.json({ error: error.message }, { status: 403 }) - } - if ( - error.message.includes('Invalid table name') || - error.message.includes('Invalid schema') || - error.message.includes('already exists') - ) { - return NextResponse.json({ error: error.message }, { status: 400 }) - } - } + const classified = orchestrationErrorResponse(error) + if (classified) return classified logger.error(`[${requestId}] Error creating table:`, error) return NextResponse.json({ error: 'Failed to create table' }, { status: 500 }) diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index 7ae333cc5bd..45ed1e6fcb3 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -1,7 +1,7 @@ import { NextResponse } from 'next/server' import type { ZodError } from 'zod' import { getValidationErrorMessage, serializeZodIssues } from '@/lib/api/server' -import type { OrchestrationErrorCode } from '@/lib/core/orchestration/types' +import { asOrchestrationError, type OrchestrationErrorCode } from '@/lib/core/orchestration/types' import type { RateLimitResult, WorkspaceAccessError } from '@/app/api/v1/middleware' /** @@ -159,6 +159,7 @@ export function decodeCursor>(cursor: string): T | n const V2_CODE_BY_ORCHESTRATION_ERROR: Record = { validation: 'BAD_REQUEST', + forbidden: 'FORBIDDEN', not_found: 'NOT_FOUND', conflict: 'CONFLICT', locked: 'LOCKED', @@ -177,3 +178,14 @@ export function v2ErrorForOrchestration( const v2Code = code ? V2_CODE_BY_ORCHESTRATION_ERROR[code] : 'INTERNAL_ERROR' return v2Error(v2Code, v2Code === 'INTERNAL_ERROR' ? 'Internal server error' : message) } + +/** + * Renders a thrown domain failure in the v2 envelope, or `null` when the error + * carries no classification and the caller should log it and return its own + * generic 500. The v2 counterpart of `orchestrationErrorResponse`. + */ +export function v2CaughtOrchestrationError(error: unknown): NextResponse | null { + const classified = asOrchestrationError(error) + if (!classified) return null + return v2ErrorForOrchestration(classified.code, classified.message) +} diff --git a/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts index c5833176b15..ce480142cab 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/columns/route.ts @@ -16,6 +16,7 @@ import { checkAccess, normalizeColumn } from '@/app/api/table/utils' import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware' import { v2ApiGateError } from '@/app/api/v2/lib/gate' import { + v2CaughtOrchestrationError, v2Data, v2Error, v2ErrorForOrchestration, @@ -84,14 +85,8 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Colum } catch (error) { if (isZodError(error)) return v2ValidationError(error) - if (error instanceof Error) { - if (error.message.includes('already exists') || error.message.includes('maximum column')) { - return v2Error('BAD_REQUEST', error.message) - } - if (error.message === 'Table not found') { - return v2Error('NOT_FOUND', error.message) - } - } + const classified = v2CaughtOrchestrationError(error) + if (classified) return classified logger.error(`[${requestId}] Error adding column to table`, { error: getErrorMessage(error, 'Unknown error'), @@ -208,14 +203,8 @@ export const DELETE = withRouteHandler( } catch (error) { if (isZodError(error)) return v2ValidationError(error) - if (error instanceof Error) { - if (error.message.includes('not found') || error.message === 'Table not found') { - return v2Error('NOT_FOUND', error.message) - } - if (error.message.includes('Cannot delete') || error.message.includes('last column')) { - return v2Error('BAD_REQUEST', error.message) - } - } + const classified = v2CaughtOrchestrationError(error) + if (classified) return classified logger.error(`[${requestId}] Error deleting column from table`, { error: getErrorMessage(error, 'Unknown error'), diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts index 7fe976ec553..b0bb10b78d3 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/[rowId]/route.ts @@ -1,7 +1,7 @@ import { db } from '@sim/db' import { userTableRows } from '@sim/db/schema' import { createLogger } from '@sim/logger' -import { getErrorMessage, toError } from '@sim/utils/errors' +import { getErrorMessage } from '@sim/utils/errors' import { and, eq } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { @@ -20,6 +20,7 @@ import { checkAccess } from '@/app/api/table/utils' import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware' import { v2ApiGateError } from '@/app/api/v2/lib/gate' import { + v2CaughtOrchestrationError, v2Data, v2Error, v2ErrorForOrchestration, @@ -165,18 +166,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: RowR } catch (error) { if (isZodError(error)) return v2ValidationError(error) - const errorMessage = toError(error).message - if (errorMessage === 'Row not found') return v2Error('NOT_FOUND', errorMessage) - - if ( - errorMessage.includes('Row size exceeds') || - errorMessage.includes('Schema validation') || - errorMessage.includes('must be unique') || - errorMessage.includes('Unique constraint violation') || - errorMessage.includes('Cannot set unique column') - ) { - return v2Error('BAD_REQUEST', errorMessage) - } + const classified = v2CaughtOrchestrationError(error) + if (classified) return classified logger.error(`[${requestId}] Error updating row`, { error: getErrorMessage(error, 'Unknown error'), @@ -226,9 +217,8 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row } catch (error) { const lockError = v2TableLockError(error) if (lockError) return lockError - if (error instanceof Error && error.message === 'Row not found') { - return v2Error('NOT_FOUND', 'Row not found') - } + const classified = v2CaughtOrchestrationError(error) + if (classified) return classified logger.error(`[${requestId}] Error deleting row`, { error: getErrorMessage(error, 'Unknown error'), }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts index 08f4b0873af..a8b4c21593b 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/upsert/route.ts @@ -1,5 +1,5 @@ import { createLogger } from '@sim/logger' -import { getErrorMessage, toError } from '@sim/utils/errors' +import { getErrorMessage } from '@sim/utils/errors' import type { NextRequest } from 'next/server' import { v2UpsertTableRowContract } from '@/lib/api/contracts/v2/tables' import { isZodError, parseRequest } from '@/lib/api/server' @@ -12,6 +12,7 @@ import { checkAccess } from '@/app/api/table/utils' import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware' import { v2ApiGateError } from '@/app/api/v2/lib/gate' import { + v2CaughtOrchestrationError, v2Data, v2Error, v2RateLimitError, @@ -82,18 +83,8 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Upser } catch (error) { if (isZodError(error)) return v2ValidationError(error) - const errorMessage = toError(error).message - if ( - errorMessage.includes('unique column') || - errorMessage.includes('Unique constraint violation') || - errorMessage.includes('conflictTarget') || - errorMessage.includes('row limit') || - errorMessage.includes('Schema validation') || - errorMessage.includes('Upsert requires') || - errorMessage.includes('Row size exceeds') - ) { - return v2Error('BAD_REQUEST', errorMessage) - } + const classified = v2CaughtOrchestrationError(error) + if (classified) return classified logger.error(`[${requestId}] Error upserting row`, { error: getErrorMessage(error, 'Unknown error'), diff --git a/apps/sim/app/api/v2/tables/route.ts b/apps/sim/app/api/v2/tables/route.ts index 9082e9280b9..85df923214c 100644 --- a/apps/sim/app/api/v2/tables/route.ts +++ b/apps/sim/app/api/v2/tables/route.ts @@ -11,6 +11,7 @@ import { normalizeColumn } from '@/app/api/table/utils' import { checkRateLimit, resolveWorkspaceAccess } from '@/app/api/v1/middleware' import { v2ApiGateError } from '@/app/api/v2/lib/gate' import { + v2CaughtOrchestrationError, v2CursorList, v2Data, v2Error, @@ -128,18 +129,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => { } catch (error) { if (isZodError(error)) return v2ValidationError(error) - if (error instanceof Error) { - if (error.message.includes('maximum table limit')) { - return v2Error('FORBIDDEN', error.message) - } - if ( - error.message.includes('Invalid table name') || - error.message.includes('Invalid schema') || - error.message.includes('already exists') - ) { - return v2Error('BAD_REQUEST', error.message) - } - } + const classified = v2CaughtOrchestrationError(error) + if (classified) return classified logger.error(`[${requestId}] Error creating table`, { error: getErrorMessage(error, 'Unknown error'), diff --git a/apps/sim/lib/core/orchestration/types.ts b/apps/sim/lib/core/orchestration/types.ts index 7b6c4687d44..59ccf54aad6 100644 --- a/apps/sim/lib/core/orchestration/types.ts +++ b/apps/sim/lib/core/orchestration/types.ts @@ -1,4 +1,10 @@ -export type OrchestrationErrorCode = 'validation' | 'not_found' | 'conflict' | 'locked' | 'internal' +export type OrchestrationErrorCode = + | 'validation' + | 'not_found' + | 'forbidden' + | 'conflict' + | 'locked' + | 'internal' /** * Transport-neutral failure classes returned by every `lib/[resource]/orchestration` @@ -7,12 +13,57 @@ export type OrchestrationErrorCode = 'validation' | 'not_found' | 'conflict' | ' */ export function statusForOrchestrationError(code: OrchestrationErrorCode | undefined): number { if (code === 'validation') return 400 + if (code === 'forbidden') return 403 if (code === 'not_found') return 404 if (code === 'conflict') return 409 if (code === 'locked') return 423 return 500 } +/** + * A domain failure that already knows its own class. + * + * Services throw this instead of a bare `Error` whenever the failure is + * caller-fixable, so the layers above classify by `instanceof` and read `code` + * rather than searching the message for a phrase. Message text is then free to + * be reworded, translated, or made more specific without silently changing the + * status every caller returns — the failure mode this replaced, where adding + * "already exists" to a message demoted a 409 to a 400. + * + * The code is transport-neutral on purpose: `statusForOrchestrationError` maps + * it for the UI and v1 routes, `v2ErrorForOrchestration` maps it to the v2 + * error vocabulary, and the copilot tools surface `message` with no status at + * all. An anything-else error stays unclassified and becomes a generic 500, + * which is what an unexpected fault should be. + */ +export class OrchestrationError extends Error { + constructor( + readonly code: OrchestrationErrorCode, + message: string + ) { + super(message) + this.name = 'OrchestrationError' + } +} + +/** + * The {@link OrchestrationError} in `error`'s cause chain, or `null` when the + * failure is not a classified one. + * + * Walks `cause` rather than testing `error` alone because drizzle wraps a throw + * raised inside a transaction callback in a `DrizzleQueryError` whose own + * message is the failed SQL — the same reason the message-matching this + * replaced had to dig for a root cause before it could classify anything. + */ +export function asOrchestrationError(error: unknown): OrchestrationError | null { + let current: unknown = error + while (current instanceof Error) { + if (current instanceof OrchestrationError) return current + current = current.cause + } + return null +} + /** * The slice of an HTTP request the audit log reads for client IP and user-agent * capture. Optional on every orchestration function so the non-HTTP callers — diff --git a/apps/sim/lib/table/billing.ts b/apps/sim/lib/table/billing.ts index af4aa9ee168..824b102d17b 100644 --- a/apps/sim/lib/table/billing.ts +++ b/apps/sim/lib/table/billing.ts @@ -9,6 +9,7 @@ import { resolveWorkspaceBillingPayer } from '@/lib/billing/core/billing-attribu import { maybeNotifyLimit } from '@/lib/billing/core/limit-notifications' import { getPlanTypeForLimits } from '@/lib/billing/plan-helpers' import { isBillingEnabled } from '@/lib/core/config/env-flags' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { getBillingDisabledTableLimits, getTablePlanLimits, @@ -183,12 +184,15 @@ function cacheLimits(workspaceId: string, limits: TablePlanLimits): void { /** * Thrown by {@link assertRowCapacity} when a write would exceed the workspace's - * current plan row limit. The message includes the lowercase `row limit` token so - * `rowWriteErrorResponse` maps it to a 400 toast carrying the real reason. + * current plan row limit. Typed as a `validation` failure so the routes answer + * 400 with the real reason — the message used to have to carry a lowercase + * `row limit` token for a substring match to find it, which made the wording + * load-bearing. */ -export class TableRowLimitError extends Error { +export class TableRowLimitError extends OrchestrationError { constructor(readonly limit: number) { super( + 'validation', `This table has reached its row limit (${limit.toLocaleString('en-US')} rows) on your current plan.` ) this.name = 'TableRowLimitError' diff --git a/apps/sim/lib/table/columns/service.ts b/apps/sim/lib/table/columns/service.ts index 2ed1dd3cbda..327c87d264d 100644 --- a/apps/sim/lib/table/columns/service.ts +++ b/apps/sim/lib/table/columns/service.ts @@ -7,6 +7,12 @@ * * Use this for: workflow executor, background jobs, testing business logic. * Use API routes for: HTTP requests, frontend clients. + * + * Caller-fixable failures throw {@link OrchestrationError} carrying the class + * the layers above map to a status, so no caller has to search the message for + * a phrase. A duplicate column name is deliberately `validation` rather than + * `conflict` — both the v1 route and the orchestration have always answered 400 + * for it, and this refactor is not the place to change a published status. */ import { db } from '@sim/db' @@ -14,6 +20,7 @@ import { userTableDefinitions, userTableRows } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { omit } from '@sim/utils/object' import { and, count, eq, sql } from 'drizzle-orm' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { columnMatchesRef, generateColumnId, getColumnId } from '@/lib/table/column-keys' import { columnTypeById, @@ -81,30 +88,34 @@ export async function addTableColumn( return withLockedTable(tableId, async (table, trx) => { assertSchemaMutable(table) if (!NAME_PATTERN.test(column.name)) { - throw new Error( + throw new OrchestrationError( + 'validation', `Invalid column name "${column.name}". Must start with a letter or underscore and contain only alphanumeric characters and underscores.` ) } if (column.name.length > TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH) { - throw new Error( + throw new OrchestrationError( + 'validation', `Column name exceeds maximum length (${TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH} characters)` ) } if (!COLUMN_TYPES.includes(column.type as (typeof COLUMN_TYPES)[number])) { - throw new Error( + throw new OrchestrationError( + 'validation', `Invalid column type "${column.type}". Must be one of: ${COLUMN_TYPES.join(', ')}` ) } const schema = table.schema if (schema.columns.some((c) => c.name.toLowerCase() === column.name.toLowerCase())) { - throw new Error(`Column "${column.name}" already exists`) + throw new OrchestrationError('validation', `Column "${column.name}" already exists`) } if (schema.columns.length >= TABLE_LIMITS.MAX_COLUMNS_PER_TABLE) { - throw new Error( + throw new OrchestrationError( + 'validation', `Table has reached maximum column limit (${TABLE_LIMITS.MAX_COLUMNS_PER_TABLE})` ) } @@ -124,7 +135,10 @@ export async function addTableColumn( const columnValidation = validateColumnDefinition(newColumn) if (!columnValidation.valid) { - throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`) + throw new OrchestrationError( + 'validation', + `Invalid column: ${columnValidation.errors.join('; ')}` + ) } const newColumnId = getColumnId(newColumn) @@ -194,13 +208,15 @@ export async function renameColumn( return withLockedTable(data.tableId, async (table, trx) => { assertSchemaMutable(table) if (!NAME_PATTERN.test(data.newName)) { - throw new Error( + throw new OrchestrationError( + 'validation', `Invalid column name "${data.newName}". Column names must start with a letter or underscore, followed by alphanumeric characters or underscores.` ) } if (data.newName.length > TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH) { - throw new Error( + throw new OrchestrationError( + 'validation', `Column name exceeds maximum length (${TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH} characters)` ) } @@ -208,7 +224,7 @@ export async function renameColumn( const schema = table.schema const columnIndex = schema.columns.findIndex((c) => columnMatchesRef(c, data.oldName)) if (columnIndex === -1) { - throw new Error(`Column "${data.oldName}" not found`) + throw new OrchestrationError('not_found', `Column "${data.oldName}" not found`) } if ( @@ -216,7 +232,7 @@ export async function renameColumn( (c, i) => i !== columnIndex && c.name.toLowerCase() === data.newName.toLowerCase() ) ) { - throw new Error(`Column "${data.newName}" already exists`) + throw new OrchestrationError('validation', `Column "${data.newName}" already exists`) } const targetColumn = schema.columns[columnIndex] @@ -331,11 +347,11 @@ export async function deleteColumn( const schema = table.schema const columnIndex = schema.columns.findIndex((c) => columnMatchesRef(c, data.columnName)) if (columnIndex === -1) { - throw new Error(`Column "${data.columnName}" not found`) + throw new OrchestrationError('not_found', `Column "${data.columnName}" not found`) } if (schema.columns.length <= 1) { - throw new Error('Cannot delete the last column in a table') + throw new OrchestrationError('validation', 'Cannot delete the last column in a table') } const targetColumn = schema.columns[columnIndex] @@ -423,12 +439,12 @@ export async function deleteColumns( } if (notFound.length > 0) { - throw new Error(`Columns not found: ${notFound.join(', ')}`) + throw new OrchestrationError('not_found', `Columns not found: ${notFound.join(', ')}`) } const remaining = schema.columns.filter((c) => !namesToDelete.has(c.name)) if (remaining.length === 0) { - throw new Error('Cannot delete all columns from a table') + throw new OrchestrationError('validation', 'Cannot delete all columns from a table') } // For each group, drop outputs whose column (by id) is being deleted. Groups @@ -506,26 +522,32 @@ async function applyConstraints( if (data.required === undefined && data.unique === undefined) return column if (column.workflowGroupId) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot change constraints on workflow-output column "${column.name}". Constraints aren't applicable to columns whose values come from workflow execution.` ) } if (data.required === true && !column.required) { const emptyCount = await countEmptyCells(trx, tableId, columnKey) if (emptyCount > 0) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot set column "${column.name}" as required: ${emptyCount} row(s) have null, missing, or empty values` ) } } if (data.unique === true && !column.unique) { if (!columnTypeOf(column).supportsUnique) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot set column "${column.name}" as unique: ${column.type} columns compare stored values that would allow only one row per value.` ) } if (await hasDuplicateValues(trx, tableId, columnKey)) { - throw new Error(`Cannot set column "${column.name}" as unique: duplicate values exist`) + throw new OrchestrationError( + 'validation', + `Cannot set column "${column.name}" as unique: duplicate values exist` + ) } } return { @@ -592,17 +614,19 @@ export function applyPendingRename( if (newName === undefined || newName === column.name) return column if (!NAME_PATTERN.test(newName)) { - throw new Error( + throw new OrchestrationError( + 'validation', `Invalid column name "${newName}". Column names must start with a letter or underscore, followed by alphanumeric characters or underscores.` ) } if (newName.length > TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH) { - throw new Error( + throw new OrchestrationError( + 'validation', `Column name exceeds maximum length (${TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH} characters)` ) } if (columns.some((c, i) => i !== columnIndex && c.name.toLowerCase() === newName.toLowerCase())) { - throw new Error(`Column "${newName}" already exists`) + throw new OrchestrationError('validation', `Column "${newName}" already exists`) } return { ...column, name: newName } } @@ -688,7 +712,8 @@ export async function updateColumnType( await setTableTxTimeouts(trx, { statementMs: timeoutMs, idleMs: timeoutMs }) if (!(COLUMN_TYPES as readonly string[]).includes(data.newType)) { - throw new Error( + throw new OrchestrationError( + 'validation', `Invalid column type "${data.newType}". Valid types: ${COLUMN_TYPES.join(', ')}` ) } @@ -696,7 +721,7 @@ export async function updateColumnType( const schema = table.schema const columnIndex = schema.columns.findIndex((c) => columnMatchesRef(c, data.columnName)) if (columnIndex === -1) { - throw new Error(`Column "${data.columnName}" not found`) + throw new OrchestrationError('not_found', `Column "${data.columnName}" not found`) } const column = schema.columns[columnIndex] @@ -714,7 +739,8 @@ export async function updateColumnType( data.multiple !== undefined || data.currencyCode !== undefined if (carriesOtherWork) { - throw new Error( + throw new OrchestrationError( + 'validation', `Column "${column.name}" is already type "${data.newType}"; re-issue the request without a type change.` ) } @@ -760,7 +786,8 @@ export async function updateColumnType( if (targetRequired) { const emptyCount = await countEmptyCells(trx, data.tableId, columnKey) if (emptyCount > 0) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot change column "${column.name}" to a required "${data.newType}": ${emptyCount} row(s) have null, missing, or empty values. Fill them first, or apply the type change without making the column required.` ) } @@ -828,13 +855,15 @@ export async function updateColumnType( } if (blankCount > 0) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot change column "${column.name}" to a required "${data.newType}": ${blankCount} row(s) are empty. Fill them first, or apply the type change without making the column required.` ) } if (incompatibleCount > 0) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot change column "${column.name}" to type "${data.newType}": ${incompatibleCount} row(s) have incompatible values. Fix or remove the incompatible values first.` ) } @@ -846,7 +875,10 @@ export async function updateColumnType( const columnValidation = validateColumnDefinition(updatedColumns[columnIndex]) if (!columnValidation.valid) { - throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`) + throw new OrchestrationError( + 'validation', + `Invalid column: ${columnValidation.errors.join('; ')}` + ) } const updatedSchema: TableSchema = { ...schema, columns: updatedColumns } @@ -879,7 +911,8 @@ export async function updateColumnType( // irrecoverably rewritten. if (data.unique === true && !column.unique) { if (await hasDuplicateValues(trx, data.tableId, columnKey)) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot change column "${column.name}" to type "${data.newType}" and set it as unique: the converted values contain duplicates.` ) } @@ -926,7 +959,7 @@ export async function updateColumnConstraints( const schema = table.schema const columnIndex = schema.columns.findIndex((c) => columnMatchesRef(c, data.columnName)) if (columnIndex === -1) { - throw new Error(`Column "${data.columnName}" not found`) + throw new OrchestrationError('not_found', `Column "${data.columnName}" not found`) } const column = schema.columns[columnIndex] @@ -966,12 +999,15 @@ export async function updateColumnOptions( const schema = table.schema const columnIndex = schema.columns.findIndex((c) => columnMatchesRef(c, data.columnName)) if (columnIndex === -1) { - throw new Error(`Column "${data.columnName}" not found`) + throw new OrchestrationError('not_found', `Column "${data.columnName}" not found`) } const column = schema.columns[columnIndex] if (column.type !== 'select') { - throw new Error(`Cannot set options on column "${column.name}" of type "${column.type}"`) + throw new OrchestrationError( + 'validation', + `Cannot set options on column "${column.name}" of type "${column.type}"` + ) } const columnKey = getColumnId(column) @@ -984,7 +1020,10 @@ export async function updateColumnOptions( } const columnValidation = validateColumnDefinition(updatedColumn) if (!columnValidation.valid) { - throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`) + throw new OrchestrationError( + 'validation', + `Invalid column: ${columnValidation.errors.join('; ')}` + ) } const nextMultiple = !!(data.multiple ?? column.multiple) @@ -1033,7 +1072,8 @@ export async function updateColumnOptions( wasMultiple ) if (strandedCount > 0) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot remove options from required column "${column.name}": ${strandedCount} row(s) would be left empty. Reassign those rows to a remaining option first.` ) } @@ -1060,7 +1100,8 @@ export async function updateColumnOptions( } if (multiValuedCount > 0) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot switch column "${column.name}" to single-select: ${multiValuedCount} row(s) have multiple options selected. Reduce them to one option first.` ) } @@ -1132,12 +1173,15 @@ export async function updateColumnCurrency( const schema = table.schema const columnIndex = schema.columns.findIndex((c) => columnMatchesRef(c, data.columnName)) if (columnIndex === -1) { - throw new Error(`Column "${data.columnName}" not found`) + throw new OrchestrationError('not_found', `Column "${data.columnName}" not found`) } const column = schema.columns[columnIndex] if (column.type !== 'currency') { - throw new Error(`Cannot set currency on column "${column.name}" of type "${column.type}"`) + throw new OrchestrationError( + 'validation', + `Cannot set currency on column "${column.name}" of type "${column.type}"` + ) } const updatedColumn: ColumnDefinition = { @@ -1146,7 +1190,10 @@ export async function updateColumnCurrency( } const columnValidation = validateColumnDefinition(updatedColumn) if (!columnValidation.valid) { - throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`) + throw new OrchestrationError( + 'validation', + `Invalid column: ${columnValidation.errors.join('; ')}` + ) } const constrained = await applyConstraints( diff --git a/apps/sim/lib/table/import-data.ts b/apps/sim/lib/table/import-data.ts index 84664e077b2..f8e5fd8d0c6 100644 --- a/apps/sim/lib/table/import-data.ts +++ b/apps/sim/lib/table/import-data.ts @@ -9,6 +9,7 @@ import { userTableDefinitions, userTableRows } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { generateId } from '@sim/utils/id' import { eq } from 'drizzle-orm' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { assertRowCapacity, notifyTableRowUsage } from '@/lib/table/billing' import { CSV_MAX_BATCH_SIZE } from '@/lib/table/import' import { assertRowDelete, assertRowInsert, assertSchemaMutable } from '@/lib/table/mutation-locks' @@ -77,11 +78,17 @@ export async function bulkInsertImportBatch( for (let i = 0; i < data.rows.length; i++) { const sizeValidation = validateRowSize(data.rows[i]) if (!sizeValidation.valid) { - throw new Error(`Row ${i + 1}: ${sizeValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${i + 1}: ${sizeValidation.errors.join(', ')}` + ) } const schemaValidation = coerceRowToSchema(data.rows[i], table.schema) if (!schemaValidation.valid) { - throw new Error(`Row ${i + 1}: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${i + 1}: ${schemaValidation.errors.join(', ')}` + ) } } @@ -94,7 +101,8 @@ export async function bulkInsertImportBatch( db ) if (!uniqueResult.valid) { - throw new Error( + throw new OrchestrationError( + 'validation', uniqueResult.errors.map((e) => `Row ${e.row + 1}: ${e.errors.join(', ')}`).join('; ') ) } diff --git a/apps/sim/lib/table/import.ts b/apps/sim/lib/table/import.ts index 02c8a6e431a..3759e4eefe0 100644 --- a/apps/sim/lib/table/import.ts +++ b/apps/sim/lib/table/import.ts @@ -12,6 +12,7 @@ */ import { type Options as CsvParseOptions, type Parser, parse as parseCsvStream } from 'csv-parse' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { getColumnId } from '@/lib/table/column-keys' import type { ColumnType } from '@/lib/table/column-types' import { parseCurrencyInput } from '@/lib/table/currency' @@ -274,11 +275,11 @@ export async function parseCsvBuffer( const parsed = parse(text, options) as unknown as Record[] if (parsed.length === 0) { - throw new Error('CSV file has no data rows') + throw new OrchestrationError('validation', 'CSV file has no data rows') } if (headers.length === 0) { - throw new Error('CSV file has no headers') + throw new OrchestrationError('validation', 'CSV file has no headers') } return { headers, rows: parsed } @@ -648,15 +649,18 @@ export function parseJsonRows(buffer: Buffer | string): { const text = typeof buffer === 'string' ? buffer : buffer.toString('utf-8') const parsed = JSON.parse(text) if (!Array.isArray(parsed)) { - throw new Error('JSON file must contain an array of objects') + throw new OrchestrationError('validation', 'JSON file must contain an array of objects') } if (parsed.length === 0) { - throw new Error('JSON file contains an empty array') + throw new OrchestrationError('validation', 'JSON file contains an empty array') } const headerSet = new Set() for (const row of parsed) { if (typeof row !== 'object' || row === null || Array.isArray(row)) { - throw new Error('Each element in the JSON array must be a plain object') + throw new OrchestrationError( + 'validation', + 'Each element in the JSON array must be a plain object' + ) } for (const key of Object.keys(row)) headerSet.add(key) } @@ -685,5 +689,8 @@ export async function parseFileRows( ) return parseCsvBuffer(buffer, delimiter) } - throw new Error(`Unsupported file format: "${ext ?? fileName}". Supported: csv, tsv, json`) + throw new OrchestrationError( + 'validation', + `Unsupported file format: "${ext ?? fileName}". Supported: csv, tsv, json` + ) } diff --git a/apps/sim/lib/table/orchestration/columns.test.ts b/apps/sim/lib/table/orchestration/columns.test.ts index c7dde28a8db..d99eff54342 100644 --- a/apps/sim/lib/table/orchestration/columns.test.ts +++ b/apps/sim/lib/table/orchestration/columns.test.ts @@ -37,6 +37,7 @@ vi.mock('@/lib/table/columns/service', () => ({ updateColumnType: mockUpdateColumnType, })) +import { OrchestrationError } from '@/lib/core/orchestration/types' import { TableLockedError } from '@/lib/table/mutation-locks' import { performUpdateTableColumn } from '@/lib/table/orchestration/columns' @@ -185,18 +186,33 @@ describe('performUpdateTableColumn', () => { expect(mockRecordAudit).not.toHaveBeenCalled() }) - it('classifies a caller-fixable service error as validation', async () => { - mockUpdateColumnConstraints.mockRejectedValue(new Error('Column "State" already exists')) + it('reports the code the service failure carries', async () => { + mockUpdateColumnConstraints.mockRejectedValue( + new OrchestrationError('validation', 'Column "State" already exists') + ) expect((await run({ required: true })).errorCode).toBe('validation') }) it('classifies a missing column as not_found', async () => { - mockUpdateColumnConstraints.mockRejectedValue(new Error('Column "Nope" not found')) + mockUpdateColumnConstraints.mockRejectedValue( + new OrchestrationError('not_found', 'Column "Nope" not found') + ) expect((await run({ required: true })).errorCode).toBe('not_found') }) + it('keeps an unclassified fault internal and hides its message', async () => { + // Wording alone must never buy a status: this reads exactly like the + // caller-fixable failure above but carries no classification. + mockUpdateColumnConstraints.mockRejectedValue(new Error('Column "State" already exists')) + + const result = await run({ required: true }) + + expect(result.errorCode).toBe('internal') + expect(result.error).toBe('Failed to update column') + }) + it('audits a successful update on every caller', async () => { // The UI route and the copilot tool previously emitted no audit at all. await run({ required: true }) diff --git a/apps/sim/lib/table/orchestration/columns.ts b/apps/sim/lib/table/orchestration/columns.ts index b928f76a1ac..18d321d3e3b 100644 --- a/apps/sim/lib/table/orchestration/columns.ts +++ b/apps/sim/lib/table/orchestration/columns.ts @@ -1,8 +1,9 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' -import type { - OrchestrationErrorCode, - OrchestrationRequestContext, +import { + OrchestrationError, + type OrchestrationErrorCode, + type OrchestrationRequestContext, } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys' @@ -47,37 +48,12 @@ export interface PerformUpdateTableColumnResult { table?: TableDefinition } -/** - * Messages the column services raise for caller-fixable problems. They are - * thrown as plain `Error`s, so the classification lives here rather than being - * re-derived by each route. - */ -const VALIDATION_MESSAGE_FRAGMENTS = [ - 'already exists', - 'Cannot delete the last column', - 'Cannot set column', - 'Cannot set unique column', - 'Invalid column', - 'exceeds maximum', - 'incompatible', - 'duplicate', - 'option', - 'currency', - 'is already type', -] as const - function classify(error: unknown): PerformUpdateTableColumnResult { if (error instanceof TableLockedError) { return { success: false, error: error.message, errorCode: 'locked' } } - if (error instanceof Error) { - const message = error.message - if (message.includes('not found') || message.includes('Table not found')) { - return { success: false, error: message, errorCode: 'not_found' } - } - if (VALIDATION_MESSAGE_FRAGMENTS.some((fragment) => message.includes(fragment))) { - return { success: false, error: message, errorCode: 'validation' } - } + if (error instanceof OrchestrationError) { + return { success: false, error: error.message, errorCode: error.code } } return { success: false, error: 'Failed to update column', errorCode: 'internal' } } diff --git a/apps/sim/lib/table/orchestration/tables.test.ts b/apps/sim/lib/table/orchestration/tables.test.ts index 055f07afd85..09127e8e40b 100644 --- a/apps/sim/lib/table/orchestration/tables.test.ts +++ b/apps/sim/lib/table/orchestration/tables.test.ts @@ -4,26 +4,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { TableDefinition } from '@/lib/table/types' -const { - mockDeleteTable, - mockDeleteRow, - mockRenameTable, - mockCaptureServerEvent, - mockRecordAudit, - MockTableConflictError, -} = vi.hoisted(() => ({ - mockDeleteTable: vi.fn(), - mockDeleteRow: vi.fn(), - mockRenameTable: vi.fn(), - mockCaptureServerEvent: vi.fn(), - mockRecordAudit: vi.fn(), - MockTableConflictError: class extends Error { - readonly code = 'TABLE_EXISTS' as const - constructor(name: string) { - super(`A table named "${name}" already exists in this workspace`) - } - }, -})) +const { mockDeleteTable, mockDeleteRow, mockRenameTable, mockCaptureServerEvent, mockRecordAudit } = + vi.hoisted(() => ({ + mockDeleteTable: vi.fn(), + mockDeleteRow: vi.fn(), + mockRenameTable: vi.fn(), + mockCaptureServerEvent: vi.fn(), + mockRecordAudit: vi.fn(), + })) vi.mock('@sim/audit', () => ({ AuditAction: { TABLE_DELETED: 'table.deleted', TABLE_UPDATED: 'table.updated' }, @@ -36,11 +24,11 @@ vi.mock('@/lib/table/service', () => ({ moveTableToFolder: vi.fn(), renameTable: mockRenameTable, updateTableLocks: vi.fn(), - TableConflictError: MockTableConflictError, })) vi.mock('@/lib/table/rows/service', () => ({ deleteRow: mockDeleteRow })) vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: mockCaptureServerEvent })) +import { OrchestrationError } from '@/lib/core/orchestration/types' import { TableLockedError } from '@/lib/table/mutation-locks' import { performDeleteTable, @@ -106,12 +94,24 @@ describe('performRenameTable', () => { beforeEach(() => vi.clearAllMocks()) it('classifies a name collision as a conflict, not bad input', async () => { - mockRenameTable.mockRejectedValue(new MockTableConflictError('Tasks')) + // `TableConflictError` is an `OrchestrationError('conflict')` — the class + // decides the status, so the 409 no longer rides on the message wording. + mockRenameTable.mockRejectedValue( + new OrchestrationError('conflict', 'A table named "Tasks" already exists in this workspace') + ) const result = await performRenameTable({ table: TABLE, newName: 'Tasks', userId: 'user-1' }) expect(result).toMatchObject({ success: false, errorCode: 'conflict' }) }) + + it('keeps an unclassified rename failure internal', async () => { + mockRenameTable.mockRejectedValue(new Error('A table named "Tasks" already exists')) + + expect( + (await performRenameTable({ table: TABLE, newName: 'Tasks', userId: 'user-1' })).errorCode + ).toBe('internal') + }) }) describe('performDeleteTableRow', () => { @@ -133,7 +133,7 @@ describe('performDeleteTableRow', () => { }) it('classifies a missing row as not_found', async () => { - mockDeleteRow.mockRejectedValue(new Error('Row not found')) + mockDeleteRow.mockRejectedValue(new OrchestrationError('not_found', 'Row not found')) expect((await performDeleteTableRow({ table: TABLE, rowId: 'row-1' })).errorCode).toBe( 'not_found' diff --git a/apps/sim/lib/table/orchestration/tables.ts b/apps/sim/lib/table/orchestration/tables.ts index 2c89fa19941..dcec50a25cc 100644 --- a/apps/sim/lib/table/orchestration/tables.ts +++ b/apps/sim/lib/table/orchestration/tables.ts @@ -1,21 +1,16 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' -import type { - OrchestrationErrorCode, - OrchestrationRequestContext, +import { + OrchestrationError, + type OrchestrationErrorCode, + type OrchestrationRequestContext, } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' import { captureServerEvent } from '@/lib/posthog/server' import { TableLockedError } from '@/lib/table/mutation-locks' import { deleteRow } from '@/lib/table/rows/service' -import { - deleteTable, - moveTableToFolder, - renameTable, - TableConflictError, - updateTableLocks, -} from '@/lib/table/service' +import { deleteTable, moveTableToFolder, renameTable, updateTableLocks } from '@/lib/table/service' import { TABLE_LOCK_FLAGS, TABLE_LOCK_KINDS, @@ -61,6 +56,9 @@ export async function performDeleteTable( if (error instanceof TableLockedError) { return { success: false, error: error.message, errorCode: 'locked' } } + if (error instanceof OrchestrationError) { + return { success: false, error: error.message, errorCode: error.code } + } logger.error(`[${requestId}] Failed to delete table ${table.id}`, { error }) return { success: false, error: toError(error).message, errorCode: 'internal' } } @@ -120,8 +118,8 @@ export async function performDeleteTableRow( if (error instanceof TableLockedError) { return { success: false, error: error.message, errorCode: 'locked' } } - if (error instanceof Error && error.message === 'Row not found') { - return { success: false, error: 'Row not found', errorCode: 'not_found' } + if (error instanceof OrchestrationError) { + return { success: false, error: error.message, errorCode: error.code } } logger.error(`[${requestId}] Failed to delete row ${rowId} from table ${table.id}`, { error }) return { success: false, error: toError(error).message, errorCode: 'internal' } @@ -148,22 +146,18 @@ function classifyTableMutation(error: unknown, requestId: string, tableId: strin if (error instanceof TableLockedError) { return { success: false as const, error: error.message, errorCode: 'locked' as const } } - // A name collision is a conflict, not bad input — the same class - // `performRestoreTable` reports, and the 409 the UI route returned before it - // delegated. Matched on the type, not on "already exists" appearing in the - // message, so a rewording cannot silently demote it to a 400. - if (error instanceof TableConflictError) { - return { success: false as const, error: error.message, errorCode: 'conflict' as const } - } - const message = toError(error).message - if (message.includes('not found')) { - return { success: false as const, error: message, errorCode: 'not_found' as const } - } - if (message.includes('Invalid')) { - return { success: false as const, error: message, errorCode: 'validation' as const } + // `TableConflictError` is an `OrchestrationError('conflict')`, so a duplicate + // rename reaches 409 through this branch — by class, not by the message + // happening to contain "already exists". + if (error instanceof OrchestrationError) { + return { success: false as const, error: error.message, errorCode: error.code } } logger.error(`[${requestId}] Table mutation failed for ${tableId}`, { error }) - return { success: false as const, error: message, errorCode: 'internal' as const } + return { + success: false as const, + error: toError(error).message, + errorCode: 'internal' as const, + } } /** Renames a table and records the rename against `userId`. */ diff --git a/apps/sim/lib/table/rows/service.ts b/apps/sim/lib/table/rows/service.ts index 2cf9e357b63..5637daad5b5 100644 --- a/apps/sim/lib/table/rows/service.ts +++ b/apps/sim/lib/table/rows/service.ts @@ -16,6 +16,7 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { and, count, eq, inArray, lte, notInArray, type SQL, sql } from 'drizzle-orm' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { assertRowCapacity, getMaxRowsPerTable, @@ -124,13 +125,16 @@ export async function insertRow( // Validate row size const sizeValidation = validateRowSize(data.data) if (!sizeValidation.valid) { - throw new Error(sizeValidation.errors.join(', ')) + throw new OrchestrationError('validation', sizeValidation.errors.join(', ')) } // Validate against schema const schemaValidation = coerceRowToSchema(data.data, table.schema) if (!schemaValidation.valid) { - throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Schema validation failed: ${schemaValidation.errors.join(', ')}` + ) } // Check unique constraints using optimized database query @@ -138,7 +142,7 @@ export async function insertRow( if (uniqueColumns.length > 0) { const uniqueValidation = await checkUniqueConstraintsDb(data.tableId, data.data, table.schema) if (!uniqueValidation.valid) { - throw new Error(uniqueValidation.errors.join(', ')) + throw new OrchestrationError('validation', uniqueValidation.errors.join(', ')) } } @@ -261,12 +265,18 @@ export async function batchInsertRowsWithTx( const sizeValidation = validateRowSize(row) if (!sizeValidation.valid) { - throw new Error(`Row ${i + 1}: ${sizeValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${i + 1}: ${sizeValidation.errors.join(', ')}` + ) } const schemaValidation = coerceRowToSchema(row, table.schema) if (!schemaValidation.valid) { - throw new Error(`Row ${i + 1}: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${i + 1}: ${schemaValidation.errors.join(', ')}` + ) } } @@ -282,7 +292,7 @@ export async function batchInsertRowsWithTx( const errorMessages = uniqueResult.errors .map((e) => `Row ${e.row + 1}: ${e.errors.join(', ')}`) .join('; ') - throw new Error(errorMessages) + throw new OrchestrationError('validation', errorMessages) } } @@ -422,12 +432,18 @@ export async function replaceTableRowsWithTx( const sizeValidation = validateRowSize(row) if (!sizeValidation.valid) { - throw new Error(`Row ${i + 1}: ${sizeValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${i + 1}: ${sizeValidation.errors.join(', ')}` + ) } const schemaValidation = coerceRowToSchema(row, table.schema) if (!schemaValidation.valid) { - throw new Error(`Row ${i + 1}: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${i + 1}: ${schemaValidation.errors.join(', ')}` + ) } } @@ -449,7 +465,8 @@ export async function replaceTableRowsWithTx( const normalized = typeof value === 'string' ? value : JSON.stringify(value) const map = seen.get(colId)! if (map.has(normalized)) { - throw new Error( + throw new OrchestrationError( + 'validation', `Row ${i + 1}: Column "${col.name}" must be unique. Value "${String(value)}" duplicates row ${map.get(normalized)! + 1} in batch` ) } @@ -538,7 +555,8 @@ export async function upsertRow( const uniqueColumns = getUniqueColumns(schema) if (uniqueColumns.length === 0) { - throw new Error( + throw new OrchestrationError( + 'validation', 'Upsert requires at least one unique column in the schema. Please add a unique constraint to a column or use insert instead.' ) } @@ -552,7 +570,8 @@ export async function upsertRow( (c) => getColumnId(c) === data.conflictTarget || c.name === data.conflictTarget ) if (!col) { - throw new Error( + throw new OrchestrationError( + 'validation', `Column "${data.conflictTarget}" is not a unique column. Available unique columns: ${uniqueColumns.map((c) => c.name).join(', ')}` ) } @@ -560,7 +579,8 @@ export async function upsertRow( } else if (uniqueColumns.length === 1) { targetColumnKey = getColumnId(uniqueColumns[0]) } else { - throw new Error( + throw new OrchestrationError( + 'validation', `Table has multiple unique columns (${uniqueColumns.map((c) => c.name).join(', ')}). Specify a conflict column to indicate which one to match on.` ) } @@ -568,12 +588,15 @@ export async function upsertRow( // Validate row data const sizeValidation = validateRowSize(data.data) if (!sizeValidation.valid) { - throw new Error(sizeValidation.errors.join(', ')) + throw new OrchestrationError('validation', sizeValidation.errors.join(', ')) } const schemaValidation = coerceRowToSchema(data.data, schema) if (!schemaValidation.valid) { - throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Schema validation failed: ${schemaValidation.errors.join(', ')}` + ) } // Read the conflict-target value *after* coercion so `matchFilter` branches on @@ -583,7 +606,10 @@ export async function upsertRow( // Surface the display name, not the internal id — v1 callers pass a name. const targetColumnName = uniqueColumns.find((c) => getColumnId(c) === targetColumnKey)?.name ?? targetColumnKey - throw new Error(`Upsert requires a value for the conflict target column "${targetColumnName}"`) + throw new OrchestrationError( + 'validation', + `Upsert requires a value for the conflict target column "${targetColumnName}"` + ) } // Build the conflict probe through the SAME leaf as the unique-constraint check @@ -636,7 +662,10 @@ export async function upsertRow( trx ) if (!uniqueValidation.valid) { - throw new Error(`Unique constraint violation: ${uniqueValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Unique constraint violation: ${uniqueValidation.errors.join(', ')}` + ) } const now = new Date() @@ -1429,7 +1458,7 @@ export async function updateRow( // Get existing row const existingRow = await getRowById(data.tableId, data.rowId, data.workspaceId) if (!existingRow) { - throw new Error('Row not found') + throw new OrchestrationError('not_found', 'Row not found') } // Merge partial update with existing row data so callers can pass only changed fields @@ -1453,13 +1482,16 @@ export async function updateRow( // Validate size const sizeValidation = validateRowSize(mergedData) if (!sizeValidation.valid) { - throw new Error(sizeValidation.errors.join(', ')) + throw new OrchestrationError('validation', sizeValidation.errors.join(', ')) } // Validate against schema const schemaValidation = coerceRowToSchema(mergedData, table.schema) if (!schemaValidation.valid) { - throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Schema validation failed: ${schemaValidation.errors.join(', ')}` + ) } // Check unique constraints using optimized database query @@ -1472,7 +1504,7 @@ export async function updateRow( data.rowId // Exclude current row ) if (!uniqueValidation.valid) { - throw new Error(uniqueValidation.errors.join(', ')) + throw new OrchestrationError('validation', uniqueValidation.errors.join(', ')) } } @@ -1612,7 +1644,7 @@ export async function deleteRow( workspaceId: table.workspaceId, proof, }) - if (!deleted) throw new Error('Row not found') + if (!deleted) throw new OrchestrationError('not_found', 'Row not found') logger.info(`[${requestId}] Deleted row ${rowId} from table ${table.id}`) } @@ -1636,7 +1668,7 @@ export async function updateRowsByFilter( const filterClause = buildFilterClause(data.filter, tableName, table.schema.columns) if (!filterClause) { - throw new Error('Filter is required for bulk update') + throw new OrchestrationError('validation', 'Filter is required for bulk update') } const baseConditions = and( @@ -1678,12 +1710,18 @@ export async function updateRowsByFilter( const sizeValidation = validateRowSize(mergedData) if (!sizeValidation.valid) { - throw new Error(`Row ${row.id}: ${sizeValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${row.id}: ${sizeValidation.errors.join(', ')}` + ) } const schemaValidation = coerceRowToSchema(mergedData, table.schema) if (!schemaValidation.valid) { - throw new Error(`Row ${row.id}: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${row.id}: ${schemaValidation.errors.join(', ')}` + ) } } @@ -1691,7 +1729,8 @@ export async function updateRowsByFilter( const uniqueColumnsInUpdate = uniqueColumns.filter((col) => col.name in data.data) if (uniqueColumnsInUpdate.length > 0) { if (matchingRows.length > 1) { - throw new Error( + throw new OrchestrationError( + 'validation', `Cannot set unique column values when updating multiple rows. ` + `Columns with unique constraint: ${uniqueColumnsInUpdate.map((c) => c.name).join(', ')}. ` + `Updating ${matchingRows.length} rows with the same value would violate uniqueness.` @@ -1708,7 +1747,10 @@ export async function updateRowsByFilter( row.id ) if (!uniqueValidation.valid) { - throw new Error(`Unique constraint violation: ${uniqueValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Unique constraint violation: ${uniqueValidation.errors.join(', ')}` + ) } } @@ -1826,7 +1868,7 @@ export async function batchUpdateRows( const missing = rowIds.filter((id) => !existingMap.has(id)) if (missing.length > 0) { - throw new Error(`Rows not found: ${missing.join(', ')}`) + throw new OrchestrationError('validation', `Rows not found: ${missing.join(', ')}`) } const mergedUpdates: Array<{ @@ -1856,12 +1898,18 @@ export async function batchUpdateRows( const sizeValidation = validateRowSize(merged) if (!sizeValidation.valid) { - throw new Error(`Row ${update.rowId}: ${sizeValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${update.rowId}: ${sizeValidation.errors.join(', ')}` + ) } const schemaValidation = coerceRowToSchema(merged, table.schema) if (!schemaValidation.valid) { - throw new Error(`Row ${update.rowId}: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${update.rowId}: ${schemaValidation.errors.join(', ')}` + ) } mergedUpdates.push({ @@ -1884,7 +1932,10 @@ export async function batchUpdateRows( rowId ) if (!uniqueValidation.valid) { - throw new Error(`Row ${rowId}: ${uniqueValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Row ${rowId}: ${uniqueValidation.errors.join(', ')}` + ) } } } @@ -2006,7 +2057,7 @@ export async function deleteRowsByFilter( // Build filter clause const filterClause = buildFilterClause(data.filter, tableName, table.schema.columns) if (!filterClause) { - throw new Error('Filter is required for bulk delete') + throw new OrchestrationError('validation', 'Filter is required for bulk delete') } // Find matching rows diff --git a/apps/sim/lib/table/service.ts b/apps/sim/lib/table/service.ts index 0af610c0d52..5a6f78dd84a 100644 --- a/apps/sim/lib/table/service.ts +++ b/apps/sim/lib/table/service.ts @@ -14,6 +14,7 @@ import { createLogger } from '@sim/logger' import { getPostgresErrorCode } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { and, count, eq, isNull, sql } from 'drizzle-orm' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { generateRestoreName } from '@/lib/core/utils/restore-name' import type { DbOrTx } from '@/lib/db/types' import { resolveRestoredFolderId } from '@/lib/folders/queries' @@ -39,10 +40,15 @@ import { stripGroupDeps } from '@/lib/table/workflow-columns' const logger = createLogger('TableService') -export class TableConflictError extends Error { - readonly code = 'TABLE_EXISTS' as const +/** + * A table name already taken in the workspace. Kept as its own class because + * several routes branch on it specifically, and typed as a `conflict` so the + * generic classifiers reach the same 409 without reading the message. + */ +export class TableConflictError extends OrchestrationError { constructor(name: string) { - super(`A table named "${name}" already exists in this workspace`) + super('conflict', `A table named "${name}" already exists in this workspace`) + this.name = 'TableConflictError' } } @@ -101,7 +107,7 @@ export async function withLockedTable( ) const table = await getTableById(tableId, { tx: trx, includeArchived: opts?.includeArchived }) if (!table) { - throw new Error('Table not found') + throw new OrchestrationError('not_found', 'Table not found') } return mutate(table, trx) }) @@ -283,13 +289,19 @@ export async function createTable( // Validate table name const nameValidation = validateTableName(data.name) if (!nameValidation.valid) { - throw new Error(`Invalid table name: ${nameValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Invalid table name: ${nameValidation.errors.join(', ')}` + ) } // Validate schema const schemaValidation = validateTableSchema(data.schema) if (!schemaValidation.valid) { - throw new Error(`Invalid schema: ${schemaValidation.errors.join(', ')}`) + throw new OrchestrationError( + 'validation', + `Invalid schema: ${schemaValidation.errors.join(', ')}` + ) } const tableId = `tbl_${generateId().replace(/-/g, '')}` @@ -353,7 +365,12 @@ export async function createTable( ) if (Number(existingCount) >= maxTables) { - throw new Error(`Workspace has reached maximum table limit (${maxTables})`) + // A quota ceiling, not bad input — both create routes have always + // answered 403 for it. + throw new OrchestrationError( + 'forbidden', + `Workspace has reached maximum table limit (${maxTables})` + ) } const duplicateName = await trx @@ -472,23 +489,26 @@ export async function addTableColumnsWithTx( for (const column of columns) { if (!NAME_PATTERN.test(column.name)) { - throw new Error( + throw new OrchestrationError( + 'validation', `Invalid column name "${column.name}". Must start with a letter or underscore and contain only alphanumeric characters and underscores.` ) } if (column.name.length > TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH) { - throw new Error( + throw new OrchestrationError( + 'validation', `Column name exceeds maximum length (${TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH} characters)` ) } if (!COLUMN_TYPES.includes(column.type as (typeof COLUMN_TYPES)[number])) { - throw new Error( + throw new OrchestrationError( + 'validation', `Invalid column type "${column.type}". Must be one of: ${COLUMN_TYPES.join(', ')}` ) } const lower = column.name.toLowerCase() if (usedNames.has(lower)) { - throw new Error(`Column "${column.name}" already exists`) + throw new OrchestrationError('validation', `Column "${column.name}" already exists`) } usedNames.add(lower) // Honor a caller-assigned id (the CSV append path pre-assigns so coercion @@ -504,7 +524,8 @@ export async function addTableColumnsWithTx( } if (table.schema.columns.length + additions.length > TABLE_LIMITS.MAX_COLUMNS_PER_TABLE) { - throw new Error( + throw new OrchestrationError( + 'validation', `Adding ${additions.length} column(s) would exceed maximum column limit (${TABLE_LIMITS.MAX_COLUMNS_PER_TABLE})` ) } @@ -574,7 +595,7 @@ export async function renameTable( ): Promise<{ id: string; name: string }> { const nameValidation = validateTableName(newName) if (!nameValidation.valid) { - throw new Error(nameValidation.errors.join(', ')) + throw new OrchestrationError('validation', nameValidation.errors.join(', ')) } const now = new Date() @@ -586,7 +607,7 @@ export async function renameTable( .returning({ id: userTableDefinitions.id }) if (result.length === 0) { - throw new Error(`Table ${tableId} not found`) + throw new OrchestrationError('not_found', `Table ${tableId} not found`) } logger.info(`[${requestId}] Renamed table ${tableId} to "${newName}"`) @@ -643,7 +664,7 @@ export async function moveTableToFolder( }) if (result.length === 0) { - throw new Error(`Table ${tableId} not found`) + throw new OrchestrationError('not_found', `Table ${tableId} not found`) } const { name } = result[0] @@ -831,18 +852,18 @@ export async function restoreTable( ): Promise { const table = await getTableById(tableId, { includeArchived: true }) if (!table) { - throw new Error('Table not found') + throw new OrchestrationError('not_found', 'Table not found') } if (!table.archivedAt) { - throw new Error('Table is not archived') + throw new OrchestrationError('validation', 'Table is not archived') } if (table.workspaceId) { const { getWorkspaceWithOwner } = await import('@/lib/workspaces/permissions/utils') const ws = await getWorkspaceWithOwner(table.workspaceId) if (!ws || ws.archivedAt) { - throw new Error('Cannot restore table into an archived workspace') + throw new OrchestrationError('validation', 'Cannot restore table into an archived workspace') } } diff --git a/apps/sim/lib/table/workflow-columns.ts b/apps/sim/lib/table/workflow-columns.ts index 4054113d4e2..a2ccc24d4d6 100644 --- a/apps/sim/lib/table/workflow-columns.ts +++ b/apps/sim/lib/table/workflow-columns.ts @@ -18,6 +18,7 @@ import { and, eq, inArray, notInArray, sql } from 'drizzle-orm' import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution' import type { EnqueueOptions } from '@/lib/core/async-jobs/types' import { isTriggerDevEnabled } from '@/lib/core/config/env-flags' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { buildCancelledExecution } from '@/lib/table/cell-write' import type { Filter, @@ -731,8 +732,9 @@ export async function runWorkflowColumn(opts: { // this module; `@trigger.dev/sdk` is heavy and only needed on this op. const { getTableById } = await import('@/lib/table/service') const table = await getTableById(tableId) - if (!table) throw new Error('Table not found') - if (table.workspaceId !== workspaceId) throw new Error('Invalid workspace ID') + if (!table) throw new OrchestrationError('not_found', 'Table not found') + if (table.workspaceId !== workspaceId) + throw new OrchestrationError('validation', 'Invalid workspace ID') const allGroups = table.schema.workflowGroups ?? [] const targetGroups = groupIds ? allGroups.filter((g) => groupIds.includes(g.id)) : allGroups