[OGR] Fix transactional editing for GPKG/SQLite

Tell the provider to reload the fields after a rollback and add some checks to verify if after the rollback the provider still needs to update the field. Followup OSGeo/gdal#11695 Followup #59797
qgis · Jan 21, 2025 · f2f9d3a · f2f9d3a
1 parent 023015e
commit f2f9d3a
Show file tree

Hide file tree

Showing 10 changed files with 90 additions and 14 deletions.
diff --git a/python/PyQt6/core/auto_additions/qgstransaction.py b/python/PyQt6/core/auto_additions/qgstransaction.py
@@ -1,8 +1,8 @@
 # The following has been generated automatically from src/core/qgstransaction.h
 try:
-    QgsTransaction.__attribute_docs__ = {'afterRollback': 'Emitted after a rollback\n', 'dirtied': 'Emitted if a sql query is executed and the underlying data is modified\n'}
+    QgsTransaction.__attribute_docs__ = {'afterRollback': 'Emitted after a rollback\n', 'afterRollbackToSavepoint': 'Emitted after a rollback to savepoint\n\n.. versionadded:: 3.42\n', 'dirtied': 'Emitted if a sql query is executed and the underlying data is modified\n'}
     QgsTransaction.create = staticmethod(QgsTransaction.create)
     QgsTransaction.supportsTransaction = staticmethod(QgsTransaction.supportsTransaction)
-    QgsTransaction.__signal_arguments__ = {'dirtied': ['sql: str', 'name: str']}
+    QgsTransaction.__signal_arguments__ = {'afterRollbackToSavepoint': ['savepointName: str'], 'dirtied': ['sql: str', 'name: str']}
 except (NameError, AttributeError):
     pass
diff --git a/python/PyQt6/core/auto_generated/qgstransaction.sip.in b/python/PyQt6/core/auto_generated/qgstransaction.sip.in
@@ -152,6 +152,13 @@ returns the last created savepoint
     void afterRollback();
 %Docstring
 Emitted after a rollback
+%End
+
+    void afterRollbackToSavepoint( const QString &savepointName );
+%Docstring
+Emitted after a rollback to savepoint
+
+.. versionadded:: 3.42
 %End
 
     void dirtied( const QString &sql, const QString &name );

diff --git a/python/core/auto_additions/qgstransaction.py b/python/core/auto_additions/qgstransaction.py
@@ -1,8 +1,8 @@
 # The following has been generated automatically from src/core/qgstransaction.h
 try:
-    QgsTransaction.__attribute_docs__ = {'afterRollback': 'Emitted after a rollback\n', 'dirtied': 'Emitted if a sql query is executed and the underlying data is modified\n'}
+    QgsTransaction.__attribute_docs__ = {'afterRollback': 'Emitted after a rollback\n', 'afterRollbackToSavepoint': 'Emitted after a rollback to savepoint\n\n.. versionadded:: 3.42\n', 'dirtied': 'Emitted if a sql query is executed and the underlying data is modified\n'}
     QgsTransaction.create = staticmethod(QgsTransaction.create)
     QgsTransaction.supportsTransaction = staticmethod(QgsTransaction.supportsTransaction)
-    QgsTransaction.__signal_arguments__ = {'dirtied': ['sql: str', 'name: str']}
+    QgsTransaction.__signal_arguments__ = {'afterRollbackToSavepoint': ['savepointName: str'], 'dirtied': ['sql: str', 'name: str']}
 except (NameError, AttributeError):
     pass
diff --git a/python/core/auto_generated/qgstransaction.sip.in b/python/core/auto_generated/qgstransaction.sip.in
@@ -152,6 +152,13 @@ returns the last created savepoint
     void afterRollback();
 %Docstring
 Emitted after a rollback
+%End
+
+    void afterRollbackToSavepoint( const QString &savepointName );
+%Docstring
+Emitted after a rollback to savepoint
+
+.. versionadded:: 3.42
 %End
 
     void dirtied( const QString &sql, const QString &name );

diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp
@@ -544,6 +544,14 @@ void QgsOgrProvider::setTransaction( QgsTransaction *transaction )
   QgsDebugMsgLevel( QStringLiteral( "set transaction %1" ).arg( transaction != nullptr ), 2 );
   // static_cast since layers cannot be added to a transaction of a non-matching provider
   mTransaction = static_cast<QgsOgrTransaction *>( transaction );
+  connect( mTransaction, &QgsTransaction::afterRollback, this, [ = ]( )
+  {
+    mFieldsRequireReload = true;
+  } );
+  connect( mTransaction, &QgsTransaction::afterRollbackToSavepoint, this, [ = ]( const QString & )
+  {
+    mFieldsRequireReload = true;
+  } );
 }
 
 QgsAbstractFeatureSource *QgsOgrProvider::featureSource() const
@@ -1073,6 +1081,7 @@ void QgsOgrProvider::loadFields()
     mAttributeFields.append( newField );
     createdFields++;
   }
+  mFieldsRequireReload = false;
 }
 
 void QgsOgrProvider::loadMetadata()
@@ -1604,6 +1613,10 @@ long long QgsOgrProvider::featureCount() const
 
 QgsFields QgsOgrProvider::fields() const
 {
+  if ( mFieldsRequireReload )
+  {
+    const_cast<QgsOgrProvider *>( this )->loadFields();
+  }
   return mAttributeFields;
 }
 

diff --git a/src/core/providers/ogr/qgsogrprovider.h b/src/core/providers/ogr/qgsogrprovider.h
@@ -221,6 +221,7 @@ class QgsOgrProvider final: public QgsVectorDataProvider
     mutable std::unique_ptr< OGREnvelope > mExtent2D;
     mutable std::unique_ptr< OGREnvelope3D > mExtent3D;
     bool mForceRecomputeExtent = false;
+    bool mFieldsRequireReload = false;
 
     QList<int> mPrimaryKeyAttrs;
 

diff --git a/src/core/qgstransaction.cpp b/src/core/qgstransaction.cpp
@@ -271,7 +271,12 @@ bool QgsTransaction::rollbackToSavepoint( const QString &name, QString &error SI
   // the status of the DB has changed between the previous savepoint and the
   // one we are rolling back to.
   mLastSavePointIsDirty = true;
-  return executeSql( QStringLiteral( "ROLLBACK TO SAVEPOINT %1" ).arg( QgsExpression::quotedColumnRef( name ) ), error );
+  if ( ! executeSql( QStringLiteral( "ROLLBACK TO SAVEPOINT %1" ).arg( QgsExpression::quotedColumnRef( name ) ), error ) )
+  {
+    return false;
+  }
+  emit afterRollbackToSavepoint( name );
+  return true;
 }
 
 void QgsTransaction::dirtyLastSavePoint()

diff --git a/src/core/qgstransaction.h b/src/core/qgstransaction.h
@@ -173,6 +173,12 @@ class CORE_EXPORT QgsTransaction : public QObject SIP_ABSTRACT
      */
     void afterRollback();
 
+    /**
+     * Emitted after a rollback to savepoint
+     * \since QGIS 3.42
+     */
+    void afterRollbackToSavepoint( const QString &savepointName );
+
     /**
      * Emitted if a sql query is executed and the underlying data is modified
      */

