Skip to content

Commit 08d0244

Browse files
committed
fix(tables): canonicalize filter operands where the column is always known
Greptile was right that the earlier fix was incomplete. Canonicalization lived in the UI-facing converters, which only run it when the caller supplies column definitions — and the workflow Table blocks build a filter from user input long before any schema exists, so they call those converters with none. A workflow filtering an Email column for `Ada@Example.com`, or a Phone column for `020 1234 5678`, compiled an operand that never met the stored value and silently returned no rows. Moved to `fieldPredicate`, which is the one place every filter arrives with its column in hand: both wire grammars compile through it, so raw API callers and the copilot tool are covered too. The converter pass stays as a convenience for the filter UI rather than as the guarantee. Only TEXT operands are canonicalized. `date.coerce` accepts an epoch number, so running a number through it would silently reinterpret `{ birthDate: { $gte: 1704067200000 } }` as a date instead of letting the range validator reject a likely mistake. Opaque ids are skipped — they are resolved from option names upstream, where the option set is known — and an operand the type rejects is left alone rather than nulled, so the error still names what the user typed. Also routes CSV number coercion AND number inference through the same decimal parser inline edits use. A CSV could import `0x10` as 16 where typing it was rejected; had only coercion been fixed, a hex column would have inferred `number` and then nulled every cell.
1 parent 33af575 commit 08d0244

3 files changed

Lines changed: 179 additions & 29 deletions

File tree

apps/sim/lib/table/__tests__/column-types-contact.test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { metadataMigrationFor } from '@/lib/table/column-types/registry.server'
2323
import { buildConvertedColumn } from '@/lib/table/columns/service'
2424
import { coerceValue } from '@/lib/table/import'
2525
import { filterRulesToFilter, prunePredicateForColumns } from '@/lib/table/query-builder/converters'
26+
import { buildFilterClause } from '@/lib/table/sql'
2627
import type { ColumnDefinition } from '@/lib/table/types'
2728

2829
const column = (type: ColumnDefinition['type'], extra: Partial<ColumnDefinition> = {}) =>
@@ -510,6 +511,81 @@ describe('final-audit regressions', () => {
510511
})
511512
})
512513

