Skip to content

Conversation

@gargsaumya
Copy link
Contributor

Reverts #392

Copilot AI review requested due to automatic review settings January 16, 2026 08:49
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 reverts PR #392, which fixed VARCHAR buffer sizing issues related to UTF-8 character encoding. The revert removes buffer size multipliers that were designed to prevent buffer overflows when the ODBC driver converts VARCHAR data containing multi-byte UTF-8 characters.

Changes:

  • Removed buffer size multipliers (4x) for VARCHAR columns in multiple allocation paths
  • Reverted error handling from throwing exceptions to falling back to LOB streaming
  • Removed three comprehensive tests covering UTF-8 encoding edge cases with special characters

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
tests/test_004_cursor.py Removes 122 lines of tests validating UTF-8 buffer handling with special characters, Latin-1 encoding, and emoji support
mssql_python/pybind/ddbc_bindings.h Adds SQLHSTMT parameter to column processors and restores LOB fallback paths instead of throwing errors for buffer overflow
mssql_python/pybind/ddbc_bindings.cpp Reverts VARCHAR buffer sizing from 4x multiplier back to 1x in SQLGetData, SQLBindCol, and batch fetch operations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

📊 Code Coverage Report

🔥 Diff Coverage

69%


🎯 Overall Coverage

76%


📈 Total Lines Covered: 5442 out of 7117
📁 Project: mssql-python


Diff Coverage

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

  • mssql_python/pybind/ddbc_bindings.cpp (73.1%): Missing lines 2957-2961,3294-3295
  • mssql_python/pybind/ddbc_bindings.h (65.0%): Missing lines 812-813,879-880,919-921

Summary

  • Total: 46 lines
  • Missing: 14 lines
  • Coverage: 69%

mssql_python/pybind/ddbc_bindings.cpp

Lines 2953-2965

  2953                                     row.append(raw_bytes);
  2954                                 }
  2955                             } else {
  2956                                 // Buffer too small, fallback to streaming
! 2957                                 LOG("SQLGetData: CHAR column %d data truncated "
! 2958                                     "(buffer_size=%zu), using streaming LOB",
! 2959                                     i, dataBuffer.size());
! 2960                                 row.append(FetchLobColumnData(hStmt, i, SQL_C_CHAR, false, false,
! 2961                                                               charEncoding));
  2962                             }
  2963                         } else if (dataLen == SQL_NULL_DATA) {
  2964                             LOG("SQLGetData: Column %d is NULL (CHAR)", i);
  2965                             row.append(py::none());

Lines 3290-3299

  3290                             if (static_cast<size_t>(dataLen) <= columnSize) {
  3291                                 row.append(py::bytes(
  3292                                     reinterpret_cast<const char*>(dataBuffer.data()), dataLen));
  3293                             } else {
! 3294                                 row.append(
! 3295                                     FetchLobColumnData(hStmt, i, SQL_C_BINARY, false, true, ""));
  3296                             }
  3297                         } else if (dataLen == SQL_NULL_DATA) {
  3298                             row.append(py::none());
  3299                         } else if (dataLen == 0) {

mssql_python/pybind/ddbc_bindings.h

  808             PyList_SET_ITEM(row, col - 1, pyStr);
  809         }
  810     } else {
  811         // Slow path: LOB data requires separate fetch call
! 812         PyList_SET_ITEM(row, col - 1,
! 813                         FetchLobColumnData(hStmt, col, SQL_C_CHAR, false, false).release().ptr());
  814     }
  815 }
  816 
  817 // Process SQL NCHAR/NVARCHAR (wide/Unicode string) column into Python str

  875         }
  876 #endif
  877     } else {
  878         // Slow path: LOB data requires separate fetch call
! 879         PyList_SET_ITEM(row, col - 1,
! 880                         FetchLobColumnData(hStmt, col, SQL_C_WCHAR, true, false).release().ptr());
  881     }
  882 }
  883 
  884 // Process SQL BINARY/VARBINARY (binary data) column into Python bytes

  915             PyList_SET_ITEM(row, col - 1, pyBytes);
  916         }
  917     } else {
  918         // Slow path: LOB data requires separate fetch call
! 919         PyList_SET_ITEM(
! 920             row, col - 1,
! 921             FetchLobColumnData(hStmt, col, SQL_C_BINARY, false, true, "").release().ptr());
  922     }
  923 }
  924 
  925 }  // namespace ColumnProcessors


📋 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.helpers.py: 67.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.4%
mssql_python.pybind.ddbc_bindings.h: 71.7%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.1%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@gargsaumya gargsaumya merged commit 6c5d9af into main Jan 16, 2026
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants