Skip to content

Commit ddab1aa

Browse files
TheodoreSpeaksTheodoreSpeaks
authored
refactor(tables): consolidate row data-access in service.ts (#4881)
- Route both row-GET endpoints (internal + v1) and the copilot tool through the single service.queryRows instead of three inline query copies; add a withExecutions option so the public v1 route still omits executions. - Run COUNT(*) and the page fetch concurrently in queryRows. - Move CSV-import transaction ownership out of the API route into importAppendRows / importReplaceRows so routes never hold a trx. - Extract row position mechanics (reserve / shift / compact) into named private helpers in service.ts; no separate table-wrapper module.
1 parent 530b2c0 commit ddab1aa

7 files changed

Lines changed: 548 additions & 599 deletions

File tree

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

Lines changed: 60 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,15 @@ import type { TableDefinition } from '@/lib/table'
88

99
const {
1010
mockCheckAccess,
11-
mockBatchInsertRowsWithTx,
12-
mockReplaceTableRowsWithTx,
13-
mockAddTableColumnsWithTx,
11+
mockImportAppendRows,
12+
mockImportReplaceRows,
1413
mockDispatchAfterBatchInsert,
1514
mockMarkTableImporting,
1615
mockReleaseImportClaim,
1716
} = vi.hoisted(() => ({
1817
mockCheckAccess: vi.fn(),
19-
mockBatchInsertRowsWithTx: vi.fn(),
20-
mockReplaceTableRowsWithTx: vi.fn(),
21-
mockAddTableColumnsWithTx: vi.fn(),
18+
mockImportAppendRows: vi.fn(),
19+
mockImportReplaceRows: vi.fn(),
2220
mockDispatchAfterBatchInsert: vi.fn(),
2321
mockMarkTableImporting: vi.fn(),
2422
mockReleaseImportClaim: vi.fn(),
@@ -47,15 +45,15 @@ vi.mock('@/app/api/table/utils', async () => {
4745
})
4846

4947
/**
50-
* The route imports `batchInsertRows` and `replaceTableRows` from the barrel,
51-
* which forwards them from `./service`. Mocking the service module replaces
52-
* both without having to touch the other real helpers (`parseCsvBuffer`,
53-
* `coerceRowsForTable`, etc.) exported through the barrel.
48+
* The route imports `importAppendRows` / `importReplaceRows` from the barrel,
49+
* which forwards them from `./service`. These functions own the import
50+
* transaction (column adds + row writes); mocking the service module replaces
51+
* them without touching the other real helpers (`coerceRowsForTable`,
52+
* `createCsvParser`, etc.) exported through the barrel.
5453
*/
5554
vi.mock('@/lib/table/service', () => ({
56-
batchInsertRowsWithTx: mockBatchInsertRowsWithTx,
57-
replaceTableRowsWithTx: mockReplaceTableRowsWithTx,
58-
addTableColumnsWithTx: mockAddTableColumnsWithTx,
55+
importAppendRows: mockImportAppendRows,
56+
importReplaceRows: mockImportReplaceRows,
5957
dispatchAfterBatchInsert: mockDispatchAfterBatchInsert,
6058
markTableImporting: mockMarkTableImporting,
6159
releaseImportClaim: mockReleaseImportClaim,
@@ -125,6 +123,16 @@ function buildTable(overrides: Partial<TableDefinition> = {}): TableDefinition {
125123
}
126124
}
127125

126+
/** Additions array the route passed to importAppendRows (2nd positional arg). */
127+
function appendAdditions(): { name: string; type: string }[] {
128+
return mockImportAppendRows.mock.calls[0][1] as { name: string; type: string }[]
129+
}
130+
131+
/** Rows array the route passed to importAppendRows (3rd positional arg). */
132+
function appendRows(): unknown[] {
133+
return mockImportAppendRows.mock.calls[0][2] as unknown[]
134+
}
135+
128136
async function callPost(form: FormData, { tableId }: { tableId: string } = { tableId: 'tbl_1' }) {
129137
// Building the request from a FormData body gives a real multipart stream and
130138
// boundary, exercising the streaming `readMultipart` parser end-to-end.
@@ -144,27 +152,15 @@ describe('POST /api/table/[tableId]/import', () => {
144152
authType: 'session',
145153
})
146154
mockCheckAccess.mockResolvedValue({ ok: true, table: buildTable() })
147-
mockBatchInsertRowsWithTx.mockImplementation(async (_trx, data: { rows: unknown[] }) =>
148-
data.rows.map((_, i) => ({ id: `row_${i}` }))
155+
mockImportAppendRows.mockImplementation(
156+
async (table: TableDefinition, _additions: unknown, rows: unknown[]) => ({
157+
inserted: rows.map((_, i) => ({ id: `row_${i}` })),
158+
table,
159+
})
149160
)
150-
mockReplaceTableRowsWithTx.mockResolvedValue({ deletedCount: 0, insertedCount: 0 })
161+
mockImportReplaceRows.mockResolvedValue({ deletedCount: 0, insertedCount: 0 })
151162
mockMarkTableImporting.mockResolvedValue(true)
152163
mockReleaseImportClaim.mockResolvedValue(undefined)
153-
mockAddTableColumnsWithTx.mockImplementation(
154-
async (
155-
_trx,
156-
table: { schema: { columns: { name: string; type: string }[] } },
157-
columns: { name: string; type: string }[]
158-
) => ({
159-
...table,
160-
schema: {
161-
columns: [
162-
...table.schema.columns,
163-
...columns.map((c) => ({ name: c.name, type: c.type as 'string' })),
164-
],
165-
},
166-
})
167-
)
168164
})
169165

170166
it('returns 401 when the user is not authenticated', async () => {
@@ -180,8 +176,8 @@ describe('POST /api/table/[tableId]/import', () => {
180176
mockMarkTableImporting.mockResolvedValueOnce(false)
181177
const response = await callPost(createFormData(createCsvFile('name,age\nAlice,30')))
182178
expect(response.status).toBe(409)
183-
expect(mockBatchInsertRowsWithTx).not.toHaveBeenCalled()
184-
expect(mockReplaceTableRowsWithTx).not.toHaveBeenCalled()
179+
expect(mockImportAppendRows).not.toHaveBeenCalled()
180+
expect(mockImportReplaceRows).not.toHaveBeenCalled()
185181
expect(mockReleaseImportClaim).not.toHaveBeenCalled()
186182
})
187183

@@ -242,8 +238,8 @@ describe('POST /api/table/[tableId]/import', () => {
242238

243239
const response = await POST(req, { params: Promise.resolve({ tableId: 'tbl_1' }) })
244240
expect(response.status).toBe(400)
245-
expect(mockBatchInsertRowsWithTx).not.toHaveBeenCalled()
246-
expect(mockReplaceTableRowsWithTx).not.toHaveBeenCalled()
241+
expect(mockImportAppendRows).not.toHaveBeenCalled()
242+
expect(mockImportReplaceRows).not.toHaveBeenCalled()
247243
})
248244

249245
it('returns 400 when the CSV is missing a required column', async () => {
@@ -252,24 +248,23 @@ describe('POST /api/table/[tableId]/import', () => {
252248
const data = await response.json()
253249
expect(data.error).toMatch(/missing required columns/i)
254250
expect(data.details?.missingRequired).toEqual(['name'])
255-
expect(mockBatchInsertRowsWithTx).not.toHaveBeenCalled()
251+
expect(mockImportAppendRows).not.toHaveBeenCalled()
256252
})
257253

258-
it('appends rows via batchInsertRows', async () => {
254+
it('appends rows via importAppendRows', async () => {
259255
const response = await callPost(
260256
createFormData(createCsvFile('name,age\nAlice,30\nBob,40'), { mode: 'append' })
261257
)
262258
expect(response.status).toBe(200)
263259
const data = await response.json()
264260
expect(data.data.mode).toBe('append')
265261
expect(data.data.insertedCount).toBe(2)
266-
expect(mockBatchInsertRowsWithTx).toHaveBeenCalledTimes(1)
267-
const callArgs = mockBatchInsertRowsWithTx.mock.calls[0][1] as { rows: unknown[] }
268-
expect(callArgs.rows).toEqual([
262+
expect(mockImportAppendRows).toHaveBeenCalledTimes(1)
263+
expect(appendRows()).toEqual([
269264
{ name: 'Alice', age: 30 },
270265
{ name: 'Bob', age: 40 },
271266
])
272-
expect(mockReplaceTableRowsWithTx).not.toHaveBeenCalled()
267+
expect(mockImportReplaceRows).not.toHaveBeenCalled()
273268
})
274269

275270
it('accepts chunked multipart imports without a content-length header', async () => {
@@ -284,7 +279,7 @@ describe('POST /api/table/[tableId]/import', () => {
284279
const response = await POST(req, { params: Promise.resolve({ tableId: 'tbl_1' }) })
285280

286281
expect(response.status).toBe(200)
287-
expect(mockBatchInsertRowsWithTx).toHaveBeenCalledTimes(1)
282+
expect(mockImportAppendRows).toHaveBeenCalledTimes(1)
288283
})
289284

290285
it('rejects append when it would exceed maxRows', async () => {
@@ -298,11 +293,11 @@ describe('POST /api/table/[tableId]/import', () => {
298293
expect(response.status).toBe(400)
299294
const data = await response.json()
300295
expect(data.error).toMatch(/exceed table row limit/)
301-
expect(mockBatchInsertRowsWithTx).not.toHaveBeenCalled()
296+
expect(mockImportAppendRows).not.toHaveBeenCalled()
302297
})
303298

304-
it('replaces rows via replaceTableRows', async () => {
305-
mockReplaceTableRowsWithTx.mockResolvedValueOnce({ deletedCount: 5, insertedCount: 2 })
299+
it('replaces rows via importReplaceRows', async () => {
300+
mockImportReplaceRows.mockResolvedValueOnce({ deletedCount: 5, insertedCount: 2 })
306301
const response = await callPost(
307302
createFormData(createCsvFile('name,age\nAlice,30\nBob,40'), { mode: 'replace' })
308303
)
@@ -311,8 +306,8 @@ describe('POST /api/table/[tableId]/import', () => {
311306
expect(data.data.mode).toBe('replace')
312307
expect(data.data.deletedCount).toBe(5)
313308
expect(data.data.insertedCount).toBe(2)
314-
expect(mockReplaceTableRowsWithTx).toHaveBeenCalledTimes(1)
315-
expect(mockBatchInsertRowsWithTx).not.toHaveBeenCalled()
309+
expect(mockImportReplaceRows).toHaveBeenCalledTimes(1)
310+
expect(mockImportAppendRows).not.toHaveBeenCalled()
316311
})
317312

318313
it('uses an explicit mapping when provided', async () => {
@@ -325,8 +320,7 @@ describe('POST /api/table/[tableId]/import', () => {
325320
expect(response.status).toBe(200)
326321
const data = await response.json()
327322
expect(data.data.mappedColumns).toEqual(['First Name', 'Years'])
328-
const callArgs = mockBatchInsertRowsWithTx.mock.calls[0][1] as { rows: unknown[] }
329-
expect(callArgs.rows).toEqual([
323+
expect(appendRows()).toEqual([
330324
{ name: 'Alice', age: 30 },
331325
{ name: 'Bob', age: 40 },
332326
])
@@ -356,8 +350,8 @@ describe('POST /api/table/[tableId]/import', () => {
356350
expect(data.error).toMatch(/Mapping values must be/)
357351
})
358352

359-
it('surfaces unique violations from batchInsertRows as 400', async () => {
360-
mockBatchInsertRowsWithTx.mockRejectedValueOnce(
353+
it('surfaces unique violations from importAppendRows as 400', async () => {
354+
mockImportAppendRows.mockRejectedValueOnce(
361355
new Error('Row 1: Column "name" must be unique. Value "Alice" already exists in row row_xxx')
362356
)
363357
const response = await callPost(
@@ -377,7 +371,7 @@ describe('POST /api/table/[tableId]/import', () => {
377371
)
378372
)
379373
expect(response.status).toBe(200)
380-
expect(mockBatchInsertRowsWithTx).toHaveBeenCalledTimes(1)
374+
expect(mockImportAppendRows).toHaveBeenCalledTimes(1)
381375
})
382376

383377
it('returns 400 for unsupported file extensions', async () => {
@@ -398,12 +392,9 @@ describe('POST /api/table/[tableId]/import', () => {
398392
})
399393
)
400394
expect(response.status).toBe(200)
401-
expect(mockAddTableColumnsWithTx).toHaveBeenCalledTimes(1)
402-
const [, , columns] = mockAddTableColumnsWithTx.mock.calls[0]
403-
expect(columns).toEqual([{ name: 'email', type: 'string' }])
404-
405-
const callArgs = mockBatchInsertRowsWithTx.mock.calls[0][1] as { rows: unknown[] }
406-
expect(callArgs.rows).toEqual([
395+
expect(mockImportAppendRows).toHaveBeenCalledTimes(1)
396+
expect(appendAdditions()).toEqual([{ name: 'email', type: 'string' }])
397+
expect(appendRows()).toEqual([
407398
{ name: 'Alice', age: 30, email: 'a@x.io' },
408399
{ name: 'Bob', age: 40, email: 'b@x.io' },
409400
])
@@ -417,8 +408,7 @@ describe('POST /api/table/[tableId]/import', () => {
417408
})
418409
)
419410
expect(response.status).toBe(200)
420-
const [, , columns] = mockAddTableColumnsWithTx.mock.calls[0]
421-
expect(columns).toEqual([{ name: 'score', type: 'number' }])
411+
expect(appendAdditions()).toEqual([{ name: 'score', type: 'number' }])
422412
})
423413

424414
it('dedupes when sanitized name collides with an existing column', async () => {
@@ -441,8 +431,7 @@ describe('POST /api/table/[tableId]/import', () => {
441431
})
442432
)
443433
expect(response.status).toBe(200)
444-
const [, , columns] = mockAddTableColumnsWithTx.mock.calls[0]
445-
expect(columns).toEqual([{ name: 'Email_2', type: 'string' }])
434+
expect(appendAdditions()).toEqual([{ name: 'Email_2', type: 'string' }])
446435
})
447436

448437
it('returns 400 when createColumns references a header not in the CSV', async () => {
@@ -455,8 +444,7 @@ describe('POST /api/table/[tableId]/import', () => {
455444
expect(response.status).toBe(400)
456445
const data = await response.json()
457446
expect(data.error).toMatch(/unknown CSV headers/)
458-
expect(mockAddTableColumnsWithTx).not.toHaveBeenCalled()
459-
expect(mockBatchInsertRowsWithTx).not.toHaveBeenCalled()
447+
expect(mockImportAppendRows).not.toHaveBeenCalled()
460448
})
461449

462450
it('returns 400 when createColumns is not an array of strings', async () => {
@@ -469,7 +457,7 @@ describe('POST /api/table/[tableId]/import', () => {
469457
expect(response.status).toBe(400)
470458
const data = await response.json()
471459
expect(data.error).toMatch(/createColumns must be a JSON array/)
472-
expect(mockAddTableColumnsWithTx).not.toHaveBeenCalled()
460+
expect(mockImportAppendRows).not.toHaveBeenCalled()
473461
})
474462

475463
it('returns 400 when createColumns is invalid JSON', async () => {
@@ -484,8 +472,8 @@ describe('POST /api/table/[tableId]/import', () => {
484472
expect(data.error).toMatch(/createColumns must be valid JSON/)
485473
})
486474

487-
it('surfaces addTableColumns failures as 400', async () => {
488-
mockAddTableColumnsWithTx.mockRejectedValueOnce(new Error('Column "email" already exists'))
475+
it('surfaces column-creation failures from importAppendRows as 400', async () => {
476+
mockImportAppendRows.mockRejectedValueOnce(new Error('Column "email" already exists'))
489477
const response = await callPost(
490478
createFormData(createCsvFile('name,age,email\nAlice,30,a@x.io'), {
491479
mode: 'append',
@@ -495,30 +483,30 @@ describe('POST /api/table/[tableId]/import', () => {
495483
expect(response.status).toBe(400)
496484
const data = await response.json()
497485
expect(data.error).toMatch(/already exists/)
498-
expect(mockBatchInsertRowsWithTx).not.toHaveBeenCalled()
499486
})
500487

501488
it('surfaces row insert failures without success when schema was mutated', async () => {
502-
mockBatchInsertRowsWithTx.mockRejectedValueOnce(new Error('must be unique'))
489+
mockImportAppendRows.mockRejectedValueOnce(new Error('must be unique'))
503490
const response = await callPost(
504491
createFormData(createCsvFile('name,age,email\nAlice,30,a@x.io'), {
505492
mode: 'append',
506493
createColumns: ['email'],
507494
})
508495
)
509-
expect(mockAddTableColumnsWithTx).toHaveBeenCalled()
496+
// Route forwarded the column addition into the (now atomic) import op.
497+
expect(appendAdditions()).toEqual([{ name: 'email', type: 'string' }])
510498
expect(response.status).toBe(400)
511499
const data = await response.json()
512500
expect(data.success).toBeUndefined()
513501
expect(data.error).toMatch(/must be unique/)
514502
})
515503

516-
it('does not call addTableColumns when createColumns is omitted', async () => {
504+
it('passes no additions when createColumns is omitted', async () => {
517505
const response = await callPost(
518506
createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' })
519507
)
520508
expect(response.status).toBe(200)
521-
expect(mockAddTableColumnsWithTx).not.toHaveBeenCalled()
509+
expect(appendAdditions()).toEqual([])
522510
})
523511
})
524512
})

0 commit comments

Comments
 (0)