Skip to content

Commit 9dc14c1

Browse files
committed
fix(tables): map typed service errors in the import routes' own catches
The append branch returns instead of rethrowing, so nothing the outer catch does applies to it. addTableColumnsWithTx runs inside importAppendRows, so the column cap and an invalid column name surfaced there as a generic 500 with the real reason replaced by 'Failed to import CSV'. Extract tableRequestErrorResponse alongside the existing rowWriteErrorResponse funnel and call it from all three catches (both import routes), so a new service validation message is classified the day it is added rather than when someone remembers to extend a substring list.
1 parent cb8994d commit 9dc14c1

6 files changed

Lines changed: 133 additions & 8 deletions

File tree

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { hybridAuthMockFns } from '@sim/testing'
55
import { NextRequest } from 'next/server'
66
import { beforeEach, describe, expect, it, vi } from 'vitest'
77
import type { TableDefinition } from '@/lib/table'
8+
import { TableRequestError } from '@/lib/table/errors'
89

910
const {
1011
mockCheckAccess,
@@ -32,6 +33,7 @@ vi.mock('@sim/utils/id', () => ({
3233
vi.mock('@/app/api/table/utils', async () => {
3334
const { NextResponse } = await import('next/server')
3435
const { TableLockedError } = await import('@/lib/table/mutation-locks')
36+
const { TableRequestError } = await import('@/lib/table/errors')
3537
return {
3638
checkAccess: mockCheckAccess,
3739
accessError: (result: { status: number }) => {
@@ -43,6 +45,10 @@ vi.mock('@/app/api/table/utils', async () => {
4345
error instanceof TableLockedError
4446
? NextResponse.json({ error: error.message, lock: error.lock }, { status: 423 })
4547
: null,
48+
tableRequestErrorResponse: (error: unknown) =>
49+
error instanceof TableRequestError
50+
? NextResponse.json({ error: error.message }, { status: error.status })
51+
: null,
4652
multipartErrorResponse: (error: { code: string; message: string }) =>
4753
NextResponse.json(
4854
{ error: error.message },
@@ -316,6 +322,43 @@ describe('POST /api/table/[tableId]/import', () => {
316322
expect(mockImportAppendRows).not.toHaveBeenCalled()
317323
})
318324

325+
/**
326+
* `addTableColumnsWithTx` runs INSIDE `importAppendRows`, so the service's own
327+
* typed failures surface in the append catch — which returns instead of
328+
* rethrowing, so nothing the outer catch does applies. Without the explicit
329+
* mapping there, the column cap and an invalid column type both come back as
330+
* a 500 whose real reason has been replaced by 'Failed to import CSV'.
331+
*/
332+
it.each([
333+
['the column cap', 'Adding 2 column(s) would exceed maximum column limit (100)'],
334+
['an invalid column type', 'Invalid column type "sometype". Must be one of: string, number'],
335+
])('surfaces %s from an append as the service status', async (_label, message) => {
336+
mockImportAppendRows.mockRejectedValueOnce(new TableRequestError(message))
337+
const response = await callPost(
338+
createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' })
339+
)
340+
expect(response.status).toBe(400)
341+
expect((await response.json()).error).toBe(message)
342+
})
343+
344+
it('keeps a 404 from an append a 404 rather than flattening it to 400', async () => {
345+
mockImportAppendRows.mockRejectedValueOnce(new TableRequestError('Table not found', 404))
346+
const response = await callPost(
347+
createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' })
348+
)
349+
expect(response.status).toBe(404)
350+
expect((await response.json()).error).toBe('Table not found')
351+
})
352+
353+
it('still returns a generic 500 for an append failure the service did not type', async () => {
354+
mockImportAppendRows.mockRejectedValueOnce(new Error('connection terminated unexpectedly'))
355+
const response = await callPost(
356+
createFormData(createCsvFile('name,age\nAlice,30'), { mode: 'append' })
357+
)
358+
expect(response.status).toBe(500)
359+
expect((await response.json()).error).toBe('Failed to import CSV')
360+
})
361+
319362
it('replaces rows via importReplaceRows', async () => {
320363
mockImportReplaceRows.mockResolvedValueOnce({ deletedCount: 5, insertedCount: 2 })
321364
const response = await callPost(

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ import {
3737
wouldExceedRowLimit,
3838
} from '@/lib/table'
3939
import { sniffCsvDelimiterFromStream } from '@/lib/table/csv-delimiter-stream'
40-
import { TableRequestError } from '@/lib/table/errors'
4140
import { signalTableSchemaChanged } from '@/lib/table/events'
4241
import { importAppendRows, importReplaceRows } from '@/lib/table/import-data'
4342
import { getUserSettings } from '@/lib/users/queries'
@@ -47,6 +46,7 @@ import {
4746
csvProxyBodyCapResponse,
4847
multipartErrorResponse,
4948
tableLockErrorResponse,
49+
tableRequestErrorResponse,
5050
} from '@/app/api/table/utils'
5151

5252
const logger = createLogger('TableImportCSVExisting')
@@ -346,11 +346,17 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
346346
},
347347
})
348348
} catch (err) {
349-
// This branch returns rather than rethrowing, so the outer catch's
350-
// mapper is unreachable from here — map the lock error first or a 423
351-
// degrades into a generic 500 (replace mode rethrows and maps fine).
349+
// This branch returns rather than rethrowing, so NOTHING in the outer
350+
// catch runs for an append failure — every mapper it applies has to be
351+
// repeated here (replace mode rethrows and maps fine). A 423 lock
352+
// violation and the service's own typed failures both degrade into a
353+
// generic 500 without these two lines: `addTableColumnsWithTx` runs
354+
// INSIDE `importAppendRows`, so an invalid column name or the column
355+
// cap surfaces here, not out there.
352356
const lockError = tableLockErrorResponse(err)
353357
if (lockError) return lockError
358+
const requestError = tableRequestErrorResponse(err)
359+
if (requestError) return requestError
354360

355361
const message = toError(err).message
356362
logger.warn(`[${requestId}] Append failed for table ${tableId}`, {
@@ -437,9 +443,8 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
437443
// is not table-aware — but the service's own validation (an invalid column
438444
// type, the column cap) matched none of those strings and was reported as a
439445
// 500 with the message swallowed.
440-
if (error instanceof TableRequestError) {
441-
return NextResponse.json({ error: error.message }, { status: error.status })
442-
}
446+
const requestError = tableRequestErrorResponse(error)
447+
if (requestError) return requestError
443448

444449
const isClientError =
445450
message.includes('CSV file has no') ||

apps/sim/app/api/table/import-csv/route.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ vi.mock('@/lib/table/rows/service', () => ({
3232
vi.mock('@/lib/table/billing', () => ({ getWorkspaceTableLimits: mockGetLimits }))
3333
vi.mock('@/app/api/table/utils', async () => {
3434
const { NextResponse } = await import('next/server')
35+
const { TableRequestError } = await import('@/lib/table/errors')
3536
return {
3637
normalizeColumn: (column: unknown) => column,
3738
csvProxyBodyCapResponse: () => null,
@@ -40,6 +41,10 @@ vi.mock('@/app/api/table/utils', async () => {
4041
{ error: error.message },
4142
{ status: error.code === 'FILE_TOO_LARGE' ? 413 : 400 }
4243
),
44+
tableRequestErrorResponse: (error: unknown) =>
45+
error instanceof TableRequestError
46+
? NextResponse.json({ error: error.message }, { status: error.status })
47+
: null,
4348
rowWriteErrorResponse: (error: unknown) => {
4449
const message = getErrorMessage(error)
4550
return message.includes('row limit')
@@ -50,6 +55,7 @@ vi.mock('@/app/api/table/utils', async () => {
5055
})
5156
vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
5257

58+
import { TableRequestError } from '@/lib/table/errors'
5359
import { POST } from '@/app/api/table/import-csv/route'
5460

5561
type Part =
@@ -193,6 +199,20 @@ describe('POST /api/table/import-csv', () => {
193199
expect(data.error).toMatch(/row limit/)
194200
})
195201

202+
/**
203+
* `createTable` validates the name, the schema, the per-column rules, and both
204+
* plan caps. The substring list in this route's catch named only some of those
205+
* messages, so the rest reached the client as a generic 500.
206+
*/
207+
it('surfaces a typed createTable failure the substring list never named', async () => {
208+
const message = 'Column name exceeds maximum length (64 characters)'
209+
mockCreateTable.mockRejectedValueOnce(new TableRequestError(message))
210+
const response = await POST(makeRequest(uploadParts(csvWithRows(5))))
211+
212+
expect(response.status).toBe(400)
213+
expect((await response.json()).error).toBe(message)
214+
})
215+
196216
it('rolls back the created table when a batch insert fails mid-stream', async () => {
197217
mockBatchInsertRows
198218
.mockResolvedValueOnce(Array.from({ length: 100 }, () => ({ id: 'row' })))

apps/sim/app/api/table/import-csv/route.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import {
3535
multipartErrorResponse,
3636
normalizeColumn,
3737
rowWriteErrorResponse,
38+
tableRequestErrorResponse,
3839
} from '@/app/api/table/utils'
3940

4041
const logger = createLogger('TableImportCSV')
@@ -254,6 +255,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
254255
const rowWriteError = rowWriteErrorResponse(error)
255256
if (rowWriteError) return rowWriteError
256257

258+
// `createTable` validates the name, the schema, the per-column rules, and
259+
// both plan caps — most of which the substring list below never named.
260+
const requestError = tableRequestErrorResponse(error)
261+
if (requestError) return requestError
262+
257263
const message = toError(error).message
258264
const isClientError =
259265
message.includes('maximum table limit') ||

apps/sim/app/api/table/utils.test.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,14 @@
33
*/
44
import { describe, expect, it } from 'vitest'
55
import { TableRowLimitError } from '@/lib/table/billing'
6+
import { TableRequestError } from '@/lib/table/errors'
67
import type { ColumnDefinition } from '@/lib/table/types'
7-
import { rootErrorMessage, rowWriteErrorResponse, tableFilterError } from '@/app/api/table/utils'
8+
import {
9+
rootErrorMessage,
10+
rowWriteErrorResponse,
11+
tableFilterError,
12+
tableRequestErrorResponse,
13+
} from '@/app/api/table/utils'
814

915
/** Mimics drizzle's DrizzleQueryError: message is the failed SQL, real error on `cause`. */
1016
function wrapLikeDrizzle(cause: Error): Error {
@@ -55,6 +61,34 @@ describe('rowWriteErrorResponse', () => {
5561
})
5662
})
5763

64+
/**
65+
* The service classifies its own failures, so callers must not re-derive the
66+
* verdict from message text. These cases are exactly the ones no substring list
67+
* named — which is how they reached clients as a generic 500.
68+
*/
69+
describe('tableRequestErrorResponse', () => {
70+
it('carries the service message at the status the service chose', async () => {
71+
const response = tableRequestErrorResponse(
72+
new TableRequestError('Adding 2 column(s) would exceed maximum column limit (100)')
73+
)
74+
expect(response?.status).toBe(400)
75+
const body = await response?.json()
76+
expect(body.error).toBe('Adding 2 column(s) would exceed maximum column limit (100)')
77+
})
78+
79+
it('preserves a 404 rather than flattening every typed failure to 400', () => {
80+
expect(tableRequestErrorResponse(new TableRequestError('Table not found', 404))?.status).toBe(
81+
404
82+
)
83+
})
84+
85+
it('returns null for anything the service did not type', () => {
86+
expect(tableRequestErrorResponse(new Error('connection refused'))).toBeNull()
87+
expect(tableRequestErrorResponse(new TableRowLimitError(10000))).toBeNull()
88+
expect(tableRequestErrorResponse('not an error')).toBeNull()
89+
})
90+
})
91+
5892
/**
5993
* The async destructive routes (delete-async, cancel-runs, columns/run)
6094
* validate the WIRE filter here. The predicate branch must reject unknown

apps/sim/app/api/table/utils.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import type { ColumnDefinition, Filter, TableDefinition, TablePredicate } from '
1313
import { buildFilterClause, getTableById, TableQueryValidationError } from '@/lib/table'
1414
import { typeMetadataOf } from '@/lib/table/column-types'
1515
import { USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants'
16+
import { TableRequestError } from '@/lib/table/errors'
1617
import { TableLockedError } from '@/lib/table/mutation-locks'
1718
import { isTablePredicate } from '@/lib/table/query-builder/converters'
1819
import { validateStoragePredicate } from '@/lib/table/query-builder/validate'
@@ -105,6 +106,22 @@ export function rootErrorMessage(error: unknown): string {
105106
return toError(current).message
106107
}
107108

109+
/**
110+
* Maps the table service's own typed failures to the status they declare, or
111+
* `null` when the error came from somewhere else.
112+
*
113+
* Prefer this over matching message substrings: the service says whose fault a
114+
* failure is, so a new validation message is classified correctly the day it is
115+
* added. The substring lists still cover the layers that are not table-aware
116+
* (the CSV parser, drizzle), which is why callers run both — this one first,
117+
* since a typed error carries an explicit status that a substring match would
118+
* flatten to 400.
119+
*/
120+
export function tableRequestErrorResponse(error: unknown): NextResponse | null {
121+
if (!(error instanceof TableRequestError)) return null
122+
return NextResponse.json({ error: error.message }, { status: error.status })
123+
}
124+
108125
/**
109126
* Known user-facing row-write failures (service validation + the best-effort
110127
* plan row-limit check). Anything outside this list stays a generic 500 —

0 commit comments

Comments
 (0)