Support non-ObjectId _id types in ClustersClient (fixes #217)

l10n/bundle.l10n.json

@@ -765,7 +765,6 @@
   "Invalid connection string format. It should start with \"mongodb://\" or \"mongodb+srv://\"": "Invalid connection string format. It should start with \"mongodb://\" or \"mongodb+srv://\"",
   "Invalid Connection String: {error}": "Invalid Connection String: {error}",
   "Invalid connection type selected.": "Invalid connection type selected.",
-  "Invalid document ID: {0}": "Invalid document ID: {0}",
   "Invalid filter syntax: {0}. Please use valid JSON or a DocumentDB API expression, for example: { name: \"value\" }": "Invalid filter syntax: {0}. Please use valid JSON or a DocumentDB API expression, for example: { name: \"value\" }",
   "Invalid folder type.": "Invalid folder type.",
   "Invalid node type.": "Invalid node type.",
@@ -1388,6 +1387,7 @@
   "TypeScript-powered completions are unavailable on this read-only extension install. Click to retry.": "TypeScript-powered completions are unavailable on this read-only extension install. Click to retry.",
   "Unable to connect to the local database instance. Make sure it is started correctly. See {link} for tips.": "Unable to connect to the local database instance. Make sure it is started correctly. See {link} for tips.",
   "Unable to connect to the local instance. Make sure it is started correctly. See {link} for tips.": "Unable to connect to the local instance. Make sure it is started correctly. See {link} for tips.",
+  "Unable to parse the document _id. See the output channel for details.": "Unable to parse the document _id. See the output channel for details.",
   "Unable to retrieve credentials for cluster \"{cluster}\".": "Unable to retrieve credentials for cluster \"{cluster}\".",
   "Unable to retrieve credentials for the selected cluster.": "Unable to retrieve credentials for the selected cluster.",
   "Understanding Your Query Execution Plan": "Understanding Your Query Execution Plan",

src/documentdb/ClusterSession.ts

@@ -7,14 +7,15 @@ import { type FieldEntry } from '@documentdb-js/schema-analyzer';
 import { ParseMode, parse as parseShellBSON } from '@mongodb-js/shell-bson-parser';
 import * as l10n from '@vscode/l10n';
 import { EJSON } from 'bson';
-import { ObjectId, type Document, type Filter, type WithId } from 'mongodb';
+import { type Document, type Filter, type WithId } from 'mongodb';
 import { ext } from '../extensionVariables';
 import { getDataAtPath } from '../utils/slickgrid/mongo/toSlickGridTable';
 import { toSlickGridTree, type TreeData } from '../utils/slickgrid/mongo/toSlickGridTree';
 import { type QueryInsightsStage2Response } from '../webviews/documentdb/collectionView/types/queryInsights';
 import { ClustersClient, type FindQueryParams } from './ClustersClient';
 import { SchemaStore } from './SchemaStore';
 import { fixupDocumentDbExplain } from './utils/fixupDocumentDbExplain';
+import { parseDocumentId } from './utils/parseDocumentId';
 import { toFilterQueryObj } from './utils/toFilterQuery';
 
 export type TableDataEntry = {
@@ -335,31 +336,11 @@ export class ClusterSession {
 
         if (acknowledged) {
             this._currentRawDocuments = this._currentRawDocuments.filter((doc) => {
-                // Convert documentIds to BSON types and compare them with doc._id
                 return !documentIds.some((id) => {
-                    let parsedId;
-                    try {
-                        // eslint-disable-next-line
-                        parsedId = EJSON.parse(id);
-                    } catch {
-                        if (ObjectId.isValid(id)) {
-                            parsedId = new ObjectId(id);
-                        } else {
-                            parsedId = id;
-                        }
-                    }
-
-                    /**
-                     * deep equality for _id is tricky as we'd have to consider embedded objects,
-                     * arrays, etc. For now, we'll just stringify the _id and compare the strings.
-                     * The reasoning here is that this operation is used during interactive work
-                     * and were not expecting to delete a large number of documents at once.
-                     * Hence, the performance impact of this approach is negligible, and it's more
-                     * about simplicity here.
-                     */
+                    const parsedId = parseDocumentId(id);
 
                     const docIdStr = EJSON.stringify(doc._id, { relaxed: false }, 0);
-                    const parsedIdStr = EJSON.stringify(parsedId, { relaxed: false }, 0);
+                    const parsedIdStr = EJSON.stringify(parsedId as Document, { relaxed: false }, 0);
 
                     return docIdStr === parsedIdStr;
                 });

src/documentdb/ClustersClient.ts

@@ -17,7 +17,6 @@ import {
 } from '@microsoft/vscode-azext-utils';
 import { ParseMode, parse as parseShellBSON } from '@mongodb-js/shell-bson-parser';
 import * as l10n from '@vscode/l10n';
-import { EJSON } from 'bson';
 import { randomUUID } from 'crypto';
 import {
     MongoBulkWriteError,
@@ -60,6 +59,7 @@ import { SchemaStore } from './SchemaStore';
 import { getHostsFromConnectionString, hasAzureDomain } from './utils/connectionStringHelpers';
 import { fixupDocumentDbExplain } from './utils/fixupDocumentDbExplain';
 import { getClusterMetadata, type ClusterMetadata } from './utils/getClusterMetadata';
+import { parseDocumentId } from './utils/parseDocumentId';
 import { toFilterQueryObj } from './utils/toFilterQuery';
 
 export interface DatabaseItemModel {
@@ -974,49 +974,24 @@ export class ClustersClient {
     // TODO: revisit, maybe we can work on BSON here for the documentIds, and the conversion from string etc.,
     // will remain in the ClusterSession class
     async deleteDocuments(databaseName: string, collectionName: string, documentIds: string[]): Promise<boolean> {
-        // Convert input data to BSON types
-        const parsedDocumentIds = documentIds.map((id) => {
-            let parsedId;
-            try {
-                // eslint-disable-next-line
-                parsedId = EJSON.parse(id);
-            } catch {
-                if (ObjectId.isValid(id)) {
-                    parsedId = new ObjectId(id);
-                } else {
-                    throw new Error(l10n.t('Invalid document ID: {0}', id));
-                }
-            }
-            // eslint-disable-next-line @typescript-eslint/no-unsafe-return
-            return parsedId;
-        });
+        const parsedDocumentIds = documentIds.map((id) => parseDocumentId(id));
 
         // Connect and execute
         const collection = this._mongoClient.db(databaseName).collection(collectionName);
-        const deleteResult: DeleteResult = await collection.deleteMany({ _id: { $in: parsedDocumentIds } });
+        const deleteResult: DeleteResult = await collection.deleteMany({
+            _id: { $in: parsedDocumentIds },
+        } as Filter<Document>);
 
         return deleteResult.acknowledged;
     }
 
     async pointRead(databaseName: string, collectionName: string, documentId: string) {
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        let parsedDocumentId: any;
-        try {
-            // eslint-disable-next-line
-            parsedDocumentId = EJSON.parse(documentId);
-        } catch (error) {
-            if (ObjectId.isValid(documentId)) {
-                parsedDocumentId = new ObjectId(documentId);
-            } else {
-                throw error;
-            }
-        }
+        const parsedDocumentId = parseDocumentId(documentId);
 
         // connect and execute
         const collection = this._mongoClient.db(databaseName).collection(collectionName);
 
-        // eslint-disable-next-line
-        const documentContent = await collection.findOne({ _id: parsedDocumentId });
+        const documentContent = await collection.findOne({ _id: parsedDocumentId } as Filter<Document>);
 
         return documentContent;
     }
@@ -1033,17 +1008,10 @@ export class ClustersClient {
         let parsedId: any;
 
         if (documentId === '') {
-            // TODO: do not rely in empty string, use null or undefined
+            // TODO: do not rely on empty string, use null or undefined
             parsedId = new ObjectId();
         } else {
-            try {
-                // eslint-disable-next-line
-                parsedId = EJSON.parse(documentId);
-            } catch {
-                if (ObjectId.isValid(documentId)) {
-                    parsedId = new ObjectId(documentId);
-                }
-            }
+            parsedId = parseDocumentId(documentId);
         }
 
         // connect and execute

src/documentdb/utils/parseDocumentId.test.ts

@@ -0,0 +1,125 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { EJSON, ObjectId } from 'bson';
+
+// Mock vscode l10n so the localized throw message is deterministic.
+jest.mock('vscode', () => ({
+    l10n: {
+        t: (message: string, ...args: unknown[]) => {
+            let result = message;
+            args.forEach((arg, index) => {
+                result = result.replace(`{${index}}`, String(arg));
+            });
+            return result;
+        },
+    },
+}));
+
+// Capture output-channel diagnostics emitted on failure.
+const outputChannelError = jest.fn();
+jest.mock('../../extensionVariables', () => ({
+    ext: {
+        outputChannel: {
+            error: (...args: unknown[]) => outputChannelError(...args),
+        },
+    },
+}));
+
+import { parseDocumentId } from './parseDocumentId';
+
+/**
+ * The ids passed to parseDocumentId are produced by EJSON.stringify (canonical
+ * for the table grid, relaxed for the document view). Build inputs the same way
+ * so the tests exercise the real contract.
+ */
+const canonical = (value: unknown): string => EJSON.stringify(value, { relaxed: false });
+
+describe('parseDocumentId', () => {
+    beforeEach(() => {
+        outputChannelError.mockClear();
+    });
+
+    it('parses an ObjectId from its EJSON $oid form (no hex guessing needed)', () => {
+        const oid = new ObjectId();
+        // This is exactly how the UI serializes an ObjectId _id: {"$oid":"..."}.
+        expect(canonical(oid)).toBe(`{"$oid":"${oid.toHexString()}"}`);
+
+        const result = parseDocumentId(canonical(oid));
+        expect(result).toBeInstanceOf(ObjectId);
+        expect((result as ObjectId).toHexString()).toBe(oid.toHexString());
+    });
+
+    it('does NOT guess: a bare 24-character hex string (no $oid) throws', () => {
+        const hex = 'a'.repeat(24);
+        expect(() => parseDocumentId(hex)).toThrow(/Unable to parse the document _id/);
+        expect(outputChannelError).toHaveBeenCalledTimes(1);
+    });
+
+    it('keeps a string _id as a string', () => {
+        expect(parseDocumentId(canonical('my-key'))).toBe('my-key');
+    });
+
+    it('keeps a numeric-looking string _id as a string (not coerced to a number)', () => {
+        expect(parseDocumentId(canonical('42'))).toBe('42');
+    });
+
+    it('keeps a 24-hex-char string _id as a string when it arrives as EJSON', () => {
+        const hexLikeString = 'f'.repeat(24);
+        expect(parseDocumentId(canonical(hexLikeString))).toBe(hexLikeString);
+    });
+
+    it('parses a number _id to a number', () => {
+        expect(parseDocumentId(canonical(2343345))).toBe(2343345);
+    });
+
+    it('keeps a UUID string _id as a string', () => {
+        const uuid = '550e8400-e29b-41d4-a716-446655440000';
+        expect(parseDocumentId(canonical(uuid))).toBe(uuid);
+    });
+
+    it('parses an embedded-document _id, preserving field order and value types', () => {
+        const embedded = { author: 'John', userId: 2343345 };
+        const result = parseDocumentId(canonical(embedded)) as Record<string, unknown>;
+
+        expect(result).toEqual(embedded);
+        // Field order matters for embedded-document matching in the driver.
+        expect(Object.keys(result)).toEqual(['author', 'userId']);
+        expect(typeof result.userId).toBe('number');
+        // Round-trips back to the exact canonical EJSON the driver would match on.
+        expect(canonical(result)).toBe(canonical(embedded));
+    });
+
+    it('parses an array _id', () => {
+        expect(parseDocumentId(canonical([1, 'a']))).toEqual([1, 'a']);
+    });
+
+    describe('unparseable ids', () => {
+        it.each(['', '   ', 'not json', '{bad json', 'undefined'])(
+            'throws and logs to the output channel for %p',
+            (badId) => {
+                expect(() => parseDocumentId(badId)).toThrow(/Unable to parse the document _id/);
+                expect(outputChannelError).toHaveBeenCalledTimes(1);
+                expect(outputChannelError.mock.calls[0][0]).toContain('Unable to parse document _id');
+            },
+        );
+
+        it('echoes the offending value into the diagnostics', () => {
+            expect(() => parseDocumentId('not json')).toThrow();
+            expect(outputChannelError.mock.calls[0][0]).toContain('"not json"');
+        });
+
+        it('caps the echoed value for very long ids', () => {
+            const longId = 'x'.repeat(5000); // not valid EJSON, not a hex ObjectId
+            expect(() => parseDocumentId(longId)).toThrow();
+
+            const logged = String(outputChannelError.mock.calls[0][0]);
+            expect(logged).toContain('chars total');
+            expect(logged).toContain('5000');
+            // The raw value must not be dumped in full.
+            expect(logged).not.toContain('x'.repeat(300));
+        });
+    });
+});

src/documentdb/utils/parseDocumentId.ts

@@ -0,0 +1,56 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import * as l10n from '@vscode/l10n';
+import { EJSON } from 'bson';
+import { ext } from '../../extensionVariables';
+
+/**
+ * Upper bound on how many characters of an unparseable `_id` we echo into
+ * diagnostics. Ids can be arbitrarily large (e.g. embedded-document ids), so we
+ * cap the logged representation to keep the output channel readable and to
+ * avoid dumping large payloads.
+ */
+const MAX_LOGGED_ID_LENGTH = 256;
+
+/**
+ * Convert a stringified document `_id` into the exact BSON value the driver understands.
+ *
+ * The id is expected to be Extended JSON (EJSON) produced by `EJSON.stringify`,
+ * which is the BSON library's own canonical/relaxed serialization. `EJSON.parse`
+ * is therefore the ground truth: it round-trips every `_id` type the driver
+ * supports — `ObjectId` (as `{"$oid":"…"}`), string, number, `UUID`, `Date`,
+ * `Decimal128`, **embedded documents** (e.g. `{ author: 'John', userId: 2343345 }`)
+ * and arrays — preserving field order, which is significant when the driver
+ * matches an embedded-document `_id`.
+ *
+ * If the id cannot be parsed, we **throw** rather than guess. Silently coercing
+ * an unrecognized id into a raw string (or assuming it is an ObjectId) would
+ * change which document the driver matches and could read or delete the wrong
+ * record, so we fail loudly instead. The offending value (length-capped) is
+ * written to the output channel for follow-up.
+ *
+ * @throws Error if the id cannot be interpreted as a BSON value the driver understands.
+ */
+export function parseDocumentId(id: string): unknown {
+    try {
+        return EJSON.parse(id);
+    } catch {
+        // Not Extended JSON — fall through to fail loudly below rather than guessing.
+    }
+
+    const preview = id.length > MAX_LOGGED_ID_LENGTH ? id.slice(0, MAX_LOGGED_ID_LENGTH) : id;
+    const detail = `${JSON.stringify(preview)}${
+        id.length > MAX_LOGGED_ID_LENGTH ? ` … (${id.length} chars total)` : ''
+    }`;
+
+    ext.outputChannel.error(
+        'Unable to parse document _id. The value could not be interpreted as a BSON type understood ' +
+            'by the driver (expected Extended JSON — e.g. {"$oid":"…"}, {"$numberInt":"…"}, or an ' +
+            `embedded document). Received _id: ${detail}`,
+    );
+
+    throw new Error(l10n.t('Unable to parse the document _id. See the output channel for details.'));
+}