Skip to content

Conversation

@jahnvi480
Copy link
Contributor

@jahnvi480 jahnvi480 commented Oct 19, 2025

Work Item / Issue Reference

AB#38478


Summary

This pull request refactors the connection.cpp and connection.h files to improve code readability, maintainability, and consistency, while also making minor corrections and clarifications to comments. The changes mainly involve formatting, type usage, and error handling improvements, as well as updating include paths and constructor signatures.

Code Formatting and Readability Improvements

  • Reformatted function calls and argument lists for better readability, including breaking up long lines and grouping parameters logically in methods such as getEnvHandle, allocateDbcHandle, commit, rollback, and others in connection.cpp.
  • Improved comment formatting and clarity, including updating TODOs and explanatory comments to be more precise and easier to understand.

Type and Variable Usage Updates

  • Updated integer types in setAttribute from long long to int64_t for clarity and platform consistency.
  • Improved buffer management for string and binary attributes by clarifying buffer lifetime logic and using more explicit type casts.

Error Handling Enhancements

  • Enhanced error handling in attribute setting and connection attribute application, including more detailed error messages and fallback logic.

Include Path and Constructor Signature Updates

  • Updated include paths in both connection.cpp and connection.h for consistency and to support future platform agnostic changes.
  • Modified the ConnectionHandle constructor signature to improve clarity and maintainability.

@jahnvi480 jahnvi480 requested review from Copilot and gargsaumya and removed request for Copilot October 19, 2025 16:23
@github-actions github-actions bot added the pr-size: medium Moderate update size label Oct 19, 2025
@jahnvi480 jahnvi480 requested review from bewithgaurav, gargsaumya and sumitmsft and removed request for gargsaumya and sumitmsft October 19, 2025 16:23
@github-actions
Copy link

github-actions bot commented Oct 19, 2025

📊 Code Coverage Report

🔥 Diff Coverage

81%


🎯 Overall Coverage

77%


📈 Total Lines Covered: 4616 out of 5965
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/pybind/connection/connection.cpp (90.5%): Missing lines 226-227,262-263,276-277,294-295
  • mssql_python/pybind/connection/connection_pool.cpp (75.0%): Missing lines 62-63,103-104
  • mssql_python/pybind/connection/connection_pool.h (100%)
  • mssql_python/pybind/ddbc_bindings.h (100%)
  • mssql_python/pybind/unix_buffers.h (0.0%): Missing lines 37,65,73,81,102,114,128,130,133-134,136,152,158,163,167

Summary

  • Total: 147 lines
  • Missing: 27 lines
  • Coverage: 81%

mssql_python/pybind/connection/connection.cpp

Lines 222-231

  222             // Limit static buffer growth for memory safety
  223             constexpr size_t MAX_BUFFER_COUNT = 100;
  224             if (wstr_buffers.size() >= MAX_BUFFER_COUNT) {
  225             // Remove oldest 50% of entries when limit reached
! 226             wstr_buffers.erase(wstr_buffers.begin(),
! 227                                wstr_buffers.begin() + (MAX_BUFFER_COUNT / 2));
  228             }
  229 
  230             wstr_buffers.push_back(wstr);

Lines 258-267

  258                 LOG("Set string attribute successfully");
  259             }
  260             return ret;
  261         } catch (const std::exception& e) {
! 262             LOG("Exception during string attribute setting: " +
! 263                 std::string(e.what()));
  264             return SQL_ERROR;
  265         }
  266     } else if (py::isinstance<py::bytes>(value) ||
  267                py::isinstance<py::bytearray>(value)) {

Lines 272-281

  272             // Limit static buffer growth
  273             constexpr size_t MAX_BUFFER_COUNT = 100;
  274             if (buffers.size() >= MAX_BUFFER_COUNT) {
  275             // Remove oldest 50% of entries when limit reached
! 276             buffers.erase(buffers.begin(),
! 277                           buffers.begin() + (MAX_BUFFER_COUNT / 2));
  278             }
  279 
  280             buffers.emplace_back(std::move(binary_data));
  281             SQLPOINTER ptr = const_cast<char*>(buffers.back().c_str());

Lines 290-299

  290                 LOG("Set attribute successfully with binary data");
  291             }
  292             return ret;
  293         } catch (const std::exception& e) {
! 294             LOG("Exception during binary attribute setting: " +
! 295                 std::string(e.what()));
  296             return SQL_ERROR;
  297         }
  298     } else {
  299         LOG("Unsupported attribute value type");

mssql_python/pybind/connection/connection_pool.cpp

Lines 58-67

  58             valid_conn = std::make_shared<Connection>(connStr, true);
  59             valid_conn->connect(attrs_before);
  60             ++_current_size;
  61         } else if (!valid_conn) {
! 62             throw std::runtime_error(
! 63                 "ConnectionPool::acquire: pool size limit reached");
  64         }
  65     }
  66 
  67     // Phase 3: Disconnect expired/bad connections outside lock

