Skip to content

Commit e1708af

Browse files
rouaultrouault
authoredMay 23, 2018
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)
·
final-2_18_28final-2_18_21
2 parents e8e15d5 + b9be0a5 commit e1708af

File tree

7 files changed

+125
-32
lines changed

7 files changed

+125
-32
lines changed
 

‎src/core/qgsgml.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -856,7 +856,7 @@ void QgsGmlStreamingParser::endElement( const XML_Char* el )
856856
const int localNameLen = ( pszSep ) ? ( int )( elLen - nsLen ) - 1 : elLen;
857857
ParseMode theParseMode( mParseModeStack.isEmpty() ? none : mParseModeStack.top() );
858858

859-
mDimension = mDimensionStack.isEmpty() ? 0 : mDimensionStack.top() ;
859+
mDimension = mDimensionStack.isEmpty() ? 0 : mDimensionStack.pop() ;
860860

861861
const bool isGMLNS = ( nsLen == mGMLNameSpaceURI.size() && mGMLNameSpaceURIPtr && memcmp( el, mGMLNameSpaceURIPtr, nsLen ) == 0 );
862862

‎src/providers/wfs/qgswfsfeatureiterator.cpp

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,7 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
436436
int pagingIter = 1;
437437
QString gmlIdFirstFeatureFirstIter;
438438
bool disablePaging = false;
439+
// Top level loop to do feature paging in WFS 2
439440
while ( true )
440441
{
441442
success = true;
@@ -456,9 +457,14 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
456457
false /* cache */ );
457458

458459
int featureCountForThisResponse = 0;
460+
bool bytesStillAvailableInReply = false;
461+
// Loop until there is no data coming from the current request
459462
while ( true )
460463
{
461-
loop.exec( QEventLoop::ExcludeUserInputEvents );
464+
if ( !bytesStillAvailableInReply )
465+
{
466+
loop.exec( QEventLoop::ExcludeUserInputEvents );
467+
}
462468
if ( mStop )
463469
{
464470
interrupted = true;
@@ -475,7 +481,10 @@ void QgsWFSFeatureDownloader::run( bool serializeFeatures, int maxFeatures )
475481
bool finished = false;
476482
if ( mReply )
477483
{
478-
data = mReply->readAll();
484+
// Limit the number of bytes to process at once, to avoid the GML parser to
485+
// create too many objects.
486+
data = mReply->read( 10 * 1024 * 1024 );
487+
bytesStillAvailableInReply = mReply->bytesAvailable() > 0;
479488
}
480489
else
481490
{
@@ -779,7 +788,7 @@ void QgsWFSThreadedFeatureDownloader::run()
779788
mWaitCond.wakeOne();
780789
}
781790
mDownloader->run( true, /* serialize features */
782-
0 /* user max features */ );
791+
mShared->requestLimit() /* user max features */ );
783792
}
784793

785794
// -------------------------
@@ -805,14 +814,35 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource* source,
805814
if ( !threshold.isEmpty() )
806815
mWriteTransferThreshold = threshold.toInt();
807816

817+
// Particular case: if a single feature is requested by Fid and we already
818+
// have it in cache, no need to interrupt any download
819+
if ( mShared->mCacheDataProvider &&
820+
mRequest.filterType() == QgsFeatureRequest::FilterFid )
821+
{
822+
QgsFeatureRequest requestCache = buildRequestCache( -1 );
823+
QgsFeature f;
824+
if ( mShared->mCacheDataProvider->getFeatures( requestCache ).nextFeature( f ) )
825+
{
826+
mCacheIterator = mShared->mCacheDataProvider->getFeatures( requestCache );
827+
mDownloadFinished = true;
828+
return;
829+
}
830+
}
831+
808832
int genCounter = ( mShared->mURI.isRestrictedToRequestBBOX() && !request.filterRect().isNull() ) ?
809-
mShared->registerToCache( this, request.filterRect() ) : mShared->registerToCache( this );
833+
mShared->registerToCache( this, static_cast<int>( mRequest.limit() ), request.filterRect() ) :
834+
mShared->registerToCache( this, static_cast<int>( mRequest.limit() ) );
810835
mDownloadFinished = genCounter < 0;
811836
if ( !mShared->mCacheDataProvider )
812837
return;
813838

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

841+
mCacheIterator = mShared->mCacheDataProvider->getFeatures( buildRequestCache( genCounter ) );
842+
}
843+
844+
QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )
845+
{
816846
QgsFeatureRequest requestCache;
817847
if ( mRequest.filterType() == QgsFeatureRequest::FilterFid ||
818848
mRequest.filterType() == QgsFeatureRequest::FilterFids )
@@ -874,7 +904,7 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource* source,
874904
requestCache.setSubsetOfAttributes( cacheSubSet );
875905
}
876906

