Skip to content

Commit 7c0c24d

Browse files
fix(tables): block required blank conversions; sort multiselect by option name
`required` only rejects null/undefined on a write, so a required string column legitimately holds ''. Converting it to a required select stored null (or [] for a multi), and every later update of that row then failed its own required check. Reject a blank source value when the target select is required. Multiselect sort compared the raw id-array text, since `["opt_b","opt_a"]` matches no single-id CASE branch. It now resolves elements to names and joins them in stored order — the same text the grid renders and an export writes.
1 parent a0c9bba commit 7c0c24d

4 files changed

Lines changed: 66 additions & 9 deletions

File tree

apps/sim/lib/table/__tests__/column-conversion.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,17 @@ describe('isValueCompatibleWithType — select cardinality', () => {
7979
expect(isValueCompatibleWithType(flattened, 'select', OPTIONS, false)).toBe(false)
8080
})
8181

82+
it('rejects a blank source value when the target select is required', () => {
83+
// `required` only rejects null/undefined on a write, so a required string
84+
// column legitimately holds ''. Converting it stores null (or [] for a
85+
// multi), which would then fail that column's own required check on the
86+
// next update of the row.
87+
expect(isValueCompatibleWithType('', 'select', OPTIONS, false, true)).toBe(false)
88+
expect(isValueCompatibleWithType('', 'select', OPTIONS, true, true)).toBe(false)
89+
// Optional target is unchanged — a cleared cell stays convertible.
90+
expect(isValueCompatibleWithType('', 'select', OPTIONS, false, false)).toBe(true)
91+
})
92+
8293
it('still rejects a comma string holding an undeclared option', () => {
8394
expect(isValueCompatibleWithType('Alpha, Gone', 'select', OPTIONS, true)).toBe(false)
8495
})

apps/sim/lib/table/__tests__/sql.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,18 @@ describe('SQL Builder', () => {
516516
expect(out).toContain("WHEN 'opt_closed' THEN 'Closed'")
517517
expect(out.trim().endsWith('ASC NULLS LAST')).toBe(true)
518518
})
519+
520+
it('sorts a multiselect by its option names, not the raw id array', () => {
521+
// A multi cell extracts as `["opt_b","opt_a"]`, which matches no single-id
522+
// CASE branch — without the array arm it would order on that opaque text.
523+
const out = render(buildSortClause({ tags: 'asc' }, TABLE, [tagsCol]))
524+
expect(out).toContain('jsonb_array_elements_text')
525+
expect(out).toContain('string_agg')
526+
expect(out).toContain('ORDER BY e.ord')
527+
// The scalar arm survives for values left over from a single→multi toggle.
528+
expect(out).toContain('CASE')
529+
expect(out.trim().endsWith('ASC NULLS LAST')).toBe(true)
530+
})
519531
})
520532

521533
describe('Field name validation', () => {

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

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,15 @@ export async function updateColumnType(
541541

542542
const effective = convertingAwayFromSelect ? selectValueForConversion(column, value) : value
543543

544-
if (!isValueCompatibleWithType(effective, data.newType, targetOptions, !!targetMultiple)) {
544+
if (
545+
!isValueCompatibleWithType(
546+
effective,
547+
data.newType,
548+
targetOptions,
549+
!!targetMultiple,
550+
!!column.required
551+
)
552+
) {
545553
incompatibleCount++
546554
}
547555
}
@@ -1015,7 +1023,8 @@ export function isValueCompatibleWithType(
10151023
value: unknown,
10161024
targetType: (typeof COLUMN_TYPES)[number],
10171025
targetOptions: SelectOption[] = [],
1018-
targetMultiple = false
1026+
targetMultiple = false,
1027+
targetRequired = false
10191028
): boolean {
10201029
if (value === null || value === undefined) return true
10211030

@@ -1026,8 +1035,12 @@ export function isValueCompatibleWithType(
10261035
// this check, so anything still structured here is genuinely lossy.
10271036
return typeof value !== 'object'
10281037
case 'select': {
1029-
// A cleared select cell is written as '' — still convertible.
1030-
if (value === '') return true
1038+
// A cleared select cell is written as '' — still convertible, unless the
1039+
// target is required. Required only rejects null/undefined on a write, so
1040+
// a required string column legitimately holds ''; the migration turns that
1041+
// into null (or [] for a multi), and every later update of that row would
1042+
// then fail its own required check.
1043+
if (value === '') return !targetRequired
10311044
// Read the value exactly as the write-path coercion will. A multi target
10321045
// splits a comma-delimited string, so a multiselect → text → multiselect
10331046
// round-trip (text holding this feature's own `Bug, Docs` export shape)

apps/sim/lib/table/sql.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -662,10 +662,13 @@ function buildSortFieldClause(
662662

663663
// Select cells store opaque option ids; sort by the option **name** so ordering
664664
// is alphabetical by the label the user sees, not by the internal id. A stored
665-
// id with no matching option (deleted) falls back to the raw text. Multiselect
666-
// renders as a JSON array text, which won't match any id and sorts by that text.
665+
// id with no matching option (deleted) falls back to the raw text.
667666
if (column?.type === 'select') {
668-
const orderExpr = buildSelectNameOrderExpr(jsonbExtract, column)
667+
const orderExpr = buildSelectNameOrderExpr(
668+
jsonbExtract,
669+
`${tableName}.data->'${escapedField}'`,
670+
column
671+
)
669672
return sql.raw(`${orderExpr} ${directionSql} NULLS LAST`)
670673
}
671674

@@ -685,12 +688,30 @@ function buildSortFieldClause(
685688
* text extract) to its option name, so an ORDER BY sorts alphabetically by label.
686689
* Ids and names are SQL-escaped and embedded literally (options are trusted schema
687690
* data, not caller input); an unmapped id falls through to the raw extract.
691+
*
692+
* A multiselect cell is an array of ids, which matches no single-id branch — it
693+
* sorts on its elements resolved to names and joined in stored order, the same
694+
* text the grid renders and an export writes. A scalar left over from before a
695+
* single→multi toggle still takes the single-id branch.
688696
*/
689-
function buildSelectNameOrderExpr(jsonbExtract: string, column: ColumnDefinition): string {
697+
function buildSelectNameOrderExpr(
698+
jsonbExtract: string,
699+
jsonbValue: string,
700+
column: ColumnDefinition
701+
): string {
690702
const options = column.options ?? []
691703
if (options.length === 0) return jsonbExtract
692704
const whens = options
693705
.map((o) => `WHEN '${o.id.replace(/'/g, "''")}' THEN '${o.name.replace(/'/g, "''")}'`)
694706
.join(' ')
695-
return `CASE ${jsonbExtract} ${whens} ELSE ${jsonbExtract} END`
707+
const singleExpr = `CASE ${jsonbExtract} ${whens} ELSE ${jsonbExtract} END`
708+
if (!column.multiple) return singleExpr
709+
710+
const nameById = JSON.stringify(
711+
Object.fromEntries(new Map(options.map((o) => [o.id, o.name])))
712+
).replace(/'/g, "''")
713+
return `CASE WHEN jsonb_typeof(${jsonbValue}) = 'array' THEN (
714+
SELECT string_agg(COALESCE('${nameById}'::jsonb ->> e.v, e.v), ', ' ORDER BY e.ord)
715+
FROM jsonb_array_elements_text(${jsonbValue}) WITH ORDINALITY AS e(v, ord)
716+
) ELSE ${singleExpr} END`
696717
}

0 commit comments

Comments
 (0)