Skip to content

Commit 7496079

Browse files
committed
fix(tables): typed column errors, contact CSV inference, decimal parsing
Three follow-ups from the residual audit. Routes decided 400-vs-500 by matching substrings of the error message, a list that had to grow with every new message and silently mis-classified anything it missed. The metadata-ownership error ("Cannot set precision on column …") matched nothing and returned a 500, telling the caller the server broke when their request was invalid. The service now raises `TableColumnError` carrying its own status; all 38 throws in the column service and the shared "Table not found" are migrated, and both routes replace the lists with one typed check. An untyped throw now correctly means a genuine internal error. CSV import never inferred Email or Phone, so a contacts file arrived as text. Email is inferred through the type's OWN coerce, so inference and the write path cannot disagree. Phone is deliberately inferred only on strong evidence — every value carrying a `+` or a parenthesised area code — because ISBNs, SKUs and part numbers are dash-separated digit runs too, and a phone column keeps only the digits, so a wrong guess drops their punctuation silently. A column of bare digits still infers Number, which is right for an ID column. `Number()` accepts far more than a decimal number: `0x10` read as 16, `0b11` as 3, `Infinity` as infinity. Someone typing `0x10` means the text, so Number and Percent now parse through a shared decimal parser. Exponent notation stays accepted — spreadsheets export it — while the alternate bases are refused.
1 parent 13868b9 commit 7496079

12 files changed

Lines changed: 207 additions & 113 deletions

File tree

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { hybridAuthMockFns } from '@sim/testing'
1212
import { getErrorMessage } from '@sim/utils/errors'
1313
import { NextRequest } from 'next/server'
1414
import { beforeEach, describe, expect, it, vi } from 'vitest'
15+
import { TableColumnError } from '@/lib/table/errors'
1516

1617
const {
1718
mockCheckAccess,
@@ -160,9 +161,12 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
160161
},
161162
})
162163
// Stands in for the race the guards cannot close: the column stopped being
163-
// a currency between the snapshot the guards read and this write.
164+
// a currency between the snapshot the guards read and this write. The
165+
// service raises `TableColumnError` for a caller-fixable failure, which is
166+
// what earns the 400 — a plain `Error` now means the server genuinely broke
167+
// and correctly returns 500.
164168
mockUpdateColumnMetadata.mockRejectedValue(
165-
new Error('Cannot set currency on column "amount" of type "string"')
169+
new TableColumnError('Cannot set currency on column "amount" of type "string"')
166170
)
167171

168172
const response = await patch({ name: 'renamed', currencyCode: 'USD' })

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

Lines changed: 9 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
2323
import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types'
2424
import { validateMetadataUpdate } from '@/lib/table/columns/metadata'
25+
import { TableColumnError } from '@/lib/table/errors'
2526
import { signalTableSchemaChanged } from '@/lib/table/events'
2627
import {
2728
accessError,
@@ -78,18 +79,10 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Colum
7879
return validationErrorResponse(error, 'Invalid request data')
7980
}
8081

81-
const msg = rootErrorMessage(error)
82-
if (
83-
msg.includes('already exists') ||
84-
msg.includes('maximum column') ||
85-
msg.includes('Invalid column') ||
86-
msg.includes('exceeds maximum') ||
87-
msg.includes('option')
88-
) {
89-
return NextResponse.json({ error: msg }, { status: 400 })
90-
}
91-
if (msg === 'Table not found') {
92-
return NextResponse.json({ error: msg }, { status: 404 })
82+
// One typed check instead of a per-message substring list: the service
83+
// says whether a failure is the caller's and what status it deserves.
84+
if (error instanceof TableColumnError) {
85+
return NextResponse.json({ error: error.message }, { status: error.status })
9386
}
9487

9588
logger.error(`[${requestId}] Error adding column to table ${tableId}:`, error)
@@ -305,24 +298,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
305298
return validationErrorResponse(error, 'Invalid request data')
306299
}
307300

