Skip to content

Commit 4d42a81

Browse files
committed
fix(tables): finish the typed-error migration in the table service
The bulk column-add path kept throwing plain `Error` after the single-column path was migrated, so an invalid column type or the column cap fell past the import route's substring list and surfaced as a 500 with the message swallowed ("Failed to import CSV") — the caller never learned their header was invalid. All 13 remaining throws in the table service are now typed: a missing table is 404, everything else 400. The import route and both table-create routes check the type before their substring lists, which stay only for messages raised by the CSV parser, which is not table-aware. Extracts `buildAddedColumns` so the batch validation is testable without a transaction, mirroring `buildAddedColumn`, and pins the typing rather than the message — this is the third error-status gap found on this PR, and each came from a list that had to be remembered. Also pins that message text and `instanceof Error` are unchanged, since the runners, the copilot tool and sibling routes still match on text.
1 parent d9cad69 commit 4d42a81

5 files changed

Lines changed: 118 additions & 28 deletions

File tree

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {
3737
wouldExceedRowLimit,
3838
} from '@/lib/table'
3939
import { sniffCsvDelimiterFromStream } from '@/lib/table/csv-delimiter-stream'
40+
import { TableRequestError } from '@/lib/table/errors'
4041
import { signalTableSchemaChanged } from '@/lib/table/events'
4142
import { importAppendRows, importReplaceRows } from '@/lib/table/import-data'
4243
import { getUserSettings } from '@/lib/users/queries'
@@ -425,6 +426,15 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
425426
const message = toError(error).message
426427
logger.error(`[${requestId}] CSV import into existing table failed:`, error)
427428

429+
// The table service says whether a failure is the caller's and what status
430+
// it deserves. The substring list below still covers the CSV parser, which
431+
// is not table-aware — but the service's own validation (an invalid column
432+
// type, the column cap) matched none of those strings and was reported as a
433+
// 500 with the message swallowed.
434+
if (error instanceof TableRequestError) {
435+
return NextResponse.json({ error: error.message }, { status: error.status })
436+
}
437+
428438
const isClientError =
429439
message.includes('CSV file has no') ||
430440
message.includes('already exists') ||

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
type TableSchema,
1616
type TableScope,
1717
} from '@/lib/table'
18+
import { TableRequestError } from '@/lib/table/errors'
1819
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1920
import { normalizeColumn } from '@/app/api/table/utils'
2021

