Merge pull request #60198 from elpaso/fix-transactional-field-editing…

…-with-gdal

[OGR] Fix transactional editing for GPKG/SQLite

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

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 );

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

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 );

src/core/providers/ogr/qgsogrprovider.cpp

@@ -544,6 +544,11 @@ 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 );
+  if ( transaction )
+  {
+    connect( mTransaction, &QgsTransaction::afterRollback, this, &QgsOgrProvider::afterRollback );
+    connect( mTransaction, &QgsTransaction::afterRollbackToSavepoint, this, &QgsOgrProvider::afterRollbackToSavepoint );
+  }
 }
 
 QgsAbstractFeatureSource *QgsOgrProvider::featureSource() const
@@ -726,6 +731,17 @@ QList<QgsProviderSublayerDetails> QgsOgrProvider::_subLayers( Qgis::SublayerQuer
   return mSubLayerList;
 }
 
+void QgsOgrProvider::afterRollbackToSavepoint( const QString &savepointName )
+{
+  Q_UNUSED( savepointName );
+  mFieldsRequireReload = true;
+}
+
+void QgsOgrProvider::afterRollback()
+{
+  mFieldsRequireReload = true;
+}
+
 void QgsOgrProvider::setEncoding( const QString &e )
 {
   QgsCPLHTTPFetchOverrider oCPLHTTPFetcher( mAuthCfg );
@@ -1073,6 +1089,7 @@ void QgsOgrProvider::loadFields()
     mAttributeFields.append( newField );
     createdFields++;
   }
+  mFieldsRequireReload = false;
 }
 
 void QgsOgrProvider::loadMetadata()
@@ -1205,7 +1222,7 @@ void QgsOgrProvider::setRelevantFields( bool fetchGeometry, const QgsAttributeLi
   QRecursiveMutex *mutex = nullptr;
   OGRLayerH ogrLayer = mOgrLayer->getHandleAndMutex( mutex );
   QMutexLocker locker( mutex );
-  QgsOgrProviderUtils::setRelevantFields( ogrLayer, mAttributeFields.count(), fetchGeometry, fetchAttributes, mFirstFieldIsFid, QgsOgrProviderUtils::cleanSubsetString( mSubsetString ) );
+  QgsOgrProviderUtils::setRelevantFields( ogrLayer, fields().count(), fetchGeometry, fetchAttributes, mFirstFieldIsFid, QgsOgrProviderUtils::cleanSubsetString( mSubsetString ) );
 }
 
 QgsFeatureIterator QgsOgrProvider::getFeatures( const QgsFeatureRequest &request ) const
@@ -1452,7 +1469,7 @@ QVariant QgsOgrProvider::defaultValue( int fieldId ) const
   QgsCPLHTTPFetchOverrider oCPLHTTPFetcher( mAuthCfg );
   QgsSetCPLHTTPFetchOverriderInitiatorClass( oCPLHTTPFetcher, QStringLiteral( "QgsOgrProvider" ) );
 
-  if ( fieldId < 0 || fieldId >= mAttributeFields.count() )
+  if ( fieldId < 0 || fieldId >= fields().count() )
     return QVariant();
 
   QString defaultVal = mDefaultValues.value( fieldId, QString() );
@@ -1516,7 +1533,7 @@ QVariant QgsOgrProvider::defaultValue( int fieldId ) const
     }
   }
 
-  const bool compatible = mAttributeFields.at( fieldId ).convertCompatible( resultVar );
+  const bool compatible = fields().at( fieldId ).convertCompatible( resultVar );
   return compatible && !QgsVariantUtils::isNull( resultVar ) ? resultVar : QVariant();
 }
 
@@ -1604,6 +1621,10 @@ long long QgsOgrProvider::featureCount() const
 
 QgsFields QgsOgrProvider::fields() const
 {
+  if ( mFieldsRequireReload )
+  {
+    const_cast<QgsOgrProvider *>( this )->loadFields();
+  }
   return mAttributeFields;
 }
 
@@ -1803,6 +1824,8 @@ bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags, QgsFeatureId
     {
       bool errorEmitted = false;
       bool ok = false;
+      // Get an updated copy
+      const QgsFields fieldsCopy { f.fields() };
       switch ( type )
       {
         case OFTInteger:
@@ -1815,7 +1838,7 @@ bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags, QgsFeatureId
             if ( !ok )
             {
               pushError( tr( "wrong value for attribute %1 of feature %2: %3" )
-                         .arg( mAttributeFields.at( qgisAttributeId ).name() )
+                         .arg( fieldsCopy.at( qgisAttributeId ).name() )
                          .arg( f.id() )
                          .arg( strVal ) );
               errorEmitted = true;
@@ -2033,9 +2056,9 @@ bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags, QgsFeatureId
         if ( !errorEmitted )
         {
           pushError( tr( "wrong data type for attribute %1 of feature %2: Got %3, expected %4" )
-                     .arg( mAttributeFields.at( qgisAttributeId ).name() )
+                     .arg( fieldsCopy.at( qgisAttributeId ).name() )
                      .arg( f.id() )
-                     .arg( attrVal.typeName(), QVariant::typeToName( mAttributeFields.at( qgisAttributeId ).type() ) ) );
+                     .arg( attrVal.typeName(), QVariant::typeToName( fieldsCopy.at( qgisAttributeId ).type() ) ) );
         }
         returnValue = false;
       }
@@ -2304,8 +2327,9 @@ bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
   }
 
   // Backup existing fields. We need them to 'restore' field type, length, precision
-  QgsFields oldFields = mAttributeFields;
+  QgsFields oldFields = fields();
 
