Skip to content

Commit cb8994d

Browse files
committed
fix(tables): share one inferred-column builder across every import path
The append route and the import runner each had their own inline copy of "infer a type, build a column", so the `includeTime: true` added to the create-from-CSV path never reached them. An appended date column was persisted DATE-ONLY while its rows were coerced WITH their times — the schema and the data disagreeing about the same column, which then shows up as wrong display, wrong filters, and truncation on the next write. Fixing the third copy in place would have left a fourth: the import runner had the same inline call. The decision now lives in `inferredColumnDefinition`, and all four sites call it — because the inferred TYPE is not the whole answer, and splitting the two halves across call sites is what let them drift. Pinned by asserting the entry points agree with each other rather than restating the expected value in each, plus that the inferred column survives persistence so the stored shape matches how its rows were coerced.
1 parent 4d42a81 commit cb8994d

4 files changed

Lines changed: 98 additions & 23 deletions

File tree

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import {
2727
dispatchAfterBatchInsert,
2828
generateColumnId,
2929
getMaxRowsPerTable,
30-
inferColumnType,
30+
inferredColumnDefinition,
3131
markTableJobRunning,
3232
releaseJobClaim,
3333
sanitizeName,
@@ -230,15 +230,21 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
230230
suffix++
231231
}
232232
usedNames.add(columnName.toLowerCase())
233-
const inferredType = inferColumnType(rows.map((r) => r[header]))
233+
// Same helper the create-from-CSV path uses, so the two cannot answer
234+
// differently. Inlining just the TYPE here persisted an appended date
235+
// column as date-only while its rows were coerced with their times.
236+
const inferred = inferredColumnDefinition(
237+
columnName,
238+
rows.map((r) => r[header])
239+
)
234240
// Pre-assign the id so the prospective schema (used to coerce rows) and
235241
// the persisted column (created in importAppendRows) share the same key.
236242
const id = generateColumnId()
237-
additions.push({ id, name: columnName, type: inferredType })
243+
additions.push({ ...inferred, id })
238244
newColumns.push({
245+
...inferred,
239246
id,
240-
name: columnName,
241-
type: inferredType as TableSchema['columns'][number]['type'],
247+
type: inferred.type as TableSchema['columns'][number]['type'],
242248
required: false,
243249
unique: false,
244250
})

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

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,13 @@ import { describe, expect, it } from 'vitest'
1919
import { COLUMN_TYPE_REGISTRY } from '@/lib/table/column-types'
2020
import { TABLE_LIMITS } from '@/lib/table/constants'
2121
import { TableRequestError, tableNotFound } from '@/lib/table/errors'
22-
import { coerceValue, inferColumnType } from '@/lib/table/import'
23-
import { buildAddedColumns } from '@/lib/table/service'
22+
import {
23+
coerceValue,
24+
inferColumnType,
25+
inferredColumnDefinition,
26+
inferSchemaFromCsv,
27+
} from '@/lib/table/import'
28+
import { buildAddedColumn, buildAddedColumns } from '@/lib/table/service'
2429
import { buildFilterClause, buildSortClause } from '@/lib/table/sql'
2530
import type { ColumnDefinition, Filter, JsonValue, Sort } from '@/lib/table/types'
2631
import { validateColumnDefinition } from '@/lib/table/validation'
@@ -302,3 +307,44 @@ describe('caller-fixable failures are typed, not string-matched', () => {
302307
expect(new TableRequestError('anything').status).toBe(400)
303308
})
304309
})
310+
311+
describe('every import path builds an inferred column identically', () => {
312+
const withTimes = [{ when: '2024-01-15T14:30' }, { when: '2024-02-20T09:00' }]
313+
314+
it('gives an inferred date column includeTime through the shared helper', () => {
315+
const column = inferredColumnDefinition(
316+
'when',
317+
withTimes.map((r) => r.when)
318+
)
319+
expect(column.type).toBe('date')
320+
expect(column.includeTime).toBe(true)
321+
})
322+
323+
it('agrees with inferSchemaFromCsv, which is the other entry point', () => {
324+
// Three call sites built this inline and drifted: the append route and the
325+
// import runner persisted an appended date column as date-only while its
326+
// rows were coerced WITH their times.
327+
const viaSchema = inferSchemaFromCsv(['when'], withTimes).columns[0]
328+
const viaHelper = inferredColumnDefinition(
329+
'when',
330+
withTimes.map((r) => r.when)
331+
)
332+
expect(viaSchema).toEqual(viaHelper)
333+
})
334+
335+
it('survives persistence, so the stored column matches how rows were coerced', () => {
336+
const inferred = inferredColumnDefinition(
337+
'when',
338+
withTimes.map((r) => r.when)
339+
)
340+
const stored = buildAddedColumn(inferred, 'col_1')
341+
expect(stored.includeTime).toBe(true)
342+
// And a value with a time round-trips rather than being truncated.
343+
expect(String(coerceValue('2024-01-15T14:30', 'date', { column: stored }))).toContain('14:30')
344+
})
345+
346+
it('adds no date metadata to a non-date inferred column', () => {
347+
expect(inferredColumnDefinition('n', ['1', '2']).includeTime).toBeUndefined()
348+
expect(inferredColumnDefinition('s', ['a', 'b']).type).toBe('string')
349+
})
350+
})

