fix(featurepool): cache management - fixes #58113

python/PyQt6/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in

@@ -121,15 +121,20 @@ To be used by implementations of ``addFeature``.
    the original layer's thread.
 %End
 
-    void refreshCache( const QgsFeature &feature );
+    void refreshCache( QgsFeature feature, const QgsFeature &origFeature );
 %Docstring
 Changes a feature in the cache and the spatial index.
 To be used by implementations of ``updateFeature``.
 
+:param feature: the new feature to put in the cache and index
+:param origFeature: the original feature to remove from the index
+
 .. note::
 
    This method can safely be called from a different thread vs the object's creation thread or
    the original layer's thread.
+
+.. versionadded:: 3.42
 %End
 
     void removeFeature( const QgsFeatureId featureId );

python/analysis/auto_generated/vector/geometry_checker/qgsfeaturepool.sip.in

@@ -121,15 +121,20 @@ To be used by implementations of ``addFeature``.
    the original layer's thread.
 %End
 
-    void refreshCache( const QgsFeature &feature );
+    void refreshCache( QgsFeature feature, const QgsFeature &origFeature );
 %Docstring
 Changes a feature in the cache and the spatial index.
 To be used by implementations of ``updateFeature``.
 
+:param feature: the new feature to put in the cache and index
+:param origFeature: the original feature to remove from the index
+
 .. note::
 
    This method can safely be called from a different thread vs the object's creation thread or
    the original layer's thread.
+
+.. versionadded:: 3.42
 %End
 
     void removeFeature( const QgsFeatureId featureId );

src/analysis/vector/geometry_checker/qgsfeaturepool.cpp

@@ -49,7 +49,18 @@ bool QgsFeaturePool::getFeature( QgsFeatureId id, QgsFeature &feature )
   //
   // https://bugreports.qt.io/browse/QTBUG-19794
 
-  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
+  // If the feature we want is amongst the features that have been updated,
+  // then get it from the dedicated hash.
+  // It would not be thread-safe to get it directly from the layer,
+  // and it could be outdated in the feature source (in case of a memory layer),
+  // and it could have been cleared from the cache due to a cache overflow.
+  if ( mUpdatedFeatures.contains( id ) )
+  {
+    feature = mUpdatedFeatures[id];
+    return true;
+  }
+
+  QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Read );
   QgsFeature *cachedFeature = mFeatureCache.object( id );
   if ( cachedFeature )
   {
@@ -78,6 +89,7 @@ QgsFeatureIds QgsFeaturePool::getFeatures( const QgsFeatureRequest &request, Qgs
   Q_ASSERT( mLayer );
   Q_ASSERT( QThread::currentThread() == mLayer->thread() );
 
+  mUpdatedFeatures.clear();
   mFeatureCache.clear();
   mIndex = QgsSpatialIndex();
 
@@ -131,19 +143,21 @@ void QgsFeaturePool::insertFeature( const QgsFeature &feature, bool skipLock )
   mIndex.addFeature( indexFeature );
 }
 
-void QgsFeaturePool::refreshCache( const QgsFeature &feature )
+void QgsFeaturePool::refreshCache( QgsFeature feature, const QgsFeature &origFeature )
 {
+  // insert/refresh the updated features as well
+  mUpdatedFeatures.insert( feature.id(), feature );
+
   QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Write );
-  mFeatureCache.remove( feature.id() );
-  mIndex.deleteFeature( feature );
+  mFeatureCache.insert( feature.id(), new QgsFeature( feature ) );
+  mIndex.deleteFeature( origFeature );
+  mIndex.addFeature( feature );
   locker.unlock();
-
-  QgsFeature tempFeature;
-  getFeature( feature.id(), tempFeature );
 }
 
 void QgsFeaturePool::removeFeature( const QgsFeatureId featureId )
 {
+  mUpdatedFeatures.remove( featureId );
   QgsFeature origFeature;
   QgsReadWriteLocker locker( mCacheLock, QgsReadWriteLocker::Unlocked );
   if ( getFeature( featureId, origFeature ) )

src/analysis/vector/geometry_checker/qgsfeaturepool.h

@@ -171,10 +171,14 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
      * Changes a feature in the cache and the spatial index.
      * To be used by implementations of ``updateFeature``.
      *
+     * \param feature the new feature to put in the cache and index
+     * \param origFeature the original feature to remove from the index
+     *
      * \note This method can safely be called from a different thread vs the object's creation thread or
      * the original layer's thread.
+     * \since QGIS 3.42
      */
-    void refreshCache( const QgsFeature &feature );
+    void refreshCache( QgsFeature feature, const QgsFeature &origFeature );
 
     /**
      * Removes a feature from the cache and the spatial index.
@@ -219,6 +223,20 @@ class ANALYSIS_EXPORT QgsFeaturePool : public QgsFeatureSink SIP_ABSTRACT
     QString mLayerId;
     QString mLayerName;
     QgsCoordinateReferenceSystem mCrs;
+
+    /**
+    * Hash containing features whose geometry has been updated
+    * but which are not accessible if we get them
+    * from the layer (not thread-safe).
+    *
+    * The philosophy is that as soon as a feature geometry has
+    * changed, it will be in this hash, and the cache will not
+    * be used any more.
+    * \since QGIS 3.42
+    */
+    QHash<QgsFeatureId, QgsFeature> mUpdatedFeatures;
+
+    friend class TestQgsVectorLayerFeaturePool;
 };
 
 #endif // QGS_FEATUREPOOL_H

