Skip to content

Commit 0a5f624

Browse files
authored
Support non-ObjectId _id types in ClustersClient (fixes #217) (#719)
2 parents 8fda157 + 7a60e0c commit 0a5f624

5 files changed

Lines changed: 195 additions & 65 deletions

File tree

l10n/bundle.l10n.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -765,7 +765,6 @@
765765
"Invalid connection string format. It should start with \"mongodb://\" or \"mongodb+srv://\"": "Invalid connection string format. It should start with \"mongodb://\" or \"mongodb+srv://\"",
766766
"Invalid Connection String: {error}": "Invalid Connection String: {error}",
767767
"Invalid connection type selected.": "Invalid connection type selected.",
768-
"Invalid document ID: {0}": "Invalid document ID: {0}",
769768
"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\" }",
770769
"Invalid folder type.": "Invalid folder type.",
771770
"Invalid node type.": "Invalid node type.",
@@ -1388,6 +1387,7 @@
13881387
"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.",
13891388
"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.",
13901389
"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.",
1390+
"Unable to parse the document _id. See the output channel for details.": "Unable to parse the document _id. See the output channel for details.",
13911391
"Unable to retrieve credentials for cluster \"{cluster}\".": "Unable to retrieve credentials for cluster \"{cluster}\".",
13921392
"Unable to retrieve credentials for the selected cluster.": "Unable to retrieve credentials for the selected cluster.",
13931393
"Understanding Your Query Execution Plan": "Understanding Your Query Execution Plan",

src/documentdb/ClusterSession.ts

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,15 @@ import { type FieldEntry } from '@documentdb-js/schema-analyzer';
77
import { ParseMode, parse as parseShellBSON } from '@mongodb-js/shell-bson-parser';
88
import * as l10n from '@vscode/l10n';
99
import { EJSON } from 'bson';
10-
import { ObjectId, type Document, type Filter, type WithId } from 'mongodb';
10+
import { type Document, type Filter, type WithId } from 'mongodb';
1111
import { ext } from '../extensionVariables';
1212
import { getDataAtPath } from '../utils/slickgrid/mongo/toSlickGridTable';
1313
import { toSlickGridTree, type TreeData } from '../utils/slickgrid/mongo/toSlickGridTree';
1414
import { type QueryInsightsStage2Response } from '../webviews/documentdb/collectionView/types/queryInsights';
1515
import { ClustersClient, type FindQueryParams } from './ClustersClient';
1616
import { SchemaStore } from './SchemaStore';
1717
import { fixupDocumentDbExplain } from './utils/fixupDocumentDbExplain';
18+
import { parseDocumentId } from './utils/parseDocumentId';
1819
import { toFilterQueryObj } from './utils/toFilterQuery';
1920

2021
export type TableDataEntry = {
@@ -335,31 +336,11 @@ export class ClusterSession {
335336

336337
if (acknowledged) {
337338
this._currentRawDocuments = this._currentRawDocuments.filter((doc) => {
338-
// Convert documentIds to BSON types and compare them with doc._id
339339
return !documentIds.some((id) => {
340-
let parsedId;
341-
try {
342-
// eslint-disable-next-line
343-
parsedId = EJSON.parse(id);
344-
} catch {
345-
if (ObjectId.isValid(id)) {
346-
parsedId = new ObjectId(id);
347-
} else {
348-
parsedId = id;
349-
}
350-
}
351-
352-
/**
353-
* deep equality for _id is tricky as we'd have to consider embedded objects,
354-
* arrays, etc. For now, we'll just stringify the _id and compare the strings.
355-
* The reasoning here is that this operation is used during interactive work
356-
* and were not expecting to delete a large number of documents at once.
357-
* Hence, the performance impact of this approach is negligible, and it's more
358-
* about simplicity here.
359-
*/
340+
const parsedId = parseDocumentId(id);
360341

361342
const docIdStr = EJSON.stringify(doc._id, { relaxed: false }, 0);
362-
const parsedIdStr = EJSON.stringify(parsedId, { relaxed: false }, 0);
343+
const parsedIdStr = EJSON.stringify(parsedId as Document, { relaxed: false }, 0);
363344

364345
return docIdStr === parsedIdStr;
365346
});

src/documentdb/ClustersClient.ts

Lines changed: 9 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import {
1717
} from '@microsoft/vscode-azext-utils';
1818
import { ParseMode, parse as parseShellBSON } from '@mongodb-js/shell-bson-parser';
1919
import * as l10n from '@vscode/l10n';
20-
import { EJSON } from 'bson';
2120
import { randomUUID } from 'crypto';
2221
import {
2322
MongoBulkWriteError,
@@ -60,6 +59,7 @@ import { SchemaStore } from './SchemaStore';
6059
import { getHostsFromConnectionString, hasAzureDomain } from './utils/connectionStringHelpers';
6160
import { fixupDocumentDbExplain } from './utils/fixupDocumentDbExplain';
6261
import { getClusterMetadata, type ClusterMetadata } from './utils/getClusterMetadata';
62+
import { parseDocumentId } from './utils/parseDocumentId';
6363
import { toFilterQueryObj } from './utils/toFilterQuery';
6464

6565
export interface DatabaseItemModel {
@@ -974,49 +974,24 @@ export class ClustersClient {
974974
// TODO: revisit, maybe we can work on BSON here for the documentIds, and the conversion from string etc.,
975975
// will remain in the ClusterSession class
976976
async deleteDocuments(databaseName: string, collectionName: string, documentIds: string[]): Promise<boolean> {
977-
// Convert input data to BSON types
978-
const parsedDocumentIds = documentIds.map((id) => {
979-
let parsedId;
980-
try {
981-
// eslint-disable-next-line
982-
parsedId = EJSON.parse(id);
983-
} catch {
984-
if (ObjectId.isValid(id)) {
985-
parsedId = new ObjectId(id);
986-
} else {
987-
throw new Error(l10n.t('Invalid document ID: {0}', id));
988-
}
989-
}
990-
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
991-
return parsedId;
992-
});
977+
const parsedDocumentIds = documentIds.map((id) => parseDocumentId(id));
993978

994979
// Connect and execute
995980
const collection = this._mongoClient.db(databaseName).collection(collectionName);
996-
const deleteResult: DeleteResult = await collection.deleteMany({ _id: { $in: parsedDocumentIds } });
981+
const deleteResult: DeleteResult = await collection.deleteMany({
982+
_id: { $in: parsedDocumentIds },
983+
} as Filter<Document>);
997984

998985
return deleteResult.acknowledged;
999986
}
1000987

1001988
async pointRead(databaseName: string, collectionName: string, documentId: string) {
1002-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
1003-
let parsedDocumentId: any;
1004-
try {
1005-
// eslint-disable-next-line
1006-
parsedDocumentId = EJSON.parse(documentId);
1007-
} catch (error) {
1008-
if (ObjectId.isValid(documentId)) {
1009-
parsedDocumentId = new ObjectId(documentId);
1010-
} else {
1011-
throw error;
1012-
}
1013-
}
989+
const parsedDocumentId = parseDocumentId(documentId);
1014990

1015991
// connect and execute
1016992
const collection = this._mongoClient.db(databaseName).collection(collectionName);
1017993

1018-
// eslint-disable-next-line
1019-
const documentContent = await collection.findOne({ _id: parsedDocumentId });
994+
const documentContent = await collection.findOne({ _id: parsedDocumentId } as Filter<Document>);
1020995

1021996
return documentContent;
1022997
}
@@ -1033,17 +1008,10 @@ export class ClustersClient {
10331008
let parsedId: any;
10341009

10351010
if (documentId === '') {
1036-
// TODO: do not rely in empty string, use null or undefined
1011+
// TODO: do not rely on empty string, use null or undefined
10371012
parsedId = new ObjectId();
10381013
} else {
1039-
try {
1040-
// eslint-disable-next-line
1041-
parsedId = EJSON.parse(documentId);
1042-
} catch {
1043-
if (ObjectId.isValid(documentId)) {
1044-
parsedId = new ObjectId(documentId);
1045-
}
1046-
}
1014+
parsedId = parseDocumentId(documentId);
10471015
}
10481016

10491017
// connect and execute
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import { EJSON, ObjectId } from 'bson';
7+
8+
// Mock vscode l10n so the localized throw message is deterministic.
9+
jest.mock('vscode', () => ({
10+
l10n: {
11+
t: (message: string, ...args: unknown[]) => {
12+
let result = message;
13+
args.forEach((arg, index) => {
14+
result = result.replace(`{${index}}`, String(arg));
15+
});
16+
return result;
17+
},
18+
},
19+
}));
20+
21+
// Capture output-channel diagnostics emitted on failure.
22+
const outputChannelError = jest.fn();
23+
jest.mock('../../extensionVariables', () => ({
24+
ext: {
25+
outputChannel: {
26+
error: (...args: unknown[]) => outputChannelError(...args),
27+
},
28+
},
29+
}));
30+
31+
import { parseDocumentId } from './parseDocumentId';
32+
33+
/**
34+
* The ids passed to parseDocumentId are produced by EJSON.stringify (canonical
35+
* for the table grid, relaxed for the document view). Build inputs the same way
36+
* so the tests exercise the real contract.
37+
*/
38+
const canonical = (value: unknown): string => EJSON.stringify(value, { relaxed: false });
39+
40+
describe('parseDocumentId', () => {
41+
beforeEach(() => {
42+
outputChannelError.mockClear();
43+
});
44+
45+
it('parses an ObjectId from its EJSON $oid form (no hex guessing needed)', () => {
46+
const oid = new ObjectId();
47+
// This is exactly how the UI serializes an ObjectId _id: {"$oid":"..."}.
48+
expect(canonical(oid)).toBe(`{"$oid":"${oid.toHexString()}"}`);
49+
50+
const result = parseDocumentId(canonical(oid));
51+
expect(result).toBeInstanceOf(ObjectId);
52+
expect((result as ObjectId).toHexString()).toBe(oid.toHexString());
53+
});
54+
55+
it('does NOT guess: a bare 24-character hex string (no $oid) throws', () => {
56+
const hex = 'a'.repeat(24);
57+
expect(() => parseDocumentId(hex)).toThrow(/Unable to parse the document _id/);
58+
expect(outputChannelError).toHaveBeenCalledTimes(1);
59+
});
60+
61+
it('keeps a string _id as a string', () => {
62+
expect(parseDocumentId(canonical('my-key'))).toBe('my-key');
63+
});
64+
65+
it('keeps a numeric-looking string _id as a string (not coerced to a number)', () => {
66+
expect(parseDocumentId(canonical('42'))).toBe('42');
67+
});
68+
69+
it('keeps a 24-hex-char string _id as a string when it arrives as EJSON', () => {
70+
const hexLikeString = 'f'.repeat(24);
71+
expect(parseDocumentId(canonical(hexLikeString))).toBe(hexLikeString);
72+
});
73+
74+
it('parses a number _id to a number', () => {
75+
expect(parseDocumentId(canonical(2343345))).toBe(2343345);
76+
});
77+
78+
it('keeps a UUID string _id as a string', () => {
79+
const uuid = '550e8400-e29b-41d4-a716-446655440000';
80+
expect(parseDocumentId(canonical(uuid))).toBe(uuid);
81+
});
82+
83+
it('parses an embedded-document _id, preserving field order and value types', () => {
84+
const embedded = { author: 'John', userId: 2343345 };
85+
const result = parseDocumentId(canonical(embedded)) as Record<string, unknown>;
86+
87+
expect(result).toEqual(embedded);
88+
// Field order matters for embedded-document matching in the driver.
89+
expect(Object.keys(result)).toEqual(['author', 'userId']);
90+
expect(typeof result.userId).toBe('number');
91+
// Round-trips back to the exact canonical EJSON the driver would match on.
92+
expect(canonical(result)).toBe(canonical(embedded));
93+
});
94+
95+
it('parses an array _id', () => {
96+
expect(parseDocumentId(canonical([1, 'a']))).toEqual([1, 'a']);
97+
});
98+
99+
describe('unparseable ids', () => {
100+
it.each(['', ' ', 'not json', '{bad json', 'undefined'])(
101+
'throws and logs to the output channel for %p',
102+
(badId) => {
103+
expect(() => parseDocumentId(badId)).toThrow(/Unable to parse the document _id/);
104+
expect(outputChannelError).toHaveBeenCalledTimes(1);
105+
expect(outputChannelError.mock.calls[0][0]).toContain('Unable to parse document _id');
106+
},
107+
);
108+
109+
it('echoes the offending value into the diagnostics', () => {
110+
expect(() => parseDocumentId('not json')).toThrow();
111+
expect(outputChannelError.mock.calls[0][0]).toContain('"not json"');
112+
});
113+
114+
it('caps the echoed value for very long ids', () => {
115+
const longId = 'x'.repeat(5000); // not valid EJSON, not a hex ObjectId
116+
expect(() => parseDocumentId(longId)).toThrow();
117+
118+
const logged = String(outputChannelError.mock.calls[0][0]);
119+
expect(logged).toContain('chars total');
120+
expect(logged).toContain('5000');
121+
// The raw value must not be dumped in full.
122+
expect(logged).not.toContain('x'.repeat(300));
123+
});
124+
});
125+
});
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import * as l10n from '@vscode/l10n';
7+
import { EJSON } from 'bson';
8+
import { ext } from '../../extensionVariables';
9+
10+
/**
11+
* Upper bound on how many characters of an unparseable `_id` we echo into
12+
* diagnostics. Ids can be arbitrarily large (e.g. embedded-document ids), so we
13+
* cap the logged representation to keep the output channel readable and to
14+
* avoid dumping large payloads.
15+
*/
16+
const MAX_LOGGED_ID_LENGTH = 256;
17+
18+
/**
19+
* Convert a stringified document `_id` into the exact BSON value the driver understands.
20+
*
21+
* The id is expected to be Extended JSON (EJSON) produced by `EJSON.stringify`,
22+
* which is the BSON library's own canonical/relaxed serialization. `EJSON.parse`
23+
* is therefore the ground truth: it round-trips every `_id` type the driver
24+
* supports — `ObjectId` (as `{"$oid":"…"}`), string, number, `UUID`, `Date`,
25+
* `Decimal128`, **embedded documents** (e.g. `{ author: 'John', userId: 2343345 }`)
26+
* and arrays — preserving field order, which is significant when the driver
27+
* matches an embedded-document `_id`.
28+
*
29+
* If the id cannot be parsed, we **throw** rather than guess. Silently coercing
30+
* an unrecognized id into a raw string (or assuming it is an ObjectId) would
31+
* change which document the driver matches and could read or delete the wrong
32+
* record, so we fail loudly instead. The offending value (length-capped) is
33+
* written to the output channel for follow-up.
34+
*
35+
* @throws Error if the id cannot be interpreted as a BSON value the driver understands.
36+
*/
37+
export function parseDocumentId(id: string): unknown {
38+
try {
39+
return EJSON.parse(id);
40+
} catch {
41+
// Not Extended JSON — fall through to fail loudly below rather than guessing.
42+
}
43+
44+
const preview = id.length > MAX_LOGGED_ID_LENGTH ? id.slice(0, MAX_LOGGED_ID_LENGTH) : id;
45+
const detail = `${JSON.stringify(preview)}${
46+
id.length > MAX_LOGGED_ID_LENGTH ? ` … (${id.length} chars total)` : ''
47+
}`;
48+
49+
ext.outputChannel.error(
50+
'Unable to parse document _id. The value could not be interpreted as a BSON type understood ' +
51+
'by the driver (expected Extended JSON — e.g. {"$oid":"…"}, {"$numberInt":"…"}, or an ' +
52+
`embedded document). Received _id: ${detail}`,
53+
);
54+
55+
throw new Error(l10n.t('Unable to parse the document _id. See the output channel for details.'));
56+
}

0 commit comments

Comments
 (0)