feat(connectors)!: return per-statement result sets, wire up frontend tabs - #389
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes SQL Server connector behavior for multi-statement batches so that all result sets are returned (matching MySQL/MariaDB behavior) and rowCount stays consistent with the aggregated results.
Changes:
- Switch SQL Server execution paths from
result.recordset(first result set only) toresult.recordsets(all result sets), flattening rows in emission order. - Update
rowCountcalculation to sumrowsAffectedacross all statements in the batch (including mixed write/read batches). - Add SQL Server integration tests covering multi-statement SELECT, mixed write/read batches, and readonly mode behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/connectors/sqlserver/index.ts | Reads and flattens all SQL Server recordsets and sums rowsAffected across batch statements for consistent rows/rowCount. |
| src/connectors/tests/sqlserver.integration.test.ts | Adds integration coverage for multi-result-set batches and summed rowCount, including readonly mode. |
…pers 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.
…g 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.
tianzhou
changed the title
… 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.
tianzhou
changed the title
… 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/connectors/tests/sqlserver.integration.test.ts:833
- This test name is misleading: the connector no longer returns a single summed
rowCountfor a mixed batch; it returns one result set for the SELECT plus a trailing write-summary result set. Renaming the test will make the asserted behavior easier to understand and prevent future confusion.
it('should sum rowCount across a mixed write/read multi-statement batch', async () => {
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Started as the SQL Server fix for #380 (dropped result sets in a multi-statement batch), but landed on a broader redesign: instead of flattening every statement's rows into one array,
SQLResultnow returns a list of per-statement results, and the local web frontend gets a real multi-tab UI for it.Why the redesign, not just a SQL Server fix: making SQL Server flatten multi-statement rows the same way MySQL/MariaDB already did was itself the wrong shape.
SELECT 1 AS a; SELECT 2 AS breturning{rows: [{a:1},{b:2}], rowCount: 2}is indistinguishable from a single query that returned two differently-shaped rows glued together - a caller has no way to tell where one statement's output ends and the next begins.Backend (
src/connectors/interface.ts):SQLResultis now{ resultSets: SQLResultSet[], messages? }, one entry per statement in execution order. EachSQLResultSetalso carries an optionalsql- the statement that produced it, when a connector can attribute it unambiguously.sqlfor 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: node-mssql'srecordsets/rowsAffectedarrays aren't index-aligned per statement (documented inbuildResultSets), 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, which is the actual SQL Server returns only the first result set of a multi-statement query, while MySQL/MariaDB return all #380 bug.execute_sql/custom-tool JSON responses now key their statement list asstatements: [{sql, rows, count}, ...]instead of flatrows/count.explain_sqlkeeps its flat shape since it only ever runs exactly one statement.Frontend (
frontend/): the shipped local web UI readsexecute_sql's response directly and broke under this change, so it's rebuilt around the same idea:executeTool()now returns oneQueryResultper statement instead of a single merged result, andToolDetailViewcreates one result tab per statement (labeled(i/N)for batches of more than one) instead of forcing a whole batch into a single tab. Verified end-to-end in a browser against the--demoSQLite backend.Also fixed, as a byproduct of that browser verification:
frontend/src/api/tools.tscalledresponse.json()directly, but the backend's stateless HTTP transport answers SSE-framed responses for this request shape - every query through the web UI over HTTP transport was silently broken before this PR too, unrelated to theresultSetschange itself.BREAKING CHANGE:
execute_sql/custom tool JSON responses no longer have top-levelrows/count- they're understatements, each entry additionally carryingsql.Test plan
pnpm test- full suite, 1272/1272 passingpnpm exec tsc --noEmit(backend) andtsc -b(frontend) - no new errorsCloses #380
🤖 Generated with Claude Code