Skip to content

Commit 78cf534

Browse files
waleedlatif1waleedlatif1
committed
refactor(tables): drive column metadata updates from the registry
Adding a type-specific metadata key needed ~9 near-identical edits, none compiler-enforced: a bespoke service writer, a branch in both column routes, the copilot tool, and a per-key validity check duplicated in each. The add-column-type skill lists this as its known gap. One writer now handles every key. `updateColumnMetadata` replaces `updateColumnCurrency`, reading ownership from `ownedMetadata`, normalization from `defaultMetadata`, and validation from `validateDefinition`, so callers name no keys at all. Routes route on `metadataKeysIn(updates)` rather than testing `updates.currencyCode !== undefined`, and `UpdateColumnTypeData` derives its metadata slice from `ColumnDefinition` so `buildConvertedColumn`'s indexed read fails to compile until a new key is carriable. Types whose metadata changes the stored bytes declare `migrateCellsForMetadata` and get a scaled-timeout rewrite inside the same transaction; presentational metadata still touches no row. The update path's hand-written "Invalid currency code" message is gone in favour of the currency type's own, which the add-column path already returned.
1 parent ccc2ec9 commit 78cf534

12 files changed

Lines changed: 427 additions & 163 deletions

File tree

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

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ const {
1717
mockCheckAccess,
1818
mockRenameColumn,
1919
mockUpdateColumnType,
20-
mockUpdateColumnCurrency,
20+
mockUpdateColumnMetadata,
2121
mockUpdateColumnOptions,
2222
mockUpdateColumnConstraints,
2323
mockAddTableColumn,
@@ -26,7 +26,7 @@ const {
2626
mockCheckAccess: vi.fn(),
2727
mockRenameColumn: vi.fn(),
2828
mockUpdateColumnType: vi.fn(),
29-
mockUpdateColumnCurrency: vi.fn(),
29+
mockUpdateColumnMetadata: vi.fn(),
3030
mockUpdateColumnOptions: vi.fn(),
3131
mockUpdateColumnConstraints: vi.fn(),
3232
mockAddTableColumn: vi.fn(),
@@ -38,7 +38,7 @@ vi.mock('@/lib/table', () => ({
3838
deleteColumn: mockDeleteColumn,
3939
renameColumn: mockRenameColumn,
4040
updateColumnConstraints: mockUpdateColumnConstraints,
41-
updateColumnCurrency: mockUpdateColumnCurrency,
41+
updateColumnMetadata: mockUpdateColumnMetadata,
4242
updateColumnOptions: mockUpdateColumnOptions,
4343
updateColumnType: mockUpdateColumnType,
4444
}))
@@ -92,7 +92,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
9292
})
9393
// The whole point: the rename must not have been committed.
9494
expect(mockRenameColumn).not.toHaveBeenCalled()
95-
expect(mockUpdateColumnCurrency).not.toHaveBeenCalled()
95+
expect(mockUpdateColumnMetadata).not.toHaveBeenCalled()
9696
})
9797

9898
it('rejects an unsupported currency code without renaming first', async () => {
@@ -107,14 +107,17 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
107107
const response = await patch({ name: 'renamed', currencyCode: 'ZZZ' })
108108

109109
expect(response.status).toBe(400)
110+
// The currency type's own `validateDefinition` message — the same one the
111+
// add-column path already returned. The route no longer carries a second,
112+
// differently-worded copy of this check.
110113
expect(await response.json()).toMatchObject({
111-
error: expect.stringContaining('Invalid currency code'),
114+
error: 'Column "amount" has invalid currency code "ZZZ". Use an ISO 4217 code, e.g. USD',
112115
})
113116
expect(mockRenameColumn).not.toHaveBeenCalled()
114117
})
115118

116119
it('still applies a rename when the currency it rides on is unchanged', async () => {
117-
// `updateColumnCurrency` no-ops on an unchanged code. The rename folded into
120+
// `updateColumnMetadata` no-ops on an unchanged code. The rename folded into
118121
// the same request must not be dropped with it.
119122
mockCheckAccess.mockResolvedValue({
120123
ok: true,
@@ -125,13 +128,13 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
125128
},
126129
},
127130
})
128-
mockUpdateColumnCurrency.mockResolvedValue({ schema: { columns: [] } })
131+
mockUpdateColumnMetadata.mockResolvedValue({ schema: { columns: [] } })
129132