+  // Updates mAttributeFields
   loadFields();
 
   // The check in QgsVectorLayerEditBuffer::commitChanges() is questionable with
@@ -2413,13 +2437,13 @@ bool QgsOgrProvider::renameAttributes( const QgsFieldNameMap &renamedAttributes
   for ( ; renameIt != renamedAttributes.constEnd(); ++renameIt )
   {
     int fieldIndex = renameIt.key();
-    if ( fieldIndex < 0 || fieldIndex >= mAttributeFields.count() )
+    if ( fieldIndex < 0 || fieldIndex >= fields().count() )
     {
       pushError( tr( "Invalid attribute index" ) );
       result = false;
       continue;
     }
-    if ( mAttributeFields.indexFromName( renameIt.value() ) >= 0 )
+    if ( fields().indexFromName( renameIt.value() ) >= 0 )
     {
       //field name already in use
       pushError( tr( "Error renaming field %1: name '%2' already exists" ).arg( fieldIndex ).arg( renameIt.value() ) );
@@ -3360,7 +3384,7 @@ bool QgsOgrProvider::createAttributeIndex( int field )
   QgsCPLHTTPFetchOverrider oCPLHTTPFetcher( mAuthCfg );
   QgsSetCPLHTTPFetchOverriderInitiatorClass( oCPLHTTPFetcher, QStringLiteral( "QgsOgrProvider" ) );
 
-  if ( field < 0 || field >= mAttributeFields.count() )
+  if ( field < 0 || field >= fields().count() )
     return false;
 
   if ( !doInitialActionsForEdition() )
@@ -3751,10 +3775,10 @@ QSet<QVariant> QgsOgrProvider::uniqueValues( int index, int limit ) const
 {
   QSet<QVariant> uniqueValues;
 
-  if ( !mValid || index < 0 || index >= mAttributeFields.count() )
+  if ( !mValid || index < 0 || index >= fields().count() )
     return uniqueValues;
 
-  const QgsField fld = mAttributeFields.at( index );
+  const QgsField fld = fields().at( index );
   if ( fld.name().isNull() )
   {
     return uniqueValues; //not a provider field
@@ -3816,10 +3840,10 @@ QStringList QgsOgrProvider::uniqueStringsMatching( int index, const QString &sub
 {
   QStringList results;
 
-  if ( !mValid || index < 0 || index >= mAttributeFields.count() )
+  if ( !mValid || index < 0 || index >= fields().count() )
     return results;
 
-  QgsField fld = mAttributeFields.at( index );
+  QgsField fld = fields().at( index );
   if ( fld.name().isNull() )
   {
     return results; //not a provider field
@@ -4097,15 +4121,15 @@ QList<QgsRelation> QgsOgrProvider::discoverRelations( const QgsVectorLayer *targ
 
 QVariant QgsOgrProvider::minimumValue( int index ) const
 {
-  if ( !mValid || index < 0 || index >= mAttributeFields.count() )
+  if ( !mValid || index < 0 || index >= fields().count() )
   {
     return QVariant();
   }
 
   QgsCPLHTTPFetchOverrider oCPLHTTPFetcher( mAuthCfg );
   QgsSetCPLHTTPFetchOverriderInitiatorClass( oCPLHTTPFetcher, QStringLiteral( "QgsOgrProvider" ) );
 
-  const QgsField originalField = mAttributeFields.at( index );
+  const QgsField originalField = fields().at( index );
   QgsField fld = originalField;
 
   // can't use native date/datetime types -- OGR converts these to string in the MAX return value
@@ -4157,15 +4181,15 @@ QVariant QgsOgrProvider::minimumValue( int index ) const
 
 QVariant QgsOgrProvider::maximumValue( int index ) const
 {
-  if ( !mValid || index < 0 || index >= mAttributeFields.count() )
+  if ( !mValid || index < 0 || index >= fields().count() )
   {
     return QVariant();
   }
 
   QgsCPLHTTPFetchOverrider oCPLHTTPFetcher( mAuthCfg );
   QgsSetCPLHTTPFetchOverriderInitiatorClass( oCPLHTTPFetcher, QStringLiteral( "QgsOgrProvider" ) );
 
-  const QgsField originalField = mAttributeFields.at( index );
+  const QgsField originalField = fields().at( index );
   QgsField fld = originalField;
 
   // can't use native date/datetime types -- OGR converts these to string in the MAX return value
@@ -4708,7 +4732,10 @@ bool QgsOgrProvider::leaveUpdateMode()
       // Backup fields since if we created new fields, but didn't populate it
       // with any feature yet, it will disappear.
       const QgsFields oldFields = mAttributeFields;
+
+      // This will also update mAttributeFields
       reloadData();
+
       if ( mValid )
       {
         // Make sure that new fields we added, but didn't populate yet, are

src/core/providers/ogr/qgsogrprovider.h

@@ -222,6 +222,13 @@ class QgsOgrProvider final: public QgsVectorDataProvider
     mutable std::unique_ptr< OGREnvelope3D > mExtent3D;
     bool mForceRecomputeExtent = false;
 
+    //! Flag set after a rollback to indicate that fields require reloading
+    bool mFieldsRequireReload = false;
+
+    //! Called after a transaction rollback
+    void afterRollback();
+    void afterRollbackToSavepoint( const QString &savePointName );
+
     QList<int> mPrimaryKeyAttrs;
 
     /**

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()

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
      */

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
   {

tests/src/python/test_qgsvectorlayereditbuffer.py

@@ -851,7 +851,10 @@ 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)