Skip to content

Commit aba4433

Browse files
committed
fix(tables): run the column PATCH guards ahead of the rename, not after it
Greptile was right and my previous reply was wrong. The guards were added in the right shape but the wrong place — below `renameColumn`, which is the first write and commits in its own transaction. A PATCH combining a rename with an invalid currency therefore still committed the rename and then returned 400, exactly the counterexample reported. Moved the column lookup and all three pre-flight guards above every write. This also closes the same latent hole for the pre-existing unique-on-select guard, which sat in the same position. Adds route tests that assert `renameColumn` was never called on each rejection path, and that a valid combined rename + currency change still targets the new name. Verified to fail against the previous ordering.
1 parent f819588 commit aba4433

8 files changed

Lines changed: 184 additions & 35 deletions

File tree

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* The PATCH handler performs several writes, each in its own locked
5+
* transaction. `renameColumn` is the first, so any rejection raised after it
6+
* returns an error with the rename already committed — a partial update the
7+
* caller cannot see or undo. These pin the pre-flight guards ahead of it.
8+
*/
9+
import { hybridAuthMockFns } from '@sim/testing'
10+
import { NextRequest } from 'next/server'
11+
import { beforeEach, describe, expect, it, vi } from 'vitest'
12+
13+
const {
14+
mockCheckAccess,
15+
mockRenameColumn,
16+
mockUpdateColumnType,
17+
mockUpdateColumnCurrency,
18+
mockUpdateColumnOptions,
19+
mockUpdateColumnConstraints,
20+
mockAddTableColumn,
21+
mockDeleteColumn,
22+
} = vi.hoisted(() => ({
23+
mockCheckAccess: vi.fn(),
24+
mockRenameColumn: vi.fn(),
25+
mockUpdateColumnType: vi.fn(),
26+
mockUpdateColumnCurrency: vi.fn(),
27+
mockUpdateColumnOptions: vi.fn(),
28+
mockUpdateColumnConstraints: vi.fn(),
29+
mockAddTableColumn: vi.fn(),
30+
mockDeleteColumn: vi.fn(),
31+
}))
32+
33+
vi.mock('@/lib/table', () => ({
34+
addTableColumn: mockAddTableColumn,
35+
deleteColumn: mockDeleteColumn,
36+
renameColumn: mockRenameColumn,
37+
updateColumnConstraints: mockUpdateColumnConstraints,
38+
updateColumnCurrency: mockUpdateColumnCurrency,
39+
updateColumnOptions: mockUpdateColumnOptions,
40+
updateColumnType: mockUpdateColumnType,
41+
}))
42+
vi.mock('@/app/api/table/utils', () => ({
43+
accessError: () => new Response('denied', { status: 403 }),
44+
checkAccess: mockCheckAccess,
45+
normalizeColumn: (c: unknown) => c,
46+
rootErrorMessage: (e: unknown) => (e instanceof Error ? e.message : String(e)),
47+
tableLockErrorResponse: () => null,
48+
}))
49+
50+
import { PATCH } from '@/app/api/table/[tableId]/columns/route'
51+
52+
const WORKSPACE_ID = '11111111-1111-4111-8111-111111111111'
53+
54+
function patch(updates: Record<string, unknown>) {
55+
return PATCH(
56+
new NextRequest('http://localhost/api/table/t1/columns', {
57+
method: 'PATCH',
58+
body: JSON.stringify({ workspaceId: WORKSPACE_ID, columnName: 'amount', updates }),
59+
headers: { 'content-type': 'application/json' },
60+
}),
61+
{ params: Promise.resolve({ tableId: 't1' }) }
62+
)
63+
}
64+
65+
describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
66+
beforeEach(() => {
67+
vi.clearAllMocks()
68+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
69+
success: true,
70+
userId: 'user-1',
71+
authType: 'session',
72+
})
73+
mockCheckAccess.mockResolvedValue({
74+
ok: true,
75+
table: {
76+
workspaceId: WORKSPACE_ID,
77+
schema: { columns: [{ id: 'col_a', name: 'amount', type: 'number' }] },
78+
},
79+
})
80+
mockRenameColumn.mockResolvedValue({ schema: { columns: [] } })
81+
})
82+
83+
it('rejects a currency code on a non-currency column without renaming first', async () => {
84+
const response = await patch({ name: 'renamed', currencyCode: 'USD' })
85+
86+
expect(response.status).toBe(400)
87+
expect(await response.json()).toMatchObject({
88+
error: expect.stringContaining('Cannot set currency'),
89+
})
90+
// The whole point: the rename must not have been committed.
91+
expect(mockRenameColumn).not.toHaveBeenCalled()
92+
expect(mockUpdateColumnCurrency).not.toHaveBeenCalled()
93+
})
94+
95+
it('rejects an unsupported currency code without renaming first', async () => {
96+
mockCheckAccess.mockResolvedValue({
97+
ok: true,
98+
table: {
99+
workspaceId: WORKSPACE_ID,
100+
schema: { columns: [{ id: 'col_a', name: 'amount', type: 'currency' }] },
101+
},
102+
})
103+
104+
const response = await patch({ name: 'renamed', currencyCode: 'ZZZ' })
105+
106+
expect(response.status).toBe(400)
107+
expect(await response.json()).toMatchObject({
108+
error: expect.stringContaining('Invalid currency code'),
109+
})
110+
expect(mockRenameColumn).not.toHaveBeenCalled()
111+
})
112+
113+
it('rejects unique on a type that cannot carry it without renaming first', async () => {
114+
mockCheckAccess.mockResolvedValue({
115+
ok: true,
116+
table: {
117+
workspaceId: WORKSPACE_ID,
118+
schema: {
119+
columns: [
120+
{ id: 'col_a', name: 'amount', type: 'select', options: [{ id: 'o', name: 'O' }] },
121+
],
122+
},
123+
},
124+
})
125+
126+
const response = await patch({ name: 'renamed', unique: true })
127+
128+
expect(response.status).toBe(400)
129+
expect(mockRenameColumn).not.toHaveBeenCalled()
130+
})
131+
132+
it('still performs a valid combined rename + currency change', async () => {
133+
mockCheckAccess.mockResolvedValue({
134+
ok: true,
135+
table: {
136+
workspaceId: WORKSPACE_ID,
137+
schema: { columns: [{ id: 'col_a', name: 'amount', type: 'currency' }] },
138+
},
139+
})
140+
mockUpdateColumnCurrency.mockResolvedValue({ schema: { columns: [] } })
141+
142+
const response = await patch({ name: 'renamed', currencyCode: 'eur' })
143+
144+
expect(response.status).toBe(200)
145+
expect(mockRenameColumn).toHaveBeenCalledTimes(1)
146+
expect(mockUpdateColumnCurrency).toHaveBeenCalledWith(
147+
// The contract upper-cases on the way in, and the rename means the
148+
// currency write must target the NEW name.
149+
expect.objectContaining({ columnName: 'renamed', currencyCode: 'EUR' }),
150+
expect.any(String)
151+
)
152+
})
153+
})

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

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -123,13 +123,6 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
123123
const { updates } = validated
124124
let updatedTable = null
125125