src/analysis/vector/geometry_checker/qgsvectordataproviderfeaturepool.cpp

@@ -153,7 +153,7 @@ void QgsVectorDataProviderFeaturePool::updateFeature( QgsFeature &feature )
     }
   } );
 
-  refreshCache( feature );
+  refreshCache( feature, origFeature );
 }
 
 void QgsVectorDataProviderFeaturePool::deleteFeature( QgsFeatureId fid )

src/analysis/vector/geometry_checker/qgsvectorlayerfeaturepool.cpp

@@ -109,6 +109,9 @@ bool QgsVectorLayerFeaturePool::addFeatures( QgsFeatureList &features, QgsFeatur
 
 void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature )
 {
+  QgsFeature origFeature;
+  getFeature( feature.id(), origFeature );
+
   QgsThreadingUtils::runOnMainThread( [this, &feature]()
   {
     QgsVectorLayer *lyr = layer();
@@ -118,7 +121,7 @@ void QgsVectorLayerFeaturePool::updateFeature( QgsFeature &feature )
     }
   } );
 
-  refreshCache( feature );
+  refreshCache( feature, origFeature );
 }
 
 void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )
@@ -136,14 +139,12 @@ void QgsVectorLayerFeaturePool::deleteFeature( QgsFeatureId fid )
 
 void QgsVectorLayerFeaturePool::onGeometryChanged( QgsFeatureId fid, const QgsGeometry &geometry )
 {
-  Q_UNUSED( geometry )
-
-  if ( isFeatureCached( fid ) )
-  {
-    QgsFeature feature;
-    getFeature( fid, feature );
-    refreshCache( feature );
-  }
+  QgsFeature feature;
+  QgsFeature origFeature;
+  getFeature( fid, origFeature );
+  feature = origFeature;
+  feature.setGeometry( geometry );
+  refreshCache( feature, origFeature );
 }
 
 void QgsVectorLayerFeaturePool::onFeatureDeleted( QgsFeatureId fid )

tests/src/geometry_checker/testqgsgeometrychecks.cpp

@@ -63,7 +63,8 @@ class TestQgsGeometryChecks: public QObject
     };
     double layerToMapUnits( const QgsMapLayer *layer, const QgsCoordinateReferenceSystem &mapCrs ) const;
     QgsFeaturePool *createFeaturePool( QgsVectorLayer *layer, bool selectedOnly = false ) const;
-    QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), double prec = 8 ) const;
+    QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const;
+    QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > createMemoryTestContext( QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs = QgsCoordinateReferenceSystem( "EPSG:4326" ), int prec = 8 ) const;
     void cleanupTestContext( QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > ctx ) const;
     void listErrors( const QList<QgsGeometryCheckError *> &checkErrors, const QStringList &messages ) const;
     QList<QgsGeometryCheckError *> searchCheckErrors( const QList<QgsGeometryCheckError *> &checkErrors, const QString &layerId, const QgsFeatureId &featureId = -1, const QgsPointXY &pos = QgsPointXY(), const QgsVertexId &vid = QgsVertexId(), const QVariant &value = QVariant(), double tol = 1E-4 ) const;
