FIX: cursor.execute() with reset_cursor=False raises Invalid cursor state #521
FIX: cursor.execute() with reset_cursor=False raises Invalid cursor state #521
Conversation
…tate #506 Add SqlHandle::close_cursor() which calls SQLFreeStmt(SQL_CLOSE) to close the ODBC cursor without destroying the prepared statement handle. When reset_cursor=False, execute() now calls close_cursor() instead of skipping cleanup entirely, allowing the prepared plan to be reused. Added 5 tests covering reset_cursor=False scenarios.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 67.8%
mssql_python.row.py: 70.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 74.2%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.__init__.py: 77.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 85.2%🔗 Quick Links
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes Invalid cursor state errors when re-executing statements with reset_cursor=False by closing only the ODBC cursor (not the prepared statement), enabling safe prepared-plan reuse across executions.
Changes:
- Add
SqlHandle::close_cursor()(ODBCSQLFreeStmt(SQL_CLOSE)) and expose it via Pybind11. - Update
cursor.execute()to close the ODBC cursor whenreset_cursor=False. - Add regression tests covering re-execution, multi-iteration reuse, no-params execution, and re-execution without fully consuming prior results.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_004_cursor.py | Adds regression tests validating reset_cursor=False prepared-plan reuse and cursor state handling. |
| mssql_python/pybind/ddbc_bindings.h | Declares SqlHandle::close_cursor() API. |
| mssql_python/pybind/ddbc_bindings.cpp | Implements close_cursor() and exposes it to Python via Pybind11. |
| mssql_python/cursor.py | Calls hstmt.close_cursor() when reset_cursor=False to prevent invalid cursor state on re-exec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (_handle && SQLFreeStmt_ptr && _type == SQL_HANDLE_STMT) { | ||
| SQLFreeStmt_ptr(_handle, SQL_CLOSE); | ||
| } |
There was a problem hiding this comment.
close_cursor() silently no-ops when SQLFreeStmt_ptr is not loaded and also ignores the SQLRETURN from SQLFreeStmt. This can mask failures and leave the cursor open, potentially preserving the original 'Invalid cursor state' behavior without surfacing an actionable error. Consider aligning behavior with other wrappers: (1) throw when SQLFreeStmt_ptr is not loaded, and (2) check the return code and raise via the existing error/diagnostics path when the call fails (treat SQL_SUCCESS_WITH_INFO appropriately).
| if (_handle && SQLFreeStmt_ptr && _type == SQL_HANDLE_STMT) { | |
| SQLFreeStmt_ptr(_handle, SQL_CLOSE); | |
| } | |
| if (_type != SQL_HANDLE_STMT || !_handle) { | |
| return; | |
| } | |
| if (!SQLFreeStmt_ptr) { | |
| ThrowStdException("SQLFreeStmt function not loaded"); | |
| } | |
| SQLRETURN ret = SQLFreeStmt_ptr(_handle, SQL_CLOSE); | |
| if (ret != SQL_SUCCESS && ret != SQL_SUCCESS_WITH_INFO) { | |
| ThrowStdException("SQLFreeStmt(SQL_CLOSE) failed"); | |
| } |
| kwargs = {"reset_cursor": False} if i > 0 else {} | ||
| cursor.execute("SELECT ? AS iter", (i,), **kwargs) | ||
| row = cursor.fetchone() | ||
| assert row[0] == i, f"Expected {i}, got {row[0]}" |
There was a problem hiding this comment.
The assertion message indexes row[0] even in the failure path; if fetchone() unexpectedly returns None, this will raise a TypeError and obscure the real failure. Prefer asserting the whole row (e.g., assert row == (i,)) or add a separate assert row is not None before indexing so test failures remain clear and diagnostic.
| assert row[0] == i, f"Expected {i}, got {row[0]}" | |
| assert row == (i,), f"Expected {(i,)}, got {row}" |
Work Item / Issue Reference
Summary
This pull request improves the handling of cursor state when executing prepared statements with
reset_cursor=False, ensuring that the prepared plan is correctly reused and the cursor state is properly managed between executions. It introduces a newclose_cursormethod at the ODBC statement handle level, updates the Python bindings, and adds comprehensive tests to verify the new behavior.Enhancements to cursor state management:
cursor.pyto callhstmt.close_cursor()(which issues an ODBCSQLFreeStmt(SQL_CLOSE)) whenreset_cursor=False, allowing prepared statements to be reused safely without resetting the entire cursor state. This resolves issues with invalid cursor state on re-execution.ODBC handle and Python bindings updates:
close_cursormethod in theSqlHandleC++ class, which closes only the cursor on the statement handle without freeing the prepared statement.close_cursormethod to Python via theddbc_bindingsPybind11 module, allowing it to be called from Python code.Testing improvements:
test_004_cursor.pyto verify thatreset_cursor=Falsecorrectly reuses prepared plans, returns new results, works across multiple iterations, handles queries without parameters, and functions correctly when the previous result set was not fully consumed.