Skip to content

Conversation

jahnvi480
Copy link
Contributor

@jahnvi480 jahnvi480 commented Aug 13, 2025

Work Item / Issue Reference

AB#39058


Summary

This pull request introduces comprehensive support for setting ODBC connection attributes in the mssql_python library, aligning its functionality with pyodbc's set_attr API. The changes include new constants for connection attributes, transaction isolation levels, and related options, as well as robust error handling and input validation in both Python and C++ layers. This enables users to configure connection behavior (e.g., autocommit, isolation level, timeouts) in a standardized and secure manner.

Connection Attribute Support

  • Added a wide set of ODBC connection attribute constants, transaction isolation level constants, access mode constants, and related enums to mssql_python/__init__.py and mssql_python/constants.py, making them available for use in Python code.
  • Implemented the set_attr method in the Connection Python class, providing pyodbc-compatible functionality for setting connection attributes with detailed input validation and error handling.

C++ Backend Enhancements

  • Exposed setAttribute as a public method in the C++ Connection class, and added a new setAttr method in ConnectionHandle, with improved error reporting and range validation for SQLUINTEGER values.
  • Registered the new set_attr method with the Python bindings, making it accessible from Python code.

Code Cleanup and Refactoring

  • Moved and consolidated connection attribute constants in ConstantsDDBC to improve maintainability, and removed legacy/unused constants.

These changes provide a robust interface for configuring ODBC connection attributes, improve compatibility with pyodbc, and enhance error handling for attribute operations.

@github-actions github-actions bot added the pr-size: large Substantial code update label Aug 13, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 13, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 26, 2025
Copy link
Contributor

@gargsaumya gargsaumya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss the current implementation. Want to understand what we plan to do here.
Also, has this been thoroughly tested?

@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 26, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 26, 2025
@jahnvi480 jahnvi480 requested a review from gargsaumya August 26, 2025 17:26
@jahnvi480 jahnvi480 requested a review from sumitmsft August 26, 2025 17:26
gargsaumya
gargsaumya previously approved these changes Aug 27, 2025
@jahnvi480 jahnvi480 marked this pull request as draft August 27, 2025 11:31
@jahnvi480
Copy link
Contributor Author

Parking this API for now need to validate the behavior of few attributes before giving support. (AUTOCOMMIT)

@jahnvi480 jahnvi480 changed the base branch from jahnvi/conn_setdecoding to main September 30, 2025 06:50
@jahnvi480 jahnvi480 dismissed gargsaumya’s stale review September 30, 2025 06:50

The base branch was changed.

@jahnvi480 jahnvi480 marked this pull request as ready for review October 6, 2025 12:45
@Copilot Copilot AI review requested due to automatic review settings October 6, 2025 12:45
Copy link
Contributor

@Copilot Copilot AI left a 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 introduces comprehensive support for setting ODBC connection attributes through a new set_attr method, providing pyodbc-compatible functionality for configuring connection behavior. The implementation includes robust input validation, error handling, and proper timing constraints for different attribute types.

  • Added set_attr method to Connection class with input validation and error handling
  • Implemented comprehensive connection attribute constants and transaction isolation levels
  • Enhanced C++ backend with improved attribute setting capabilities and error reporting

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/test_003_connection.py Added extensive test cases for setencoding, setdecoding, and set_attr functionality
mssql_python/pybind/ddbc_bindings.cpp Registered set_attr method with Python bindings
mssql_python/pybind/connection/connection.h Added setAttribute as public method and setAttr to ConnectionHandle
mssql_python/pybind/connection/connection.cpp Enhanced setAttribute implementation with better error handling and type support
mssql_python/helpers.py Added validate_attribute_value function for input validation
mssql_python/constants.py Added connection attribute constants and timing validation helpers
mssql_python/connection.py Implemented set_attr method in Python Connection class
mssql_python/init.py Exported connection attribute constants for public API

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}
else if (py::isinstance<py::str>(value)) {
try {
static std::vector<std::wstring> wstr_buffers; // Keep buffers alive
Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using static vectors for buffer management can lead to memory leaks and thread safety issues. Consider using a more controlled approach like thread-local storage or instance-based buffer management.

Copilot uses AI. Check for mistakes.

Comment on lines +251 to +264
static std::vector<std::string> buffers;
std::string binary_data = value.cast<std::string>();

// Limit static buffer growth
constexpr size_t MAX_BUFFER_COUNT = 100;
if (buffers.size() >= MAX_BUFFER_COUNT) {
// Remove oldest 50% of entries when limit reached
buffers.erase(buffers.begin(), buffers.begin() + (MAX_BUFFER_COUNT / 2));
}

buffers.emplace_back(std::move(binary_data));
SQLPOINTER ptr = const_cast<char*>(buffers.back().c_str());
SQLINTEGER length = static_cast<SQLINTEGER>(buffers.back().size());

Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same static buffer issue as with wstr_buffers. This pattern duplicates the memory management problem for binary data.

Suggested change
static std::vector<std::string> buffers;
std::string binary_data = value.cast<std::string>();
// Limit static buffer growth
constexpr size_t MAX_BUFFER_COUNT = 100;
if (buffers.size() >= MAX_BUFFER_COUNT) {
// Remove oldest 50% of entries when limit reached
buffers.erase(buffers.begin(), buffers.begin() + (MAX_BUFFER_COUNT / 2));
}
buffers.emplace_back(std::move(binary_data));
SQLPOINTER ptr = const_cast<char*>(buffers.back().c_str());
SQLINTEGER length = static_cast<SQLINTEGER>(buffers.back().size());
std::string binary_data = value.cast<std::string>();
SQLPOINTER ptr = const_cast<char*>(binary_data.c_str());
SQLINTEGER length = static_cast<SQLINTEGER>(binary_data.size());

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: large Substantial code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants