Skip to content

Commit 0a6199b

Browse files
committed
fix(tables): keep the legacy numeric fallback for unknown filter fields
A backward-compatibility check on the whole change turned this up. Making the range-comparison cast registry-driven fixed the real bug — a text-shaped type emitting `(data->>'email')::numeric` and 500ing the rows query — but it also swept in fields with NO schema entry, which resolve to `string` through `columnTypeById`'s fallback and so began comparing lexicographically. Ad-hoc numeric fields have always compared numerically there, and `'10' > '5'` is false as text, so a saved filter on a field that is not in the schema (a typo, or a column since removed) would quietly return a different row set with no error at all. An absent column type is now distinguished from a known text type: unknown keeps `::numeric` and its operand validation exactly as before, while a known type still answers from the registry. Both cases are pinned by tests.
1 parent 08d0244 commit 0a6199b

2 files changed

Lines changed: 25 additions & 19 deletions

File tree

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

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -114,16 +114,13 @@ describe('SQL Builder', () => {
114114
expect(out).not.toContain('::timestamp')
115115
})
116116

117-
it('compares an unknown column type as TEXT, never with an invented cast', () => {
118-
// An unknown type resolves to `string` (`columnTypeById`'s documented
119-
// fallback), and `string` compares as text. The `?? 'numeric'` default
120-
// this replaced was reached by every type whose `jsonbCast` is null —
121-
// so a range filter on an `email` column emitted
122-
// `(data->>'email')::numeric`, which Postgres errors on at the first
123-
// non-numeric row and takes the whole rows query down with it.
117+
it('falls back to ::numeric for a field with no schema entry', () => {
118+
// An UNKNOWN field keeps the legacy numeric default — ad-hoc numeric
119+
// fields have always compared numerically, and switching them to
120+
// lexicographic ordering would change a saved filter's row set silently.
121+
// A known text-shaped type is a different question; see the case below.
124122
const out = render(buildFilterClause({ score: { $gte: 5 } }, TABLE, NO_COLUMNS))
125-
expect(out).toContain(`${TABLE}.data->>'score' >= `)
126-
expect(out).not.toContain('::numeric')
123+
expect(out).toContain(`(${TABLE}.data->>'score')::numeric >= `)
127124
expect(out).not.toContain('::timestamp')
128125
})
129126

@@ -381,14 +378,10 @@ describe('SQL Builder', () => {
381378
).toThrow(/column "birthDate" \(date\) requires a date string, got number/)
382379
})
383380

384-
it('accepts a string on an unknown column, which compares as text', () => {
385-
// Previously threw, because an unknown type fell through to a numeric
386-
// cast that then demanded a numeric operand. Text comparison has no such
387-
// requirement, and refusing a string on a column that renders as text
388-
// was the wrong answer.
381+
it('throws when $lt on an unknown column (numeric fallback) receives a string', () => {
389382
expect(() =>
390383
buildFilterClause({ score: { $lt: 'high' } } as Filter, TABLE, NO_COLUMNS)
391-
).not.toThrow()
384+
).toThrow(/requires a number, got string/)
392385
})
393386

394387
it('accepts valid number on number column', () => {

apps/sim/lib/table/sql.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -834,6 +834,19 @@ function buildComparisonClause(
834834
columnType: ColumnType | undefined
835835
): SQL {
836836
const escapedField = field.replace(/'/g, "''")
837+
838+
// A field with NO schema entry keeps the legacy `::numeric` default. It is
839+
// not a text-shaped column — it is an unknown one, and ad-hoc numeric fields
840+
// have always compared numerically here. Falling through to the registry's
841+
// `string` fallback would silently switch them to LEXICOGRAPHIC ordering,
842+
// where `'10' > '5'` is false — a saved filter would start returning a
843+
// different row set with no error.
844+
if (columnType === undefined) {
845+
validateComparisonValue(field, columnType, 'numeric', value)
846+
const cell = sql.raw(`(${tableName}.data->>'${escapedField}')::numeric`)
847+
return sql`${cell} ${sql.raw(operator)} ${value}`
848+
}
849+
837850
const definition = columnTypeById(columnType)
838851

839852
if (!definition.orderable) {
@@ -842,10 +855,10 @@ function buildComparisonClause(
842855
)
843856
}
844857

845-
// `jsonbCast === null` MEANS text comparison is correct — for `string` and
846-
// for every other text-shaped type. Reading the registry rather than testing
847-
// for `'string'` is what stops a new type falling past this into the cast
848-
// below: `(data->>'email')::numeric` errors in Postgres on the first
858+
// For a KNOWN type, `jsonbCast === null` means text comparison is correct —
859+
// for `string` and for every other text-shaped type. Reading the registry
860+
// rather than testing for `'string'` is what stops a new type falling into
861+
// the cast below: `(data->>'email')::numeric` errors in Postgres on the first
849862
// non-numeric row, taking the whole rows query down with it.
850863
const cast = definition.jsonbCast
851864
if (cast === null) {

0 commit comments

Comments
 (0)