diff --git a/packages/lib/src/compiler.ts b/packages/lib/src/compiler.ts index 3662715..d363f38 100644 --- a/packages/lib/src/compiler.ts +++ b/packages/lib/src/compiler.ts @@ -1401,9 +1401,29 @@ export class SqlCompilerImpl implements SqlCompiler { } // Handle single values with value property else if ('value' in value) { - return value.value; + return this.resolveObjectIdSentinel(value.value); } } + if (typeof value === 'string') { + return this.resolveObjectIdSentinel(value); + } + return value; + } + + /** + * If the string is a sentinel injected by preprocessObjectIdCasts(), return a + * typed marker that the executor will convert to a real ObjectId instance. + * Otherwise return the string unchanged. + */ + private resolveObjectIdSentinel(value: string): any { + if ( + typeof value === 'string' && + value.startsWith('__QL_OBJECTID_') && + value.endsWith('__') + ) { + const hex = value.slice('__QL_OBJECTID_'.length, -2); + return { __qlObjectId: hex }; + } return value; } diff --git a/packages/lib/src/executor/index.ts b/packages/lib/src/executor/index.ts index 142dd55..59c3af5 100644 --- a/packages/lib/src/executor/index.ts +++ b/packages/lib/src/executor/index.ts @@ -5,7 +5,7 @@ import { CursorResult, CursorOptions, } from '../interfaces'; -import { Document, MongoClient, ObjectId } from 'mongodb'; +import { Db, Document, MongoClient, ObjectId } from 'mongodb'; /** * MongoDB command executor implementation for Node.js @@ -75,9 +75,14 @@ export class MongoExecutor implements CommandExecutor { } // Create the cursor with all options at once + const findFilter = await this.resolveFilterObjectIds( + database, + command.collection, + command.filter || {} + ); const findCursor = database .collection(command.collection) - .find(this.convertObjectIds(command.filter || {}), findOptions); + .find(findFilter, findOptions); // Always return array for the regular execute result = await findCursor.toArray(); @@ -89,20 +94,29 @@ export class MongoExecutor implements CommandExecutor { .insertMany(command.documents.map((doc) => this.convertObjectIds(doc))); break; - case 'UPDATE': + case 'UPDATE': { + const updateFilter = await this.resolveFilterObjectIds( + database, + command.collection, + command.filter || {} + ); result = await database .collection(command.collection) - .updateMany( - this.convertObjectIds(command.filter || {}), - this.convertObjectIds(command.update) - ); + .updateMany(updateFilter, this.convertObjectIds(command.update)); break; + } - case 'DELETE': + case 'DELETE': { + const deleteFilter = await this.resolveFilterObjectIds( + database, + command.collection, + command.filter || {} + ); result = await database .collection(command.collection) - .deleteMany(this.convertObjectIds(command.filter || {})); + .deleteMany(deleteFilter); break; + } case 'AGGREGATE': // Handle aggregation commands @@ -176,9 +190,14 @@ export class MongoExecutor implements CommandExecutor { } // Create the cursor with all options at once + const cursorFilter = await this.resolveFilterObjectIds( + database, + command.collection, + command.filter || {} + ); const findCursor = database .collection(command.collection) - .find(this.convertObjectIds(command.filter || {}), findOptions); + .find(cursorFilter, findOptions); // Return the cursor directly result = findCursor; @@ -214,9 +233,107 @@ export class MongoExecutor implements CommandExecutor { } /** - * Convert string ObjectIds to MongoDB ObjectId instances - * @param obj Object to convert - * @returns Object with converted ObjectIds + * Sample one document from the collection to discover which fields contain ObjectIds, + * then apply conversions to the filter accordingly. + */ + private async resolveFilterObjectIds( + db: Db, + collectionName: string, + filter: Record + ): Promise> { + const objectIdFields = new Set(); + const sample = await db.collection(collectionName).findOne({}); + if (sample) { + for (const [key, value] of Object.entries(sample)) { + if (value instanceof ObjectId) { + objectIdFields.add(key); + } + } + } else { + // Empty collection: assume _id is ObjectId (MongoDB default) + objectIdFields.add('_id'); + } + // Schema-based conversion first, then resolve any explicit cast sentinels + const schemaConverted = this.applyFilterConversions(filter, objectIdFields); + return this.resolveSentinels(schemaConverted); + } + + /** + * Recursively resolve { __qlObjectId: hex } sentinels emitted by the compiler + * for explicit CAST('...' AS OBJECTID) / '...'::OBJECTID expressions. + */ + private resolveSentinels(value: any): any { + if (!value || typeof value !== 'object') return value; + if ('__qlObjectId' in value) { + try { + return new ObjectId(value.__qlObjectId); + } catch { + return value.__qlObjectId; + } + } + if (Array.isArray(value)) return value.map((v) => this.resolveSentinels(v)); + // Skip non-plain objects (ObjectId, Date, RegExp, Buffer, etc.) — they are already + // properly typed BSON values and must not be destructured into plain objects. + const proto = Object.getPrototypeOf(value); + if (proto !== Object.prototype && proto !== null) return value; + const result: Record = {}; + for (const [k, v] of Object.entries(value)) result[k] = this.resolveSentinels(v); + return result; + } + + /** + * Recursively apply ObjectId conversions to a filter using a set of known ObjectId fields. + * Handles logical operators ($and, $or, $nor) and comparison operators ($eq, $in, etc.). + */ + private applyFilterConversions(filter: any, objectIdFields: Set): any { + if (!filter || typeof filter !== 'object') return filter; + + if (Array.isArray(filter)) { + return filter.map((item) => this.applyFilterConversions(item, objectIdFields)); + } + + const result: Record = {}; + for (const [key, value] of Object.entries(filter)) { + if (key.startsWith('$')) { + // Logical operator ($and, $or, $nor) — recurse without a field context + result[key] = this.applyFilterConversions(value, objectIdFields); + } else if (objectIdFields.has(key)) { + // Known ObjectId field — convert any hex strings in the value + result[key] = this.convertToObjectId(value); + } else { + result[key] = value; + } + } + return result; + } + + /** + * Convert a filter value (or nested operator expression) to ObjectId where applicable. + */ + private convertToObjectId(value: any): any { + if (typeof value === 'string' && /^[0-9a-fA-F]{24}$/.test(value)) { + try { + return new ObjectId(value); + } catch { + return value; + } + } + if (Array.isArray(value)) { + return value.map((v) => this.convertToObjectId(v)); + } + if (value && typeof value === 'object') { + // Operator expression like { $eq: '...', $in: [...], $ne: '...' } + const result: Record = {}; + for (const [op, v] of Object.entries(value)) { + result[op] = this.convertToObjectId(v); + } + return result; + } + return value; + } + + /** + * Recursively convert _id fields in documents (used for INSERT payloads). */ private convertObjectIds(obj: any): any { if (!obj) return obj; @@ -226,47 +343,28 @@ export class MongoExecutor implements CommandExecutor { } if (typeof obj === 'object') { + // Resolve explicit cast sentinels from compiler + if ('__qlObjectId' in obj) { + try { + return new ObjectId(obj.__qlObjectId); + } catch { + return obj.__qlObjectId; + } + } const result: Record = {}; - for (const [key, value] of Object.entries(obj)) { - // Special handling for _id field and fields ending with Id - if ( - (key === '_id' || key.endsWith('Id') || key.endsWith('Ids')) && - typeof value === 'string' - ) { + if (key === '_id' && typeof value === 'string' && /^[0-9a-fA-F]{24}$/.test(value)) { try { - // Check if it's a valid ObjectId string - if (/^[0-9a-fA-F]{24}$/.test(value)) { - result[key] = new ObjectId(value); - continue; - } - } catch (error) { - // If it's not a valid ObjectId, keep it as a string - console.warn(`Could not convert ${key} value to ObjectId: ${value}`); + result[key] = new ObjectId(value); + } catch { + result[key] = value; } - } else if (Array.isArray(value) && (key.endsWith('Ids') || key === 'productIds')) { - // For arrays of IDs - result[key] = value.map((item: any) => { - if (typeof item === 'string' && /^[0-9a-fA-F]{24}$/.test(item)) { - try { - return new ObjectId(item); - } catch (error) { - return item; - } - } - return this.convertObjectIds(item); - }); - continue; } else if (typeof value === 'object' && value !== null) { - // Recursively convert nested objects result[key] = this.convertObjectIds(value); - continue; + } else { + result[key] = value; } - - // Copy other values as is - result[key] = value; } - return result; } diff --git a/packages/lib/src/parser.ts b/packages/lib/src/parser.ts index 37a560d..f863b6b 100644 --- a/packages/lib/src/parser.ts +++ b/packages/lib/src/parser.ts @@ -67,8 +67,11 @@ export class SqlParserImpl implements SqlParser { */ parse(sql: string): SqlStatement { try { + // Transform explicit ObjectId cast syntax before any other preprocessing + const preprocessedObjectIdSql = this.preprocessObjectIdCasts(sql); + // First, handle nested dot notation in field access - const preprocessedNestedSql = this.preprocessNestedFields(sql); + const preprocessedNestedSql = this.preprocessNestedFields(preprocessedObjectIdSql); // Then transform array index notation to a form the parser can handle const preprocessedSql = this.preprocessArrayIndexes(preprocessedNestedSql); @@ -97,7 +100,7 @@ export class SqlParserImpl implements SqlParser { if (errorMessage.includes('[')) { // Make a more aggressive transformation of the SQL for bracket syntax - const fallbackSql = this.aggressivePreprocessing(sql); + const fallbackSql = this.aggressivePreprocessing(this.preprocessObjectIdCasts(sql)); log('Fallback SQL for array syntax:', fallbackSql); try { const ast = this.parser.astify(fallbackSql, { database: 'PostgreSQL' }); @@ -120,6 +123,28 @@ export class SqlParserImpl implements SqlParser { } } + /** + * Preprocess explicit ObjectId cast syntax into a sentinel string literal + * that survives SQL parsing and is recognised later in convertValue(). + * + * Supported forms (case-insensitive): + * CAST('507f...' AS OBJECTID) → '__QL_OBJECTID_507f...__' + * '507f...'::OBJECTID → '__QL_OBJECTID_507f...__' + */ + private preprocessObjectIdCasts(sql: string): string { + // CAST('value' AS OBJECTID) + let result = sql.replace( + /CAST\s*\(\s*'([^']*)'\s+AS\s+OBJECTID\s*\)/gi, + (_match, value) => `'__QL_OBJECTID_${value}__'` + ); + // 'value'::OBJECTID + result = result.replace( + /'([^']*)'\s*::\s*OBJECTID/gi, + (_match, value) => `'__QL_OBJECTID_${value}__'` + ); + return result; + } + /** * Preprocess nested field access in SQL before parsing * diff --git a/packages/lib/tests/integration/edge-cases.integration.test.ts b/packages/lib/tests/integration/edge-cases.integration.test.ts index a817ee1..5f898c2 100644 --- a/packages/lib/tests/integration/edge-cases.integration.test.ts +++ b/packages/lib/tests/integration/edge-cases.integration.test.ts @@ -119,19 +119,169 @@ describe('Edge Cases Integration Tests', () => { _id: objectId, name: 'ObjectId Test' }); - + // Act const queryLeaf = testSetup.getQueryLeaf(); // Use the string representation of ObjectId in SQL const sql = `SELECT name FROM edge_test WHERE _id = '${objectId.toString()}'`; - + const results = ensureArray(await queryLeaf.execute(sql)); - + // Assert expect(results).toHaveLength(1); expect(results[0].name).toBe('ObjectId Test'); }); + // Regression test for: https://github.com/beekeeper-studio/queryleaf/issues/12 + // Non-primary ObjectId fields (snake_case like transaction_id) were not being + // converted from string to ObjectId, causing queries to return no results. + test('should handle ObjectId conversions on non-primary snake_case id fields (issue #12)', async () => { + // Arrange + const db = testSetup.getDb(); + const transactionId = new ObjectId(); + await db.collection('edge_test').insertOne({ + name: 'sale record', + transaction_id: transactionId, + }); + + // Act + const queryLeaf = testSetup.getQueryLeaf(); + const sql = `SELECT name FROM edge_test WHERE transaction_id = '${transactionId.toString()}'`; + + const results = ensureArray(await queryLeaf.execute(sql)); + + // Assert - should find the document by its non-primary ObjectId field + expect(results).toHaveLength(1); + expect(results[0].name).toBe('sale record'); + }); + + test('should NOT convert *_id fields that store integers to ObjectId', async () => { + const db = testSetup.getDb(); + await db.collection('edge_test').insertOne({ + name: 'integer id record', + external_id: 42, + }); + + const queryLeaf = testSetup.getQueryLeaf(); + const results = ensureArray( + await queryLeaf.execute("SELECT name FROM edge_test WHERE external_id = 42") + ); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('integer id record'); + }); + + test('should NOT convert *_id fields that store strings to ObjectId', async () => { + const db = testSetup.getDb(); + // A 24-char hex string stored as a plain string — NOT an ObjectId + const hexLikeString = 'aabbccddeeff001122334455'; + await db.collection('edge_test').insertOne({ + name: 'string id record', + external_id: hexLikeString, + }); + + const queryLeaf = testSetup.getQueryLeaf(); + const results = ensureArray( + await queryLeaf.execute(`SELECT name FROM edge_test WHERE external_id = '${hexLikeString}'`) + ); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('string id record'); + }); + + test('should NOT convert _id to ObjectId when stored as an integer', async () => { + const db = testSetup.getDb(); + await db.collection('edge_test').insertOne({ + _id: 99 as any, + name: 'integer _id record', + }); + + const queryLeaf = testSetup.getQueryLeaf(); + const results = ensureArray( + await queryLeaf.execute("SELECT name FROM edge_test WHERE _id = 99") + ); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('integer _id record'); + }); + + test('should NOT convert _id to ObjectId when stored as a plain string', async () => { + const db = testSetup.getDb(); + // A 24-char hex string stored as a plain string _id — NOT an ObjectId + const stringId = 'aabbccddeeff001122334455'; + await db.collection('edge_test').insertOne({ + _id: stringId as any, + name: 'string _id record', + }); + + const queryLeaf = testSetup.getQueryLeaf(); + const results = ensureArray( + await queryLeaf.execute(`SELECT name FROM edge_test WHERE _id = '${stringId}'`) + ); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('string _id record'); + }); + + test('should support explicit CAST(value AS OBJECTID) syntax', async () => { + // Arrange + const db = testSetup.getDb(); + const refId = new ObjectId(); + await db.collection('edge_test').insertOne({ + name: 'cast test', + arbitrary_ref: refId, + }); + + // Act - field name has no Id hint; user explicitly casts the value + const queryLeaf = testSetup.getQueryLeaf(); + const sql = `SELECT name FROM edge_test WHERE arbitrary_ref = CAST('${refId.toString()}' AS OBJECTID)`; + + const results = ensureArray(await queryLeaf.execute(sql)); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('cast test'); + }); + + test('should support explicit PostgreSQL-style ::OBJECTID cast syntax', async () => { + // Arrange + const db = testSetup.getDb(); + const refId = new ObjectId(); + await db.collection('edge_test').insertOne({ + name: 'pg cast test', + arbitrary_ref: refId, + }); + + // Act + const queryLeaf = testSetup.getQueryLeaf(); + const sql = `SELECT name FROM edge_test WHERE arbitrary_ref = '${refId.toString()}'::OBJECTID`; + + const results = ensureArray(await queryLeaf.execute(sql)); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('pg cast test'); + }); + + // The real fix for issue #12: field name is arbitrary, conversion must be based on value shape + test('should handle ObjectId conversions on arbitrarily-named ObjectId fields', async () => { + // Arrange - field name has no Id/id suffix hint whatsoever + const db = testSetup.getDb(); + const refId = new ObjectId(); + await db.collection('edge_test').insertOne({ + name: 'arbitrary field test', + source_ref: refId, + }); + + // Act + const queryLeaf = testSetup.getQueryLeaf(); + const sql = `SELECT name FROM edge_test WHERE source_ref = '${refId.toString()}'`; + + const results = ensureArray(await queryLeaf.execute(sql)); + + // Assert + expect(results).toHaveLength(1); + expect(results[0].name).toBe('arbitrary field test'); + }); + test('should handle extremely large result sets', async () => { // Arrange const db = testSetup.getDb();