Skip to content

Commit ccbb065

Browse files
committed
fix(tables): reject a flattened list as an amount; fold constraints into every typed write
Two findings from round 11. Multi-select converted to nonsense amounts. `selectValueForConversion` flattens a multi cell to its comma-joined option names, and the parser read that as a formatted number: options 12 and 34 became 12.34, and 100 and 200 became 100200. No real amount puts whitespace after a separator, but a delimited list does — so a separator followed by whitespace is now refused. Every legitimate form still parses, including space-grouped locales. Combined options-or-currency + constraints could still commit partially. Those two writes now carry constraints the same way the retype does, through one shared `applyConstraints` that validates (workflow-output, empty cells for required, supportsUnique and duplicates for unique) and applies them. The separate constraint write now runs only when no typed write does. Three copies of those rules is the drift that produced the original required-check bug, so they live in one place.
1 parent de16494 commit ccbb065

5 files changed

Lines changed: 110 additions & 12 deletions

File tree

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
139139
// A retype applies and validates the constraints itself, so the separate
140140
// constraint write only runs when the type is unchanged. The rename rides
141141
// whichever write actually runs last.
142+
const typedWriteRuns =
143+
typeChanging ||
144+
updates.currencyCode !== undefined ||
145+
updates.options !== undefined ||
146+
updates.multiple !== undefined
142147
const constraintsWriteRuns =
143-
!typeChanging && (updates.required !== undefined || updates.unique !== undefined)
148+
!typedWriteRuns && (updates.required !== undefined || updates.unique !== undefined)
144149
const renameWithTypedWrite =
145150
updates.name && !constraintsWriteRuns ? { newName: updates.name } : {}
146151

@@ -230,6 +235,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
230235
tableId,
231236
columnName: columnRef,
232237
currencyCode: updates.currencyCode,
238+
...(updates.required !== undefined ? { required: updates.required } : {}),
239+
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
233240
...renameWithTypedWrite,
234241
},
235242
requestId
@@ -244,15 +251,16 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
244251
// Forwarded so the removal guard validates against the constraint this
245252
// same request is about to set, not the column's current one.
246253
...(updates.required !== undefined ? { required: updates.required } : {}),
254+
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
247255
...renameWithTypedWrite,
248256
},
249257
requestId
250258
)
251259
}
252260

253-
// Skipped when the type changed: that write already applied and validated
254-
// these, in one transaction with the conversion.
255-
if (!typeChanging && (updates.required !== undefined || updates.unique !== undefined)) {
261+
// Skipped whenever a typed write ran: that write already applied and
262+
// validated these, in one transaction with the change they accompany.
263+
if (constraintsWriteRuns) {
256264
updatedTable = await updateColumnConstraints(
257265
{
258266
tableId,

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
173173
// A retype applies and validates the constraints itself, so the separate
174174
// constraint write only runs when the type is unchanged. The rename rides
175175
// whichever write actually runs last.
176+
const typedWriteRuns =
177+
typeChanging ||
178+
updates.currencyCode !== undefined ||
179+
updates.options !== undefined ||
180+
updates.multiple !== undefined
176181
const constraintsWriteRuns =
177-
!typeChanging && (updates.required !== undefined || updates.unique !== undefined)
182+
!typedWriteRuns && (updates.required !== undefined || updates.unique !== undefined)
178183
const renameWithTypedWrite =
179184
updates.name && !constraintsWriteRuns ? { newName: updates.name } : {}
180185

@@ -264,6 +269,8 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
264269
tableId,
265270
columnName: columnRef,
266271
currencyCode: updates.currencyCode,
272+
...(updates.required !== undefined ? { required: updates.required } : {}),
273+
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
267274
...renameWithTypedWrite,
268275
},
269276
requestId
@@ -278,15 +285,16 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Colu
278285
// Forwarded so the removal guard validates against the constraint this
279286
// same request is about to set, not the column's current one.
280287
...(updates.required !== undefined ? { required: updates.required } : {}),
288+
...(updates.unique !== undefined ? { unique: updates.unique } : {}),
281289
...renameWithTypedWrite,
282290
},
283291
requestId
284292
)
285293
}
286294

