Skip to content

Commit 9736288

Browse files
refactor(tables): make lib/table/orchestration the single implementation
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 <noreply@anthropic.com>
1 parent bbc82b2 commit 9736288

24 files changed

Lines changed: 1180 additions & 567 deletions

File tree

apps/sim/app/api/table/[tableId]/columns/route.ts

Lines changed: 15 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,11 @@ import {
88
import { parseRequest } from '@/lib/api/server'
99
import { isZodError, validationErrorResponse } from '@/lib/api/server/validation'
1010
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
11+
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1112
import { generateRequestId } from '@/lib/core/utils/request'
1213
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
13-
import {
14-
addTableColumn,
15-
deleteColumn,
16-
renameColumn,
17-
updateColumnConstraints,
18-
updateColumnOptions,
19-
updateColumnType,
20-
} from '@/lib/table'
21-
import { columnMatchesRef } from '@/lib/table/column-keys'
14+
import { addTableColumn, deleteColumn } from '@/lib/table'
15+
import { performUpdateTableColumn } from '@/lib/table/orchestration'
2216
import {
2317
accessError,
2418
checkAccess,
@@ -117,111 +111,31 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
117111
return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 })
118112
}
119113

120-
const { updates } = validated
121-
let updatedTable = null
122-
123-
if (updates.name) {
124-
updatedTable = await renameColumn(
125-
{ tableId, oldName: validated.columnName, newName: updates.name },
126-
requestId
127-
)
128-
}
129-
130-
// A payload that repeats the current type must not go through
131-
// `updateColumnType` — it early-returns on an unchanged type and would drop
132-
// any `options` alongside it. Only a real type change routes there; an
133-
// unchanged type with options routes to the options-only update.
134-
const currentColumn = table.schema.columns.find((c) =>
135-
columnMatchesRef(c, validated.columnName)
136-
)
137-
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
138-
139-
// Every write below is its own locked transaction, so any of them paired
140-
// with a constraint write that is going to fail commits and then errors.
141-
// Gate on the type the column ENDS UP with, not on whether the type is
142-
// changing: an options-only update on an existing select column carries the
143-
// same hazard as a conversion does.
144-
const resultingType = updates.type ?? currentColumn?.type
145-
if (updates.unique === true && resultingType === 'select') {
146-
return NextResponse.json({ error: 'Cannot set a select column as unique' }, { status: 400 })
147-
}
148-
149-
if (typeChanging) {
150-
updatedTable = await updateColumnType(
151-
{
152-
tableId,
153-
columnName: updates.name ?? validated.columnName,
154-
newType: updates.type as NonNullable<typeof updates.type>,
155-
...(updates.options !== undefined ? { options: updates.options } : {}),
156-
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
157-
// Forwarded so the conversion validates against the constraint this
158-
// same request is about to set, not the column's current one.
159-
...(updates.required !== undefined ? { required: updates.required } : {}),
160-
},
161-
requestId
162-
)
163-
} else if (updates.options !== undefined || updates.multiple !== undefined) {
164-
updatedTable = await updateColumnOptions(
165-
{
166-
tableId,
167-
columnName: updates.name ?? validated.columnName,
168-
options: updates.options ?? currentColumn?.options ?? [],
169-
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
170-
// Forwarded so the removal guard validates against the constraint this
171-
// same request is about to set, not the column's current one.
172-
...(updates.required !== undefined ? { required: updates.required } : {}),
173-
},
174-
requestId
175-
)
176-
}
177-
178-
if (updates.required !== undefined || updates.unique !== undefined) {
179-
updatedTable = await updateColumnConstraints(
180-
{
181-
tableId,
182-
columnName: updates.name ?? validated.columnName,
183-
...(updates.required !== undefined ? { required: updates.required } : {}),
184-
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
185-
},
186-
requestId
114+
const outcome = await performUpdateTableColumn({
115+
table,
116+
columnName: validated.columnName,
117+
userId: authResult.userId,
118+
updates: validated.updates,
119+
requestId,
120+
})
121+
if (!outcome.success || !outcome.table) {
122+
return NextResponse.json(
123+
{ error: outcome.error ?? 'Failed to update column' },
124+
{ status: statusForOrchestrationError(outcome.errorCode) }
187125
)
188126
}
189127

190-
if (!updatedTable) {
191-
return NextResponse.json({ error: 'No updates specified' }, { status: 400 })
192-
}
193-
194128
return NextResponse.json({
195129
success: true,
196130
data: {
197-
columns: updatedTable.schema.columns.map(normalizeColumn),
131+
columns: outcome.table.schema.columns.map(normalizeColumn),
198132
},
199133
})
200134
} catch (error) {
201-
const lockError = tableLockErrorResponse(error)
202-
if (lockError) return lockError
203135
if (isZodError(error)) {
204136
return validationErrorResponse(error, 'Invalid request data')
205137
}
206138

207-
const msg = rootErrorMessage(error)
208-
if (msg.includes('not found') || msg.includes('Table not found')) {
209-
return NextResponse.json({ error: msg }, { status: 404 })
210-
}
211-
if (
212-
msg.includes('already exists') ||
213-
msg.includes('Cannot delete the last column') ||
214-
msg.includes('Cannot set column') ||
215-
msg.includes('Cannot set unique column') ||
216-
msg.includes('Invalid column') ||
217-
msg.includes('exceeds maximum') ||
218-
msg.includes('incompatible') ||
219-
msg.includes('duplicate') ||
220-
msg.includes('option')
221-
) {
222-
return NextResponse.json({ error: msg }, { status: 400 })
223-
}
224-
225139
logger.error(`[${requestId}] Error updating column in table ${tableId}:`, error)
226140
return NextResponse.json({ error: 'Failed to update column' }, { status: 500 })
227141
}

apps/sim/app/api/table/[tableId]/route.ts

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,11 @@ import { getTableQuerySchema, updateTableContract } from '@/lib/api/contracts/ta
55
import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation'
66
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
77
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
8+
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
89
import { generateRequestId } from '@/lib/core/utils/request'
910
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1011
import { findActiveFolder } from '@/lib/folders/queries'
11-
import { captureServerEvent } from '@/lib/posthog/server'
1212
import {
13-
deleteTable,
1413
getTableById,
1514
moveTableToFolder,
1615
renameTable,
@@ -19,6 +18,7 @@ import {
1918
updateTableLocks,
2019
} from '@/lib/table'
2120
import { getWorkspaceTableLimits } from '@/lib/table/billing'
21+
import { performDeleteTable } from '@/lib/table/orchestration'
2222
import { TABLE_LOCK_FLAGS, TABLE_LOCK_KINDS } from '@/lib/table/types'
2323
import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils'
2424
import {
@@ -268,14 +268,13 @@ export const DELETE = withRouteHandler(
268268
return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 })
269269
}
270270

271-
await deleteTable(tableId, requestId, authResult.userId)
272-
273-
captureServerEvent(
274-
authResult.userId,
275-
'table_deleted',
276-
{ table_id: tableId, workspace_id: table.workspaceId },
277-
{ groups: { workspace: table.workspaceId } }
278-
)
271+
const outcome = await performDeleteTable({ table, userId: authResult.userId, requestId })
272+
if (!outcome.success) {
273+
return NextResponse.json(
274+
{ error: outcome.error ?? 'Failed to delete table' },
275+
{ status: statusForOrchestrationError(outcome.errorCode) }
276+
)
277+
}
279278

280279
return NextResponse.json({
281280
success: true,

apps/sim/app/api/table/[tableId]/rows/[rowId]/route.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,12 @@ import {
1111
} from '@/lib/api/contracts/tables'
1212
import { isZodError, parseRequest, validationErrorResponse } from '@/lib/api/server/validation'
1313
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
14+
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1415
import { generateRequestId } from '@/lib/core/utils/request'
1516
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1617
import type { RowData, TableSchema } from '@/lib/table'
17-
import { deleteRow, updateRow } from '@/lib/table'
18+
import { updateRow } from '@/lib/table'
19+
import { performDeleteTableRow } from '@/lib/table/orchestration'
1820
import { rowWireTranslators } from '@/app/api/table/row-wire'
1921
import {
2022
accessError,
@@ -212,7 +214,13 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Row
212214
return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 })
213215
}
214216

215-
await deleteRow(table, rowId, requestId)
217+
const outcome = await performDeleteTableRow({ table, rowId, requestId })
218+
if (!outcome.success) {
219+
return NextResponse.json(
220+
{ error: outcome.error ?? 'Failed to delete row' },
221+
{ status: statusForOrchestrationError(outcome.errorCode) }
222+
)
223+
}
216224

217225
return NextResponse.json({
218226
success: true,

apps/sim/app/api/v1/tables/[tableId]/columns/route.ts

Lines changed: 15 additions & 114 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,11 @@ import {
77
v1UpdateTableColumnContract,
88
} from '@/lib/api/contracts/v1/tables'
99
import { parseRequest } from '@/lib/api/server'
10+
import { statusForOrchestrationError } from '@/lib/core/orchestration/types'
1011
import { generateRequestId } from '@/lib/core/utils/request'
1112
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
12-
import {
13-
addTableColumn,
14-
deleteColumn,
15-
renameColumn,
16-
updateColumnConstraints,
17-
updateColumnOptions,
18-
updateColumnType,
19-
} from '@/lib/table'
20-
import { columnMatchesRef } from '@/lib/table/column-keys'
13+
import { addTableColumn, deleteColumn } from '@/lib/table'
14+
import { performUpdateTableColumn } from '@/lib/table/orchestration'
2115
import {
2216
accessError,
2317
checkAccess,
@@ -151,123 +145,30 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
151145
return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 })
152146
}
153147

154-
const { updates } = validated
155-
let updatedTable = null
156-
157-
if (updates.name) {
158-
updatedTable = await renameColumn(
159-
{ tableId, oldName: validated.columnName, newName: updates.name },
160-
requestId
161-
)
162-
}
163-
164-
// A payload that repeats the current type must not go through
165-
// `updateColumnType` — it early-returns on an unchanged type and would drop
166-
// any `options` alongside it. Only a real type change routes there; an
167-
// unchanged type with options routes to the options-only update.
168-
const currentColumn = table.schema.columns.find((c) =>
169-
columnMatchesRef(c, validated.columnName)
170-
)
171-
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
172-
173-
// Every write below is its own locked transaction, so any of them paired
174-
// with a constraint write that is going to fail commits and then errors.
175-
// Gate on the type the column ENDS UP with, not on whether the type is
176-
// changing: an options-only update on an existing select column carries the
177-
// same hazard as a conversion does.
178-
const resultingType = updates.type ?? currentColumn?.type
179-
if (updates.unique === true && resultingType === 'select') {
180-
return NextResponse.json({ error: 'Cannot set a select column as unique' }, { status: 400 })
181-
}
182-
183-
if (typeChanging) {
184-
updatedTable = await updateColumnType(
185-
{
186-
tableId,
187-
columnName: updates.name ?? validated.columnName,
188-
newType: updates.type as NonNullable<typeof updates.type>,
189-
...(updates.options !== undefined ? { options: updates.options } : {}),
190-
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
191-
// Forwarded so the conversion validates against the constraint this
192-
// same request is about to set, not the column's current one.
193-
...(updates.required !== undefined ? { required: updates.required } : {}),
194-
},
195-
requestId
196-
)
197-
} else if (updates.options !== undefined || updates.multiple !== undefined) {
198-
updatedTable = await updateColumnOptions(
199-
{
200-
tableId,
201-
columnName: updates.name ?? validated.columnName,
202-
options: updates.options ?? currentColumn?.options ?? [],
203-
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
204-
// Forwarded so the removal guard validates against the constraint this
205-
// same request is about to set, not the column's current one.
206-
...(updates.required !== undefined ? { required: updates.required } : {}),
207-
},
208-
requestId
209-
)
210-
}
211-
212-
if (updates.required !== undefined || updates.unique !== undefined) {
213-
updatedTable = await updateColumnConstraints(
214-
{
215-
tableId,
216-
columnName: updates.name ?? validated.columnName,
217-
...(updates.required !== undefined ? { required: updates.required } : {}),
218-
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
219-
},
220-
requestId
148+
const outcome = await performUpdateTableColumn({
149+
table,
150+
columnName: validated.columnName,
151+
userId,
152+
updates: validated.updates,
153+
requestId,
154+
})
155+
if (!outcome.success || !outcome.table) {
156+
return NextResponse.json(
157+
{ error: outcome.error ?? 'Failed to update column' },
158+
{ status: statusForOrchestrationError(outcome.errorCode) }
221159
)
222160
}
223161

224-
if (!updatedTable) {
225-
return NextResponse.json({ error: 'No updates specified' }, { status: 400 })
226-
}
227-
228-
recordAudit({
229-
workspaceId: validated.workspaceId,
230-
actorId: userId,
231-
action: AuditAction.TABLE_UPDATED,
232-
resourceType: AuditResourceType.TABLE,
233-
resourceId: tableId,
234-
resourceName: table.name,
235-
description: `Updated column "${validated.columnName}" in table "${table.name}"`,
236-
metadata: { columnName: validated.columnName, updates },
237-
request,
238-
})
239-
240162
return NextResponse.json({
241163
success: true,
242164
data: {
243-
columns: updatedTable.schema.columns.map(normalizeColumn),
165+
columns: outcome.table.schema.columns.map(normalizeColumn),
244166
},
245167
})
246168
} catch (error) {
247-
const lockError = tableLockErrorResponse(error)
248-
if (lockError) return lockError
249169
const validationResponse = v1ValidationErrorResponseFromError(error)
250170
if (validationResponse) return validationResponse
251171

252-
if (error instanceof Error) {
253-
const msg = error.message
254-
if (msg.includes('not found') || msg.includes('Table not found')) {
255-
return NextResponse.json({ error: msg }, { status: 404 })
256-
}
257-
if (
258-
msg.includes('already exists') ||
259-
msg.includes('Cannot delete the last column') ||
260-
msg.includes('Cannot set column') ||
261-
msg.includes('Invalid column') ||
262-
msg.includes('exceeds maximum') ||
263-
msg.includes('incompatible') ||
264-
msg.includes('duplicate') ||
265-
msg.includes('option')
266-
) {
267-
return NextResponse.json({ error: msg }, { status: 400 })
268-
}
269-
}
270-
271172
logger.error(`[${requestId}] Error updating column in table:`, error)
272173
return NextResponse.json({ error: 'Failed to update column' }, { status: 500 })
273174
}

0 commit comments

Comments
 (0)