514+
describe('server-side operand canonicalization', () => {
515+
const emailCol: ColumnDefinition = { id: 'c', name: 'c', type: 'email' }
516+
const phoneCol: ColumnDefinition = { id: 'c', name: 'c', type: 'phone' }
517+
const render = (clause: unknown) => JSON.stringify(clause)
518+
519+
it('canonicalizes even when the caller supplies NO column definitions', () => {
520+
// The workflow Table blocks build a filter long before any schema is known
521+
// and call the converters with no columns, so the client-side pass is
522+
// skipped entirely. The SQL builder is the one place the column is always
523+
// in hand, which is why the guarantee lives there.
524+
const withoutColumns = filterRulesToFilter(
525+
[
526+
{
527+
id: 'r1',
528+
logicalOperator: 'and' as const,
529+
column: 'c',
530+
operator: 'eq',
531+
value: 'Ada@Example.COM',
532+
},
533+
],
534+
[]
535+
)
536+
// The converter left it raw...
537+
expect(withoutColumns).toEqual({ c: 'Ada@Example.COM' })
538+
// ...and the SQL builder still compiles the canonical operand.
539+
const clause = buildFilterClause(withoutColumns as Filter, 'user_table_rows', [emailCol])
540+
expect(render(clause)).toContain('ada@example.com')
541+
expect(render(clause)).not.toContain('Ada@Example.COM')
542+
})
543+
544+
it('canonicalizes a phone operand arriving formatted from a workflow', () => {
545+
const clause = buildFilterClause(
546+
{ c: { $eq: '+44 20 7123 4567' } } as Filter,
547+
'user_table_rows',
548+
[phoneCol]
549+
)
550+
expect(render(clause)).toContain('+442071234567')
551+
})
552+
553+
it('normalizes a fragment for a text match, not the whole value', () => {
554+
const clause = buildFilterClause(
555+
{ c: { $contains: '(555) 123' } } as Filter,
556+
'user_table_rows',
557+
[phoneCol]
558+
)
559+
expect(render(clause)).toContain('555123')
560+
})
561+
562+
it('is idempotent, so an already-canonical operand is unchanged', () => {
563+
const once = buildFilterClause({ c: { $eq: 'ada@example.com' } } as Filter, 'user_table_rows', [
564+
emailCol,
565+
])
566+
expect(render(once)).toContain('ada@example.com')
567+
})
568+
569+
it('leaves a non-text operand to the existing validation', () => {
570+
// `date.coerce` accepts an epoch, so canonicalizing a NUMBER would silently
571+
// reinterpret a likely mistake instead of rejecting it.
572+
expect(() =>
573+
buildFilterClause({ d: { $gte: 1704067200000 } } as Filter, 'user_table_rows', [
574+
{ id: 'd', name: 'd', type: 'date' },
575+
])
576+
).toThrow(/requires a date string, got number/)
577+
})
578+
579+
it('leaves an unparseable operand alone rather than nulling it', () => {
580+
const clause = buildFilterClause(
581+
{ c: { $eq: 'not an address' } } as Filter,
582+
'user_table_rows',
583+
[emailCol]
584+
)
585+
expect(render(clause)).toContain('not an address')
586+
})
587+
})
588+
513589
describe('date includeTime', () => {
514590
it('truncates to a calendar day only when includeTime is explicitly false', () => {
515591
const dateOnly = column('date', { includeTime: false })

apps/sim/lib/table/import.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { getColumnId } from '@/lib/table/column-keys'
1616
import { type ColumnType, columnTypeById } from '@/lib/table/column-types'
1717
import { parseCurrencyInput } from '@/lib/table/currency'
1818
import { type NormalizeDateCellOptions, normalizeDateCellValue } from '@/lib/table/dates'
19+
import { parseDecimalNumber } from '@/lib/table/numeric'
1920
import type { ColumnDefinition, JsonValue, RowData, TableSchema } from '@/lib/table/types'
2021

2122
/**
@@ -295,10 +296,10 @@ export function inferColumnType(values: unknown[]): InferredCsvColumnType {
295296
const nonEmpty = values.filter((v) => v !== null && v !== undefined && v !== '')
296297
if (nonEmpty.length === 0) return 'string'
297298

298-
const allNumber = nonEmpty.every((v) => {
299-
const n = Number(v)
300-
return !Number.isNaN(n) && String(v).trim() !== ''
301-
})
299+
// The same parser `coerceValue` uses below. If inference read `0x10` as a
300+
// number while coercion rejected it, the column would be created as `number`
301+
// and then have every one of those cells nulled on write.
302+
const allNumber = nonEmpty.every((v) => parseDecimalNumber(v) !== null)
302303
if (allNumber) return 'number'
303304

304305
const allBoolean = nonEmpty.every((v) => {
@@ -405,10 +406,11 @@ export function coerceValue(
405406
): string | number | boolean | null | Record<string, unknown> | unknown[] {
406407
if (value === null || value === undefined || value === '') return null
407408
switch (colType) {
408-
case 'number': {
409-
const n = Number(value)
410-
return Number.isNaN(n) ? null : n
411-
}
409+
case 'number':
410+
// The same decimal parser inline edits use. Bare `Number()` read `0x10`
411+
// as 16 and `Infinity` as infinity, so a CSV could import values the
412+
// grid would reject if typed.
413+
return parseDecimalNumber(value)
412414
// Importing into an existing currency column: the file carries the
413415
// formatted amount (`$1,234.56`) but the cell stores a bare number. The
414416
// column's currency is forwarded because it decides how a lone separator

apps/sim/lib/table/sql.ts

Lines changed: 93 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
columnTypeOf,
1515
filterOperatorsFor,
1616
MULTI_SELECT_OPERATORS,
17+
normalizeFilterFragment,
1718
predicateOperatorsFor,
1819
SINGLE_SELECT_OPERATORS,
1920
storesMultipleValues,
@@ -474,12 +475,25 @@ export function fieldPredicate(
474475
)
475476
}
476477

478+
// Read the operand the way the column stores its cells. This is the one place
479+
// every filter reaches with its column in hand — both wire grammars compile
480+
// through here — so canonicalizing at this point covers callers that cannot
481+
// supply column definitions at all: the workflow Table blocks build a filter
482+
// from user input long before any schema is known, and a raw API caller need
483+
// not know the storage form either.
484+
//
485+
// The client-side pass in `converters.ts` is a convenience for the filter UI,
486+
// not the guarantee. When it was the only pass, a workflow filtering an Email
487+
// column for `Ada@Example.com` compiled to an operand that never met the
488+
// stored `ada@example.com` and silently returned no rows.
489+
const operand = column ? canonicalizeOperand(value, op, column) : value
490+
477491
if (isMultiSelect) {
478492
switch (op) {
479493
case 'contains':
480-
return buildArrayMembershipClause(tableName, field, value as JsonValue)
494+
return buildArrayMembershipClause(tableName, field, operand as JsonValue)
481495
case 'ncontains':
482-
return sql`NOT (${buildArrayMembershipClause(tableName, field, value as JsonValue)})`
496+
return sql`NOT (${buildArrayMembershipClause(tableName, field, operand as JsonValue)})`
483497
case 'isEmpty':
484498
return buildEmptyClause(tableName, field, true, true)
485499
case 'isNotEmpty':
@@ -491,55 +505,55 @@ export function fieldPredicate(
491505

492506
switch (op) {
493507
case 'eq':
494-
return buildContainmentClause(tableName, field, value as JsonValue)
508+
return buildContainmentClause(tableName, field, operand as JsonValue)
495509

496510
case 'ne':
497-
return sql`NOT (${buildContainmentClause(tableName, field, value as JsonValue)})`
511+
return sql`NOT (${buildContainmentClause(tableName, field, operand as JsonValue)})`
498512

499513
case 'gt':
500-
return buildComparisonClause(tableName, field, '>', value as number | string, columnType)
514+
return buildComparisonClause(tableName, field, '>', operand as number | string, columnType)
501515
case 'gte':
502-
return buildComparisonClause(tableName, field, '>=', value as number | string, columnType)
516+
return buildComparisonClause(tableName, field, '>=', operand as number | string, columnType)
503517
case 'lt':
504-
return buildComparisonClause(tableName, field, '<', value as number | string, columnType)
518+
return buildComparisonClause(tableName, field, '<', operand as number | string, columnType)
505519
case 'lte':
506-
return buildComparisonClause(tableName, field, '<=', value as number | string, columnType)
520+
return buildComparisonClause(tableName, field, '<=', operand as number | string, columnType)
507521

508522
case 'in': {
509-
if (!Array.isArray(value) || value.length === 0) return undefined
510-
if (value.length === 1) return buildContainmentClause(tableName, field, value[0])
511-
const inConditions = value.map((v) => buildContainmentClause(tableName, field, v))
523+
if (!Array.isArray(operand) || operand.length === 0) return undefined
524+
if (operand.length === 1) return buildContainmentClause(tableName, field, operand[0])
525+
const inConditions = operand.map((v) => buildContainmentClause(tableName, field, v))
512526
return sql`(${sql.join(inConditions, sql.raw(' OR '))})`
513527
}
514528

515529
case 'nin': {
516-
if (!Array.isArray(value) || value.length === 0) return undefined
517-
const ninConditions = value.map(
530+
if (!Array.isArray(operand) || operand.length === 0) return undefined
531+
const ninConditions = operand.map(
518532
(v) => sql`NOT (${buildContainmentClause(tableName, field, v)})`
519533
)
520534
return sql`(${sql.join(ninConditions, sql.raw(' AND '))})`
521535
}
522536

523537
case 'contains':
524-
return buildLikeClause(tableName, field, value as string, 'contains')
538+
return buildLikeClause(tableName, field, operand as string, 'contains')
525539
case 'ncontains':
526-
return buildLikeClause(tableName, field, value as string, 'contains', { negate: true })
540+
return buildLikeClause(tableName, field, operand as string, 'contains', { negate: true })
527541
case 'startsWith':
528-
return buildLikeClause(tableName, field, value as string, 'startsWith')
542+
return buildLikeClause(tableName, field, operand as string, 'startsWith')
529543
case 'endsWith':
530-
return buildLikeClause(tableName, field, value as string, 'endsWith')
544+
return buildLikeClause(tableName, field, operand as string, 'endsWith')
531545

532546
case 'like':
533-
return buildPatternClause(tableName, field, value as string, { caseInsensitive: false })
547+
return buildPatternClause(tableName, field, operand as string, { caseInsensitive: false })
534548
case 'ilike':
535-
return buildPatternClause(tableName, field, value as string, { caseInsensitive: true })
549+
return buildPatternClause(tableName, field, operand as string, { caseInsensitive: true })
536550
case 'nlike':
537-
return buildPatternClause(tableName, field, value as string, {
551+
return buildPatternClause(tableName, field, operand as string, {
538552
caseInsensitive: false,
539553
negate: true,
540554
})
541555
case 'nilike':
542-
return buildPatternClause(tableName, field, value as string, {
556+
return buildPatternClause(tableName, field, operand as string, {
543557
caseInsensitive: true,
544558
negate: true,
545559
})
@@ -754,6 +768,64 @@ function buildArrayMembershipClause(tableName: string, field: string, value: Jso
754768
* Cannot use the GIN index — falls back to a sequential scan over the table's
755769
* rows (bounded by the btree prefix on `table_id`).
756770
*/
771+
/**
772+
* Canonicalizes a filter operand into the shape the column stores.
773+
*
774+
* Three cases, because "the value the user typed" relates to storage three
775+
* different ways:
776+
*
777+
* - **Opaque ids** (`select`) are resolved from option NAMES to ids upstream by
778+
* `resolveFilterSelectValues`, which needs the whole option set. Touching
779+
* them here would re-coerce an already-resolved id.
780+
* - **Text matches** take a FRAGMENT, which is not a whole value and so cannot
781+
* go through `coerce` — a partial phone number fails validation. The type
782+
* normalizes it instead, if its canonical form drops characters.
783+
* - **Everything else** goes through the type's own `coerce`, which is by
784+
* definition how the cell was written. Idempotent, so an operand a caller
785+
* already canonicalized passes through unchanged.
786+
*
787+
* An operand `coerce` rejects is left alone rather than nulled: the value is
788+
* the user's, and the range-operator validator below still gets to reject it
789+
* with a message naming what they actually typed.
790+
*/
791+
function canonicalizeOperand(
792+
value: JsonValue | undefined,
793+
op: FilterOp,
794+
column: ColumnDefinition
795+
): JsonValue | undefined {
796+
if (value === undefined || value === null) return value
797+
const definition = columnTypeOf(column)
798+
if (definition.storesOpaqueIds || !definition.canonicalizesValues) return value
799+
800+
if (TEXT_MATCH_OPS.has(op)) {
801+
return typeof value === 'string' ? normalizeFilterFragment(column, value) : value
802+
}
803+
if (Array.isArray(value)) {
804+
return value.map((entry) =>
805+
typeof entry === 'string' ? canonicalizeText(entry, column) : entry
806+
)
807+
}
808+
return typeof value === 'string' ? canonicalizeText(value, column) : value
809+
}
810+
811+
/**
812+
* Runs one TEXT operand through the column's own coercion.
813+
*
814+
* Strings only, deliberately. Canonicalization exists to reconcile what a user
815+
* TYPED with how the cell was stored; an operand that arrived as another type
816+
* was not typed as text and keeps its existing path — which matters because
817+
* `date.coerce` accepts an epoch number, so canonicalizing one would silently
818+
* reinterpret `{ birthDate: { $gte: 1704067200000 } }` as a date rather than
819+
* letting `validateComparisonValue` reject a likely mistake.
820+
*/
821+
function canonicalizeText(value: string, column: ColumnDefinition): JsonValue {
822+
const coerced = columnTypeOf(column).coerce(value, column)
823+
return coerced.ok ? coerced.value : value
824+
}
825+
826+
/** Operators whose operand is a search fragment rather than a whole value. */
827+
const TEXT_MATCH_OPS = new Set<FilterOp>(['contains', 'ncontains', 'startsWith', 'endsWith'])
828+
757829
function buildComparisonClause(
758830
tableName: string,
759831
field: string,

0 commit comments

Comments
 (0)