Skip to content

Commit a4da13a

Browse files
REFACTOR: Enhance & Fix Logs (#137)
### ADO Work Item Reference <!-- Insert your ADO Work Item ID below (e.g. AB#37452) --> > [AB#37336](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/37336) ------------------------------------------------------------------- ### Summary <!-- Insert your Copilot Generated Summary below --> 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](diffhunk://#diff-36f7c1765dd718a86de00178366ca7ab8e503766ca04b601e859d943d82a292dR11-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](diffhunk://#diff-7e002427036075c912554bb92d9e19681ea477e7c2a0eec56d284934d9125343R187-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]](diffhunk://#diff-29bb94de45aae51c23a6426d40133c28e4161e68769e08d046059c7186264e90L129-R125); `mssql_python/cursor.py`, [[2]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280L465-R449); `mssql_python/exceptions.py`, [[3]](diffhunk://#diff-261e8fab7c29f0ba7f4dcdd1f977df5c9f02d3423c86cc935c3f617fd15c2d5fL644-R644) * Removed redundant `logger` initialization from several modules, as the `log` function now handles logger retrieval. (`mssql_python/auth.py`, [[1]](diffhunk://#diff-19a0c93fc8573a5a7bfcadda0a2fb8f1b340c4502e1308c4f8a1e4508136c6e1L10-L14); `mssql_python/cursor.py`, [[2]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280L17-L23) ### 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](diffhunk://#diff-7e002427036075c912554bb92d9e19681ea477e7c2a0eec56d284934d9125343R187-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](diffhunk://#diff-29bb94de45aae51c23a6426d40133c28e4161e68769e08d046059c7186264e90R186)) * Refactored cursor cleanup logic to avoid unnecessary operations and ensure weak references are handled correctly. (`mssql_python/cursor.py`, [mssql_python/cursor.pyL419-R432](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280L419-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]](diffhunk://#diff-29bb94de45aae51c23a6426d40133c28e4161e68769e08d046059c7186264e90L273-R285); `mssql_python/cursor.py`, [[2]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280L750-R737) <!-- ### PR Title Guide > For feature requests FEAT: (short-description) > For non-feature requests like test case updates, config updates , dependency updates etc CHORE: (short-description) > For Fix requests FIX: (short-description) > For doc update requests DOC: (short-description) > For Formatting, indentation, or styling update STYLE: (short-description) > For Refactor, without any feature changes REFACTOR: (short-description) > For release related changes, without any feature changes RELEASE: #<RELEASE_VERSION> (short-description) --> --------- Co-authored-by: Jahnvi Thakkar <61936179+jahnvi480@users.noreply.github.com>
1 parent 2dac682 commit a4da13a

File tree

11 files changed

+411
-147
lines changed

11 files changed

+411
-147
lines changed

mssql_python/auth.py

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,8 @@
77
import platform
88
import struct
99
from typing import Tuple, Dict, Optional, Union
10-
from mssql_python.logging_config import get_logger, ENABLE_LOGGING
1110
from mssql_python.constants import AuthType
1211

13-
logger = get_logger()
14-
1512
class AADAuth:
1613
"""Handles Azure Active Directory authentication"""
1714

mssql_python/connection.py

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,12 @@
1313
import weakref
1414
import re
1515
from mssql_python.cursor import Cursor
16-
from mssql_python.logging_config import get_logger, ENABLE_LOGGING
17-
from mssql_python.constants import ConstantsDDBC as ddbc_sql_const
18-
from mssql_python.helpers import add_driver_to_connection_str, check_error
16+
from mssql_python.helpers import add_driver_to_connection_str, sanitize_connection_string, log
1917
from mssql_python import ddbc_bindings
2018
from mssql_python.pooling import PoolingManager
21-
from mssql_python.exceptions import DatabaseError, InterfaceError
19+
from mssql_python.exceptions import InterfaceError
2220
from mssql_python.auth import process_connection_string
2321

24-
logger = get_logger()
25-
2622

2723
class Connection:
2824
"""
@@ -126,8 +122,7 @@ def _construct_connection_string(self, connection_str: str = "", **kwargs) -> st
126122
continue
127123
conn_str += f"{key}={value};"
128124

129-
if ENABLE_LOGGING:
130-
logger.info("Final connection string: %s", conn_str)
125+
log('info', "Final connection string: %s", sanitize_connection_string(conn_str))
131126

132127
return conn_str
133128

@@ -150,8 +145,7 @@ def autocommit(self, value: bool) -> None:
150145
None
151146
"""
152147
self.setautocommit(value)
153-
if ENABLE_LOGGING:
154-
logger.info("Autocommit mode set to %s.", value)
148+
log('info', "Autocommit mode set to %s.", value)
155149

156150
def setautocommit(self, value: bool = True) -> None:
157151
"""
@@ -189,6 +183,7 @@ def cursor(self) -> Cursor:
189183
)
190184

191185
cursor = Cursor(self)
186+
self._cursors.add(cursor) # Track the cursor
192187
return cursor
193188

194189
def commit(self) -> None:
@@ -205,8 +200,7 @@ def commit(self) -> None:
205200
"""
206201
# Commit the current transaction
207202
self._conn.commit()
208-
if ENABLE_LOGGING:
209-
logger.info("Transaction committed successfully.")
203+
log('info', "Transaction committed successfully.")
210204

211205
def rollback(self) -> None:
212206
"""
@@ -221,8 +215,7 @@ def rollback(self) -> None:
221215
"""
222216
# Roll back the current transaction
223217
self._conn.rollback()
224-
if ENABLE_LOGGING:
225-
logger.info("Transaction rolled back successfully.")
218+
log('info', "Transaction rolled back successfully.")
226219

227220
def close(self) -> None:
228221
"""
@@ -246,20 +239,19 @@ def close(self) -> None:
246239
# Convert to list to avoid modification during iteration
247240
cursors_to_close = list(self._cursors)
248241
close_errors = []
249-
242+
250243
for cursor in cursors_to_close:
251244
try:
252245
if not cursor.closed:
253246
cursor.close()
254247
except Exception as e:
255248
# Collect errors but continue closing other cursors
256249
close_errors.append(f"Error closing cursor: {e}")
257-
if ENABLE_LOGGING:
258-
logger.warning(f"Error closing cursor: {e}")
250+
log('warning', f"Error closing cursor: {e}")
259251

260252
# If there were errors closing cursors, log them but continue
261-
if close_errors and ENABLE_LOGGING:
262-
logger.warning(f"Encountered {len(close_errors)} errors while closing cursors")
253+
if close_errors:
254+
log('warning', f"Encountered {len(close_errors)} errors while closing cursors")
263255

264256
# Clear the cursor set explicitly to release any internal references
265257
self._cursors.clear()
@@ -270,26 +262,24 @@ def close(self) -> None:
270262
self._conn.close()
271263
self._conn = None
272264
except Exception as e:
273-
if ENABLE_LOGGING:
274-
logger.error(f"Error closing database connection: {e}")
265+
log('error', f"Error closing database connection: {e}")
275266
# Re-raise the connection close error as it's more critical
276267
raise
277268
finally:
278269
# Always mark as closed, even if there were errors
279270
self._closed = True
280271

281-
if ENABLE_LOGGING:
282-
logger.info("Connection closed successfully.")
272+
log('info', "Connection closed successfully.")
283273

284274
def __del__(self):
285275
"""
286276
Destructor to ensure the connection is closed when the connection object is no longer needed.
287277
This is a safety net to ensure resources are cleaned up
288278
even if close() was not called explicitly.
289279
"""
290-
if not self._closed:
280+
if "_closed" not in self.__dict__ or not self._closed:
291281
try:
292282
self.close()
293283
except Exception as e:
294-
if ENABLE_LOGGING:
295-
logger.error(f"Error during connection cleanup in __del__: {e}")
284+
# Dont raise exceptions from __del__ to avoid issues during garbage collection
285+
log('error', f"Error during connection cleanup: {e}")

mssql_python/cursor.py

Lines changed: 22 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,11 @@
1414
import datetime
1515
from typing import List, Union
1616
from mssql_python.constants import ConstantsDDBC as ddbc_sql_const
17-
from mssql_python.helpers import check_error
18-
from mssql_python.logging_config import get_logger, ENABLE_LOGGING
17+
from mssql_python.helpers import check_error, log
1918
from mssql_python import ddbc_bindings
19+
from mssql_python.exceptions import InterfaceError
2020
from .row import Row
2121

22-
logger = get_logger()
2322

2423
class Cursor:
2524
"""
@@ -415,36 +414,22 @@ def _initialize_cursor(self) -> None:
415414
"""
416415
Initialize the DDBC statement handle.
417416
"""
418-
# Allocate the DDBC statement handle
419417
self._allocate_statement_handle()
420-
# Add the cursor to the connection's cursor set
421-
self.connection._cursors.add(self)
422418

423419
def _allocate_statement_handle(self):
424420
"""
425421
Allocate the DDBC statement handle.
426422
"""
427423
self.hstmt = self.connection._conn.alloc_statement_handle()
428424

429-
def _free_cursor(self) -> None:
425+
def _reset_cursor(self) -> None:
430426
"""
431-
Free the DDBC statement handle and remove the cursor from the connection's cursor set.
427+
Reset the DDBC statement handle.
432428
"""
433429
if self.hstmt:
434430
self.hstmt.free()
435431
self.hstmt = None
436-
if ENABLE_LOGGING:
437-
logger.debug("SQLFreeHandle succeeded")
438-
# We don't need to remove the cursor from the connection's cursor set here,
439-
# as it is a weak reference and will be automatically removed
440-
# when the cursor is garbage collected.
441-
442-
def _reset_cursor(self) -> None:
443-
"""
444-
Reset the DDBC statement handle.
445-
"""
446-
# Free the current cursor if it exists
447-
self._free_cursor()
432+
log('debug', "SQLFreeHandle succeeded")
448433
# Reinitialize the statement handle
449434
self._initialize_cursor()
450435

@@ -461,8 +446,7 @@ def close(self) -> None:
461446
if self.hstmt:
462447
self.hstmt.free()
463448
self.hstmt = None
464-
if ENABLE_LOGGING:
465-
logger.debug("SQLFreeHandle succeeded")
449+
log('debug', "SQLFreeHandle succeeded")
466450
self.closed = True
467451

468452
def _check_closed(self):
@@ -596,15 +580,14 @@ def execute(
596580
# Executing a new statement. Reset is_stmt_prepared to false
597581
self.is_stmt_prepared = [False]
598582

599-
if ENABLE_LOGGING:
600-
logger.debug("Executing query: %s", operation)
601-
for i, param in enumerate(parameters):
602-
logger.debug(
603-
"""Parameter number: %s, Parameter: %s,
604-
Param Python Type: %s, ParamInfo: %s, %s, %s, %s, %s""",
605-
i + 1,
606-
param,
607-
str(type(param)),
583+
log('debug', "Executing query: %s", operation)
584+
for i, param in enumerate(parameters):
585+
log('debug',
586+
"""Parameter number: %s, Parameter: %s,
587+
Param Python Type: %s, ParamInfo: %s, %s, %s, %s, %s""",
588+
i + 1,
589+
param,
590+
str(type(param)),
608591
parameters_type[i].paramSQLType,
609592
parameters_type[i].paramCType,
610593
parameters_type[i].columnSize,
@@ -709,10 +692,9 @@ def executemany(self, operation: str, seq_of_parameters: list) -> None:
709692
)
710693

711694
columnwise_params = self._transpose_rowwise_to_columnwise(seq_of_parameters)
712-
if ENABLE_LOGGING:
713-
logger.info("Executing batch query with %d parameter sets:\n%s",
714-
len(seq_of_parameters),"\n".join(f" {i+1}: {tuple(p) if isinstance(p, (list, tuple)) else p}" for i, p in enumerate(seq_of_parameters))
715-
)
695+
log('info', "Executing batch query with %d parameter sets:\n%s",
696+
len(seq_of_parameters), "\n".join(f" {i+1}: {tuple(p) if isinstance(p, (list, tuple)) else p}" for i, p in enumerate(seq_of_parameters))
697+
)
716698

717699
# Execute batched statement
718700
ret = ddbc_bindings.SQLExecuteMany(
@@ -784,6 +766,7 @@ def fetchall(self) -> List[Row]:
784766
# Fetch raw data
785767
rows_data = []
786768
ret = ddbc_bindings.DDBCSQLFetchAll(self.hstmt, rows_data)
769+
787770
# Convert raw data to Row objects
788771
return [Row(row_data, self.description) for row_data in rows_data]
789772

@@ -805,15 +788,16 @@ def nextset(self) -> Union[bool, None]:
805788
if ret == ddbc_sql_const.SQL_NO_DATA.value:
806789
return False
807790
return True
808-
791+
809792
def __del__(self):
810793
"""
811794
Destructor to ensure the cursor is closed when it is no longer needed.
812795
This is a safety net to ensure resources are cleaned up
813796
even if close() was not called explicitly.
814797
"""
815-
if not self.closed:
798+
if "_closed" not in self.__dict__ or not self._closed:
816799
try:
817800
self.close()
818801
except Exception as e:
819-
logger.error(f"Error closing cursor: {e}")
802+
# Don't raise an exception in __del__, just log it
803+
log('error', "Error during cursor cleanup in __del__: %s", e)

mssql_python/exceptions.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
This module contains custom exception classes for the mssql_python package.
55
These classes are used to raise exceptions when an error occurs while executing a query.
66
"""
7-
from mssql_python.logging_config import get_logger, ENABLE_LOGGING
7+
from mssql_python.logging_config import get_logger
88

99
logger = get_logger()
1010

@@ -621,7 +621,7 @@ def truncate_error_message(error_message: str) -> str:
621621
string_third = string_second[string_second.index("]") + 1 :]
622622
return string_first + string_third
623623
except Exception as e:
624-
if ENABLE_LOGGING:
624+
if logger:
625625
logger.error("Error while truncating error message: %s",e)
626626
return error_message
627627

@@ -641,7 +641,7 @@ def raise_exception(sqlstate: str, ddbc_error: str) -> None:
641641
"""
642642
exception_class = sqlstate_to_exception(sqlstate, ddbc_error)
643643
if exception_class:
644-
if ENABLE_LOGGING:
644+
if logger:
645645
logger.error(exception_class)
646646
raise exception_class
647647
raise DatabaseError(

mssql_python/helpers.py

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
from mssql_python import ddbc_bindings
88
from mssql_python.exceptions import raise_exception
9-
from mssql_python.logging_config import get_logger, ENABLE_LOGGING
9+
from mssql_python.logging_config import get_logger
1010
import platform
1111
from pathlib import Path
1212
from mssql_python.ddbc_bindings import normalize_architecture
@@ -73,7 +73,7 @@ def check_error(handle_type, handle, ret):
7373
"""
7474
if ret < 0:
7575
error_info = ddbc_bindings.DDBCSQLCheckError(handle_type, handle, ret)
76-
if ENABLE_LOGGING:
76+
if logger:
7777
logger.error("Error: %s", error_info.ddbcErrorMsg)
7878
raise_exception(error_info.sqlState, error_info.ddbcErrorMsg)
7979

@@ -184,3 +184,31 @@ def get_driver_path(module_dir, architecture):
184184
raise RuntimeError(f"ODBC driver not found at: {driver_path_str}")
185185

186186
return driver_path_str
187+
188+
189+
def sanitize_connection_string(conn_str: str) -> str:
190+
"""
191+
Sanitize the connection string by removing sensitive information.
192+
Args:
193+
conn_str (str): The connection string to sanitize.
194+
Returns:
195+
str: The sanitized connection string.
196+
"""
197+
# Remove sensitive information from the connection string, Pwd section
198+
# Replace Pwd=...; or Pwd=... (end of string) with Pwd=***;
199+
import re
200+
return re.sub(r"(Pwd\s*=\s*)[^;]*", r"\1***", conn_str, flags=re.IGNORECASE)
201+
202+
203+
def log(level: str, message: str, *args) -> None:
204+
"""
205+
Universal logging helper that gets a fresh logger instance.
206+
207+
Args:
208+
level: Log level ('debug', 'info', 'warning', 'error')
209+
message: Log message with optional format placeholders
210+
*args: Arguments for message formatting
211+
"""
212+
logger = get_logger()
213+
if logger:
214+
getattr(logger, level)(message, *args)

0 commit comments

Comments
 (0)