Skip to content

Commit 3fc73a3

Browse files
waleedlatif1waleedlatif1
committed
fix(tables): rename the column last so a failed write leaves it untouched
Greptile's remaining concern: the pre-flight guards read a schema snapshot, so a column-type change landing concurrently can still make a later write fail — and with the rename running first, that failure returned an error with the rename already committed. Guards cannot close that window; each write is its own locked transaction and only the write itself sees the authoritative state. Ordering can. The rename is the one write that is purely cosmetic, so it now runs last: a failed typed write leaves the column entirely untouched, and a failed rename leaves the typed change applied under the old name — the recoverable half. The typed writes target the column's current name, since no rename has happened yet. Tests cover both directions: a typed write rejected mid-flight must not rename, and a successful one must rename strictly after. Verified to fail under the previous ordering.
1 parent 522cc62 commit 3fc73a3

3 files changed

Lines changed: 72 additions & 39 deletions

File tree

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

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
* @vitest-environment node
33
*
44
* 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.
5+
* transaction, so one that fails leaves the earlier ones committed. Two things
6+
* keep that from producing a partial update the caller cannot see or undo: the
7+
* guards reject the knowable cases before any write, and the rename — the only
8+
* write that is purely cosmetic — goes LAST, so a failed typed write leaves the
9+
* column entirely untouched. These pin both.
810
*/
911
import { hybridAuthMockFns } from '@sim/testing'
1012
import { getErrorMessage } from '@sim/utils/errors'
@@ -111,6 +113,26 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
111113
expect(mockRenameColumn).not.toHaveBeenCalled()
112114
})
113115