@@ -78,6 +79,7 @@ class TestQgsGeometryChecks: public QObject
 
   private slots:
     void testAngleCheck();
+    void testAngleCheckMemoryLayers();
     void testAreaCheck();
     void testContainedCheck();
     void testDangleCheck();
@@ -117,6 +119,85 @@ void TestQgsGeometryChecks::cleanupTestCase()
 
 ///////////////////////////////////////////////////////////////////////////////
 
+void TestQgsGeometryChecks::testAngleCheckMemoryLayers()
+{
+  QTemporaryDir dir;
+  QMap<QString, QString> layers;
+  layers.insert( "point_layer.shp", "" );
+  layers.insert( "line_layer.shp", "" );
+  layers.insert( "polygon_layer.shp", "" );
+  auto testContext = createMemoryTestContext( layers );
+
+  // Test detection
+  QList<QgsGeometryCheckError *> checkErrors;
+  QStringList messages;
+
+  QVariantMap configuration;
+  configuration.insert( "minAngle", 15 );
+
+  const QgsGeometryAngleCheck check( testContext.first, configuration );
+  QgsFeedback feedback;
+  check.collectErrors( testContext.second, checkErrors, messages, &feedback );
+  listErrors( checkErrors, messages );
+
+  QList<QgsGeometryCheckError *> errs1;
+  QList<QgsGeometryCheckError *> errs2;
+
+  QCOMPARE( checkErrors.size(), 8 );
+  QVERIFY( searchCheckErrors( checkErrors, layers["point_layer.shp"] ).isEmpty() );
+  QVERIFY( ( errs1 = searchCheckErrors( checkErrors, layers["line_layer.shp"], 1, QgsPointXY( -0.2225,  0.5526 ), QgsVertexId( 0, 0, 3 ), 10.5865 ) ).size() == 1 );
+  QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 1, QgsPointXY( -0.94996, 0.99967 ), QgsVertexId( 1, 0, 1 ), 8.3161 ).size() == 1 );
+  QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 3, QgsPointXY( -0.4547, -0.3059 ), QgsVertexId( 0, 0, 1 ), 5.4165 ).size() == 1 );
+  QVERIFY( searchCheckErrors( checkErrors, layers["line_layer.shp"], 3, QgsPointXY( -0.7594, -0.1971 ), QgsVertexId( 0, 0, 2 ), 12.5288 ).size() == 1 );
+  QVERIFY( ( errs2 = searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 1, QgsPointXY( 0.2402, 1.0786 ), QgsVertexId( 0, 0, 1 ), 13.5140 ) ).size() == 1 );
+  QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 2, QgsPointXY( 0.6960, 0.5908 ), QgsVertexId( 0, 0, 0 ), 7.0556 ).size() == 1 );
+  QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 2, QgsPointXY( 0.98690, 0.55699 ), QgsVertexId( 1, 0, 5 ), 7.7351 ).size() == 1 );
+  QVERIFY( searchCheckErrors( checkErrors, layers["polygon_layer.shp"], 12, QgsPointXY( -0.3186, 1.6734 ), QgsVertexId( 0, 0, 1 ), 3.5092 ).size() == 1 );
+
+  // Test fixes
+  QgsFeature f;
+  int n1, n2;
+
+  testContext.second[errs1[0]->layerId()]->getFeature( errs1[0]->featureId(), f );
+  n1 = f.geometry().constGet()->vertexCount( errs1[0]->vidx().part, errs1[0]->vidx().ring );
+  QVERIFY( fixCheckError( testContext.second,  errs1[0],
+                          QgsGeometryAngleCheck::DeleteNode, QgsGeometryCheckError::StatusFixed,
+  {{errs1[0]->layerId(), errs1[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs1[0]->vidx()}} ) );
+  testContext.second[errs1[0]->layerId()]->getFeature( errs1[0]->featureId(), f );
+  n2 = f.geometry().constGet()->vertexCount( errs1[0]->vidx().part, errs1[0]->vidx().ring );
+  QCOMPARE( n1, n2 + 1 );
+
+  testContext.second[errs2[0]->layerId()]->getFeature( errs2[0]->featureId(), f );
+  n1 = f.geometry().constGet()->vertexCount( errs2[0]->vidx().part, errs2[0]->vidx().ring );
+  QVERIFY( fixCheckError( testContext.second,  errs2[0],
+                          QgsGeometryAngleCheck::DeleteNode, QgsGeometryCheckError::StatusFixed,
+  {{errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs2[0]->vidx()}} ) );
+  testContext.second[errs2[0]->layerId()]->getFeature( errs2[0]->featureId(), f );
+  n2 = f.geometry().constGet()->vertexCount( errs2[0]->vidx().part, errs2[0]->vidx().ring );
+  QCOMPARE( n1, n2 + 1 );
+
+  // Test change tracking
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeFeature, QgsGeometryCheck::ChangeRemoved, QgsVertexId()} ) ) );
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeFeature, QgsGeometryCheck::ChangeChanged, QgsVertexId()} ) ) );
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part )} ) ) );
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part )} ) ) );
+  QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part + 1 )} ) ) );
+  QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangePart, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part + 1 )} ) ) );
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring )} ) ) );
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring )} ) ) );
+  QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring + 1 )} ) ) );
+  QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeRing, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring + 1 )} ) ) );
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, errs2[0]->vidx()} ) ) );
+  QVERIFY( !errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeChanged, errs2[0]->vidx()} ) ) );
+  QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex + 1 )} ) ) );
+  QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeChanged, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex + 1 )} ) ) );
+  const QgsVertexId oldVidx = errs2[0]->vidx();
+  QVERIFY( errs2[0]->handleChanges( change2changes( {errs2[0]->layerId(), errs2[0]->featureId(), QgsGeometryCheck::ChangeNode, QgsGeometryCheck::ChangeRemoved, QgsVertexId( errs2[0]->vidx().part, errs2[0]->vidx().ring, errs2[0]->vidx().vertex - 1 )} ) ) );
+  QVERIFY( errs2[0]->vidx().vertex == oldVidx.vertex - 1 );
+
+  cleanupTestContext( testContext );
+}
+
 void TestQgsGeometryChecks::testAngleCheck()
 {
   QTemporaryDir dir;
@@ -1328,7 +1409,24 @@ QgsFeaturePool *TestQgsGeometryChecks::createFeaturePool( QgsVectorLayer *layer,
   return new QgsVectorDataProviderFeaturePool( layer, selectedOnly );
 }
 
-QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, double prec ) const
+QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createMemoryTestContext( QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const
+{
+  const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) );
+
+  QMap<QString, QgsFeaturePool *> featurePools;
+  for ( auto it = layers.begin(); it != layers.end(); it++ )
+  {
+    const QString layerFile = it.key();
+    QgsVectorLayer *layer = ( new QgsVectorLayer( testDataDir.absoluteFilePath( layerFile ), layerFile ) )->materialize( QgsFeatureRequest() );
+    Q_ASSERT( layer && layer->isValid() );
+    layers[layerFile] = layer->id();
+    layer->dataProvider()->enterUpdateMode();
+    featurePools.insert( layer->id(), createFeaturePool( layer ) );
+  }
+  return qMakePair( new QgsGeometryCheckContext( prec, mapCrs, QgsProject::instance()->transformContext(), QgsProject::instance() ), featurePools );
+}
+
+QPair<QgsGeometryCheckContext *, QMap<QString, QgsFeaturePool *> > TestQgsGeometryChecks::createTestContext( QTemporaryDir &tempDir, QMap<QString, QString> &layers, const QgsCoordinateReferenceSystem &mapCrs, int prec ) const
 {
   const QDir testDataDir( QDir( TEST_DATA_DIR ).absoluteFilePath( "geometry_checker" ) );
   const QDir tmpDir( tempDir.path() );