Skip to content

Commit 569f89f

Browse files
fix(tables): stop auditing no-op deletes on v1 and v2
deleteTable records TABLE_DELETED itself, but only when a row was actually archived and an actingUserId was passed — omitting the actor is how rollback callers opt out. v1 and v2 called it without the actor and then hand-rolled recordAudit outside the `if (deleted)` check, so deleting an already-archived table emitted a TABLE_DELETED event for a delete that did nothing. The UI and copilot paths pass the actor and behave correctly. Both now pass the actor and drop the local audit. v2 also gains lock handling, so a delete-locked table returns 423 instead of a 500. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 4d15378 commit 569f89f

3 files changed

Lines changed: 110 additions & 27 deletions

File tree

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

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
21
import { createLogger } from '@sim/logger'
32
import { type NextRequest, NextResponse } from 'next/server'
43
import { v1DeleteTableContract, v1GetTableContract } from '@/lib/api/contracts/v1/tables'
@@ -139,18 +138,10 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab
139138
return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 })
140139
}
141140

142-
await deleteTable(tableId, requestId)
143-
144-
recordAudit({
145-
workspaceId,
146-
actorId: userId,
147-
action: AuditAction.TABLE_DELETED,
148-
resourceType: AuditResourceType.TABLE,
149-
resourceId: tableId,
150-
resourceName: result.table.name,
151-
description: `Archived table "${result.table.name}"`,
152-
request,
153-
})
141+
// The actor makes the service audit the delete — and only when a row was
142+
// actually archived. Auditing out here emitted TABLE_DELETED for a no-op
143+
// delete of an already-archived table.
144+
await deleteTable(tableId, requestId, userId)
154145

155146
return NextResponse.json({
156147
success: true,
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Public v2 table delete: the actor is handed to the service so the audit is
5+
* emitted there — and only for a delete that actually archived a row.
6+
*/
7+
import { NextRequest } from 'next/server'
8+
import { beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
const {
11+
mockCheckRateLimit,
12+
mockResolveWorkspaceScope,
13+
mockCheckAccess,
14+
mockDeleteTable,
15+
mockRecordAudit,
16+
} = vi.hoisted(() => ({
17+
mockCheckRateLimit: vi.fn(),
18+
mockResolveWorkspaceScope: vi.fn(),
19+
mockCheckAccess: vi.fn(),
20+
mockDeleteTable: vi.fn(),
21+
mockRecordAudit: vi.fn(),
22+
}))
23+
24+
vi.mock('@sim/audit', () => ({
25+
AuditAction: { TABLE_DELETED: 'table.deleted', TABLE_UPDATED: 'table.updated' },
26+
AuditResourceType: { TABLE: 'table' },
27+
recordAudit: mockRecordAudit,
28+
}))
29+
30+
vi.mock('@/app/api/v1/middleware', () => ({
31+
checkRateLimit: mockCheckRateLimit,
32+
resolveWorkspaceScope: mockResolveWorkspaceScope,
33+
}))
34+
35+
vi.mock('@/app/api/table/utils', () => ({
36+
checkAccess: mockCheckAccess,
37+
normalizeColumn: (col: Record<string, unknown>) => col,
38+
rootErrorMessage: (error: unknown) => String(error),
39+
rowWriteErrorResponse: () => null,
40+
}))
41+
42+
vi.mock('@/lib/table', async () => {
43+
const actual = await import('@/lib/table/column-keys')
44+
return { ...actual, deleteTable: mockDeleteTable, updateTable: vi.fn(), getTableById: vi.fn() }
45+
})
46+
47+
vi.mock('@/app/api/v2/lib/gate', () => ({
48+
v2ApiGateError: vi.fn().mockResolvedValue(null),
49+
}))
50+
51+
import { TableLockedError } from '@/lib/table/mutation-locks'
52+
import { DELETE } from '@/app/api/v2/tables/[tableId]/route'
53+
54+
const TABLE = { id: 'table-1', name: 'Tasks', workspaceId: 'ws-1', schema: { columns: [] } }
55+
56+
function callDelete() {
57+
const req = new NextRequest('http://localhost:3000/api/v2/tables/table-1?workspaceId=ws-1', {
58+
method: 'DELETE',
59+
})
60+
return DELETE(req, { params: Promise.resolve({ tableId: 'table-1' }) })
61+
}
62+
63+
describe('DELETE /api/v2/tables/[tableId]', () => {
64+
beforeEach(() => {
65+
vi.clearAllMocks()
66+
mockCheckRateLimit.mockResolvedValue({
67+
allowed: true,
68+
userId: 'user-1',
69+
keyType: 'workspace',
70+
workspaceId: 'ws-1',
71+
limit: 100,
72+
remaining: 99,
73+
resetAt: new Date('2026-01-01T01:00:00Z'),
74+
})
75+
mockResolveWorkspaceScope.mockResolvedValue(null)
76+
mockCheckAccess.mockResolvedValue({ ok: true, table: TABLE })
77+
})
78+
79+
it('passes the actor so the service owns the audit', async () => {
80+
mockDeleteTable.mockResolvedValue(undefined)
81+
82+
const res = await callDelete()
83+
84+
expect(res.status).toBe(200)
85+
expect(mockDeleteTable).toHaveBeenCalledWith('table-1', expect.any(String), 'user-1')
86+
// The route no longer audits: doing so out here fired TABLE_DELETED even
87+
// when the delete was a no-op on an already-archived table.
88+
expect(mockRecordAudit).not.toHaveBeenCalled()
89+
})
90+
91+
it('returns 423 LOCKED for a delete-locked table instead of a 500', async () => {
92+
mockDeleteTable.mockRejectedValue(new TableLockedError('delete'))
93+
94+
const res = await callDelete()
95+
96+
expect(res.status).toBe(423)
97+
expect((await res.json()).error.code).toBe('LOCKED')
98+
})
99+
})

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

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
21
import { createLogger } from '@sim/logger'
32
import { getErrorMessage } from '@sim/utils/errors'
43
import type { NextRequest } from 'next/server'
@@ -17,7 +16,7 @@ import {
1716
v2ValidationError,
1817
v2WorkspaceAccessError,
1918
} from '@/app/api/v2/lib/response'
20-
import { toApiTable, v2TableAccessError } from '@/app/api/v2/tables/utils'
19+
import { toApiTable, v2TableAccessError, v2TableLockError } from '@/app/api/v2/tables/utils'
2120

2221
const logger = createLogger('V2TableDetailAPI')
2322

@@ -100,21 +99,15 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Tab
10099
return v2Error('NOT_FOUND', 'Table not found')
101100
}
102101

103-
await deleteTable(tableId, requestId)
104-
105-
recordAudit({
106-
workspaceId,
107-
actorId: userId,
108-
action: AuditAction.TABLE_DELETED,
109-
resourceType: AuditResourceType.TABLE,
110-
resourceId: tableId,
111-
resourceName: result.table.name,
112-
description: `Archived table "${result.table.name}"`,
113-
request,
114-
})
102+
// The actor makes the service audit the delete — and only when a row was
103+
// actually archived. Auditing out here emitted TABLE_DELETED for a no-op
104+
// delete of an already-archived table.
105+
await deleteTable(tableId, requestId, userId)
115106

116107
return v2Data({ id: tableId }, { rateLimit })
117108
} catch (error) {
109+
const lockError = v2TableLockError(error)
110+
if (lockError) return lockError
118111
logger.error(`[${requestId}] Error deleting table`, {
119112
error: getErrorMessage(error, 'Unknown error'),
120113
})

0 commit comments

Comments
 (0)