[WFS provider] Ensure stability of QGIS FeatureId when reloading layer (fixes #20865)

Author: rouault

src/providers/wfs/qgswfsfeatureiterator.cpp

@@ -32,13 +32,16 @@
 #include "qgssettings.h"
 #include "qgsexception.h"
 #include "qgsfeedback.h"
+#include "qgssqliteutils.h"
 
 #include <algorithm>
 #include <QDir>
 #include <QProgressDialog>
 #include <QTimer>
 #include <QStyle>
 
+#include <sqlite3.h>
+
 QgsWFSFeatureHitsAsyncRequest::QgsWFSFeatureHitsAsyncRequest( QgsWFSDataSourceURI &uri )
   : QgsWfsRequest( uri )
   , mNumberMatched( -1 )
@@ -986,7 +989,15 @@ QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )
   QgsFeatureRequest requestCache;
   if ( mRequest.filterType() == QgsFeatureRequest::FilterFid ||
        mRequest.filterType() == QgsFeatureRequest::FilterFids )
-    requestCache = mRequest;
+  {
+    QgsFeatureIds qgisIds;
+    if ( mRequest.filterType() == QgsFeatureRequest::FilterFid )
+      qgisIds.insert( mRequest.filterFid() );
+    else
+      qgisIds = mRequest.filterFids();
+
+    requestCache.setFilterFids( mShared->dbIdsFromQgisIds( qgisIds ) );
+  }
   else
   {
     if ( mRequest.filterType() == QgsFeatureRequest::FilterExpression )
@@ -1238,6 +1249,19 @@ bool QgsWFSFeatureIterator::fetchFeature( QgsFeature &f )
 
     copyFeature( cachedFeature, f );
     geometryToDestinationCrs( f, mTransform );
+
+    // Retrieve the user-visible id from the Spatialite cache database Id
+    if ( mShared->mCacheIdDb.get() )
+    {
+      auto sql = QgsSqlite3Mprintf( "SELECT qgisId FROM id_cache WHERE dbId = %lld", cachedFeature.id() );
+      int resultCode;
+      auto stmt = mShared->mCacheIdDb.prepare( sql, resultCode );
+      if ( stmt.step() == SQLITE_ROW )
+      {
+        f.setId( stmt.columnAsInt64( 0 ) );
+      }
+    }
+
     return true;
   }
 

src/providers/wfs/qgswfsshareddata.cpp

@@ -62,6 +62,16 @@ QgsWFSSharedData::~QgsWFSSharedData()
   QgsDebugMsgLevel( QStringLiteral( "~QgsWFSSharedData()" ), 4 );
 
   invalidateCache();
+
+  mCacheIdDb.reset();
+  if ( !mCacheIdDbname.isEmpty() )
+  {
+    QFile::remove( mCacheIdDbname );
+    QFile::remove( mCacheIdDbname + "-wal" );
+    QFile::remove( mCacheIdDbname + "-shm" );
+    QgsWFSUtils::releaseCacheDirectory();
+    mCacheIdDbname.clear();
+  }
 }
 
 QString QgsWFSSharedData::srsName() const
@@ -215,7 +225,8 @@ bool QgsWFSSharedData::createCache()
 
   static QAtomicInt sTmpCounter = 0;
   int tmpCounter = ++sTmpCounter;
-  mCacheDbname = QDir( QgsWFSUtils::acquireCacheDirectory() ).filePath( QStringLiteral( "wfs_cache_%1.sqlite" ).arg( tmpCounter ) );
+  QString cacheDirectory( QgsWFSUtils::acquireCacheDirectory() );
+  mCacheDbname = QDir( cacheDirectory ).filePath( QStringLiteral( "wfs_cache_%1.sqlite" ).arg( tmpCounter ) );
   Q_ASSERT( !QFile::exists( mCacheDbname ) );
 
   QgsFields cacheFields;
@@ -466,6 +477,35 @@ bool QgsWFSSharedData::createCache()
     return false;
   }
 
