Skip to content

Commit

Permalink
[WFS provider] Avoid request by feature id to cause a full layer down…
Browse files Browse the repository at this point in the history 
…load (fixes #18740)

Backport of 6cf1c50
  • Loading branch information
@rouault
rouault committed May 22, 2018
1 parent 1121565 commit b9be0a5
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 29 deletions.
27 changes: 24 additions & 3 deletions src/providers/wfs/qgswfsfeatureiterator.cpp
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -788,7 +788,7 @@ void QgsWFSThreadedFeatureDownloader::run()
mWaitCond.wakeOne(); mWaitCond.wakeOne();
} }
mDownloader->run( true, /* serialize features */ mDownloader->run( true, /* serialize features */
0 /* user max features */ ); mShared->requestLimit() /* user max features */ );
} }


// ------------------------- // -------------------------
Expand All @@ -814,14 +814,35 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource* source,
if ( !threshold.isEmpty() ) if ( !threshold.isEmpty() )
mWriteTransferThreshold = threshold.toInt(); 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() ) ? 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; mDownloadFinished = genCounter < 0;
if ( !mShared->mCacheDataProvider ) if ( !mShared->mCacheDataProvider )
return; return;


QgsDebugMsg( QString( "QgsWFSFeatureIterator::constructor(): genCounter=%1 " ).arg( genCounter ) ); QgsDebugMsg( QString( "QgsWFSFeatureIterator::constructor(): genCounter=%1 " ).arg( genCounter ) );


mCacheIterator = mShared->mCacheDataProvider->getFeatures( buildRequestCache( genCounter ) );
}

QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )
{
QgsFeatureRequest requestCache; QgsFeatureRequest requestCache;
if ( mRequest.filterType() == QgsFeatureRequest::FilterFid || if ( mRequest.filterType() == QgsFeatureRequest::FilterFid ||
mRequest.filterType() == QgsFeatureRequest::FilterFids ) mRequest.filterType() == QgsFeatureRequest::FilterFids )
Expand Down Expand Up @@ -883,7 +904,7 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource* source,
requestCache.setSubsetOfAttributes( cacheSubSet ); requestCache.setSubsetOfAttributes( cacheSubSet );
} }


mCacheIterator = mShared->mCacheDataProvider->getFeatures( requestCache ); return requestCache;
} }


QgsWFSFeatureIterator::~QgsWFSFeatureIterator() QgsWFSFeatureIterator::~QgsWFSFeatureIterator()
Expand Down
5 changes: 4 additions & 1 deletion src/providers/wfs/qgswfsfeatureiterator.h
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -213,7 +213,10 @@ class QgsWFSFeatureIterator : public QObject,


private: 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*/ /** Copies feature attributes / geometry from srcFeature to dstFeature*/
void copyFeature( const QgsFeature& srcFeature, QgsFeature& dstFeature ); void copyFeature( const QgsFeature& srcFeature, QgsFeature& dstFeature );
Expand Down
55 changes: 35 additions & 20 deletions src/providers/wfs/qgswfsshareddata.cpp
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ QgsWFSSharedData::QgsWFSSharedData( const QString& uri )
, mCacheDataProvider( nullptr ) , mCacheDataProvider( nullptr )
, mMaxFeatures( 0 ) , mMaxFeatures( 0 )
, mMaxFeaturesWasSetFromDefaultForPaging( false ) , mMaxFeaturesWasSetFromDefaultForPaging( false )
, mRequestLimit( 0 )
, mHideProgressDialog( mURI.hideDownloadProgressDialog() ) , mHideProgressDialog( mURI.hideDownloadProgressDialog() )
, mDistinctSelect( false ) , mDistinctSelect( false )
, mHasWarnedAboutMissingFeatureId( false ) , mHasWarnedAboutMissingFeatureId( false )
Expand Down Expand Up @@ -478,7 +479,7 @@ bool QgsWFSSharedData::createCache()
return true; 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 // This locks prevents 2 readers to register at the same time (and particularly
// destroy the current mDownloader at the same time) // destroy the current mDownloader at the same time)
Expand Down Expand Up @@ -542,10 +543,17 @@ int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator* iterator, QgsRecta
{ {
newDownloadNeeded = true; 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 ) if ( newDownloadNeeded || !mDownloader )
{ {
mRect = rect; 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() // to prevent deadlock when waiting the end of the downloader thread that will try to take the mutex in serializeFeatures()
mMutex.unlock(); mMutex.unlock();
delete mDownloader; delete mDownloader;
Expand Down Expand Up @@ -939,16 +947,19 @@ void QgsWFSSharedData::serializeFeatures( QVector<QgsWFSFeatureGmlIdPair>& featu


{ {
QMutexLocker locker( &mMutex ); QMutexLocker locker( &mMutex );
if ( !mFeatureCountExact ) if ( mRequestLimit != 1 )
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. " if ( !mFeatureCountExact )
"You may need to zoom again on layer while features are being downloaded" ), tr( "WFS" ) ); 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;
} }
} }


Expand Down Expand Up @@ -1031,19 +1042,22 @@ void QgsWFSSharedData::endOfDownload( bool success, int featureCount,
mCachedRegions = QgsSpatialIndex(); mCachedRegions = QgsSpatialIndex();
} }