@@ -154,6 +155,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
154155
})
155156
} catch (error) {
156157
if (error instanceof Error) {
158+
// One typed check: the service says whether a failure is the caller's
159+
// and what status it deserves.
160+
if (error instanceof TableRequestError) {
161+
return NextResponse.json({ error: error.message }, { status: error.status })
162+
}
157163
if (error.message.includes('maximum table limit')) {
158164
return NextResponse.json({ error: error.message }, { status: 403 })
159165
}

apps/sim/app/api/v1/tables/route.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { parseRequest } from '@/lib/api/server'
66
import { generateRequestId } from '@/lib/core/utils/request'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
88
import { createTable, getWorkspaceTableLimits, listTables, type TableSchema } from '@/lib/table'
9+
import { TableRequestError } from '@/lib/table/errors'
910
import { normalizeColumn } from '@/app/api/table/utils'
1011
import {
1112
checkRateLimit,
@@ -172,6 +173,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
172173
if (validationResponse) return validationResponse
173174

174175
if (error instanceof Error) {
176+
// One typed check: the service says whether a failure is the caller's
177+
// and what status it deserves.
178+
if (error instanceof TableRequestError) {
179+
return NextResponse.json({ error: error.message }, { status: error.status })
180+
}
175181
if (error.message.includes('maximum table limit')) {
176182
return NextResponse.json({ error: error.message }, { status: 403 })
177183
}

apps/sim/lib/table/__tests__/backward-compatibility.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@
1717
*/
1818
import { describe, expect, it } from 'vitest'
1919
import { COLUMN_TYPE_REGISTRY } from '@/lib/table/column-types'
20+
import { TABLE_LIMITS } from '@/lib/table/constants'
21+
import { TableRequestError, tableNotFound } from '@/lib/table/errors'
2022
import { coerceValue, inferColumnType } from '@/lib/table/import'
23+
import { buildAddedColumns } from '@/lib/table/service'
2124
import { buildFilterClause, buildSortClause } from '@/lib/table/sql'
2225
import type { ColumnDefinition, Filter, JsonValue, Sort } from '@/lib/table/types'
2326
import { validateColumnDefinition } from '@/lib/table/validation'
@@ -257,3 +260,45 @@ describe('export is unchanged for pre-existing types', () => {
257260
expect(COLUMN_TYPE_REGISTRY.select.formatForDisplay('o1', LEGACY_COLUMNS[6])).toBe('One')
258261
})
259262
})
263+
264+
describe('caller-fixable failures are typed, not string-matched', () => {
265+
const table = { schema: { columns: [{ id: 'a', name: 'a', type: 'string' as const }] } }
266+
267+
it.each([
268+
['invalid name', { name: '1bad', type: 'string' }],
269+
['name too long', { name: 'a'.repeat(200), type: 'string' }],
270+
['invalid type', { name: 'ok', type: 'nonsense' }],
271+
['duplicate name', { name: 'a', type: 'string' }],
272+
])('raises TableRequestError(400) for %s', (_label, column) => {
273+
// The bulk path kept throwing plain `Error` after the single-column path was
274+
// migrated, so an invalid type or the column cap fell past the import
275+
// route's substring list and became a 500 with the message swallowed.
276+
// Asserting the TYPE is what stops the two drifting again.
277+
let thrown: unknown
278+
try {
279+
buildAddedColumns(table as never, [column as never])
280+
} catch (error) {
281+
thrown = error
282+
}
283+
expect(thrown).toBeInstanceOf(TableRequestError)
284+
expect((thrown as TableRequestError).status).toBe(400)
285+
})
286+
287+
it('raises TableRequestError(400) when the column cap is exceeded', () => {
288+
const many = Array.from({ length: TABLE_LIMITS.MAX_COLUMNS_PER_TABLE + 1 }, (_, i) => ({
289+
name: `c${i}`,
290+
type: 'string',
291+
}))
292+
expect(() => buildAddedColumns(table as never, many as never)).toThrow(TableRequestError)
293+
})
294+
295+
it('keeps message text and Error-ness, so existing string matchers still work', () => {
296+
// The migration changed the CLASS, never the message — other consumers
297+
// (runners, the copilot tool, sibling routes) still match on text.
298+
const notFound = tableNotFound('Table not found')
299+
expect(notFound).toBeInstanceOf(Error)
300+
expect(notFound.message).toBe('Table not found')
301+
expect(notFound.status).toBe(404)
302+
expect(new TableRequestError('anything').status).toBe(400)
303+
})
304+
})

apps/sim/lib/table/service.ts

Lines changed: 51 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import {
2727
pickMetadata,
2828
} from '@/lib/table/column-types'
2929
import { COLUMN_TYPES, NAME_PATTERN, TABLE_LIMITS } from '@/lib/table/constants'
30-
import { tableNotFound } from '@/lib/table/errors'
30+
import { TableRequestError, tableNotFound } from '@/lib/table/errors'
3131
import { appendTableEvent } from '@/lib/table/events'
3232
import { EMPTY_JOB_FIELDS, latestJobForTable, latestJobsForTables } from '@/lib/table/jobs/service'
3333
import { assertSchemaMutable, TableLockedError } from '@/lib/table/mutation-locks'
@@ -294,13 +294,13 @@ export async function createTable(
294294
// Validate table name
295295
const nameValidation = validateTableName(data.name)
296296
if (!nameValidation.valid) {
297-
throw new Error(`Invalid table name: ${nameValidation.errors.join(', ')}`)
297+
throw new TableRequestError(`Invalid table name: ${nameValidation.errors.join(', ')}`)
298298
}
299299

300300
// Validate schema
301301
const schemaValidation = validateTableSchema(data.schema)
302302
if (!schemaValidation.valid) {
303-
throw new Error(`Invalid schema: ${schemaValidation.errors.join(', ')}`)
303+
throw new TableRequestError(`Invalid schema: ${schemaValidation.errors.join(', ')}`)
304304
}
305305

306306
const tableId = `tbl_${generateId().replace(/-/g, '')}`
@@ -364,7 +364,7 @@ export async function createTable(
364364
)
365365

366366
if (Number(existingCount) >= maxTables) {
367-
throw new Error(`Workspace has reached maximum table limit (${maxTables})`)
367+
throw new TableRequestError(`Workspace has reached maximum table limit (${maxTables})`)
368368
}
369369

370370
const duplicateName = await trx
@@ -510,59 +510,82 @@ export function buildAddedColumn(
510510
}
511511
}
512512

513-
export async function addTableColumnsWithTx(
514-
trx: DbTransaction,
515-
table: TableDefinition,
513+
/**
514+
* Validates a batch of new columns and returns what to persist.
515+
*
516+
* Extracted and exported so the validation is testable without a transaction —
517+
* the same reason `buildAddedColumn` is. Every failure here is caller-fixable,
518+
* so each raises `TableRequestError` rather than a plain `Error`: the single
519+
* -column path was migrated first, and while this one lagged, an invalid type
520+
* or the column cap fell past the import route's substring list and surfaced as
521+
* a 500 with the message swallowed.
522+
*/
523+
export function buildAddedColumns(
524+
table: Pick<TableDefinition, 'schema'>,
516525
columns: (ColumnTypeMetadata & {
517526
id?: string
518527
name: string
519528
type: string
520529
required?: boolean
521530
unique?: boolean
522-
})[],
523-
requestId: string
524-
): Promise<TableDefinition> {
525-
if (columns.length === 0) return table
526-
527-
// Runs outside `withLockedTable` (reachable from CSV import with new
528-
// headers), so it must assert directly.
529-
assertSchemaMutable(table)
530-
531+
})[]
532+
): TableSchema['columns'] {
531533
const usedNames = new Set(table.schema.columns.map((c) => c.name.toLowerCase()))
532534
const additions: TableSchema['columns'] = []
533535

534536
for (const column of columns) {
535537
if (!NAME_PATTERN.test(column.name)) {
536-
throw new Error(
538+
throw new TableRequestError(
537539
`Invalid column name "${column.name}". Must start with a letter or underscore and contain only alphanumeric characters and underscores.`
538540
)
539541
}
540542
if (column.name.length > TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH) {
541-
throw new Error(
543+
throw new TableRequestError(
542544
`Column name exceeds maximum length (${TABLE_LIMITS.MAX_COLUMN_NAME_LENGTH} characters)`
543545
)
544546
}
545547
if (!COLUMN_TYPES.includes(column.type as (typeof COLUMN_TYPES)[number])) {
546-
throw new Error(
548+
throw new TableRequestError(
547549
`Invalid column type "${column.type}". Must be one of: ${COLUMN_TYPES.join(', ')}`
548550
)
549551
}
550552
const lower = column.name.toLowerCase()
551553
if (usedNames.has(lower)) {
552-
throw new Error(`Column "${column.name}" already exists`)
554+
throw new TableRequestError(`Column "${column.name}" already exists`)
553555
}
554556
usedNames.add(lower)
555557
// Honor a caller-assigned id (the CSV append path pre-assigns so coercion
556558
// and persistence agree); otherwise mint one.
557-
const id = column.id ?? generateColumnId()
558-
additions.push(buildAddedColumn(column, id))
559+
additions.push(buildAddedColumn(column, column.id ?? generateColumnId()))
559560
}
560561

561562
if (table.schema.columns.length + additions.length > TABLE_LIMITS.MAX_COLUMNS_PER_TABLE) {
562-
throw new Error(
563+
throw new TableRequestError(
563564
`Adding ${additions.length} column(s) would exceed maximum column limit (${TABLE_LIMITS.MAX_COLUMNS_PER_TABLE})`
564565
)
565566
}
567+
return additions
568+
}
569+
570+
export async function addTableColumnsWithTx(
571+
trx: DbTransaction,
572+
table: TableDefinition,
573+
columns: (ColumnTypeMetadata & {
574+
id?: string
575+
name: string
576+
type: string
577+
required?: boolean
578+
unique?: boolean
579+
})[],
580+
requestId: string
581+
): Promise<TableDefinition> {
582+
if (columns.length === 0) return table
583+
584+
// Runs outside `withLockedTable` (reachable from CSV import with new
585+
// headers), so it must assert directly.
586+
assertSchemaMutable(table)
587+
588+
const additions = buildAddedColumns(table, columns)
566589

567590
// Spread `table.schema` first so workflow groups (and any future top-level
568591
// schema fields) survive a CSV import that only adds plain columns.
@@ -630,7 +653,7 @@ export async function renameTable(
630653
): Promise<{ id: string; name: string }> {
631654
const nameValidation = validateTableName(newName)
632655
if (!nameValidation.valid) {
633-
throw new Error(nameValidation.errors.join(', '))
656+
throw new TableRequestError(nameValidation.errors.join(', '))
634657
}
635658

636659
const now = new Date()
@@ -646,7 +669,7 @@ export async function renameTable(
646669
})
647670

648671
if (result.length === 0) {
649-
throw new Error(`Table ${tableId} not found`)
672+
throw tableNotFound(`Table ${tableId} not found`)
650673
}
651674

652675
const { createdBy, workspaceId } = result[0]
@@ -723,7 +746,7 @@ export async function moveTableToFolder(
723746
})
724747

725748
if (result.length === 0) {
726-
throw new Error(`Table ${tableId} not found`)
749+
throw tableNotFound(`Table ${tableId} not found`)
727750
}
728751

729752
const { name, createdBy } = result[0]
@@ -977,14 +1000,14 @@ export async function restoreTable(
9771000
}
9781001

9791002
if (!table.archivedAt) {
980-
throw new Error('Table is not archived')
1003+
throw new TableRequestError('Table is not archived')
9811004
}
9821005

9831006
if (table.workspaceId) {
9841007
const { getWorkspaceWithOwner } = await import('@/lib/workspaces/permissions/utils')
9851008
const ws = await getWorkspaceWithOwner(table.workspaceId)
9861009
if (!ws || ws.archivedAt) {
987-
throw new Error('Cannot restore table into an archived workspace')
1010+
throw new TableRequestError('Cannot restore table into an archived workspace')
9881011
}
9891012
}
9901013

0 commit comments

Comments
 (0)