apps/sim/lib/table/import-runner.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {
1111
type CsvHeaderMapping,
1212
coerceRowsForTable,
1313
createCsvParser,
14-
inferColumnType,
14+
inferredColumnDefinition,
1515
inferSchemaFromCsv,
1616
sanitizeName,
1717
type TableSchema,
@@ -217,7 +217,14 @@ export async function runTableImport(payload: TableImportPayload): Promise<void>
217217
suffix++
218218
}
219219
usedNames.add(columnName.toLowerCase())
220-
additions.push({ name: columnName, type: inferColumnType(sample.map((r) => r[header])) })
220+
// Shared with both import routes, so an appended date column is
221+
// never persisted date-only while its rows carry times.
222+
additions.push(
223+
inferredColumnDefinition(
224+
columnName,
225+
sample.map((r) => r[header])
226+
)
227+
)
221228
updatedMapping[header] = columnName
222229
}
223230
const updated = await addImportColumns(

apps/sim/lib/table/import.ts

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,32 @@ export async function parseCsvBuffer(
292292
* column would also have to guess an ISO code from a symbol, and guessing wrong
293293
* mislabels every amount in the column.
294294
*/
295+
/**
296+
* The column definition an import creates for a header, from its sampled values.
297+
*
298+
* The single place that turns an inferred type into a persisted column, because
299+
* the type alone is not the whole answer. Both import entry points call it —
300+
* creating a table from a CSV, and appending a CSV that introduces new headers
301+
* — and when the second had its own inline copy, an appended date column was
302+
* persisted date-only while its rows were coerced WITH their times.
303+
*/
304+
export function inferredColumnDefinition(name: string, values: unknown[]): ColumnDefinition {
305+
const type = inferColumnType(values)
306+
return {
307+
name,
308+
type,
309+
// An inferred date column keeps its times. The pattern that infers `date`
310+
// accepts `2024-01-15T14:30`, so the file may genuinely contain them, and
311+
// stamping the sidebar's date-only default would truncate every one of them
312+
// on write. Set explicitly rather than left absent, so a fresh column is
313+
// never in the "predates the key" state.
314+
//
315+
// Deliberately different from creating a Date column by hand: there the
316+
// user sees the toggle and there is no data to lose yet.
317+
...(type === 'date' ? { includeTime: true } : {}),
318+
}
319+
}
320+
295321
export function inferColumnType(values: unknown[]): InferredCsvColumnType {
296322
const nonEmpty = values.filter((v) => v !== null && v !== undefined && v !== '')
297323
if (nonEmpty.length === 0) return 'string'
@@ -372,20 +398,10 @@ export function inferSchemaFromCsv(
372398
seen.add(colName.toLowerCase())
373399
headerToColumn.set(header, colName)
374400

375-
const type = inferColumnType(sample.map((r) => r[header]))
376-
return {
377-
name: colName,
378-
type,
379-
// An inferred date column keeps its times. The pattern that infers `date`
380-
// accepts `2024-01-15T14:30`, so the file may genuinely contain them —
381-
// and stamping the sidebar's date-only default here would truncate every
382-
// one of them on write. Set explicitly rather than left absent so a fresh
383-
// column is never in the "predates the key" state.
384-
//
385-
// Deliberately different from creating a Date column by hand: there the
386-
// user sees the toggle and there is no data to lose yet.
387-
...(type === 'date' ? { includeTime: true } : {}),
388-
} satisfies ColumnDefinition
401+
return inferredColumnDefinition(
402+
colName,
403+
sample.map((r) => r[header])
404+
)
389405
})
390406

391407
return { columns, headerToColumn }

0 commit comments

Comments
 (0)