Skip to content

FEAT: Context manager support for connection and cursor class #160

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

jahnvi480
Copy link
Contributor

@jahnvi480 jahnvi480 commented Aug 4, 2025

Work Item / Issue Reference

AB#38074


Summary

This pull request introduces context manager support for the Connection and Cursor classes in the mssql_python module, aligning their behavior with pyodbc. It also adds extensive tests to ensure correct functionality, covering various scenarios such as normal exits, exceptions, nested transactions, and manual commit/rollback. The key changes are grouped below:

Context Manager Support

  • Added __enter__ and __exit__ methods to the Connection class to enable usage with the with statement. The connection commits transactions on normal exit and rolls back on exceptions, without closing the connection. [1] [2]
  • Added __enter__ and __exit__ methods to the Cursor class to allow usage with the with statement. The cursor commits transactions on normal exit but does not close itself, aligning with pyodbc behavior.

Logging Enhancements

  • Enhanced the Connection.close method to log when uncommitted changes are rolled back before closing.

Test Coverage

  • Added new tests in tests/test_003_connection.py to verify context manager behavior for connections, including:
    • Committing transactions on normal exit.
    • Rolling back transactions on exceptions.
    • Handling autocommit mode.
    • Ensuring connections remain open after context exit.
    • Supporting nested context managers.
    • Testing manual commit/rollback within a context manager. [1] [2]
  • Added a test for using contextlib.closing with connections to ensure proper closure after context exit.
  • Updated tests/test_004_cursor.py to include contextlib.closing for cursor tests.

@github-actions github-actions bot added the pr-size: large Substantial code update label Aug 4, 2025
@LarnuUK
Copy link

LarnuUK commented Aug 4, 2025

Why, out of interest @jahnvi480 , should the context managers not close the connection/cursor? I am aware that you state that this aligns with pyodbc but is that the right choice? The behaviour, honestly, feels more like a "gotcha", as often it would be expected that the connection/cursor would be disposed of when the context manage is closed. Look, for example, at Psycopg 3 which changed their default behaviour to close to the connection. I actually think that the context managers closing the objects implicitly would be far better. If people don't want them closed like that, then they (likely) wouldn't use a context manager.

@jahnvi480 jahnvi480 changed the base branch from jahnvi/connection_pyodbc to main August 6, 2025 08:33
@Copilot Copilot AI review requested due to automatic review settings August 6, 2025 09:27
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 6, 2025
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 adds context manager support (__enter__ and __exit__ methods) to both the Connection and Cursor classes, enabling their use with Python's with statement. The implementation follows pyodbc behavior where context managers commit transactions on normal exit and rollback on exceptions, but do not close the connection/cursor itself.

Key changes:

  • Context manager methods added to both Connection and Cursor classes with transaction management
  • Enhanced logging in Connection.close method to track rollback operations
  • Comprehensive test coverage for various context manager scenarios including autocommit modes, exceptions, and nested usage

Reviewed Changes

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

File Description
mssql_python/connection.py Adds __enter__ and __exit__ methods with transaction commit/rollback logic and enhanced logging
mssql_python/cursor.py Adds __enter__ and __exit__ methods that delegate transaction management to the connection
tests/test_003_connection.py Comprehensive context manager tests for connections including edge cases and contextlib.closing usage
tests/test_004_cursor.py Extensive cursor context manager tests covering various scenarios and error conditions

@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 6, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 6, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Aug 6, 2025
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: large Substantial code update labels Aug 6, 2025
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 6, 2025
@roganjoshp
Copy link

I have to strongly agree with @LarnuUK here. In a past job, I was so convinced by the context-manager behaviour of Python that I persuaded my whole team to do a code review and change all of their connections to use a context manager.

These idle connections linger for no purpose at all that I can see. The server will clear them (over long periods of time), but the more software that's running over the DB, the more they start to accumulate. Depending on the RDBMS you're using, it's when you hit ~500 that things will go south. In this case, my decision ended up crashing our central Redshift cluster and brought down our services for more than 10 clients and launched a major issue.

As mentioned previously; psycopg3 changed the default behaviour of __exit__ to also cover closing the connection, which psycopg2 did not do. Although I know that the traditional DB API suggested that this behaviour exists, I am yet to understand any utility that it adds. I can only see this as a flaw in the design of the context manager. Please reconsider following the pyodbc design and instead follow the example of psycopg3.

@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 7, 2025
@sumitmsft
Copy link
Contributor

sumitmsft commented Aug 7, 2025

Thanks @LarnuUK & @roganjoshp for your invaluable feedback and comments. This PR is still under review and our team was still investigating these points in detail. Having said that, we will close connection automatically upon __exit__() as default behavior of this driver.

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.

6 participants