Merge pull request #52150 from elpaso/bugfix-gh51934-filtered-gpkg-transaction-group

OGR: fix transaction issue with filtered GPKGs

src/core/providers/ogr/qgsogrfeatureiterator.cpp

@@ -287,7 +287,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
   //       when the logger was disabled.
   //       There is currently no API to connect the change of state of the
   //       logger to the data provider.
-  if ( QgsApplication::databaseQueryLog()->enabled() )
+  if ( QgsApplication::databaseQueryLog()->enabled() && mConn )
   {
     GDALDatasetSetQueryLoggerFunc( mConn->ds, [ ]( const char *pszSQL, const char *pszError, int64_t lNumRecords, int64_t lExecutionTimeMilliseconds, void *pQueryLoggerArg )
     {
@@ -517,6 +517,13 @@ bool QgsOgrFeatureIterator::rewind()
 
 bool QgsOgrFeatureIterator::close()
 {
+  // Finally reset the data source filter, in case it was changed by a previous request
+  // this fixes https://github.com/qgis/QGIS/issues/51934
+  if ( mOgrLayer && ! mSource->mSubsetString.isEmpty() )
+  {
+    OGR_L_SetAttributeFilter( mOgrLayer,  mSource->mEncoding->fromUnicode( mSource->mSubsetString ).constData() );
+  }
+
   if ( mSharedDS )
   {
     iteratorClosed();

src/core/qgstransaction.cpp

@@ -72,14 +72,21 @@ QString QgsTransaction::connectionString() const
 }
 
 // For the needs of the OGR provider with GeoPackage datasources, remove
-// any reference to layers in the connection string
+// any reference to layers and filters in the connection string
-QString QgsTransaction::removeLayerIdOrName( const QString &str )
+QString QgsTransaction::cleanupConnectionString( const QString &str )
 {
   QString res( str );
 
-  for ( int i = 0; i < 2; i++ )
+  static const QStringList toRemove
   {
-    const int pos = res.indexOf( i == 0 ? QLatin1String( "|layername=" ) :  QLatin1String( "|layerid=" ) );
+    { QStringLiteral( "|layername=" )},
+    { QStringLiteral( "|layerid=" )},
+    { QStringLiteral( "|subset=" )},
+  };
+
+  for ( const auto &strToRm : std::as_const( toRemove ) )
+  {
+    const int pos = res.indexOf( strToRm );
     if ( pos >= 0 )
     {
       const int end = res.indexOf( '|', pos + 1 );
@@ -105,7 +112,7 @@ QString QgsTransaction::connectionString( const QString &layerUri )
   // reference to layers from it.
   if ( connString.isEmpty() )
   {
-    connString = removeLayerIdOrName( layerUri );
+    connString = cleanupConnectionString( layerUri );
   }
   return connString;
 }

src/core/qgstransaction.h

@@ -201,7 +201,7 @@ class CORE_EXPORT QgsTransaction : public QObject SIP_ABSTRACT
 
     void setLayerTransactionIds( QgsTransaction *transaction );
 
-    static QString removeLayerIdOrName( const QString &str );
+    static QString cleanupConnectionString( const QString &str );
 
     virtual bool beginTransaction( QString &error, int statementTimeout ) = 0;
     virtual bool commitTransaction( QString &error ) = 0;

tests/src/python/test_provider_ogr_gpkg.py

@@ -2638,6 +2638,59 @@ class TestPyQgsOGRProviderGpkg(unittest.TestCase):
         got = [feat for feat in vl.getFeatures()]
         self.assertEqual(got[0]["dt"], new_dt)
 
+    def testTransactionModeAutoWithFilter(self):
+
+        temp_dir = QTemporaryDir()
+        temp_path = temp_dir.path()
+        filename = os.path.join(temp_path, "test.gpkg")
+        ds = ogr.GetDriverByName("GPKG").CreateDataSource(filename)
+        lyr = ds.CreateLayer("points", geom_type=ogr.wkbPoint)
+        lyr.CreateField(ogr.FieldDefn('name', ogr.OFTString))
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f['name'] = 'a'
+        f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(1 2)'))
+        lyr.CreateFeature(f)
+        f = None
+        ds = None
+        ds = None
+
+        vl = QgsVectorLayer(filename)
+        self.assertTrue(vl.isValid())
+        self.assertEqual(vl.featureCount(), 1)
+        self.assertEqual(next(vl.getFeatures())['name'], 'a')
+
+        p = QgsProject()
+        p.setAutoTransaction(True)
+        self.assertTrue(p.addMapLayers([vl]))
+        self.assertTrue(vl.setSubsetString('"name" IN (\'a\', \'b\')'))
+        self.assertEqual(vl.featureCount(), 1)
+        self.assertTrue(vl.startEditing())
+
+        # Add feature
+        f = QgsFeature(vl.fields())
+        f.setAttribute('name', 'b')
+        g = QgsGeometry.fromWkt('POINT(3 4)')
+        f.setGeometry(g)
+
+        # This triggers the issue because it sets the subset filter
+        req = QgsFeatureRequest()
+        req.setFilterExpression('"name" IS NULL')
+        self.assertEqual([f for f in vl.getFeatures(req)], [])
+
+        self.assertTrue(vl.addFeatures([f]))
+        self.assertTrue(vl.commitChanges())
+        self.assertEqual(vl.featureCount(), 2)
+        attrs = [f['name'] for f in vl.getFeatures()]
+        self.assertEqual(attrs, ['a', 'b'])
+
+        # verify
+        del p
+        vl2 = QgsVectorLayer(filename)
+        self.assertTrue(vl2.isValid())
+        self.assertEqual(vl2.featureCount(), 2)
+        attrs = [f['name'] for f in vl2.getFeatures()]
+        self.assertEqual(attrs, ['a', 'b'])
+
 
 if __name__ == '__main__':
     unittest.main()