Skip to content

Commit a0da8e2

Browse files
committed
improvement(table): tighten filter-cast types & workspace guards
- Drop redundant tableId/workspaceId from BulkUpdateData and BulkDeleteData; service uses table.id / table.workspaceId so column metadata and DB scope can't drift apart. - Add missing workspace-id guards to copilot user-table cases (insert_row, batch_insert_rows, update_row, batch_update_rows, rename); collapse duplicated rename check. - Add service-level integration tests that buildFilterClause/buildSortClause receive table.schema.columns from queryRows, updateRowsByFilter, deleteRowsByFilter.
1 parent 0af4891 commit a0da8e2

6 files changed

Lines changed: 139 additions & 35 deletions

File tree

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -395,11 +395,9 @@ export const PUT = withRouteHandler(
395395
const result = await updateRowsByFilter(
396396
table,
397397
{
398-
tableId,
399398
filter: validated.filter as Filter,
400399
data: validated.data as RowData,
401400
limit: validated.limit,
402-
workspaceId: validated.workspaceId,
403401
},
404402
requestId
405403
)
@@ -510,10 +508,8 @@ export const DELETE = withRouteHandler(
510508
const result = await deleteRowsByFilter(
511509
table,
512510
{
513-
tableId,
514511
filter: validated.filter as Filter,
515512
limit: validated.limit,
516-
workspaceId: validated.workspaceId,
517513
},
518514
requestId
519515
)

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -385,11 +385,9 @@ export const PUT = withRouteHandler(async (request: NextRequest, context: TableR
385385
const result = await updateRowsByFilter(
386386
table,
387387
{
388-
tableId,
389388
filter: validated.filter as Filter,
390389
data: validated.data as RowData,
391390
limit: validated.limit,
392-
workspaceId: validated.workspaceId,
393391
},
394392
requestId
395393
)
@@ -491,10 +489,8 @@ export const DELETE = withRouteHandler(
491489
const result = await deleteRowsByFilter(
492490
table,
493491
{
494-
tableId,
495492
filter: validated.filter as Filter,
496493
limit: validated.limit,
497-
workspaceId: validated.workspaceId,
498494
},
499495
requestId
500496
)

apps/sim/lib/copilot/tools/server/table/user-table.ts

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
367367
}
368368

369369
const table = await getTableById(args.tableId)
370-
if (!table) {
370+
if (!table || table.workspaceId !== workspaceId) {
371371
return { success: false, message: `Table not found: ${args.tableId}` }
372372
}
373373

@@ -418,7 +418,7 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
418418
}
419419

420420
const table = await getTableById(args.tableId)
421-
if (!table) {
421+
if (!table || table.workspaceId !== workspaceId) {
422422
return { success: false, message: `Table not found: ${args.tableId}` }
423423
}
424424

@@ -513,7 +513,7 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
513513
}
514514

515515
const table = await getTableById(args.tableId)
516-
if (!table) {
516+
if (!table || table.workspaceId !== workspaceId) {
517517
return { success: false, message: `Table not found: ${args.tableId}` }
518518
}
519519

@@ -582,11 +582,9 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
582582
const result = await updateRowsByFilter(
583583
table,
584584
{
585-
tableId: args.tableId,
586585
filter: args.filter,
587586
data: args.data,
588587
limit: args.limit,
589-
workspaceId,
590588
},
591589
requestId
592590
)
@@ -619,10 +617,8 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
619617
const result = await deleteRowsByFilter(
620618
table,
621619
{
622-
tableId: args.tableId,
623620
filter: args.filter,
624621
limit: args.limit,
625-
workspaceId,
626622
},
627623
requestId
628624
)
@@ -674,7 +670,7 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
674670
}
675671

676672
const table = await getTableById(args.tableId)
677-
if (!table) {
673+
if (!table || table.workspaceId !== workspaceId) {
678674
return { success: false, message: `Table not found: ${args.tableId}` }
679675
}
680676

@@ -1099,12 +1095,9 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
10991095
}
11001096

11011097
const table = await getTableById(args.tableId)
1102-
if (!table) {
1098+
if (!table || table.workspaceId !== workspaceId) {
11031099
return { success: false, message: `Table not found: ${args.tableId}` }
11041100
}
1105-
if (table.workspaceId !== workspaceId) {
1106-
return { success: false, message: 'Table not found' }
1107-
}
11081101

11091102
const requestId = generateId().slice(0, 8)
11101103
assertNotAborted()
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Integration test asserting that `table.schema.columns` is forwarded to
5+
* `buildFilterClause` from each service function that filters rows. This
6+
* guards the contract that type-aware JSONB casts (numeric for numbers,
7+
* timestamp for dates) are always available at the SQL builder layer — the
8+
* latent bug that PR #4657 was originally fixing.
9+
*/
10+
import { dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing'
11+
import { sql } from 'drizzle-orm'
12+
import { beforeEach, describe, expect, it, vi } from 'vitest'
13+
import { buildFilterClause, buildSortClause } from '@/lib/table/sql'
14+
import type { ColumnDefinition, TableDefinition } from '@/lib/table/types'
15+
16+
vi.mock('@sim/db', () => dbChainMock)
17+
18+
vi.mock('@/lib/table/sql', () => ({
19+
buildFilterClause: vi.fn(() => sql`true`),
20+
buildSortClause: vi.fn(() => sql`true`),
21+
}))
22+
23+
vi.mock('@/lib/table/trigger', () => ({
24+
fireTableTrigger: vi.fn(),
25+
}))
26+
27+
vi.mock('@/lib/table/workflow-columns', () => ({
28+
assertValidSchema: vi.fn(),
29+
scheduleRunsForRows: vi.fn(),
30+
scheduleRunsForTable: vi.fn(),
31+
stripGroupDeps: vi.fn(),
32+
}))
33+
34+
vi.mock('@/lib/table/validation', () => ({
35+
validateRowSize: vi.fn(() => ({ valid: true, errors: [] })),
36+
validateRowAgainstSchema: vi.fn(() => ({ valid: true, errors: [] })),
37+
validateTableName: vi.fn(() => ({ valid: true, errors: [] })),
38+
validateTableSchema: vi.fn(() => ({ valid: true, errors: [] })),
39+
getUniqueColumns: vi.fn(() => []),
40+
checkUniqueConstraintsDb: vi.fn(async () => ({ valid: true, errors: [] })),
41+
checkBatchUniqueConstraintsDb: vi.fn(async () => ({ valid: true, errors: [] })),
42+
}))
43+
44+
import { deleteRowsByFilter, queryRows, updateRowsByFilter } from '@/lib/table/service'
45+
46+
const COLUMNS: ColumnDefinition[] = [
47+
{ name: 'name', type: 'string' },
48+
{ name: 'birthDate', type: 'date' },
49+
{ name: 'score', type: 'number' },
50+
]
51+
52+
const TABLE: TableDefinition = {
53+
id: 'tbl-1',
54+
name: 'People',
55+
description: null,
56+
schema: { columns: COLUMNS },
57+
metadata: null,
58+
rowCount: 0,
59+
maxRows: 1000,
60+
workspaceId: 'ws-1',
61+
createdBy: 'user-1',
62+
archivedAt: null,
63+
createdAt: new Date('2024-01-01'),
64+
updatedAt: new Date('2024-01-01'),
65+
}
66+
67+
describe('service filter threading', () => {
68+
beforeEach(() => {
69+
vi.clearAllMocks()
70+
resetDbChainMock()
71+
})
72+
73+
it('queryRows forwards table.schema.columns to buildFilterClause', async () => {
74+
await queryRows(
75+
TABLE,
76+
{ filter: { birthDate: { $gte: '2024-01-01' } }, includeTotal: false },
77+
'req-1'
78+
).catch(() => {})
79+
80+
expect(buildFilterClause).toHaveBeenCalledTimes(1)
81+
expect(buildFilterClause).toHaveBeenCalledWith(
82+
{ birthDate: { $gte: '2024-01-01' } },
83+
expect.any(String),
84+
COLUMNS
85+
)
86+
})
87+
88+
it('queryRows forwards columns to buildSortClause as well', async () => {
89+
await queryRows(TABLE, { sort: { birthDate: 'asc' }, includeTotal: false }, 'req-1').catch(
90+
() => {}
91+
)
92+
93+
expect(buildSortClause).toHaveBeenCalledWith({ birthDate: 'asc' }, expect.any(String), COLUMNS)
94+
})
95+
96+
it('updateRowsByFilter forwards table.schema.columns to buildFilterClause', async () => {
97+
dbChainMockFns.where.mockResolvedValueOnce([])
98+
await updateRowsByFilter(
99+
TABLE,
100+
{ filter: { birthDate: { $lt: '2024-06-01' } }, data: { name: 'x' } },
101+
'req-1'
102+
)
103+
104+
expect(buildFilterClause).toHaveBeenCalledTimes(1)
105+
expect(buildFilterClause).toHaveBeenCalledWith(
106+
{ birthDate: { $lt: '2024-06-01' } },
107+
expect.any(String),
108+
COLUMNS
109+
)
110+
})
111+
112+
it('deleteRowsByFilter forwards table.schema.columns to buildFilterClause', async () => {
113+
dbChainMockFns.where.mockResolvedValueOnce([])
114+
await deleteRowsByFilter(TABLE, { filter: { score: { $gt: 90 } } }, 'req-1')
115+
116+
expect(buildFilterClause).toHaveBeenCalledTimes(1)
117+
expect(buildFilterClause).toHaveBeenCalledWith(
118+
{ score: { $gt: 90 } },
119+
expect.any(String),
120+
COLUMNS
121+
)
122+
})
123+
})

apps/sim/lib/table/service.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1823,8 +1823,8 @@ export async function updateRowsByFilter(
18231823
}
18241824

18251825
const baseConditions = and(
1826-
eq(userTableRows.tableId, data.tableId),
1827-
eq(userTableRows.workspaceId, data.workspaceId)
1826+
eq(userTableRows.tableId, table.id),
1827+
eq(userTableRows.workspaceId, table.workspaceId)
18281828
)
18291829

18301830
let query = db
@@ -1872,7 +1872,7 @@ export async function updateRowsByFilter(
18721872
const row = matchingRows[0]
18731873
const mergedData = { ...(row.data as RowData), ...data.data }
18741874
const uniqueValidation = await checkUniqueConstraintsDb(
1875-
data.tableId,
1875+
table.id,
18761876
mergedData,
18771877
table.schema,
18781878
row.id
@@ -1900,7 +1900,7 @@ export async function updateRowsByFilter(
19001900
}
19011901
})
19021902

1903-
logger.info(`[${requestId}] Updated ${matchingRows.length} rows in table ${data.tableId}`)
1903+
logger.info(`[${requestId}] Updated ${matchingRows.length} rows in table ${table.id}`)
19041904

19051905
const oldRows = new Map(matchingRows.map((r) => [r.id, r.data as RowData]))
19061906
const updatedRows: TableRow[] = matchingRows.map((r) => ({
@@ -1912,7 +1912,7 @@ export async function updateRowsByFilter(
19121912
updatedAt: now,
19131913
}))
19141914
void fireTableTrigger(
1915-
data.tableId,
1915+
table.id,
19161916
table.name,
19171917
'update',
19181918
updatedRows,
@@ -2137,8 +2137,8 @@ export async function deleteRowsByFilter(
21372137

21382138
// Find matching rows
21392139
const baseConditions = and(
2140-
eq(userTableRows.tableId, data.tableId),
2141-
eq(userTableRows.workspaceId, data.workspaceId)
2140+
eq(userTableRows.tableId, table.id),
2141+
eq(userTableRows.workspaceId, table.workspaceId)
21422142
)
21432143

21442144
let query = db
@@ -2168,8 +2168,8 @@ export async function deleteRowsByFilter(
21682168
const batch = rowIds.slice(i, i + TABLE_LIMITS.DELETE_BATCH_SIZE)
21692169
await trx.delete(userTableRows).where(
21702170
and(
2171-
eq(userTableRows.tableId, data.tableId),
2172-
eq(userTableRows.workspaceId, data.workspaceId),
2171+
eq(userTableRows.tableId, table.id),
2172+
eq(userTableRows.workspaceId, table.workspaceId),
21732173
sql`${userTableRows.id} = ANY(ARRAY[${sql.join(
21742174
batch.map((id) => sql`${id}`),
21752175
sql`, `
@@ -2178,10 +2178,10 @@ export async function deleteRowsByFilter(
21782178
)
21792179
}
21802180

2181-
await recompactPositions(data.tableId, trx, minDeletedPos)
2181+
await recompactPositions(table.id, trx, minDeletedPos)
21822182
})
21832183

2184-
logger.info(`[${requestId}] Deleted ${matchingRows.length} rows from table ${data.tableId}`)
2184+
logger.info(`[${requestId}] Deleted ${matchingRows.length} rows from table ${table.id}`)
21852185

21862186
return {
21872187
affectedCount: matchingRows.length,

apps/sim/lib/table/types.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -319,11 +319,9 @@ export interface UpdateRowData {
319319
}
320320

321321
export interface BulkUpdateData {
322-
tableId: string
323322
filter: Filter
324323
data: RowData
325324
limit?: number
326-
workspaceId: string
327325
}
328326

329327
export interface BatchUpdateByIdData {
@@ -339,10 +337,8 @@ export interface BatchUpdateByIdData {
339337
}
340338

341339
export interface BulkDeleteData {
342-
tableId: string
343340
filter: Filter
344341
limit?: number
345-
workspaceId: string
346342
}
347343

348344
export interface BulkDeleteByIdsData {

0 commit comments

Comments
 (0)