QgsVectorLayerEditBuffer::commitChanges(): do not do anything if adde…

…d features are of incompatible type Fixes #41283
qgis · May 21, 2021 · f4ecfae · f4ecfae
1 parent b4fd455
commit f4ecfae
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 9 deletions.
diff --git a/src/core/vector/qgsvectorlayereditbuffer.cpp b/src/core/vector/qgsvectorlayereditbuffer.cpp
@@ -339,10 +339,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList &commitErrors )
   // yes                yes                   => changeFeatures
 
   // to fix https://github.com/qgis/QGIS/issues/23663
-  // first of all check if feature to add is compatible with provider type
-  // this check have to be done before all checks to avoid to clear internal
-  // buffer if some of next steps success.
-  if ( success && !mAddedFeatures.isEmpty() )
+  if ( !mAddedFeatures.isEmpty() )
   {
     if ( cap & QgsVectorDataProvider::AddFeatures )
     {
@@ -373,7 +370,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList &commitErrors )
   //
   // update geometries
   //
-  if ( !mChangedGeometries.isEmpty() && ( ( cap & QgsVectorDataProvider::ChangeFeatures ) == 0 || mChangedAttributeValues.isEmpty() ) )
+  if ( success && !mChangedGeometries.isEmpty() && ( ( cap & QgsVectorDataProvider::ChangeFeatures ) == 0 || mChangedAttributeValues.isEmpty() ) )
   {
     if ( provider->changeGeometryValues( mChangedGeometries ) )
     {
@@ -395,7 +392,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList &commitErrors )
   // delete attributes
   //
   bool attributesChanged = false;
-  if ( !mDeletedAttributeIds.isEmpty() )
+  if ( success && !mDeletedAttributeIds.isEmpty() )
   {
     if ( ( cap & QgsVectorDataProvider::DeleteAttributes ) && provider->deleteAttributes( qgis::listToSet( mDeletedAttributeIds ) ) )
     {
@@ -423,7 +420,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList &commitErrors )
   }
 
   // rename attributes
-  if ( !mRenamedAttributes.isEmpty() )
+  if ( success && !mRenamedAttributes.isEmpty() )
   {
     if ( ( cap & QgsVectorDataProvider::RenameAttributes ) && provider->renameAttributes( mRenamedAttributes ) )
     {
@@ -475,7 +472,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList &commitErrors )
   // check that addition/removal went as expected
   //
   bool attributeChangesOk = true;
-  if ( attributesChanged )
+  if ( success && attributesChanged )
   {
     L->updateFields();
     QgsFields newFields = L->fields();
@@ -515,7 +512,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList &commitErrors )
     }
   }
 
-  if ( attributeChangesOk )
+  if ( success && attributeChangesOk )
   {
     if ( cap & QgsVectorDataProvider::ChangeFeatures && !mChangedGeometries.isEmpty() && !mChangedAttributeValues.isEmpty() )
     {

diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py
@@ -1138,6 +1138,35 @@ def test_SplitFeature(self):
         self.assertCountEqual([geom.asWkt() for geom in [g, g2]], ['Polygon ((0 0, 0 1, 0.5 1, 0.5 0, 0 0))',
                                                                    'Polygon ((0.5 0, 0.5 1, 1 1, 1 0, 0.5 0))'])
 
+    def test_SplitFeatureErrorIncompatibleGeometryType(self):
+        """Test we behave correctly when split feature is not possible due to incompatible geometry type"""
+        tmpfile = os.path.join(self.basetestpath, 'test_SplitFeatureErrorIncompatibleGeometryType.gpkg')
+        ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
+        lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        # For the purpose of this test, we insert a Polygon in a Point layer
+        # which is normally not allowed
+        f.SetGeometry(ogr.CreateGeometryFromWkt('POLYGON ((0 0,0 1,1 1,1 0,0 0))'))
+        gdal.PushErrorHandler('CPLQuietErrorHandler')
+        self.assertEqual(lyr.CreateFeature(f), ogr.OGRERR_NONE)
+        gdal.PopErrorHandler()
+        f = None
+        ds = None
+
+        # Split features
+        layer = QgsVectorLayer(u'{}'.format(tmpfile) + "|layername=" + "test", 'test', u'ogr')
+        self.assertTrue(layer.isValid())
+        self.assertTrue(layer.isSpatial())
+        self.assertEqual([f for f in layer.getFeatures()][0].geometry().asWkt(), 'Polygon ((0 0, 0 1, 1 1, 1 0, 0 0))')
+        layer.startEditing()
+        self.assertEqual(layer.splitFeatures([QgsPointXY(0.5, 0), QgsPointXY(0.5, 1)], 0), 0)
+        self.assertFalse(layer.commitChanges())
+
+        layer = QgsVectorLayer(u'{}'.format(tmpfile) + "|layername=" + "test", 'test', u'ogr')
+        self.assertEqual(layer.featureCount(), 1)
+        g = [f.geometry() for f in layer.getFeatures()][0]
+        self.assertEqual(g.asWkt(), 'Polygon ((0 0, 0 1, 1 1, 1 0, 0 0))')
+
     def testCreateAttributeIndex(self):
         tmpfile = os.path.join(self.basetestpath, 'testGeopackageAttributeIndex.gpkg')
         ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)