Fixes #33880 : fix backgroundcachefeatureiterator when there is

a sidefilterexpression and filter fids
qgis · Mar 19, 2020 · 4fee5e9 · 4fee5e9
1 parent c2129a8
commit 4fee5e9
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 13 deletions.
diff --git a/src/providers/wfs/qgsbackgroundcachedfeatureiterator.cpp b/src/providers/wfs/qgsbackgroundcachedfeatureiterator.cpp
@@ -257,25 +257,27 @@ 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 = mRequest;
+      mRequest = 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 +603,11 @@ bool QgsBackgroundCachedFeatureIterator::fetchFeature( QgsFeature &f )
       continue;
     }
 
+    if ( !mAdditionalRequest.acceptFeature( cachedFeature ) )
+    {
+      continue;
+    }
+
     copyFeature( cachedFeature, f, true );
     geometryToDestinationCrs( f, mTransform );
 
@@ -692,6 +699,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/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'