FEAT: isolate native handles using smart pointers, eliminate raw pointer access from Python

* initial changes for encapsulating raw ODBC handles

* refactor: isolate native ODBC handles using smart pointers, eliminate raw pointer access from Python

* resolved review comments

* added newline for test in ddbc_bindings.cpp

* fix sqlhandle

* FIX: Fixed Pytests and corner cases

* random

* line endings

* added back PR template

* changed pytest structure

* pipeline steps change for coverage

* removed pytest-azurepipelines from dependency

* fixed coverage generation

* cpp to use lf instead of crlf

* cleanup

* python owns memory

* python owns memory - pipeline reverted

* cleanup and comments

---------

Co-authored-by: Saumya Garg <gargsaumya@microsoft.com>
Co-authored-by: Gaurav Sharma <sharmag@microsoft.com>

Author: 3 people

.gitignore

@@ -3,8 +3,8 @@ mssql_python/pybind/build/
 
 mssql_python/pybind/pymsbuild/build/
 
-# Ignore pyd file
-mssql_python/ddbc_bindings.pyd
+# Ignore all pyd files
+**.pyd
 
 # Ignore pycache files and folders
 __pycache__/
@@ -26,6 +26,11 @@ mssql_python/.vs
 test-*.xml
 **/test-**.xml
 
+# Ignore coverage files
+coverage.xml
+.coverage
+.coverage.*
+
 # Ignore the build & mssql_python.egg-info directories
 build/
 mssql_python.egg-info/

eng/pipelines/test-pipeline.yml

@@ -65,5 +65,5 @@ jobs:
   - task: PublishCodeCoverageResults@1
     inputs:
       codeCoverageTool: 'Cobertura'
-      summaryFileLocation: '$(System.DefaultWorkingDirectory)/**/coverage.xml'
+      summaryFileLocation: 'coverage.xml'
     displayName: 'Publish code coverage results'

mssql_python/connection.py

@@ -53,8 +53,8 @@ def __init__(self, connection_str: str, autocommit: bool = False, **kwargs) -> N
         preparing it for further operations such as connecting to the 
         database, executing queries, etc.
         """
-        self.henv = ctypes.c_void_p()
-        self.hdbc = ctypes.c_void_p()
+        self.henv = None
+        self.hdbc = None
         self.connection_str = self._construct_connection_string(
             connection_str, **kwargs
         )
@@ -95,6 +95,15 @@ def _construct_connection_string(self, connection_str: str, **kwargs) -> str:
             conn_str += f"{key}={value};"
         return conn_str
 
+    def _is_closed(self) -> bool:
+        """
+        Check if the connection is closed.
+
+        Returns:
+            bool: True if the connection is closed, False otherwise.
+        """
+        return self.hdbc is None
+
     def _initializer(self) -> None:
         """
         Initialize the environment and connection handles.
@@ -113,52 +122,48 @@ def _allocate_environment_handle(self):
         """
         Allocate the environment handle.
         """
