Skip to content

FIX: Capture PRINT messages in nextset() via SQL_SUCCESS_WITH_INFO#618

Open
jahnvi480 wants to merge 5 commits into
mainfrom
jahnvi/fix-nextset-messages-612
Open

FIX: Capture PRINT messages in nextset() via SQL_SUCCESS_WITH_INFO#618
jahnvi480 wants to merge 5 commits into
mainfrom
jahnvi/fix-nextset-messages-612

Conversation

@jahnvi480
Copy link
Copy Markdown
Contributor

@jahnvi480 jahnvi480 commented Jun 2, 2026

Work Item / Issue Reference

AB#45395

GitHub Issue: #612


Summary

This pull request addresses an issue where diagnostic messages (such as SQL Server PRINT output) from subsequent result sets were not properly captured when using the nextset() method. The fix ensures that all messages are collected, even across multiple result sets, and adds comprehensive tests to verify this behavior.

Bug Fix: Diagnostic Message Handling

  • Updated the nextset() method in cursor.py to capture diagnostic messages (e.g., PRINT output) when SQL returns SQL_SUCCESS_WITH_INFO, ensuring messages from all result sets are preserved.

Testing Improvements

  • Added several new tests in test_004_cursor.py to verify that PRINT messages are correctly captured across multiple result sets and that messages from previous result sets are cleared appropriately:
    • Test for multiple PRINT statements across result sets via nextset()
    • Test for PRINT messages interleaved with SELECT statements
    • Test for three consecutive PRINT messages across nextset() calls
    • Test to ensure nextset() clears messages from the previous result set

Copilot AI review requested due to automatic review settings June 2, 2026 14:42
@github-actions github-actions Bot added the pr-size: medium Moderate update size label Jun 2, 2026
Copy link
Copy Markdown
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 PR fixes a gap in diagnostic message handling when iterating across multiple result sets: Cursor.nextset() now collects diagnostics (e.g., SQL Server PRINT output) when SQLMoreResults returns SQL_SUCCESS_WITH_INFO, ensuring messages from subsequent result sets aren’t silently dropped.

Changes:

  • Update Cursor.nextset() to pull diagnostic records on SQL_SUCCESS_WITH_INFO from SQLMoreResults.
  • Add regression and scenario tests validating PRINT message capture across multiple nextset() calls (including mixed PRINT + SELECT).
  • Add a test asserting nextset() clears prior result-set messages.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
mssql_python/cursor.py Capture diagnostics on SQL_SUCCESS_WITH_INFO during nextset() to preserve PRINT output across result sets.
tests/test_004_cursor.py Add regression tests covering multi-result PRINT diagnostics behavior through nextset().

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

…H-612)

nextset() called DDBCSQLMoreResults but never checked for
SQL_SUCCESS_WITH_INFO, so diagnostic messages (e.g. PRINT output)
from subsequent result sets were silently lost.

Add the same SQL_SUCCESS_WITH_INFO -> DDBCSQLGetAllDiagRecords
pattern already used in execute().

Tests added:
- test_cursor_messages_nextset_multiple_prints (exact repro from issue)
- test_cursor_messages_nextset_print_with_select (mixed PRINT + SELECT)
- test_cursor_messages_nextset_three_prints (3 consecutive PRINTs)
- test_cursor_messages_nextset_clears_previous (clearing semantics)

Closes #612
@jahnvi480 jahnvi480 force-pushed the jahnvi/fix-nextset-messages-612 branch from 31d4f4d to 6e4f388 Compare June 2, 2026 14:47
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

📊 Code Coverage Report

🔥 Diff Coverage

100%


🎯 Overall Coverage

80%


📈 Total Lines Covered: 6656 out of 8258
📁 Project: mssql-python


Diff Coverage

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

  • mssql_python/cursor.py (100%)

Summary

  • Total: 3 lines
  • Missing: 0 lines
  • Coverage: 100%

📋 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: 59.7%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.1%
mssql_python.row.py: 76.9%
mssql_python.__init__.py: 77.3%
mssql_python.pybind.connection.connection.cpp: 77.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.logging.py: 85.5%
mssql_python.connection.py: 85.6%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@KellyKr
Copy link
Copy Markdown

KellyKr commented Jun 3, 2026

I just ran into this and see the fix will be coming. Nice!

To ask for a bit more: the type of cursor.messages is list[Unknown], it would be nice to have correct typing on it.

Comment thread mssql_python/cursor.py Outdated
Comment thread mssql_python/cursor.py Outdated
Comment thread tests/test_004_cursor.py Outdated
Comment thread tests/test_004_cursor.py
Comment thread tests/test_004_cursor.py
Comment thread mssql_python/cursor.py Outdated
…H-612)

nextset() called DDBCSQLMoreResults but never captured diagnostic
messages, so PRINT output from subsequent result sets was silently
lost.

Changes:
- Add _capture_diagnostics() helper to centralise the pattern of
  collecting diagnostic records on SQL_SUCCESS_WITH_INFO and
  SQL_NO_DATA (trailing PRINT after the final result set).
- Use the helper in execute() and nextset().
- Restore nextset() return type to Optional[bool] to keep the door
  open for future PEP-249 alignment (True/None). Current behaviour
  remains True/False for backward compatibility.

Tests added:
- test_cursor_messages_nextset_multiple_prints (exact repro from issue)
- test_cursor_messages_nextset_print_with_select (mixed PRINT + SELECT,
  verifies only nextset-captured messages)
- test_cursor_messages_nextset_three_prints (3 consecutive PRINTs)
- test_cursor_messages_nextset_clears_previous (clearing semantics)
- test_cursor_messages_nextset_trailing_print (SQL_NO_DATA diagnostics)

Closes #612
@sumitmsft
Copy link
Copy Markdown
Contributor

I just ran into this and see the fix will be coming. Nice!

To ask for a bit more: the type of cursor.messages is list[Unknown], it would be nice to have correct typing on it.

@jahnvi480 While we're touching the messages handling, can we also fix the typing? Today self.messages = [] is inferred as list[Unknown] by IDEs and type-checkers.

The C++ binding (SQLGetAllDiagRecords) always returns Tuple[str, str] per entry. (sql_state_with_error_code, message_text) and the tests already assert this shape. So. do you think it's a safe fix?

The C++ binding (DDBCSQLGetAllDiagRecords) always returns
(sql_state_with_error_code, message_text) tuples.  Without the
annotation, IDEs infer list[Unknown].  Existing tests already
assert this shape.
@jahnvi480
Copy link
Copy Markdown
Contributor Author

Thanks @KellyKr and @sumitmsft for detailed review, I have verified and updated the typing for messages.

@jahnvi480 jahnvi480 requested a review from sumitmsft June 5, 2026 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: medium Moderate update size

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants