mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(tables): coerce row values to column types on write instead of failing (#4761)
* fix(tables): coerce row values to column types on write instead of failing * fix(tables): persist coerced values in upsert match + bulk update, normalize Date to ISO * fix(tables): guard date coercion against out-of-range values
This commit is contained in:
@@ -34,6 +34,8 @@ vi.mock('@/lib/table/workflow-columns', () => ({
|
||||
vi.mock('@/lib/table/validation', () => ({
|
||||
validateRowSize: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
validateRowAgainstSchema: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
coerceRowToSchema: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
coerceRowValues: vi.fn(),
|
||||
validateTableName: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
validateTableSchema: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
getUniqueColumns: vi.fn(() => []),
|
||||
|
||||
@@ -20,6 +20,8 @@ vi.mock('@sim/db', () => dbChainMock)
|
||||
vi.mock('@/lib/table/validation', () => ({
|
||||
validateRowSize: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
validateRowAgainstSchema: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
coerceRowToSchema: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
coerceRowValues: vi.fn(),
|
||||
validateTableName: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
validateTableSchema: vi.fn(() => ({ valid: true, errors: [] })),
|
||||
getUniqueColumns: vi.fn(() => []),
|
||||
|
||||
@@ -5,6 +5,8 @@ import { describe, expect, it } from 'vitest'
|
||||
import { TABLE_LIMITS } from '../constants'
|
||||
import {
|
||||
type ColumnDefinition,
|
||||
coerceRowToSchema,
|
||||
coerceRowValues,
|
||||
getUniqueColumns,
|
||||
type TableSchema,
|
||||
validateColumnDefinition,
|
||||
@@ -277,6 +279,127 @@ describe('Validation', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('coerceRowToSchema', () => {
|
||||
const schema: TableSchema = {
|
||||
columns: [
|
||||
{ name: 'name', type: 'string', required: true },
|
||||
{ name: 'age', type: 'number' },
|
||||
{ name: 'founded', type: 'number', required: true },
|
||||
{ name: 'active', type: 'boolean' },
|
||||
{ name: 'created', type: 'date' },
|
||||
{ name: 'metadata', type: 'json' },
|
||||
],
|
||||
}
|
||||
|
||||
it('coerces a numeric string to a number in place', () => {
|
||||
const data = { name: 'Acme', founded: '1999' }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.founded).toBe(1999)
|
||||
})
|
||||
|
||||
it('nulls an un-coercible value for an optional number column', () => {
|
||||
const data = { name: 'Acme', founded: 2000, age: 'unknown' }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.age).toBeNull()
|
||||
})
|
||||
|
||||
it('rejects an un-coercible value for a required number column', () => {
|
||||
const data = { name: 'Acme', founded: 'unknown' }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(false)
|
||||
expect(result.errors[0]).toContain('founded must be number')
|
||||
expect(data.founded).toBe('unknown')
|
||||
})
|
||||
|
||||
it('coerces a number to a string for a string column', () => {
|
||||
const data = { name: 12345, founded: 2000 }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.name).toBe('12345')
|
||||
})
|
||||
|
||||
it('coerces "true"/"false" strings to booleans', () => {
|
||||
const data = { name: 'Acme', founded: 2000, active: 'false' }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.active).toBe(false)
|
||||
})
|
||||
|
||||
it('coerces an epoch number to an ISO date string', () => {
|
||||
const epoch = Date.parse('2024-01-15T00:00:00Z')
|
||||
const data = { name: 'Acme', founded: 2000, created: epoch }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.created).toBe(new Date(epoch).toISOString())
|
||||
})
|
||||
|
||||
it('coerces a Date instance to an ISO date string', () => {
|
||||
const date = new Date('2024-01-15T00:00:00Z')
|
||||
const data = { name: 'Acme', founded: 2000, created: date }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.created).toBe(date.toISOString())
|
||||
})
|
||||
|
||||
it('nulls an out-of-range epoch number for an optional date column without throwing', () => {
|
||||
const data = { name: 'Acme', founded: 2000, created: 1e20 }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.created).toBeNull()
|
||||
})
|
||||
|
||||
it('nulls an invalid Date instance for an optional date column without throwing', () => {
|
||||
const data = { name: 'Acme', founded: 2000, created: new Date('not-a-date') }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data.created).toBeNull()
|
||||
})
|
||||
|
||||
it('leaves already-correct values untouched and passes through json', () => {
|
||||
const data = { name: 'Acme', founded: 2000, metadata: { k: 'v' } }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(true)
|
||||
expect(data).toEqual({ name: 'Acme', founded: 2000, metadata: { k: 'v' } })
|
||||
})
|
||||
|
||||
it('still rejects a missing required field', () => {
|
||||
const data = { name: 'Acme' }
|
||||
const result = coerceRowToSchema(data, schema)
|
||||
expect(result.valid).toBe(false)
|
||||
expect(result.errors).toContain('Missing required field: founded')
|
||||
})
|
||||
})
|
||||
|
||||
describe('coerceRowValues', () => {
|
||||
const schema: TableSchema = {
|
||||
columns: [
|
||||
{ name: 'name', type: 'string', required: true },
|
||||
{ name: 'founded', type: 'number', required: true },
|
||||
{ name: 'age', type: 'number' },
|
||||
],
|
||||
}
|
||||
|
||||
it('coerces a partial patch in place without flagging absent required fields', () => {
|
||||
const patch = { age: '42' }
|
||||
coerceRowValues(patch, schema)
|
||||
expect(patch.age).toBe(42)
|
||||
})
|
||||
|
||||
it('nulls an un-coercible optional value in a patch', () => {
|
||||
const patch: { age: unknown } = { age: 'nope' }
|
||||
coerceRowValues(patch as never, schema)
|
||||
expect(patch.age).toBeNull()
|
||||
})
|
||||
|
||||
it('leaves an un-coercible required value in place for downstream validation', () => {
|
||||
const patch: { founded: unknown } = { founded: 'nope' }
|
||||
coerceRowValues(patch as never, schema)
|
||||
expect(patch.founded).toBe('nope')
|
||||
})
|
||||
})
|
||||
|
||||
describe('getUniqueColumns', () => {
|
||||
it('should return only columns with unique=true', () => {
|
||||
const schema: TableSchema = {
|
||||
|
||||
@@ -61,8 +61,9 @@ import type {
|
||||
import {
|
||||
checkBatchUniqueConstraintsDb,
|
||||
checkUniqueConstraintsDb,
|
||||
coerceRowToSchema,
|
||||
coerceRowValues,
|
||||
getUniqueColumns,
|
||||
validateRowAgainstSchema,
|
||||
validateRowSize,
|
||||
validateTableName,
|
||||
validateTableSchema,
|
||||
@@ -913,7 +914,7 @@ export async function insertRow(
|
||||
}
|
||||
|
||||
// Validate against schema
|
||||
const schemaValidation = validateRowAgainstSchema(data.data, table.schema)
|
||||
const schemaValidation = coerceRowToSchema(data.data, table.schema)
|
||||
if (!schemaValidation.valid) {
|
||||
throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`)
|
||||
}
|
||||
@@ -1060,7 +1061,7 @@ export async function batchInsertRowsWithTx(
|
||||
throw new Error(`Row ${i + 1}: ${sizeValidation.errors.join(', ')}`)
|
||||
}
|
||||
|
||||
const schemaValidation = validateRowAgainstSchema(row, table.schema)
|
||||
const schemaValidation = coerceRowToSchema(row, table.schema)
|
||||
if (!schemaValidation.valid) {
|
||||
throw new Error(`Row ${i + 1}: ${schemaValidation.errors.join(', ')}`)
|
||||
}
|
||||
@@ -1201,7 +1202,7 @@ export async function replaceTableRowsWithTx(
|
||||
throw new Error(`Row ${i + 1}: ${sizeValidation.errors.join(', ')}`)
|
||||
}
|
||||
|
||||
const schemaValidation = validateRowAgainstSchema(row, table.schema)
|
||||
const schemaValidation = coerceRowToSchema(row, table.schema)
|
||||
if (!schemaValidation.valid) {
|
||||
throw new Error(`Row ${i + 1}: ${schemaValidation.errors.join(', ')}`)
|
||||
}
|
||||
@@ -1331,22 +1332,24 @@ export async function upsertRow(
|
||||
)
|
||||
}
|
||||
|
||||
const targetValue = data.data[targetColumnName]
|
||||
if (targetValue === undefined || targetValue === null) {
|
||||
throw new Error(`Upsert requires a value for the conflict target column "${targetColumnName}"`)
|
||||
}
|
||||
|
||||
// Validate row data
|
||||
const sizeValidation = validateRowSize(data.data)
|
||||
if (!sizeValidation.valid) {
|
||||
throw new Error(sizeValidation.errors.join(', '))
|
||||
}
|
||||
|
||||
const schemaValidation = validateRowAgainstSchema(data.data, schema)
|
||||
const schemaValidation = coerceRowToSchema(data.data, schema)
|
||||
if (!schemaValidation.valid) {
|
||||
throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`)
|
||||
}
|
||||
|
||||
// Read the conflict-target value *after* coercion so `matchFilter` branches on
|
||||
// the persisted type (e.g. a coerced `"123"` → `123` matches existing rows).
|
||||
const targetValue = data.data[targetColumnName]
|
||||
if (targetValue === undefined || targetValue === null) {
|
||||
throw new Error(`Upsert requires a value for the conflict target column "${targetColumnName}"`)
|
||||
}
|
||||
|
||||
// `data->` and `data->>` accept the JSON key as a parameterized text value;
|
||||
// no need for `sql.raw` interpolation.
|
||||
const matchFilter =
|
||||
@@ -1957,7 +1960,7 @@ export async function updateRow(
|
||||
}
|
||||
|
||||
// Validate against schema
|
||||
const schemaValidation = validateRowAgainstSchema(mergedData, table.schema)
|
||||
const schemaValidation = coerceRowToSchema(mergedData, table.schema)
|
||||
if (!schemaValidation.valid) {
|
||||
throw new Error(`Schema validation failed: ${schemaValidation.errors.join(', ')}`)
|
||||
}
|
||||
@@ -2167,6 +2170,12 @@ export async function updateRowsByFilter(
|
||||
return { affectedCount: 0, affectedRowIds: [] }
|
||||
}
|
||||
|
||||
// Coerce the patch itself in place — the write below persists `data.data`
|
||||
// (as `patchJson`), so coercing only the per-row merged copies would be
|
||||
// discarded. The merged validation in the loop still enforces required
|
||||
// fields against the full row.
|
||||
coerceRowValues(data.data, table.schema)
|
||||
|
||||
for (const row of matchingRows) {
|
||||
const existingData = row.data as RowData
|
||||
const mergedData = { ...existingData, ...data.data }
|
||||
@@ -2176,7 +2185,7 @@ export async function updateRowsByFilter(
|
||||
throw new Error(`Row ${row.id}: ${sizeValidation.errors.join(', ')}`)
|
||||
}
|
||||
|
||||
const schemaValidation = validateRowAgainstSchema(mergedData, table.schema)
|
||||
const schemaValidation = coerceRowToSchema(mergedData, table.schema)
|
||||
if (!schemaValidation.valid) {
|
||||
throw new Error(`Row ${row.id}: ${schemaValidation.errors.join(', ')}`)
|
||||
}
|
||||
@@ -2334,7 +2343,7 @@ export async function batchUpdateRows(
|
||||
throw new Error(`Row ${update.rowId}: ${sizeValidation.errors.join(', ')}`)
|
||||
}
|
||||
|
||||
const schemaValidation = validateRowAgainstSchema(merged, table.schema)
|
||||
const schemaValidation = coerceRowToSchema(merged, table.schema)
|
||||
if (!schemaValidation.valid) {
|
||||
throw new Error(`Row ${update.rowId}: ${schemaValidation.errors.join(', ')}`)
|
||||
}
|
||||
@@ -3247,8 +3256,8 @@ export async function updateWorkflowGroup(
|
||||
|
||||
// Resolve the new leaf type for each remap so the column's declared type
|
||||
// matches what the new mapping produces. Without this, a string→number
|
||||
// remap would keep `type: 'string'` and validateRowAgainstSchema would
|
||||
// reject every backfilled value.
|
||||
// remap would keep `type: 'string'` and coerceRowToSchema would coerce
|
||||
// every backfilled value toward the wrong type.
|
||||
try {
|
||||
const [
|
||||
{ loadWorkflowFromNormalizedTables },
|
||||
|
||||
@@ -7,7 +7,7 @@ import { userTableRows } from '@sim/db/schema'
|
||||
import { and, eq, or, sql } from 'drizzle-orm'
|
||||
import { NextResponse } from 'next/server'
|
||||
import { COLUMN_TYPES, NAME_PATTERN, TABLE_LIMITS } from './constants'
|
||||
import type { ColumnDefinition, RowData, TableSchema, ValidationResult } from './types'
|
||||
import type { ColumnDefinition, JsonValue, RowData, TableSchema, ValidationResult } from './types'
|
||||
|
||||
export type { ColumnDefinition, TableSchema, ValidationResult }
|
||||
|
||||
@@ -57,7 +57,7 @@ export async function validateRowData(
|
||||
}
|
||||
}
|
||||
|
||||
const schemaValidation = validateRowAgainstSchema(rowData, schema)
|
||||
const schemaValidation = coerceRowToSchema(rowData, schema)
|
||||
if (!schemaValidation.valid) {
|
||||
return {
|
||||
valid: false,
|
||||
@@ -105,7 +105,7 @@ export async function validateBatchRows(
|
||||
continue
|
||||
}
|
||||
|
||||
const schemaValidation = validateRowAgainstSchema(rowData, schema)
|
||||
const schemaValidation = coerceRowToSchema(rowData, schema)
|
||||
if (!schemaValidation.valid) {
|
||||
errors.push({ row: i, errors: schemaValidation.errors })
|
||||
}
|
||||
@@ -255,6 +255,96 @@ export function validateRowAgainstSchema(data: RowData, schema: TableSchema): Va
|
||||
return { valid: errors.length === 0, errors }
|
||||
}
|
||||
|
||||
/**
|
||||
* Attempts to coerce a non-null value to a column's declared type. Returns the
|
||||
* coerced value when the value already matches or can be converted without
|
||||
* ambiguity (e.g. the string `"1999"` to the number `1999`), and `ok: false`
|
||||
* when no safe conversion exists.
|
||||
*/
|
||||
function coerceValueToColumnType(
|
||||
value: JsonValue,
|
||||
type: ColumnDefinition['type']
|
||||
): { ok: true; value: JsonValue } | { ok: false } {
|
||||
switch (type) {
|
||||
case 'string':
|
||||
if (typeof value === 'string') return { ok: true, value }
|
||||
if (typeof value === 'number' || typeof value === 'boolean') {
|
||||
return { ok: true, value: String(value) }
|
||||
}
|
||||
return { ok: false }
|
||||
case 'number':
|
||||
if (typeof value === 'number') {
|
||||
return Number.isFinite(value) ? { ok: true, value } : { ok: false }
|
||||
}
|
||||
if (typeof value === 'string' && value.trim() !== '') {
|
||||
const parsed = Number(value)
|
||||
return Number.isFinite(parsed) ? { ok: true, value: parsed } : { ok: false }
|
||||
}
|
||||
return { ok: false }
|
||||
case 'boolean':
|
||||
if (typeof value === 'boolean') return { ok: true, value }
|
||||
if (typeof value === 'string') {
|
||||
const normalized = value.trim().toLowerCase()
|
||||
if (normalized === 'true') return { ok: true, value: true }
|
||||
if (normalized === 'false') return { ok: true, value: false }
|
||||
}
|
||||
return { ok: false }
|
||||
case 'date': {
|
||||
if (typeof value === 'string' && !Number.isNaN(Date.parse(value))) return { ok: true, value }
|
||||
// Date instances and epoch numbers may still be out of the representable
|
||||
// range (>±8.64e15ms) — guard `toISOString()`, which throws RangeError on
|
||||
// an Invalid Date, so an over-range value degrades to `{ ok: false }`
|
||||
// rather than crashing the write.
|
||||
const date =
|
||||
value instanceof Date ? value : typeof value === 'number' ? new Date(value) : null
|
||||
if (date && !Number.isNaN(date.getTime())) return { ok: true, value: date.toISOString() }
|
||||
return { ok: false }
|
||||
}
|
||||
default:
|
||||
return { ok: true, value }
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Coerces each present value in `data` toward its column's declared type **in
|
||||
* place**. Values that already match are untouched; unambiguous conversions
|
||||
* (e.g. `"1999"` → `1999`) are applied; values that cannot be coerced are set to
|
||||
* `null` when the column is optional, or left in place when required (so a
|
||||
* subsequent {@link validateRowAgainstSchema} reports them).
|
||||
*
|
||||
* Operates per-present-column, so it is safe on a partial patch (columns absent
|
||||
* from `data` are skipped — it never invents a missing-required-field error).
|
||||
*/
|
||||
export function coerceRowValues(data: RowData, schema: TableSchema): void {
|
||||
for (const column of schema.columns) {
|
||||
const value = data[column.name]
|
||||
if (value === null || value === undefined) continue
|
||||
|
||||
const coerced = coerceValueToColumnType(value, column.type)
|
||||
if (coerced.ok) {
|
||||
data[column.name] = coerced.value
|
||||
} else if (!column.required) {
|
||||
data[column.name] = null
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Coerces a full row toward its schema **in place** (see {@link coerceRowValues})
|
||||
* then validates the result.
|
||||
*
|
||||
* This is the write-path entry point — callers that persist a complete row use
|
||||
* it instead of {@link validateRowAgainstSchema} so a single off-type field (a
|
||||
* tool returning `"unknown"` for a numeric column, say) nulls that one cell
|
||||
* rather than failing the entire row write. Callers persisting only a partial
|
||||
* patch should use {@link coerceRowValues} on the patch and validate the merged
|
||||
* row separately.
|
||||
*/
|
||||
export function coerceRowToSchema(data: RowData, schema: TableSchema): ValidationResult {
|
||||
coerceRowValues(data, schema)
|
||||
return validateRowAgainstSchema(data, schema)
|
||||
}
|
||||
|
||||
/** Validates row data size is within limits. */
|
||||
export function validateRowSize(data: RowData): ValidationResult {
|
||||
const size = JSON.stringify(data).length
|
||||
|
||||
Reference in New Issue
Block a user