Skip to content

Commit f5df1cb

Browse files
committed
fix(tables): scope and verify delete triggers
1 parent c2b789a commit f5df1cb

10 files changed

Lines changed: 147 additions & 6 deletions

File tree

apps/sim/background/cleanup-table-row-ttl.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ describe('table row TTL cleanup', () => {
108108
expect(mockFireTableTrigger).toHaveBeenNthCalledWith(
109109
1,
110110
table.id,
111+
table.workspaceId,
111112
table.name,
112113
'delete',
113114
deletedRows(500),

apps/sim/background/cleanup-table-row-ttl.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,7 @@ async function deleteExpiredRowsForTable(
207207
if (batch.attempted && batch.rows.length > 0) {
208208
await fireTableTrigger(
209209
ref.id,
210+
ref.workspaceId,
210211
batch.tableName,
211212
'delete',
212213
batch.rows,

apps/sim/lib/table/__tests__/service-filter-threading.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,7 @@ describe('delete trigger dispatch', () => {
200200

201201
expect(mockFireTableTrigger).toHaveBeenCalledWith(
202202
TABLE.id,
203+
TABLE.workspaceId,
203204
TABLE.name,
204205
'delete',
205206
[{ id: 'row-1', data: { name: 'Ada' } }],
@@ -223,6 +224,7 @@ describe('delete trigger dispatch', () => {
223224

224225
expect(mockFireTableTrigger).toHaveBeenCalledWith(
225226
TABLE.id,
227+
TABLE.workspaceId,
226228
TABLE.name,
227229
'delete',
228230
[
@@ -254,8 +256,8 @@ describe('delete trigger dispatch', () => {
254256
}
255257

256258
expect(mockFireTableTrigger).toHaveBeenCalledTimes(2)
257-
expect(mockFireTableTrigger.mock.calls[0][3]).toEqual([{ id: 'row-1', data: { name: 'Ada' } }])
258-
expect(mockFireTableTrigger.mock.calls[1][3]).toEqual([
259+
expect(mockFireTableTrigger.mock.calls[0][4]).toEqual([{ id: 'row-1', data: { name: 'Ada' } }])
260+
expect(mockFireTableTrigger.mock.calls[1][4]).toEqual([
259261
{ id: 'row-2', data: { name: 'Grace' } },
260262
])
261263
})

apps/sim/lib/table/__tests__/trigger.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ interface Payload {
6060
function webhookEntry(config: Record<string, unknown> = {}) {
6161
return {
6262
webhook: { id: 'wh_1', providerConfig: { tableId: 'tbl_1', eventType: 'insert', ...config } },
63-
workflow: { id: 'wf_1' },
63+
workflow: { id: 'wf_1', workspaceId: 'ws_1' },
6464
}
6565
}
6666

@@ -75,6 +75,7 @@ async function fire(
7575
) {
7676
await fireTableTrigger(
7777
'tbl_1',
78+
'ws_1',
7879
'Issues',
7980
eventType,
8081
[{ id: 'row_1', data } as never],
@@ -193,6 +194,14 @@ describe('fireTableTrigger — gating', () => {
193194
expect(mockProcessPolledWebhookEvent).not.toHaveBeenCalled()
194195
})
195196

197+
it('fires nothing for a workflow in a different workspace', async () => {
198+
mockFetchActiveWebhooks.mockResolvedValue([
199+
{ ...webhookEntry(), workflow: { id: 'wf_other', workspaceId: 'ws_other' } },
200+
])
201+
await fire('insert', { col_title: 'x' })
202+
expect(mockProcessPolledWebhookEvent).not.toHaveBeenCalled()
203+
})
204+
196205
it('fires nothing when the event type does not match', async () => {
197206
mockFetchActiveWebhooks.mockResolvedValue([webhookEntry({ eventType: 'update' })])
198207
await fire('insert', { col_title: 'x' })

apps/sim/lib/table/constants.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ export const TABLE_LIMITS = {
4444
* post-commit trigger dispatch. The row-count cap is derived from this budget
4545
* and the configured maximum row size before a DELETE materializes snapshots.
4646
*/
47-
DELETE_SNAPSHOT_BATCH_MAX_BYTES: 32 * 1024 * 1024, // 32MB
47+
DELETE_SNAPSHOT_BATCH_MAX_BYTES: 32 * 1024 * 1024,
4848
/** Maximum rows per batch insert */
4949
MAX_BATCH_INSERT_SIZE: 1000,
5050
/** Maximum rows per bulk update/delete operation */

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,7 @@ describe('runTableDelete', () => {
196196
expect(mockFireTableTrigger).toHaveBeenNthCalledWith(
197197
1,
198198
'tbl_1',
199+
'ws_1',
199200
'Issues',
200201
'delete',
201202
[
@@ -230,6 +231,7 @@ describe('runTableDelete', () => {
230231

231232
expect(mockFireTableTrigger).toHaveBeenCalledWith(
232233
renamedTable.id,
234+
renamedTable.workspaceId,
233235
renamedTable.name,
234236
'delete',
235237
[{ id: 'a', data: { 'col-title': 'a' } }],

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ export async function runTableDelete(payload: TableDeletePayload): Promise<void>
130130
const triggerTable = committedTable ?? table
131131
await fireTableTrigger(
132132
triggerTable.id,
133+
triggerTable.workspaceId,
133134
triggerTable.name,
134135
'delete',
135136
rows,
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { databaseMock } from '@sim/testing'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
import type { MutationProof } from '@/lib/table/mutation-locks'
7+
import type { DbTransaction } from '@/lib/table/planner'
8+
9+
vi.mock('@/lib/table/constants', () => ({
10+
getDeleteSnapshotBatchSize: () => 1,
11+
TABLE_LIMITS: { UPDATE_BATCH_SIZE: 100 },
12+
}))
13+
vi.mock('@/lib/table/tx', () => ({ setTableTxTimeouts: vi.fn() }))
14+
15+
import {
16+
type DeletedRowsHandler,
17+
deleteOrderedRowsByIds,
18+
deletePageByIds,
19+
} from '@/lib/table/rows/ordering'
20+
21+
const mockTransaction = databaseMock.db.transaction as ReturnType<typeof vi.fn>
22+
const proof = {} as MutationProof<'delete'>
23+
24+
type DeleteRunner = (onDeleted: DeletedRowsHandler) => Promise<unknown>
25+
26+
describe('ordered row delete trigger handoff', () => {
27+
beforeEach(() => {
28+
vi.clearAllMocks()
29+
})
30+
31+
it.each([
32+
[
33+
'direct deletes',
34+
(onDeleted: DeletedRowsHandler) =>
35+
deleteOrderedRowsByIds({
36+
tableId: 'table-1',
37+
workspaceId: 'workspace-1',
38+
rowIds: ['row-1', 'row-2'],
39+
proof,
40+
onDeleted,
41+
}),
42+
],
43+
[
44+
'background delete pages',
45+
(onDeleted: DeletedRowsHandler) =>
46+
deletePageByIds('table-1', 'workspace-1', ['row-1', 'row-2'], proof, undefined, onDeleted),
47+
],
48+
])(
49+
'runs %s handlers after commit and before the next batch',
50+
async (_label, run: DeleteRunner) => {
51+
const events: string[] = []
52+
let batchIndex = 0
53+
let releaseFirstHandler: (() => void) | undefined
54+
const firstHandlerGate = new Promise<void>((resolve) => {
55+
releaseFirstHandler = resolve
56+
})
57+
const trx = {
58+
delete: () => ({
59+
where: () => ({
60+
returning: async () => {
61+
const id = `row-${batchIndex + 1}`
62+
batchIndex++
63+
return [{ id, data: { title: id } }]
64+
},
65+
}),
66+
}),
67+
} as unknown as DbTransaction
68+
69+
mockTransaction.mockImplementation(
70+
async (callback: (transaction: DbTransaction) => Promise<unknown>) => {
71+
const result = await callback(trx)
72+
events.push(`commit-${mockTransaction.mock.calls.length}`)
73+
return result
74+
}
75+
)
76+
77+
const onDeleted = vi.fn(async (rows: Array<{ id: string }>) => {
78+
events.push(`trigger-${rows[0]?.id}`)
79+
if (rows[0]?.id === 'row-1') await firstHandlerGate
80+
})
81+
const pending = run(onDeleted)
82+
83+
await vi.waitFor(() => {
84+
expect(events).toEqual(['commit-1', 'trigger-row-1'])
85+
})
86+
expect(mockTransaction).toHaveBeenCalledTimes(1)
87+
88+
releaseFirstHandler?.()
89+
await pending
90+
91+
expect(events).toEqual(['commit-1', 'trigger-row-1', 'commit-2', 'trigger-row-2'])
92+
expect(onDeleted.mock.calls.map(([rows]) => rows)).toEqual([
93+
[{ id: 'row-1', data: { title: 'row-1' } }],
94+
[{ id: 'row-2', data: { title: 'row-2' } }],
95+
])
96+
}
97+
)
98+
})

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

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,16 @@ async function dispatchDeleteTriggers(
117117
requestId: string
118118
): Promise<void> {
119119
if (deletedRows.length === 0) return
120-
await fireTableTrigger(table.id, table.name, 'delete', deletedRows, null, table.schema, requestId)
120+
await fireTableTrigger(
121+
table.id,
122+
table.workspaceId,
123+
table.name,
124+
'delete',
125+
deletedRows,
126+
null,
127+
table.schema,
128+
requestId
129+
)
121130
}
122131

123132
/**
@@ -217,6 +226,7 @@ export async function insertRow(
217226

218227
void fireTableTrigger(
219228
data.tableId,
229+
table.workspaceId,
220230
table.name,
221231
'insert',
222232
[insertedRow],
@@ -393,7 +403,16 @@ export function dispatchAfterBatchInsert(
393403
requestId: string,
394404
actorUserId?: string | null
395405
): void {
396-
void fireTableTrigger(table.id, table.name, 'insert', result, null, table.schema, requestId)
406+
void fireTableTrigger(
407+
table.id,
408+
table.workspaceId,
409+
table.name,
410+
'insert',
411+
result,
412+
null,
413+
table.schema,
414+
requestId
415+
)
397416
// Scope to the newly-inserted row ids so the dispatcher doesn't walk every
398417
// row in the table. After the sidecar migration, all existing rows have
399418
// zero entries → `mode:'new'`'s `NOT EXISTS` filter would otherwise include
@@ -875,6 +894,7 @@ export async function upsertRow(
875894
})
876895
void fireTableTrigger(
877896
data.tableId,
897+
table.workspaceId,
878898
table.name,
879899
'insert',
880900
[result.row],
@@ -886,6 +906,7 @@ export async function upsertRow(
886906
const oldRows = new Map([[result.row.id, result.previousData]])
887907
void fireTableTrigger(
888908
data.tableId,
909+
table.workspaceId,
889910
table.name,
890911
'update',
891912
[result.row],
@@ -1811,6 +1832,7 @@ export async function updateRow(
18111832
const oldRows = new Map([[data.rowId, existingRow.data as RowData]])
18121833
void fireTableTrigger(
18131834
data.tableId,
1835+
table.workspaceId,
18141836
table.name,
18151837
'update',
18161838
[updatedRow],
@@ -2072,6 +2094,7 @@ function dispatchBulkUpdateEffects(
20722094
}))
20732095
void fireTableTrigger(
20742096
table.id,
2097+
table.workspaceId,
20752098
table.name,
20762099
'update',
20772100
updatedRows,
@@ -2494,6 +2517,7 @@ export async function batchUpdateRows(
24942517
if (updatedRowsForTrigger.length > 0) {
24952518
void fireTableTrigger(
24962519
data.tableId,
2520+
table.workspaceId,
24972521
table.name,
24982522
'update',
24992523
updatedRowsForTrigger,

apps/sim/lib/table/trigger.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,14 @@ interface WebhookConfig {
4545
* This is fire-and-forget - errors are logged but never thrown.
4646
* Call with `void fireTableTrigger(...)` to avoid blocking the caller.
4747
*
48+
* @param workspaceId - Canonical workspace that owns the mutated table.
4849
* @param eventType - The committed row mutation that should trigger workflows.
4950
* @param rows - Committed row snapshots; only the ID and data are needed, including for deletes.
5051
* @param oldRows - Map of row ID to previous data. Pass null for inserts and deletes.
5152
*/
5253
export async function fireTableTrigger(
5354
tableId: string,
55+
workspaceId: string,
5456
tableName: string,
5557
eventType: EventType,
5658
rows: TableTriggerRow[],
@@ -75,6 +77,7 @@ export async function fireTableTrigger(
7577

7678
// Filter to webhooks watching this table with a matching event type
7779
const matching = webhooks.filter((entry) => {
80+
if (entry.workflow.workspaceId !== workspaceId) return false
7881
const config = entry.webhook.providerConfig as WebhookConfig | null
7982
// Canonical key `tableId` first; `tableSelector`/`manualTableId` are a transitional
8083
// basic-first fallback for configs deployed before the canonical key was written.

0 commit comments

Comments
 (0)