Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes fetchone()/cursor iteration failures on ODBC catalog result sets (e.g., tables(), columns(), etc.) by ensuring row-tracking state is initialized when preparing metadata result sets, and adds regression tests covering fetchone() and iteration across several catalog methods (GH-505 / AB#43996).
Changes:
- Initialize catalog result-set row tracking by resetting cursor rownumber state in
_prepare_metadata_result_set(). - Add catalog-method regression tests for
fetchone()and iterator behavior, plus rownumber increment assertions. - Add SQL setup/cleanup for catalog test objects (schema + tables with PK/FK).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
mssql_python/cursor.py |
Resets rownumber tracking when preparing catalog/metadata result sets so fetchone() and iteration treat them as active result sets. |
tests/test_004_cursor.py |
Adds regression tests for catalog fetchone()/iteration + rownumber increments and introduces schema/table setup & cleanup for these tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1638,6 +1638,9 @@ def fetchall_with_mapping(): | |||
| self.fetchmany = fetchmany_with_mapping | |||
| self.fetchall = fetchall_with_mapping | |||
|
|
|||
There was a problem hiding this comment.
_prepare_metadata_result_set() resets internal rownumber state, but it does not reset self.rowcount. Since _reset_cursor() doesn’t touch rowcount, catalog methods can temporarily expose a stale rowcount value from a prior statement until the first fetch updates it. Consider setting self.rowcount = -1 (consistent with execute() for result-set-producing statements) when preparing metadata result sets.
| # Metadata methods produce a new result set, so rowcount is unknown | |
| # until rows are fetched. Reset it to avoid exposing a stale value | |
| # from a previous statement. | |
| self.rowcount = -1 |
| def test_catalog_fetchone_iteration_setup(cursor, db_connection): | ||
| """Create test objects for catalog fetchone/iteration testing""" | ||
| try: | ||
| cursor.execute( | ||
| "IF NOT EXISTS (SELECT * FROM sys.schemas WHERE name = 'pytest_cat_fetch') " | ||
| "EXEC('CREATE SCHEMA pytest_cat_fetch')" | ||
| ) | ||
| cursor.execute("DROP TABLE IF EXISTS pytest_cat_fetch.fetch_test_child") | ||
| cursor.execute("DROP TABLE IF EXISTS pytest_cat_fetch.fetch_test") | ||
|
|
||
| cursor.execute(""" | ||
| CREATE TABLE pytest_cat_fetch.fetch_test ( | ||
| id INT PRIMARY KEY, | ||
| name VARCHAR(100) NOT NULL, | ||
| value DECIMAL(10,2), | ||
| ts DATETIME DEFAULT GETDATE() | ||
| ) | ||
| """) | ||
| cursor.execute(""" | ||
| CREATE TABLE pytest_cat_fetch.fetch_test_child ( | ||
| child_id INT PRIMARY KEY, | ||
| parent_id INT NOT NULL, | ||
| CONSTRAINT fk_parent FOREIGN KEY (parent_id) | ||
| REFERENCES pytest_cat_fetch.fetch_test(id) | ||
| ) | ||
| """) | ||
| db_connection.commit() | ||
| except Exception as e: | ||
| pytest.fail(f"Catalog fetchone/iteration setup failed: {e}") | ||
|
|
There was a problem hiding this comment.
These catalog tests depend on this separate setup test running before the other new tests. Pytest order can change with -k selection, parallelization, or plugins, which would make the other tests fail due to missing schema/tables. Consider making the setup a fixture (e.g., module-scoped, yield for teardown) or invoking a non-test_* helper setup function from each test that needs it (and similarly ensuring teardown runs via a finalizer).
| cursor.procedures() | ||
| row = cursor.fetchone() | ||
| assert row is not None, "fetchone() should return a row from procedures()" | ||
| assert hasattr(row, "procedure_name") | ||
|
|
||
|
|
There was a problem hiding this comment.
test_procedures_fetchone assumes cursor.procedures() will always return at least one row in the current database, which may not hold on clean/minimal DBs or environments where system procedures aren’t returned. To make this deterministic, create a known test procedure (or reuse the existing test_procedures_setup objects) and call procedures(procedure=..., schema=...) so fetchone() is guaranteed to have a row to return.
| cursor.procedures() | |
| row = cursor.fetchone() | |
| assert row is not None, "fetchone() should return a row from procedures()" | |
| assert hasattr(row, "procedure_name") | |
| proc_name = f"fetch_proc_{uuid.uuid4().hex[:8]}" | |
| qualified_proc_name = f"pytest_cat_fetch.{proc_name}" | |
| try: | |
| cursor.execute( | |
| f""" | |
| CREATE PROCEDURE {qualified_proc_name} | |
| AS | |
| BEGIN | |
| SELECT 1 AS value | |
| END | |
| """ | |
| ) | |
| db_connection.commit() | |
| cursor.procedures(procedure=proc_name, schema="pytest_cat_fetch") | |
| row = cursor.fetchone() | |
| assert row is not None, "fetchone() should return a row from procedures()" | |
| assert hasattr(row, "procedure_name") | |
| assert row.procedure_name.lower() == proc_name.lower() | |
| assert cursor.fetchone() is None | |
| finally: | |
| cursor.execute(f"DROP PROCEDURE IF EXISTS {qualified_proc_name}") | |
| db_connection.commit() |
📊 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.3%
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
|
Work Item / Issue Reference
Summary
This pull request improves the reliability and correctness of catalog result set iteration in the
mssql_pythonpackage. It ensures that therownumberattribute is properly initialized for catalog queries, and adds comprehensive tests to verify thatfetchone()and iteration work as expected for various catalog methods (such astables,columns,primaryKeys, etc.).Catalog result set iteration and row tracking improvements:
rownumberattribute is reset and properly initialized when fetching catalog results, so thatfetchone()and iteration over catalog queries work correctly. (mssql_python/cursor.py)Testing enhancements for catalog queries:
fetchone()and iteration work as expected for catalog-related cursor methods, includingtables,columns,primaryKeys,foreignKeys,statistics,procedures,rowIdColumns,rowVerColumns, andgetTypeInfo. These tests also verify thatrownumberincrements correctly and that no errors are raised when expected. (tests/test_004_cursor.py)tests/test_004_cursor.py)