116+
it('leaves the column untouched when a typed write fails', async () => {
117+
mockCheckAccess.mockResolvedValue({
118+
ok: true,
119+
table: {
120+
workspaceId: WORKSPACE_ID,
121+
schema: { columns: [{ id: 'col_a', name: 'amount', type: 'currency' }] },
122+
},
123+
})
124+
// Stands in for the race the guards cannot close: the column stopped being
125+
// a currency between the snapshot the guards read and this write.
126+
mockUpdateColumnCurrency.mockRejectedValue(
127+
new Error('Cannot set currency on column "amount" of type "string"')
128+
)
129+
130+
const response = await patch({ name: 'renamed', currencyCode: 'USD' })
131+
132+
expect(response.status).toBe(400)
133+
expect(mockRenameColumn).not.toHaveBeenCalled()
134+
})
135+
114136
it('rejects unique on a type that cannot carry it without renaming first', async () => {
115137
mockCheckAccess.mockResolvedValue({
116138
ok: true,
@@ -130,7 +152,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
130152
expect(mockRenameColumn).not.toHaveBeenCalled()
131153
})
132154

133-
it('still performs a valid combined rename + currency change', async () => {
155+
it('renames only after the typed write succeeds', async () => {
134156
mockCheckAccess.mockResolvedValue({
135157
ok: true,
136158
table: {
@@ -145,10 +167,13 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
145167
expect(response.status).toBe(200)
146168
expect(mockRenameColumn).toHaveBeenCalledTimes(1)
147169
expect(mockUpdateColumnCurrency).toHaveBeenCalledWith(
148-
// The contract upper-cases on the way in, and the rename means the
149-
// currency write must target the NEW name.
150-
expect.objectContaining({ columnName: 'renamed', currencyCode: 'EUR' }),
170+
// Targets the column's CURRENT name: the rename has not run yet. The
171+
// contract upper-cases the code on the way in.
172+
expect.objectContaining({ columnName: 'amount', currencyCode: 'EUR' }),
151173
expect.any(String)
152174
)
175+
expect(mockUpdateColumnCurrency.mock.invocationCallOrder[0]).toBeLessThan(
176+
mockRenameColumn.mock.invocationCallOrder[0]
177+
)
153178
})
154179
})

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

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -132,11 +132,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
132132
)
133133
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
134134

135-
// Every write below is its own locked transaction, so any of them paired
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.
135+
// Every write below is its own locked transaction, so one that is going to
136+
// fail leaves the earlier ones committed. These guards reject the knowable
137+
// cases up front, before any write at all.
140138
// Gate on the type the column ENDS UP with, not on whether the type is
141139
// changing: an options-only update on an existing select column carries the
142140
// same hazard as a conversion does.
@@ -166,18 +164,11 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
166164
)
167165
}
168166

169-
if (updates.name) {
170-
updatedTable = await renameColumn(
171-
{ tableId, oldName: validated.columnName, newName: updates.name },
172-
requestId
173-
)
174-
}
175-
176167
if (typeChanging) {
177168
updatedTable = await updateColumnType(
178169
{
179170
tableId,
180-
columnName: updates.name ?? validated.columnName,
171+
columnName: validated.columnName,
181172
newType: updates.type as NonNullable<typeof updates.type>,
182173
...(updates.options !== undefined ? { options: updates.options } : {}),
183174
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
@@ -195,7 +186,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
195186
updatedTable = await updateColumnCurrency(
196187
{
197188
tableId,
198-
columnName: updates.name ?? validated.columnName,
189+
columnName: validated.columnName,
199190
currencyCode: updates.currencyCode,
200191
},
201192
requestId
@@ -204,7 +195,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
204195
updatedTable = await updateColumnOptions(
205196
{
206197
tableId,
207-
columnName: updates.name ?? validated.columnName,
198+
columnName: validated.columnName,
208199
options: updates.options ?? currentColumn?.options ?? [],
209200
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
210201
// Forwarded so the removal guard validates against the constraint this
@@ -219,14 +210,27 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
219210
updatedTable = await updateColumnConstraints(
220211
{
221212
tableId,
222-
columnName: updates.name ?? validated.columnName,
213+
columnName: validated.columnName,
223214
...(updates.required !== undefined ? { required: updates.required } : {}),
224215
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
225216
},
226217
requestId
227218
)
228219
}
229220

221+
// Rename LAST, on purpose. Each write above is its own locked transaction,
222+
// and the pre-flight guards read a schema snapshot — so a concurrent change
223+
// can still make one of them fail. Renaming first meant that failure
224+
// returned an error with the rename already committed. Going last, a failed
225+
// typed write leaves the column untouched, and a failed rename leaves the
226+
// typed change applied under the old name, which is the recoverable half.
227+
if (updates.name) {
228+
updatedTable = await renameColumn(
229+
{ tableId, oldName: validated.columnName, newName: updates.name },
230+
requestId
231+
)
232+
}
233+
230234
if (!updatedTable) {
231235
return NextResponse.json({ error: 'No updates specified' }, { status: 400 })
232236
}

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

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
166166
)
167167
const typeChanging = updates.type !== undefined && updates.type !== currentColumn?.type
168168

169-
// Every write below is its own locked transaction, so any of them paired
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.
169+
// Every write below is its own locked transaction, so one that is going to
170+
// fail leaves the earlier ones committed. These guards reject the knowable
171+
// cases up front, before any write at all.
174172
// Gate on the type the column ENDS UP with, not on whether the type is
175173
// changing: an options-only update on an existing select column carries the
176174
// same hazard as a conversion does.
@@ -200,18 +198,11 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
200198
)
201199
}
202200

203-
if (updates.name) {
204-
updatedTable = await renameColumn(
205-
{ tableId, oldName: validated.columnName, newName: updates.name },
206-
requestId
207-
)
208-
}
209-
210201
if (typeChanging) {
211202
updatedTable = await updateColumnType(
212203
{
213204
tableId,
214-
columnName: updates.name ?? validated.columnName,
205+
columnName: validated.columnName,
215206
newType: updates.type as NonNullable<typeof updates.type>,
216207
...(updates.options !== undefined ? { options: updates.options } : {}),
217208
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
@@ -229,7 +220,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
229220
updatedTable = await updateColumnCurrency(
230221
{
231222
tableId,
232-
columnName: updates.name ?? validated.columnName,
223+
columnName: validated.columnName,
233224
currencyCode: updates.currencyCode,
234225
},
235226
requestId
@@ -238,7 +229,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
238229
updatedTable = await updateColumnOptions(
239230
{
240231
tableId,
241-
columnName: updates.name ?? validated.columnName,
232+
columnName: validated.columnName,
242233
options: updates.options ?? currentColumn?.options ?? [],
243234
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
244235
// Forwarded so the removal guard validates against the constraint this
@@ -253,14 +244,27 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
253244
updatedTable = await updateColumnConstraints(
254245
{
255246
tableId,
256-
columnName: updates.name ?? validated.columnName,
247+
columnName: validated.columnName,
257248
...(updates.required !== undefined ? { required: updates.required } : {}),
258249
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
259250
},
260251
requestId
261252
)
262253
}
263254

255+
// Rename LAST, on purpose. Each write above is its own locked transaction,
256+
// and the pre-flight guards read a schema snapshot — so a concurrent change
257+
// can still make one of them fail. Renaming first meant that failure
258+
// returned an error with the rename already committed. Going last, a failed
259+
// typed write leaves the column untouched, and a failed rename leaves the
260+
// typed change applied under the old name, which is the recoverable half.
261+
if (updates.name) {
262+
updatedTable = await renameColumn(
263+
{ tableId, oldName: validated.columnName, newName: updates.name },
264+
requestId
265+
)
266+
}
267+
264268
if (!updatedTable) {
265269
return NextResponse.json({ error: 'No updates specified' }, { status: 400 })
266270
}

0 commit comments

Comments
 (0)