Lines 99-108

   99     for (auto& conn : to_close) {
  100         try {
  101             conn->disconnect();
  102         } catch (const std::exception& ex) {
! 103             LOG("ConnectionPool::close: disconnect failed: {}",
! 104                 ex.what());
  105         }
  106     }
  107 }

mssql_python/pybind/unix_buffers.h

  33  public:
  34     /**
  35      * Constructor allocates a buffer of the specified size
  36      */
! 37     explicit SQLWCHARBuffer(size_t size) : buffer_size(size) {
  38         buffer = std::make_unique<SQLWCHAR[]>(size);
  39         // Initialize to zero
  40         for (size_t i = 0; i < size; i++) {
  41             buffer[i] = 0;

  61      * Similar to the UCS_dec function in the Python PoC
  62      */
  63     std::wstring toString(SQLSMALLINT length = -1) const {
  64         std::wstring result;
! 65 
  66         // If length is provided, use it
  67         if (length > 0) {
  68             for (SQLSMALLINT i = 0; i < length; i++) {
  69                 result.push_back(static_cast<wchar_t>(buffer[i]));

  69                 result.push_back(static_cast<wchar_t>(buffer[i]));
  70             }
  71             return result;
  72         }
! 73 
  74         // Otherwise, read until null terminator
  75         for (size_t i = 0; i < buffer_size; i++) {
  76             if (buffer[i] == 0) {
  77                 break;

  77                 break;
  78             }
  79             result.push_back(static_cast<wchar_t>(buffer[i]));
  80         }
! 81 
  82         return result;
  83     }
  84 };

   98     std::vector<Record> records;
   99 
  100  public:
  101     void addRecord(const std::wstring& sqlState,
! 102                    const std::wstring& message, SQLINTEGER nativeError) {
  103         records.push_back({sqlState, message, nativeError});
  104     }
  105 
  106     bool empty() const {

  110     std::wstring getSQLState() const {
  111         if (!records.empty()) {
  112             return records[0].sqlState;
  113         }
! 114         return L"HY000";  // General error
  115     }
  116 
  117     std::wstring getFirstErrorMessage() const {
  118         if (!records.empty()) {

  124     std::wstring getFullErrorMessage() const {
  125         if (records.empty()) {
  126             return L"No error information available";
  127         }
! 128 
  129         std::wstring fullMessage = records[0].message;
! 130 
  131         // Add additional error messages if there are any
  132         for (size_t i = 1; i < records.size(); i++) {
! 133             fullMessage += L"; [" + records[i].sqlState + L"] " +
! 134                           records[i].message;
  135         }
! 136 
  137         return fullMessage;
  138     }
  139 
  140     size_t size() const {

  148  */
  149 inline std::wstring UCS_dec(const SQLWCHAR* buffer, size_t maxLength = 0) {
  150     std::wstring result;
  151     size_t i = 0;
! 152 
  153     while (true) {
  154         // Break if we've reached the maximum length
  155         if (maxLength > 0 && i >= maxLength) {
  156             break;

  154         // Break if we've reached the maximum length
  155         if (maxLength > 0 && i >= maxLength) {
  156             break;
  157         }
! 158 
  159         // Break if we've reached a null terminator
  160         if (buffer[i] == 0) {
  161             break;
  162         }
! 163 
  164         result.push_back(static_cast<wchar_t>(buffer[i]));
  165         i++;
  166     }
! 167 
  168     return result;
  169 }
  170 
  171 }  // namespace unix_buffers


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.ddbc_bindings.cpp: 70.7%
mssql_python.row.py: 77.9%
mssql_python.pybind.connection.connection_pool.cpp: 78.9%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection.cpp: 81.2%
mssql_python.connection.py: 82.9%
mssql_python.cursor.py: 83.6%
mssql_python.auth.py: 87.1%
mssql_python.pooling.py: 87.7%
mssql_python.exceptions.py: 92.1%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Copilot AI review requested due to automatic review settings October 20, 2025 22:01
Copy link
Contributor

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 applies C++ linting and code style improvements to the connection implementation files in the mssql_python/pybind/connection directory. The changes focus on code readability, maintainability, and type safety without altering functional behavior.

Key changes include:

  • Reformatted long function calls and statements across multiple lines for improved readability with consistent indentation
  • Enhanced type safety by replacing long long with int64_t and using reinterpret_cast for pointer conversions
  • Improved error handling with more descriptive error messages and robust exception handling

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
mssql_python/pybind/connection/connection.h Updated include statements and reformatted function signatures for consistency
mssql_python/pybind/connection/connection.cpp Extensive formatting improvements including line splitting, type safety enhancements, and improved error handling throughout

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

@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: medium Moderate update size labels Oct 20, 2025
gargsaumya
gargsaumya previously approved these changes Oct 27, 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.

LGTM. The added comments are non-blocking.

@jahnvi480 jahnvi480 merged commit 695719d into main Oct 28, 2025
21 checks passed
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.

4 participants