Skip to content

Conversation

@bewithgaurav
Copy link
Collaborator

@bewithgaurav bewithgaurav commented Jul 14, 2025

ADO Work Item Reference

AB#37336


Summary

This pull request refactors the logging system in the mssql_python package, replacing the previous global logging mechanism with a centralized LoggingManager class. It also introduces a universal logging helper function (log) and a utility to sanitize sensitive information in connection strings. Additionally, it simplifies the codebase by removing redundant logging checks and improving resource management for cursors and connections.

Logging System Refactor:

  • Introduced LoggingManager as a singleton class to manage logging configuration, replacing the global ENABLE_LOGGING variable. The class centralizes logging setup and provides backward compatibility for checking if logging is enabled. (mssql_python/logging_config.py, mssql_python/logging_config.pyR11-R52)
  • Added a universal log function in helpers.py to streamline logging calls across the codebase. This function dynamically retrieves a logger instance and supports multiple log levels. (mssql_python/helpers.py, mssql_python/helpers.pyR187-R214)

Code Simplification:

  • Replaced conditional ENABLE_LOGGING checks with calls to the new log function, simplifying logging logic in connection.py, cursor.py, and exceptions.py. (mssql_python/connection.py, [1]; mssql_python/cursor.py, [2]; mssql_python/exceptions.py, [3]
  • Removed redundant logger initialization from several modules, as the log function now handles logger retrieval. (mssql_python/auth.py, [1]; mssql_python/cursor.py, [2]

New Utilities:

  • Added sanitize_connection_string in helpers.py to mask sensitive information (e.g., passwords) in connection strings before logging. (mssql_python/helpers.py, mssql_python/helpers.pyR187-R214)

Resource Management Improvements:

  • Improved cursor tracking by explicitly adding cursors to a connection's cursor set and ensuring proper cleanup during resource deallocation. (mssql_python/connection.py, mssql_python/connection.pyR186)
  • Refactored cursor cleanup logic to avoid unnecessary operations and ensure weak references are handled correctly. (mssql_python/cursor.py, mssql_python/cursor.pyL419-R432)

Minor Enhancements:

  • Updated __del__ methods in connection.py and cursor.py to handle exceptions gracefully during cleanup, avoiding issues during garbage collection. (mssql_python/connection.py, [1]; mssql_python/cursor.py, [2]

Copilot AI review requested due to automatic review settings July 14, 2025 13:30
@github-actions github-actions bot added the pr-size: medium Moderate update size label Jul 14, 2025
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 PR refactors the logging system by replacing the global ENABLE_LOGGING flag with a singleton LoggingManager, centralizes logger setup with file rotation and timestamped filenames, and sanitizes connection strings. It also updates logging calls across modules to use the new manager, enhances the C++ binding logging function with formatting and error handling, and adjusts driver path logic for Windows compatibility.

  • Introduce LoggingManager singleton to manage logging configuration
  • Replace ENABLE_LOGGING checks with logger existence and use new sanitize_connection_string
  • Update C++ LOG function and Windows driver path logic

Reviewed Changes

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

Show a summary per file
File Description
tests/test_006_logging.py Updated tests to use LoggingManager and removed global flag
mssql_python/logging_config.py Added LoggingManager class, removed ENABLE_LOGGING, wrapped exports
mssql_python/helpers.py Removed global flag import, added sanitize_connection_string
mssql_python/exceptions.py Replaced global flag checks with logger existence
mssql_python/cursor.py Replaced global flag checks with logger existence
mssql_python/connection.py Initialized logger at import, use sanitize_connection_string
mssql_python/pybind/ddbc_bindings.cpp Enhanced LOG templated function and updated Windows driver path logic
Comments suppressed due to low confidence (3)

mssql_python/helpers.py:189

  • The new sanitize_connection_string function lacks unit tests. Add tests to verify that sensitive fields like Pwd are correctly masked.
def sanitize_connection_string(conn_str: str) -> str:

mssql_python/helpers.py:76

  • The logger variable is not defined in this module, resulting in a NameError. You should call logger = get_logger() at the top or fetch a logger inside the function.
        if logger:

mssql_python/cursor.py:434

  • The logger variable is not defined in this module. Define logger = get_logger() at the top or retrieve the logger in scope before using it.
            if logger:

@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Jul 14, 2025
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Jul 16, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: medium Moderate update size labels Jul 16, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 16, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 16, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 16, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 16, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 16, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: medium Moderate update size labels Jul 17, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 17, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 17, 2025
gargsaumya
gargsaumya previously approved these changes Jul 17, 2025
jahnvi480
jahnvi480 previously approved these changes Jul 17, 2025
@bewithgaurav bewithgaurav dismissed stale reviews from jahnvi480 and gargsaumya via d58c71c July 17, 2025 07:56
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 17, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 17, 2025
@github-actions github-actions bot added pr-size: large Substantial code update and removed pr-size: large Substantial code update labels Jul 17, 2025
@jahnvi480 jahnvi480 merged commit a4da13a into main Jul 17, 2025
16 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