-
Notifications
You must be signed in to change notification settings - Fork 30
FIX: Timeout during cursor creation and not execute #348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… jahnvi/final_linting
… jahnvi/timeout_291
…rosoft/mssql-python into jahnvi/timeout_291
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors query timeout handling in the mssql-python driver by moving timeout configuration from the execute method to cursor initialization, following pyodbc's approach for better performance. The timeout is now set once when a cursor is created or reset, rather than on every query execution.
- Introduces
_set_timeout()method to configure query timeout during cursor initialization - Updates C++ bindings to properly handle integer statement attributes like
SQL_ATTR_QUERY_TIMEOUT - Adds comprehensive test coverage for timeout behavior across various scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| mssql_python/cursor.py | Refactors timeout handling by adding _set_timeout() method called during cursor initialization and removing redundant timeout logic from execute |
| mssql_python/pybind/ddbc_bindings.cpp | Updates DDBCSQLSetStmtAttr binding to handle integer attributes properly; minor comment additions and code reorganization for error handling |
| mssql_python/constants.py | Adds SQL_ATTR_QUERY_TIMEOUT constant for ODBC statement attribute configuration |
| tests/test_003_connection.py | Adds 6 new comprehensive test functions covering timeout behavior with long-running queries, multiple executions, cursor resets, compatibility, and edge cases |
| mssql_python/connection.py | Contains duplicate function/constant definitions and changes threading lock type; also includes minor documentation changes to encoding-related methods |
Comments suppressed due to low confidence (1)
mssql_python/connection.py:160
- Duplicate function and constant definitions detected. The
_validate_utf16_wchar_compatibilityfunction (lines 61-107 and 114-160) andUTF16_ENCODINGSconstant (lines 58, 111) are defined twice identically. This appears to be an accidental duplication that should be removed to avoid maintenance issues.
def _validate_utf16_wchar_compatibility(
encoding: str, wchar_type: int, context: str = "SQL_WCHAR"
) -> None:
"""
Validates UTF-16 encoding compatibility with SQL_WCHAR.
Centralizes the validation logic to eliminate duplication across setencoding/setdecoding.
Args:
encoding: The encoding string (already normalized to lowercase)
wchar_type: The SQL_WCHAR constant value to check against
context: Context string for error messages ('SQL_WCHAR', 'SQL_WCHAR ctype', etc.)
Raises:
ProgrammingError: If encoding is incompatible with SQL_WCHAR
"""
if encoding == "utf-16":
# UTF-16 with BOM is rejected due to byte order ambiguity
logger.warning("utf-16 with BOM rejected for %s", context)
raise ProgrammingError(
driver_error="UTF-16 with Byte Order Mark not supported for SQL_WCHAR",
ddbc_error=(
"Cannot use 'utf-16' encoding with SQL_WCHAR due to Byte Order Mark ambiguity. "
"Use 'utf-16le' or 'utf-16be' instead for explicit byte order."
),
)
elif encoding not in UTF16_ENCODINGS:
# Non-UTF-16 encodings are not supported with SQL_WCHAR
logger.warning(
"Non-UTF-16 encoding %s attempted with %s", sanitize_user_input(encoding), context
)
# Generate context-appropriate error messages
if "ctype" in context:
driver_error = f"SQL_WCHAR ctype only supports UTF-16 encodings"
ddbc_context = "SQL_WCHAR ctype"
else:
driver_error = f"SQL_WCHAR only supports UTF-16 encodings"
ddbc_context = "SQL_WCHAR"
raise ProgrammingError(
driver_error=driver_error,
ddbc_error=(
f"Cannot use encoding '{encoding}' with {ddbc_context}. "
f"SQL_WCHAR requires UTF-16 encodings (utf-16le, utf-16be)"
),
)
# Note: "utf-16" with BOM is NOT included as it's problematic for SQL_WCHAR
UTF16_ENCODINGS: frozenset[str] = frozenset(["utf-16le", "utf-16be"])
def _validate_utf16_wchar_compatibility(
encoding: str, wchar_type: int, context: str = "SQL_WCHAR"
) -> None:
"""
Validates UTF-16 encoding compatibility with SQL_WCHAR.
Centralizes the validation logic to eliminate duplication across setencoding/setdecoding.
Args:
encoding: The encoding string (already normalized to lowercase)
wchar_type: The SQL_WCHAR constant value to check against
context: Context string for error messages ('SQL_WCHAR', 'SQL_WCHAR ctype', etc.)
Raises:
ProgrammingError: If encoding is incompatible with SQL_WCHAR
"""
if encoding == "utf-16":
# UTF-16 with BOM is rejected due to byte order ambiguity
logger.warning("utf-16 with BOM rejected for %s", context)
raise ProgrammingError(
driver_error="UTF-16 with Byte Order Mark not supported for SQL_WCHAR",
ddbc_error=(
"Cannot use 'utf-16' encoding with SQL_WCHAR due to Byte Order Mark ambiguity. "
"Use 'utf-16le' or 'utf-16be' instead for explicit byte order."
),
)
elif encoding not in UTF16_ENCODINGS:
# Non-UTF-16 encodings are not supported with SQL_WCHAR
logger.warning(
"Non-UTF-16 encoding %s attempted with %s", sanitize_user_input(encoding), context
)
# Generate context-appropriate error messages
if "ctype" in context:
driver_error = f"SQL_WCHAR ctype only supports UTF-16 encodings"
ddbc_context = "SQL_WCHAR ctype"
else:
driver_error = f"SQL_WCHAR only supports UTF-16 encodings"
ddbc_context = "SQL_WCHAR"
raise ProgrammingError(
driver_error=driver_error,
ddbc_error=(
f"Cannot use encoding '{encoding}' with {ddbc_context}. "
f"SQL_WCHAR requires UTF-16 encodings (utf-16le, utf-16be)"
),
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/cursor.pyLines 705-713 705 timeout_value,
706 )
707 check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
708 logger.debug("Query timeout set to %d seconds", timeout_value)
! 709 except Exception as e: # pylint: disable=broad-exception-caught
710 logger.warning("Failed to set query timeout: %s", str(e))
711
712 def _reset_cursor(self) -> None:
713 """mssql_python/pybind/ddbc_bindings.cppLines 4400-4409 4400 ptr_value =
4401 reinterpret_cast<SQLPOINTER>(static_cast<SQLULEN>(value.cast<int64_t>()));
4402 } else {
4403 // For pointer attributes
! 4404 ptr_value = value.cast<SQLPOINTER>();
! 4405 }
4406 return SQLSetStmtAttr_ptr(stmt->get(), attr, ptr_value, 0);
4407 },
4408 "Set statement attributes");
4409 m.def("DDBCSQLGetTypeInfo", &SQLGetTypeInfo_Wrapper,📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 66.3%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.7%
mssql_python.cursor.py: 84.3%
mssql_python.logging.py: 85.3%🔗 Quick Links
|
Work Item / Issue Reference
Summary
This pull request refactors how query timeouts are set for statement handles in the
mssql_python/cursor.pymodule. The main improvement is moving the logic for setting the query timeout from theexecutemethod to the cursor initialization process, ensuring the timeout is consistently applied whenever the statement handle is allocated or reset.Statement handle timeout management:
_set_timeoutmethod to set the query timeout attribute on the statement handle during cursor initialization, following best practices for performance. (mssql_python/cursor.py)executemethod, centralizing timeout management in the cursor lifecycle. (mssql_python/cursor.py)