fix(tables): make a combined column PATCH a single transaction

Finishes the fold-in rather than pre-validating around the seam. A retype now
APPLIES the constraints it already validates against — it checks empty cells for
`required` and post-conversion duplicates for `unique`, so it was doing the work
without persisting the result — and the route skips the separate constraint
write when the type changed.

A request combining a rename, a retype and constraint changes is now one
locked transaction: no half of it can commit while another fails. The separate
constraint write remains for requests that do not change type, which is the
only case that still needs it.

Deliberately still NOT merging the two service functions. They assert different
lock levels (destructive vs schema-only) and only the retype needs the full row
scan into memory, so a merged function would force a constraints-only toggle to
materialize every row or reintroduce the branching it was meant to remove.
Folding the payload in gets atomicity without either cost.

Author: waleedlatif1

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

@@ -217,6 +217,45 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
     )
   })
 
+  it('applies a rename, retype and constraints in a single write', async () => {
+    mockCheckAccess.mockResolvedValue({
+      ok: true,
+      table: {
+        workspaceId: WORKSPACE_ID,
+        schema: { columns: [{ id: 'col_a', name: 'amount', type: 'string' }] },
+      },
+    })
+    mockUpdateColumnType.mockResolvedValue({ schema: { columns: [] } })
+
+    const response = await patch({ name: 'total', type: 'number', required: true, unique: true })
+
+    expect(response.status).toBe(200)
+    // One transaction for the whole request: no separate rename, no separate
+    // constraint write, so no half of it can commit without the others.
+    expect(mockRenameColumn).not.toHaveBeenCalled()
+    expect(mockUpdateColumnConstraints).not.toHaveBeenCalled()
+    expect(mockUpdateColumnType).toHaveBeenCalledTimes(1)
+    expect(mockUpdateColumnType).toHaveBeenCalledWith(
+      expect.objectContaining({
+        newType: 'number',
+        required: true,
+        unique: true,
+        newName: 'total',
+      }),
+      expect.any(String)
+    )
+  })
+
+  it('still runs the constraint write when the type is unchanged', async () => {
+    mockUpdateColumnConstraints.mockResolvedValue({ schema: { columns: [] } })
+
+    const response = await patch({ required: true })
+
+    expect(response.status).toBe(200)
+    expect(mockUpdateColumnConstraints).toHaveBeenCalledTimes(1)
+    expect(mockUpdateColumnType).not.toHaveBeenCalled()
+  })
+
   it('rejects constraint changes on a workflow-output column before any write', async () => {
     mockCheckAccess.mockResolvedValue({
       ok: true,

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

@@ -135,10 +135,14 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
     const columnRef = currentColumn ? getColumnId(currentColumn) : validated.columnName
     // The constraints write below is a separate, unconditional step, so it is
     // the last one whenever it runs — that is the write the rename rides on.
-    const constraintsChanging = updates.required !== undefined || updates.unique !== undefined
-    const renameWithTypedWrite =
-      updates.name && !constraintsChanging ? { newName: updates.name } : {}
     const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
+    // A retype applies and validates the constraints itself, so the separate
+    // constraint write only runs when the type is unchanged. The rename rides
+    // whichever write actually runs last.
+    const constraintsWriteRuns =
+      !typeChanging && (updates.required !== undefined || updates.unique !== undefined)
+    const renameWithTypedWrite =
+      updates.name && !constraintsWriteRuns ? { newName: updates.name } : {}
 
     // Every write below is its own locked transaction, so one that is going to
     // fail leaves the earlier ones committed. These guards reject the knowable
@@ -246,7 +250,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       )
     }
 
-    if (updates.required !== undefined || updates.unique !== undefined) {
+    // Skipped when the type changed: that write already applied and validated
+    // these, in one transaction with the conversion.
+    if (!typeChanging && (updates.required !== undefined || updates.unique !== undefined)) {
       updatedTable = await updateColumnConstraints(
         {
           tableId,

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

@@ -169,10 +169,14 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
     const columnRef = currentColumn ? getColumnId(currentColumn) : validated.columnName
     // The constraints write below is a separate, unconditional step, so it is
     // the last one whenever it runs — that is the write the rename rides on.
-    const constraintsChanging = updates.required !== undefined || updates.unique !== undefined
-    const renameWithTypedWrite =
-      updates.name && !constraintsChanging ? { newName: updates.name } : {}
     const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
+    // A retype applies and validates the constraints itself, so the separate
+    // constraint write only runs when the type is unchanged. The rename rides
+    // whichever write actually runs last.
+    const constraintsWriteRuns =
+      !typeChanging && (updates.required !== undefined || updates.unique !== undefined)
+    const renameWithTypedWrite =
+      updates.name && !constraintsWriteRuns ? { newName: updates.name } : {}
 
     // Every write below is its own locked transaction, so one that is going to
     // fail leaves the earlier ones committed. These guards reject the knowable
@@ -280,7 +284,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
       )
     }
 
-    if (updates.required !== undefined || updates.unique !== undefined) {
+    // Skipped when the type changed: that write already applied and validated
+    // these, in one transaction with the conversion.
+    if (!typeChanging && (updates.required !== undefined || updates.unique !== undefined)) {
       updatedTable = await updateColumnConstraints(
         {
           tableId,

apps/sim/lib/table/columns/service.ts

@@ -571,10 +571,19 @@ function buildConvertedColumn(
   // cannot ride through `...rest` onto a target that does not own it — which
   // `validateColumnDefinition` would then reject on every later write.
   const rest = omit(column, [...TYPE_SPECIFIC_COLUMN_KEYS]) as ColumnDefinition
+  // Constraints arriving with the retype are APPLIED here, not left to a second
+  // transaction. `updateColumnType` already validates against them (empty cells
+  // for `required`, post-conversion duplicates for `unique`), so applying them
+  // in the same write is what makes a combined request all-or-nothing.
+  const withConstraints: ColumnDefinition = {
+    ...rest,
+    ...(data.required !== undefined ? { required: data.required } : {}),
+    ...(data.unique !== undefined ? { unique: data.unique } : {}),
+  }
 
   if (isSelectType) {
     return {
-      ...rest,
+      ...withConstraints,
       type: data.newType,
       options: data.options ?? column.options,
       ...(targetMultiple ? { multiple: true } : {}),
@@ -593,7 +602,7 @@ function buildConvertedColumn(
   // future type.
   const definition = columnTypeById(data.newType)
   const owned = new Set<string>(definition.ownedMetadata)
-  const carried: ColumnDefinition = { ...rest, type: data.newType }
+  const carried: ColumnDefinition = { ...withConstraints, type: data.newType }
   for (const key of TYPE_SPECIFIC_COLUMN_KEYS) {
     if (!owned.has(key)) continue
     const value = data[key] ?? column[key]