Skip to content

Commit 4d15378

Browse files
fix(v2-tables): restore the column-update guards v1 carries
v2's PATCH called updateColumnType whenever `type` was present, without the typeChanging check — and updateColumnType early-returns on an unchanged type, dropping any `options` sent with it. It also had no options/multiple branch at all, even though the v2 contract shares v1's body schema and already accepts both, so those fields were silently ignored on a 200. The select-unique guard was missing too. Each write is its own locked transaction, so a rename or type change paired with a constraint write that is going to fail commits first and then throws, half-applying the schema change. That guard is why the other three implementations gate on the resulting type. Also forwards `required` into the type/options writes so a conversion validates against the constraint the same request is setting, and maps a table lock to 423 LOCKED rather than a 500. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent f5f0fdf commit 4d15378

2 files changed

Lines changed: 208 additions & 3 deletions

File tree

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Public v2 column update: the guards that keep a schema change from
5+
* half-applying, and the options/multiple support the shared contract already
6+
* accepts.
7+
*/
8+
import { NextRequest } from 'next/server'
9+
import { beforeEach, describe, expect, it, vi } from 'vitest'
10+
11+
const {
12+
mockCheckRateLimit,
13+
mockResolveWorkspaceScope,
14+
mockCheckAccess,
15+
mockRenameColumn,
16+
mockUpdateColumnType,
17+
mockUpdateColumnOptions,
18+
mockUpdateColumnConstraints,
19+
} = vi.hoisted(() => ({
20+
mockCheckRateLimit: vi.fn(),
21+
mockResolveWorkspaceScope: vi.fn(),
22+
mockCheckAccess: vi.fn(),
23+
mockRenameColumn: vi.fn(),
24+
mockUpdateColumnType: vi.fn(),
25+
mockUpdateColumnOptions: vi.fn(),
26+
mockUpdateColumnConstraints: vi.fn(),
27+
}))
28+
29+
vi.mock('@/app/api/v1/middleware', () => ({
30+
checkRateLimit: mockCheckRateLimit,
31+
resolveWorkspaceScope: mockResolveWorkspaceScope,
32+
}))
33+
34+
vi.mock('@/app/api/table/utils', () => ({
35+
checkAccess: mockCheckAccess,
36+
normalizeColumn: (col: Record<string, unknown>) => col,
37+
rootErrorMessage: (error: unknown) => String(error),
38+
rowWriteErrorResponse: () => null,
39+
}))
40+
41+
vi.mock('@/lib/table', async () => {
42+
const actual = await import('@/lib/table/column-keys')
43+
return {
44+
...actual,
45+
addTableColumn: vi.fn(),
46+
deleteColumn: vi.fn(),
47+
renameColumn: mockRenameColumn,
48+
updateColumnConstraints: mockUpdateColumnConstraints,
49+
updateColumnOptions: mockUpdateColumnOptions,
50+
updateColumnType: mockUpdateColumnType,
51+
}
52+
})
53+
54+
vi.mock('@/app/api/v2/lib/gate', () => ({
55+
v2ApiGateError: vi.fn().mockResolvedValue(null),
56+
}))
57+
58+
import { PATCH } from '@/app/api/v2/tables/[tableId]/columns/route'
59+
60+
const SELECT_COLUMN = {
61+
id: 'col-1',
62+
name: 'Status',
63+
type: 'select',
64+
options: [{ id: 'o1', name: 'Open' }],
65+
}
66+
const TEXT_COLUMN = { id: 'col-2', name: 'Priority', type: 'text' }
67+
const TABLE = {
68+
id: 'table-1',
69+
name: 'Tasks',
70+
workspaceId: 'ws-1',
71+
schema: { columns: [SELECT_COLUMN, TEXT_COLUMN] },
72+
}
73+
const UPDATED = { schema: { columns: [SELECT_COLUMN] } }
74+
75+
function patch(updates: Record<string, unknown>, columnName = 'Status') {
76+
const req = new NextRequest('http://localhost:3000/api/v2/tables/table-1/columns', {
77+
method: 'PATCH',
78+
headers: { 'content-type': 'application/json' },
79+
body: JSON.stringify({ workspaceId: 'ws-1', columnName, updates }),
80+
})
81+
return PATCH(req, { params: Promise.resolve({ tableId: 'table-1' }) })
82+
}
83+
84+
describe('PATCH /api/v2/tables/[tableId]/columns', () => {
85+
beforeEach(() => {
86+
vi.clearAllMocks()
87+
mockCheckRateLimit.mockResolvedValue({
88+
allowed: true,
89+
userId: 'user-1',
90+
keyType: 'workspace',
91+
workspaceId: 'ws-1',
92+
limit: 100,
93+
remaining: 99,
94+
resetAt: new Date('2026-01-01T01:00:00Z'),
95+
})
96+
mockResolveWorkspaceScope.mockResolvedValue(null)
97+
mockCheckAccess.mockResolvedValue({ ok: true, table: TABLE })
98+
mockUpdateColumnOptions.mockResolvedValue(UPDATED)
99+
mockUpdateColumnType.mockResolvedValue(UPDATED)
100+
mockUpdateColumnConstraints.mockResolvedValue(UPDATED)
101+
})
102+
103+
it('rejects making a select column unique before writing anything', async () => {
104+
const res = await patch({ unique: true })
105+
106+
expect(res.status).toBe(400)
107+
expect((await res.json()).error.code).toBe('BAD_REQUEST')
108+
// Each write is its own transaction, so an un-gated constraint write would
109+
// commit the earlier writes and then throw, half-applying the change.
110+
expect(mockUpdateColumnConstraints).not.toHaveBeenCalled()
111+
})
112+
113+
it('routes an unchanged type with options to the options update, not the type update', async () => {
114+
await patch({ type: 'select', options: [{ id: 'o1', name: 'Open' }] })
115+
116+
// updateColumnType early-returns on an unchanged type and would drop the options.
117+
expect(mockUpdateColumnType).not.toHaveBeenCalled()
118+
expect(mockUpdateColumnOptions).toHaveBeenCalledWith(
119+
expect.objectContaining({ options: [{ id: 'o1', name: 'Open' }] }),
120+
expect.any(String)
121+
)
122+
})
123+
124+
it('applies options on a real type change instead of silently dropping them', async () => {
125+
await patch(
126+
{ type: 'select', options: [{ id: 'o2', name: 'Done' }], required: true },
127+
'Priority'
128+
)
129+
130+
expect(mockUpdateColumnOptions).not.toHaveBeenCalled()
131+
expect(mockUpdateColumnType).toHaveBeenCalledWith(
132+
expect.objectContaining({
133+
newType: 'select',
134+
options: [{ id: 'o2', name: 'Done' }],
135+
required: true,
136+
}),
137+
expect.any(String)
138+
)
139+
})
140+
141+
it('rejects converting a column to select and making it unique in one request', async () => {
142+
const res = await patch(
143+
{ type: 'select', options: [{ id: 'o2', name: 'Done' }], unique: true },
144+
'Priority'
145+
)
146+
147+
expect(res.status).toBe(400)
148+
expect(mockUpdateColumnType).not.toHaveBeenCalled()
149+
})
150+
151+
it('renames without touching the type when only a name is sent', async () => {
152+
mockRenameColumn.mockResolvedValue(UPDATED)
153+
154+
const res = await patch({ name: 'State' })
155+
156+
expect(res.status).toBe(200)
157+
expect(mockRenameColumn).toHaveBeenCalled()
158+
expect(mockUpdateColumnType).not.toHaveBeenCalled()
159+
})
160+
})

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

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,10 @@ import {
1515
deleteColumn,
1616
renameColumn,
1717
updateColumnConstraints,
18+
updateColumnOptions,
1819
updateColumnType,
1920
} from '@/lib/table'
21+
import { columnMatchesRef } from '@/lib/table/column-keys'
2022
import { checkAccess, normalizeColumn } from '@/app/api/table/utils'
2123
import { checkRateLimit, resolveWorkspaceScope } from '@/app/api/v1/middleware'
2224
import { v2ApiGateError } from '@/app/api/v2/lib/gate'
@@ -27,7 +29,7 @@ import {
2729
v2ValidationError,
2830
v2WorkspaceAccessError,
2931
} from '@/app/api/v2/lib/response'
30-
import { v2TableAccessError } from '@/app/api/v2/tables/utils'
32+
import { v2TableAccessError, v2TableLockError } from '@/app/api/v2/tables/utils'
3133

