fix(tables): write back coerced values on every conversion

waleedlatif1 · waleedlatif1 · commit 6c5fc2af0c00 · 2026-07-30T15:35:09.000-07:00
Round 4 findings, all real.

A conversion is allowed exactly when the target type's `coerce` accepts the
value — and `coerce` frequently TRANSFORMS it. Only `select` and `currency`
wrote the transformed value back, so a conversion to any other transforming
type left the cell holding its old bytes under the new type. Converting a
number column to `date` accepted epoch values, stored them unchanged, and then
`(data-&gt;&gt;'col')::timestamptz` failed on EVERY query against that column. I
opened this myself by defaulting `isCompatibleWith` to `coerce(...).ok`.

Fixed at the class rather than the instance: the compatibility scan now records
whatever `coerce` produced whenever it differs from what is stored, and one
generic write-back applies it. That subsumes the currency-specific migration
entirely, so it and its helpers are gone. `select` keeps its own id↔name
migrations, which are not coerce-expressible in the outbound direction. The
post-conversion column definition is built once, before the scan, so the
coercion reads the same metadata the stored value is later validated against.

Exponent parsing was ambiguous when followed by text: `1e5 EUR` read as 15.
An `e` with a digit on both sides is an exponent marker, so if the string is
not a clean numeric literal it is refused rather than guessed — the digit on
both sides is what keeps the `E` inside `12 EUR` parsing normally.

A failed rename could still leave a typed change committed. The one rename
failure a caller can cause — a name already taken — is now rejected up front,
leaving only the concurrent-collision race, which no pre-flight check can close
without spanning all writes in one transaction.
diff --git a/apps/sim/app/api/table/[tableId]/columns/route.test.ts b/apps/sim/app/api/table/[tableId]/columns/route.test.ts
@@ -133,6 +133,31 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
     expect(mockRenameColumn).not.toHaveBeenCalled()
   })
 
+  it('rejects a name already taken before any write runs', async () => {
+    mockCheckAccess.mockResolvedValue({
+      ok: true,
+      table: {
+        workspaceId: WORKSPACE_ID,
+        schema: {
+          columns: [
+            { id: 'col_a', name: 'amount', type: 'currency' },
+            { id: 'col_b', name: 'taken', type: 'string' },
+          ],
+        },
+      },
+    })
+
+    const response = await patch({ name: 'taken', currencyCode: 'EUR' })
+
+    expect(response.status).toBe(400)
+    expect(await response.json()).toMatchObject({
+      error: expect.stringContaining('already exists'),
+    })
+    // The typed write would otherwise have committed under a rename that fails.
+    expect(mockUpdateColumnCurrency).not.toHaveBeenCalled()
+    expect(mockRenameColumn).not.toHaveBeenCalled()
+  })
+
   it('rejects unique on a type that cannot carry it without renaming first', async () => {
     mockCheckAccess.mockResolvedValue({
       ok: true,
diff --git a/apps/sim/app/api/table/[tableId]/columns/route.ts b/apps/sim/app/api/table/[tableId]/columns/route.ts
@@ -157,6 +157,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
         )
       }
     }
+    // The rename runs last (see below), so a name already taken would fail after
+    // the typed write committed. This is the only rename failure a caller can
+    // cause; catching it here leaves just the concurrent-collision race, which
+    // no pre-flight check can close.
+    if (
+      updates.name &&
+      table.schema.columns.some(
+        (c) =>
+          c.name.toLowerCase() === updates.name?.toLowerCase() &&
+          !columnMatchesRef(c, validated.columnName)
+      )
+    ) {
+      return NextResponse.json(
+        { error: `Column "${updates.name}" already exists` },
+        { status: 400 }
+      )
+    }
     if (updates.unique === true && !columnTypeById(resultingType).supportsUnique) {
       return NextResponse.json(
         { error: `Cannot set a ${resultingType} column as unique` },
diff --git a/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts b/apps/sim/app/api/v1/tables/[tableId]/columns/route.ts
@@ -191,6 +191,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
         )
       }
     }