287-
// Skipped when the type changed: that write already applied and validated
288-
// these, in one transaction with the conversion.
289-
if (!typeChanging && (updates.required !== undefined || updates.unique !== undefined)) {
295+
// Skipped whenever a typed write ran: that write already applied and
296+
// validated these, in one transaction with the change they accompany.
297+
if (constraintsWriteRuns) {
290298
updatedTable = await updateColumnConstraints(
291299
{
292300
tableId,

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

Lines changed: 73 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { and, count, eq, sql } from 'drizzle-orm'
1717
import { columnMatchesRef, generateColumnId, getColumnId } from '@/lib/table/column-keys'
1818
import {
1919
columnTypeById,
20+
columnTypeOf,
2021
isValueCompatible,
2122
TYPE_SPECIFIC_COLUMN_KEYS,
2223
} from '@/lib/table/column-types'
@@ -485,6 +486,55 @@ export async function deleteColumns(
485486
return def
486487
}
487488

489+
/**
490+
* Validates a constraint change against the column's stored data, and returns
491+
* the column with those constraints applied.
492+
*
493+
* Shared by every write that can carry constraints, for the reason the
494+
* duplicate scan and {@link countEmptyCells} are shared: three copies of these
495+
* rules is the drift that produced the original required-check bug. Applying
496+
* them in the same write as the change they accompany is what stops a combined
497+
* request from committing one half and then failing on the other.
498+
*/
499+
async function applyConstraints(
500+
trx: DbTransaction,
501+
tableId: string,
502+
column: ColumnDefinition,
503+
columnKey: string,
504+
data: { required?: boolean; unique?: boolean }
505+
): Promise<ColumnDefinition> {
506+
if (data.required === undefined && data.unique === undefined) return column
507+
508+
if (column.workflowGroupId) {
509+
throw new Error(
510+
`Cannot change constraints on workflow-output column "${column.name}". Constraints aren't applicable to columns whose values come from workflow execution.`
511+
)
512+
}
513+
if (data.required === true && !column.required) {
514+
const emptyCount = await countEmptyCells(trx, tableId, columnKey)
515+
if (emptyCount > 0) {
516+
throw new Error(
517+
`Cannot set column "${column.name}" as required: ${emptyCount} row(s) have null, missing, or empty values`
518+
)
519+
}
520+
}
521+
if (data.unique === true && !column.unique) {
522+
if (!columnTypeOf(column).supportsUnique) {
523+
throw new Error(
524+
`Cannot set column "${column.name}" as unique: ${column.type} columns compare stored values that would allow only one row per value.`
525+
)
526+
}
527+
if (await hasDuplicateValues(trx, tableId, columnKey)) {
528+
throw new Error(`Cannot set column "${column.name}" as unique: duplicate values exist`)
529+
}
530+
}
531+
return {
532+
...column,
533+
...(data.required !== undefined ? { required: data.required } : {}),
534+
...(data.unique !== undefined ? { unique: data.unique } : {}),
535+
}
536+
}
537+
488538
/** Persists a column list as the table's schema and returns the updated definition. */
489539
async function persistColumns(
490540
trx: DbTransaction,
@@ -964,7 +1014,14 @@ export async function updateColumnOptions(
9641014
throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`)
9651015
}
9661016

967-
const withOptions = schema.columns.map((c, i) => (i === columnIndex ? updatedColumn : c))
1017+
const constrainedColumn = await applyConstraints(
1018+
trx,
1019+
data.tableId,
1020+
updatedColumn,
1021+
columnKey,
1022+
data
1023+
)
1024+
const withOptions = schema.columns.map((c, i) => (i === columnIndex ? constrainedColumn : c))
9681025
const updatedColumns = withOptions.map((c, i) =>
9691026
i === columnIndex ? applyPendingRename(withOptions, columnIndex, data.newName) : c
9701027
)
@@ -1129,13 +1186,25 @@ export async function updateColumnCurrency(
11291186
throw new Error(`Invalid column: ${columnValidation.errors.join('; ')}`)
11301187
}
11311188

1132-
// Only a no-op when the currency is unchanged AND no rename is riding along.
1189+
const constrained = await applyConstraints(
1190+
trx,
1191+
data.tableId,
1192+
updatedColumn,
1193+
getColumnId(column),
1194+
data
1195+
)
1196+
1197+
// Only a no-op when nothing at all changed — currency, constraints, name.
11331198
const renamePending = data.newName !== undefined && data.newName !== column.name
1134-
if (updatedColumn.currencyCode === column.currencyCode && !renamePending) {
1199+
if (
1200+
constrained === updatedColumn &&
1201+
updatedColumn.currencyCode === column.currencyCode &&
1202+
!renamePending
1203+
) {
11351204
return table
11361205
}
11371206

1138-
const withCurrency = schema.columns.map((c, i) => (i === columnIndex ? updatedColumn : c))
1207+
const withCurrency = schema.columns.map((c, i) => (i === columnIndex ? constrained : c))
11391208
const updatedColumns = withCurrency.map((c, i) =>
11401209
i === columnIndex ? applyPendingRename(withCurrency, columnIndex, data.newName) : c
11411210
)

apps/sim/lib/table/currency.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ function hasValidGrouping(text: string, separator: string): boolean {
2929
return /^\d{1,3}$/.test(groups[0]) && groups.slice(1).every((group) => /^\d{3}$/.test(group))
3030
}
3131

32+
/** A decimal/grouping separator followed by whitespace — a list, not an amount. */
33+
const SEPARATOR_THEN_SPACE = /[.,]\s/
34+
3235
/** An `e` with a digit on both sides — an exponent marker, however spaced. */
3336
const INTERIOR_EXPONENT = /\d\s*[eE][+-]?\s*\d/
3437

@@ -148,6 +151,12 @@ export function parseCurrencyInput(raw: unknown): number | null {
148151
const parenthesized = /^\((.*)\)$/.exec(trimmed)
149152
const body = parenthesized ? parenthesized[1] : trimmed
150153

154+
// No real amount puts whitespace after a separator, but a delimited LIST
155+
// does — and a multi-select column flattens to exactly that when it converts.
156+
// Without this, `12, 34` reads as 12.34 and `100, 200` as 100200, so a
157+
// multi-select column of numeric option names would convert to nonsense.
158+
if (SEPARATOR_THEN_SPACE.test(body)) return null
159+
151160
// Exponent form is taken at face value. `String()` emits it for any magnitude
152161
// past 1e21, so a stored amount round-trips through the editor as `1e+21` —
153162
// and stripping the `e` as decoration would read that back as 121, silently

apps/sim/lib/table/types.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -816,6 +816,8 @@ export interface UpdateColumnOptionsData {
816816
* what stops a combined request from committing one half and then failing.
817817
*/
818818
newName?: string
819+
/** Constraints to apply in the SAME transaction as this write. */
820+
unique?: boolean
819821
options: SelectOption[]
820822
/** Toggle single/multi selection alongside the options update. */
821823
multiple?: boolean
@@ -841,6 +843,8 @@ export interface UpdateColumnCurrencyData {
841843
* what stops a combined request from committing one half and then failing.
842844
*/
843845
newName?: string
846+
/** Constraints to apply in the SAME transaction as this write. */
847+
unique?: boolean
844848
currencyCode: string
845849
}
846850

0 commit comments

Comments
 (0)