diff --git a/l10n/bundle.l10n.json b/l10n/bundle.l10n.json index cda983463..729b981bd 100644 --- a/l10n/bundle.l10n.json +++ b/l10n/bundle.l10n.json @@ -1085,6 +1085,7 @@ "This will also delete {0}.": "This will also delete {0}.", "This will execute all statements in the file against the connected cluster.": "This will execute all statements in the file against the connected cluster.", "This will prevent the query planner from using this index.": "This will prevent the query planner from using this index.", + "Tip: use .maxTimeMS() to increase the time limit for this query.": "Tip: use .maxTimeMS() to increase the time limit for this query.", "TLS/SSL Disabled": "TLS/SSL Disabled", "TLS/SSL Enabled": "TLS/SSL Enabled", "To connect to Azure resources, you need to sign in to Azure accounts.": "To connect to Azure resources, you need to sign in to Azure accounts.", diff --git a/package.json b/package.json index 4cead97da..52bcb9882 100644 --- a/package.json +++ b/package.json @@ -1167,11 +1167,6 @@ "default": 50, "minimum": 1 }, - "documentDB.timeout": { - "type": "number", - "description": "Maximum time (in seconds) allowed for a query to execute in the Query Playground or Interactive Shell before the operation is cancelled.", - "default": 30 - }, "documentDB.shell.initTimeout": { "type": "number", "description": "Maximum time (in seconds) to wait for the Interactive Shell to connect during initialization. Default value is 60 seconds.", diff --git a/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.test.ts b/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.test.ts index 057a5f990..8833c87f5 100644 --- a/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.test.ts +++ b/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.test.ts @@ -5,7 +5,7 @@ import { type MongoClient } from 'mongodb'; import { DocumentDBServiceProvider } from './DocumentDBServiceProvider'; -import { DocumentDBShellRuntime } from './DocumentDBShellRuntime'; +import { DocumentDBShellRuntime, normalizeDirectCommands } from './DocumentDBShellRuntime'; // Mock @mongosh modules to avoid needing a real database connection jest.mock('@mongosh/shell-api', () => ({ @@ -119,3 +119,130 @@ describe('DocumentDBShellRuntime', () => { }); }); }); + +describe('normalizeDirectCommands', () => { + describe('single-line input (no transformation)', () => { + it('leaves bare use unchanged', () => { + expect(normalizeDirectCommands('use mydb')).toBe('use mydb'); + }); + + it('leaves bare show unchanged', () => { + expect(normalizeDirectCommands('show dbs')).toBe('show dbs'); + }); + + it('leaves function-call form unchanged', () => { + expect(normalizeDirectCommands('use("mydb")')).toBe('use("mydb")'); + }); + + it('leaves regular code unchanged', () => { + expect(normalizeDirectCommands('db.test.find()')).toBe('db.test.find()'); + }); + }); + + describe('multi-line input with bare use', () => { + it('rewrites bare use followed by a query', () => { + const input = 'use mydb\ndb.test.find()'; + const expected = 'use("mydb");\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + + it('rewrites bare use with trailing semicolon', () => { + const input = 'use mydb;\ndb.test.find()'; + const expected = 'use("mydb");\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + + it('rewrites bare use with leading whitespace', () => { + const input = ' use mydb\ndb.test.find()'; + const expected = ' use("mydb");\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + + it('appends a trailing semicolon to neutralize ASI hazards', () => { + // Without the trailing `;`, the next line starting with `[` + // would bind to the call expression as member access. + const input = 'use mydb\n[1, 2, 3].forEach((x) => x)'; + const expected = 'use("mydb");\n[1, 2, 3].forEach((x) => x)'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + + it('rewrites use in the middle of multi-line code', () => { + // With the AST-based implementation the rewrite covers every + // top-level `use`/`show` statement, not just the first line. + const input = 'db.test.find()\nuse otherdb\ndb.other.find()'; + const expected = 'db.test.find()\nuse("otherdb");\ndb.other.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + + it('rewrites multiple top-level use/show statements in one block', () => { + const input = 'show dbs\nuse mydb\ndb.test.find()\nuse otherdb\ndb.other.find()'; + const expected = 'show("dbs");\nuse("mydb");\ndb.test.find()\nuse("otherdb");\ndb.other.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + + it('leaves matching lines inside template literals unchanged', () => { + const input = 'const s = `\nuse mydb\n`;\nconsole.log(s)'; + expect(normalizeDirectCommands(input)).toBe(input); + }); + + it('leaves matching lines inside block comments unchanged', () => { + const input = '/* use mydb\n show dbs */\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(input); + }); + + it('leaves matching text inside line comments unchanged', () => { + const input = '// use mydb\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(input); + }); + + it('leaves matching text inside regex literals unchanged', () => { + const input = 'const x = /use mydb/;\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(input); + }); + + it('rewrites first line even when preceded by blank lines', () => { + const input = '\n\n use mydb\ndb.test.find()'; + const expected = '\n\n use("mydb");\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + }); + + describe('multi-line input with bare show', () => { + it('rewrites bare show followed by a query', () => { + const input = 'show dbs\ndb.test.find()'; + const expected = 'show("dbs");\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + + it('rewrites bare show collections', () => { + const input = 'show collections\ndb.test.find()'; + const expected = 'show("collections");\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + }); + + describe('does not transform function-call form', () => { + it('leaves use("name") unchanged in multi-line', () => { + const input = 'use("mydb")\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(input); + }); + + it("leaves use('name') unchanged in multi-line", () => { + const input = "use('mydb')\ndb.test.find()"; + expect(normalizeDirectCommands(input)).toBe(input); + }); + + it('leaves show("dbs") unchanged in multi-line', () => { + const input = 'show("dbs")\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(input); + }); + }); + + describe('escapes special characters in database names', () => { + it('escapes quotes in database name', () => { + const input = 'use my"db\ndb.test.find()'; + const expected = 'use("my\\"db");\ndb.test.find()'; + expect(normalizeDirectCommands(input)).toBe(expected); + }); + }); +}); diff --git a/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.ts b/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.ts index ca1a40597..8a56c9f4f 100644 --- a/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.ts +++ b/packages/documentdb-shell-runtime/src/DocumentDBShellRuntime.ts @@ -18,6 +18,13 @@ import { type ShellRuntimeOptions, } from './types'; +/** + * Matches ` ` on a single line. Used to extract the argument text + * after the scanner has already confirmed that `` starts a real top-level + * `use`/`show` statement (not inside a string, comment, or regex literal). + */ +const DIRECT_COMMAND_LINE_RE = /^(\s*)(use|show)\s+([^(\s][^;]*?)\s*;?\s*$/; + const DEFAULT_OPTIONS: ShellRuntimeOptions = { productName: 'DocumentDB for VS Code', productDocsLink: 'https://github.com/microsoft/vscode-documentdb', @@ -104,6 +111,13 @@ export class DocumentDBShellRuntime { return intercepted; } + // Normalize bare direct commands (`use dbName`, `show dbs`) into function-call + // form (`use("dbName")`, `show("dbs")`) so they go through the async rewriter + // instead of short-circuiting. Without this, a bare `use` as the first token + // of a multi-line block consumes the entire input and silently drops subsequent + // statements. + code = normalizeDirectCommands(code); + this.log( 'trace', `Evaluating code (${code.split('\n').length} lines, ${code.length} chars, db: ${databaseName})`, @@ -298,3 +312,300 @@ export class DocumentDBShellRuntime { this._callbacks.onLog?.(level, message); } } + +/** + * Rewrites bare `use ` / `show ` direct shell commands into + * function-call form (`use("");` / `show("");`). + * + * ## Why this is needed + * + * Direct shell commands like `use mydb` are detected by the evaluation + * pipeline as special tokens. When they appear as the first line of a + * multi-line code block, the pipeline processes only the direct command + * and silently discards all subsequent statements. Converting them to + * function-call form bypasses that short-circuit so the entire block is + * evaluated normally. + * + * ## How it works + * + * A single linear scan of the input tracks whether each character sits in + * plain code, a `//` line comment, a `/* ... *\/` block comment, a `'...'` + * or `"..."` string literal, a `` `...` `` template literal (including + * `${...}` expression nesting), or a `/.../` regex literal. Only line + * starts that fall in the plain-code state are considered candidates for + * rewriting. The per-line regex extracts the argument once the context + * check has passed. + * + * This avoids the collateral rewrites a naive regex would produce in: + * + * - single- and double-quoted string literals + * - template literals (`` `use mydb` `` as string content) + * - line and block comments + * - regular-expression literals (`/use mydb/`) + * + * ### Scope note — why a scanner, not a parser + * + * This is a lexical scanner, not a syntactic parser. It recognizes the + * literal forms listed above, which covers every collateral-rewrite + * failure mode reported so far. It intentionally does **not** understand + * JavaScript statement structure, so a handful of exotic cases are not + * distinguished: + * + * - `use` / `show` as a declared identifier rather than a statement + * starter (e.g. `const use = mydb; use` — runtime-invalid anyway). + * - `use mydb` nested inside a block such as `if (x) { use mydb }` + * (the bare form was never valid inside a block either; rewriting it + * to `use("mydb");` is still a legal expression statement). + * - Contextual keywords used where a regex is legal but the scanner + * guessed division, or vice-versa. + * + * Getting 100% of these right would require a real JS parser (e.g. + * `acorn` / `acorn-loose`) or the TypeScript compiler. We deliberately + * avoid adding that dependency: `@microsoft/documentdb-vscode-shell-runtime` + * is intended to also ship as a lightweight standalone runtime for CLI + * tooling, and pulling in a full JS parser would dominate its footprint + * for an edge case that is not observed in real user input. + * + * ## ASI safety + * + * The emitted replacement always ends with `;` so a following line that + * begins with `[`, `(`, `+`, `-`, or `/` starts a fresh statement instead + * of binding to the call expression. + */ +export function normalizeDirectCommands(code: string): string { + if (!code.includes('\n')) { + return code; + } + + // Cheap early exit: if neither token appears anywhere, skip scanning. + if (!/\b(use|show)\b/.test(code)) { + return code; + } + + const candidateLineStarts = findCodeLineStarts(code); + if (candidateLineStarts.length === 0) { + return code; + } + + type Edit = { lineStart: number; lineEnd: number; replacement: string }; + const edits: Edit[] = []; + + for (const lineStart of candidateLineStarts) { + const nextNewline = code.indexOf('\n', lineStart); + const lineEnd = nextNewline === -1 ? code.length : nextNewline; + const line = code.slice(lineStart, lineEnd); + + const match = DIRECT_COMMAND_LINE_RE.exec(line); + if (!match) continue; + + const [, indent, cmd, arg] = match; + edits.push({ + lineStart, + lineEnd, + replacement: `${indent}${cmd}(${JSON.stringify(arg)});`, + }); + } + + if (edits.length === 0) { + return code; + } + + // Apply right-to-left so earlier offsets stay valid. + edits.sort((a, b) => b.lineStart - a.lineStart); + let result = code; + for (const edit of edits) { + result = result.slice(0, edit.lineStart) + edit.replacement + result.slice(edit.lineEnd); + } + return result; +} + +/** + * Scan `code` once and return the offsets of every line start that falls in + * plain-code state (i.e., outside any string, template, comment, or regex + * literal). The returned offsets are candidates for direct-command rewriting. + * + * The scanner covers exactly what we need to avoid false rewrites: + * + * - `//` line comments and `/* *\/` block comments + * - single- and double-quoted strings with `\` escapes + * - template literals, including nested `${ ... }` expressions (which are + * themselves code and can contain further strings/templates) + * - regex literals, disambiguated from division by tracking whether a `/` + * can begin an expression at its position + * + * It is **lexical** only; it does not build an AST or understand statement + * structure. A full parser (e.g. `acorn` / `acorn-loose`) would be needed + * for 100% syntactic accuracy — see the note on `normalizeDirectCommands` + * for why that trade-off is intentional here. + */ +function findCodeLineStarts(code: string): number[] { + const starts: number[] = []; + // `${...}` nesting inside template literals: each element counts the + // currently-open `{` inside that expression so we know when to pop back + // into template-literal state. + const templateStack: number[] = []; + let inLineComment = false; + let inBlockComment = false; + let stringQuote: '"' | "'" | null = null; + let inTemplate = false; + let inRegex = false; + let regexCharClass = false; + // Whether a `/` at the current cursor may start a regex literal. + let canRegex = true; + // True only at the FIRST offset of a line (offset 0, or the position + // right after a newline). Cleared as soon as we consume that offset, + // so we never record the same line twice. + let atLineStart = true; + + const len = code.length; + for (let i = 0; i < len; i++) { + const ch = code[i]; + const next = i + 1 < len ? code[i + 1] : ''; + + // Record plain-code line starts (at most once per line). + if ( + atLineStart && + !inLineComment && + !inBlockComment && + stringQuote === null && + !inTemplate && + !inRegex && + templateStack.length === 0 + ) { + starts.push(i); + } + atLineStart = false; + + if (inLineComment) { + if (ch === '\n') { + inLineComment = false; + atLineStart = true; + canRegex = true; + } + continue; + } + if (inBlockComment) { + if (ch === '*' && next === '/') { + inBlockComment = false; + i++; + canRegex = true; + } else if (ch === '\n') { + atLineStart = true; + } + continue; + } + if (stringQuote !== null) { + if (ch === '\\' && next !== '') { + i++; + continue; + } + if (ch === stringQuote) { + stringQuote = null; + canRegex = false; + } else if (ch === '\n') { + // Unterminated string at newline: recover by exiting string + // state so we don't swallow the rest of the input. + stringQuote = null; + atLineStart = true; + canRegex = true; + } + continue; + } + if (inTemplate) { + if (ch === '\\' && next !== '') { + i++; + continue; + } + if (ch === '`') { + inTemplate = false; + canRegex = false; + } else if (ch === '$' && next === '{') { + templateStack.push(1); + inTemplate = false; + i++; + canRegex = true; + } else if (ch === '\n') { + atLineStart = true; + } + continue; + } + if (inRegex) { + if (ch === '\\' && next !== '') { + i++; + continue; + } + if (ch === '[') { + regexCharClass = true; + } else if (ch === ']') { + regexCharClass = false; + } else if (ch === '/' && !regexCharClass) { + inRegex = false; + // Consume optional flags. + while (i + 1 < len && /[a-z]/i.test(code[i + 1])) i++; + canRegex = false; + } else if (ch === '\n') { + // Unterminated regex: recover. + inRegex = false; + regexCharClass = false; + atLineStart = true; + canRegex = true; + } + continue; + } + + // Plain-code state (outer) or template-expression state (inner). + if (ch === '/' && next === '/') { + inLineComment = true; + i++; + continue; + } + if (ch === '/' && next === '*') { + inBlockComment = true; + i++; + continue; + } + if (ch === '"' || ch === "'") { + stringQuote = ch as '"' | "'"; + canRegex = false; + continue; + } + if (ch === '`') { + inTemplate = true; + canRegex = false; + continue; + } + if (ch === '/' && canRegex) { + inRegex = true; + regexCharClass = false; + continue; + } + if (ch === '{' && templateStack.length > 0) { + templateStack[templateStack.length - 1]++; + } + if (ch === '}' && templateStack.length > 0) { + templateStack[templateStack.length - 1]--; + if (templateStack[templateStack.length - 1] === 0) { + templateStack.pop(); + inTemplate = true; + canRegex = false; + continue; + } + } + if (ch === '\n') { + atLineStart = true; + canRegex = true; + continue; + } + + // Heuristic for the regex/division ambiguity: letters/digits/closers + // disallow a regex at the next `/`; other punctuation allows one. + // Adequate for line-start candidates, which is all we care about. + if (/[A-Za-z0-9_$)\]]/.test(ch)) { + canRegex = false; + } else if (!/\s/.test(ch)) { + canRegex = true; + } + } + + return starts; +} diff --git a/src/documentdb/playground/PlaygroundEvaluator.ts b/src/documentdb/playground/PlaygroundEvaluator.ts index 69537b5e7..f9e6e8fad 100644 --- a/src/documentdb/playground/PlaygroundEvaluator.ts +++ b/src/documentdb/playground/PlaygroundEvaluator.ts @@ -5,7 +5,7 @@ import * as l10n from '@vscode/l10n'; import { randomUUID } from 'crypto'; -import * as vscode from 'vscode'; +import type * as vscode from 'vscode'; import { ext } from '../../extensionVariables'; import { getBatchSizeSetting } from '../../utils/workspacUtils'; import { CredentialCache } from '../CredentialCache'; @@ -156,8 +156,6 @@ export class PlaygroundEvaluator implements vscode.Disposable { // Send eval message and await result onProgress?.(l10n.t('Running query…')); - const timeoutSec = vscode.workspace.getConfiguration().get(ext.settingsKeys.shellTimeout) ?? 30; - const timeoutMs = timeoutSec * 1000; this._sessionEvalCount++; @@ -169,7 +167,7 @@ export class PlaygroundEvaluator implements vscode.Disposable { displayBatchSize: getBatchSizeSetting(), }; - const workerResult = await this._workerManager.sendEval(evalMsg, timeoutMs); + const workerResult = await this._workerManager.sendEval(evalMsg); return this.deserializeResult(workerResult.result); } diff --git a/src/documentdb/playground/WorkerSessionManager.ts b/src/documentdb/playground/WorkerSessionManager.ts index 746b85b5c..0c9762e20 100644 --- a/src/documentdb/playground/WorkerSessionManager.ts +++ b/src/documentdb/playground/WorkerSessionManager.ts @@ -122,14 +122,11 @@ export class WorkerSessionManager implements vscode.Disposable { * * @returns The serializable execution result from the worker. */ - async sendEval( - evalMsg: MainToWorkerMessage & { type: 'eval' }, - timeoutMs: number, - ): Promise<{ result: SerializableExecutionResult }> { + async sendEval(evalMsg: MainToWorkerMessage & { type: 'eval' }): Promise<{ result: SerializableExecutionResult }> { this._workerState = 'executing'; try { - const result = await this.sendRequest<{ result: SerializableExecutionResult }>(evalMsg, timeoutMs); + const result = await this.sendRequest<{ result: SerializableExecutionResult }>(evalMsg); return result; } finally { if (this._workerState === 'executing') { @@ -214,7 +211,7 @@ export class WorkerSessionManager implements vscode.Disposable { // If init fails (bad credentials, unreachable host, etc.), tear down // the worker so the next call can respawn cleanly. try { - await this.sendRequest(initMsg, initTimeoutMs); + await this.sendRequest(initMsg, initTimeoutMs, 'documentDB.shell.initTimeout'); this._workerState = 'ready'; } catch (error) { this.terminateWorker(); @@ -227,8 +224,17 @@ export class WorkerSessionManager implements vscode.Disposable { /** * Send a message to the worker and return a promise that resolves * when the corresponding response arrives. + * + * @param timeoutMs - Optional timeout. When set, kills the worker on expiry. + * Used for init and shutdown messages. Eval messages have no IPC timeout— + * the user cancels via Cancel button or Ctrl+C, and query-level timeouts + * are enforced by the database via maxTimeMS. + * @param timeoutSettingKey - Optional VS Code setting key used when building + * the user-facing `SettingsHintError` on timeout. When omitted, a plain + * `Error` is thrown (appropriate for internal/swallowed paths like + * shutdown where no actionable user setting exists). */ - private sendRequest(msg: MainToWorkerMessage, timeoutMs: number): Promise { + private sendRequest(msg: MainToWorkerMessage, timeoutMs?: number, timeoutSettingKey?: string): Promise { if (!this._worker) { return Promise.reject(new Error(l10n.t('Worker is not running'))); } @@ -242,32 +248,41 @@ export class WorkerSessionManager implements vscode.Disposable { reject, }); - // Timeout — kills the worker for safety (infinite loop protection) - const timer = setTimeout(() => { - const pending = this._pendingRequests.get(requestId); - if (pending) { - this._pendingRequests.delete(requestId); - this.killWorker(); - pending.reject( - new SettingsHintError( - l10n.t('Operation timed out after {0} seconds.', String(Math.round(timeoutMs / 1000))), - 'documentDB.timeout', - l10n.t('You can increase the timeout in Settings:'), - ), - ); - } - }, timeoutMs); + // Optional timeout — used for init/shutdown, not for eval + let timer: ReturnType | undefined; + if (timeoutMs !== undefined) { + timer = setTimeout(() => { + const pending = this._pendingRequests.get(requestId); + if (pending) { + this._pendingRequests.delete(requestId); + this.killWorker(); + const message = l10n.t( + 'Operation timed out after {0} seconds.', + String(Math.round(timeoutMs / 1000)), + ); + pending.reject( + timeoutSettingKey + ? new SettingsHintError( + message, + timeoutSettingKey, + l10n.t('You can increase the timeout in Settings:'), + ) + : new Error(message), + ); + } + }, timeoutMs); + } // Wire up timer clearing on resolve/reject const entry = this._pendingRequests.get(requestId)!; const originalResolve = entry.resolve; const originalReject = entry.reject; entry.resolve = (value: unknown) => { - clearTimeout(timer); + if (timer) clearTimeout(timer); originalResolve(value); }; entry.reject = (error: Error) => { - clearTimeout(timer); + if (timer) clearTimeout(timer); originalReject(error); }; @@ -306,10 +321,13 @@ export class WorkerSessionManager implements vscode.Disposable { const pending = this._pendingRequests.get(msg.requestId); if (pending) { this._pendingRequests.delete(msg.requestId); - const error = new Error(msg.error); + const error: Error & { code?: number } = new Error(msg.error); if (msg.stack) { error.stack = msg.stack; } + if (msg.code !== undefined) { + error.code = msg.code; + } pending.reject(error); } break; diff --git a/src/documentdb/playground/playgroundWorker.ts b/src/documentdb/playground/playgroundWorker.ts index 8ce7eb5dc..5038a9451 100644 --- a/src/documentdb/playground/playgroundWorker.ts +++ b/src/documentdb/playground/playgroundWorker.ts @@ -72,11 +72,16 @@ parentPort.on('message', (msg: MainToWorkerMessage) => { currentEvalRequestId = undefined; const errorMessage = err instanceof Error ? err.message : String(err); const stack = err instanceof Error ? err.stack : undefined; + const code = + err instanceof Error && 'code' in err && typeof (err as { code: unknown }).code === 'number' + ? (err as { code: number }).code + : undefined; const response: WorkerToMainMessage = { type: 'evalError', requestId: msg.requestId, error: errorMessage, stack, + code, }; parentPort!.postMessage(response); }); diff --git a/src/documentdb/playground/resultFormatter.test.ts b/src/documentdb/playground/resultFormatter.test.ts new file mode 100644 index 000000000..d77069e22 --- /dev/null +++ b/src/documentdb/playground/resultFormatter.test.ts @@ -0,0 +1,88 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { formatError, formatResult } from './resultFormatter'; +import { type PlaygroundConnection } from './types'; + +const connection: PlaygroundConnection = { + clusterId: 'test-cluster', + clusterDisplayName: 'Test Cluster', + databaseName: 'testDb', + viewId: 'connections', +}; + +describe('resultFormatter', () => { + describe('formatError', () => { + it('strips ANSI color codes from error messages', () => { + const error = new Error('\x1b[31mSyntaxError\x1b[0m: \x1b[1mMissing semicolon\x1b[0m'); + const result = formatError(error, 'db.test.find(', 10, connection); + + expect(result).not.toContain('\x1b['); + expect(result).toContain('SyntaxError: Missing semicolon'); + }); + + it('handles error messages without ANSI codes', () => { + const error = new Error('Connection refused'); + const result = formatError(error, 'db.test.find()', 5, connection); + + expect(result).toContain('Connection refused'); + }); + + it('strips complex ANSI sequences with multiple parameters', () => { + const error = new Error('\x1b[1;31mError\x1b[0m: \x1b[33mwarning text\x1b[0m'); + const result = formatError(error, 'bad code', 15, connection); + + expect(result).not.toContain('\x1b['); + expect(result).toContain('Error: warning text'); + }); + + it('strips error code prefix AND ANSI codes', () => { + const error = new Error('[COMMON-10001] \x1b[31mInvalid operation\x1b[0m'); + const result = formatError(error, 'db.test.find()', 10, connection); + + expect(result).not.toContain('[COMMON-10001]'); + expect(result).not.toContain('\x1b['); + expect(result).toContain('Invalid operation'); + }); + it('shows maxTimeMS hint for timeout errors with error code 50', () => { + const error: Error & { code?: number } = new Error('command timeout'); + error.code = 50; + const result = formatError(error, 'db.test.find()', 30000, connection); + + expect(result).toContain('.maxTimeMS()'); + }); + + it('does not show maxTimeMS hint without error code 50', () => { + const error = new Error('some other error'); + const result = formatError(error, 'db.test.find()', 30000, connection); + + expect(result).not.toContain('.maxTimeMS()'); + }); + }); + + describe('formatResult', () => { + it('formats a simple document result', () => { + const result = formatResult( + { type: 'Document', printable: { _id: 1, name: 'test' }, durationMs: 42 }, + 'db.test.findOne()', + connection, + ); + + expect(result).toContain('Result: Document'); + expect(result).toContain('42ms'); + expect(result).toContain('"name": "test"'); + }); + + it('formats a cursor result with document count', () => { + const result = formatResult( + { type: 'Cursor', printable: [{ _id: 1 }, { _id: 2 }], durationMs: 100 }, + 'db.test.find()', + connection, + ); + + expect(result).toContain('Cursor (2 documents)'); + }); + }); +}); diff --git a/src/documentdb/playground/resultFormatter.ts b/src/documentdb/playground/resultFormatter.ts index 634f59595..9865cdb93 100644 --- a/src/documentdb/playground/resultFormatter.ts +++ b/src/documentdb/playground/resultFormatter.ts @@ -9,6 +9,25 @@ import { SettingsHintError } from '../shell/SettingsHintError'; import { extractErrorCode } from '../shell/ShellOutputFormatter'; import { type ExecutionResult, type PlaygroundConnection } from './types'; +/** + * Strips ANSI escape sequences (SGR codes) from a string. + * Error messages from the evaluation pipeline may contain terminal color codes + * that render correctly in the interactive shell (PTY) but appear as raw + * escape characters in the playground's read-only output panel. + */ +// eslint-disable-next-line no-control-regex +const ANSI_SGR_RE = /\x1b\[[0-9;]*m/g; +function stripAnsi(str: string): string { + return str.replace(ANSI_SGR_RE, ''); +} + +/** + * Detect query timeout errors from the DocumentDB API (error code 50). + */ +function isMaxTimeMSError(error: unknown): boolean { + return error instanceof Error && 'code' in error && (error as { code: unknown }).code === 50; +} + /** * Formats a query playground execution result for display in a read-only output panel. * @@ -92,7 +111,8 @@ export function formatError( // Strip technical error codes for clean user-facing output; // the extracted code is preserved for future telemetry. const { message: errorMessage } = extractErrorCode(rawMessage); - lines.push(errorMessage); + // Strip ANSI color codes that are meant for terminal rendering + lines.push(stripAnsi(errorMessage)); // Show a settings hint for timeout errors (mirrors the shell's clickable hint) if (error instanceof SettingsHintError) { @@ -101,6 +121,12 @@ export function formatError( lines.push(`// Settings → '${error.settingKey}'`); } + // Detect query timeout errors from the DocumentDB API (code 50: MaxTimeMSExpired / ExceededTimeLimit) + if (isMaxTimeMSError(error)) { + lines.push(''); + lines.push(`// ${l10n.t('Tip: use .maxTimeMS() to increase the time limit for this query.')}`); + } + return lines.join('\n'); } diff --git a/src/documentdb/playground/workerTypes.ts b/src/documentdb/playground/workerTypes.ts index 7b165989e..14f675255 100644 --- a/src/documentdb/playground/workerTypes.ts +++ b/src/documentdb/playground/workerTypes.ts @@ -120,6 +120,8 @@ export type WorkerToMainMessage = readonly requestId: string; readonly error: string; readonly stack?: string; + /** Error code from the database (e.g. 50 for MaxTimeMSExpired). */ + readonly code?: number; } | { readonly type: 'shutdownComplete'; diff --git a/src/documentdb/shell/DocumentDBShellPty.test.ts b/src/documentdb/shell/DocumentDBShellPty.test.ts index 2b4f8f17b..aa86260c6 100644 --- a/src/documentdb/shell/DocumentDBShellPty.test.ts +++ b/src/documentdb/shell/DocumentDBShellPty.test.ts @@ -61,9 +61,6 @@ describe('DocumentDBShellPty', () => { if (_key === 'documentDB.shell.display.colorOutput') { return false; // Disable colors for easier test assertions } - if (_key === 'documentDB.timeout') { - return 120; - } } if (section === 'documentDB.shell' && _key === 'multiLinePasteBehavior') { return 'runLineByLine'; // Default to line-by-line in tests for backward compat @@ -171,7 +168,7 @@ describe('DocumentDBShellPty', () => { pty.handleInput('\r'); await new Promise((resolve) => setTimeout(resolve, 10)); - expect(mockEvaluate).toHaveBeenCalledWith('db.test.find()', expect.any(Number)); + expect(mockEvaluate).toHaveBeenCalledWith('db.test.find()'); }); it('should display evaluation result', async () => { @@ -248,8 +245,8 @@ describe('DocumentDBShellPty', () => { pty.handleInput('\r'); await new Promise((resolve) => setTimeout(resolve, 10)); - // ANSI clear screen sequence - expect(written).toContain('\x1b[2J\x1b[H'); + // ANSI clear screen + scrollback clear sequence + expect(written).toContain('\x1b[2J\x1b[3J\x1b[H'); }); }); @@ -511,7 +508,7 @@ describe('DocumentDBShellPty', () => { await new Promise((resolve) => setTimeout(resolve, 10)); - expect(mockEvaluate).toHaveBeenCalledWith('db.test.find({\n age: 25\n})', expect.any(Number)); + expect(mockEvaluate).toHaveBeenCalledWith('db.test.find({\n age: 25\n})'); }); it('should show database prompt after multi-line evaluation completes', async () => { @@ -559,7 +556,7 @@ describe('DocumentDBShellPty', () => { await new Promise((resolve) => setTimeout(resolve, 10)); - expect(mockEvaluate).toHaveBeenCalledWith('db.test.find({\n age: 25\n})', expect.any(Number)); + expect(mockEvaluate).toHaveBeenCalledWith('db.test.find({\n age: 25\n})'); }); it('should process sequential pasted commands via paste queue', async () => { @@ -582,8 +579,8 @@ describe('DocumentDBShellPty', () => { await new Promise((resolve) => setTimeout(resolve, 50)); expect(mockEvaluate).toHaveBeenCalledTimes(2); - expect(mockEvaluate).toHaveBeenNthCalledWith(1, 'show dbs', expect.any(Number)); - expect(mockEvaluate).toHaveBeenNthCalledWith(2, 'use newdb', expect.any(Number)); + expect(mockEvaluate).toHaveBeenNthCalledWith(1, 'show dbs'); + expect(mockEvaluate).toHaveBeenNthCalledWith(2, 'use newdb'); }); }); @@ -603,14 +600,10 @@ describe('DocumentDBShellPty', () => { if (section === 'terminal.integrated' && _key === 'enableMultiLinePasteWarning') { return vscodePasteWarning; } - // Preserve base settings needed by the PTY if (section === undefined || section === '') { if (_key === 'documentDB.shell.display.colorOutput') { return false; } - if (_key === 'documentDB.timeout') { - return 120; - } } return defaultValue; }), @@ -657,7 +650,7 @@ describe('DocumentDBShellPty', () => { await new Promise((resolve) => setTimeout(resolve, 100)); // Lines starting with . should be joined directly (no space) - expect(mockEvaluate).toHaveBeenCalledWith('db.restaurants.find({}).limit(5);', expect.any(Number)); + expect(mockEvaluate).toHaveBeenCalledWith('db.restaurants.find({}).limit(5);'); showQuickPickSpy.mockRestore(); }); @@ -675,7 +668,7 @@ describe('DocumentDBShellPty', () => { await new Promise((resolve) => setTimeout(resolve, 50)); - expect(mockEvaluate).toHaveBeenCalledWith('var x = 42;', expect.any(Number)); + expect(mockEvaluate).toHaveBeenCalledWith('var x = 42;'); }); it('should run line by line when behavior is "runLineByLine"', async () => { @@ -718,7 +711,7 @@ describe('DocumentDBShellPty', () => { await new Promise((resolve) => setTimeout(resolve, 50)); expect(showQuickPickSpy).not.toHaveBeenCalled(); - expect(mockEvaluate).toHaveBeenCalledWith('show dbs', expect.any(Number)); + expect(mockEvaluate).toHaveBeenCalledWith('show dbs'); showQuickPickSpy.mockRestore(); }); diff --git a/src/documentdb/shell/DocumentDBShellPty.ts b/src/documentdb/shell/DocumentDBShellPty.ts index a4d2f6004..7f7f72680 100644 --- a/src/documentdb/shell/DocumentDBShellPty.ts +++ b/src/documentdb/shell/DocumentDBShellPty.ts @@ -5,7 +5,6 @@ import * as l10n from '@vscode/l10n'; import * as vscode from 'vscode'; -import { ext } from '../../extensionVariables'; import { deserializeResultForSchema, feedResultToSchemaStore } from '../feedResultToSchemaStore'; import { type SerializableExecutionResult } from '../playground/workerTypes'; import { SchemaStore } from '../SchemaStore'; @@ -539,8 +538,7 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { private async evaluateInput(input: string): Promise { try { - const timeoutMs = this.getShellTimeoutMs(); - const result = await this._sessionManager.evaluate(input, timeoutMs); + const result = await this._sessionManager.evaluate(input); // Stop the spinner before writing any output so the spinner // character doesn't collide with the result text. @@ -598,6 +596,15 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { ), ); } + + // Detect query timeout errors (error code 50: MaxTimeMSExpired / ExceededTimeLimit) + if (error instanceof Error && 'code' in error && (error as { code: unknown }).code === 50) { + this.writeLine( + this._outputFormatter.formatSystemMessage( + l10n.t('Tip: use .maxTimeMS() to increase the time limit for this query.'), + ), + ); + } } } @@ -614,8 +621,8 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { } if (result.type === 'clear') { - // Write ANSI clear screen and cursor home - this._writeEmitter.fire('\x1b[2J\x1b[H'); + // Clear visible display, scrollback buffer, and move cursor home + this._writeEmitter.fire('\x1b[2J\x1b[3J\x1b[H'); return true; } @@ -1056,12 +1063,6 @@ export class DocumentDBShellPty implements vscode.Pseudoterminal { // ─── Private: Settings ─────────────────────────────────────────────────── - private getShellTimeoutMs(): number { - const config = vscode.workspace.getConfiguration(); - const timeoutSec = config.get(ext.settingsKeys.shellTimeout, 30); - return timeoutSec * 1000; - } - private isColorEnabled(): boolean { const config = vscode.workspace.getConfiguration(); return config.get('documentDB.shell.display.colorOutput', true); diff --git a/src/documentdb/shell/SettingsHintError.ts b/src/documentdb/shell/SettingsHintError.ts index 737befa31..08b794bcb 100644 --- a/src/documentdb/shell/SettingsHintError.ts +++ b/src/documentdb/shell/SettingsHintError.ts @@ -9,7 +9,7 @@ * * When the shell PTY catches a {@link SettingsHintError}, it displays * the error message followed by a hint line and a clickable settings - * action line (e.g., `⚙ [documentDB.timeout]`) that opens the setting. + * action line (e.g., `⚙ [documentDB.shell.initTimeout]`) that opens the setting. */ export class SettingsHintError extends Error { constructor( diff --git a/src/documentdb/shell/ShellSessionManager.ts b/src/documentdb/shell/ShellSessionManager.ts index 91dbb31da..246f5dc1e 100644 --- a/src/documentdb/shell/ShellSessionManager.ts +++ b/src/documentdb/shell/ShellSessionManager.ts @@ -200,10 +200,9 @@ export class ShellSessionManager implements vscode.Disposable { * and `it` are handled by @mongosh within the persistent context. * * @param code - JavaScript code or shell command to evaluate. - * @param timeoutMs - Timeout in milliseconds (kills worker on expiry). * @returns The serializable execution result from the worker. */ - async evaluate(code: string, timeoutMs: number): Promise { + async evaluate(code: string): Promise { // Reconnect if the worker is not alive — handles all cases: // timeout kills, Ctrl+C cancellation, unexpected worker crashes. if (!this._initialized || !this._workerManager.isAlive) { @@ -226,7 +225,7 @@ export class ShellSessionManager implements vscode.Disposable { displayBatchSize: getBatchSizeSetting(), }; - const workerResult = await this._workerManager.sendEval(evalMsg, timeoutMs); + const workerResult = await this._workerManager.sendEval(evalMsg); return workerResult.result; } diff --git a/src/documentdb/shell/ShellTerminalLinkProvider.test.ts b/src/documentdb/shell/ShellTerminalLinkProvider.test.ts index 1d08edebf..bc02fa243 100644 --- a/src/documentdb/shell/ShellTerminalLinkProvider.test.ts +++ b/src/documentdb/shell/ShellTerminalLinkProvider.test.ts @@ -212,7 +212,7 @@ describe('ShellTerminalLinkProvider', () => { it('should detect settings action line and return a settings link', () => { registerShellTerminal(mockTerminal, () => mockShellInfo('test-id')); - const actionLine = `${SETTINGS_ACTION_PREFIX}[documentDB.timeout]`; + const actionLine = `${SETTINGS_ACTION_PREFIX}[documentDB.shell.initTimeout]`; const context = { terminal: mockTerminal, line: actionLine, @@ -222,7 +222,7 @@ describe('ShellTerminalLinkProvider', () => { expect(links).toHaveLength(1); expect(links[0]).toMatchObject({ linkType: 'settings', - settingKey: 'documentDB.timeout', + settingKey: 'documentDB.shell.initTimeout', }); }); @@ -246,7 +246,7 @@ describe('ShellTerminalLinkProvider', () => { it('should not match settings line for non-shell terminals', () => { const context = { terminal: { name: 'bash' } as unknown as vscode.Terminal, - line: `${SETTINGS_ACTION_PREFIX}[documentDB.timeout]`, + line: `${SETTINGS_ACTION_PREFIX}[documentDB.shell.initTimeout]`, } as vscode.TerminalLinkContext; const links = provider.provideTerminalLinks(context); @@ -260,14 +260,14 @@ describe('ShellTerminalLinkProvider', () => { linkType: 'settings' as const, startIndex: 0, length: 40, - settingKey: 'documentDB.timeout', + settingKey: 'documentDB.shell.initTimeout', }; provider.handleTerminalLink(link as Parameters[0]); return new Promise((resolve) => { setTimeout(() => { - expect(spy).toHaveBeenCalledWith('workbench.action.openSettings', 'documentDB.timeout'); + expect(spy).toHaveBeenCalledWith('workbench.action.openSettings', 'documentDB.shell.initTimeout'); spy.mockRestore(); resolve(); }, 50); diff --git a/src/documentdb/shell/ShellTerminalLinkProvider.ts b/src/documentdb/shell/ShellTerminalLinkProvider.ts index 90fbd8f0e..934c29107 100644 --- a/src/documentdb/shell/ShellTerminalLinkProvider.ts +++ b/src/documentdb/shell/ShellTerminalLinkProvider.ts @@ -138,7 +138,7 @@ const PLAYGROUND_LINE_PATTERN = /(?:\x1b\[\d+m)*\u{2197} Query Playground \[([^\ * Regex to match the "Open Settings" action line. * * Captures: - * - Group 1: the VS Code setting key (e.g., `documentDB.timeout`) + * - Group 1: the VS Code setting key (e.g., `documentDB.shell.initTimeout`) * * The pattern accounts for optional ANSI color codes that wrap the line. * The format is locale-independent. diff --git a/src/extensionVariables.ts b/src/extensionVariables.ts index 714cff2ed..6c8b0a92e 100644 --- a/src/extensionVariables.ts +++ b/src/extensionVariables.ts @@ -64,7 +64,6 @@ export namespace ext { export namespace settingsKeys { export const batchSize = 'documentDB.batchSize'; - export const shellTimeout = 'documentDB.timeout'; export const shellInitTimeout = 'documentDB.shell.initTimeout'; export const confirmationStyle = 'documentDB.confirmations.confirmationStyle'; export const showOperationSummaries = 'documentDB.userInterface.ShowOperationSummaries';