diff --git a/.changeset/prevent-js-operators-in-queries.md b/.changeset/prevent-js-operators-in-queries.md new file mode 100644 index 000000000..4bacc7259 --- /dev/null +++ b/.changeset/prevent-js-operators-in-queries.md @@ -0,0 +1,14 @@ +--- +'@tanstack/db': patch +--- + +Add development warnings for JavaScript operators in query callbacks + +Warns developers when they mistakenly use JavaScript operators (`||`, `&&`, `??`, `?:`) in query callbacks. These operators are evaluated at query construction time rather than execution time, causing silent unexpected behavior. + +Changes: + +- Add `Symbol.toPrimitive` trap to RefProxy to catch primitive coercion (throws error) +- Add `checkCallbackForJsOperators()` to detect operators in callback source code (warns in dev only) +- Integrate checks into `select()`, `where()`, and `having()` methods +- Detection is disabled in production mode for zero runtime overhead diff --git a/packages/db-collection-e2e/src/suites/predicates.suite.ts b/packages/db-collection-e2e/src/suites/predicates.suite.ts index 0ea3a9947..1aaf7ce91 100644 --- a/packages/db-collection-e2e/src/suites/predicates.suite.ts +++ b/packages/db-collection-e2e/src/suites/predicates.suite.ts @@ -472,7 +472,7 @@ export function createPredicatesTestSuite( .where(({ post }) => or( like(lower(post.title), `%${searchLower}%`), - like(lower(post.content ?? ``), `%${searchLower}%`), + like(lower(post.content!), `%${searchLower}%`), ), ), ) diff --git a/packages/db/src/errors.ts b/packages/db/src/errors.ts index dc1c7b900..4eccda9bf 100644 --- a/packages/db/src/errors.ts +++ b/packages/db/src/errors.ts @@ -427,6 +427,30 @@ export class QueryCompilationError extends TanStackDBError { } } +export class JavaScriptOperatorInQueryError extends QueryBuilderError { + constructor(operator: string, hint?: string) { + const defaultHint = + operator === `||` || operator === `??` + ? `Use coalesce() instead: coalesce(value, defaultValue)` + : operator === `&&` + ? `Use and() for logical conditions` + : operator === `?:` + ? `Use cond() for conditional expressions: cond(condition, trueValue, falseValue)` + : `Use the appropriate query function instead` + + super( + `JavaScript operator "${operator}" cannot be used in queries.\n\n` + + `Query callbacks should only use field references and query functions, not JavaScript logic.\n` + + `${hint || defaultHint}\n\n` + + `Example of incorrect usage:\n` + + ` .select(({users}) => ({ data: users.data || [] }))\n\n` + + `Correct usage:\n` + + ` .select(({users}) => ({ data: coalesce(users.data, []) }))`, + ) + this.name = `JavaScriptOperatorInQueryError` + } +} + export class DistinctRequiresSelectError extends QueryCompilationError { constructor() { super(`DISTINCT requires a SELECT clause.`) diff --git a/packages/db/src/query/builder/index.ts b/packages/db/src/query/builder/index.ts index 6ef4d1ddc..18a88d1a4 100644 --- a/packages/db/src/query/builder/index.ts +++ b/packages/db/src/query/builder/index.ts @@ -18,6 +18,7 @@ import { SubQueryMustHaveFromClauseError, } from '../../errors.js' import { + checkCallbackForJsOperators, createRefProxy, createRefProxyWithSelected, toExpression, @@ -364,6 +365,9 @@ export class BaseQueryBuilder { * ``` */ where(callback: WhereCallback): QueryBuilder { + // Check for JavaScript operators that cannot be translated to query operations + checkCallbackForJsOperators(callback) + const aliases = this._getCurrentAliases() const refProxy = createRefProxy(aliases) as RefsForContext const expression = callback(refProxy) @@ -412,6 +416,9 @@ export class BaseQueryBuilder { * ``` */ having(callback: WhereCallback): QueryBuilder { + // Check for JavaScript operators that cannot be translated to query operations + checkCallbackForJsOperators(callback) + const aliases = this._getCurrentAliases() // Add $selected namespace if SELECT clause exists const refProxy = ( @@ -473,6 +480,9 @@ export class BaseQueryBuilder { select( callback: (refs: RefsForContext) => TSelectObject, ): QueryBuilder>> { + // Check for JavaScript operators that cannot be translated to query operations + checkCallbackForJsOperators(callback) + const aliases = this._getCurrentAliases() const refProxy = createRefProxy(aliases) as RefsForContext const selectObject = callback(refProxy) diff --git a/packages/db/src/query/builder/ref-proxy.ts b/packages/db/src/query/builder/ref-proxy.ts index b3f79ef51..96a2c6519 100644 --- a/packages/db/src/query/builder/ref-proxy.ts +++ b/packages/db/src/query/builder/ref-proxy.ts @@ -1,7 +1,37 @@ import { PropRef, Value } from '../ir.js' +import { JavaScriptOperatorInQueryError } from '../../errors.js' import type { BasicExpression } from '../ir.js' import type { RefLeaf } from './types.js' +/** + * Creates a handler for Symbol.toPrimitive that throws an error when + * JavaScript tries to coerce a RefProxy to a primitive value. + * This catches misuse like string concatenation, arithmetic, etc. + */ +function getOperatorTypeFromHint(hint: string): string { + switch (hint) { + case `number`: + return `arithmetic` + case `string`: + return `string concatenation` + default: + return `comparison` + } +} + +function createToPrimitiveHandler( + path: Array, +): (hint: string) => never { + return (hint: string) => { + const pathStr = path.length > 0 ? path.join(`.`) : `` + throw new JavaScriptOperatorInQueryError( + getOperatorTypeFromHint(hint), + `Attempted to use "${pathStr}" in a JavaScript ${hint} context.\n` + + `Query references can only be used with query functions, not JavaScript operators.`, + ) + } +} + export interface RefProxy { /** @internal */ readonly __refProxy: true @@ -44,6 +74,10 @@ export function createSingleRowRefProxy< if (prop === `__refProxy`) return true if (prop === `__path`) return path if (prop === `__type`) return undefined // Type is only for TypeScript inference + // Intercept Symbol.toPrimitive to catch JS coercion attempts + if (prop === Symbol.toPrimitive) { + return createToPrimitiveHandler(path) + } if (typeof prop === `symbol`) return Reflect.get(target, prop, receiver) const newPath = [...path, String(prop)] @@ -97,6 +131,10 @@ export function createRefProxy>( if (prop === `__refProxy`) return true if (prop === `__path`) return path if (prop === `__type`) return undefined // Type is only for TypeScript inference + // Intercept Symbol.toPrimitive to catch JS coercion attempts + if (prop === Symbol.toPrimitive) { + return createToPrimitiveHandler(path) + } if (typeof prop === `symbol`) return Reflect.get(target, prop, receiver) const newPath = [...path, String(prop)] @@ -140,6 +178,10 @@ export function createRefProxy>( if (prop === `__refProxy`) return true if (prop === `__path`) return [] if (prop === `__type`) return undefined // Type is only for TypeScript inference + // Intercept Symbol.toPrimitive to catch JS coercion attempts + if (prop === Symbol.toPrimitive) { + return createToPrimitiveHandler([]) + } if (typeof prop === `symbol`) return Reflect.get(target, prop, receiver) const propStr = String(prop) @@ -204,6 +246,10 @@ export function createRefProxyWithSelected>( if (prop === `__refProxy`) return true if (prop === `__path`) return [`$selected`, ...path] if (prop === `__type`) return undefined + // Intercept Symbol.toPrimitive to catch JS coercion attempts + if (prop === Symbol.toPrimitive) { + return createToPrimitiveHandler([`$selected`, ...path]) + } if (typeof prop === `symbol`) return Reflect.get(target, prop, receiver) const newPath = [...path, String(prop)] @@ -303,3 +349,83 @@ export function isRefProxy(value: any): value is RefProxy { export function val(value: T): BasicExpression { return new Value(value) } + +/** + * Checks a callback function's source code for JavaScript operators that + * cannot be translated to query operations. + * + * Only runs in development mode (NODE_ENV !== 'production') and logs a warning + * instead of throwing, since regex-based detection can have false positives + * (e.g., operators inside regex literals). + * + * All detection logic is inside the dev check so bundlers can eliminate it + * entirely from production builds. + * + * @param callback - The callback function to check + * + * @example + * // This will log a warning in dev: + * checkCallbackForJsOperators(({users}) => users.data || []) + * + * // This is fine: + * checkCallbackForJsOperators(({users}) => users.data) + */ +export function checkCallbackForJsOperators< + T extends (...args: Array) => any, +>(callback: T): void { + if (process.env.NODE_ENV !== `production`) { + // Patterns that indicate JavaScript operators being used in query callbacks + const JS_OPERATOR_PATTERNS = [ + { pattern: /\|\|/, operator: `||`, description: `logical OR` }, + { pattern: /&&/, operator: `&&`, description: `logical AND` }, + { pattern: /\?\?/, operator: `??`, description: `nullish coalescing` }, + { + // Matches ? followed by : with something in between, + // but not ?. (optional chaining) or ?? (nullish coalescing) + pattern: /\?[^.?][^:]*:/, + operator: `?:`, + description: `ternary`, + }, + ] + + const getHintForOperator = (operator: string): string => { + switch (operator) { + case `||`: + case `??`: + return `Use coalesce() instead: coalesce(value, defaultValue)` + case `&&`: + return `Use and() for logical conditions` + case `?:`: + return `Use cond() for conditional expressions: cond(condition, trueValue, falseValue)` + default: + return `Use the appropriate query function instead` + } + } + + // Strip string literals and comments to avoid false positives + const cleanedSource = callback + .toString() + .replace(/`(?:[^`\\]|\\.)*`/g, `""`) // template literals + .replace(/"(?:[^"\\]|\\.)*"/g, `""`) // double-quoted strings + .replace(/'(?:[^'\\]|\\.)*'/g, `""`) // single-quoted strings + .replace(/\/\/[^\n]*/g, ``) // single-line comments + .replace(/\/\*[\s\S]*?\*\//g, ``) // multi-line comments + + for (const { pattern, operator, description } of JS_OPERATOR_PATTERNS) { + if (pattern.test(cleanedSource)) { + console.warn( + `[TanStack DB] JavaScript operator "${operator}" detected in query callback.\n\n` + + `Found JavaScript ${description} operator (${operator}) in query callback.\n` + + `This operator is evaluated at query construction time, not at query execution time,\n` + + `which means it will not behave as expected.\n\n` + + `${getHintForOperator(operator)}\n\n` + + `Example of incorrect usage:\n` + + ` .select(({users}) => ({ data: users.data || [] }))\n\n` + + `Correct usage:\n` + + ` .select(({users}) => ({ data: coalesce(users.data, []) }))`, + ) + return // Only warn once per callback + } + } + } +} diff --git a/packages/db/tests/query/builder/ref-proxy.test.ts b/packages/db/tests/query/builder/ref-proxy.test.ts index ec41351da..f916d9c38 100644 --- a/packages/db/tests/query/builder/ref-proxy.test.ts +++ b/packages/db/tests/query/builder/ref-proxy.test.ts @@ -1,11 +1,13 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { + checkCallbackForJsOperators, createRefProxy, isRefProxy, toExpression, val, } from '../../../src/query/builder/ref-proxy.js' import { PropRef, Value } from '../../../src/query/ir.js' +import { JavaScriptOperatorInQueryError } from '../../../src/errors.js' describe(`ref-proxy`, () => { describe(`createRefProxy`, () => { @@ -214,4 +216,125 @@ describe(`ref-proxy`, () => { expect((val({ a: 1 }) as Value).value).toEqual({ a: 1 }) }) }) + + describe(`checkCallbackForJsOperators`, () => { + let warnSpy: ReturnType + const originalEnv = process.env.NODE_ENV + + beforeEach(() => { + warnSpy = vi.spyOn(console, `warn`).mockImplementation(() => {}) + // Ensure we're in dev mode for tests + process.env.NODE_ENV = `development` + }) + + afterEach(() => { + warnSpy.mockRestore() + process.env.NODE_ENV = originalEnv + }) + + it(`warns for || operator`, () => { + const callback = ({ users }: any) => ({ data: users.data || [] }) + checkCallbackForJsOperators(callback) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy.mock.calls[0]![0]).toContain(`||`) + }) + + it(`warns for && operator`, () => { + const callback = ({ users }: any) => users.active && users.name + checkCallbackForJsOperators(callback) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy.mock.calls[0]![0]).toContain(`&&`) + }) + + it(`warns for ?? operator`, () => { + const callback = ({ users }: any) => ({ name: users.name ?? `default` }) + checkCallbackForJsOperators(callback) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy.mock.calls[0]![0]).toContain(`??`) + }) + + it(`warns for ternary operator`, () => { + const callback = ({ users }: any) => ({ + status: users.active ? `active` : `inactive`, + }) + checkCallbackForJsOperators(callback) + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(warnSpy.mock.calls[0]![0]).toContain(`?:`) + }) + + it(`does not warn for valid query callbacks`, () => { + // Simple property access + checkCallbackForJsOperators(({ users }: any) => users.name) + expect(warnSpy).not.toHaveBeenCalled() + + // Object with property access + checkCallbackForJsOperators(({ users }: any) => ({ + id: users.id, + name: users.name, + })) + expect(warnSpy).not.toHaveBeenCalled() + + // Optional chaining is allowed + checkCallbackForJsOperators(({ users }: any) => users.profile?.bio) + expect(warnSpy).not.toHaveBeenCalled() + }) + + it(`does not warn for operators in string literals`, () => { + // || in a string literal should not trigger warning + checkCallbackForJsOperators(() => ({ message: `a || b is valid` })) + expect(warnSpy).not.toHaveBeenCalled() + + // && in a string literal should not trigger warning + checkCallbackForJsOperators(() => ({ message: `a && b is valid` })) + expect(warnSpy).not.toHaveBeenCalled() + + // ?: in a string literal should not trigger warning + checkCallbackForJsOperators(() => ({ message: `a ? b : c is valid` })) + expect(warnSpy).not.toHaveBeenCalled() + }) + + it(`does not warn for optional chaining`, () => { + // Optional chaining should not be confused with ternary + checkCallbackForJsOperators(({ users }: any) => users?.name) + expect(warnSpy).not.toHaveBeenCalled() + }) + + it(`warns for operators in regex literals (known limitation)`, () => { + // This is a known limitation - regex literals containing operators + // will trigger false positives. Document the behavior. + const callbackWithRegexOr = () => ({ pattern: /a||b/ }) + checkCallbackForJsOperators(callbackWithRegexOr) + expect(warnSpy).toHaveBeenCalledTimes(1) + }) + + it(`does not warn in production mode`, () => { + process.env.NODE_ENV = `production` + const callback = ({ users }: any) => ({ data: users.data || [] }) + checkCallbackForJsOperators(callback) + expect(warnSpy).not.toHaveBeenCalled() + }) + }) + + describe(`Symbol.toPrimitive trap`, () => { + it(`throws error when proxy is coerced to string`, () => { + const proxy = createRefProxy<{ users: { id: number } }>([`users`]) + expect(() => String(proxy.users.id)).toThrow( + JavaScriptOperatorInQueryError, + ) + }) + + it(`throws error when proxy is used in arithmetic`, () => { + const proxy = createRefProxy<{ users: { id: number } }>([`users`]) + expect(() => Number(proxy.users.id)).toThrow( + JavaScriptOperatorInQueryError, + ) + }) + + it(`throws error when proxy is concatenated with string`, () => { + const proxy = createRefProxy<{ users: { name: string } }>([`users`]) + expect(() => `Hello ${proxy.users.name}`).toThrow( + JavaScriptOperatorInQueryError, + ) + }) + }) }) diff --git a/packages/db/tests/query/live-query-collection.test.ts b/packages/db/tests/query/live-query-collection.test.ts index 58ed050ea..ca695e280 100644 --- a/packages/db/tests/query/live-query-collection.test.ts +++ b/packages/db/tests/query/live-query-collection.test.ts @@ -2126,9 +2126,9 @@ describe(`createLiveQueryCollection`, () => { .join({ users }, ({ comments: c, users: u }) => eq(c.userId, u.id)) .select(({ comments: c, users: u }) => ({ id: c.id, - userId: u?.id ?? c.userId, + userId: u!.id, text: c.text, - userName: u?.name, + userName: u!.name, })), getKey: (item) => item.userId, startSync: true, diff --git a/packages/db/tests/query/scheduler.test.ts b/packages/db/tests/query/scheduler.test.ts index adb85c979..d53798486 100644 --- a/packages/db/tests/query/scheduler.test.ts +++ b/packages/db/tests/query/scheduler.test.ts @@ -1,6 +1,11 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { createCollection } from '../../src/collection/index.js' -import { createLiveQueryCollection, eq, isNull } from '../../src/query/index.js' +import { + coalesce, + createLiveQueryCollection, + eq, + isNull, +} from '../../src/query/index.js' import { createTransaction } from '../../src/transactions.js' import { createOptimisticAction } from '../../src/optimistic-action.js' import { transactionScopedScheduler } from '../../src/scheduler.js' @@ -721,7 +726,7 @@ describe(`live query scheduler`, () => { .select(({ a, b }) => ({ id: a.id, aValue: a.value, - bValue: b?.value ?? null, + bValue: coalesce(b!.value, null), })), })