Merge pull request #7047 from rouault/fix_18740

[WFS provider] Avoid request by feature id to cause a full layer download (fixes #18740)

Author: rouault

src/providers/wfs/qgswfsfeatureiterator.cpp

@@ -809,7 +809,7 @@ void QgsWFSThreadedFeatureDownloader::run()
     mWaitCond.wakeOne();
   }
   mDownloader->run( true, /* serialize features */
-                    0 /* user max features */ );
+                    mShared->requestLimit() /* user max features */ );
 }
 
 // -------------------------
@@ -844,14 +844,35 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource *source,
   if ( !threshold.isEmpty() )
     mWriteTransferThreshold = threshold.toInt();
 
+  // Particular case: if a single feature is requested by Fid and we already
+  // have it in cache, no need to interrupt any download
+  if ( mShared->mCacheDataProvider &&
+       mRequest.filterType() == QgsFeatureRequest::FilterFid )
+  {
+    QgsFeatureRequest requestCache = buildRequestCache( -1 );
+    QgsFeature f;
+    if ( mShared->mCacheDataProvider->getFeatures( requestCache ).nextFeature( f ) )
+    {
+      mCacheIterator = mShared->mCacheDataProvider->getFeatures( requestCache );
+      mDownloadFinished = true;
+      return;
+    }
+  }
+
   int genCounter = ( mShared->mURI.isRestrictedToRequestBBOX() && !mFilterRect.isNull() ) ?
-                   mShared->registerToCache( this, mFilterRect ) : mShared->registerToCache( this );
+                   mShared->registerToCache( this, static_cast<int>( mRequest.limit() ), mFilterRect ) :
+                   mShared->registerToCache( this, static_cast<int>( mRequest.limit() ) );
   mDownloadFinished = genCounter < 0;
   if ( !mShared->mCacheDataProvider )
     return;
 
   QgsDebugMsg( QString( "QgsWFSFeatureIterator::constructor(): genCounter=%1 " ).arg( genCounter ) );
 
+  mCacheIterator = mShared->mCacheDataProvider->getFeatures( buildRequestCache( genCounter ) );
+}
+
+QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )
+{
   QgsFeatureRequest requestCache;
   if ( mRequest.filterType() == QgsFeatureRequest::FilterFid ||
        mRequest.filterType() == QgsFeatureRequest::FilterFids )
@@ -928,7 +949,7 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource *source,
     requestCache.setSubsetOfAttributes( cacheSubSet );
   }
 
-  mCacheIterator = mShared->mCacheDataProvider->getFeatures( requestCache );
+  return requestCache;
 }
 
 QgsWFSFeatureIterator::~QgsWFSFeatureIterator()

src/providers/wfs/qgswfsfeatureiterator.h

@@ -219,6 +219,9 @@ class QgsWFSFeatureIterator : public QObject,
 
   private:
 
+    //! Translate mRequest to a request compatible of the Spatialite cache
+    QgsFeatureRequest buildRequestCache( int gencounter );
+
     bool fetchFeature( QgsFeature &f ) override;
 
     //! Copies feature attributes / geometry from srcFeature to dstFeature

src/providers/wfs/qgswfsshareddata.cpp

@@ -40,6 +40,7 @@ QgsWFSSharedData::QgsWFSSharedData( const QString &uri )
   , mSourceCRS( 0 )
   , mMaxFeatures( 0 )
   , mMaxFeaturesWasSetFromDefaultForPaging( false )
+  , mRequestLimit( 0 )
   , mHideProgressDialog( mURI.hideDownloadProgressDialog() )
   , mDistinctSelect( false )
   , mHasWarnedAboutMissingFeatureId( false )
@@ -463,7 +464,7 @@ bool QgsWFSSharedData::createCache()
   return true;
 }
 
