Skip to content

Commit 7ca070b

Browse files
fix(tables): map append-import locks to 423 and stop conflating locks with permissions
- The sync append branch returns instead of rethrowing, so the outer catch's mapper never saw a TableLockedError and every lock violation became a 500. Map it in that catch, with a regression test (replace mode already rethrows) - Stop mounting the workflow-group column menu for users without edit access: passing the blocked handlers unconditionally made it appear for read-only members and report a lock even on an unlocked table - Enter/F2 now raises the same lock notice as double-click and Space instead of silently doing nothing - guardBatch returns the freshly-read definition so addTableColumnsWithTx asserts live state; a schema lock cleared mid-import no longer fails the createColumns step. The snapshot pre-asserts now only run as the fallback for callers that pass no revalidator - Append-only no longer labels a table whose schema is also locked, which claimed columns were mutable when they weren't - Import dialog withholds Replace on a delete-locked table and create-column on a schema-locked one, instead of offering a configuration that only 423s
1 parent 198ea4d commit 7ca070b

10 files changed

Lines changed: 105 additions & 23 deletions

File tree

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,18 @@ vi.mock('@sim/utils/id', () => ({
3131

3232
vi.mock('@/app/api/table/utils', async () => {
3333
const { NextResponse } = await import('next/server')
34+
const { TableLockedError } = await import('@/lib/table/mutation-locks')
3435
return {
3536
checkAccess: mockCheckAccess,
3637
accessError: (result: { status: number }) => {
3738
const message = result.status === 404 ? 'Table not found' : 'Access denied'
3839
return NextResponse.json({ error: message }, { status: result.status })
3940
},
4041
csvProxyBodyCapResponse: () => null,
42+
tableLockErrorResponse: (error: unknown) =>
43+
error instanceof TableLockedError
44+
? NextResponse.json({ error: error.message, lock: error.lock }, { status: 423 })
45+
: null,
4146
multipartErrorResponse: (error: { code: string; message: string }) =>
4247
NextResponse.json(
4348
{ error: error.message },
@@ -74,6 +79,7 @@ vi.mock('@/lib/table/billing', () => ({
7479
limit >= 0 && current + added > limit,
7580
}))
7681

82+
import { TableLockedError } from '@/lib/table/mutation-locks'
7783
import { POST } from '@/app/api/table/[tableId]/import/route'
7884

7985
function createCsvFile(contents: string, name = 'data.csv', type = 'text/csv'): File {
@@ -377,6 +383,21 @@ describe('POST /api/table/[tableId]/import', () => {
377383
expect(data.data?.insertedCount).toBe(0)
378384
})
379385

386+
it('maps a lock violation from importAppendRows to 423, not 500', async () => {
387+
// The append branch returns instead of rethrowing, so it must map the lock
388+
// error itself — the outer catch's mapper never sees it.
389+
mockImportAppendRows.mockRejectedValueOnce(new TableLockedError('insert'))
390+
const response = await callPost(
391+
createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' })
392+
)
393+
expect(response.status).toBe(423)
394+
const data = await response.json()
395+
expect(data.lock).toBe('insert')
396+
// A `details` array would make the client treat it as a validation error
397+
// and swallow the toast.
398+
expect(data.details).toBeUndefined()
399+
})
400+
380401
it('accepts TSV files', async () => {
381402
const response = await callPost(
382403
createFormData(

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,12 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
337337
},
338338
})
339339
} catch (err) {
340+
// This branch returns rather than rethrowing, so the outer catch's
341+
// mapper is unreachable from here — map the lock error first or a 423
342+
// degrades into a generic 500 (replace mode rethrows and maps fine).
343+
const lockError = tableLockErrorResponse(err)
344+
if (lockError) return lockError
345+
340346
const message = toError(err).message
341347
logger.warn(`[${requestId}] Append failed for table ${tableId}`, {
342348
total: coerced.length,

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table-grid/table-grid.tsx

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2277,8 +2277,15 @@ export function TableGrid({
22772277
}
22782278

22792279
if (e.key === 'Enter' || e.key === 'F2') {
2280-
if (!canEditCellRef.current) return
2280+
if (!canEditRef.current) return
22812281
e.preventDefault()
2282+
// The primary keyboard edit path — same lock notice as double-click and
2283+
// Space, rather than a keypress that silently does nothing.
2284+
if (updateLockedRef.current) {
2285+
onBlockedActionRef.current('edit-cell')
2286+
return
2287+
}
2288+
if (!canEditCellRef.current) return
22822289
const col = cols[anchor.colIndex]
22832290
if (!col) return
22842291

@@ -3773,18 +3780,32 @@ export function TableGrid({
37733780
// the whole menu — and each item should explain the lock
37743781
// rather than silently vanish or fail with a 423 toast.
37753782
onInsertLeft={
3776-
canMutateSchema ? handleInsertColumnLeft : handleBlockedAddColumn
3783+
!userPermissions.canEdit
3784+
? undefined
3785+
: canMutateSchema
3786+
? handleInsertColumnLeft
3787+
: handleBlockedAddColumn
37773788
}
37783789
onInsertRight={
3779-
canMutateSchema ? handleInsertColumnRight : handleBlockedAddColumn
3790+
!userPermissions.canEdit
3791+
? undefined
3792+
: canMutateSchema
3793+
? handleInsertColumnRight
3794+
: handleBlockedAddColumn
37803795
}
37813796
onDeleteColumn={
3782-
canDestroyColumn ? handleDeleteColumn : handleBlockedDeleteColumn
3797+
!userPermissions.canEdit
3798+
? undefined
3799+
: canDestroyColumn
3800+
? handleDeleteColumn
3801+
: handleBlockedDeleteColumn
37833802
}
37843803
onDeleteGroup={
3785-
canDestroyColumn
3786-
? handleDeleteWorkflowGroup
3787-
: handleBlockedDeleteColumn
3804+
!userPermissions.canEdit
3805+
? undefined
3806+
: canDestroyColumn
3807+
? handleDeleteWorkflowGroup
3808+
: handleBlockedDeleteColumn
37883809
}
37893810
onViewWorkflow={
37903811
workflowGroupById.get(g.groupId)?.type === 'enrichment'
@@ -3867,6 +3888,8 @@ export function TableGrid({
38673888
onRenameSubmit={columnRename.submitRename}
38683889
onRenameCancel={columnRename.cancelRename}
38693890
onColumnSelect={handleColumnSelect}
3891+
// Required props here, and the menu is already
3892+
// suppressed for non-editors by `readOnly`.
38703893
onInsertLeft={
38713894
canMutateSchema ? handleInsertColumnLeft : handleBlockedAddColumn
38723895
}

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/lock-copy.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,9 @@ export function describeLocks(locks: TableLocks): { name: string; detail: string
6060
if (locked.length === LOCK_FIELDS.length) {
6161
return { name: 'Read-only', detail: 'no one can change this table’s rows or columns.' }
6262
}
63-
if (!locks.insertLocked && locks.updateLocked && locks.deleteLocked) {
63+
// Only the exact three-lock shape is Append-only — with the schema locked too
64+
// the chip would claim columns are mutable when they aren't.
65+
if (!locks.insertLocked && locks.updateLocked && locks.deleteLocked && !locks.schemaLocked) {
6466
return { name: 'Append-only', detail: 'rows can be added, but not edited or deleted.' }
6567
}
6668
return { name: 'Locked', detail: `${locked.join(', ')} locked.` }

apps/sim/app/workspace/[workspaceId]/tables/components/import-csv-dialog/import-csv-dialog.tsx

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -175,10 +175,16 @@ export function ImportCsvDialog({
175175
resetState()
176176
}
177177

178+
// Replace deletes every existing row and creating columns is a schema change,
179+
// so each needs its own lock clear. Withholding them here keeps the dialog
180+
// from offering a configuration the server can only answer with a 423.
181+
const canReplace = !table.locks?.deleteLocked
182+
const canCreateColumns = !table.locks?.schemaLocked
183+
178184
const columnOptions: ComboboxOption[] = useMemo(() => {
179185
const options: ComboboxOption[] = [
180186
{ label: 'Do not import', value: SKIP_VALUE },
181-
{ label: '+ Create new column', value: CREATE_VALUE },
187+
...(canCreateColumns ? [{ label: '+ Create new column', value: CREATE_VALUE }] : []),
182188
]
183189
for (const col of table.schema.columns) {
184190
options.push({
@@ -187,7 +193,7 @@ export function ImportCsvDialog({
187193
})
188194
}
189195
return options
190-
}, [table.schema.columns])
196+
}, [table.schema.columns, canCreateColumns])
191197

192198
async function handleFileSelected(file: File) {
193199
const ext = file.name.split('.').pop()?.toLowerCase()
@@ -257,6 +263,7 @@ export function ImportCsvDialog({
257263

258264
function handleModeChange(value: string) {
259265
setSubmitError(null)
266+
if (value === 'replace' && !canReplace) return
260267
setMode(value as CsvImportMode)
261268
}
262269

@@ -307,7 +314,8 @@ export function ImportCsvDialog({
307314
async function handleSubmit() {
308315
if (!parsed || !canSubmit) return
309316
setSubmitError(null)
310-
const createColumns = createHeaders.size > 0 ? [...createHeaders] : undefined
317+
const createColumns =
318+
canCreateColumns && createHeaders.size > 0 ? [...createHeaders] : undefined
311319

312320
// Large files can't be POSTed through the server (request-body cap) — upload them
313321
// straight to storage and import in the background instead. Seed the header tray and
@@ -426,14 +434,19 @@ export function ImportCsvDialog({
426434
<ChipModalField type='custom' title='Mode'>
427435
<ButtonGroup value={mode} onValueChange={handleModeChange}>
428436
<ButtonGroupItem value='append'>Append</ButtonGroupItem>
429-
<ButtonGroupItem value='replace'>Replace all rows</ButtonGroupItem>
437+
{canReplace && <ButtonGroupItem value='replace'>Replace all rows</ButtonGroupItem>}
430438
</ButtonGroup>
431439
</ChipModalField>
432440

433441
<ChipModalField type='custom' title='Column mapping'>
434442
{skipCount > 0 && (
435443
<div className='flex justify-end'>
436-
<Button variant='ghost' size='sm' onClick={handleCreateAllUnmapped}>
444+
<Button
445+
variant='ghost'
446+
size='sm'
447+
disabled={!canCreateColumns}
448+
onClick={handleCreateAllUnmapped}
449+
>
437450
Create columns for {skipCount} unmapped
438451
</Button>
439452
</div>

apps/sim/lib/table/delete-runner.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ export async function runTableDelete(payload: TableDeletePayload): Promise<void>
104104
const revalidate = async (trx: DbTransaction) => {
105105
const fresh = await getTableById(tableId, { tx: trx, includeArchived: true })
106106
if (fresh) assertRowDelete(fresh)
107+
return fresh ?? undefined
107108
}
108109

109110
const filterClause = filter

apps/sim/lib/table/import-data.ts

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,9 @@ export async function bulkInsertImportBatch(
6969
/** Re-asserts the insert lock inside the write transaction. See {@link guardBatch}. */
7070
revalidate?: MutationRevalidator
7171
): Promise<{ inserted: number; lastOrderKey: string | null }> {
72-
assertRowInsert(table)
72+
// Superseded by the in-tx revalidation when one is supplied; asserting
73+
// the caller's snapshot too would reject a since-cleared lock.
74+
if (!revalidate) assertRowInsert(table)
7375

7476
for (let i = 0; i < data.rows.length; i++) {
7577
const sizeValidation = validateRowSize(data.rows[i])
@@ -130,7 +132,9 @@ export async function deleteAllTableRows(
130132
/** Re-asserts the delete lock inside the write transaction. See {@link guardBatch}. */
131133
revalidate?: MutationRevalidator
132134
): Promise<void> {
133-
assertRowDelete(table)
135+
// Superseded by the in-tx revalidation when one is supplied; asserting
136+
// the caller's snapshot too would reject a since-cleared lock.
137+
if (!revalidate) assertRowDelete(table)
134138
await db.transaction(async (trx) => {
135139
await guardBatch(trx, table.id, revalidate)
136140
await trx.delete(userTableRows).where(eq(userTableRows.tableId, table.id))
@@ -150,8 +154,11 @@ export async function addImportColumns(
150154
revalidate?: MutationRevalidator
151155
): Promise<TableDefinition> {
152156
const updated = await db.transaction(async (trx) => {
153-
await guardBatch(trx, table.id, revalidate)
154-
return addTableColumnsWithTx(trx, table, additions, requestId)
157+
// `addTableColumnsWithTx` re-asserts the schema lock, so hand it the
158+
// freshly-read definition — asserting the caller's snapshot would reject a
159+
// lock that has since been cleared.
160+
const fresh = await guardBatch(trx, table.id, revalidate)
161+
return addTableColumnsWithTx(trx, fresh ?? table, additions, requestId)
155162
})
156163
auditTableColumnsAdded(
157164
table,
@@ -168,7 +175,9 @@ export async function setTableSchemaForImport(
168175
/** Re-asserts the schema lock inside the write transaction. See {@link guardBatch}. */
169176
revalidate?: MutationRevalidator
170177
): Promise<void> {
171-
assertSchemaMutable(table)
178+
// Superseded by the in-tx revalidation when one is supplied; asserting
179+
// the caller's snapshot too would reject a since-cleared lock.
180+
if (!revalidate) assertSchemaMutable(table)
172181
await db.transaction(async (trx) => {
173182
await guardBatch(trx, table.id, revalidate)
174183
await trx

apps/sim/lib/table/import-runner.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,16 +114,19 @@ export async function runTableImport(payload: TableImportPayload): Promise<void>
114114
const revalidateInsert = async (trx: DbTransaction) => {
115115
const fresh = await getTableById(tableId, { tx: trx, includeArchived: true })
116116
if (fresh) assertRowInsert(fresh)
117+
return fresh ?? undefined
117118
}
118119
/** Same guard for the replace-mode wipe, which lands before the first batch. */
119120
const revalidateDelete = async (trx: DbTransaction) => {
120121
const fresh = await getTableById(tableId, { tx: trx, includeArchived: true })
121122
if (fresh) assertRowDelete(fresh)
123+
return fresh ?? undefined
122124
}
123125
/** Same guard for the inferred-schema write and `createColumns`. */
124126
const revalidateSchema = async (trx: DbTransaction) => {
125127
const fresh = await getTableById(tableId, { tx: trx, includeArchived: true })
126128
if (fresh) assertSchemaMutable(fresh)
129+
return fresh ?? undefined
127130
}
128131

129132
// Total byte size for the progress estimate — a cheap HEAD, no download. May be null on

apps/sim/lib/table/rows/ordering.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import type { MutationProof } from '@/lib/table/mutation-locks'
1515
import { keyBetween, nKeysBetween } from '@/lib/table/order-key'
1616
import { type DbExecutor, type DbTransaction, withSeqscanOff } from '@/lib/table/planner'
1717
import { setTableTxTimeouts } from '@/lib/table/tx'
18-
import type { RowData } from '@/lib/table/types'
18+
import type { RowData, TableDefinition } from '@/lib/table/types'
1919

2020
/**
2121
* Starting `position` for an append import — `max(position) + 1`, or 0 when empty. Read once,
@@ -399,7 +399,7 @@ export async function selectRowDataPage(params: {
399399
* Re-verifies the table's locks from inside a write transaction. Supplied by
400400
* the background runners; see {@link deletePageByIds}.
401401
*/
402-
export type MutationRevalidator = (trx: DbTransaction) => Promise<void>
402+
export type MutationRevalidator = (trx: DbTransaction) => Promise<TableDefinition | undefined>
403403

404404
/**
405405
* Takes the table's schema advisory lock and runs `revalidate` inside the
@@ -408,17 +408,20 @@ export type MutationRevalidator = (trx: DbTransaction) => Promise<void>
408408
* a lock committed before this call is seen and throws; one committed after it
409409
* waits for this batch to finish. Without it, the caller's proof would only
410410
* describe the lock state at some earlier point in the run.
411+
*
412+
* Returns the freshly-read definition so tx-bound helpers can act on live state
413+
* instead of the caller's snapshot.
411414
*/
412415
export async function guardBatch(
413416
trx: DbTransaction,
414417
tableId: string,
415418
revalidate: MutationRevalidator | undefined
416-
): Promise<void> {
417-
if (!revalidate) return
419+
): Promise<TableDefinition | undefined> {
420+
if (!revalidate) return undefined
418421
await trx.execute(
419422
sql`SELECT pg_advisory_xact_lock(hashtextextended(${`user_table_schema:${tableId}`}, 0))`
420423
)
421-
await revalidate(trx)
424+
return revalidate(trx)
422425
}
423426

424427
/**

apps/sim/lib/table/update-runner.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ export async function runTableUpdate(payload: TableUpdatePayload): Promise<void>
112112
const revalidate = async (trx: DbTransaction) => {
113113
const fresh = await getTableById(tableId, { tx: trx, includeArchived: true })
114114
if (fresh) assertRowUpdate(fresh, patchColumnIds(data))
115+
return fresh ?? undefined
115116
}
116117

117118
const filterClause = buildFilterClause(filter, USER_TABLE_ROWS_SQL_NAME, table.schema.columns)

0 commit comments

Comments
 (0)