877-
mCacheIterator = mShared->mCacheDataProvider->getFeatures( requestCache );
907+
return requestCache;
878908
}
879909

880910
QgsWFSFeatureIterator::~QgsWFSFeatureIterator()

‎src/providers/wfs/qgswfsfeatureiterator.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,10 @@ class QgsWFSFeatureIterator : public QObject,
213213

214214
private:
215215

216-
bool fetchFeature( QgsFeature& f ) override;
216+
//! Translate mRequest to a request compatible of the Spatialite cache
217+
QgsFeatureRequest buildRequestCache( int gencounter );
218+
219+
bool fetchFeature( QgsFeature &f ) override;
217220

218221
/** Copies feature attributes / geometry from srcFeature to dstFeature*/
219222
void copyFeature( const QgsFeature& srcFeature, QgsFeature& dstFeature );

‎src/providers/wfs/qgswfsrequest.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
#include <QNetworkCacheMetaData>
2525
#include <QCryptographicHash> // just for testin file:// fake_qgis_http_endpoint hack
2626

27+
const qint64 READ_BUFFER_SIZE_HINT = 1024 * 1024;
28+
2729
QgsWFSRequest::QgsWFSRequest( const QString& theUri )
2830
: mUri( theUri )
2931
, mReply( nullptr )
@@ -116,6 +118,7 @@ bool QgsWFSRequest::sendGET( const QUrl& url, bool synchronous, bool forceRefres
116118
}
117119

