diff --git a/packages/db/src/errors.ts b/packages/db/src/errors.ts index 50cd442ac..592307502 100644 --- a/packages/db/src/errors.ts +++ b/packages/db/src/errors.ts @@ -398,6 +398,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 e7d2be0c4..033beac51 100644 --- a/packages/db/src/query/builder/index.ts +++ b/packages/db/src/query/builder/index.ts @@ -16,7 +16,11 @@ import { QueryMustHaveFromClauseError, SubQueryMustHaveFromClauseError, } from '../../errors.js' -import { createRefProxy, toExpression } from './ref-proxy.js' +import { + checkCallbackForJsOperators, + createRefProxy, + toExpression, +} from './ref-proxy.js' import type { NamespacedRow, SingleResult } from '../../types.js' import type { Aggregate, @@ -357,6 +361,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) @@ -398,6 +405,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() const refProxy = createRefProxy(aliases) as RefsForContext const expression = callback(refProxy) @@ -447,6 +457,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 2f060b5f2..318de0943 100644 --- a/packages/db/src/query/builder/ref-proxy.ts +++ b/packages/db/src/query/builder/ref-proxy.ts @@ -1,7 +1,30 @@ 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 createToPrimitiveHandler( + path: Array, +): (hint: string) => never { + return (hint: string) => { + const pathStr = path.length > 0 ? path.join(`.`) : `` + throw new JavaScriptOperatorInQueryError( + hint === `number` + ? `arithmetic` + : hint === `string` + ? `string concatenation` + : `comparison`, + `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 +67,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 +124,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 +171,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) @@ -213,3 +248,86 @@ export function isRefProxy(value: any): value is RefProxy { export function val(value: T): BasicExpression { return new Value(value) } + +/** + * Patterns that indicate JavaScript operators being used in query callbacks. + * These operators cannot be translated to query operations and will silently + * produce incorrect results. + */ +const JS_OPERATOR_PATTERNS: Array<{ + pattern: RegExp + operator: string + description: string +}> = [ + { + // Match || that's not inside a string or comment + // This regex looks for || not preceded by quotes that would indicate a string + pattern: /\|\|/, + operator: `||`, + description: `logical OR`, + }, + { + // Match && that's not inside a string or comment + pattern: /&&/, + operator: `&&`, + description: `logical AND`, + }, + { + // Match ?? nullish coalescing + pattern: /\?\?/, + operator: `??`, + description: `nullish coalescing`, + }, +] + +/** + * Removes string literals and comments from source code to avoid false positives + * when checking for JavaScript operators. + */ +function stripStringsAndComments(source: string): string { + // Remove template literals (backtick strings) + let result = source.replace(/`(?:[^`\\]|\\.)*`/g, `""`) + // Remove double-quoted strings + result = result.replace(/"(?:[^"\\]|\\.)*"/g, `""`) + // Remove single-quoted strings + result = result.replace(/'(?:[^'\\]|\\.)*'/g, `""`) + // Remove single-line comments + result = result.replace(/\/\/[^\n]*/g, ``) + // Remove multi-line comments + result = result.replace(/\/\*[\s\S]*?\*\//g, ``) + return result +} + +/** + * Checks a callback function's source code for JavaScript operators that + * cannot be translated to query operations. + * + * @param callback - The callback function to check + * @throws JavaScriptOperatorInQueryError if a problematic operator is found + * + * @example + * // This will throw an error: + * checkCallbackForJsOperators(({users}) => users.data || []) + * + * // This is fine: + * checkCallbackForJsOperators(({users}) => users.data) + */ +export function checkCallbackForJsOperators< + T extends (...args: Array) => any, +>(callback: T): void { + const source = callback.toString() + + // Strip strings and comments to avoid false positives + const cleanedSource = stripStringsAndComments(source) + + for (const { pattern, operator, description } of JS_OPERATOR_PATTERNS) { + if (pattern.test(cleanedSource)) { + throw new JavaScriptOperatorInQueryError( + operator, + `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.`, + ) + } + } +} diff --git a/packages/db/tests/query/builder/ref-proxy.test.ts b/packages/db/tests/query/builder/ref-proxy.test.ts index ec41351da..050472125 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 { + 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,85 @@ describe(`ref-proxy`, () => { expect((val({ a: 1 }) as Value).value).toEqual({ a: 1 }) }) }) + + describe(`checkCallbackForJsOperators`, () => { + it(`throws error for || operator`, () => { + const callback = ({ users }: any) => ({ data: users.data || [] }) + expect(() => checkCallbackForJsOperators(callback)).toThrow( + JavaScriptOperatorInQueryError, + ) + expect(() => checkCallbackForJsOperators(callback)).toThrow(`||`) + }) + + it(`throws error for && operator`, () => { + const callback = ({ users }: any) => users.active && users.name + expect(() => checkCallbackForJsOperators(callback)).toThrow( + JavaScriptOperatorInQueryError, + ) + expect(() => checkCallbackForJsOperators(callback)).toThrow(`&&`) + }) + + it(`throws error for ?? operator`, () => { + const callback = ({ users }: any) => ({ name: users.name ?? `default` }) + expect(() => checkCallbackForJsOperators(callback)).toThrow( + JavaScriptOperatorInQueryError, + ) + expect(() => checkCallbackForJsOperators(callback)).toThrow(`??`) + }) + + it(`does not throw for valid query callbacks`, () => { + // Simple property access + expect(() => + checkCallbackForJsOperators(({ users }: any) => users.name), + ).not.toThrow() + + // Object with property access + expect(() => + checkCallbackForJsOperators(({ users }: any) => ({ + id: users.id, + name: users.name, + })), + ).not.toThrow() + + // Optional chaining is allowed + expect(() => + checkCallbackForJsOperators(({ users }: any) => users.profile?.bio), + ).not.toThrow() + }) + + it(`does not throw for operators in string literals`, () => { + // || in a string literal should not trigger error + expect(() => + checkCallbackForJsOperators(() => ({ message: `a || b is valid` })), + ).not.toThrow() + + // && in a string literal should not trigger error + expect(() => + checkCallbackForJsOperators(() => ({ message: `a && b is valid` })), + ).not.toThrow() + }) + }) + + 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 8c10c3eb8..bc2c8bbeb 100644 --- a/packages/db/tests/query/live-query-collection.test.ts +++ b/packages/db/tests/query/live-query-collection.test.ts @@ -1911,9 +1911,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), })), })