diff --git a/.changeset/fix-electric-null-params.md b/.changeset/fix-electric-null-params.md new file mode 100644 index 000000000..b97c395e3 --- /dev/null +++ b/.changeset/fix-electric-null-params.md @@ -0,0 +1,12 @@ +--- +"@tanstack/electric-db-collection": patch +--- + +Fix invalid Electric proxy queries with missing params for null/undefined values + +When comparison operators were used with null/undefined values, the SQL compiler would generate placeholders ($1, $2) in the WHERE clause but skip adding the params to the dictionary. This resulted in invalid queries being sent to Electric. + +Now: + +- `eq(col, null)` and `eq(col, undefined)` transform to `"col" IS NULL` syntax +- Other comparisons (gt, lt, gte, lte, like, ilike) with null/undefined throw a clear error since null comparisons don't make semantic sense in SQL diff --git a/docs/design/eq-null-handling.md b/docs/design/eq-null-handling.md new file mode 100644 index 000000000..83dac8208 --- /dev/null +++ b/docs/design/eq-null-handling.md @@ -0,0 +1,105 @@ +# Handling Null Values in Comparison Operators + +## Background + +PR #765 introduced 3-valued logic (true/false/unknown) for all comparison and logical operators to match SQL database behavior. The changeset stated: + +> With 3-valued logic, `eq(anything, null)` evaluates to `null` (UNKNOWN) and is filtered out. Use `isNull()` instead. + +This was an intentional breaking change to align local query evaluation with SQL semantics. + +## The Problem + +When using cursor-based pagination (e.g., `useLiveInfiniteQuery`), if a cursor column has a `null` value, the next page query would generate something like `gt(col, null)`. This caused invalid SQL queries with missing parameters because `serialize(null)` returns an empty string. + +Example of the bug: +``` +subset__where="projectId" = $1 AND "name" > $2 +subset__params={"1":"uuid..."} // missing $2! +``` + +## The Solution + +All comparison operators (`eq`, `gt`, `gte`, `lt`, `lte`, `like`, `ilike`) now throw a clear error when used with `null` or `undefined` values: + +``` +Cannot use null/undefined value with 'eq' operator. +Comparisons with null always evaluate to UNKNOWN in SQL. +Use isNull() or isUndefined() to check for null values, +or filter out null values before building the query. +``` + +## Why Not Transform `eq(col, null)` to `IS NULL`? + +We considered automatically transforming `eq(col, null)` to `IS NULL` syntax, but decided against it for these reasons: + +1. **Consistency with 3-valued logic design**: PR #765 explicitly documented that `eq(col, null)` should return UNKNOWN and users should use `isNull()` instead. + +2. **Explicitness**: Requiring users to write `isNull(col)` makes the intent clear and avoids confusion about SQL null semantics. + +3. **Runtime null values**: If we transformed literal `null` to `IS NULL`, users might expect `eq(col, variable)` where `variable` is `null` at runtime to also work like `IS NULL`. But we can't distinguish these cases at the IR level, and silently returning no results (UNKNOWN) would be confusing. + +## Correct Usage + +### Checking for null values + +```typescript +// Correct - use isNull() +q.where(({ user }) => isNull(user.email)) + +// Correct - use isUndefined() +q.where(({ user }) => isUndefined(user.deletedAt)) + +// Correct - negate with not() +q.where(({ user }) => not(isNull(user.email))) +``` + +### Handling dynamic values that might be null + +```typescript +const filterValue = getUserInput() // might be null + +// Option 1: Filter out null values before building the query +if (filterValue !== null) { + q.where(({ user }) => eq(user.name, filterValue)) +} + +// Option 2: Handle null case explicitly +q.where(({ user }) => + filterValue === null + ? isNull(user.name) + : eq(user.name, filterValue) +) +``` + +### Cursor-based pagination + +The original bug was caused by cursor columns having null values. The fix is to ensure cursor columns are never null: + +```typescript +// Add a secondary orderBy on a non-nullable column (like primary key) +q.orderBy(({ user }) => [ + asc(user.name), // might be null + asc(user.id), // never null - ensures valid cursor +]) +``` + +## 3-Valued Logic Behavior + +With the current implementation, 3-valued logic applies consistently: + +| Expression | Result | +|------------|--------| +| `eq(col, null)` | **Error** - use `isNull(col)` | +| `eq(col, value)` where `col` is null at runtime | `null` (UNKNOWN) | +| `eq(col1, col2)` where either is null at runtime | `null` (UNKNOWN) | +| `isNull(col)` | `true` or `false` | + +The UNKNOWN result from 3-valued logic causes rows to be filtered out (not included in WHERE clause results), matching SQL behavior. + +## Summary + +- All comparison operators throw errors for null/undefined values +- Use `isNull()` or `isUndefined()` to check for null values +- 3-valued logic applies when column values are null at runtime +- Cursor-based pagination should use non-nullable columns or add a secondary sort key diff --git a/packages/electric-db-collection/src/sql-compiler.ts b/packages/electric-db-collection/src/sql-compiler.ts index c941c7fef..7e4289a04 100644 --- a/packages/electric-db-collection/src/sql-compiler.ts +++ b/packages/electric-db-collection/src/sql-compiler.ts @@ -124,6 +124,13 @@ function compileOrderByClause( return sql } +/** + * Check if a BasicExpression represents a null/undefined value + */ +function isNullValue(exp: IR.BasicExpression): boolean { + return exp.type === `val` && (exp.value === null || exp.value === undefined) +} + function compileFunction( exp: IR.Func, params: Array = [] @@ -132,6 +139,26 @@ function compileFunction( const opName = getOpName(name) + // Handle comparison operators with null/undefined values + // These would create invalid queries with missing params (e.g., "col = $1" with empty params) + // In SQL, all comparisons with NULL return UNKNOWN, so these are almost always mistakes + if (isComparisonOp(name)) { + const nullArgIndex = args.findIndex((arg: IR.BasicExpression) => + isNullValue(arg) + ) + + if (nullArgIndex !== -1) { + // All comparison operators (including eq) throw an error for null values + // Users should use isNull() or isUndefined() to check for null values + throw new Error( + `Cannot use null/undefined value with '${name}' operator. ` + + `Comparisons with null always evaluate to UNKNOWN in SQL. ` + + `Use isNull() or isUndefined() to check for null values, ` + + `or filter out null values before building the query.` + ) + } + } + const compiledArgs = args.map((arg: IR.BasicExpression) => compileBasicExpression(arg, params) ) @@ -198,6 +225,16 @@ function isBinaryOp(name: string): boolean { return binaryOps.includes(name) } +/** + * Check if operator is a comparison operator that takes two values + * These operators cannot accept null/undefined as values + * (null comparisons in SQL always evaluate to UNKNOWN) + */ +function isComparisonOp(name: string): boolean { + const comparisonOps = [`eq`, `gt`, `gte`, `lt`, `lte`, `like`, `ilike`] + return comparisonOps.includes(name) +} + function getOpName(name: string): string { const opNames = { eq: `=`, diff --git a/packages/electric-db-collection/tests/sql-compiler.test.ts b/packages/electric-db-collection/tests/sql-compiler.test.ts new file mode 100644 index 000000000..6d6dc9a2a --- /dev/null +++ b/packages/electric-db-collection/tests/sql-compiler.test.ts @@ -0,0 +1,312 @@ +import { describe, expect, it } from "vitest" +import { compileSQL } from "../src/sql-compiler" +import type { IR } from "@tanstack/db" + +// Helper to create a value expression +function val(value: T): IR.BasicExpression { + return { type: `val`, value } as IR.BasicExpression +} + +// Helper to create a reference expression +function ref(...path: Array): IR.BasicExpression { + return { type: `ref`, path } as IR.BasicExpression +} + +// Helper to create a function expression + +function func(name: string, args: Array): IR.BasicExpression { + return { type: `func`, name, args } as IR.BasicExpression +} + +describe(`sql-compiler`, () => { + describe(`compileSQL`, () => { + describe(`basic where clauses`, () => { + it(`should compile eq with string value`, () => { + const result = compileSQL({ + where: func(`eq`, [ref(`name`), val(`John`)]), + }) + expect(result.where).toBe(`"name" = $1`) + expect(result.params).toEqual({ "1": `John` }) + }) + + it(`should compile eq with number value`, () => { + const result = compileSQL({ + where: func(`eq`, [ref(`age`), val(25)]), + }) + expect(result.where).toBe(`"age" = $1`) + expect(result.params).toEqual({ "1": `25` }) + }) + + it(`should compile gt operator`, () => { + const result = compileSQL({ + where: func(`gt`, [ref(`age`), val(18)]), + }) + expect(result.where).toBe(`"age" > $1`) + expect(result.params).toEqual({ "1": `18` }) + }) + + it(`should compile lt operator`, () => { + const result = compileSQL({ + where: func(`lt`, [ref(`price`), val(100)]), + }) + expect(result.where).toBe(`"price" < $1`) + expect(result.params).toEqual({ "1": `100` }) + }) + + it(`should compile gte operator`, () => { + const result = compileSQL({ + where: func(`gte`, [ref(`quantity`), val(10)]), + }) + expect(result.where).toBe(`"quantity" >= $1`) + expect(result.params).toEqual({ "1": `10` }) + }) + + it(`should compile lte operator`, () => { + const result = compileSQL({ + where: func(`lte`, [ref(`rating`), val(5)]), + }) + expect(result.where).toBe(`"rating" <= $1`) + expect(result.params).toEqual({ "1": `5` }) + }) + }) + + describe(`compound where clauses`, () => { + it(`should compile AND with two conditions`, () => { + const result = compileSQL({ + where: func(`and`, [ + func(`eq`, [ref(`projectId`), val(`uuid-123`)]), + func(`gt`, [ref(`name`), val(`cursor-value`)]), + ]), + }) + // Note: 2-arg AND doesn't add parentheses around the operands + expect(result.where).toBe(`"projectId" = $1 AND "name" > $2`) + expect(result.params).toEqual({ "1": `uuid-123`, "2": `cursor-value` }) + }) + + it(`should compile AND with more than two conditions`, () => { + const result = compileSQL({ + where: func(`and`, [ + func(`eq`, [ref(`a`), val(`1`)]), + func(`eq`, [ref(`b`), val(`2`)]), + func(`eq`, [ref(`c`), val(`3`)]), + ]), + }) + // >2 args adds parentheses + expect(result.where).toBe(`("a" = $1) AND ("b" = $2) AND ("c" = $3)`) + expect(result.params).toEqual({ "1": `1`, "2": `2`, "3": `3` }) + }) + + it(`should compile OR with two conditions`, () => { + const result = compileSQL({ + where: func(`or`, [ + func(`eq`, [ref(`status`), val(`active`)]), + func(`eq`, [ref(`status`), val(`pending`)]), + ]), + }) + expect(result.where).toBe(`"status" = $1 OR "status" = $2`) + expect(result.params).toEqual({ "1": `active`, "2": `pending` }) + }) + }) + + describe(`null/undefined value handling`, () => { + it(`should throw error for eq(col, null)`, () => { + // Users should use isNull() instead of eq(col, null) + expect(() => + compileSQL({ + where: func(`eq`, [ref(`deletedAt`), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + + it(`should throw error for eq(col, undefined)`, () => { + expect(() => + compileSQL({ + where: func(`eq`, [ref(`deletedAt`), val(undefined)]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + + it(`should throw error for eq(null, col) (reversed order)`, () => { + expect(() => + compileSQL({ + where: func(`eq`, [val(null), ref(`name`)]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + + it(`should throw error for gt with null value`, () => { + expect(() => + compileSQL({ + where: func(`gt`, [ref(`age`), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'gt' operator`) + }) + + it(`should throw error for lt with undefined value`, () => { + expect(() => + compileSQL({ + where: func(`lt`, [ref(`age`), val(undefined)]), + }) + ).toThrow(`Cannot use null/undefined value with 'lt' operator`) + }) + + it(`should throw error for gte with null value`, () => { + expect(() => + compileSQL({ + where: func(`gte`, [ref(`price`), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'gte' operator`) + }) + + it(`should throw error for lte with null value`, () => { + expect(() => + compileSQL({ + where: func(`lte`, [ref(`rating`), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'lte' operator`) + }) + + it(`should throw error for like with null value`, () => { + expect(() => + compileSQL({ + where: func(`like`, [ref(`name`), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'like' operator`) + }) + + it(`should throw error for ilike with null value`, () => { + expect(() => + compileSQL({ + where: func(`ilike`, [ref(`name`), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'ilike' operator`) + }) + + it(`should throw error for eq(col, null) in AND clause`, () => { + expect(() => + compileSQL({ + where: func(`and`, [ + func(`eq`, [ref(`projectId`), val(`uuid-123`)]), + func(`eq`, [ref(`deletedAt`), val(null)]), + ]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + + it(`should throw error for eq(col, null) in mixed conditions`, () => { + expect(() => + compileSQL({ + where: func(`and`, [ + func(`eq`, [ref(`status`), val(`active`)]), + func(`eq`, [ref(`archivedAt`), val(null)]), + func(`gt`, [ref(`createdAt`), val(`2024-01-01`)]), + ]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + + it(`should not include params for null values in complex queries`, () => { + // This test simulates the bug scenario: a query with both valid params and null + // Before the fix, this would generate: + // where: "projectId" = $1 AND "name" > $2 + // params: { "1": "uuid" } // missing $2! + // After the fix, gt(name, null) throws an error + expect(() => + compileSQL({ + where: func(`and`, [ + func(`eq`, [ref(`projectId`), val(`uuid`)]), + func(`gt`, [ref(`name`), val(null)]), + ]), + }) + ).toThrow(`Cannot use null/undefined value with 'gt' operator`) + }) + + it(`should throw error for eq(null, null)`, () => { + // Both args are null - this is nonsensical and would cause missing params + expect(() => + compileSQL({ + where: func(`eq`, [val(null), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + + it(`should throw error for eq(null, literal)`, () => { + // Comparing null to a literal is nonsensical (always evaluates to UNKNOWN) + expect(() => + compileSQL({ + where: func(`eq`, [val(null), val(42)]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + + it(`should throw error for eq(col, null) - use isNull(col) instead`, () => { + // eq(col, null) should throw an error + // Users should use isNull(col) which works correctly + expect(() => + compileSQL({ + where: func(`eq`, [ref(`email`), val(null)]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + + // isNull(col) should work correctly + const isNullResult = compileSQL({ + where: func(`isNull`, [ref(`email`)]), + }) + expect(isNullResult.where).toBe(`"email" IS NULL`) + expect(isNullResult.params).toEqual({}) + }) + + it(`should throw error for eq(col, null) in OR clause`, () => { + expect(() => + compileSQL({ + where: func(`or`, [ + func(`eq`, [ref(`deletedAt`), val(null)]), + func(`eq`, [ref(`status`), val(`active`)]), + ]), + }) + ).toThrow(`Cannot use null/undefined value with 'eq' operator`) + }) + }) + + describe(`isNull/isUndefined operators`, () => { + it(`should compile isNull correctly`, () => { + const result = compileSQL({ + where: func(`isNull`, [ref(`deletedAt`)]), + }) + expect(result.where).toBe(`"deletedAt" IS NULL`) + expect(result.params).toEqual({}) + }) + + it(`should compile isUndefined correctly`, () => { + const result = compileSQL({ + where: func(`isUndefined`, [ref(`field`)]), + }) + expect(result.where).toBe(`"field" IS NULL`) + expect(result.params).toEqual({}) + }) + + it(`should compile NOT isNull correctly`, () => { + const result = compileSQL({ + where: func(`not`, [func(`isNull`, [ref(`name`)])]), + }) + expect(result.where).toBe(`"name" IS NOT NULL`) + expect(result.params).toEqual({}) + }) + }) + + describe(`empty where clause`, () => { + it(`should add true = true when no where clause`, () => { + const result = compileSQL({}) + expect(result.where).toBe(`true = true`) + expect(result.params).toEqual({}) + }) + }) + + describe(`limit`, () => { + it(`should include limit in result`, () => { + const result = compileSQL({ limit: 10 }) + expect(result.limit).toBe(10) + }) + }) + }) +})