From 8c0e3931665e8ccb5cd113d67d79c669b6f60760 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Nov 2025 03:11:33 +0000 Subject: [PATCH 1/3] Improve error message when invalid source type is passed to .from() Fixes #873 The issue was that users were getting a confusing error message when passing a string (or other invalid type) to the .from() method instead of an object with a collection. For example: ```javascript // Incorrect usage q.from("conversations") // String instead of object // Correct usage q.from({ conversations: conversationsCollection }) ``` When a string was passed, Object.keys("conversations") would return an array of character indices ['0', '1', ...], which has length > 1, triggering the "Only one source is allowed in the from clause" error. This was misleading because the real issue was passing a string instead of an object. Changes: - Added validation to check if the source is a plain object before checking its key count - Improved error message to explicitly state the expected format with an example: .from({ alias: collection }) - Added comprehensive tests for various invalid input types (string, null, array, undefined) The new error message is: "Invalid source for from clause: Expected an object with a single key-value pair like { alias: collection }. For example: .from({ todos: todosCollection }). Got: string \"conversations\"" --- packages/db/src/query/builder/index.ts | 17 ++++++++ packages/db/tests/query/builder/from.test.ts | 45 ++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/packages/db/src/query/builder/index.ts b/packages/db/src/query/builder/index.ts index bd8b95178..cd7f1cfd2 100644 --- a/packages/db/src/query/builder/index.ts +++ b/packages/db/src/query/builder/index.ts @@ -12,6 +12,7 @@ import { InvalidSourceError, JoinConditionMustBeEqualityError, OnlyOneSourceAllowedError, + QueryBuilderError, QueryMustHaveFromClauseError, SubQueryMustHaveFromClauseError, } from "../../errors.js" @@ -60,6 +61,22 @@ export class BaseQueryBuilder { source: TSource, context: string ): [string, CollectionRef | QueryRef] { + // Check if source is a plain object (not null, array, string, etc.) + // We need this check at runtime even though TypeScript knows the type, + // because callers can bypass type checks with `as any` + if ( + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + source === null || + typeof source !== `object` || + Array.isArray(source) || + typeof source === `string` + ) { + throw new QueryBuilderError( + `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + + `For example: .from({ todos: todosCollection }). Got: ${typeof source === `string` ? `string "${source}"` : typeof source}` + ) + } + if (Object.keys(source).length !== 1) { throw new OnlyOneSourceAllowedError(context) } diff --git a/packages/db/tests/query/builder/from.test.ts b/packages/db/tests/query/builder/from.test.ts index 62946b094..f282b3449 100644 --- a/packages/db/tests/query/builder/from.test.ts +++ b/packages/db/tests/query/builder/from.test.ts @@ -4,6 +4,7 @@ import { Query, getQueryIR } from "../../../src/query/builder/index.js" import { eq } from "../../../src/query/builder/functions.js" import { OnlyOneSourceAllowedError, + QueryBuilderError, QueryMustHaveFromClauseError, } from "../../../src/errors" @@ -108,4 +109,48 @@ describe(`QueryBuilder.from`, () => { } as any) }).toThrow(OnlyOneSourceAllowedError) }) + + it(`throws helpful error when passing a string instead of an object`, () => { + const builder = new Query() + + expect(() => { + builder.from(`employees` as any) + }).toThrow(QueryBuilderError) + + expect(() => { + builder.from(`employees` as any) + }).toThrow( + /Invalid source for from clause: Expected an object with a single key-value pair/ + ) + }) + + it(`throws helpful error when passing null`, () => { + const builder = new Query() + + expect(() => { + builder.from(null as any) + }).toThrow( + /Invalid source for from clause: Expected an object with a single key-value pair/ + ) + }) + + it(`throws helpful error when passing an array`, () => { + const builder = new Query() + + expect(() => { + builder.from([employeesCollection] as any) + }).toThrow( + /Invalid source for from clause: Expected an object with a single key-value pair/ + ) + }) + + it(`throws helpful error when passing undefined`, () => { + const builder = new Query() + + expect(() => { + builder.from(undefined as any) + }).toThrow( + /Invalid source for from clause: Expected an object with a single key-value pair/ + ) + }) }) From 01f8671003a814704b05254c3d0db2d8a47e585b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Nov 2025 03:22:51 +0000 Subject: [PATCH 2/3] Refactor error validation to use positive checks Simplified the validation logic in _createRefForSource to use positive checks instead of negative checks: - Check if it's a valid object (handles null/undefined via try-catch) - Check if it's an array (arrays pass Object.keys but aren't valid) - Check if it has exactly one key - Check if keys look like numeric indices (indicates string was passed) This approach is cleaner and handles all edge cases including when callers bypass TypeScript's type checks with 'as any'. Also added a changeset documenting the improvement. --- .../improve-from-clause-error-messages.md | 5 ++ packages/db/src/query/builder/index.ts | 46 +++++++++++++------ 2 files changed, 38 insertions(+), 13 deletions(-) create mode 100644 .changeset/improve-from-clause-error-messages.md diff --git a/.changeset/improve-from-clause-error-messages.md b/.changeset/improve-from-clause-error-messages.md new file mode 100644 index 000000000..ce7b5a2b1 --- /dev/null +++ b/.changeset/improve-from-clause-error-messages.md @@ -0,0 +1,5 @@ +--- +"@tanstack/db": patch +--- + +Improved error messages when invalid source types are passed to `.from()` or `.join()` methods. When users mistakenly pass a string, null, array, or other invalid type instead of an object with a collection, they now receive a clear, actionable error message with an example of the correct usage (e.g., `.from({ todos: todosCollection })`). diff --git a/packages/db/src/query/builder/index.ts b/packages/db/src/query/builder/index.ts index cd7f1cfd2..60bc040dd 100644 --- a/packages/db/src/query/builder/index.ts +++ b/packages/db/src/query/builder/index.ts @@ -61,29 +61,49 @@ export class BaseQueryBuilder { source: TSource, context: string ): [string, CollectionRef | QueryRef] { - // Check if source is a plain object (not null, array, string, etc.) - // We need this check at runtime even though TypeScript knows the type, - // because callers can bypass type checks with `as any` - if ( - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - source === null || - typeof source !== `object` || - Array.isArray(source) || - typeof source === `string` - ) { + // Validate source is a plain object (not null, array, string, etc.) + // We use try-catch to handle null/undefined gracefully + let keys: Array + try { + keys = Object.keys(source) + } catch { throw new QueryBuilderError( `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + - `For example: .from({ todos: todosCollection }). Got: ${typeof source === `string` ? `string "${source}"` : typeof source}` + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + `For example: .from({ todos: todosCollection }). Got: ${source === null ? `null` : `undefined`}` ) } - if (Object.keys(source).length !== 1) { + // Check if it's an array (arrays pass Object.keys but aren't valid sources) + if (Array.isArray(source)) { + throw new QueryBuilderError( + `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + + `For example: .from({ todos: todosCollection }). Got: array` + ) + } + + // Validate exactly one key + if (keys.length !== 1) { + if (keys.length === 0) { + throw new QueryBuilderError( + `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + + `For example: .from({ todos: todosCollection }). Got: empty object` + ) + } + // Check if it looks like a string was passed (has numeric keys) + if (keys.every((k) => !isNaN(Number(k)))) { + throw new QueryBuilderError( + `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + + `For example: .from({ todos: todosCollection }). Got: string` + ) + } throw new OnlyOneSourceAllowedError(context) } - const alias = Object.keys(source)[0]! + const alias = keys[0]! const sourceValue = source[alias] + // Validate the value is a Collection or QueryBuilder let ref: CollectionRef | QueryRef if (sourceValue instanceof CollectionImpl) { From 28444e115785a663dfc837989d1f30c98f5afce4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Nov 2025 14:49:33 +0000 Subject: [PATCH 3/3] Move error messages to dedicated error class Addresses code review feedback by creating InvalidSourceTypeError class in errors.ts and using it consistently across all validation cases instead of duplicating error message strings. Changes: - Added InvalidSourceTypeError class to errors.ts that takes context and type parameters - Updated _createRefForSource to use InvalidSourceTypeError in all four validation cases (null/undefined, array, empty object, string) - Updated tests to expect InvalidSourceTypeError instead of QueryBuilderError - This eliminates string duplication and makes error messages consistent across .from() and .join() methods --- packages/db/src/errors.ts | 9 +++++++ packages/db/src/query/builder/index.ts | 25 ++++++-------------- packages/db/tests/query/builder/from.test.ts | 16 +++++++++++-- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/packages/db/src/errors.ts b/packages/db/src/errors.ts index 55f9205cc..8abc79fe5 100644 --- a/packages/db/src/errors.ts +++ b/packages/db/src/errors.ts @@ -360,6 +360,15 @@ export class InvalidSourceError extends QueryBuilderError { } } +export class InvalidSourceTypeError extends QueryBuilderError { + constructor(context: string, type: string) { + super( + `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + + `For example: .from({ todos: todosCollection }). Got: ${type}` + ) + } +} + export class JoinConditionMustBeEqualityError extends QueryBuilderError { constructor() { super(`Join condition must be an equality expression`) diff --git a/packages/db/src/query/builder/index.ts b/packages/db/src/query/builder/index.ts index 60bc040dd..e25cca1f4 100644 --- a/packages/db/src/query/builder/index.ts +++ b/packages/db/src/query/builder/index.ts @@ -10,9 +10,9 @@ import { } from "../ir.js" import { InvalidSourceError, + InvalidSourceTypeError, JoinConditionMustBeEqualityError, OnlyOneSourceAllowedError, - QueryBuilderError, QueryMustHaveFromClauseError, SubQueryMustHaveFromClauseError, } from "../../errors.js" @@ -67,35 +67,24 @@ export class BaseQueryBuilder { try { keys = Object.keys(source) } catch { - throw new QueryBuilderError( - `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - `For example: .from({ todos: todosCollection }). Got: ${source === null ? `null` : `undefined`}` - ) + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + const type = source === null ? `null` : `undefined` + throw new InvalidSourceTypeError(context, type) } // Check if it's an array (arrays pass Object.keys but aren't valid sources) if (Array.isArray(source)) { - throw new QueryBuilderError( - `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + - `For example: .from({ todos: todosCollection }). Got: array` - ) + throw new InvalidSourceTypeError(context, `array`) } // Validate exactly one key if (keys.length !== 1) { if (keys.length === 0) { - throw new QueryBuilderError( - `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + - `For example: .from({ todos: todosCollection }). Got: empty object` - ) + throw new InvalidSourceTypeError(context, `empty object`) } // Check if it looks like a string was passed (has numeric keys) if (keys.every((k) => !isNaN(Number(k)))) { - throw new QueryBuilderError( - `Invalid source for ${context}: Expected an object with a single key-value pair like { alias: collection }. ` + - `For example: .from({ todos: todosCollection }). Got: string` - ) + throw new InvalidSourceTypeError(context, `string`) } throw new OnlyOneSourceAllowedError(context) } diff --git a/packages/db/tests/query/builder/from.test.ts b/packages/db/tests/query/builder/from.test.ts index f282b3449..6e4ec2f64 100644 --- a/packages/db/tests/query/builder/from.test.ts +++ b/packages/db/tests/query/builder/from.test.ts @@ -3,8 +3,8 @@ import { CollectionImpl } from "../../../src/collection/index.js" import { Query, getQueryIR } from "../../../src/query/builder/index.js" import { eq } from "../../../src/query/builder/functions.js" import { + InvalidSourceTypeError, OnlyOneSourceAllowedError, - QueryBuilderError, QueryMustHaveFromClauseError, } from "../../../src/errors" @@ -115,7 +115,7 @@ describe(`QueryBuilder.from`, () => { expect(() => { builder.from(`employees` as any) - }).toThrow(QueryBuilderError) + }).toThrow(InvalidSourceTypeError) expect(() => { builder.from(`employees` as any) @@ -127,6 +127,10 @@ describe(`QueryBuilder.from`, () => { it(`throws helpful error when passing null`, () => { const builder = new Query() + expect(() => { + builder.from(null as any) + }).toThrow(InvalidSourceTypeError) + expect(() => { builder.from(null as any) }).toThrow( @@ -137,6 +141,10 @@ describe(`QueryBuilder.from`, () => { it(`throws helpful error when passing an array`, () => { const builder = new Query() + expect(() => { + builder.from([employeesCollection] as any) + }).toThrow(InvalidSourceTypeError) + expect(() => { builder.from([employeesCollection] as any) }).toThrow( @@ -147,6 +155,10 @@ describe(`QueryBuilder.from`, () => { it(`throws helpful error when passing undefined`, () => { const builder = new Query() + expect(() => { + builder.from(undefined as any) + }).toThrow(InvalidSourceTypeError) + expect(() => { builder.from(undefined as any) }).toThrow(