Merge pull request #60716 from qgis/backport-60698-to-release-3_42

[Backport release-3_42] Fix leak when directly iterating feature in PyQGIS

python/PyQt6/core/auto_generated/qgsfeature.sip.in

@@ -40,6 +40,9 @@ geometry and a list of field/values attributes.
     QgsAttributes attributes = sipCpp->attributes();
     PyObject *attrs = sipConvertFromType( &attributes, sipType_QgsAttributes, Py_None );
     sipRes = PyObject_GetIter( attrs );
+    // PyObject_GetIter has added a ref to attrs - we need to decrement the ref from sipConvertFromType,
+    // so that the garbage collector will delete attrs when the iterator is deleted
+    Py_DECREF( attrs );
 %End
 
 

python/core/auto_generated/qgsfeature.sip.in

@@ -40,6 +40,9 @@ geometry and a list of field/values attributes.
     QgsAttributes attributes = sipCpp->attributes();
     PyObject *attrs = sipConvertFromType( &attributes, sipType_QgsAttributes, Py_None );
     sipRes = PyObject_GetIter( attrs );
+    // PyObject_GetIter has added a ref to attrs - we need to decrement the ref from sipConvertFromType,
+    // so that the garbage collector will delete attrs when the iterator is deleted
+    Py_DECREF( attrs );
 %End
 
     SIP_PYOBJECT __getitem__( int key ) /HoldGIL/;

src/core/qgsfeature.h

@@ -76,6 +76,9 @@ class CORE_EXPORT QgsFeature
     QgsAttributes attributes = sipCpp->attributes();
     PyObject *attrs = sipConvertFromType( &attributes, sipType_QgsAttributes, Py_None );
     sipRes = PyObject_GetIter( attrs );
+    // PyObject_GetIter has added a ref to attrs - we need to decrement the ref from sipConvertFromType,
+    // so that the garbage collector will delete attrs when the iterator is deleted
+    Py_DECREF( attrs );
     % End
 #endif
 

tests/src/python/test_qgsfeature.py

@@ -40,10 +40,9 @@ def test_CreateFeature(self):
         feat.initAttributes(1)
         feat.setAttribute(0, "text")
         feat.setGeometry(QgsGeometry.fromPointXY(QgsPointXY(123, 456)))
-        myId = feat.id()
-        myExpectedId = 0
-        myMessage = f"\nExpected: {myExpectedId}\nGot: {myId}"
-        assert myId == myExpectedId, myMessage
+        self.assertEqual(feat.id(), 0)
+        self.assertEqual(feat.attributes(), ["text"])
+        self.assertEqual(feat.geometry().asWkt(), "Point (123 456)")
 
     def test_FeatureDefaultConstructor(self):
         """Test for FID_IS_NULL default constructors See: https://github.com/qgis/QGIS/issues/36962"""
@@ -138,9 +137,7 @@ def test_ValidFeature(self):
         feat = QgsFeature()
         fit.nextFeature(feat)
         fit.close()
-        myValidValue = feat.isValid()
-        myMessage = f"\nExpected: True\nGot: {myValidValue}"
-        assert myValidValue, myMessage
+        self.assertTrue(feat.isValid())
 
     def test_Validity(self):
         f = QgsFeature()
@@ -170,21 +167,14 @@ def test_Attributes(self):
         feat = QgsFeature()
         fit.nextFeature(feat)
         fit.close()
-        myAttributes = feat.attributes()
-        myExpectedAttributes = ["Highway", 1]
-
-        # Only for printing purposes
-        myExpectedAttributes = ["Highway", 1]
-        myMessage = f"\nExpected: {myExpectedAttributes}\nGot: {myAttributes}"
-
-        assert myAttributes == myExpectedAttributes, myMessage
+        self.assertEqual(feat.attributes(), ["Highway", 1])
 
     def test_SetAttributes(self):
         feat = QgsFeature()
         feat.initAttributes(1)
         feat.setAttributes([0])
         feat.setAttributes([NULL])
-        assert [NULL] == feat.attributes()
+        self.assertEqual(feat.attributes(), [NULL])
 
         # Test different type of attributes
         attributes = [
@@ -203,6 +193,7 @@ def test_SetAttributes(self):
         feat.initAttributes(len(attributes))
         feat.setAttributes(attributes)
         self.assertEqual(feat.attributes(), attributes)
+        self.assertEqual([attr for attr in feat.attributes()], attributes)
 
     def test_setAttribute(self):
         feat = QgsFeature()
@@ -291,10 +282,7 @@ def test_DeleteAttribute(self):
         feat[1] = "text2"
         feat[2] = "text3"
         feat.deleteAttribute(1)
-        myAttrs = [feat[0], feat[1]]
-        myExpectedAttrs = ["text1", "text3"]
-        myMessage = f"\nExpected: {str(myExpectedAttrs)}\nGot: {str(myAttrs)}"
-        assert myAttrs == myExpectedAttrs, myMessage
+        self.assertEqual([feat[0], feat[1]], ["text1", "text3"])
 
     def test_DeleteAttributeByName(self):
         fields = QgsFields()
@@ -315,10 +303,7 @@ def test_DeleteAttributeByName(self):
     def test_SetGeometry(self):
         feat = QgsFeature()
         feat.setGeometry(QgsGeometry.fromPointXY(QgsPointXY(123, 456)))
-        myGeometry = feat.geometry()
-        myExpectedGeometry = "!None"
-        myMessage = f"\nExpected: {myExpectedGeometry}\nGot: {myGeometry}"
-        assert myGeometry is not None, myMessage
+        self.assertEqual(feat.geometry().asWkt(), "Point (123 456)")
 
         # set from QgsAbstractGeometry
         feat.setGeometry(QgsPoint(12, 34))

tests/src/python/test_qgsvectorlayer.py

@@ -320,6 +320,39 @@ def test_FeatureCount(self):
         myCount = myLayer.featureCount()
         self.assertEqual(myCount, 6)
 
+    def test_attribute_iteration(self):
+        layer = QgsVectorLayer(
+            self.get_test_data_path("lines.shp").as_posix(), "Lines", "ogr"
+        )
+        self.assertTrue(layer.isValid())
+        all_attrs = [
+            [attr for attr in feat.attributes()] for feat in layer.getFeatures()
+        ]
+        self.assertCountEqual(
+            all_attrs,
+            [
+                ["Highway", 1.0],
+                ["Highway", 1.0],
+                ["Arterial", 2.0],
+                ["Arterial", 2.0],
+                ["Arterial", 2.0],
+                ["Arterial", 2.0],
+            ],
+        )
+
+        all_attrs = [[attr for attr in feat] for feat in layer.getFeatures()]
+        self.assertCountEqual(
+            all_attrs,
+            [
+                ["Highway", 1.0],
+                ["Highway", 1.0],
+                ["Arterial", 2.0],
+                ["Arterial", 2.0],
+                ["Arterial", 2.0],
+                ["Arterial", 2.0],
+            ],
+        )
+
     # undo stack
     def testUndoStack(self):
         layer = createLayerWithOnePoint()