Skip to content

Commit 56a2a2e

Browse files
fix: drop unreachable drain loop from exec_sql
The C dqlite server aggregates multi-statement EXEC_SQL internally and emits exactly one RESULT frame (handle_exec_sql_done_cb recurses on exec->tail; fill_result reports sqlite3_changes of the last statement only). The drain loop added by #029 never ran more than once in practice and mis-advertised semantics ("sum of all rows_affected") the server cannot deliver. Read a single ResultResponse and return it. Update the docstring to reflect the real server behaviour. Replace the test that simulated the dead-code path with one asserting the client does not silently consume stray responses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 59e186b commit 56a2a2e

2 files changed

Lines changed: 29 additions & 34 deletions

File tree

src/dqliteclient/protocol.py

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -147,33 +147,23 @@ async def exec_sql(
147147
"""Execute SQL directly.
148148
149149
Returns (last_insert_id, rows_affected). For multi-statement SQL
150-
(semicolon-separated), returns the last statement's last_insert_id
151-
and the sum of all rows_affected, matching the Go client behavior.
150+
(semicolon-separated), the server aggregates internally and returns
151+
a single RESULT with sqlite3_changes() of the last statement only —
152+
rows_affected is NOT a sum across statements.
152153
"""
153154
request = ExecSqlRequest(db_id=db_id, sql=sql, params=params if params is not None else [])
154155
self._writer.write(request.encode())
155156
await self._writer.drain()
156157

157-
last_insert_id = 0
158-
rows_affected = 0
159-
160-
while True:
161-
response = await self._read_response()
162-
163-
if isinstance(response, FailureResponse):
164-
raise OperationalError(response.code, response.message)
165-
166-
if not isinstance(response, ResultResponse):
167-
raise ProtocolError(f"Expected ResultResponse, got {type(response).__name__}")
158+
response = await self._read_response()
168159

169-
last_insert_id = response.last_insert_id
170-
rows_affected += response.rows_affected
160+
if isinstance(response, FailureResponse):
161+
raise OperationalError(response.code, response.message)
171162

172-
# Check for more results from multi-statement SQL
173-
if not self._decoder.has_message():
174-
break
163+
if not isinstance(response, ResultResponse):
164+
raise ProtocolError(f"Expected ResultResponse, got {type(response).__name__}")
175165

176-
return last_insert_id, rows_affected
166+
return response.last_insert_id, response.rows_affected
177167

178168
async def query_sql(
179169
self, db_id: int, sql: str, params: Sequence[Any] | None = None

tests/test_protocol.py

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -126,30 +126,35 @@ async def test_exec_sql_with_empty_tuple_params(
126126
# Verify the request was written (meaning params=() was accepted)
127127
mock_writer.write.assert_called()
128128

129-
async def test_exec_sql_multi_statement(
129+
async def test_exec_sql_reads_single_response(
130130
self,
131131
protocol: DqliteProtocol,
132132
mock_reader: AsyncMock,
133133
) -> None:
134-
"""Multi-statement SQL should drain all ResultResponse messages."""
134+
"""exec_sql reads exactly one ResultResponse — it must not drain extras.
135+
136+
The C dqlite server emits exactly one RESULT per EXEC_SQL call, even
137+
for multi-statement SQL: handle_exec_sql_done_cb recurses internally
138+
on exec->tail and calls SUCCESS(..., RESULT) only after the last
139+
statement completes. fill_result reports sqlite3_changes() of the
140+
last statement only.
141+
142+
If any additional response bytes are buffered (server protocol
143+
violation, pipelining, test artefact), the client must leave them
144+
alone rather than silently consuming them.
145+
"""
135146
from dqlitewire.messages import ResultResponse
136147

137-
# Server sends two ResultResponse messages (one per statement)
138148
result1 = ResultResponse(last_insert_id=1, rows_affected=1)
139-
result2 = ResultResponse(last_insert_id=2, rows_affected=1)
140-
# Both arrive in a single read
141-
mock_reader.read.return_value = result1.encode() + result2.encode()
149+
stray = ResultResponse(last_insert_id=99, rows_affected=99)
150+
mock_reader.read.return_value = result1.encode() + stray.encode()
142151

143-
last_id, rows_affected = await protocol.exec_sql(
144-
1, "INSERT INTO t VALUES (1); INSERT INTO t VALUES (2)"
145-
)
146-
147-
# Should return the last statement's result with accumulated rows_affected
148-
assert last_id == 2
149-
assert rows_affected == 2
152+
last_id, rows_affected = await protocol.exec_sql(1, "INSERT INTO t VALUES (1)")
150153

151-
# The decoder buffer should be clean -- no leftover messages
152-
assert not protocol._decoder.has_message()
154+
assert last_id == 1
155+
assert rows_affected == 1
156+
# The stray message must still be buffered — not silently consumed.
157+
assert protocol._decoder.has_message()
153158

154159
async def test_query_sql_multi_statement_drains_extra(
155160
self,

0 commit comments

Comments
 (0)