3234
const logger = createLogger('V2TableColumnsAPI')
3335

@@ -146,9 +148,50 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
146148
)
147149
}
148150

149-
if (updates.type) {
151+
// A payload that repeats the current type must not go through
152+
// `updateColumnType` — it early-returns on an unchanged type and would drop
153+
// any `options` alongside it. Only a real type change routes there; an
154+
// unchanged type with options routes to the options-only update.
155+
const currentColumn = table.schema.columns.find((c) =>
156+
columnMatchesRef(c, validated.columnName)
157+
)
158+
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
159+
160+
// Every write below is its own locked transaction, so any of them paired
161+
// with a constraint write that is going to fail commits and then errors.
162+
// Gate on the type the column ENDS UP with, not on whether the type is
163+
// changing: an options-only update on an existing select column carries the
164+
// same hazard as a conversion does.
165+
const resultingType = updates.type ?? currentColumn?.type
166+
if (updates.unique === true && resultingType === 'select') {
167+
return v2Error('BAD_REQUEST', 'Cannot set a select column as unique')
168+
}
169+
170+
if (typeChanging) {
150171
updatedTable = await updateColumnType(
151-
{ tableId, columnName: updates.name ?? validated.columnName, newType: updates.type },
172+
{
173+
tableId,
174+
columnName: updates.name ?? validated.columnName,
175+
newType: updates.type as NonNullable<typeof updates.type>,
176+
...(updates.options !== undefined ? { options: updates.options } : {}),
177+
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
178+
// Forwarded so the conversion validates against the constraint this
179+
// same request is about to set, not the column's current one.
180+
...(updates.required !== undefined ? { required: updates.required } : {}),
181+
},
182+
requestId
183+
)
184+
} else if (updates.options !== undefined || updates.multiple !== undefined) {
185+
updatedTable = await updateColumnOptions(
186+
{
187+
tableId,
188+
columnName: updates.name ?? validated.columnName,
189+
options: updates.options ?? currentColumn?.options ?? [],
190+
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
191+
// Forwarded so the removal guard validates against the constraint this
192+
// same request is about to set, not the column's current one.
193+
...(updates.required !== undefined ? { required: updates.required } : {}),
194+
},
152195
requestId
153196
)
154197
}
@@ -183,6 +226,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
183226

184227
return v2Data({ columns: updatedTable.schema.columns.map(normalizeColumn) }, { rateLimit })
185228
} catch (error) {
229+
const lockError = v2TableLockError(error)
230+
if (lockError) return lockError
186231
if (isZodError(error)) return v2ValidationError(error)
187232

188233
if (error instanceof Error) {

0 commit comments

Comments
 (0)