Correct handling of binding closed cursors as REF CURSORS (#368);

variables containing cursors, LOBs or DbObject values now return the
same instances when calling getvalue(), matching Thin mode behavior.
Previously, new instances were created for each call in Thick mode.

Author: anthony-tuininga

doc/src/release_notes.rst

@@ -17,12 +17,24 @@ oracledb 2.4.0 (TBD)
 Thin Mode Changes
 +++++++++++++++++
 
+#)  Fixed bug resulting in a segfault when a closed cursor is bound as a REF
+    CURSOR.
+
 Thick Mode Changes
 ++++++++++++++++++
 
+#)  Variables containing cursors, LOBs or DbObject values now return the same
+    instances when calling :meth:`Variable.getvalue()`, matching Thin mode
+    behavior. Previously, new instances were created for each call in Thick
+    mode.
+
 Common Changes
 ++++++++++++++
 
+#)  Error ``DPY-1006: cursor is not open`` is now raised consistently when
+    attempting to bind a closed cursor. Previously, thin mode would result in a
+    segfault and thick mode would result in unusual errors.
+
 
 oracledb 2.3.0 (July 2024)
 --------------------------

src/oracledb/base_impl.pxd

@@ -643,6 +643,7 @@ cdef class BaseVarImpl:
         BaseConnImpl _conn_impl
         int _preferred_num_type
         FetchInfoImpl _fetch_info
+        list _values
         bint _is_value_set
 
     cdef int _bind(self, object conn, BaseCursorImpl cursor,

src/oracledb/impl/base/connection.pyx

@@ -133,6 +133,7 @@ cdef class BaseConnImpl:
                 return value
         elif db_type_num == DB_TYPE_NUM_CURSOR:
             if isinstance(value, (PY_TYPE_CURSOR, PY_TYPE_ASYNC_CURSOR)):
+                value._verify_open()
                 return value
         elif db_type_num == DB_TYPE_NUM_BOOLEAN:
             return bool(value)

src/oracledb/impl/thick/utils.pyx

@@ -104,9 +104,8 @@ cdef object _convert_from_json_node(dpiJsonNode *node):
 
 cdef int _convert_from_python(object value, DbType dbtype,
                               ThickDbObjectTypeImpl obj_type_impl,
-                              dpiDataBuffer *dbvalue, StringBuffer buf,
-                              ThickVarImpl var_impl=None,
-                              uint32_t pos=0) except -1:
+                              dpiDataBuffer *dbvalue,
+                              StringBuffer buf) except -1:
     cdef:
         uint32_t oracle_type = dbtype.num
         ThickDbObjectImpl obj_impl
@@ -183,24 +182,14 @@ cdef int _convert_from_python(object value, DbType dbtype,
         if not isinstance(value, PY_TYPE_DB_OBJECT):
             raise TypeError("expecting DbObject")
         obj_impl = <ThickDbObjectImpl> value._impl
-        if var_impl is not None:
-            if dpiVar_setFromObject(var_impl._handle, pos,
-                                    obj_impl._handle) < 0:
-                _raise_from_odpi()
-        else:
-            dbvalue.asObject = obj_impl._handle
+        dbvalue.asObject = obj_impl._handle
     elif oracle_type == DPI_ORACLE_TYPE_CLOB \
             or oracle_type == DPI_ORACLE_TYPE_BLOB \
             or oracle_type == DPI_ORACLE_TYPE_NCLOB \
             or oracle_type == DPI_ORACLE_TYPE_BFILE:
         if isinstance(value, PY_TYPE_LOB):
             lob_impl = value._impl
-            if var_impl is not None:
-                if dpiVar_setFromLob(var_impl._handle, pos,
-                                     lob_impl._handle) < 0:
-                    _raise_from_odpi()
-            else:
-                dbvalue.asLOB = lob_impl._handle
+            dbvalue.asLOB = lob_impl._handle
         else:
             buf.set_value(value)
             dbvalue.asBytes.ptr = buf.ptr

src/oracledb/impl/thick/var.pyx

@@ -49,8 +49,11 @@ cdef class ThickVarImpl(BaseVarImpl):
             dpiDataBuffer *dbvalue
             const char *name_ptr
             bytes name_bytes
+            object cursor
         if self.dbtype.num == DB_TYPE_NUM_CURSOR:
-            for i in range(self.num_elements):
+            for i, cursor in enumerate(self._values):
+                if cursor is not None and cursor._impl is None:
+                    errors._raise_err(errors.ERR_CURSOR_NOT_OPEN)
                 if self._data[i].isNull:
                     continue
                 dbvalue = &self._data[i].value
@@ -93,6 +96,12 @@ cdef class ThickVarImpl(BaseVarImpl):
         Internal method that finalizes initialization of the variable.
         """
         BaseVarImpl._finalize_init(self)
+        if self.dbtype._native_num in (
+            DPI_NATIVE_TYPE_LOB,
+            DPI_NATIVE_TYPE_OBJECT,
+            DPI_NATIVE_TYPE_STMT,
+        ):
+            self._values = [None] * self.num_elements
         self._create_handle()
 
     cdef list _get_array_value(self):
@@ -103,18 +112,66 @@ cdef class ThickVarImpl(BaseVarImpl):
         return [self._get_scalar_value(i) \
                 for i in range(self.num_elements_in_array)]
 
-    cdef object _get_cursor_value(self, dpiDataBuffer *dbvalue):
+    cdef object _get_cursor_value(self, dpiDataBuffer *dbvalue, uint32_t pos):
+        """
+        Returns the cursor stored in the variable at the given position. If a
+        cursor was previously available, use it instead of creating a new one.
+        """
         cdef:
             ThickCursorImpl cursor_impl
             object cursor
-        cursor = self._conn.cursor()
+        cursor = self._values[pos]
+        if cursor is None:
+            cursor = self._values[pos] = self._conn.cursor()
         cursor_impl = <ThickCursorImpl> cursor._impl
         if dpiStmt_addRef(dbvalue.asStmt) < 0:
             _raise_from_odpi()
         cursor_impl._handle = dbvalue.asStmt
         cursor_impl._fixup_ref_cursor = True
         return cursor
 
+    cdef object _get_dbobject_value(self, dpiDataBuffer *dbvalue,
+                                    uint32_t pos):
+        """
+        Returns the DbObject stored in the variable at the given position. If a
+        DbObject was previously available, use it instead of creating a new
+        one, unless the handle has changed.
+        """
+        cdef:
+            ThickDbObjectImpl obj_impl
+            object obj
+        obj = self._values[pos]
+        if obj is not None:
+            obj_impl = <ThickDbObjectImpl> obj._impl
+            if obj_impl._handle == dbvalue.asObject:
+                return obj
+        obj_impl = ThickDbObjectImpl.__new__(ThickDbObjectImpl)
+        obj_impl.type = self.objtype
+        if dpiObject_addRef(dbvalue.asObject) < 0:
+            _raise_from_odpi()
+        obj_impl._handle = dbvalue.asObject
+        obj = self._values[pos] = PY_TYPE_DB_OBJECT._from_impl(obj_impl)
+        return obj
+
+    cdef object _get_lob_value(self, dpiDataBuffer *dbvalue, uint32_t pos):
+        """
+        Returns the LOB stored in the variable at the given position. If a LOB
+        was previously created, use it instead of creating a new one, unless
+        the handle has changed.
+        """
+        cdef:
+            ThickLobImpl lob_impl
+            object lob
+        lob = self._values[pos]
+        if lob is not None:
+            lob_impl = <ThickLobImpl> lob._impl
+            if lob_impl._handle == dbvalue.asLOB:
+                return lob
+        lob_impl = ThickLobImpl._create(self._conn_impl, self.dbtype,
+                                        dbvalue.asLOB)
+        lob = self._values[pos] = PY_TYPE_LOB._from_impl(lob_impl)
+        return lob
+
     cdef object _get_scalar_value(self, uint32_t pos):
         """
         Internal method to return the value of the variable at the given
@@ -182,6 +239,10 @@ cdef class ThickVarImpl(BaseVarImpl):
             dpiVar_release(orig_handle)
 
     cdef int _set_cursor_value(self, object cursor, uint32_t pos) except -1:
+        """
+        Sets a cursor value in the variable. If the cursor does not have a
+        statement handle already, associate the one created by the variable.
+        """
         cdef:
             ThickCursorImpl cursor_impl = cursor._impl
             dpiData *data
@@ -197,9 +258,30 @@ cdef class ThickVarImpl(BaseVarImpl):
             if dpiStmt_addRef(data.value.asStmt) < 0:
                 _raise_from_odpi()
             cursor_impl._handle = data.value.asStmt
+        self._values[pos] = cursor
         cursor_impl._fixup_ref_cursor = True
         cursor_impl.statement = None
 
+    cdef int _set_dbobject_value(self, object obj, uint32_t pos) except -1:
+        """
+        Sets an object value in the variable. The object is retained so that
+        multiple calls to getvalue() return the same instance.
+        """
+        cdef ThickDbObjectImpl obj_impl = <ThickDbObjectImpl> obj._impl
+        if dpiVar_setFromObject(self._handle, pos, obj_impl._handle) < 0:
+            _raise_from_odpi()
+        self._values[pos] = obj
+
+    cdef int _set_lob_value(self, object lob, uint32_t pos) except -1:
+        """
+        Sets a LOB value in the variable. The LOB is retained so that multiple
+        calls to getvalue() return the same instance.
+        """
+        cdef ThickLobImpl lob_impl = <ThickLobImpl> lob._impl
+        if dpiVar_setFromLob(self._handle, pos, lob_impl._handle) < 0:
+            _raise_from_odpi()
+        self._values[pos] = lob
+
     cdef int _set_num_elements_in_array(self, uint32_t num_elements) except -1:
         """
         Sets the number of elements in the array.
@@ -217,26 +299,26 @@ cdef class ThickVarImpl(BaseVarImpl):
             dpiDataBuffer temp_dbvalue
             dpiDataBuffer *dbvalue
             dpiBytes *as_bytes
-            bint needs_set
             dpiData *data
         data = &self._data[pos]
         data.isNull = (value is None)
         if not data.isNull:
-            if self.dbtype.num == DB_TYPE_NUM_CURSOR:
+            if self.dbtype._native_num == DPI_NATIVE_TYPE_STMT:
                 self._set_cursor_value(value, pos)
+            elif self.dbtype._native_num == DPI_NATIVE_TYPE_LOB:
+                self._set_lob_value(value, pos)
+            elif self.dbtype._native_num == DPI_NATIVE_TYPE_OBJECT:
+                self._set_dbobject_value(value, pos)
             else:
-                needs_set = self.dbtype._native_num == DPI_NATIVE_TYPE_BYTES \
-                        or (self.dbtype._native_num == DPI_NATIVE_TYPE_LOB \
-                                and not isinstance(value, PY_TYPE_LOB))
-                if needs_set:
+                if self.dbtype._native_num == DPI_NATIVE_TYPE_BYTES:
                     dbvalue = &temp_dbvalue
                 else:
                     dbvalue = &data.value
                 if self._buf is None:
                     self._buf = StringBuffer.__new__(StringBuffer)
                 _convert_from_python(value, self.dbtype, self.objtype, dbvalue,
-                                     self._buf, self, pos)
-                if needs_set:
+                                     self._buf)
+                if self.dbtype._native_num == DPI_NATIVE_TYPE_BYTES:
                     as_bytes = &dbvalue.asBytes
                     if dpiVar_setFromBytes(self._handle, pos, as_bytes.ptr,
                                            as_bytes.length) < 0:
@@ -270,15 +352,20 @@ cdef class ThickVarImpl(BaseVarImpl):
             object value
         data = &data[pos]
         if not data.isNull:
-            if self.dbtype.num == DB_TYPE_NUM_CURSOR:
-                return self._get_cursor_value(&data.value)
-            if self.encoding_errors is not None:
-                encoding_errors_bytes = self.encoding_errors.encode()
-                encoding_errors = encoding_errors_bytes
-            value = _convert_to_python(self._conn_impl, self.dbtype,
-                                       self.objtype, &data.value,
-                                       self._preferred_num_type,
-                                       self.bypass_decode, encoding_errors)
+            if self.dbtype._native_num == DPI_NATIVE_TYPE_STMT:
+                value = self._get_cursor_value(&data.value, pos)
+            elif self.dbtype._native_num == DPI_NATIVE_TYPE_LOB:
+                value = self._get_lob_value(&data.value, pos)
+            elif self.dbtype._native_num == DPI_NATIVE_TYPE_OBJECT:
+                value = self._get_dbobject_value(&data.value, pos)
+            else:
+                if self.encoding_errors is not None:
+                    encoding_errors_bytes = self.encoding_errors.encode()
+                    encoding_errors = encoding_errors_bytes
+                value = _convert_to_python(self._conn_impl, self.dbtype,
+                                           self.objtype, &data.value,
+                                           self._preferred_num_type,
+                                           self.bypass_decode, encoding_errors)
             if self.outconverter is not None:
                 value = self.outconverter(value)
             return value

src/oracledb/impl/thin/messages.pyx

@@ -1082,6 +1082,8 @@ cdef class MessageWithData(Message):
             buf.write_binary_float(value)
         elif ora_type_num == TNS_DATA_TYPE_CURSOR:
             cursor_impl = value._impl
+            if cursor_impl is None:
+                errors._raise_err(errors.ERR_CURSOR_NOT_OPEN)
             if cursor_impl._statement is None:
                 cursor_impl._statement = self.conn_impl._get_statement()
             if cursor_impl._statement._cursor_id == 0:

src/oracledb/impl/thin/var.pyx

@@ -31,7 +31,6 @@
 
 cdef class ThinVarImpl(BaseVarImpl):
     cdef:
-        list _values
         object _conv_func
         object _last_raw_value
 

tests/test_1300_cursor_var.py

@@ -230,6 +230,7 @@ def type_handler(cursor, metadata):
             )
         ref_cursor = var.getvalue()
         self.assertEqual(ref_cursor.fetchall(), [(string_val,)])
+        self.assertIs(var.getvalue(), ref_cursor)
 
     def test_1310(self):
         "1310 - bind a REF cursor but never open it"
@@ -400,6 +401,47 @@ def test_1315(self):
             ],
         )
 
+    def test_1316(self):
+        "1316 - test using a closed ref cursor for OUT bind"
+        value = "test 1316a"
+        sql = """
+            declare
+                t_Cursor sys_refcursor;
+            begin
+                open t_Cursor for
+                    select :value
+                    from dual;
+                :cursor := t_Cursor;
+            end;
+        """
+        var = self.cursor.var(oracledb.DB_TYPE_CURSOR)
+        self.cursor.execute(sql, [value, var])
+        ref_cursor = var.getvalue()
+        self.assertEqual(ref_cursor.fetchall(), [(value,)])
+        ref_cursor.close()
+        with self.assertRaisesFullCode("DPY-1006"):
+            self.cursor.execute(sql, [value, var])
+
+    def test_1317(self):
+        "1317 - test binding a closed cursor"
+        ref_cursor = self.conn.cursor()
+        ref_cursor.close()
+        with self.assertRaisesFullCode("DPY-1006"):
+            self.cursor.callproc(
+                "pkg_testRefCursors.TestInCursor", [ref_cursor]
+            )
+
+    def test_1318(self):
+        "1318 - test ref cursor doesn't work after connection is closed"
+        conn = test_env.get_connection()
+        cursor = conn.cursor()
+        var = cursor.var(oracledb.DB_TYPE_CURSOR)
+        cursor.callproc("myrefcursorproc", [var])
+        conn.close()
+        with self.assertRaisesFullCode("DPY-1001"):
+            ref_cursor = var.getvalue()
+            ref_cursor.fetchall()
+
 
 if __name__ == "__main__":
     test_env.run_test_cases()

tests/test_1900_lob_var.py

@@ -602,6 +602,18 @@ def test_1937(self):
             with self.assertRaisesFullCode("DPY-3026"):
                 lob.fileexists()
 
+    def test_1938(self):
+        "1938 - confirm that LOB objects are retained across getvalue() calls"
+        for typ in (
+            oracledb.DB_TYPE_BLOB,
+            oracledb.DB_TYPE_CLOB,
+            oracledb.DB_TYPE_NCLOB,
+        ):
+            var = self.cursor.var(typ)
+            lob = self.conn.createlob(typ, "Some data for test 1938")
+            var.setvalue(0, lob)
+            self.assertIs(var.getvalue(), lob)
+
 
 if __name__ == "__main__":
     test_env.run_test_cases()

tests/test_2300_object_var.py

@@ -803,6 +803,14 @@ def test_2339(self):
         self.assertEqual(obj.XMLVALUE, xml_val)
         self.assertEqual(obj.STRINGVALUE, str_val)
 
+    def test_2340(self):
+        "2340 - test DbObject instances are retained across getvalue() calls"
+        typ = self.conn.gettype("UDT_OBJECT")
+        obj = typ.newobject()
+        var = self.cursor.var(typ)
+        var.setvalue(0, obj)
+        self.assertIs(var.getvalue(), obj)
+
 
 if __name__ == "__main__":
     test_env.run_test_cases()