diff --git a/.changeset/reject-fn-select-query-values.md b/.changeset/reject-fn-select-query-values.md new file mode 100644 index 0000000000..4cd30f0fae --- /dev/null +++ b/.changeset/reject-fn-select-query-values.md @@ -0,0 +1,5 @@ +--- +'@tanstack/db': patch +--- + +Reject child query builders and query-construction helpers returned from `fn.select()` with type and runtime errors instead of exposing internal query objects. diff --git a/docs/guides/live-queries.md b/docs/guides/live-queries.md index eb2bf08f04..42a6b96f6c 100644 --- a/docs/guides/live-queries.md +++ b/docs/guides/live-queries.md @@ -1330,6 +1330,8 @@ The singleton vs. array result type is inferred from whether the wrapped query e Like `toArray()`, `materialize()` is only valid as a top-level value in `.select()` — it cannot be nested inside expression helpers such as `coalesce()` or `eq()`. +Do not return child queries, `toArray()`, `materialize()`, or query expressions such as `eq()` and `caseWhen()` from `.fn.select()`. Functional select callbacks run after the compiler builds the query graph, so they cannot add query operations to it. + ### Aggregates You can use aggregate functions in child queries. Aggregates are computed per parent: diff --git a/packages/db/src/errors.ts b/packages/db/src/errors.ts index 0281b13afd..25d5b4db85 100644 --- a/packages/db/src/errors.ts +++ b/packages/db/src/errors.ts @@ -481,6 +481,16 @@ export class FnSelectWithGroupByError extends QueryCompilationError { } } +export class UnsupportedFnSelectResultError extends QueryCompilationError { + constructor(valueDescription: string) { + super( + `fn.select() cannot return ${valueDescription}. ` + + `Child query builders, query expressions, and helpers such as eq(), toArray(), materialize(), concat(toArray()), and caseWhen() are query-construction values. ` + + `Use them as direct fields in .select() instead.`, + ) + } +} + export class UnsupportedRootScalarSelectError extends QueryCompilationError { constructor() { super( diff --git a/packages/db/src/query/builder/index.ts b/packages/db/src/query/builder/index.ts index 0291c204cd..6beb23bc34 100644 --- a/packages/db/src/query/builder/index.ts +++ b/packages/db/src/query/builder/index.ts @@ -81,6 +81,57 @@ type CollectionResolver = ( options: CollectionOptionsIdentity, ) => CollectionImpl +type FnSelectQueryConstructionValue = + | QueryBuilder + | InitialQueryBuilder + | BasicExpression + | Aggregate + | ToArrayWrapper + | ConcatToArrayWrapper + | MaterializeWrapper + | CaseWhenWrapper + +type IsAnyType = 0 extends 1 & T ? true : false + +// Bound recursive inspection so deeply recursive result types do not exceed +// TypeScript's instantiation limit. The runtime check has no depth limit. +type ContainsFnSelectQueryConstructionValue< + T, + TDepth extends ReadonlyArray = [], +> = + IsAnyType extends true + ? false + : T extends FnSelectQueryConstructionValue + ? true + : TDepth[`length`] extends 8 + ? false + : T extends (...args: Array) => any + ? false + : T extends ReadonlyArray + ? ContainsFnSelectQueryConstructionValue< + TItem, + [...TDepth, unknown] + > + : T extends object + ? true extends { + [K in keyof T]-?: ContainsFnSelectQueryConstructionValue< + T[K], + [...TDepth, unknown] + > + }[keyof T] + ? true + : false + : false + +type InvalidFnSelectResult = { + readonly __tanstackDbFnSelectResultError__: `fn.select() cannot return child query builders, query expressions, or query helpers. Use them as direct fields in .select() instead.` +} + +type FnSelectQueryResult = + true extends ContainsFnSelectQueryConstructionValue + ? InvalidFnSelectResult + : QueryBuilder> + export class BaseQueryBuilder { private readonly query: Partial = {} @@ -889,10 +940,15 @@ export class BaseQueryBuilder { * age: row.users.age + 1, * })) * ``` + * + * Child query builders, query expressions, and helpers such as eq(), + * toArray(), and materialize() cannot be returned from fn.select(). Use + * them as fields in select() so the compiler can add them to the query + * graph. */ select( callback: (row: TContext[`schema`]) => TFuncSelectResult, - ): QueryBuilder> { + ): FnSelectQueryResult { return builder._clone({ ...builder.query, select: undefined, // remove the select clause if it exists diff --git a/packages/db/src/query/compiler/index.ts b/packages/db/src/query/compiler/index.ts index d075539213..6f2ff6a55e 100644 --- a/packages/db/src/query/compiler/index.ts +++ b/packages/db/src/query/compiler/index.ts @@ -16,9 +16,17 @@ import { FnSelectWithGroupByError, HavingRequiresGroupByError, LimitOffsetRequireOrderByError, + UnsupportedFnSelectResultError, UnsupportedFromTypeError, } from '../../errors.js' import { VIRTUAL_PROP_NAMES } from '../../virtual-props.js' +import { BaseQueryBuilder } from '../builder/index.js' +import { + CaseWhenWrapper, + ConcatToArrayWrapper, + MaterializeWrapper, + ToArrayWrapper, +} from '../builder/functions.js' import { ConditionalSelect, IncludesSubquery, @@ -69,6 +77,50 @@ export const INCLUDES_PUBLIC_KEY = Symbol(`includesPublicKey`) export const FN_SELECT_STATE = Symbol(`fnSelectState`) const SKIP_INCLUDE = Symbol(`skipInclude`) +function getUnsupportedFnSelectResultDescription( + value: unknown, + seen: Set = new Set(), +): string | undefined { + if (value instanceof BaseQueryBuilder) return `a child query builder` + if (value instanceof ToArrayWrapper) return `toArray()` + if (value instanceof ConcatToArrayWrapper) return `concat(toArray())` + if (value instanceof MaterializeWrapper) return `materialize()` + if (value instanceof CaseWhenWrapper) return `caseWhen()` + if (isExpressionLike(value)) { + return value && + typeof value === `object` && + `name` in value && + typeof value.name === `string` + ? `${value.name}()` + : `a query expression` + } + if (value === null || typeof value !== `object` || seen.has(value)) { + return undefined + } + + seen.add(value) + const keys = [ + ...Object.keys(value), + ...Object.getOwnPropertySymbols(value).filter((key) => + Object.prototype.propertyIsEnumerable.call(value, key), + ), + ] + for (const key of keys) { + const entry = (value as Record)[key] + const unsupported = getUnsupportedFnSelectResultDescription(entry, seen) + if (unsupported) return unsupported + } + return undefined +} + +export function validateFnSelectResult(value: unknown): void { + const unsupportedValueDescription = + getUnsupportedFnSelectResultDescription(value) + if (unsupportedValueDescription) { + throw new UnsupportedFnSelectResultError(unsupportedValueDescription) + } +} + type ConditionalSelectGuard = { condition: BasicExpression expected: boolean @@ -785,6 +837,7 @@ export function compileQuery( pipeline = pipeline.pipe( map(([key, namespacedRow]) => { const selectResults = query.fnSelect!(namespacedRow) + validateFnSelectResult(selectResults) let selected = selectResults if (selectResults && typeof selectResults === `object`) { selected = Array.isArray(selectResults) diff --git a/packages/db/src/query/live/materialized-pipeline.ts b/packages/db/src/query/live/materialized-pipeline.ts index 915113ef3e..c05de5639a 100644 --- a/packages/db/src/query/live/materialized-pipeline.ts +++ b/packages/db/src/query/live/materialized-pipeline.ts @@ -7,7 +7,11 @@ import { reduce, serializeValue, } from '@tanstack/db-ivm' -import { FN_SELECT_STATE, INCLUDES_ROUTING } from '../compiler/index.js' +import { + FN_SELECT_STATE, + INCLUDES_ROUTING, + validateFnSelectResult, +} from '../compiler/index.js' import { VIRTUAL_PROP_NAMES } from '../../virtual-props.js' import { deepEquals } from '../../utils.js' import type { @@ -492,6 +496,7 @@ function setMaterializedInclude( const sourceRow = setNestedValue(state.sourceRow, path, materialized) const selectedValue = state.fnSelect(sourceRow) + validateFnSelectResult(selectedValue) if (!selectedValue || typeof selectedValue !== `object`) { throw new Error(`fn.select must return an object when it projects includes`) } diff --git a/packages/db/tests/query/functional-variants.test-d.ts b/packages/db/tests/query/functional-variants.test-d.ts index 15130260ee..a388579f48 100644 --- a/packages/db/tests/query/functional-variants.test-d.ts +++ b/packages/db/tests/query/functional-variants.test-d.ts @@ -1,9 +1,13 @@ import { describe, expectTypeOf, test } from 'vitest' import { + Query, + caseWhen, count, createLiveQueryCollection, eq, gt, + materialize, + toArray, } from '../../src/query/index.js' import { createCollection } from '../../src/collection/index.js' import { mockSyncCollectionOptions } from '../utils.js' @@ -144,6 +148,60 @@ describe(`Functional Variants Types`, () => { >() }) + test(`fn.select rejects child queries and materialization helpers`, () => { + // @ts-expect-error query helpers are only supported in select() + createLiveQueryCollection((q) => + q.from({ user: usersCollection }).fn.select((row) => ({ + id: row.user.id, + nested: { + departments: toArray(q.from({ department: departmentsCollection })), + }, + })), + ) + + // @ts-expect-error materialize() is only supported in select() + createLiveQueryCollection((q) => + q.from({ user: usersCollection }).fn.select((row) => ({ + id: row.user.id, + departments: materialize(q.from({ department: departmentsCollection })), + })), + ) + + // @ts-expect-error child query builders are only supported in select() + createLiveQueryCollection((q) => + q.from({ user: usersCollection }).fn.select((row) => ({ + id: row.user.id, + departments: q.from({ department: departmentsCollection }), + })), + ) + + // @ts-expect-error query expressions are only supported in select() + createLiveQueryCollection((q) => + q.from({ user: usersCollection }).fn.select((row) => ({ + id: row.user.id, + active: eq(row.user.active, true), + })), + ) + + // @ts-expect-error caseWhen() is only supported in select() + createLiveQueryCollection((q) => + q.from({ user: usersCollection }).fn.select((row) => ({ + id: row.user.id, + label: caseWhen(eq(row.user.active, true), `active`, `inactive`), + })), + ) + }) + + test(`fn.select accepts unresolved generic result types`, () => { + const query = new Query().from({ user: usersCollection }) + + function selectValue(value: T) { + return query.fn.select(() => value) + } + + selectValue({ label: `active` }) + }) + test(`fn.where with filtered original type`, () => { const liveCollection = createLiveQueryCollection({ query: (q) => diff --git a/packages/db/tests/query/functional-variants.test.ts b/packages/db/tests/query/functional-variants.test.ts index 8456526b91..f803e1f04e 100644 --- a/packages/db/tests/query/functional-variants.test.ts +++ b/packages/db/tests/query/functional-variants.test.ts @@ -1,9 +1,12 @@ import { beforeEach, describe, expect, test } from 'vitest' import { + caseWhen, count, createLiveQueryCollection, eq, gt, + materialize, + toArray, } from '../../src/query/index.js' import { createCollection } from '../../src/collection/index.js' import { mockSyncCollectionOptions, stripVirtualProps } from '../utils.js' @@ -219,6 +222,135 @@ describe(`Functional Variants Query`, () => { yearsToRetirement: 37, }) }) + + test(`rejects query-construction values returned from fn.select`, () => { + const departmentsCollection = createDepartmentsCollection() + + expect(() => + createLiveQueryCollection({ + startSync: true, + query: (q) => { + const users = q.from({ user: usersCollection }) + const departments = q.from({ department: departmentsCollection }) + + return users.fn.select( + (row) => + ({ + id: row.user.id, + nested: { + departments: toArray( + q.from({ + department: departments.fn.where( + ({ department }) => + row.user.department_id === department.id, + ), + }), + ), + }, + }) as any, + ) + }, + }), + ).toThrow( + `fn.select() cannot return toArray(). Child query builders, query expressions, and helpers`, + ) + + expect(() => + createLiveQueryCollection({ + startSync: true, + query: (q) => + q.from({ user: usersCollection }).fn.select( + (row) => + ({ + id: row.user.id, + departments: materialize( + q.from({ department: departmentsCollection }), + ), + }) as any, + ), + }), + ).toThrow(`fn.select() cannot return materialize()`) + + expect(() => + createLiveQueryCollection({ + startSync: true, + query: (q) => + q.from({ user: usersCollection }).fn.select( + (row) => + ({ + id: row.user.id, + departments: q.from({ department: departmentsCollection }), + }) as any, + ), + }), + ).toThrow(`fn.select() cannot return a child query builder`) + + class Wrapper { + constructor(readonly departments: unknown) {} + } + + expect(() => + createLiveQueryCollection({ + startSync: true, + query: (q) => + q + .from({ user: usersCollection }) + .fn.select( + () => + new Wrapper( + q.from({ department: departmentsCollection }), + ) as any, + ), + }), + ).toThrow(`fn.select() cannot return a child query builder`) + + const departments = Symbol(`departments`) + expect(() => + createLiveQueryCollection({ + startSync: true, + query: (q) => + q.from({ user: usersCollection }).fn.select( + (row) => + ({ + id: row.user.id, + [departments]: q.from({ department: departmentsCollection }), + }) as any, + ), + }), + ).toThrow(`fn.select() cannot return a child query builder`) + + expect(() => + createLiveQueryCollection({ + startSync: true, + query: (q) => + q.from({ user: usersCollection }).fn.select( + (row) => + ({ + id: row.user.id, + active: eq(row.user.active, true), + }) as any, + ), + }), + ).toThrow(`fn.select() cannot return eq()`) + + expect(() => + createLiveQueryCollection({ + startSync: true, + query: (q) => + q.from({ user: usersCollection }).fn.select( + (row) => + ({ + id: row.user.id, + label: caseWhen( + eq(row.user.active, true), + `active`, + `inactive`, + ), + }) as any, + ), + }), + ).toThrow(`fn.select() cannot return caseWhen()`) + }) }) describe(`fn.where`, () => { diff --git a/packages/db/tests/query/group-by.test.ts b/packages/db/tests/query/group-by.test.ts index 34c225cc57..39851ed823 100644 --- a/packages/db/tests/query/group-by.test.ts +++ b/packages/db/tests/query/group-by.test.ts @@ -2224,11 +2224,14 @@ function createGroupByTests(autoIndex: `off` | `eager`): void { q .from({ orders: ordersCollection }) .groupBy(({ orders }) => orders.customer_id) - .fn.select((row) => ({ - customerId: row.orders.customer_id, - totalAmount: sum(row.orders.amount), - orderCount: count(row.orders.id), - })), + .fn.select( + (row) => + ({ + customerId: row.orders.customer_id, + totalAmount: sum(row.orders.amount), + orderCount: count(row.orders.id), + }) as any, + ), }), ).toThrow(`fn.select() cannot be used with groupBy()`) }) diff --git a/packages/db/tests/query/includes-collection-oracle.property.test.ts b/packages/db/tests/query/includes-collection-oracle.property.test.ts index 6f001b586d..73a5f4123b 100644 --- a/packages/db/tests/query/includes-collection-oracle.property.test.ts +++ b/packages/db/tests/query/includes-collection-oracle.property.test.ts @@ -857,6 +857,77 @@ describe(`Collection-valued includes oracle`, () => { }, ) + fcTest( + `fn.select rejects query values returned during include rematerialization`, + async () => { + const messages = createControlledCollection(`fn-select-reject-messages`, [ + { id: 1, group: 1 }, + ]) + const tools = createControlledCollection(`fn-select-reject-tools`, [ + { id: 2, group: 2 }, + ]) + const children = createControlledCollection(`fn-select-reject-children`, [ + { id: 10, parentGroup: 1, value: 1 }, + ]) + const live = createLiveQueryCollection((q) => { + const messageRows = q + .from({ message: messages.collection }) + .select(({ message }) => ({ + kind: `message` as const, + id: message.id, + children: toArray( + q + .from({ messageChild: children.collection }) + .where(({ messageChild }) => + eq(messageChild.parentGroup, message.group), + ), + ), + })) + const toolRows = q + .from({ tool: tools.collection }) + .select(({ tool }) => ({ + kind: `tool` as const, + id: tool.id, + })) + + return q.unionAll(messageRows, toolRows).fn.select((row) => { + const includedChildren = + row.kind === `message` + ? (row.children as typeof row.children | null) + : null + + return { + kind: row.kind, + id: row.id, + leakedQuery: + includedChildren?.[0]?.value === 2 + ? q.from({ child: children.collection }) + : null, + } as any + }) + }) + + try { + await live.preload() + + expect(() => + children.write(`update`, { + id: 10, + parentGroup: 1, + value: 2, + }), + ).toThrow(`fn.select() cannot return a child query builder`) + } finally { + await Promise.all([ + live.cleanup(), + messages.collection.cleanup(), + tools.collection.cleanup(), + children.collection.cleanup(), + ]) + } + }, + ) + fcTest( `rejects collapsed contributors that disagree by value, order, or outgoing route`, async () => {