+  // The id_cache should be generated once for the lifetime of QgsWFSConstants
+  // to ensure consistency of the ids returned to the user.
+  if ( mCacheIdDbname.isEmpty() )
+  {
+    mCacheIdDbname = QDir( cacheDirectory ).filePath( QStringLiteral( "wfs_id_cache_%1.sqlite" ).arg( tmpCounter ) );
+    Q_ASSERT( !QFile::exists( mCacheIdDbname ) );
+    if ( mCacheIdDb.open( mCacheIdDbname ) != SQLITE_OK )
+    {
+      QgsMessageLog::logMessage( tr( "Cannot create temporary id cache" ), tr( "WFS" ) );
+      return false;
+    }
+    QString errorMsg;
+    bool ok = mCacheIdDb.exec( QStringLiteral( "PRAGMA synchronous=OFF" ), errorMsg ) == SQLITE_OK;
+    // WAL is needed to avoid reader to block writers
+    ok &= mCacheIdDb.exec( QStringLiteral( "PRAGMA journal_mode=WAL" ), errorMsg ) == SQLITE_OK;
+    // gmlid is the gmlid or fid attribute coming from the GML GetFeature response
+    // qgisId is the feature id of the features returned to QGIS. That one should remain the same for a given gmlid even after a layer reload
+    // dbId is the feature id of the Spatialite feature in mCacheDataProvider. It might change for a given gmlid after a layer reload
+    ok &= mCacheIdDb.exec( QStringLiteral( "CREATE TABLE id_cache(gmlid TEXT, dbId INTEGER, qgisId INTEGER)" ), errorMsg ) == SQLITE_OK;
+    ok &= mCacheIdDb.exec( QStringLiteral( "CREATE INDEX idx_gmlid ON id_cache(gmlid)" ), errorMsg ) == SQLITE_OK;
+    ok &= mCacheIdDb.exec( QStringLiteral( "CREATE INDEX idx_dbId ON id_cache(dbId)" ), errorMsg ) == SQLITE_OK;
+    ok &= mCacheIdDb.exec( QStringLiteral( "CREATE INDEX idx_qgisId ON id_cache(qgisId)" ), errorMsg ) == SQLITE_OK;
+    if ( !ok )
+    {
+      QgsDebugMsg( errorMsg );
+      return false;
+    }
+  }
+
   return true;
 }
 
