Merge pull request #35201 from troopa81/backport_34009_to_release_312

[Backport release-3.12] Fix background cache iterator when using with both subset string and filter on fid(s)
qgis · Mar 25, 2020 · 1449d60 · 1449d60
2 parents 54e5974 + 9b02ea3
commit 1449d60
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 13 deletions.
diff --git a/src/providers/wfs/qgsbackgroundcachedfeatureiterator.cpp b/src/providers/wfs/qgsbackgroundcachedfeatureiterator.cpp
@@ -257,25 +257,26 @@ void QgsThreadedFeatureDownloader::run()
 
 // -------------------------
 
-static QgsFeatureRequest addSubsetToFeatureRequest( const QgsFeatureRequest &requestIn,
-    const QgsBackgroundCachedSharedData *shared )
-{
-  if ( shared->clientSideFilterExpression().isEmpty() )
-  {
-    return requestIn;
-  }
-  QgsFeatureRequest requestOut( requestIn );
-  requestOut.combineFilterExpression( shared->clientSideFilterExpression() );
-  return requestOut;
-}
-
 QgsBackgroundCachedFeatureIterator::QgsBackgroundCachedFeatureIterator(
   QgsBackgroundCachedFeatureSource *source, bool ownSource,
   std::shared_ptr<QgsBackgroundCachedSharedData> shared,
   const QgsFeatureRequest &request )
-  : QgsAbstractFeatureIteratorFromSource<QgsBackgroundCachedFeatureSource>( source, ownSource, addSubsetToFeatureRequest( request, shared.get() ) )
+  : QgsAbstractFeatureIteratorFromSource<QgsBackgroundCachedFeatureSource>( source, ownSource, request )
   , mShared( shared )
 {
+  if ( !shared->clientSideFilterExpression().isEmpty() )
+  {
+    // backup current request because combine filter expression will remove the fid(s) filtering
+    if ( mRequest.filterType() == QgsFeatureRequest::FilterFid || mRequest.filterType() == QgsFeatureRequest::FilterFids )
+    {
+      mAdditionalRequest = QgsFeatureRequest( shared->clientSideFilterExpression() );
+    }
+    else
+    {
+      mRequest.combineFilterExpression( shared->clientSideFilterExpression() );
+    }
+  }
+
   if ( mRequest.destinationCrs().isValid() && mRequest.destinationCrs() != mShared->sourceCrs() )
   {
     mTransform = QgsCoordinateTransform( mShared->sourceCrs(), mRequest.destinationCrs(), mRequest.transformContext() );
@@ -601,6 +602,11 @@ bool QgsBackgroundCachedFeatureIterator::fetchFeature( QgsFeature &f )
       continue;
     }
 
+    if ( !mAdditionalRequest.acceptFeature( cachedFeature ) )
+    {
+      continue;
+    }
+
     copyFeature( cachedFeature, f, true );
     geometryToDestinationCrs( f, mTransform );
 
@@ -692,6 +698,11 @@ bool QgsBackgroundCachedFeatureIterator::fetchFeature( QgsFeature &f )
           continue;
         }
 
+        if ( !mAdditionalRequest.acceptFeature( feat ) )
+        {
+          continue;
+        }
+
         copyFeature( feat, f, false );
         return true;
       }

diff --git a/src/providers/wfs/qgsbackgroundcachedfeatureiterator.h b/src/providers/wfs/qgsbackgroundcachedfeatureiterator.h
@@ -320,6 +320,9 @@ class QgsBackgroundCachedFeatureIterator final: public QObject,
     QgsCoordinateTransform mTransform;
     QgsRectangle mFilterRect;
 
+    //! typically to save a FilterFid/FilterFids request that will not be captured by mRequest
+    QgsFeatureRequest mAdditionalRequest;
+
     ///////////////// METHODS
 
     //! Translate mRequest to a request compatible of the Spatialite cache

diff --git a/tests/src/python/provider_python.py b/tests/src/python/provider_python.py
@@ -78,6 +78,10 @@ def __init__(self, source, request):
         if self._filter_rect is not None and self._source._provider._spatialindex is not None:
             self._feature_id_list = self._source._provider._spatialindex.intersects(self._filter_rect)
 
+        if self._request.filterType() == QgsFeatureRequest.FilterFid or self._request.filterType() == QgsFeatureRequest.FilterFids:
+            fids = [self._request.filterFid()] if self._request.filterType() == QgsFeatureRequest.FilterFid else self._request.filterFids()
+            self._feature_id_list = list(set(self._feature_id_list).intersection(set(fids))) if self._feature_id_list else fids
+
     def fetchFeature(self, f):
         """fetch next feature, return true on success"""
         #virtual bool nextFeature( QgsFeature &f );

diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py
@@ -210,6 +210,29 @@ def testSubsetString(self):
                                                                                                       result, subset)
         self.assertTrue(all_valid)
 
+        # Subset string AND filter fid
+        ids = {f['pk']: f.id() for f in self.source.getFeatures()}
+        self.source.setSubsetString(subset)
+        request = QgsFeatureRequest().setFilterFid(4)
+        result = set([f.id() for f in self.source.getFeatures(request)])
+        all_valid = (all(f.isValid() for f in self.source.getFeatures(request)))
+        self.source.setSubsetString(None)
+        expected = set([4])
+        assert set(expected) == result, 'Expected {} and got {} when testing subset string {}'.format(set(expected),
+                                                                                                      result, subset)
+        self.assertTrue(all_valid)
+
+        # Subset string AND filter fids
+        self.source.setSubsetString(subset)
+        request = QgsFeatureRequest().setFilterFids([ids[2], ids[4]])
+        result = set([f.id() for f in self.source.getFeatures(request)])
+        all_valid = (all(f.isValid() for f in self.source.getFeatures(request)))
+        self.source.setSubsetString(None)
+        expected = set([ids[2], ids[4]])
+        assert set(expected) == result, 'Expected {} and got {} when testing subset string {}'.format(set(expected),
+                                                                                                      result, subset)
+        self.assertTrue(all_valid)
+
     def getSubsetString(self):
         """Individual providers may need to override this depending on their subset string formats"""
         return '"cnt" > 100 and "cnt" < 410'