[OGR provider] Check return value of commitTransaction() in addFeatur…

…es() and deleteFeatures(); call rollback when an error was detected before
qgis · Oct 21, 2020 · c857a33 · c857a33
1 parent 3e00e0c
commit c857a33
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 5 deletions.
diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp
@@ -1866,7 +1866,10 @@ bool QgsOgrProvider::addFeatures( QgsFeatureList &flist, Flags flags )
 
   if ( inTransaction )
   {
-    commitTransaction();
+    if ( returnvalue )
+      returnvalue = commitTransaction();
+    else
+      rollbackTransaction();
   }
 
   if ( !syncToDisc() )
@@ -2177,6 +2180,17 @@ bool QgsOgrProvider::commitTransaction()
   return true;
 }
 
+
+bool QgsOgrProvider::rollbackTransaction()
+{
+  if ( mOgrLayer->RollbackTransaction() != OGRERR_NONE )
+  {
+    pushError( tr( "OGR error rolling back transaction: %1" ).arg( CPLGetLastErrorMsg() ) );
+    return false;
+  }
+  return true;
+}
+
 bool QgsOgrProvider::_setSubsetString( const QString &theSQL, bool updateFeatureCount, bool updateCapabilities, bool hasExistingRef )
 {
   QgsCPLErrorHandler handler;
@@ -2487,7 +2501,10 @@ bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_
 
   if ( inTransaction )
   {
-    if ( ! commitTransaction() ) return false;
+    if ( returnValue )
+      returnValue = commitTransaction();
+    else
+      rollbackTransaction();
   }
 
   if ( mTransaction )
@@ -2526,6 +2543,7 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
   }
 #endif
 
+  bool returnvalue = true;
   for ( QgsGeometryMap::const_iterator it = geometry_map.constBegin(); it != geometry_map.constEnd(); ++it )
   {
     gdal::ogr_feature_unique_ptr theOGRFeature( mOgrLayer->GetFeature( FID_TO_NUMBER( it.key() ) ) );
@@ -2581,6 +2599,7 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
     if ( mOgrLayer->SetFeature( theOGRFeature.get() ) != OGRERR_NONE )
     {
       pushError( tr( "OGR error setting feature %1: %2" ).arg( it.key() ).arg( CPLGetLastErrorMsg() ) );
+      returnvalue = false;
       continue;
     }
     mShapefileMayBeCorrupted = true;
@@ -2590,14 +2609,21 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
 
   if ( inTransaction )
   {
-    if ( ! commitTransaction() ) return false;
+    if ( returnvalue )
+      returnvalue = commitTransaction();
+    else
+      rollbackTransaction();
   }
 
   if ( mTransaction )
     mTransaction->dirtyLastSavePoint();
 
+  if ( mOgrLayer->SyncToDisk() != OGRERR_NONE )
+  {
+    pushError( tr( "OGR error syncing to disk: %1" ).arg( CPLGetLastErrorMsg() ) );
+  }
   QgsOgrConnPool::instance()->invalidateConnections( QgsOgrProviderUtils::connectionPoolId( dataSourceUri( true ), mShareSameDatasetAmongLayers ) );
-  return syncToDisc();
+  return returnvalue;
 }
 
 bool QgsOgrProvider::createSpatialIndex()
@@ -2698,7 +2724,10 @@ bool QgsOgrProvider::deleteFeatures( const QgsFeatureIds &id )
 
   if ( inTransaction )
   {
-    if ( ! commitTransaction() ) return false;
+    if ( returnvalue )
+      returnvalue = commitTransaction();
+    else
+      rollbackTransaction();
   }
 
   if ( mTransaction )

diff --git a/src/core/providers/ogr/qgsogrprovider.h b/src/core/providers/ogr/qgsogrprovider.h
@@ -211,6 +211,9 @@ class QgsOgrProvider final: public QgsVectorDataProvider
     //! Commits a transaction
     bool commitTransaction();
 
+    //! Rolls back a transaction
+    bool rollbackTransaction();
+
     //! Does the real job of settings the subset string and adds an argument to disable update capabilities
     bool _setSubsetString( const QString &theSQL, bool updateFeatureCount = true, bool updateCapabilities = true, bool hasExistingRef = true );
 

diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py
@@ -1924,6 +1924,44 @@ def testVectorLayerExporterDeferredSpatialIndexSpatialIndexDisallowed(self):
         del ds
         gdal.Unlink(filename)
 
+    def testRollback(self):
+        """ Test that a failed operation is properly rolled back """
+        tmpfile = os.path.join(self.basetestpath, 'testRollback.gpkg')
+
+        ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
+        lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint, options=['SPATIAL_INDEX=NO'])
+        # Ugly hack to be able to create a column with unique constraint with GDAL < 3.2
+        ds.ExecuteSQL('CREATE TABLE test2 ("fid" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "geom" POINT, v INTEGER, v_unique INTEGER UNIQUE)')
+        ds.ExecuteSQL("UPDATE gpkg_contents SET table_name = 'test2'")
+        ds.ExecuteSQL("UPDATE gpkg_geometry_columns SET table_name = 'test2'")
+        ds.ExecuteSQL('INSERT INTO test2 (fid, geom, v, v_unique) VALUES (1, NULL, -1, 123)')
+        ds = None
+
+        vl = QgsVectorLayer(u'{}'.format(tmpfile), 'test', u'ogr')
+        self.assertTrue(vl.isValid())
+
+        features = [f for f in vl.getFeatures()]
+        self.assertEqual(len(features), 1)
+        self.assertEqual(features[0].attributes(), [1, -1, 123])
+
+        f = QgsFeature()
+        # violates unique constraint
+        f.setAttributes([None, -2, 123])
+        f2 = QgsFeature()
+        f2.setAttributes([None, -3, 124])
+        ret, _ = vl.dataProvider().addFeatures([f, f2])
+        self.assertFalse(ret)
+
+        f = QgsFeature()
+        f.setAttributes([None, -4, 125])
+        ret, _ = vl.dataProvider().addFeatures([f])
+        self.assertTrue(ret)
+
+        features = [f for f in vl.getFeatures()]
+        self.assertEqual(len(features), 2)
+        self.assertEqual(features[0].attributes(), [1, -1, 123])
+        self.assertEqual(features[1].attributes(), [2, -4, 125])
+
 
 if __name__ == '__main__':
     unittest.main()