Skip to content

Commit 233125b

Browse files
authored
feat(connectors)!: return per-statement result sets, wire up frontend tabs (#389)
* fix(sqlserver): return every result set from multi-statement batches executeSQL and executeReadOnly read result.recordset (singular), which node-mssql defines as only the first statement's result set. A batch like "SELECT 1 AS a; SELECT 2 AS b" silently dropped the second SELECT, unlike MySQL/MariaDB which already concatenate all statements' rows. Read result.recordsets (plural) instead, flattening every SELECT's rows in order, and sum rowsAffected across all statements so rowCount stays consistent with rows rather than reporting only the first statement's count. Closes #380 * fix(sqlserver): use nullish coalescing in recordsets/rowsAffected helpers Addresses Copilot review feedback on #389: || masked a genuine 0 identically to a missing value, which was accidental rather than intentional. Deliberately not adding Array.isArray guards — mssql's IResult<T> type guarantees recordsets: Array<IRecordSet> and rowsAffected: number[], the same contract extractPlanXml already trusts without validation. * feat(connectors)!: return per-statement result sets instead of merging them SQLResult.rows/rowCount flattened every statement in a batch into one array/count, so a caller had no way to tell "SELECT 1 AS a; SELECT 2 AS b" apart from a single query that happened to return two rows - the flattening the SQL Server fix (#389) added for MySQL/MariaDB-parity was itself the wrong shape to standardize on. SQLResult is now `{ resultSets: SQLResultSet[], messages? }`, one entry per statement in execution order. execute_sql, explain_sql, and custom tools already read connector-provided rows through this shape; their JSON output changes to `resultSets: [{rows, count}, ...]` instead of flat `rows`/`count` (except explain_sql, which only ever executes one statement and keeps returning the flat shape internally). Per connector: - postgres/sqlite: already looped per-statement for multi-statement batches: now push one result set per statement instead of merging. - mysql/mariadb: multi-statement-result-parser.ts now builds a SQLResultSet per statement (parseQueryResultSets) instead of concatenating rows across statements. - sqlserver: recordsets/rowsAffected aren't index-aligned per statement in node-mssql's API (see buildResultSets' comment), so a batch mixing writes with selects gets one result set per SELECT plus a trailing result set summarizing the write-only statements - not fully per-statement, but no read statement's rows are ever merged with another's. BREAKING CHANGE: execute_sql/custom tool JSON responses now nest rows under `resultSets` instead of returning them at the top level. * feat: rename resultSets to statements, add per-statement sql, wire up frontend tabs The prior commit introduced SQLResult.resultSets but named the tool-facing JSON key the same, and never attributed a statement's source text to its result - both of which broke the local web frontend's query editor (frontend/src/api/tools.ts), a real, shipped consumer of execute_sql that reads the tool response directly. It was silently going to return empty results for every query. - SQLResultSet gains an optional `sql` field: the statement that produced it, when a connector can attribute it unambiguously. postgres/sqlite/ mysql/mariadb populate it for every statement (they process statements in a way that preserves reliable order/count alignment); SQL Server only populates it for an unambiguous single-statement batch, since recordsets/ rowsAffected aren't index-aligned per statement there (same limitation buildResultSets already documented). - execute_sql/custom-tool JSON responses now key their statement array as `statements` (previously `resultSets`), each entry `{sql, rows, count}`. - Rebuilt the frontend's tab model around this: executeTool now returns one QueryResult per statement instead of a single merged result, and ToolDetailView creates one ResultTab per statement (labeled "(i/N)" for batches of more than one) instead of forcing a whole batch into one tab. - Also fixed an unrelated pre-existing bug this surfaced while verifying in a browser: frontend/src/api/tools.ts called response.json() directly, but the backend's stateless HTTP transport answers SSE-framed responses (event:/data: lines) for this request shape - every query through the web UI over HTTP transport was silently broken before this fix too. Verified end-to-end in a browser against the --demo SQLite backend: a two-statement batch produces two correctly-labeled, independently scrollable tabs, each showing its own statement's SQL and rows; a single-statement query still shows one untabbed result as before. * refactor: dedupe resultSets->statements mapping, avoid re-parsing SQL in buildResultSets - Extract toStatementsPayload() to tool-handler-helpers.ts, used by both execute-sql.ts and custom-tool-handler.ts instead of each inlining the same resultSets.map(...) - keeps their output contracts from silently diverging if the shape changes later. - SQLServerConnector.buildResultSets no longer re-parses the source SQL internally just to gate the `sql` attribution. executeSQL now computes isSingleStatement once and threads it through (directly, or via executeReadOnly's new parameter) instead of calling splitSQLStatements a second time on every query.
1 parent 1e1b139 commit 233125b

28 files changed

Lines changed: 832 additions & 558 deletions

frontend/src/api/tools.ts

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { ApiError } from './errors';
22

33
export interface QueryResult {
4+
/** Source text of the statement that produced this result, when known. */
5+
sql?: string;
46
columns: string[];
57
rows: any[][];
68
rowCount: number;
@@ -21,17 +23,37 @@ interface McpResponse {
2123
interface ToolResultData {
2224
success: boolean;
2325
data: {
24-
rows: Record<string, any>[];
25-
count: number;
26+
statements: Array<{
27+
sql?: string;
28+
rows: Record<string, any>[];
29+
count: number;
30+
}>;
2631
source_id: string;
2732
} | null;
2833
error: string | null;
2934
}
3035

36+
function toQueryResult(statement: { sql?: string; rows: Record<string, any>[]; count: number }): QueryResult {
37+
if (statement.rows.length === 0) {
38+
// For INSERT/UPDATE/DELETE, rows is empty but count reflects affected rows
39+
return { sql: statement.sql, columns: [], rows: [], rowCount: statement.count };
40+
}
41+
42+
const columns = Object.keys(statement.rows[0]);
43+
const rowArrays = statement.rows.map((row) => columns.map((col) => row[col]));
44+
45+
return { sql: statement.sql, columns, rows: rowArrays, rowCount: statement.count };
46+
}
47+
48+
/**
49+
* Executes a tool and returns one `QueryResult` per statement in the batch -
50+
* a single SELECT (the common case) is an array of length 1, rather than
51+
* different statements' rows being merged together.
52+
*/
3153
export async function executeTool(
3254
toolName: string,
3355
args: Record<string, any>
34-
): Promise<QueryResult> {
56+
): Promise<QueryResult[]> {
3557
const response = await fetch('/mcp', {
3658
method: 'POST',
3759
headers: {
@@ -53,7 +75,20 @@ export async function executeTool(
5375
throw new ApiError(`HTTP error: ${response.status}`, response.status);
5476
}
5577

56-
const mcpResponse: McpResponse = await response.json();
78+
// The stateless legacy path (2025-era MCP clients) answers spec-standard
79+
// SSE framing - a single "data: {...}" event per exchange - rather than a
80+
// plain JSON body, even though this fetch requests both content types.
81+
const text = await response.text();
82+
let mcpResponse: McpResponse;
83+
if (response.headers.get('content-type')?.includes('text/event-stream')) {
84+
const dataLine = text.split('\n').find((line) => line.startsWith('data: '));
85+
if (!dataLine) {
86+
throw new ApiError('No data event in SSE response', 500);
87+
}
88+
mcpResponse = JSON.parse(dataLine.slice('data: '.length));
89+
} else {
90+
mcpResponse = JSON.parse(text);
91+
}
5792

5893
if (mcpResponse.error) {
5994
throw new ApiError(mcpResponse.error.message, mcpResponse.error.code);
@@ -69,22 +104,9 @@ export async function executeTool(
69104
throw new ApiError(toolResult.error || 'Tool execution failed', 500);
70105
}
71106

72-
if (!toolResult.data || !toolResult.data.rows) {
73-
return { columns: [], rows: [], rowCount: 0 };
74-
}
75-
76-
const rows = toolResult.data.rows;
77-
if (rows.length === 0) {
78-
// For INSERT/UPDATE/DELETE, rows is empty but count reflects affected rows
79-
return { columns: [], rows: [], rowCount: toolResult.data.count };
107+
if (!toolResult.data || !toolResult.data.statements) {
108+
return [{ columns: [], rows: [], rowCount: 0 }];
80109
}
81110

82-
const columns = Object.keys(rows[0]);
83-
const rowArrays = rows.map((row) => columns.map((col) => row[col]));
84-
85-
return {
86-
columns,
87-
rows: rowArrays,
88-
rowCount: toolResult.data.count,
89-
};
111+
return toolResult.data.statements.map(toQueryResult);
90112
}

frontend/src/components/tool/ResultsTabs.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,14 @@ function formatTimestamp(date: Date): string {
2222
});
2323
}
2424

25+
function formatTabLabel(tab: ResultTab): string {
26+
const time = formatTimestamp(tab.timestamp);
27+
if (tab.statementTotal && tab.statementTotal > 1) {
28+
return `${time} (${tab.statementIndex}/${tab.statementTotal})`;
29+
}
30+
return time;
31+
}
32+
2533
export function ResultsTabs({
2634
tabs,
2735
activeTabId,
@@ -123,7 +131,7 @@ export function ResultsTabs({
123131
: 'border-transparent text-muted-foreground hover:text-foreground'
124132
)}
125133
>
126-
<span>{formatTimestamp(tab.timestamp)}</span>
134+
<span>{formatTabLabel(tab)}</span>
127135
{tab.error && (
128136
<span className="w-1.5 h-1.5 rounded-full bg-destructive" aria-label="Error" />
129137
)}

frontend/src/components/tool/types.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,8 @@ export interface ResultTab {
77
error: string | null;
88
executedSql: string;
99
executionTimeMs: number;
10+
/** 1-based position of this statement within its batch, when the batch had more than one. */
11+
statementIndex?: number;
12+
/** Total number of statements in the batch this tab's statement belongs to. */
13+
statementTotal?: number;
1014
}

frontend/src/components/views/ToolDetailView.tsx

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -209,31 +209,39 @@ export default function ToolDetailView() {
209209
const startTime = performance.now();
210210

211211
try {
212-
let queryResult: QueryResult;
212+
let queryResults: QueryResult[];
213213
let sqlToExecute: string;
214214

215215
if (toolType === 'execute_sql') {
216216
// Get selected SQL from editor (returns selection if any, otherwise full content)
217217
sqlToExecute = sqlEditorRef.current?.getSelectedSql() ?? sql;
218-
queryResult = await executeTool(toolName, { sql: sqlToExecute });
218+
queryResults = await executeTool(toolName, { sql: sqlToExecute });
219219
} else {
220220
sqlToExecute = getSqlPreview();
221-
queryResult = await executeTool(toolName, params);
221+
queryResults = await executeTool(toolName, params);
222222
}
223223

224224
const endTime = performance.now();
225225
const duration = endTime - startTime;
226+
const timestamp = new Date();
227+
const isBatch = queryResults.length > 1;
226228

227-
const newTab: ResultTab = {
229+
// One tab per statement, so a multi-statement batch's results don't get
230+
// collapsed into a single tab - only the first tab's time reflects the
231+
// whole batch's duration, since the others didn't wait separately.
232+
const newTabs: ResultTab[] = queryResults.map((result, index) => ({
228233
id: crypto.randomUUID(),
229-
timestamp: new Date(),
230-
result: queryResult,
234+
// Offset timestamps so tabs from the same run sort stably and get distinct ids/keys.
235+
timestamp: new Date(timestamp.getTime() + index),
236+
result,
231237
error: null,
232-
executedSql: sqlToExecute,
233-
executionTimeMs: duration,
234-
};
235-
setResultTabs(prev => [newTab, ...prev]);
236-
setActiveTabId(newTab.id);
238+
executedSql: result.sql ?? sqlToExecute,
239+
executionTimeMs: index === 0 ? duration : 0,
240+
statementIndex: isBatch ? index + 1 : undefined,
241+
statementTotal: isBatch ? queryResults.length : undefined,
242+
}));
243+
setResultTabs(prev => [...newTabs, ...prev]);
244+
setActiveTabId(newTabs[0].id);
237245
} catch (err) {
238246
const errorTab: ResultTab = {
239247
id: crypto.randomUUID(),

src/__tests__/json-rpc-integration.test.ts

Lines changed: 47 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -208,11 +208,13 @@ describe('JSON RPC Integration Tests', () => {
208208

209209
const content = JSON.parse(response.result.content[0].text);
210210
expect(content.success).toBe(true);
211-
expect(content.data).toHaveProperty('rows');
212-
expect(content.data).toHaveProperty('count');
213-
expect(content.data.rows).toHaveLength(2);
214-
expect(content.data.rows[0].name).toBe('John Doe');
215-
expect(content.data.rows[1].name).toBe('Bob Johnson');
211+
expect(content.data).toHaveProperty('statements');
212+
expect(content.data.statements).toHaveLength(1);
213+
expect(content.data.statements[0]).toHaveProperty('rows');
214+
expect(content.data.statements[0]).toHaveProperty('count');
215+
expect(content.data.statements[0].rows).toHaveLength(2);
216+
expect(content.data.statements[0].rows[0].name).toBe('John Doe');
217+
expect(content.data.statements[0].rows[1].name).toBe('Bob Johnson');
216218
});
217219

218220
it('should execute a JOIN query successfully', async () => {
@@ -229,9 +231,10 @@ describe('JSON RPC Integration Tests', () => {
229231
expect(response).toHaveProperty('result');
230232
const content = JSON.parse(response.result.content[0].text);
231233
expect(content.success).toBe(true);
232-
expect(content.data.rows).toHaveLength(2);
233-
expect(content.data.rows[0].total).toBe(149.50);
234-
expect(content.data.rows[1].total).toBe(99.99);
234+
expect(content.data.statements).toHaveLength(1);
235+
expect(content.data.statements[0].rows).toHaveLength(2);
236+
expect(content.data.statements[0].rows[0].total).toBe(149.50);
237+
expect(content.data.statements[0].rows[1].total).toBe(99.99);
235238
});
236239

237240
it('should execute aggregate queries successfully', async () => {
@@ -249,11 +252,12 @@ describe('JSON RPC Integration Tests', () => {
249252
expect(response).toHaveProperty('result');
250253
const content = JSON.parse(response.result.content[0].text);
251254
expect(content.success).toBe(true);
252-
expect(content.data.rows).toHaveLength(1);
253-
expect(content.data.rows[0].user_count).toBe(3);
254-
expect(content.data.rows[0].avg_age).toBe(30);
255-
expect(content.data.rows[0].min_age).toBe(25);
256-
expect(content.data.rows[0].max_age).toBe(35);
255+
expect(content.data.statements).toHaveLength(1);
256+
expect(content.data.statements[0].rows).toHaveLength(1);
257+
expect(content.data.statements[0].rows[0].user_count).toBe(3);
258+
expect(content.data.statements[0].rows[0].avg_age).toBe(30);
259+
expect(content.data.statements[0].rows[0].min_age).toBe(25);
260+
expect(content.data.statements[0].rows[0].max_age).toBe(35);
257261
});
258262

259263
it('should handle multiple statements in a single call', async () => {
@@ -267,8 +271,17 @@ describe('JSON RPC Integration Tests', () => {
267271
expect(response).toHaveProperty('result');
268272
const content = JSON.parse(response.result.content[0].text);
269273
expect(content.success).toBe(true);
270-
expect(content.data.rows).toHaveLength(1);
271-
expect(content.data.rows[0].total_users).toBe(4);
274+
// SQLite runs all write statements before read statements, so the
275+
// INSERT's entry comes first even though it's first in source order
276+
// too here; the SELECT's entry follows.
277+
expect(content.data.statements).toHaveLength(2);
278+
expect(content.data.statements[0]).toEqual({
279+
sql: "INSERT INTO users (name, email, age) VALUES ('Test User', 'test@example.com', 28)",
280+
rows: [],
281+
count: 1,
282+
});
283+
expect(content.data.statements[1].rows).toHaveLength(1);
284+
expect(content.data.statements[1].rows[0].total_users).toBe(4);
272285
});
273286

274287
it('should handle SQLite-specific functions', async () => {
@@ -285,10 +298,11 @@ describe('JSON RPC Integration Tests', () => {
285298
expect(response).toHaveProperty('result');
286299
const content = JSON.parse(response.result.content[0].text);
287300
expect(content.success).toBe(true);
288-
expect(content.data.rows).toHaveLength(1);
289-
expect(content.data.rows[0].version).toBeDefined();
290-
expect(content.data.rows[0].uppercase).toBe('HELLO WORLD');
291-
expect(content.data.rows[0].str_length).toBe(11);
301+
expect(content.data.statements).toHaveLength(1);
302+
expect(content.data.statements[0].rows).toHaveLength(1);
303+
expect(content.data.statements[0].rows[0].version).toBeDefined();
304+
expect(content.data.statements[0].rows[0].uppercase).toBe('HELLO WORLD');
305+
expect(content.data.statements[0].rows[0].str_length).toBe(11);
292306
});
293307

294308
it('should return error for invalid SQL', async () => {
@@ -311,8 +325,9 @@ describe('JSON RPC Integration Tests', () => {
311325
expect(response).toHaveProperty('result');
312326
const content = JSON.parse(response.result.content[0].text);
313327
expect(content.success).toBe(true);
314-
expect(content.data.rows).toHaveLength(0);
315-
expect(content.data.count).toBe(0);
328+
expect(content.data.statements).toHaveLength(1);
329+
expect(content.data.statements[0].rows).toHaveLength(0);
330+
expect(content.data.statements[0].count).toBe(0);
316331
});
317332

318333
it('should work with SQLite transactions', async () => {
@@ -328,9 +343,13 @@ describe('JSON RPC Integration Tests', () => {
328343
expect(response).toHaveProperty('result');
329344
const content = JSON.parse(response.result.content[0].text);
330345
expect(content.success).toBe(true);
331-
expect(content.data.rows).toHaveLength(1);
332-
expect(content.data.rows[0].name).toBe('Transaction User');
333-
expect(content.data.rows[0].age).toBe(40);
346+
// SQLite runs all write statements (BEGIN TRANSACTION, INSERT, COMMIT)
347+
// before the read statement (SELECT), so the SELECT's resultSet is last.
348+
expect(content.data.statements).toHaveLength(4);
349+
const selectSet = content.data.statements[content.data.statements.length - 1];
350+
expect(selectSet.rows).toHaveLength(1);
351+
expect(selectSet.rows[0].name).toBe('Transaction User');
352+
expect(selectSet.rows[0].age).toBe(40);
334353
});
335354

336355
it('should handle PRAGMA statements', async () => {
@@ -341,9 +360,10 @@ describe('JSON RPC Integration Tests', () => {
341360
expect(response).toHaveProperty('result');
342361
const content = JSON.parse(response.result.content[0].text);
343362
expect(content.success).toBe(true);
344-
expect(content.data.rows.length).toBeGreaterThan(0);
345-
expect(content.data.rows.some((row: any) => row.name === 'id')).toBe(true);
346-
expect(content.data.rows.some((row: any) => row.name === 'name')).toBe(true);
363+
expect(content.data.statements).toHaveLength(1);
364+
expect(content.data.statements[0].rows.length).toBeGreaterThan(0);
365+
expect(content.data.statements[0].rows.some((row: any) => row.name === 'id')).toBe(true);
366+
expect(content.data.statements[0].rows.some((row: any) => row.name === 'name')).toBe(true);
347367
});
348368
});
349369

0 commit comments

Comments
 (0)