Skip to content

Commit 46e803d

Browse files
tianzhouclaude
andcommitted
fix: surface rollback failure in executeReadOnly when query succeeds
Track whether the query itself failed so the finally block can distinguish a rollback error after a successful query (dangerous — data may have persisted) from one after a query error (safe to suppress so the original error propagates). Also removes the redundant outer try block. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent ad418f9 commit 46e803d

1 file changed

Lines changed: 56 additions & 51 deletions

File tree

src/connectors/sqlserver/index.ts

Lines changed: 56 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -733,67 +733,72 @@ export class SQLServerConnector implements Connector {
733733

734734
const transaction = new sql.Transaction(this.connection!);
735735
await transaction.begin();
736-
try {
737-
const request = new sql.Request(transaction);
738-
const messages: DatabaseMessage[] = [];
739-
request.on(
740-
'info',
741-
(info: { message: string; number?: number; class?: number; lineNumber?: number }) => {
742-
messages.push({
743-
text: info.message,
744-
severity: info.class !== undefined ? String(info.class) : undefined,
745-
code: info.number,
746-
line: info.lineNumber,
747-
});
748-
},
749-
);
750736

751-
if (parameters && parameters.length > 0) {
752-
parameters.forEach((param, index) => {
753-
const paramName = `p${index + 1}`;
754-
if (typeof param === 'string') {
755-
request.input(paramName, sql.VarChar, param);
756-
} else if (typeof param === 'number') {
757-
if (Number.isInteger(param)) {
758-
request.input(paramName, sql.Int, param);
759-
} else {
760-
request.input(paramName, sql.Float, param);
761-
}
762-
} else if (typeof param === 'boolean') {
763-
request.input(paramName, sql.Bit, param);
764-
} else if (param === null || param === undefined) {
765-
request.input(paramName, sql.VarChar, param);
766-
} else if (Array.isArray(param)) {
767-
request.input(paramName, sql.VarChar, JSON.stringify(param));
768-
} else {
769-
request.input(paramName, sql.VarChar, JSON.stringify(param));
770-
}
737+
const request = new sql.Request(transaction);
738+
const messages: DatabaseMessage[] = [];
739+
request.on(
740+
'info',
741+
(info: { message: string; number?: number; class?: number; lineNumber?: number }) => {
742+
messages.push({
743+
text: info.message,
744+
severity: info.class !== undefined ? String(info.class) : undefined,
745+
code: info.number,
746+
line: info.lineNumber,
771747
});
772-
}
748+
},
749+
);
773750

774-
let result;
775-
try {
776-
result = await request.query(processedSQL);
777-
} catch (error) {
778-
console.error(`[SQL Server executeReadOnly] ERROR: ${(error as Error).message}`);
779-
console.error(`[SQL Server executeReadOnly] SQL: ${processedSQL}`);
780-
if (parameters && parameters.length > 0) {
781-
console.error(`[SQL Server executeReadOnly] Parameters: ${JSON.stringify(parameters)}`);
751+
if (parameters && parameters.length > 0) {
752+
parameters.forEach((param, index) => {
753+
const paramName = `p${index + 1}`;
754+
if (typeof param === 'string') {
755+
request.input(paramName, sql.VarChar, param);
756+
} else if (typeof param === 'number') {
757+
if (Number.isInteger(param)) {
758+
request.input(paramName, sql.Int, param);
759+
} else {
760+
request.input(paramName, sql.Float, param);
761+
}
762+
} else if (typeof param === 'boolean') {
763+
request.input(paramName, sql.Bit, param);
764+
} else if (param === null || param === undefined) {
765+
request.input(paramName, sql.VarChar, param);
766+
} else if (Array.isArray(param)) {
767+
request.input(paramName, sql.VarChar, JSON.stringify(param));
768+
} else {
769+
request.input(paramName, sql.VarChar, JSON.stringify(param));
782770
}
783-
throw error;
771+
});
772+
}
773+
774+
let result;
775+
let queryFailed = false;
776+
try {
777+
result = await request.query(processedSQL);
778+
} catch (error) {
779+
queryFailed = true;
780+
console.error(`[SQL Server executeReadOnly] ERROR: ${(error as Error).message}`);
781+
console.error(`[SQL Server executeReadOnly] SQL: ${processedSQL}`);
782+
if (parameters && parameters.length > 0) {
783+
console.error(`[SQL Server executeReadOnly] Parameters: ${JSON.stringify(parameters)}`);
784784
}
785-
return {
786-
rows: result.recordset || [],
787-
rowCount: result.rowsAffected[0] || 0,
788-
...(messages.length > 0 ? { messages } : {}),
789-
};
785+
throw error;
790786
} finally {
791787
try {
792788
await transaction.rollback();
793-
} catch {
794-
// ignore; the original error is more useful
789+
} catch (rollbackError) {
790+
if (!queryFailed) {
791+
throw new Error(
792+
`Read-only rollback failed — data may have been modified: ${(rollbackError as Error).message}`,
793+
);
794+
}
795795
}
796796
}
797+
return {
798+
rows: result.recordset || [],
799+
rowCount: result.rowsAffected[0] || 0,
800+
...(messages.length > 0 ? { messages } : {}),
801+
};
797802
}
798803

799804
/**

0 commit comments

Comments
 (0)