-int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator *iterator, const QgsRectangle &rect )
+int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator *iterator, int limit, const QgsRectangle &rect )
 {
   // This locks prevents 2 readers to register at the same time (and particularly
   // destroy the current mDownloader at the same time)
@@ -527,10 +528,17 @@ int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator *iterator, const Qg
   {
     newDownloadNeeded = true;
   }
+  // If there's a ongoing download with a limitation, and the new download is
+  // unlimited, we need a new download.
+  else if ( !( mWFSVersion.startsWith( QLatin1String( "1.0" ) ) ) && limit <= 0 && mRequestLimit > 0 )
+  {
+    newDownloadNeeded = true;
+  }
 
   if ( newDownloadNeeded || !mDownloader )
   {
     mRect = rect;
+    mRequestLimit = ( limit > 0 && !( mWFSVersion.startsWith( QLatin1String( "1.0" ) ) ) ) ? limit : 0;
     // to prevent deadlock when waiting the end of the downloader thread that will try to take the mutex in serializeFeatures()
     mMutex.unlock();
     delete mDownloader;
@@ -918,16 +926,19 @@ void QgsWFSSharedData::serializeFeatures( QVector<QgsWFSFeatureGmlIdPair> &featu
 
     {
       QMutexLocker locker( &mMutex );
-      if ( !mFeatureCountExact )
-        mFeatureCount += featureListToCache.size();
-      mTotalFeaturesAttemptedToBeCached += featureListToCache.size();
-      if ( !localComputedExtent.isNull() && mComputedExtent.isNull() && !mTryFetchingOneFeature &&
-           !localComputedExtent.intersects( mCapabilityExtent ) )
+      if ( mRequestLimit != 1 )
       {
-        QgsMessageLog::logMessage( tr( "Layer extent reported by the server is not correct. "
-                                       "You may need to zoom again on layer while features are being downloaded" ), tr( "WFS" ) );
+        if ( !mFeatureCountExact )
+          mFeatureCount += featureListToCache.size();
+        mTotalFeaturesAttemptedToBeCached += featureListToCache.size();
+        if ( !localComputedExtent.isNull() && mComputedExtent.isNull() && !mTryFetchingOneFeature &&
+             !localComputedExtent.intersects( mCapabilityExtent ) )
+        {
+          QgsMessageLog::logMessage( tr( "Layer extent reported by the server is not correct. "
+                                         "You may need to zoom again on layer while features are being downloaded" ), tr( "WFS" ) );
+        }
+        mComputedExtent = localComputedExtent;
       }
-      mComputedExtent = localComputedExtent;
     }
   }
 
@@ -1010,19 +1021,22 @@ void QgsWFSSharedData::endOfDownload( bool success, int featureCount,
       mCachedRegions = QgsSpatialIndex();
     }
 
-    // In case the download was successful, we will remember this bbox
-    // and if the download reached the download limit or not
-    QgsFeature f;
-    f.setGeometry( QgsGeometry::fromRect( mRect ) );
-    QgsFeatureId id = mRegions.size();
-    f.setId( id );
-    f.initAttributes( 1 );
-    f.setAttribute( 0, QVariant( bDownloadLimit ) );
-    mRegions.push_back( f );
-    mCachedRegions.insertFeature( f );
+    if ( mRequestLimit == 0 )
+    {
+      // In case the download was successful, we will remember this bbox
+      // and if the download reached the download limit or not
+      QgsFeature f;
+      f.setGeometry( QgsGeometry::fromRect( mRect ) );
+      QgsFeatureId id = mRegions.size();
+      f.setId( id );
+      f.initAttributes( 1 );
+      f.setAttribute( 0, QVariant( bDownloadLimit ) );
+      mRegions.push_back( f );
+      mCachedRegions.insertFeature( f );
+    }
   }
 
-  if ( mRect.isEmpty() && success && !bDownloadLimit && !mFeatureCountExact )
+  if ( mRect.isEmpty() && success && !bDownloadLimit && mRequestLimit == 0 && !mFeatureCountExact )
   {
     mFeatureCountExact = true;
     if ( featureCount != mFeatureCount )
@@ -1066,6 +1080,7 @@ void QgsWFSSharedData::invalidateCache()
   mCachedRegions = QgsSpatialIndex();
   mRegions.clear();
   mRect = QgsRectangle();
+  mRequestLimit = 0;
   mGetFeatureHitsIssued = false;
   mFeatureCount = 0;
   mFeatureCountExact = false;

src/providers/wfs/qgswfsshareddata.h

@@ -55,7 +55,7 @@ class QgsWFSSharedData : public QObject
     /**
      * Used by a QgsWFSFeatureIterator to start a downloader and get the
         generation counter. */
-    int registerToCache( QgsWFSFeatureIterator *iterator, const QgsRectangle &rect = QgsRectangle() );
+    int registerToCache( QgsWFSFeatureIterator *iterator, int limit, const QgsRectangle &rect = QgsRectangle() );
 
     /**
      * Used by the rewind() method of an iterator so as to get the up-to-date
@@ -111,6 +111,9 @@ class QgsWFSSharedData : public QObject
     //! Return whether the feature download is finished
     bool downloadFinished() const { return mDownloadFinished; }
 
+    //! Return maximum number of features to download, or 0 if unlimited
+    int requestLimit() const { return mRequestLimit; }
+
   signals:
 
     //! Raise error
@@ -158,6 +161,9 @@ class QgsWFSSharedData : public QObject
     //! Whether mMaxFeatures was set to a non 0 value for the purpose of paging
     bool mMaxFeaturesWasSetFromDefaultForPaging;
 
+    //! Limit of retrieved number of features for the current request
+    int mRequestLimit;
+
     //! Server capabilities
     QgsWfsCapabilities::Capabilities mCaps;
 

tests/src/python/test_provider_wfs.py

@@ -564,8 +564,18 @@ def testWFS10(self):
 
         extent = QgsRectangle(400000.0, 5400000.0, 450000.0, 5500000.0)
         request = QgsFeatureRequest().setFilterRect(extent)
-        values = [f['INTFIELD'] for f in vl.getFeatures(request)]
-        self.assertEqual(values, [100])
+        values = [(f.id(), f['INTFIELD']) for f in vl.getFeatures(request)]
+        self.assertEqual(values[0][1], 100)
+
+        # Issue a request by id on a cached feature
+        request = QgsFeatureRequest(values[0][0])
+        values = [(f.id(), f['INTFIELD']) for f in vl.getFeatures(request)]
+        self.assertEqual(values[0][1], 100)
+
+        # Check behavior with setLimit(1)
+        request = QgsFeatureRequest().setLimit(1)
+        values = [(f.id(), f['INTFIELD']) for f in vl.getFeatures(request)]
+        self.assertEqual(values[0][1], 100)
 
     def testWFS10_outputformat_GML3(self):
         """Test WFS 1.0 with OUTPUTFORMAT=GML3"""
@@ -1271,6 +1281,30 @@ def testWFSGetOnlyFeaturesInViewExtent(self):
         values = [f['ogc_fid'] for f in vl.getFeatures(request)]
         self.assertEqual(values, [101])
 
+        # Check behavior with setLimit(1)
+        with open(sanitize(endpoint, "?SERVICE=WFS&REQUEST=GetFeature&VERSION=1.1.0&TYPENAME=my:typename&MAXFEATURES=1&SRSNAME=urn:ogc:def:crs:EPSG::4326"), 'wb') as f:
+            f.write("""
+<wfs:FeatureCollection xmlns:wfs="http://www.opengis.net/wfs"
+                       xmlns:gml="http://www.opengis.net/gml"
+                       xmlns:my="http://my"
+                       numberOfFeatures="1" timeStamp="2016-03-25T14:51:48.998Z">
+  <gml:featureMembers>
+    <my:typename gml:id="typename.12345">
+      <my:geometryProperty><gml:Point srsName="urn:ogc:def:crs:EPSG::4326"><gml:pos>70 -65</gml:pos></gml:Point></my:geometryProperty>
+      <my:ogc_fid>12345</my:ogc_fid>
+    </my:typename>
+  </gml:featureMembers>
+</wfs:FeatureCollection>""".encode('UTF-8'))
+        vl = QgsVectorLayer("url='http://" + endpoint + "' typename='my:typename' restrictToRequestBBOX=1", 'test', 'WFS')
+        request = QgsFeatureRequest().setLimit(1)
+        values = [f['ogc_fid'] for f in vl.getFeatures(request)]
+        self.assertEqual(values, [12345])
+
+        # Check that the layer extent is not built from this single feature
+        reference = QgsGeometry.fromRect(QgsRectangle(-80, 60, -50, 80))
+        vl_extent = QgsGeometry.fromRect(vl.extent())
+        assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001), 'Expected {}, got {}'.format(reference.asWkt(), vl_extent.asWkt())
+
     def testWFS20TruncatedResponse(self):
         """Test WFS 2.0 truncatedResponse"""