+    // The rename runs last (see below), so a name already taken would fail after
+    // the typed write committed. This is the only rename failure a caller can
+    // cause; catching it here leaves just the concurrent-collision race, which
+    // no pre-flight check can close.
+    if (
+      updates.name &&
+      table.schema.columns.some(
+        (c) =>
+          c.name.toLowerCase() === updates.name?.toLowerCase() &&
+          !columnMatchesRef(c, validated.columnName)
+      )
+    ) {
+      return NextResponse.json(
+        { error: `Column "${updates.name}" already exists` },
+        { status: 400 }
+      )
+    }
     if (updates.unique === true && !columnTypeById(resultingType).supportsUnique) {
       return NextResponse.json(
         { error: `Cannot set a ${resultingType} column as unique` },
diff --git a/apps/sim/lib/table/__tests__/column-type-registry.test.ts b/apps/sim/lib/table/__tests__/column-type-registry.test.ts
@@ -10,6 +10,7 @@
  * here.
  */
 import { describe, expect, it } from 'vitest'
+import type { ColumnType } from '@/lib/table/column-types'
 import {
   ALL_COLUMN_TYPES,
   COLUMN_TYPE_REGISTRY,
@@ -75,6 +76,36 @@ describe('registry shape', () => {
   })
 })
 
+describe('conversion write-back', () => {
+  // A retype is allowed exactly when the target's `coerce` accepts the value,
+  // and `coerce` often TRANSFORMS it. The conversion must therefore write the
+  // transformed value back — filters and sorts apply `jsonbCast` to whatever is
+  // stored, so a value left in its old shape breaks every query on the column.
+  it.each`
+    type          | stored           | expected
+    ${'date'}     | ${1700000000000} | ${'2023-11-14T22:13:20.000Z'}
+    ${'date'}     | ${'2024-01-01'}  | ${'2024-01-01'}
+    ${'currency'} | ${'$1,234.56'}   | ${1234.56}
+    ${'currency'} | ${'1.234,56'}    | ${1234.56}
+    ${'number'}   | ${'1999'}        | ${1999}
+  `('$type coerces $stored to a value its jsonbCast can read', ({ type, stored, expected }) => {
+    const column = { name: 'c', type } as ColumnDefinition
+    const result = COLUMN_TYPE_REGISTRY[type as ColumnType].coerce(stored, column)
+    expect(result.ok && result.value).toEqual(expected)
+  })
+
+  it('never leaves a numeric-cast type holding something Postgres cannot cast', () => {
+    // The concrete failure this guards: an epoch number left in a `date`
+    // column makes `(data->>'col')::timestamptz` throw on every query.
+    for (const definition of ALL_COLUMN_TYPES) {
+      if (definition.jsonbCast !== 'timestamptz') continue
+      const coerced = definition.coerce(1700000000000, { name: 'c', type: definition.id })
+      expect(coerced.ok).toBe(true)
+      expect(typeof (coerced as { value: unknown }).value).toBe('string')
+    }
+  })
+})
+
 describe('metadata ownership', () => {
   const column = (over: Partial<ColumnDefinition>): ColumnDefinition =>
     ({ name: 'c', type: 'string', ...over }) as ColumnDefinition
diff --git a/apps/sim/lib/table/column-types/registry.server.ts b/apps/sim/lib/table/column-types/registry.server.ts
@@ -62,34 +62,35 @@ async function migrateSelectCellsToNames(
 }
 
 /** Rows rewritten per statement, bounding the size of the jsonb map parameter. */
-const CURRENCY_MIGRATION_BATCH_SIZE = 5000
+const COERCED_WRITE_BACK_BATCH_SIZE = 5000
 
 /**
- * Rewrites a column's cells into the canonical `currency` storage shape: a bare
- * JSON number.
+ * Writes back the values a conversion's coercion produced.
  *
- * Load-bearing, not cosmetic. Filters and sorts cast the stored text to
- * `numeric`, so a `$1,234.56` left behind by a text → currency conversion would
- * make **every** query against the column fail — not just render oddly.
+ * A retype is allowed exactly when the target type's `coerce` accepts the
+ * value, and `coerce` frequently *transforms* it — an epoch number becomes an
+ * ISO date, `$1,234.56` becomes `1234.56`. Without this, the cell keeps its old
+ * bytes under the new type, and since filters and sorts apply the type's
+ * `jsonbCast` to whatever is stored, an epoch left in a `date` column makes
+ * `::timestamptz` fail on EVERY query against that column.
  *
- * Unlike the `select` migrations this cannot be expressed set-based:
- * `parseCurrencyInput` disambiguates grouping from decimal separators in JS, and
- * a naive SQL strip would turn `1.234.567` into an invalid numeric and abort the
- * conversion. The already-parsed amounts are folded into a jsonb map instead, so
- * it is still one statement per batch rather than one per row.
+ * The values arrive already computed (the compatibility scan derived them), so
+ * this is purely the write. It cannot be expressed set-based — the coercions
+ * are JS, and a naive SQL equivalent would mangle exactly the inputs they
+ * disambiguate — so the map is folded into one statement per batch.
  */
-async function migrateCellsToCurrencyNumbers(
+export async function writeBackCoercedCells(
   trx: DbTransaction,
   tableId: string,
   columnKey: string,
-  amountByRowId: ReadonlyMap<string, JsonValue>
+  valueByRowId: ReadonlyMap<string, JsonValue>
 ): Promise<void> {
-  if (amountByRowId.size === 0) return
+  if (valueByRowId.size === 0) return
 
-  const entries = [...amountByRowId]
-  for (let start = 0; start < entries.length; start += CURRENCY_MIGRATION_BATCH_SIZE) {
+  const entries = [...valueByRowId]
+  for (let start = 0; start < entries.length; start += COERCED_WRITE_BACK_BATCH_SIZE) {
     const batch = JSON.stringify(
-      Object.fromEntries(entries.slice(start, start + CURRENCY_MIGRATION_BATCH_SIZE))
+      Object.fromEntries(entries.slice(start, start + COERCED_WRITE_BACK_BATCH_SIZE))
     )
     await trx.execute(
       sql`UPDATE ${userTableRows} AS r
@@ -216,11 +217,7 @@ export const COLUMN_TYPE_SERVER_REGISTRY: Record<ColumnType, ColumnTypeServerEnt
     migrateCellsFrom: ({ trx, tableId, columnKey, previous }) =>
       migrateSelectCellsToNames(trx, tableId, columnKey, previous.options ?? []),
   },
-  currency: {
-    ...COLUMN_TYPE_REGISTRY.currency,
-    migrateCellsTo: ({ trx, tableId, columnKey, resolved }) =>
-      migrateCellsToCurrencyNumbers(trx, tableId, columnKey, resolved),
-  },
+  currency: COLUMN_TYPE_REGISTRY.currency,
 }
 
 /** The inbound migration for a target type, if it has one. */
diff --git a/apps/sim/lib/table/columns/service.ts b/apps/sim/lib/table/columns/service.ts
@@ -15,9 +15,13 @@ import { createLogger } from '@sim/logger'
 import { and, count, eq, sql } from 'drizzle-orm'
 import { columnMatchesRef, generateColumnId, getColumnId } from '@/lib/table/column-keys'
 import { columnTypeById, isValueCompatible } from '@/lib/table/column-types'
-import { migrationFrom, migrationTo } from '@/lib/table/column-types/registry.server'
+import {
+  migrationFrom,
+  migrationTo,
+  writeBackCoercedCells,
+} from '@/lib/table/column-types/registry.server'
 import { COLUMN_TYPES, NAME_PATTERN, TABLE_LIMITS } from '@/lib/table/constants'
-import { parseCurrencyInput, resolveCurrencyCode } from '@/lib/table/currency'
+import { resolveCurrencyCode } from '@/lib/table/currency'
 import { assertColumnDestructive, assertSchemaMutable } from '@/lib/table/mutation-locks'
 import type { DbTransaction } from '@/lib/table/planner'
 import { stripGroupExecutions } from '@/lib/table/rows/executions'
@@ -476,6 +480,45 @@ export async function deleteColumns(
   return def
 }
 
+/**
+ * The column definition a retype produces: prior per-type metadata dropped,
+ * then only what the TARGET type declares it owns carried forward, then that
+ * type's own defaults stamped on.
+ */
+function buildConvertedColumn(
+  column: ColumnDefinition,
+  data: UpdateColumnTypeData,
+  { isSelectType, targetMultiple }: { isSelectType: boolean; targetMultiple: boolean }
+): ColumnDefinition {
+  const { options: _options, multiple: _multiple, currencyCode: _currencyCode, ...rest } = column
+
+  if (isSelectType) {
+    return {
+      ...rest,
+      type: data.newType,
+      options: data.options ?? column.options,
+      ...(targetMultiple ? { multiple: true } : {}),
+      // Select columns carry no unique constraint: it would compare the stored
+      // option id, capping each option at one row table-wide, and the UI hides
+      // the toggle so it could never be cleared again. Dropped here rather than
+      // in each caller — the sidebar was the only one clearing it, leaving the
+      // v1 and agent paths to strand it.
+      unique: false,
+    }
+  }
+
+  const definition = columnTypeById(data.newType)
+  const owned = new Set<string>(definition.ownedMetadata)
+  const carried: ColumnDefinition = {
+    ...rest,
+    type: data.newType,
+    ...(owned.has('currencyCode') && (data.currencyCode ?? column.currencyCode) !== undefined
+      ? { currencyCode: data.currencyCode ?? column.currencyCode }
+      : {}),
+  }
+  return { ...carried, ...definition.defaultMetadata?.(carried) }
+}
+
 /**
  * Changes the type of a column. Validates that existing data is compatible.
  *
@@ -535,7 +578,6 @@ export async function updateColumnType(
     // Options the column will carry after the change — a `select` value is only
     // compatible if it resolves against this set.
     const isSelectType = data.newType === 'select'
-    const isCurrencyType = data.newType === 'currency'
     const targetOptions = data.options ?? column.options ?? []
     const targetMultiple = data.multiple ?? column.multiple
     // Leaving `select` behind: stored cells hold option ids, which mean nothing
@@ -560,27 +602,41 @@ export async function updateColumnType(
       }
     }
 
+    /**
+     * The column definition the table ends up with. Built before the scan so
+     * the coercion below reads the same metadata (option set, currency) the
+     * stored value will be validated against afterwards.
+     */
+    const convertedColumn = buildConvertedColumn(column, data, {
+      isSelectType,
+      targetMultiple: !!targetMultiple,
+    })
+
     let incompatibleCount = 0
     let blankCount = 0
     /**
-     * Row id → parsed amount, for a conversion into `currency`. Collected here
-     * rather than re-derived in the migration so it reads the same `effective`
-     * value the compatibility check accepted — which for a `select` source is
-     * the option name, not the stored id.
+     * Row id → the value the cell must END UP holding.
+     *
+     * Collected during the compatibility scan rather than re-derived later, so
+     * it reads the same `effective` value the check accepted — which for a
+     * `select` source is the option name, not the stored id.
+     *
+     * Load-bearing: a conversion is allowed exactly when the target type's
+     * `coerce` accepts the value, and `coerce` frequently *transforms* it (an
+     * epoch number becomes an ISO date, a formatted amount becomes a number).
+     * Without writing the transformed value back, the cell keeps its old bytes
+     * under the new type — and since filters and sorts apply the type's
+     * `jsonbCast` to whatever is stored, an epoch left in a `date` column makes
+     * `::timestamptz` fail on EVERY query against it.
      */
-    const currencyAmountByRowId = new Map<string, number>()
+    const coercedByRowId = new Map<string, JsonValue>()
     for (const row of rows) {
       const rowData = row.data as RowData
       const value = rowData[columnKey]
       if (value === null || value === undefined) continue
 
       const effective = convertingAwayFromSelect ? selectValueForConversion(column, value) : value
 
-      if (isCurrencyType && typeof effective !== 'number') {
-        const amount = parseCurrencyInput(effective)
-        if (amount !== null) currencyAmountByRowId.set(row.id, amount)
-      }
-
       if (
         !isValueCompatibleWithType(
           effective,
@@ -592,6 +648,16 @@ export async function updateColumnType(
       ) {
         if (effective === null || effective === '') blankCount++
         else incompatibleCount++
+        continue
+      }
+
+      // `select` keeps its own id↔name migrations; everything else writes back
+      // whatever `coerce` produced, when that differs from what is stored.
+      if (!isSelectType && effective !== null) {
+        const coerced = columnTypeById(data.newType).coerce(effective as JsonValue, convertedColumn)
+        if (coerced.ok && !Object.is(coerced.value, value)) {
+          coercedByRowId.set(row.id, coerced.value)
+        }
       }
     }
 
@@ -607,45 +673,7 @@ export async function updateColumnType(
       )
     }
 
-    const updatedColumns = schema.columns.map((c, i) => {
-      if (i !== columnIndex) return c
-      const {
-        options: _prevOptions,
-        multiple: _prevMultiple,
-        currencyCode: _prevCurrencyCode,
-        ...rest
-      } = c
-      // Carry forward only metadata the TARGET type owns — everything else was
-      // destructured off above and must not survive the conversion — then let
-      // the type stamp its own defaults, so a type carrying metadata gets it on
-      // a conversion and not only on create.
-      if (!isSelectType) {
-        const definition = columnTypeById(data.newType)
-        const owned = new Set<string>(definition.ownedMetadata)
-        const converted: ColumnDefinition = {
-          ...rest,
-          type: data.newType,
-          ...(owned.has('currencyCode') && (data.currencyCode ?? c.currencyCode) !== undefined
-            ? { currencyCode: data.currencyCode ?? c.currencyCode }
-            : {}),
-        }
-        return { ...converted, ...definition.defaultMetadata?.(converted) }
-      }
-      return isSelectType
-        ? {
-            ...rest,
-            type: data.newType,
-            options: data.options ?? c.options,
-            ...(targetMultiple ? { multiple: true } : {}),
-            // Select columns carry no unique constraint: it would compare the
-            // stored option id, capping each option at one row table-wide, and
-            // the UI hides the toggle so it could never be cleared again. Drop
-            // it here rather than in each caller — the sidebar was the only one
-            // clearing it, leaving the v1 and agent paths to strand it.
-            unique: false,
-          }
-        : { ...rest, type: data.newType }
-    })
+    const updatedColumns = schema.columns.map((c, i) => (i === columnIndex ? convertedColumn : c))
 
     const columnValidation = validateColumnDefinition(updatedColumns[columnIndex])
     if (!columnValidation.valid) {
@@ -664,10 +692,14 @@ export async function updateColumnType(
       columnKey,
       previous: column,
       target: updatedColumns[columnIndex],
-      resolved: currencyAmountByRowId,
+      resolved: coercedByRowId,
     }
     await migrationFrom(column.type)?.(migrationContext)
-    await migrationTo(data.newType)?.(migrationContext)
+    if (isSelectType) {
+      await migrationTo(data.newType)?.(migrationContext)
+    } else {
+      await writeBackCoercedCells(trx, data.tableId, columnKey, coercedByRowId)
+    }
 
     await trx
       .update(userTableDefinitions)
diff --git a/apps/sim/lib/table/currency.ts b/apps/sim/lib/table/currency.ts