308-
const msg = rootErrorMessage(error)
309-
if (msg.includes('not found') || msg.includes('Table not found')) {
310-
return NextResponse.json({ error: msg }, { status: 404 })
311-
}
312-
if (
313-
msg.includes('already exists') ||
314-
msg.includes('Cannot delete the last column') ||
315-
msg.includes('Cannot set column') ||
316-
msg.includes('Cannot set unique column') ||
317-
msg.includes('Invalid column') ||
318-
msg.includes('exceeds maximum') ||
319-
msg.includes('incompatible') ||
320-
msg.includes('duplicate') ||
321-
msg.includes('option') ||
322-
msg.includes('currency') ||
323-
msg.includes('is already type')
324-
) {
325-
return NextResponse.json({ error: msg }, { status: 400 })
301+
// One typed check instead of a per-message substring list: the service
302+
// says whether a failure is the caller's and what status it deserves.
303+
if (error instanceof TableColumnError) {
304+
return NextResponse.json({ error: error.message }, { status: error.status })
326305
}
327306

328307
logger.error(`[${requestId}] Error updating column in table ${tableId}:`, error)

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -428,9 +428,9 @@ describe('POST /api/table/[tableId]/import', () => {
428428
)
429429
expect(response.status).toBe(200)
430430
expect(mockImportAppendRows).toHaveBeenCalledTimes(1)
431-
expect(appendAdditions()).toEqual([
432-
expect.objectContaining({ name: 'email', type: 'string' }),
433-
])
431+
// Inferred as `email`, not `string`: the fixture values are real
432+
// addresses, and CSV inference now recognises them.
433+
expect(appendAdditions()).toEqual([expect.objectContaining({ name: 'email', type: 'email' })])
434434
// Existing columns have no id (legacy) → keyed by name; the new `email`
435435
// column was assigned id `col_deadbeefcafef00d` (mocked generateId).
436436
expect(appendRows()).toEqual([
@@ -473,7 +473,7 @@ describe('POST /api/table/[tableId]/import', () => {
473473
)
474474
expect(response.status).toBe(200)
475475
expect(appendAdditions()).toEqual([
476-
expect.objectContaining({ name: 'Email_2', type: 'string' }),
476+
expect.objectContaining({ name: 'Email_2', type: 'email' }),
477477
])
478478
})
479479

@@ -537,9 +537,9 @@ describe('POST /api/table/[tableId]/import', () => {
537537
})
538538
)
539539
// Route forwarded the column addition into the (now atomic) import op.
540-
expect(appendAdditions()).toEqual([
541-
expect.objectContaining({ name: 'email', type: 'string' }),
542-
])
540+
// Inferred as `email`, not `string`: the fixture values are real
541+
// addresses, and CSV inference now recognises them.
542+
expect(appendAdditions()).toEqual([expect.objectContaining({ name: 'email', type: 'email' })])
543543
expect(response.status).toBe(400)
544544
const data = await response.json()
545545
expect(data.success).toBeUndefined()

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

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
2222
import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types'
2323
import { validateMetadataUpdate } from '@/lib/table/columns/metadata'
24+
import { TableColumnError } from '@/lib/table/errors'
2425
import { signalTableSchemaChanged } from '@/lib/table/events'
2526
import {
2627
accessError,
@@ -350,25 +351,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
350351
const validationResponse = v1ValidationErrorResponseFromError(error)
351352
if (validationResponse) return validationResponse
352353

353-
if (error instanceof Error) {
354-
const msg = error.message
355-
if (msg.includes('not found') || msg.includes('Table not found')) {
356-
return NextResponse.json({ error: msg }, { status: 404 })
357-
}
358-
if (
359-
msg.includes('already exists') ||
360-
msg.includes('Cannot delete the last column') ||
361-
msg.includes('Cannot set column') ||
362-
msg.includes('Invalid column') ||
363-
msg.includes('exceeds maximum') ||
364-
msg.includes('incompatible') ||
365-
msg.includes('duplicate') ||
366-
msg.includes('option') ||
367-
msg.includes('currency') ||
368-
msg.includes('is already type')
369-
) {
370-
return NextResponse.json({ error: msg }, { status: 400 })
371-
}
354+
// One typed check instead of a per-message substring list: the service
355+
// says whether a failure is the caller's and what status it deserves.
356+
if (error instanceof TableColumnError) {
357+
return NextResponse.json({ error: error.message }, { status: error.status })
372358
}
373359

374360
logger.error(`[${requestId}] Error updating column in table:`, error)

apps/sim/lib/table/__tests__/column-types-contact.test.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
pickMetadata,
2121
} from '@/lib/table/column-types'
2222
import { buildConvertedColumn } from '@/lib/table/columns/service'
23-
import { coerceValue } from '@/lib/table/import'
23+
import { coerceValue, inferColumnType } from '@/lib/table/import'
2424
import { filterRulesToFilter, prunePredicateForColumns } from '@/lib/table/query-builder/converters'
2525
import type { ColumnDefinition } from '@/lib/table/types'
2626

@@ -409,6 +409,40 @@ describe('review-round-7 regressions', () => {
409409
})
410410
})
411411

412+
describe('follow-up fixes', () => {
413+
it('reads a numeric cell as decimal, not as an alternate base', () => {
414+
// `Number()` reads `0x10` as 16 and `0b11` as 3. A user typing `0x10`
415+
// means the text, so parsing it that way stored a value never entered.
416+
for (const type of ['number', 'percent'] as const) {
417+
for (const input of ['0x10', '0b11', '0o17', 'Infinity', '1e400']) {
418+
expect(COLUMN_TYPE_REGISTRY[type].coerce(input, column(type)).ok, `${type} ${input}`).toBe(
419+
false
420+
)
421+
}
422+
// Exponent notation stays accepted — spreadsheets export it.
423+
const exp = COLUMN_TYPE_REGISTRY[type].coerce('1e3', column(type))
424+
expect(exp.ok && exp.value).toBe(1000)
425+
const neg = COLUMN_TYPE_REGISTRY[type].coerce('-2.5', column(type))
426+
expect(neg.ok && neg.value).toBe(-2.5)
427+
}
428+
})
429+
430+
it('infers an email column from a CSV, using the type’s own validation', () => {
431+
expect(inferColumnType(['ada@example.com', 'bob@example.co.uk'])).toBe('email')
432+
expect(inferColumnType(['ada@example.com', 'not an address'])).toBe('string')
433+
})
434+
435+
it('infers phone only on strong evidence, never from bare digits or dashes', () => {
436+
expect(inferColumnType(['+1 555 123 4567', '+44 20 7123 4567'])).toBe('phone')
437+
expect(inferColumnType(['(555) 123-4567', '(555) 987-6543'])).toBe('phone')
438+
// An ID column of bare digits is a Number, which is the right answer.
439+
expect(inferColumnType(['5551234567', '5559876543'])).toBe('number')
440+
// Dash-separated digit runs are ISBNs and SKUs as often as phone numbers,
441+
// and a phone column keeps only the digits — so these stay text.
442+
expect(inferColumnType(['978-0-13-235088-4', '978-0-32-135668-0'])).toBe('string')
443+
})
444+
})
445+
412446
describe('date includeTime', () => {
413447
it('truncates to a calendar day only when includeTime is explicitly false', () => {
414448
const dateOnly = column('date', { includeTime: false })

apps/sim/lib/table/column-types/number.ts

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { TypeNumber } from '@sim/emcn/icons'
22
import type { ColumnTypeDefinition } from '@/lib/table/column-types/types'
33
import { ownedKeysOf } from '@/lib/table/column-types/types'
4+
import { parseDecimalNumber } from '@/lib/table/numeric'
45
import { clampPrecision, DEFAULT_PRECISION, formatWithPrecision } from '@/lib/table/precision'
56

67
export const numberColumnType: ColumnTypeDefinition = {
@@ -22,14 +23,8 @@ export const numberColumnType: ColumnTypeDefinition = {
2223
parseErrorMessage: 'Invalid number',
2324

2425
coerce(value) {
25-
if (typeof value === 'number') {
26-
return Number.isFinite(value) ? { ok: true, value } : { ok: false }
27-
}
28-
if (typeof value === 'string' && value.trim() !== '') {
29-
const parsed = Number(value)
30-
return Number.isFinite(parsed) ? { ok: true, value: parsed } : { ok: false }
31-
}
32-
return { ok: false }
26+
const parsed = parseDecimalNumber(value)
27+
return parsed === null ? { ok: false } : { ok: true, value: parsed }
3328
},
3429

3530
validateCell(value, column) {

apps/sim/lib/table/column-types/percent.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { TypePercent } from '@sim/emcn/icons'
22
import type { ColumnTypeDefinition } from '@/lib/table/column-types/types'
33
import { ownedKeysOf } from '@/lib/table/column-types/types'
4+
import { parseDecimalNumber } from '@/lib/table/numeric'
45
import { clampPrecision, DEFAULT_PRECISION, formatWithPrecision } from '@/lib/table/precision'
56

67
/**
@@ -12,12 +13,9 @@ import { clampPrecision, DEFAULT_PRECISION, formatWithPrecision } from '@/lib/ta
1213
* written against either column means the same thing.
1314
*/
1415
function parsePercent(value: unknown): number | null {
15-
if (typeof value === 'number') return Number.isFinite(value) ? value : null
16+
if (typeof value === 'number') return parseDecimalNumber(value)
1617
if (typeof value !== 'string') return null
17-
const trimmed = value.trim().replace(/%$/, '').trim()
18-
if (trimmed === '') return null
19-
const parsed = Number(trimmed)
20-
return Number.isFinite(parsed) ? parsed : null
18+
return parseDecimalNumber(value.trim().replace(/%$/, ''))
2119
}
2220

2321
export const percentColumnType: ColumnTypeDefinition = {

0 commit comments

Comments
 (0)