-        ret = ddbc_bindings.DDBCSQLAllocHandle(
+        ret, handle = ddbc_bindings.DDBCSQLAllocHandle(
             ddbc_sql_const.SQL_HANDLE_ENV.value,  # SQL environment handle type
-            0,  # SQL input handle
-            ctypes.cast(
-                ctypes.pointer(self.henv), ctypes.c_void_p
-            ).value,  # SQL output handle pointer
+            None
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_ENV.value, self.henv.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_ENV.value, handle, ret)
+        self.henv = handle
 
     def _set_environment_attributes(self):
         """
         Set the environment attributes.
         """
         ret = ddbc_bindings.DDBCSQLSetEnvAttr(
-            self.henv.value,  # Environment handle
+            self.henv,  # Use the wrapper class
             ddbc_sql_const.SQL_ATTR_DDBC_VERSION.value,  # Attribute
             ddbc_sql_const.SQL_OV_DDBC3_80.value,  # String Length
             0,  # Null-terminated string
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_ENV.value, self.henv.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_ENV.value, self.henv, ret)
 
     def _allocate_connection_handle(self):
         """
         Allocate the connection handle.
         """
-        ret = ddbc_bindings.DDBCSQLAllocHandle(
+        ret, handle = ddbc_bindings.DDBCSQLAllocHandle(
             ddbc_sql_const.SQL_HANDLE_DBC.value,  # SQL connection handle type
-            self.henv.value,  # SQL environment handle
-            ctypes.cast(
-                ctypes.pointer(self.hdbc), ctypes.c_void_p
-            ).value,  # SQL output handle pointer
+            self.henv
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, handle, ret)
+        self.hdbc = handle
 
     def _set_connection_attributes(self):
         """
         Set the connection attributes before connecting.
         """
         if self.autocommit:
             ret = ddbc_bindings.DDBCSQLSetConnectAttr(
-                self.hdbc.value,
+                self.hdbc,  # Using the wrapper class
                 ddbc_sql_const.SQL_ATTR_AUTOCOMMIT.value,
                 ddbc_sql_const.SQL_AUTOCOMMIT_ON.value,
-                0,
+                0
             )
-            check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+            check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc, ret)
 
     def _connect_to_db(self) -> None:
         """
@@ -176,11 +181,11 @@ def _connect_to_db(self) -> None:
         if ENABLE_LOGGING:
             logger.info("Connecting to the database")
         ret = ddbc_bindings.DDBCSQLDriverConnect(
-            self.hdbc.value,  # Connection handle
+            self.hdbc,  # Connection handle (wrapper)
             0,  # Window handle
             self.connection_str,  # Connection string
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc, ret)
         if ENABLE_LOGGING:
             logger.info("Connection established successfully.")
 
@@ -192,11 +197,11 @@ def autocommit(self) -> bool:
             bool: True if autocommit is enabled, False otherwise.
         """
         autocommit_mode = ddbc_bindings.DDBCSQLGetConnectionAttr(
-            self.hdbc.value,  # Connection handle
+            self.hdbc,  # Connection handle (wrapper)
             ddbc_sql_const.SQL_ATTR_AUTOCOMMIT.value,  # Attribute
         )
         check_error(
-            ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, autocommit_mode
+            ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc, autocommit_mode
         )
         return autocommit_mode == ddbc_sql_const.SQL_AUTOCOMMIT_ON.value
 
@@ -212,7 +217,7 @@ def autocommit(self, value: bool) -> None:
             DatabaseError: If there is an error while setting the autocommit mode.
         """
         ret = ddbc_bindings.DDBCSQLSetConnectAttr(
-            self.hdbc.value,  # Connection handle
+            self.hdbc,  # Connection handle
             ddbc_sql_const.SQL_ATTR_AUTOCOMMIT.value,  # Attribute
             (
                 ddbc_sql_const.SQL_AUTOCOMMIT_ON.value
@@ -221,7 +226,7 @@ def autocommit(self, value: bool) -> None:
             ),  # Value
             0,  # String length
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc, ret)
         self._autocommit = value
         if ENABLE_LOGGING:
             logger.info("Autocommit mode set to %s.", value)
@@ -253,6 +258,9 @@ def cursor(self) -> Cursor:
             DatabaseError: If there is an error while creating the cursor.
             InterfaceError: If there is an error related to the database interface.
         """
+        if self._is_closed():
+            # Cannot create a cursor if the connection is closed
+            raise Exception("Connection is closed. Cannot create cursor.")
         return Cursor(self)
 
     def commit(self) -> None:
@@ -267,13 +275,17 @@ def commit(self) -> None:
         Raises:
             DatabaseError: If there is an error while committing the transaction.
         """
+        if self._is_closed():
+            # Cannot commit if the connection is closed
+            raise Exception("Connection is closed. Cannot commit.")
+
         # Commit the current transaction
         ret = ddbc_bindings.DDBCSQLEndTran(
             ddbc_sql_const.SQL_HANDLE_DBC.value,  # Handle type
-            self.hdbc.value,  # Connection handle
+            self.hdbc,  # Connection handle (wrapper)
             ddbc_sql_const.SQL_COMMIT.value,  # Commit the transaction
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc, ret)
         if ENABLE_LOGGING:
             logger.info("Transaction committed successfully.")
 
@@ -288,13 +300,17 @@ def rollback(self) -> None:
         Raises:
             DatabaseError: If there is an error while rolling back the transaction.
         """
+        if self._is_closed():
+            # Cannot roll back if the connection is closed
+            raise Exception("Connection is closed. Cannot roll back.")
+
         # Roll back the current transaction
         ret = ddbc_bindings.DDBCSQLEndTran(
             ddbc_sql_const.SQL_HANDLE_DBC.value,  # Handle type
-            self.hdbc.value,  # Connection handle
+            self.hdbc,  # Connection handle (wrapper)
             ddbc_sql_const.SQL_ROLLBACK.value,  # Roll back the transaction
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc, ret)
         if ENABLE_LOGGING:
             logger.info("Transaction rolled back successfully.")
 
@@ -311,15 +327,16 @@ def close(self) -> None:
         Raises:
             DatabaseError: If there is an error while closing the connection.
         """
+        if self._is_closed():
+            # Connection is already closed
+            return
         # Disconnect from the database
-        ret = ddbc_bindings.DDBCSQLDisconnect(self.hdbc.value)
-        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+        ret = ddbc_bindings.DDBCSQLDisconnect(self.hdbc)
+        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc, ret)
 
-        # Free the connection handle
-        ret = ddbc_bindings.DDBCSQLFreeHandle(
-            ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value
-        )
-        check_error(ddbc_sql_const.SQL_HANDLE_DBC.value, self.hdbc.value, ret)
+        # Set the reference to None to trigger destructor
+        self.hdbc.free()
+        self.hdbc = None
 
         if ENABLE_LOGGING:
             logger.info("Connection closed successfully.")

mssql_python/cursor.py

@@ -48,9 +48,11 @@ def __init__(self, connection) -> None:
         Args:
             connection: Database connection object.
         """
+        if connection.hdbc is None:
+            raise Exception("Connection is closed. Cannot create a cursor.")
         self.connection = connection
         # self.connection.autocommit = False
-        self.hstmt = ctypes.c_void_p()
+        self.hstmt = None
         self._initialize_cursor()
         self.description = None
         self.rowcount = -1
@@ -415,22 +417,22 @@ def _allocate_statement_handle(self):
         """
         Allocate the DDBC statement handle.
         """
-        ret = ddbc_bindings.DDBCSQLAllocHandle(
+        ret, handle = ddbc_bindings.DDBCSQLAllocHandle(
             ddbc_sql_const.SQL_HANDLE_STMT.value,
-            self.connection.hdbc.value,
-            ctypes.cast(ctypes.pointer(self.hstmt), ctypes.c_void_p).value,
+            self.connection.hdbc
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, handle, ret)
+        self.hstmt = handle
 
     def _reset_cursor(self) -> None:
         """
         Reset the DDBC statement handle.
         """
-        # Free the existing statement handle
-        if self.hstmt.value:
-            ddbc_bindings.DDBCSQLFreeHandle(
-                ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value
-            )
+        if self.hstmt:
+            self.hstmt.free()  # Free the existing statement handle
+            self.hstmt = None
+            if ENABLE_LOGGING:
+                logger.debug("SQLFreeHandle succeeded")     
         # Reinitialize the statement handle
         self._initialize_cursor()
 
@@ -442,15 +444,13 @@ def close(self) -> None:
             Error: If any operation is attempted with the cursor after it is closed.
         """
         if self.closed:
-            raise RuntimeError("Cursor is already closed.")
-
-        if self.hstmt.value:
-            ret = ddbc_bindings.DDBCSQLFreeHandle(
-                ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value
-            )
-            check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
-            self.hstmt.value = None
+            raise Exception("Cursor is already closed.")
 
+        if self.hstmt:
+            self.hstmt.free()
+            self.hstmt = None
+            if ENABLE_LOGGING:
+                logger.debug("SQLFreeHandle succeeded")
         self.closed = True
 
     def _check_closed(self):
@@ -461,7 +461,7 @@ def _check_closed(self):
             Error: If the cursor is closed.
         """
         if self.closed:
-            raise RuntimeError("Operation cannot be performed: the cursor is closed.")
+            raise Exception("Operation cannot be performed: the cursor is closed.")
 
     def _create_parameter_types_list(self, parameter, param_info, parameters_list, i):
         """
@@ -489,8 +489,8 @@ def _initialize_description(self):
         Initialize the description attribute using SQLDescribeCol.
         """
         col_metadata = []
-        ret = ddbc_bindings.DDBCSQLDescribeCol(self.hstmt.value, col_metadata)
-        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
+        ret = ddbc_bindings.DDBCSQLDescribeCol(self.hstmt, col_metadata)
+        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
 
         self.description = [
             (
@@ -602,19 +602,19 @@ def execute(
                 )
 
         ret = ddbc_bindings.DDBCSQLExecute(
-            self.hstmt.value,
+            self.hstmt,
             operation,
             parameters,
             parameters_type,
             self.is_stmt_prepared,
             use_prepare,
         )
-        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
+        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
         self.last_executed_stmt = operation
 
         # Update rowcount after execution
         # TODO: rowcount return code from SQL needs to be handled
-        self.rowcount = ddbc_bindings.DDBCSQLRowCount(self.hstmt.value)
+        self.rowcount = ddbc_bindings.DDBCSQLRowCount(self.hstmt)
 
         # Initialize description after execution
         self._initialize_description()
@@ -664,8 +664,8 @@ def fetchone(self) -> Union[None, tuple]:
         self._check_closed()  # Check if the cursor is closed
 
         row = []
-        ret = ddbc_bindings.DDBCSQLFetchOne(self.hstmt.value, row)
-        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
+        ret = ddbc_bindings.DDBCSQLFetchOne(self.hstmt, row)
+        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
         if ret == ddbc_sql_const.SQL_NO_DATA.value:
             return None
         return list(row)
@@ -690,8 +690,8 @@ def fetchmany(self, size: int = None) -> List[tuple]:
 
         # Fetch the next set of rows
         rows = []
-        ret = ddbc_bindings.DDBCSQLFetchMany(self.hstmt.value, rows, size)
-        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
+        ret = ddbc_bindings.DDBCSQLFetchMany(self.hstmt, rows, size)
+        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
         if ret == ddbc_sql_const.SQL_NO_DATA.value:
             return []
         return rows
@@ -710,8 +710,8 @@ def fetchall(self) -> List[tuple]:
 
         # Fetch all remaining rows
         rows = []
-        ret = ddbc_bindings.DDBCSQLFetchAll(self.hstmt.value, rows)
-        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
+        ret = ddbc_bindings.DDBCSQLFetchAll(self.hstmt, rows)
+        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
         if ret != ddbc_sql_const.SQL_NO_DATA.value:
             return []
         return list(rows)
@@ -729,8 +729,8 @@ def nextset(self) -> Union[bool, None]:
         self._check_closed()  # Check if the cursor is closed
 
         # Skip to the next result set
-        ret = ddbc_bindings.DDBCSQLMoreResults(self.hstmt.value)
-        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt.value, ret)
+        ret = ddbc_bindings.DDBCSQLMoreResults(self.hstmt)
+        check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
         if ret == ddbc_sql_const.SQL_NO_DATA.value:
             return False
         return True