130133
const response = await patch({ name: 'renamed', currencyCode: 'USD' })
131134

132135
expect(response.status).toBe(200)
133-
expect(mockUpdateColumnCurrency).toHaveBeenCalledWith(
134-
expect.objectContaining({ currencyCode: 'USD', newName: 'renamed' }),
136+
expect(mockUpdateColumnMetadata).toHaveBeenCalledWith(
137+
expect.objectContaining({ metadata: { currencyCode: 'USD' }, newName: 'renamed' }),
135138
expect.any(String)
136139
)
137140
})
@@ -158,7 +161,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
158161
})
159162
// Stands in for the race the guards cannot close: the column stopped being
160163
// a currency between the snapshot the guards read and this write.
161-
mockUpdateColumnCurrency.mockRejectedValue(
164+
mockUpdateColumnMetadata.mockRejectedValue(
162165
new Error('Cannot set currency on column "amount" of type "string"')
163166
)
164167

@@ -189,7 +192,7 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
189192
error: expect.stringContaining('already exists'),
190193
})
191194
// The typed write would otherwise have committed under a rename that fails.
192-
expect(mockUpdateColumnCurrency).not.toHaveBeenCalled()
195+
expect(mockUpdateColumnMetadata).not.toHaveBeenCalled()
193196
expect(mockRenameColumn).not.toHaveBeenCalled()
194197
})
195198

@@ -303,17 +306,21 @@ describe('PATCH /api/table/[tableId]/columns — pre-flight guards', () => {
303306
schema: { columns: [{ id: 'col_a', name: 'amount', type: 'currency' }] },
304307
},
305308
})
306-
mockUpdateColumnCurrency.mockResolvedValue({ schema: { columns: [] } })
309+
mockUpdateColumnMetadata.mockResolvedValue({ schema: { columns: [] } })
307310

308311
const response = await patch({ name: 'renamed', currencyCode: 'eur' })
309312

310313
expect(response.status).toBe(200)
311314
// One transaction, not two: the rename rides along with the currency write,
312315
// so neither half can commit without the other.
313316
expect(mockRenameColumn).not.toHaveBeenCalled()
314-
expect(mockUpdateColumnCurrency).toHaveBeenCalledWith(
317+
expect(mockUpdateColumnMetadata).toHaveBeenCalledWith(
315318
// Addressed by stable id; the contract upper-cases the code on the way in.
316-
expect.objectContaining({ columnName: 'col_a', currencyCode: 'EUR', newName: 'renamed' }),
319+
expect.objectContaining({
320+
columnName: 'col_a',
321+
metadata: { currencyCode: 'EUR' },
322+
newName: 'renamed',
323+
}),
317324
expect.any(String)
318325
)
319326
})

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

Lines changed: 25 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@ import {
1515
deleteColumn,
1616
renameColumn,
1717
updateColumnConstraints,
18-
updateColumnCurrency,
18+
updateColumnMetadata,
1919
updateColumnOptions,
2020
updateColumnType,
2121
} from '@/lib/table'
2222
import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
23-
import { columnTypeById } from '@/lib/table/column-types'
24-
import { isSupportedCurrencyCode } from '@/lib/table/currency'
23+
import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types'
24+
import { validateMetadataUpdate } from '@/lib/table/columns/metadata'
2525
import { signalTableSchemaChanged } from '@/lib/table/events'
2626
import {
2727
accessError,
@@ -145,14 +145,17 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
145145
)
146146
}
147147