126-
if (updates.name) {
127-
updatedTable = await renameColumn(
128-
{ tableId, oldName: validated.columnName, newName: updates.name },
129-
requestId
130-
)
131-
}
132-
133126
// A payload that repeats the current type must not go through
134127
// `updateColumnType` — it early-returns on an unchanged type and would drop
135128
// any `options` alongside it. Only a real type change routes there; an
@@ -140,15 +133,14 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
140133
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
141134

142135
// Every write below is its own locked transaction, so any of them paired
143-
// with a constraint write that is going to fail commits and then errors.
136+
// with a write that is going to fail commits and then errors. These run
137+
// ahead of EVERY write — including the rename — because `renameColumn`
138+
// commits on its own, so a rejection raised later would return an error
139+
// with the rename already applied.
144140
// Gate on the type the column ENDS UP with, not on whether the type is
145141
// changing: an options-only update on an existing select column carries the
146142
// same hazard as a conversion does.
147143
const resultingType = updates.type ?? currentColumn?.type
148-
// Same reason as the constraint guard below: `renameColumn` runs first and
149-
// commits on its own, so anything `updateColumnCurrency` would reject has
150-
// to be caught before that write rather than inside the last one —
151-
// otherwise the rename sticks and the request still errors.
152144
if (updates.currencyCode !== undefined) {
153145
if (resultingType !== 'currency') {
154146
return NextResponse.json(
@@ -174,6 +166,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
174166
)
175167
}
176168

169+
if (updates.name) {
170+
updatedTable = await renameColumn(
171+
{ tableId, oldName: validated.columnName, newName: updates.name },
172+
requestId
173+
)
174+
}
175+
177176
if (typeChanging) {
178177
updatedTable = await updateColumnType(
179178
{

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

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -157,13 +157,6 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
157157
const { updates } = validated
158158
let updatedTable = null
159159

160-
if (updates.name) {
161-
updatedTable = await renameColumn(
162-
{ tableId, oldName: validated.columnName, newName: updates.name },
163-
requestId
164-
)
165-
}
166-
167160
// A payload that repeats the current type must not go through
168161
// `updateColumnType` — it early-returns on an unchanged type and would drop
169162
// any `options` alongside it. Only a real type change routes there; an
@@ -174,15 +167,14 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
174167
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
175168

176169
// Every write below is its own locked transaction, so any of them paired
177-
// with a constraint write that is going to fail commits and then errors.
170+
// with a write that is going to fail commits and then errors. These run
171+
// ahead of EVERY write — including the rename — because `renameColumn`
172+
// commits on its own, so a rejection raised later would return an error
173+
// with the rename already applied.
178174
// Gate on the type the column ENDS UP with, not on whether the type is
179175
// changing: an options-only update on an existing select column carries the
180176
// same hazard as a conversion does.
181177
const resultingType = updates.type ?? currentColumn?.type
182-
// Same reason as the constraint guard below: `renameColumn` runs first and
183-
// commits on its own, so anything `updateColumnCurrency` would reject has
184-
// to be caught before that write rather than inside the last one —
185-
// otherwise the rename sticks and the request still errors.
186178
if (updates.currencyCode !== undefined) {
187179
if (resultingType !== 'currency') {
188180
return NextResponse.json(
@@ -208,6 +200,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
208200
)
209201
}
210202

203+
if (updates.name) {
204+
updatedTable = await renameColumn(
205+
{ tableId, oldName: validated.columnName, newName: updates.name },
206+
requestId
207+
)
208+
}
209+
211210
if (typeChanging) {
212211
updatedTable = await updateColumnType(
213212
{

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/row-modal/row-modal.tsx

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) {
220220
const hint = `Type: ${typeLabel}${column.required ? '' : ' (optional)'}`
221221
const definition = columnTypeOf(column)
222222

223-
if (column.type === 'boolean') {
223+
if (definition.editor === 'toggle') {
224224
return (
225225
<ChipModalField type='custom' title={title} required={column.required} hint={hint}>
226226
<div className='flex items-center gap-2'>
@@ -240,6 +240,9 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) {
240240
)
241241
}
242242

243+
// The one type wanting a mono multi-line field; `editor: 'text'` covers both
244+
// this and a plain input, so it stays explicit rather than inventing a field
245+
// only one type would ever set.
243246
if (column.type === 'json') {
244247
return (
245248
<ChipModalField
@@ -256,7 +259,7 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) {
256259
)
257260
}
258261

259-
if (column.type === 'date') {
262+
if (definition.editor === 'date') {
260263
const parts = dateValueToLocalParts(formatValueForInput(value, 'date'))
261264
return (
262265
<ChipModalField type='custom' title={title} required={column.required} hint={hint}>
@@ -285,7 +288,7 @@ function ColumnField({ column, value, onChange }: ColumnFieldProps) {
285288
)
286289
}
287290

288-
if (column.type === 'select') {
291+
if (definition.editor === 'select') {
289292
return (
290293
<ChipModalField type='custom' title={title} required={column.required} hint={hint}>
291294
<SelectValueEditor column={column} value={value} onChange={onChange} fullWidth />

apps/sim/lib/table/cell-format.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ import type { ColumnDefinition, JsonValue, RowData } from '@/lib/table/types'
2525
* must reach consumers byte-identical to what is stored.
2626
*/
2727
export function formatCellValue(value: unknown, column: ColumnDefinition): JsonValue {
28-
// Only types storing opaque ids need translating; the passthrough for
29-
// everything else is load-bearing (see the module doc).
3028
if (columnTypeOf(column).storesOpaqueIds) return selectValueToNames(column, value)
3129
return value as JsonValue
3230
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -609,7 +609,6 @@ export async function updateColumnType(
609609

610610
const updatedColumns = schema.columns.map((c, i) => {
611611
if (i !== columnIndex) return c
612-
// Drop any prior per-type config, then re-add whatever the target type uses.
613612
const {
614613
options: _prevOptions,
615614
multiple: _prevMultiple,
@@ -1136,8 +1135,7 @@ export function isValueCompatibleWithType(
11361135
targetRequired = false
11371136
): boolean {
11381137
if (value === null || value === undefined) return true
1139-
// The target column as it will exist after the change — each type reads only
1140-
// the metadata it owns.
1138+
// Each type reads only the metadata it owns.
11411139
return columnTypeById(targetType).isCompatibleWith(value, {
11421140
name: '',
11431141
type: targetType,

apps/sim/lib/table/export-format.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ export function formatCsvValue(value: unknown): string {
4444
* (comma-joined for multi) so the file shows the enum label, not the id.
4545
*/
4646
export function formatCsvCell(column: ColumnDefinition, value: unknown): string {
47-
// Types storing opaque ids must be resolved to their labels; every other type
48-
// writes its stored value verbatim so the file re-imports byte-identically.
47+
// Every other type writes its stored value verbatim so the file re-imports
48+
// byte-identically.
4949
if (columnTypeOf(column).storesOpaqueIds) {
5050
return neutralizeCsvFormula(columnTypeOf(column).formatForDisplay(value, column))
5151
}

apps/sim/lib/table/validation.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -650,7 +650,6 @@ export function validateColumnDefinition(column: ColumnDefinition): ValidationRe
650650
errors.push(`Column "${column.name}" of type "${column.type}" cannot be unique`)
651651
}
652652

653-
// Generic foreign-metadata check, driven by each type's declared ownership.
654653
// Type-specific metadata stored on the wrong type is inert until a later
655654
// conversion inherits it — silently overriding what that request asked for.
656655
const owned = new Set<string>(definition.ownedMetadata)

0 commit comments

Comments
 (0)