Merge pull request #7052 from rouault/fix_18740_qgis2_18

[WFS provider] 2.18 / Avoid request by feature id to cause a full layer download (fixes #18740)
qgis · May 23, 2018 · e1708af · e1708af
2 parents e8e15d5 + b9be0a5
commit e1708af
Show file tree

Hide file tree

Showing 7 changed files with 125 additions and 32 deletions.
diff --git a/src/core/qgsgml.cpp b/src/core/qgsgml.cpp
@@ -856,7 +856,7 @@ void QgsGmlStreamingParser::endElement( const XML_Char* el )
   const int localNameLen = ( pszSep ) ? ( int )( elLen - nsLen ) - 1 : elLen;
   ParseMode theParseMode( mParseModeStack.isEmpty() ? none : mParseModeStack.top() );
 
-  mDimension = mDimensionStack.isEmpty() ? 0 : mDimensionStack.top() ;
+  mDimension = mDimensionStack.isEmpty() ? 0 : mDimensionStack.pop() ;
 
   const bool isGMLNS = ( nsLen == mGMLNameSpaceURI.size() && mGMLNameSpaceURIPtr && memcmp( el, mGMLNameSpaceURIPtr, nsLen ) == 0 );
 

diff --git a/src/providers/wfs/qgswfsfeatureiterator.cpp b/src/providers/wfs/qgswfsfeatureiterator.cpp
@@ -436,6 +436,7 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
   int pagingIter = 1;
   QString gmlIdFirstFeatureFirstIter;
   bool disablePaging = false;
+  // Top level loop to do feature paging in WFS 2
   while ( true )
   {
     success = true;
@@ -456,9 +457,14 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
              false /* cache */ );
 
     int featureCountForThisResponse = 0;
+    bool bytesStillAvailableInReply = false;
+    // Loop until there is no data coming from the current request
     while ( true )
     {
-      loop.exec( QEventLoop::ExcludeUserInputEvents );
+      if ( !bytesStillAvailableInReply )
+      {
+        loop.exec( QEventLoop::ExcludeUserInputEvents );
+      }
       if ( mStop )
       {
         interrupted = true;
@@ -475,7 +481,10 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
       bool finished = false;
       if ( mReply )
       {
-        data = mReply->readAll();
+        // Limit the number of bytes to process at once, to avoid the GML parser to
+        // create too many objects.
+        data = mReply->read( 10 * 1024 * 1024 );
+        bytesStillAvailableInReply = mReply->bytesAvailable() > 0;
       }
       else
       {
@@ -779,7 +788,7 @@ void QgsWFSThreadedFeatureDownloader::run()
     mWaitCond.wakeOne();
   }
   mDownloader->run( true, /* serialize features */
-                    0 /* user max features */ );
+                    mShared->requestLimit() /* user max features */ );
 }
 
 // -------------------------
@@ -805,14 +814,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() && !request.filterRect().isNull() ) ?
-                   mShared->registerToCache( this, request.filterRect() ) : mShared->registerToCache( this );
+                   mShared->registerToCache( this, static_cast<int>( mRequest.limit() ), request.filterRect() ) :
+                   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 )
@@ -874,7 +904,7 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource* source,
     requestCache.setSubsetOfAttributes( cacheSubSet );
   }
 
-  mCacheIterator = mShared->mCacheDataProvider->getFeatures( requestCache );
+  return requestCache;
 }
 
 QgsWFSFeatureIterator::~QgsWFSFeatureIterator()

diff --git a/src/providers/wfs/qgswfsfeatureiterator.h b/src/providers/wfs/qgswfsfeatureiterator.h
@@ -213,7 +213,10 @@ class QgsWFSFeatureIterator : public QObject,
 
   private:
 
-    bool fetchFeature( QgsFeature& f ) override;
+    //! 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*/
     void copyFeature( const QgsFeature& srcFeature, QgsFeature& dstFeature );

diff --git a/src/providers/wfs/qgswfsrequest.cpp b/src/providers/wfs/qgswfsrequest.cpp
@@ -24,6 +24,8 @@
 #include <QNetworkCacheMetaData>
 #include <QCryptographicHash> // just for testin file:// fake_qgis_http_endpoint hack
 