148+
// Which type-specific keys this payload carries, and which writer owns
149+
// each. Read from the registry rather than named here, so a new metadata
150+
// key routes correctly without touching this route.
151+
const { generic: genericMetadataKeys, dedicated: dedicatedMetadataKeys } =
152+
metadataKeysIn(updates)
153+
148154
// A retype applies and validates the constraints itself, so the separate
149155
// constraint write only runs when the type is unchanged. The rename rides
150156
// whichever write actually runs last.
151157
const typedWriteRuns =
152-
typeChanging ||
153-
updates.currencyCode !== undefined ||
154-
updates.options !== undefined ||
155-
updates.multiple !== undefined
158+
typeChanging || genericMetadataKeys.length > 0 || dedicatedMetadataKeys.length > 0
156159
const constraintsWriteRuns =
157160
!typedWriteRuns && (updates.required !== undefined || updates.unique !== undefined)
158161
const renameWithTypedWrite =
@@ -165,23 +168,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
165168
// changing: an options-only update on an existing select column carries the
166169
// same hazard as a conversion does.
167170
const resultingType = updates.type ?? currentColumn?.type
168-
if (updates.currencyCode !== undefined) {
169-
if (resultingType !== 'currency') {
170-
return NextResponse.json(
171-
{
172-
error: `Cannot set currency on column "${validated.columnName}" of type "${resultingType}"`,
173-
},
174-
{ status: 400 }
175-
)
176-
}
177-
if (!isSupportedCurrencyCode(updates.currencyCode)) {
178-
return NextResponse.json(
179-
{
180-
error: `Invalid currency code "${updates.currencyCode}". Use an ISO 4217 code, e.g. USD`,
181-
},
182-
{ status: 400 }
183-
)
184-
}
171+
const metadataError = validateMetadataUpdate(currentColumn, resultingType, updates)
172+
if (metadataError) {
173+
return NextResponse.json({ error: metadataError }, { status: 400 })
185174
}
186175
// The rename runs last (see below), so a name already taken would fail after
187176
// the typed write committed. This is the only rename failure a caller can
@@ -224,9 +213,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
224213
tableId,
225214
columnName: columnRef,
226215
newType: updates.type as NonNullable<typeof updates.type>,
227-
...(updates.options !== undefined ? { options: updates.options } : {}),
228-
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
229-
...(updates.currencyCode !== undefined ? { currencyCode: updates.currencyCode } : {}),
216+
// Every type-specific key the payload carries, whichever writer would
217+
// own it standalone: a conversion applies its target's metadata in the
218+
// same transaction rather than leaving it to a second write.
219+
...pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys]),
230220
// Forwarded so the conversion validates against the constraint this
231221
// same request is about to set, not the column's current one.
232222
...(updates.required !== undefined ? { required: updates.required } : {}),
@@ -235,22 +225,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
235225
},
236226
requestId
237227
)
238-
} else if (updates.currencyCode !== undefined) {
239-
// Re-denominating an existing currency column: schema-only, no cell
240-
// rewrite. Reached only when the type is unchanged — a conversion INTO
241-
// currency carries the code through `updateColumnType` above.
242-
updatedTable = await updateColumnCurrency(
228+
} else if (genericMetadataKeys.length > 0) {
229+
// Changing a column's own metadata — re-denominating a currency, changing
230+
// a number's precision. Usually schema-only; the type declares a cell
231+
// rewrite if it needs one. Reached only when the type is unchanged, since
232+
// a conversion carries its metadata through `updateColumnType` above.
233+
updatedTable = await updateColumnMetadata(
243234
{
244235
tableId,
245236
columnName: columnRef,
246-
currencyCode: updates.currencyCode,
237+
metadata: pickMetadata(updates, genericMetadataKeys),
247238
...(updates.required !== undefined ? { required: updates.required } : {}),
248239
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
249240
...renameWithTypedWrite,
250241
},
251242
requestId
252243
)
253-
} else if (updates.options !== undefined || updates.multiple !== undefined) {
244+
} else if (dedicatedMetadataKeys.length > 0) {
254245
updatedTable = await updateColumnOptions(
255246
{
256247
tableId,

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

Lines changed: 25 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,13 @@ import {
1414
deleteColumn,
1515
renameColumn,
1616
updateColumnConstraints,
17-
updateColumnCurrency,
17+
updateColumnMetadata,
1818
updateColumnOptions,
1919
updateColumnType,
2020
} from '@/lib/table'
2121
import { columnMatchesRef, getColumnId } from '@/lib/table/column-keys'
22-
import { columnTypeById } from '@/lib/table/column-types'
23-
import { isSupportedCurrencyCode } from '@/lib/table/currency'
22+
import { columnTypeById, metadataKeysIn, pickMetadata } from '@/lib/table/column-types'
23+
import { validateMetadataUpdate } from '@/lib/table/columns/metadata'
2424
import { signalTableSchemaChanged } from '@/lib/table/events'
2525
import {
2626
accessError,
@@ -179,14 +179,17 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
179179
)
180180
}
181181

182+
// Which type-specific keys this payload carries, and which writer owns
183+
// each. Read from the registry rather than named here, so a new metadata
184+
// key routes correctly without touching this route.
185+
const { generic: genericMetadataKeys, dedicated: dedicatedMetadataKeys } =
186+
metadataKeysIn(updates)
187+
182188
// A retype applies and validates the constraints itself, so the separate
183189
// constraint write only runs when the type is unchanged. The rename rides
184190
// whichever write actually runs last.
185191
const typedWriteRuns =
186-
typeChanging ||
187-
updates.currencyCode !== undefined ||
188-
updates.options !== undefined ||
189-
updates.multiple !== undefined
192+
typeChanging || genericMetadataKeys.length > 0 || dedicatedMetadataKeys.length > 0
190193
const constraintsWriteRuns =
191194
!typedWriteRuns && (updates.required !== undefined || updates.unique !== undefined)
192195
const renameWithTypedWrite =
@@ -199,23 +202,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
199202
// changing: an options-only update on an existing select column carries the
200203
// same hazard as a conversion does.
201204
const resultingType = updates.type ?? currentColumn?.type
202-
if (updates.currencyCode !== undefined) {
203-
if (resultingType !== 'currency') {
204-
return NextResponse.json(
205-
{
206-
error: `Cannot set currency on column "${validated.columnName}" of type "${resultingType}"`,
207-
},
208-
{ status: 400 }
209-
)
210-
}
211-
if (!isSupportedCurrencyCode(updates.currencyCode)) {
212-
return NextResponse.json(
213-
{
214-
error: `Invalid currency code "${updates.currencyCode}". Use an ISO 4217 code, e.g. USD`,
215-
},
216-
{ status: 400 }
217-
)
218-
}
205+
const metadataError = validateMetadataUpdate(currentColumn, resultingType, updates)
206+
if (metadataError) {
207+
return NextResponse.json({ error: metadataError }, { status: 400 })
219208
}
220209
// The rename runs last (see below), so a name already taken would fail after
221210
// the typed write committed. This is the only rename failure a caller can
@@ -258,9 +247,10 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
258247
tableId,
259248
columnName: columnRef,
260249
newType: updates.type as NonNullable<typeof updates.type>,
261-
...(updates.options !== undefined ? { options: updates.options } : {}),
262-
...(updates.multiple !== undefined ? { multiple: updates.multiple } : {}),
263-
...(updates.currencyCode !== undefined ? { currencyCode: updates.currencyCode } : {}),
250+
// Every type-specific key the payload carries, whichever writer would
251+
// own it standalone: a conversion applies its target's metadata in the
252+
// same transaction rather than leaving it to a second write.
253+
...pickMetadata(updates, [...genericMetadataKeys, ...dedicatedMetadataKeys]),
264254
// Forwarded so the conversion validates against the constraint this
265255
// same request is about to set, not the column's current one.
266256
...(updates.required !== undefined ? { required: updates.required } : {}),
@@ -269,22 +259,23 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
269259
},
270260
requestId
271261
)
272-
} else if (updates.currencyCode !== undefined) {
273-
// Re-denominating an existing currency column: schema-only, no cell
274-
// rewrite. Reached only when the type is unchanged — a conversion INTO
275-
// currency carries the code through `updateColumnType` above.
276-
updatedTable = await updateColumnCurrency(
262+
} else if (genericMetadataKeys.length > 0) {
263+
// Changing a column's own metadata — re-denominating a currency, changing
264+
// a number's precision. Usually schema-only; the type declares a cell
265+
// rewrite if it needs one. Reached only when the type is unchanged, since
266+
// a conversion carries its metadata through `updateColumnType` above.
267+
updatedTable = await updateColumnMetadata(
277268
{
278269
tableId,
279270
columnName: columnRef,
280-
currencyCode: updates.currencyCode,
271+
metadata: pickMetadata(updates, genericMetadataKeys),
281272
...(updates.required !== undefined ? { required: updates.required } : {}),
282273
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
283274
...renameWithTypedWrite,
284275
},
285276
requestId
286277
)
287-
} else if (updates.options !== undefined || updates.multiple !== undefined) {
278+
} else if (dedicatedMetadataKeys.length > 0) {
288279
updatedTable = await updateColumnOptions(
289280
{
290281
tableId,

0 commit comments

Comments
 (0)