Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion frontend/src/api/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ export async function executeTool(

const rows = toolResult.data.rows;
if (rows.length === 0) {
return { columns: [], rows: [], rowCount: 0 };
// For INSERT/UPDATE/DELETE, rows is empty but count reflects affected rows
return { columns: [], rows: [], rowCount: toolResult.data.count };
}

const columns = Object.keys(rows[0]);
Expand Down
8 changes: 6 additions & 2 deletions frontend/src/components/tool/ResultsTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,15 @@ export function ResultsTable({ result, error, isLoading, executedSql, executionT
);
}

// No results
// No rows returned - could be empty SELECT or successful INSERT/UPDATE/DELETE
if (result.rows.length === 0) {
return (
<div className="border border-border rounded-lg bg-card p-8 text-center">
<p className="text-muted-foreground text-sm">No results returned</p>
<p className="text-muted-foreground text-sm">
{result.rowCount > 0
? `${result.rowCount} row${result.rowCount !== 1 ? 's' : ''} affected`
: 'No results returned'}
</p>
</div>
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/connectors/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export type ConnectorType = "postgres" | "mysql" | "mariadb" | "sqlite" | "sqlse
*/
export interface SQLResult {
rows: any[];
[key: string]: any;
rowCount: number;
}

export interface TableColumn {
Expand Down
5 changes: 3 additions & 2 deletions src/connectors/mariadb/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
import { SafeURL } from "../../utils/safe-url.js";
import { obfuscateDSNPassword } from "../../utils/dsn-obfuscate.js";
import { SQLRowLimiter } from "../../utils/sql-row-limiter.js";
import { parseQueryResults } from "../../utils/multi-statement-result-parser.js";
import { parseQueryResults, extractAffectedRows } from "../../utils/multi-statement-result-parser.js";

/**
* MariaDB DSN Parser
Expand Down Expand Up @@ -542,7 +542,8 @@ export class MariaDBConnector implements Connector {

// Parse results using shared utility that handles both single and multi-statement queries
const rows = parseQueryResults(results);
return { rows };
const rowCount = extractAffectedRows(results);
return { rows, rowCount };
} catch (error) {
console.error("Error executing query:", error);
throw error;
Expand Down
5 changes: 3 additions & 2 deletions src/connectors/mysql/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
import { SafeURL } from "../../utils/safe-url.js";
import { obfuscateDSNPassword } from "../../utils/dsn-obfuscate.js";
import { SQLRowLimiter } from "../../utils/sql-row-limiter.js";
import { parseQueryResults } from "../../utils/multi-statement-result-parser.js";
import { parseQueryResults, extractAffectedRows } from "../../utils/multi-statement-result-parser.js";

/**
* MySQL DSN Parser
Expand Down Expand Up @@ -553,7 +553,8 @@ export class MySQLConnector implements Connector {

// Parse results using shared utility that handles both single and multi-statement queries
const rows = parseQueryResults(firstResult);
return { rows };
const rowCount = extractAffectedRows(firstResult);
return { rows, rowCount };
} catch (error) {
console.error("Error executing query:", error);
throw error;
Expand Down
15 changes: 12 additions & 3 deletions src/connectors/postgres/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -444,17 +444,21 @@ export class PostgresConnector implements Connector {
const processedStatement = SQLRowLimiter.applyMaxRows(statements[0], options.maxRows);

// Use parameters if provided
let result;
if (parameters && parameters.length > 0) {
try {
return await client.query(processedStatement, parameters);
result = await client.query(processedStatement, parameters);
} catch (error) {
console.error(`[PostgreSQL executeSQL] ERROR: ${(error as Error).message}`);
console.error(`[PostgreSQL executeSQL] SQL: ${processedStatement}`);
console.error(`[PostgreSQL executeSQL] Parameters: ${JSON.stringify(parameters)}`);
throw error;
}
} else {
result = await client.query(processedStatement);
}
return await client.query(processedStatement);
// Explicitly return rows and rowCount to ensure rowCount is preserved
return { rows: result.rows, rowCount: result.rowCount ?? result.rows.length };
} else {
// Multiple statements - parameters not supported for multi-statement queries
if (parameters && parameters.length > 0) {
Expand All @@ -463,6 +467,7 @@ export class PostgresConnector implements Connector {

// Execute all in same session for transaction consistency
let allRows: any[] = [];
let totalRowCount = 0;

// Execute within a transaction to ensure session consistency
await client.query('BEGIN');
Expand All @@ -476,14 +481,18 @@ export class PostgresConnector implements Connector {
if (result.rows && result.rows.length > 0) {
allRows.push(...result.rows);
}
// Accumulate rowCount for INSERT/UPDATE/DELETE statements
if (result.rowCount) {
totalRowCount += result.rowCount;
}
}
await client.query('COMMIT');
} catch (error) {
await client.query('ROLLBACK');
throw error;
}

return { rows: allRows };
return { rows: allRows, rowCount: totalRowCount };
}
} finally {
client.release();
Expand Down
22 changes: 13 additions & 9 deletions src/connectors/sqlite/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ export class SQLiteConnector implements Connector {
if (parameters && parameters.length > 0) {
try {
const rows = this.db.prepare(processedStatement).all(...parameters);
return { rows };
return { rows, rowCount: rows.length };
} catch (error) {
console.error(`[SQLite executeSQL] ERROR: ${(error as Error).message}`);
console.error(`[SQLite executeSQL] SQL: ${processedStatement}`);
Expand All @@ -422,23 +422,24 @@ export class SQLiteConnector implements Connector {
}
} else {
const rows = this.db.prepare(processedStatement).all();
return { rows };
return { rows, rowCount: rows.length };
}
} else {
// Use run() for statements that don't return data
let result;
if (parameters && parameters.length > 0) {
try {
this.db.prepare(processedStatement).run(...parameters);
result = this.db.prepare(processedStatement).run(...parameters);
} catch (error) {
console.error(`[SQLite executeSQL] ERROR: ${(error as Error).message}`);
console.error(`[SQLite executeSQL] SQL: ${processedStatement}`);
console.error(`[SQLite executeSQL] Parameters: ${JSON.stringify(parameters)}`);
throw error;
}
} else {
this.db.prepare(processedStatement).run();
result = this.db.prepare(processedStatement).run();
}
return { rows: [] };
return { rows: [], rowCount: result.changes };
}
} else {
// Multiple statements - parameters not supported for multi-statement queries
Expand Down Expand Up @@ -469,9 +470,11 @@ export class SQLiteConnector implements Connector {
}
}

// Execute write statements using native .exec() for optimal performance
if (writeStatements.length > 0) {
this.db.exec(writeStatements.join('; '));
// Execute write statements individually to track changes
let totalChanges = 0;
for (const statement of writeStatements) {
const result = this.db.prepare(statement).run();
totalChanges += result.changes;
}

// Execute read statements individually to collect results
Expand All @@ -483,7 +486,8 @@ export class SQLiteConnector implements Connector {
allRows.push(...result);
}

return { rows: allRows };
// rowCount is total changes for writes, plus rows returned for reads
return { rows: allRows, rowCount: totalChanges + allRows.length };
Comment on lines +489 to +490
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rowCount calculation for multi-statement queries is inconsistent with single-statement behavior. For single statements, rowCount represents either affected rows (for writes) OR returned rows (for reads), but not both. However, here it sums totalChanges (affected rows from writes) and allRows.length (returned rows from reads), mixing two different semantics.

This inconsistency could be confusing when displaying "N rows affected" in the frontend, as it would show a misleading count for mixed queries. Consider either:

  1. Only counting affected rows from write operations (totalChanges)
  2. Only counting returned rows from read operations (allRows.length)
  3. Documenting that multi-statement queries return a combined count
Suggested change
// rowCount is total changes for writes, plus rows returned for reads
return { rows: allRows, rowCount: totalChanges + allRows.length };
// For multi-statement queries, align rowCount semantics with single-statement behavior:
// - If the batch is read-only, rowCount is the number of rows returned.
// - If the batch includes any writes, rowCount is the total number of rows affected.
const hasReads = readStatements.length > 0;
const hasWrites = writeStatements.length > 0;
const rowCount = hasWrites ? totalChanges : allRows.length;
return { rows: allRows, rowCount };

Copilot uses AI. Check for mistakes.
}
} catch (error) {
throw error;
Expand Down
6 changes: 0 additions & 6 deletions src/connectors/sqlserver/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -556,12 +556,6 @@ export class SQLServerConnector implements Connector {

return {
rows: result.recordset || [],
fields:
result.recordset && result.recordset.length > 0
? Object.keys(result.recordset[0]).map((key) => ({
name: key,
}))
: [],
rowCount: result.rowsAffected[0] || 0,
};
} catch (error) {
Expand Down
14 changes: 7 additions & 7 deletions src/tools/__tests__/execute-sql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('execute-sql tool', () => {

describe('basic execution', () => {
it('should execute SELECT and return rows', async () => {
const mockResult: SQLResult = { rows: [{ id: 1, name: 'test' }] };
const mockResult: SQLResult = { rows: [{ id: 1, name: 'test' }], rowCount: 1 };
vi.mocked(mockConnector.executeSQL).mockResolvedValue(mockResult);

const handler = createExecuteSqlToolHandler('test_source');
Expand All @@ -67,7 +67,7 @@ describe('execute-sql tool', () => {
});

it('should pass multi-statement SQL directly to connector', async () => {
const mockResult: SQLResult = { rows: [{ id: 1 }] };
const mockResult: SQLResult = { rows: [{ id: 1 }], rowCount: 1 };
vi.mocked(mockConnector.executeSQL).mockResolvedValue(mockResult);

const sql = 'SELECT * FROM users; SELECT * FROM roles;';
Expand Down Expand Up @@ -102,7 +102,7 @@ describe('execute-sql tool', () => {
});

it('should allow SELECT statements', async () => {
const mockResult: SQLResult = { rows: [{ id: 1 }] };
const mockResult: SQLResult = { rows: [{ id: 1 }], rowCount: 1 };
vi.mocked(mockConnector.executeSQL).mockResolvedValue(mockResult);

const handler = createExecuteSqlToolHandler('test_source');
Expand All @@ -114,7 +114,7 @@ describe('execute-sql tool', () => {
});

it('should allow multiple read-only statements', async () => {
const mockResult: SQLResult = { rows: [] };
const mockResult: SQLResult = { rows: [], rowCount: 0 };
vi.mocked(mockConnector.executeSQL).mockResolvedValue(mockResult);

const sql = 'SELECT * FROM users; SELECT * FROM roles;';
Expand Down Expand Up @@ -173,7 +173,7 @@ describe('execute-sql tool', () => {
mockGetToolRegistry.mockReturnValue({
getBuiltinToolConfig: vi.fn().mockReturnValue(toolConfig),
} as any);
const mockResult: SQLResult = { rows: [] };
const mockResult: SQLResult = { rows: [], rowCount: 0 };
vi.mocked(mockConnector.executeSQL).mockResolvedValue(mockResult);

const handler = createExecuteSqlToolHandler('writable_source');
Expand Down Expand Up @@ -208,7 +208,7 @@ describe('execute-sql tool', () => {
['inline comments', 'SELECT id, -- user id\n name FROM users'],
['only comments', '-- Just a comment\n/* Another */'],
])('should allow SELECT with %s', async (_, sql) => {
const mockResult: SQLResult = { rows: [] };
const mockResult: SQLResult = { rows: [], rowCount: 0 };
vi.mocked(mockConnector.executeSQL).mockResolvedValue(mockResult);

const handler = createExecuteSqlToolHandler('test_source');
Expand All @@ -231,7 +231,7 @@ describe('execute-sql tool', () => {
['empty string', ''],
['only semicolons and whitespace', ' ; ; ; '],
])('should handle %s', async (_, sql) => {
const mockResult: SQLResult = { rows: [] };
const mockResult: SQLResult = { rows: [], rowCount: 0 };
vi.mocked(mockConnector.executeSQL).mockResolvedValue(mockResult);

const handler = createExecuteSqlToolHandler('test_source');
Expand Down
2 changes: 1 addition & 1 deletion src/tools/custom-tool-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ export function createCustomToolHandler(toolConfig: ToolConfig) {
// 7. Build response data
const responseData = {
rows: result.rows,
count: result.rows.length,
count: result.rowCount,
source_id: toolConfig.source,
};

Expand Down
2 changes: 1 addition & 1 deletion src/tools/execute-sql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export function createExecuteSqlToolHandler(sourceId?: string) {
// Build response data
const responseData = {
rows: result.rows,
count: result.rows.length,
count: result.rowCount,
source_id: effectiveSourceId,
};

Expand Down
37 changes: 37 additions & 0 deletions src/utils/multi-statement-result-parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,43 @@ export function extractRowsFromMultiStatement(results: any): any[] {
return allRows;
}

/**
* Extracts total affected rows from query results.
*
* For INSERT/UPDATE/DELETE operations, returns the sum of affectedRows.
* For SELECT operations, returns the number of rows.
*
* @param results - Raw results from the database driver
* @returns Total number of affected/returned rows
*/
export function extractAffectedRows(results: any): number {
// Handle metadata object (single INSERT/UPDATE/DELETE)
if (isMetadataObject(results)) {
return results.affectedRows || 0;
}

// Handle non-array results
if (!Array.isArray(results)) {
return 0;
}

// Check if this is a multi-statement result
if (isMultiStatementResult(results)) {
let totalAffected = 0;
for (const result of results) {
if (isMetadataObject(result)) {
totalAffected += result.affectedRows || 0;
} else if (Array.isArray(result)) {
totalAffected += result.length;
}
}
return totalAffected;
Comment on lines +92 to +101
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extractAffectedRows function mixes two different semantics in multi-statement queries. For metadata objects (write operations), it counts affectedRows, but for array results (SELECT queries), it counts result.length (returned rows). This means a multi-statement query with both writes and reads will return a count that combines affected rows and returned rows, which is semantically inconsistent.

For example, "INSERT INTO users VALUES (1); SELECT * FROM users;" would return rowCount = 1 (affected) + row count from SELECT. This makes it unclear what "N rows affected" means in the UI.

Consider whether rowCount should represent:

  1. Only affected rows from write operations (skip SELECT results)
  2. Only returned rows from read operations (skip write metadata)
  3. A documented combined count with clear semantics

Copilot uses AI. Check for mistakes.
}

// Single statement result - results is the rows array directly
return results.length;
}

/**
* Parses database query results, handling both single and multi-statement queries.
*
Expand Down