// In case the download was successful, we will remember this bbox if ( mRequestLimit == 0 )
// and if the download reached the download limit or not {
QgsFeature f; // In case the download was successful, we will remember this bbox
f.setGeometry( QgsGeometry::fromRect( mRect ) ); // and if the download reached the download limit or not
QgsFeatureId id = mRegions.size(); QgsFeature f;
f.setFeatureId( id ); f.setGeometry( QgsGeometry::fromRect( mRect ) );
f.initAttributes( 1 ); QgsFeatureId id = mRegions.size();
f.setAttribute( 0, QVariant( bDownloadLimit ) ); f.setFeatureId( id );
mRegions.push_back( f ); f.initAttributes( 1 );
mCachedRegions.insertFeature( f ); 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; mFeatureCountExact = true;
if ( featureCount != mFeatureCount ) if ( featureCount != mFeatureCount )
Expand Down Expand Up @@ -1087,6 +1101,7 @@ void QgsWFSSharedData::invalidateCache()
mCachedRegions = QgsSpatialIndex(); mCachedRegions = QgsSpatialIndex();
mRegions.clear(); mRegions.clear();
mRect = QgsRectangle(); mRect = QgsRectangle();
mRequestLimit = 0;
mGetFeatureHitsIssued = false; mGetFeatureHitsIssued = false;
mFeatureCount = 0; mFeatureCount = 0;
mFeatureCountExact = false; mFeatureCountExact = false;
Expand Down
8 changes: 7 additions & 1 deletion src/providers/wfs/qgswfsshareddata.h
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class QgsWFSSharedData : public QObject


/** Used by a QgsWFSFeatureIterator to start a downloader and get the /** Used by a QgsWFSFeatureIterator to start a downloader and get the
generation counter. */ 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 /** Used by the rewind() method of an iterator so as to get the up-to-date
generation counter. */ generation counter. */
Expand Down Expand Up @@ -106,6 +106,9 @@ class QgsWFSSharedData : public QObject
/** Return whether the feature download is finished */ /** Return whether the feature download is finished */
bool downloadFinished() const { return mDownloadFinished; } bool downloadFinished() const { return mDownloadFinished; }


//! Return maximum number of features to download, or 0 if unlimited
int requestLimit() const { return mRequestLimit; }

signals: signals:


/** Raise error */ /** Raise error */
Expand Down Expand Up @@ -153,6 +156,9 @@ class QgsWFSSharedData : public QObject
/** Whether mMaxFeatures was set to a non 0 value for the purpose of paging */ /** Whether mMaxFeatures was set to a non 0 value for the purpose of paging */
bool mMaxFeaturesWasSetFromDefaultForPaging; bool mMaxFeaturesWasSetFromDefaultForPaging;


//! Limit of retrieved number of features for the current request
int mRequestLimit;

/** Server capabilities */ /** Server capabilities */
QgsWFSCapabilities::Capabilities mCaps; QgsWFSCapabilities::Capabilities mCaps;


Expand Down
42 changes: 38 additions & 4 deletions tests/src/python/test_provider_wfs.py
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -488,8 +488,18 @@ def testWFS10(self):


extent = QgsRectangle(400000.0, 5400000.0, 450000.0, 5500000.0) extent = QgsRectangle(400000.0, 5400000.0, 450000.0, 5500000.0)
request = QgsFeatureRequest().setFilterRect(extent) request = QgsFeatureRequest().setFilterRect(extent)
values = [f['INTFIELD'] for f in vl.getFeatures(request)] values = [(f.id(), f['INTFIELD']) for f in vl.getFeatures(request)]
self.assertEqual(values, [100]) 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): def testWFS10_outputformat_GML3(self):
"""Test WFS 1.0 with OUTPUTFORMAT=GML3""" """Test WFS 1.0 with OUTPUTFORMAT=GML3"""
Expand Down Expand Up @@ -1182,6 +1192,30 @@ def testWFSGetOnlyFeaturesInViewExtent(self):
values = [f['ogc_fid'] for f in vl.getFeatures(request)] values = [f['ogc_fid'] for f in vl.getFeatures(request)]
self.assertEqual(values, [101]) 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): def testWFS20TruncatedResponse(self):
"""Test WFS 2.0 truncatedResponse""" """Test WFS 2.0 truncatedResponse"""


Expand Down Expand Up @@ -2161,15 +2195,15 @@ def testGeomedia(self):
# Extent before downloading features # Extent before downloading features
reference = QgsGeometry.fromRect(QgsRectangle(243900.3520259926444851, 4427769.1559739429503679, 1525592.3040170343592763, 5607994.6020106188952923)) reference = QgsGeometry.fromRect(QgsRectangle(243900.3520259926444851, 4427769.1559739429503679, 1525592.3040170343592763, 5607994.6020106188952923))
vl_extent = QgsGeometry.fromRect(vl.extent()) 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 # Download all features
features = [f for f in vl.getFeatures()] features = [f for f in vl.getFeatures()]
self.assertEqual(len(features), 2) self.assertEqual(len(features), 2)


reference = QgsGeometry.fromRect(QgsRectangle(500000, 4500000, 510000, 4510000)) reference = QgsGeometry.fromRect(QgsRectangle(500000, 4500000, 510000, 4510000))
vl_extent = QgsGeometry.fromRect(vl.extent()) 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[0]['intfield'], 1)
self.assertEqual(features[1]['intfield'], 2) self.assertEqual(features[1]['intfield'], 2)


Expand Down

0 comments on commit b9be0a5

Please sign in to comment.