+const qint64 READ_BUFFER_SIZE_HINT = 1024 * 1024;
+
 QgsWFSRequest::QgsWFSRequest( const QString& theUri )
     : mUri( theUri )
     , mReply( nullptr )
@@ -116,6 +118,7 @@ bool QgsWFSRequest::sendGET( const QUrl& url, bool synchronous, bool forceRefres
   }
 
   mReply = QgsNetworkAccessManager::instance()->get( request );
+  mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
   if ( !mUri.auth().setAuthorizationReply( mReply ) )
   {
     mErrorCode = QgsWFSRequest::NetworkError;
@@ -167,6 +170,7 @@ bool QgsWFSRequest::sendPOST( const QUrl& url, const QString& contentTypeHeader,
   request.setHeader( QNetworkRequest::ContentTypeHeader, contentTypeHeader );
 
   mReply = QgsNetworkAccessManager::instance()->post( request, data );
+  mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
   if ( !mUri.auth().setAuthorizationReply( mReply ) )
   {
     mErrorCode = QgsWFSRequest::NetworkError;
@@ -257,6 +261,7 @@ void QgsWFSRequest::replyFinished()
 
           QgsDebugMsg( QString( "redirected: %1 forceRefresh=%2" ).arg( redirect.toString() ).arg( mForceRefresh ) );
           mReply = QgsNetworkAccessManager::instance()->get( request );
+          mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
           if ( !mUri.auth().setAuthorizationReply( mReply ) )
           {
             mResponse.clear();

diff --git a/src/providers/wfs/qgswfsshareddata.cpp b/src/providers/wfs/qgswfsshareddata.cpp
@@ -40,6 +40,7 @@ QgsWFSSharedData::QgsWFSSharedData( const QString& uri )
     , mCacheDataProvider( nullptr )
     , mMaxFeatures( 0 )
     , mMaxFeaturesWasSetFromDefaultForPaging( false )
+    , mRequestLimit( 0 )
     , mHideProgressDialog( mURI.hideDownloadProgressDialog() )
     , mDistinctSelect( false )
     , mHasWarnedAboutMissingFeatureId( false )
@@ -478,7 +479,7 @@ bool QgsWFSSharedData::createCache()
   return true;
 }
 
-int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator* iterator, 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)
@@ -542,10 +543,17 @@ int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator* iterator, QgsRecta
   {
     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;
@@ -939,16 +947,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;
     }
   }
 
@@ -1031,19 +1042,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.setFeatureId( 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.setFeatureId( 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 )
@@ -1087,6 +1101,7 @@ void QgsWFSSharedData::invalidateCache()
   mCachedRegions = QgsSpatialIndex();
   mRegions.clear();
   mRect = QgsRectangle();
+  mRequestLimit = 0;
   mGetFeatureHitsIssued = false;
   mFeatureCount = 0;
   mFeatureCountExact = false;

diff --git a/src/providers/wfs/qgswfsshareddata.h b/src/providers/wfs/qgswfsshareddata.h
@@ -53,7 +53,7 @@ class QgsWFSSharedData : public QObject
 
     /** Used by a QgsWFSFeatureIterator to start a downloader and get the
         generation counter. */
-    int registerToCache( QgsWFSFeatureIterator* iterator, 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
         generation counter. */
@@ -106,6 +106,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 */
@@ -153,6 +156,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;
 

diff --git a/tests/src/python/test_provider_wfs.py b/tests/src/python/test_provider_wfs.py
@@ -488,8 +488,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"""
@@ -1182,6 +1192,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"""
 
@@ -2161,15 +2195,15 @@ def testGeomedia(self):
         # Extent before downloading features
         reference = QgsGeometry.fromRect(QgsRectangle(243900.3520259926444851, 4427769.1559739429503679, 1525592.3040170343592763, 5607994.6020106188952923))
         vl_extent = QgsGeometry.fromRect(vl.extent())
-        assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
+        assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.1), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
 
         # Download all features
         features = [f for f in vl.getFeatures()]
         self.assertEqual(len(features), 2)
 
         reference = QgsGeometry.fromRect(QgsRectangle(500000, 4500000, 510000, 4510000))
         vl_extent = QgsGeometry.fromRect(vl.extent())
-        assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
+        assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.1), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
         self.assertEqual(features[0]['intfield'], 1)
         self.assertEqual(features[1]['intfield'], 2)