Skip to content

Commit e936f3e

Browse files
kburtramclaude
kburtram
and
claude
authored
Fix: SQL Notebook cells now display SQL Server error messages inline (#21856)
- Wire up error messages in cell output regardless of batch.hasError flag (STS can send hasError=false for parse errors even with isError messages) - Fall back to lastStartedBatchId for messages with batchId=-1 or undefined so syntax/parse errors are captured instead of silently dropped - Use stderr() mime type for error output so it renders with red styling - Show 'Commands completed successfully' only when errorMessages is empty - Mark cell execution as failed (end=false) when any batch has errors - Add unit tests covering all fixed scenarios Fixes #21792, #21700 Co-authored-by: Claude Sonnet 4.6 <[email protected]>
1 parent 8a9e6ce commit e936f3e

4 files changed

Lines changed: 300 additions & 11 deletions

File tree

‎extensions/mssql/src/notebooks/notebookQueryExecutor.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,10 @@ export class NotebookQueryExecutor {
7777
): Promise<NotebookQueryResult> {
7878
const batchResults = new Map<number, NotebookBatchResult>();
7979
let canceled = false;
80+
// Tracks the most recently started batch so that messages with
81+
// batchId=-1 or undefined (e.g. parse/syntax errors) can be attached
82+
// to the batch they logically belong to.
83+
let lastStartedBatchId = -1;
8084

8185
// Promise that resolves when query/complete arrives
8286
const completion = new Deferred<QueryExecuteCompleteNotificationResult>();
@@ -87,6 +91,7 @@ export class NotebookQueryExecutor {
8791
},
8892
handleBatchStart(result: QueryExecuteBatchNotificationParams): void {
8993
const batch = result.batchSummary;
94+
lastStartedBatchId = batch.id;
9095
batchResults.set(batch.id, {
9196
batchSummary: batch,
9297
messages: [],
@@ -130,7 +135,15 @@ export class NotebookQueryExecutor {
130135
}
131136
},
132137
handleMessage(result: QueryExecuteMessageParams): void {
133-
const batchEntry = batchResults.get(result.message.batchId);
138+
const batchId = result.message.batchId;
139+
// batchId can be -1 or undefined for query-level messages such
140+
// as parse/syntax errors that aren't tied to a specific batch.
141+
// Fall back to the most recently started batch so these errors
142+
// are surfaced in cell output rather than silently dropped.
143+
const effectiveBatchId =
144+
batchId !== undefined && batchId >= 0 ? batchId : lastStartedBatchId;
145+
const batchEntry =
146+
effectiveBatchId >= 0 ? batchResults.get(effectiveBatchId) : undefined;
134147
if (batchEntry) {
135148
batchEntry.messages.push(result.message);
136149
}

‎extensions/mssql/src/notebooks/sqlNotebookController.ts‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -465,9 +465,16 @@ export class SqlNotebookController implements vscode.Disposable {
465465
execution.end(false, Date.now());
466466
activity.end(ActivityStatus.Canceled);
467467
} else {
468+
const hasErrors = result.batches.some(
469+
(b) => b.hasError || b.messages.some((m) => m.isError),
470+
);
468471
execution.replaceOutput(outputs);
469-
execution.end(true, Date.now());
470-
activity.end(ActivityStatus.Succeeded);
472+
execution.end(!hasErrors, Date.now());
473+
if (hasErrors) {
474+
activity.endFailed(new Error("Query returned errors"));
475+
} else {
476+
activity.end(ActivityStatus.Succeeded);
477+
}
471478
}
472479
} catch (err: any) {
473480
execution.replaceOutput([
@@ -494,18 +501,18 @@ export class SqlNotebookController implements vscode.Disposable {
494501
.filter((m) => m.isError)
495502
.map((m) => m.message);
496503

497-
if (batch.hasError && errorMessages.length > 0) {
504+
// Show error messages when present. We intentionally do NOT gate on
505+
// batch.hasError because STS can omit that flag for parse/syntax errors
506+
// while still sending error messages with isError=true.
507+
if (errorMessages.length > 0) {
498508
outputs.push(
499509
new vscode.NotebookCellOutput([
500-
vscode.NotebookCellOutputItem.text(
501-
LocalizedConstants.Notebooks.errorPrefix(errorMessages.join(os.EOL)),
502-
MIME_TEXT_PLAIN,
503-
),
510+
vscode.NotebookCellOutputItem.stderr(errorMessages.join(os.EOL)),
504511
]),
505512
);
506513
}
507514

508-
// Show non-error messages (PRINT, info, row counts) once before result sets
515+
// Show non-error messages (PRINT, info, row counts) before result sets
509516
if (messages.length > 0) {
510517
outputs.push(
511518
new vscode.NotebookCellOutput([
@@ -549,8 +556,13 @@ export class SqlNotebookController implements vscode.Disposable {
549556
);
550557
}
551558

552-
// If no result sets and no messages and no errors, show a generic success message
553-
if (batch.resultSets.length === 0 && messages.length === 0 && !batch.hasError) {
559+
// Show a generic success message only when the batch produced no
560+
// result sets, no informational messages, and no error messages.
561+
if (
562+
batch.resultSets.length === 0 &&
563+
messages.length === 0 &&
564+
errorMessages.length === 0
565+
) {
554566
outputs.push(
555567
new vscode.NotebookCellOutput([
556568
vscode.NotebookCellOutputItem.text(

‎extensions/mssql/test/unit/notebooks/notebookQueryExecutor.test.ts‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,106 @@ suite("NotebookQueryExecutor", () => {
306306
expect(result.batches[0].messages[0].isError).to.be.true;
307307
});
308308

309+
test("attaches messages with batchId=-1 to the last started batch", async () => {
310+
mockClient.sendRequest.callsFake(() => {
311+
capturedHandler.handleBatchStart({
312+
batchSummary: {
313+
id: 0,
314+
hasError: false,
315+
selection: { startLine: 0, startColumn: 0, endLine: 0, endColumn: 0 },
316+
resultSetSummaries: [],
317+
executionElapsed: "00:00:00.001",
318+
executionEnd: "",
319+
executionStart: "",
320+
},
321+
ownerUri: "test-uri",
322+
});
323+
// Syntax/parse errors can arrive with batchId=-1
324+
capturedHandler.handleMessage({
325+
message: {
326+
batchId: -1,
327+
isError: true,
328+
time: new Date().toISOString(),
329+
message: "Incorrect syntax near 'SELEC'.",
330+
},
331+
ownerUri: "test-uri",
332+
});
333+
capturedHandler.handleBatchComplete({
334+
batchSummary: {
335+
id: 0,
336+
hasError: false, // STS may send false even for parse errors
337+
selection: { startLine: 0, startColumn: 0, endLine: 0, endColumn: 0 },
338+
resultSetSummaries: [],
339+
executionElapsed: "00:00:00.001",
340+
executionEnd: "",
341+
executionStart: "",
342+
},
343+
ownerUri: "test-uri",
344+
});
345+
capturedHandler.handleQueryComplete({
346+
ownerUri: "test-uri",
347+
batchSummaries: [],
348+
});
349+
return Promise.resolve({});
350+
});
351+
352+
const result = await executor.execute("test-uri", "SELEC 1");
353+
354+
expect(result.batches).to.have.length(1);
355+
expect(result.batches[0].messages).to.have.length(1);
356+
expect(result.batches[0].messages[0].isError).to.be.true;
357+
expect(result.batches[0].messages[0].message).to.equal("Incorrect syntax near 'SELEC'.");
358+
});
359+
360+
test("attaches messages with undefined batchId to the last started batch", async () => {
361+
mockClient.sendRequest.callsFake(() => {
362+
capturedHandler.handleBatchStart({
363+
batchSummary: {
364+
id: 0,
365+
hasError: false,
366+
selection: { startLine: 0, startColumn: 0, endLine: 0, endColumn: 0 },
367+
resultSetSummaries: [],
368+
executionElapsed: "00:00:00.001",
369+
executionEnd: "",
370+
executionStart: "",
371+
},
372+
ownerUri: "test-uri",
373+
});
374+
capturedHandler.handleMessage({
375+
message: {
376+
batchId: undefined,
377+
isError: true,
378+
time: new Date().toISOString(),
379+
message: "Query-level error",
380+
},
381+
ownerUri: "test-uri",
382+
});
383+
capturedHandler.handleBatchComplete({
384+
batchSummary: {
385+
id: 0,
386+
hasError: false,
387+
selection: { startLine: 0, startColumn: 0, endLine: 0, endColumn: 0 },
388+
resultSetSummaries: [],
389+
executionElapsed: "00:00:00.001",
390+
executionEnd: "",
391+
executionStart: "",
392+
},
393+
ownerUri: "test-uri",
394+
});
395+
capturedHandler.handleQueryComplete({
396+
ownerUri: "test-uri",
397+
batchSummaries: [],
398+
});
399+
return Promise.resolve({});
400+
});
401+
402+
const result = await executor.execute("test-uri", "SELEC 1");
403+
404+
expect(result.batches).to.have.length(1);
405+
expect(result.batches[0].messages).to.have.length(1);
406+
expect(result.batches[0].messages[0].message).to.equal("Query-level error");
407+
});
408+
309409
test("handles multiple batches", async () => {
310410
mockClient.sendRequest.callsFake(() => {
311411
// First batch

‎extensions/mssql/test/unit/notebooks/sqlNotebookController.test.ts‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -413,6 +413,170 @@ suite("SqlNotebookController", () => {
413413
expect(mockExecution.end).to.have.been.calledWith(true, sinon.match.number);
414414
});
415415

416+
test("shows error message for batch with isError=true messages (regardless of batch.hasError)", async () => {
417+
mockNotebookConnMgr.executeQueryString.resolves(
418+
makeQueryResult({
419+
batches: [
420+
{
421+
batchSummary: makeBatchSummary({ hasError: false }), // hasError=false even though there's an error message
422+
messages: [
423+
{
424+
batchId: 0,
425+
isError: true,
426+
time: "",
427+
message: "Incorrect syntax near 'SELEC'.",
428+
},
429+
],
430+
resultSets: [],
431+
hasError: false,
432+
},
433+
],
434+
}),
435+
);
436+
437+
const notebook = makeNotebook([{ text: "SELEC 1" }]);
438+
const cells = notebook.getCells();
439+
440+
await mockController.executeHandler(cells, notebook, mockController);
441+
442+
const outputs = mockExecution.replaceOutput.firstCall.args[0];
443+
// Should have one error output, not "Commands completed successfully"
444+
expect(outputs).to.have.lengthOf(1);
445+
// Error output uses stderr mime type
446+
expect(outputs[0].items[0].mime).to.equal("application/vnd.code.notebook.stderr");
447+
const errorText = new TextDecoder().decode(outputs[0].items[0].data);
448+
expect(errorText).to.include("Incorrect syntax near 'SELEC'.");
449+
// Cell should be marked as failed
450+
expect(mockExecution.end).to.have.been.calledWith(false, sinon.match.number);
451+
});
452+
453+
test("does not show 'Commands completed successfully' when error messages exist", async () => {
454+
mockNotebookConnMgr.executeQueryString.resolves(
455+
makeQueryResult({
456+
batches: [
457+
{
458+
batchSummary: makeBatchSummary({ hasError: true }),
459+
messages: [
460+
{
461+
batchId: 0,
462+
isError: true,
463+
time: "",
464+
message: "Divide by zero error encountered.",
465+
},
466+
],
467+
resultSets: [],
468+
hasError: true,
469+
},
470+
],
471+
}),
472+
);
473+
474+
const notebook = makeNotebook([{ text: "SELECT 1/0" }]);
475+
const cells = notebook.getCells();
476+
477+
await mockController.executeHandler(cells, notebook, mockController);
478+
479+
const outputs = mockExecution.replaceOutput.firstCall.args[0];
480+
expect(outputs).to.have.lengthOf(1);
481+
const text = new TextDecoder().decode(outputs[0].items[0].data);
482+
expect(text).to.not.include("Command completed successfully");
483+
expect(text).to.include("Divide by zero error encountered.");
484+
});
485+
486+
test("shows PRINT messages as plain text output", async () => {
487+
mockNotebookConnMgr.executeQueryString.resolves(
488+
makeQueryResult({
489+
batches: [
490+
{
491+
batchSummary: makeBatchSummary(),
492+
messages: [
493+
{
494+
batchId: 0,
495+
isError: false,
496+
time: "",
497+
message: "hello world",
498+
},
499+
],
500+
resultSets: [],
501+
hasError: false,
502+
},
503+
],
504+
}),
505+
);
506+
507+
const notebook = makeNotebook([{ text: "PRINT 'hello world'" }]);
508+
const cells = notebook.getCells();
509+
510+
await mockController.executeHandler(cells, notebook, mockController);
511+
512+
const outputs = mockExecution.replaceOutput.firstCall.args[0];
513+
expect(outputs).to.have.lengthOf(1);
514+
expect(outputs[0].items[0].mime).to.equal("text/plain");
515+
const text = new TextDecoder().decode(outputs[0].items[0].data);
516+
expect(text).to.equal("hello world");
517+
expect(mockExecution.end).to.have.been.calledWith(true, sinon.match.number);
518+
});
519+
520+
test("multi-batch: shows grid, error, grid in order", async () => {
521+
mockNotebookConnMgr.executeQueryString.resolves(
522+
makeQueryResult({
523+
batches: [
524+
{
525+
batchSummary: makeBatchSummary({ id: 0 }),
526+
messages: [],
527+
resultSets: [
528+
{
529+
columnInfo: [makeColumn("n", "int")],
530+
rows: [[{ displayValue: "1", isNull: false }]],
531+
rowCount: 1,
532+
},
533+
],
534+
hasError: false,
535+
},
536+
{
537+
batchSummary: makeBatchSummary({ id: 1, hasError: true }),
538+
messages: [
539+
{
540+
batchId: 1,
541+
isError: true,
542+
time: "",
543+
message: "Divide by zero error encountered.",
544+
},
545+
],
546+
resultSets: [],
547+
hasError: true,
548+
},
549+
{
550+
batchSummary: makeBatchSummary({ id: 2 }),
551+
messages: [],
552+
resultSets: [
553+
{
554+
columnInfo: [makeColumn("n", "int")],
555+
rows: [[{ displayValue: "2", isNull: false }]],
556+
rowCount: 1,
557+
},
558+
],
559+
hasError: false,
560+
},
561+
],
562+
}),
563+
);
564+
565+
const notebook = makeNotebook([{ text: "SELECT 1\nGO\nSELECT 1/0\nGO\nSELECT 2" }]);
566+
const cells = notebook.getCells();
567+
568+
await mockController.executeHandler(cells, notebook, mockController);
569+
570+
const outputs = mockExecution.replaceOutput.firstCall.args[0];
571+
// Expected: result grid (batch 0), error message (batch 1), result grid (batch 2)
572+
expect(outputs).to.have.lengthOf(3);
573+
expect(outputs[0].items[0].mime).to.equal("application/vnd.mssql.query-result");
574+
expect(outputs[1].items[0].mime).to.equal("application/vnd.code.notebook.stderr");
575+
expect(outputs[2].items[0].mime).to.equal("application/vnd.mssql.query-result");
576+
// Cell should be marked failed because one batch had an error
577+
expect(mockExecution.end).to.have.been.calledWith(false, sinon.match.number);
578+
});
579+
416580
test("does not show truncation warning when result set is complete", async () => {
417581
mockNotebookConnMgr.executeQueryString.resolves(
418582
makeQueryResult({

0 commit comments

Comments
 (0)