diff --git a/src/core/vector/qgsvectorlayerundopassthroughcommand.cpp b/src/core/vector/qgsvectorlayerundopassthroughcommand.cpp
@@ -393,7 +393,13 @@ void QgsVectorLayerUndoPassthroughCommandAddAttribute::undo()
   const int attr = mBuffer->L->dataProvider()->fieldNameIndex( mField.name() );
   if ( rollBackToSavePoint() )
   {
-    mBuffer->L->dataProvider()->deleteAttributes( QgsAttributeIds() << attr );
+    // GDAL SQLite-based drivers (since version 3.11) keep the fields in sync with
+    // the backend after a rollback, to stay on the safe side check if the field
+    // isn't already gone
+    if ( mBuffer->L->dataProvider()->fieldNameIndex( mField.name() ) != -1 )
+    {
+      mBuffer->L->dataProvider()->deleteAttributes( QgsAttributeIds() << attr );
+    }
     mBuffer->mAddedAttributes.removeAll( mField );
     mBuffer->updateLayerFields();
     emit mBuffer->attributeDeleted( attr );
@@ -432,11 +438,26 @@ void QgsVectorLayerUndoPassthroughCommandDeleteAttribute::undo()
   // note that the addAttributes here is only necessary to inform the provider that
   // an attribute is added back after the rollBackToSavePoint
   mBuffer->L->dataProvider()->clearErrors();
-  if ( mBuffer->L->dataProvider()->addAttributes( QList<QgsField>() << mField )  && rollBackToSavePoint() && ! mBuffer->L->dataProvider()->hasErrors() )
+  if ( rollBackToSavePoint() )
   {
-    mBuffer->mDeletedAttributeIds.removeOne( mOriginalFieldIndex );
-    mBuffer->updateLayerFields();
-    emit mBuffer->attributeAdded( mOriginalFieldIndex );
+    // GDA SQLite-based drivers (since version 3.11) keep the fields in sync with
+    // the backend after a rollback, to stay on the safe side check if the field
+    // isn't already there
+    bool ok = true;
+    if ( mBuffer->L->dataProvider()->fields().indexFromName( mField.name() ) == -1 )
+    {
+      ok = mBuffer->L->dataProvider()->addAttributes( QList<QgsField>() << mField );
+    }
+    if ( ok && ! mBuffer->L->dataProvider()->hasErrors() )
+    {
+      mBuffer->mDeletedAttributeIds.removeOne( mOriginalFieldIndex );
+      mBuffer->updateLayerFields();
+      emit mBuffer->attributeAdded( mOriginalFieldIndex );
+    }
+    else
+    {
+      setError();
+    }
   }
   else
   {
@@ -474,10 +495,25 @@ void QgsVectorLayerUndoPassthroughCommandRenameAttribute::undo()
   QgsFieldNameMap map;
   map[ mAttr ] = mOldName;
   mBuffer->L->dataProvider()->clearErrors();
-  if ( mBuffer->L->dataProvider()->renameAttributes( map ) && rollBackToSavePoint() && ! mBuffer->L->dataProvider()->hasErrors() )
+  if ( rollBackToSavePoint() )
   {
-    mBuffer->updateLayerFields();
-    emit mBuffer->attributeRenamed( mAttr, mOldName );
+    // GDAL SQLite-based drivers (since version 3.11) keep the fields in sync with
+    // the backend after a rollback, to stay on the safe side check if the field
+    // isn't already renamed
+    bool ok = true;
+    if ( mBuffer->L->dataProvider()->fields().indexFromName( mOldName ) == -1 )
+    {
+      ok = mBuffer->L->dataProvider()->renameAttributes( map );
+    }
+    if ( ok && ! mBuffer->L->dataProvider()->hasErrors() )
+    {
+      mBuffer->updateLayerFields();
+      emit mBuffer->attributeRenamed( mAttr, mOldName );
+    }
+    else
+    {
+      setError();
+    }
   }
   else
   {

diff --git a/tests/src/python/test_qgsvectorlayereditbuffer.py b/tests/src/python/test_qgsvectorlayereditbuffer.py
@@ -851,7 +851,8 @@ def _check_feature(wkt):
 
         # THIS FUNCTIONALITY IS BROKEN ON NEWER GDAL VERSIONS, DUE TO INCORRECT
         # assumptions at time of development. See https://github.com/qgis/QGIS/pull/59797#issuecomment-2544133498
-        if int(gdal.VersionInfo("VERSION_NUM")) < GDAL_COMPUTE_VERSION(3, 5, 0):
+        # see also: https://github.com/OSGeo/gdal/pull/11695 for a GDAL 3.11 fix
+        if int(gdal.VersionInfo("VERSION_NUM")) < GDAL_COMPUTE_VERSION(3, 5, 0) or int(gdal.VersionInfo("VERSION_NUM")) >= GDAL_COMPUTE_VERSION(3, 11, 0):
             _test(Qgis.TransactionMode.AutomaticGroups)
 
         _test(Qgis.TransactionMode.BufferedGroups)