118120
mReply = QgsNetworkAccessManager::instance()->get( request );
121+
mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
119122
if ( !mUri.auth().setAuthorizationReply( mReply ) )
120123
{
121124
mErrorCode = QgsWFSRequest::NetworkError;
@@ -167,6 +170,7 @@ bool QgsWFSRequest::sendPOST( const QUrl& url, const QString& contentTypeHeader,
167170
request.setHeader( QNetworkRequest::ContentTypeHeader, contentTypeHeader );
168171

169172
mReply = QgsNetworkAccessManager::instance()->post( request, data );
173+
mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
170174
if ( !mUri.auth().setAuthorizationReply( mReply ) )
171175
{
172176
mErrorCode = QgsWFSRequest::NetworkError;
@@ -257,6 +261,7 @@ void QgsWFSRequest::replyFinished()
257261

258262
QgsDebugMsg( QString( "redirected: %1 forceRefresh=%2" ).arg( redirect.toString() ).arg( mForceRefresh ) );
259263
mReply = QgsNetworkAccessManager::instance()->get( request );
264+
mReply->setReadBufferSize( READ_BUFFER_SIZE_HINT );
260265
if ( !mUri.auth().setAuthorizationReply( mReply ) )
261266
{
262267
mResponse.clear();

‎src/providers/wfs/qgswfsshareddata.cpp

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ QgsWFSSharedData::QgsWFSSharedData( const QString& uri )
4040
, mCacheDataProvider( nullptr )
4141
, mMaxFeatures( 0 )
4242
, mMaxFeaturesWasSetFromDefaultForPaging( false )
43+
, mRequestLimit( 0 )
4344
, mHideProgressDialog( mURI.hideDownloadProgressDialog() )
4445
, mDistinctSelect( false )
4546
, mHasWarnedAboutMissingFeatureId( false )
@@ -478,7 +479,7 @@ bool QgsWFSSharedData::createCache()
478479
return true;
479480
}
480481

481-
int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator* iterator, QgsRectangle rect )
482+
int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator *iterator, int limit, const QgsRectangle &rect )
482483
{
483484
// This locks prevents 2 readers to register at the same time (and particularly
484485
// destroy the current mDownloader at the same time)
@@ -542,10 +543,17 @@ int QgsWFSSharedData::registerToCache( QgsWFSFeatureIterator* iterator, QgsRecta
542543
{
543544
newDownloadNeeded = true;
544545
}
546+
// If there's a ongoing download with a limitation, and the new download is
547+
// unlimited, we need a new download.
548+
else if ( !( mWFSVersion.startsWith( QLatin1String( "1.0" ) ) ) && limit <= 0 && mRequestLimit > 0 )
549+
{
550+
newDownloadNeeded = true;
551+
}
545552

546553
if ( newDownloadNeeded || !mDownloader )
547554
{
548555
mRect = rect;
556+
mRequestLimit = ( limit > 0 && !( mWFSVersion.startsWith( QLatin1String( "1.0" ) ) ) ) ? limit : 0;
549557
// to prevent deadlock when waiting the end of the downloader thread that will try to take the mutex in serializeFeatures()
550558
mMutex.unlock();
551559
delete mDownloader;
@@ -939,16 +947,19 @@ void QgsWFSSharedData::serializeFeatures( QVector<QgsWFSFeatureGmlIdPair>& featu
939947

940948
{
941949
QMutexLocker locker( &mMutex );
942-
if ( !mFeatureCountExact )
943-
mFeatureCount += featureListToCache.size();
944-
mTotalFeaturesAttemptedToBeCached += featureListToCache.size();
945-
if ( !localComputedExtent.isNull() && mComputedExtent.isNull() && !mTryFetchingOneFeature &&
946-
!localComputedExtent.intersects( mCapabilityExtent ) )
950+
if ( mRequestLimit != 1 )
947951
{
948-
QgsMessageLog::logMessage( tr( "Layer extent reported by the server is not correct. "
949-
"You may need to zoom again on layer while features are being downloaded" ), tr( "WFS" ) );
952+
if ( !mFeatureCountExact )
953+
mFeatureCount += featureListToCache.size();
954+
mTotalFeaturesAttemptedToBeCached += featureListToCache.size();
955+
if ( !localComputedExtent.isNull() && mComputedExtent.isNull() && !mTryFetchingOneFeature &&
956+
!localComputedExtent.intersects( mCapabilityExtent ) )
957+
{
958+
QgsMessageLog::logMessage( tr( "Layer extent reported by the server is not correct. "
959+
"You may need to zoom again on layer while features are being downloaded" ), tr( "WFS" ) );
960+
}
961+
mComputedExtent = localComputedExtent;
950962
}
951-
mComputedExtent = localComputedExtent;
952963
}
953964
}
954965

@@ -1031,19 +1042,22 @@ void QgsWFSSharedData::endOfDownload( bool success, int featureCount,
10311042
mCachedRegions = QgsSpatialIndex();
10321043
}
10331044

1034-
// In case the download was successful, we will remember this bbox
1035-
// and if the download reached the download limit or not
1036-
QgsFeature f;
1037-
f.setGeometry( QgsGeometry::fromRect( mRect ) );
1038-
QgsFeatureId id = mRegions.size();
1039-
f.setFeatureId( id );
1040-
f.initAttributes( 1 );
1041-
f.setAttribute( 0, QVariant( bDownloadLimit ) );
1042-
mRegions.push_back( f );
1043-
mCachedRegions.insertFeature( f );
1045+
if ( mRequestLimit == 0 )
1046+
{
1047+
// In case the download was successful, we will remember this bbox
1048+
// and if the download reached the download limit or not
1049+
QgsFeature f;
1050+
f.setGeometry( QgsGeometry::fromRect( mRect ) );
1051+
QgsFeatureId id = mRegions.size();
1052+
f.setFeatureId( id );
1053+
f.initAttributes( 1 );
1054+
f.setAttribute( 0, QVariant( bDownloadLimit ) );
1055+
mRegions.push_back( f );
1056+
mCachedRegions.insertFeature( f );
1057+
}
10441058
}
10451059

1046-
if ( mRect.isEmpty() && success && !bDownloadLimit && !mFeatureCountExact )
1060+
if ( mRect.isEmpty() && success && !bDownloadLimit && mRequestLimit == 0 && !mFeatureCountExact )
10471061
{
10481062
mFeatureCountExact = true;
10491063
if ( featureCount != mFeatureCount )
@@ -1087,6 +1101,7 @@ void QgsWFSSharedData::invalidateCache()
10871101
mCachedRegions = QgsSpatialIndex();
10881102
mRegions.clear();
10891103
mRect = QgsRectangle();
1104+
mRequestLimit = 0;
10901105
mGetFeatureHitsIssued = false;
10911106
mFeatureCount = 0;
10921107
mFeatureCountExact = false;

‎src/providers/wfs/qgswfsshareddata.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ class QgsWFSSharedData : public QObject
5353

5454
/** Used by a QgsWFSFeatureIterator to start a downloader and get the
5555
generation counter. */
56-
int registerToCache( QgsWFSFeatureIterator* iterator, QgsRectangle rect = QgsRectangle() );
56+
int registerToCache( QgsWFSFeatureIterator *iterator, int limit, const QgsRectangle &rect = QgsRectangle() );
5757

5858
/** Used by the rewind() method of an iterator so as to get the up-to-date
5959
generation counter. */
@@ -106,6 +106,9 @@ class QgsWFSSharedData : public QObject
106106
/** Return whether the feature download is finished */
107107
bool downloadFinished() const { return mDownloadFinished; }
108108

109+
//! Return maximum number of features to download, or 0 if unlimited
110+
int requestLimit() const { return mRequestLimit; }
111+
109112
signals:
110113

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

159+
//! Limit of retrieved number of features for the current request
160+
int mRequestLimit;
161+
156162
/** Server capabilities */
157163
QgsWFSCapabilities::Capabilities mCaps;
158164

‎tests/src/python/test_provider_wfs.py

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -488,8 +488,18 @@ def testWFS10(self):
488488

489489
extent = QgsRectangle(400000.0, 5400000.0, 450000.0, 5500000.0)
490490
request = QgsFeatureRequest().setFilterRect(extent)
491-
values = [f['INTFIELD'] for f in vl.getFeatures(request)]
492-
self.assertEqual(values, [100])
491+
values = [(f.id(), f['INTFIELD']) for f in vl.getFeatures(request)]
492+
self.assertEqual(values[0][1], 100)
493+
494+
# Issue a request by id on a cached feature
495+
request = QgsFeatureRequest(values[0][0])
496+
values = [(f.id(), f['INTFIELD']) for f in vl.getFeatures(request)]
497+
self.assertEqual(values[0][1], 100)
498+
499+
# Check behavior with setLimit(1)
500+
request = QgsFeatureRequest().setLimit(1)
501+
values = [(f.id(), f['INTFIELD']) for f in vl.getFeatures(request)]
502+
self.assertEqual(values[0][1], 100)
493503

494504
def testWFS10_outputformat_GML3(self):
495505
"""Test WFS 1.0 with OUTPUTFORMAT=GML3"""
@@ -1182,6 +1192,30 @@ def testWFSGetOnlyFeaturesInViewExtent(self):
11821192
values = [f['ogc_fid'] for f in vl.getFeatures(request)]
11831193
self.assertEqual(values, [101])
11841194

1195+
# Check behavior with setLimit(1)
1196+
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:
1197+
f.write("""
1198+
<wfs:FeatureCollection xmlns:wfs="http://www.opengis.net/wfs"
1199+
xmlns:gml="http://www.opengis.net/gml"
1200+
xmlns:my="http://my"
1201+
numberOfFeatures="1" timeStamp="2016-03-25T14:51:48.998Z">
1202+
<gml:featureMembers>
1203+
<my:typename gml:id="typename.12345">
1204+
<my:geometryProperty><gml:Point srsName="urn:ogc:def:crs:EPSG::4326"><gml:pos>70 -65</gml:pos></gml:Point></my:geometryProperty>
1205+
<my:ogc_fid>12345</my:ogc_fid>
1206+
</my:typename>
1207+
</gml:featureMembers>
1208+
</wfs:FeatureCollection>""".encode('UTF-8'))
1209+
vl = QgsVectorLayer("url='http://" + endpoint + "' typename='my:typename' restrictToRequestBBOX=1", 'test', 'WFS')
1210+
request = QgsFeatureRequest().setLimit(1)
1211+
values = [f['ogc_fid'] for f in vl.getFeatures(request)]
1212+
self.assertEqual(values, [12345])
1213+
1214+
# Check that the layer extent is not built from this single feature
1215+
reference = QgsGeometry.fromRect(QgsRectangle(-80, 60, -50, 80))
1216+
vl_extent = QgsGeometry.fromRect(vl.extent())
1217+
assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001), 'Expected {}, got {}'.format(reference.asWkt(), vl_extent.asWkt())
1218+
11851219
def testWFS20TruncatedResponse(self):
11861220
"""Test WFS 2.0 truncatedResponse"""
11871221

@@ -2161,15 +2195,15 @@ def testGeomedia(self):
21612195
# Extent before downloading features
21622196
reference = QgsGeometry.fromRect(QgsRectangle(243900.3520259926444851, 4427769.1559739429503679, 1525592.3040170343592763, 5607994.6020106188952923))
21632197
vl_extent = QgsGeometry.fromRect(vl.extent())
2164-
assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
2198+
assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.1), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
21652199

21662200
# Download all features
21672201
features = [f for f in vl.getFeatures()]
21682202
self.assertEqual(len(features), 2)
21692203

21702204
reference = QgsGeometry.fromRect(QgsRectangle(500000, 4500000, 510000, 4510000))
21712205
vl_extent = QgsGeometry.fromRect(vl.extent())
2172-
assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.00001), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
2206+
assert QgsGeometry.compare(vl_extent.asPolygon()[0], reference.asPolygon()[0], 0.1), 'Expected {}, got {}'.format(reference.exportToWkt(), vl_extent.exportToWkt())
21732207
self.assertEqual(features[0]['intfield'], 1)
21742208
self.assertEqual(features[1]['intfield'], 2)
21752209

0 commit comments

Comments
 (0)
Please sign in to comment.