@@ -572,6 +612,10 @@ int QgsWFSSharedData::getUpdatedCounter()
 
 QSet<QString> QgsWFSSharedData::getExistingCachedGmlIds( const QVector<QgsWFSFeatureGmlIdPair> &featureList )
 {
+  // We query the Spatialite cache here, not the persistent id_cache,
+  // since we want to know which features in this session we have already
+  // downloaded.
+
   QString expr;
   bool first = true;
   QSet<QString> setExistingGmlIds;
@@ -675,46 +719,79 @@ QSet<QString> QgsWFSSharedData::getExistingCachedMD5( const QVector<QgsWFSFeatur
 // Used by WFS-T
 QString QgsWFSSharedData::findGmlId( QgsFeatureId fid )
 {
-  if ( !mCacheDataProvider )
+  if ( !mCacheIdDb )
     return QString();
-  QgsFeatureRequest request;
-  request.setFilterFid( fid );
 
-  QgsFields dataProviderFields = mCacheDataProvider->fields();
-  int gmlidIdx = dataProviderFields.indexFromName( QgsWFSConstants::FIELD_GMLID );
-
-  QgsAttributeList attList;
-  attList.append( gmlidIdx );
-  request.setSubsetOfAttributes( attList );
-
-  QgsFeatureIterator iterGmlIds( mCacheDataProvider->getFeatures( request ) );
-  QgsFeature gmlidFeature;
-  while ( iterGmlIds.nextFeature( gmlidFeature ) )
+  auto sql = QgsSqlite3Mprintf( "SELECT gmlid FROM id_cache WHERE qgisId = %lld", fid );
+  int resultCode;
+  auto stmt = mCacheIdDb.prepare( sql, resultCode );
+  Q_ASSERT( resultCode == SQLITE_OK );
+  if ( stmt.step() == SQLITE_ROW )
   {
-    const QVariant &v = gmlidFeature.attributes().value( gmlidIdx );
-    return v.toString();
+    return stmt.columnAsText( 0 );
   }
   return QString();
 }
 
+QgsFeatureIds QgsWFSSharedData::dbIdsFromQgisIds( const QgsFeatureIds &qgisIds )
+{
+  QgsFeatureIds dbIds;
+  if ( !mCacheIdDb )
+    return dbIds;
+  // To avoid excessive memory consumption in expression building, do not
+  // query more than 1000 ids at a time.
+  bool first = true;
+  QString expr;
+  int i = 0;
+  for ( const auto &qgisId : qgisIds )
+  {
+    if ( !first )
+      expr += ',';
+    else
+    {
+      expr = QStringLiteral( "SELECT dbId FROM id_cache WHERE qgisId IN (" );
+      first = false;
+    }
+    expr += FID_TO_STRING( qgisId );
+
+    if ( ( i > 0 && ( i % 1000 ) == 0 ) || i + 1 == qgisIds.size() )
+    {
+      expr += ')';
+
+      int resultCode;
+      auto stmt = mCacheIdDb.prepare( expr.toUtf8().constData(), resultCode );
+      Q_ASSERT( resultCode == SQLITE_OK );
+      while ( stmt.step() == SQLITE_ROW )
+      {
+        dbIds.insert( stmt.columnAsInt64( 0 ) );
+      }
+      // Should we check that we got a dbId from every qgisId... ?
+
+      first = true;
+    }
+    i++;
+  }
+  return dbIds;
+}
+
 // Used by WFS-T
 bool QgsWFSSharedData::deleteFeatures( const QgsFeatureIds &fidlist )
 {
-  if ( !mCacheDataProvider )
+  if ( !mCacheIdDb || !mCacheDataProvider )
     return false;
 
   {
     QMutexLocker locker( &mMutex );
     mFeatureCount -= fidlist.size();
   }
 
-  return mCacheDataProvider->deleteFeatures( fidlist );
+  return mCacheDataProvider->deleteFeatures( dbIdsFromQgisIds( fidlist ) );
 }
 
 // Used by WFS-T
 bool QgsWFSSharedData::changeGeometryValues( const QgsGeometryMap &geometry_map )
 {
-  if ( !mCacheDataProvider )
+  if ( !mCacheIdDb || !mCacheDataProvider )
     return false;
 
   // We need to replace the geometry by its bounding box and issue a attribute
@@ -727,22 +804,34 @@ bool QgsWFSSharedData::changeGeometryValues( const QgsGeometryMap &geometry_map
   QgsChangedAttributesMap newChangedAttrMap;
   for ( QgsGeometryMap::const_iterator iter = geometry_map.constBegin(); iter != geometry_map.constEnd(); ++iter )
   {
+    auto sql = QgsSqlite3Mprintf( "SELECT dbId FROM id_cache WHERE qgisId = %lld", iter.key() );
+    int resultCode;
+    auto stmt = mCacheIdDb.prepare( sql, resultCode );
+    Q_ASSERT( resultCode == SQLITE_OK );
+    if ( stmt.step() != SQLITE_ROW )
+    {
+      // shouldn't happen normally
+      QgsDebugMsg( QStringLiteral( "cannot find dbId corresponding to qgisId = %1" ).arg( iter.key() ) );
+      continue;
+    }
+    QgsFeatureId dbId = stmt.columnAsInt64( 0 );
+
     QByteArray wkb = iter->asWkb();
     if ( !wkb.isEmpty() )
     {
       QgsAttributeMap newAttrMap;
       newAttrMap[idx] = QString( wkb.toHex().data() );
-      newChangedAttrMap[ iter.key()] = newAttrMap;
+      newChangedAttrMap[ dbId] = newAttrMap;
 
       QgsGeometry polyBoundingBox = QgsGeometry::fromRect( iter.value().boundingBox() );
-      newGeometryMap[ iter.key()] = polyBoundingBox;
+      newGeometryMap[ dbId] = polyBoundingBox;
     }
     else
     {
       QgsAttributeMap newAttrMap;
       newAttrMap[idx] = QString();
-      newChangedAttrMap[ iter.key()] = newAttrMap;
-      newGeometryMap[ iter.key()] = QgsGeometry();
+      newChangedAttrMap[ dbId] = newAttrMap;
+      newGeometryMap[ dbId] = QgsGeometry();
     }
   }
 
@@ -753,14 +842,25 @@ bool QgsWFSSharedData::changeGeometryValues( const QgsGeometryMap &geometry_map
 // Used by WFS-T
 bool QgsWFSSharedData::changeAttributeValues( const QgsChangedAttributesMap &attr_map )
 {
-  if ( !mCacheDataProvider )
+  if ( !mCacheIdDb || !mCacheDataProvider )
     return false;
 
   QgsFields dataProviderFields = mCacheDataProvider->fields();
   QgsChangedAttributesMap newMap;
   for ( QgsChangedAttributesMap::const_iterator iter = attr_map.begin(); iter != attr_map.end(); ++iter )
   {
-    QgsFeatureId fid = iter.key();
+    auto sql = QgsSqlite3Mprintf( "SELECT dbId FROM id_cache WHERE qgisId = %lld", iter.key() );
+    int resultCode;
+    auto stmt = mCacheIdDb.prepare( sql, resultCode );
+    Q_ASSERT( resultCode == SQLITE_OK );
+    if ( stmt.step() != SQLITE_ROW )
+    {
+      // shouldn't happen normally
+      QgsDebugMsg( QStringLiteral( "cannot find dbId corresponding to qgisId = %1" ).arg( iter.key() ) );
+      continue;
+    }
+    QgsFeatureId dbId = stmt.columnAsInt64( 0 );
+
     const QgsAttributeMap &attrs = iter.value();
     if ( attrs.isEmpty() )
       continue;
@@ -774,7 +874,7 @@ bool QgsWFSSharedData::changeAttributeValues( const QgsChangedAttributesMap &att
       else
         newAttrMap[idx] = siter.value();
     }
-    newMap[fid] = newAttrMap;
+    newMap[dbId] = newAttrMap;
   }
 
   return mCacheDataProvider->changeAttributeValues( newMap );
@@ -923,10 +1023,57 @@ void QgsWFSSharedData::serializeFeatures( QVector<QgsWFSFeatureGmlIdPair> &featu
     Q_ASSERT( featureListToCache.size() == updatedFeatureList.size() );
     for ( int i = 0; i < updatedFeatureList.size(); i++ )
     {
-      if ( cacheOk )
-        updatedFeatureList[i].first.setId( featureListToCache[i].id() );
+      int resultCode;
+      QgsFeatureId dbId( cacheOk ? featureListToCache[i].id() : mTotalFeaturesAttemptedToBeCached + i + 1 );
+      QgsFeatureId qgisId;
+      const auto &gmlId( updatedFeatureList[i].second );
+      if ( gmlId.isEmpty() )
+      {
+        // Degraded case. Won't work properly in reload situations, but we
+        // can't do better.
+        qgisId = dbId;
+      }
       else
-        updatedFeatureList[i].first.setId( mTotalFeaturesAttemptedToBeCached + i + 1 );
+      {
+        auto sql = QgsSqlite3Mprintf( "SELECT qgisId, dbId FROM id_cache WHERE gmlid = '%q'",
+                                      gmlId.toUtf8().constData() );
+        auto stmt = mCacheIdDb.prepare( sql, resultCode );
+        Q_ASSERT( resultCode == SQLITE_OK );
+        if ( stmt.step() == SQLITE_ROW )
+        {
+          qgisId = stmt.columnAsInt64( 0 );
+          QgsFeatureId oldDbId = stmt.columnAsInt64( 1 );
+          if ( dbId != oldDbId )
+          {
+            sql = QgsSqlite3Mprintf( "UPDATE id_cache SET dbId = %lld WHERE gmlid = '%q'",
+                                     dbId,
+                                     gmlId.toUtf8().constData() );
+            //QgsDebugMsg( QStringLiteral( "%1" ).arg( sql ) );
+            QString errorMsg;
+            if ( mCacheIdDb.exec( sql, errorMsg ) != SQLITE_OK )
+            {
+              QgsMessageLog::logMessage( tr( "Problem when updating WFS id cache: %1 -> %2" ).arg( sql ).arg( errorMsg ), tr( "WFS" ) );
+            }
+          }
+        }
+        else
+        {
+          qgisId = mNextCachedIdQgisId;
+          mNextCachedIdQgisId ++;
+          sql = QgsSqlite3Mprintf( "INSERT INTO id_cache (gmlid, dbId, qgisId) VALUES ('%q', %lld, %lld)",
+                                   gmlId.toUtf8().constData(),
+                                   dbId,
+                                   qgisId );
+          //QgsDebugMsg( QStringLiteral( "%1" ).arg( sql ) );
+          QString errorMsg;
+          if ( mCacheIdDb.exec( sql, errorMsg ) != SQLITE_OK )
+          {
+            QgsMessageLog::logMessage( tr( "Problem when updating WFS id cache: %1 -> %2" ).arg( sql ).arg( errorMsg ), tr( "WFS" ) );
+          }
+        }
+      }
+
+      updatedFeatureList[i].first.setId( qgisId );
     }
 
     {
@@ -1104,7 +1251,6 @@ void QgsWFSSharedData::invalidateCache()
     QFile::remove( mCacheDbname );
     QFile::remove( mCacheDbname + "-wal" );
     QFile::remove( mCacheDbname + "-shm" );
-    QgsWFSUtils::releaseCacheDirectory();
     mCacheDbname.clear();
   }
 }