[afs] Fix/optimise handling of filter rect feature requests

Before a filter rect request would usually force fetching every single feature from the server before the request could be complete. Instead, if a filter rect is passed we first obtain a list from the server of matching features within this rect, and then iterate only over those. Fixes broken (multi-minute hang) identify tool use on AFS layers.
qgis · Feb 20, 2018 · 9e023bd · 9e023bd
1 parent 45ded37
commit 9e023bd
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 24 deletions.
diff --git a/src/providers/arcgisrest/qgsafsfeatureiterator.cpp b/src/providers/arcgisrest/qgsafsfeatureiterator.cpp
@@ -18,6 +18,7 @@
 #include "qgsmessagelog.h"
 #include "geometry/qgsgeometry.h"
 #include "qgsexception.h"
+#include "qgsarcgisrestutils.h"
 
 QgsAfsFeatureSource::QgsAfsFeatureSource( const std::shared_ptr<QgsAfsSharedData> &sharedData )
   : mSharedData( sharedData )
@@ -59,17 +60,39 @@ QgsAfsFeatureIterator::QgsAfsFeatureIterator( QgsAfsFeatureSource *source, bool
     return;
   }
 
+  QgsFeatureIds requestIds;
   if ( mRequest.filterType() == QgsFeatureRequest::FilterFids )
   {
-    mUsingFeatureIdList = true;
-    mFeatureIdList = mRequest.filterFids();
-    mRemainingFeatureIds = mFeatureIdList;
+    requestIds = mRequest.filterFids();
   }
   else if ( mRequest.filterType() == QgsFeatureRequest::FilterFid )
   {
-    mFeatureIdList.insert( mRequest.filterFid() );
-    mRemainingFeatureIds = mFeatureIdList;
+    requestIds.insert( mRequest.filterFid() );
   }
+
+  if ( !mFilterRect.isNull() )
+  {
+    QgsFeatureIds featuresInRect = mSource->sharedData()->getFeatureIdsInExtent( mFilterRect );
+    if ( !requestIds.isEmpty() )
+    {
+      requestIds.intersect( featuresInRect );
+    }
+    else
+    {
+      requestIds = featuresInRect;
+    }
+    if ( requestIds.empty() )
+    {
+      close();
+      return;
+    }
+  }
+
+  mFeatureIdList = requestIds.toList();
+  std::sort( mFeatureIdList.begin(), mFeatureIdList.end() );
+  mRemainingFeatureIds = mFeatureIdList;
+  if ( !mRemainingFeatureIds.empty() )
+    mFeatureIterator = mRemainingFeatureIds.at( 0 );
 }
 
 QgsAfsFeatureIterator::~QgsAfsFeatureIterator()
@@ -87,6 +110,9 @@ bool QgsAfsFeatureIterator::fetchFeature( QgsFeature &f )
   if ( mFeatureIterator >= mSource->sharedData()->featureCount() )
     return false;
 
+  if ( !mFeatureIdList.empty() && mRemainingFeatureIds.empty() )
+    return false;
+
   switch ( mRequest.filterType() )
   {
     case QgsFeatureRequest::FilterFid:
@@ -97,33 +123,32 @@ bool QgsAfsFeatureIterator::fetchFeature( QgsFeature &f )
       bool result = mSource->sharedData()->getFeature( mRequest.filterFid(), f );
       geometryToDestinationCrs( f, mTransform );
       f.setValid( result );
-      mRemainingFeatureIds.remove( f.id() );
+      mRemainingFeatureIds.removeAll( f.id() );
       return result;
     }
 
     case QgsFeatureRequest::FilterFids:
     case QgsFeatureRequest::FilterExpression:
     case QgsFeatureRequest::FilterNone:
     {
-      QgsRectangle filterRect;
-      if ( !mRequest.filterRect().isNull() )
-      {
-        filterRect = mSource->sharedData()->extent();
-        filterRect = filterRect.intersect( &mFilterRect );
-      }
       while ( mFeatureIterator < mSource->sharedData()->featureCount() )
       {
-        bool success = mSource->sharedData()->getFeature( mFeatureIterator, f, filterRect );
-        ++mFeatureIterator;
-        if ( !success )
-          continue;
-        if ( mUsingFeatureIdList )
+        if ( !mFeatureIdList.empty() && mRemainingFeatureIds.empty() )
+          return false;
+
+        bool success = mSource->sharedData()->getFeature( mFeatureIterator, f );
+        if ( !mFeatureIdList.empty() )
+        {
+          mRemainingFeatureIds.removeAll( mFeatureIterator );
+          if ( !mRemainingFeatureIds.empty() )
+            mFeatureIterator = mRemainingFeatureIds.at( 0 );
+        }
+        else
         {
-          if ( !mRemainingFeatureIds.contains( f.id() ) )
-            continue;
-          else
-            mRemainingFeatureIds.remove( f.id() );
+          ++mFeatureIterator;
         }
+        if ( !success )
+          continue;
         geometryToDestinationCrs( f, mTransform );
         f.setValid( true );
         return true;
@@ -140,6 +165,8 @@ bool QgsAfsFeatureIterator::rewind()
     return false;
   mFeatureIterator = 0;
   mRemainingFeatureIds = mFeatureIdList;
+  if ( !mRemainingFeatureIds.empty() )
+    mFeatureIterator = mRemainingFeatureIds.at( 0 );
   return true;
 }
 

diff --git a/src/providers/arcgisrest/qgsafsfeatureiterator.h b/src/providers/arcgisrest/qgsafsfeatureiterator.h
@@ -51,9 +51,8 @@ class QgsAfsFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsAfs
   private:
     QgsFeatureId mFeatureIterator = 0;
 
-    bool mUsingFeatureIdList = false;
-    QgsFeatureIds mFeatureIdList;
-    QgsFeatureIds mRemainingFeatureIds;
+    QList< QgsFeatureId > mFeatureIdList;
+    QList< QgsFeatureId > mRemainingFeatureIds;
 
     QgsCoordinateTransform mTransform;
     QgsRectangle mFilterRect;

diff --git a/src/providers/arcgisrest/qgsafsshareddata.cpp b/src/providers/arcgisrest/qgsafsshareddata.cpp
@@ -140,3 +140,22 @@ bool QgsAfsSharedData::getFeature( QgsFeatureId id, QgsFeature &f, const QgsRect
 
   return false;
 }
+
+QgsFeatureIds QgsAfsSharedData::getFeatureIdsInExtent( const QgsRectangle &extent )
+{
+  QString errorTitle;
+  QString errorText;
+
+
+  const QList<quint32> featuresInRect = QgsArcGisRestUtils::getObjectIdsByExtent( mDataSource.param( QStringLiteral( "url" ) ),
+                                        extent, errorTitle, errorText );
+
+  QgsFeatureIds ids;
+  for ( quint32 id : featuresInRect )
+  {
+    int featureId = mObjectIds.indexOf( id );
+    if ( featureId >= 0 )
+      ids.insert( featureId );
+  }
+  return ids;
+}
diff --git a/src/providers/arcgisrest/qgsafsshareddata.h b/src/providers/arcgisrest/qgsafsshareddata.h
@@ -37,6 +37,7 @@ class QgsAfsSharedData : public QObject
     void clearCache();
 
     bool getFeature( QgsFeatureId id, QgsFeature &f, const QgsRectangle &filterRect = QgsRectangle() );
+    QgsFeatureIds getFeatureIdsInExtent( const QgsRectangle &extent );
 
   private:
     friend class QgsAfsProvider;

diff --git a/src/providers/arcgisrest/qgsarcgisrestutils.cpp b/src/providers/arcgisrest/qgsarcgisrestutils.cpp
@@ -407,6 +407,32 @@ QVariantMap QgsArcGisRestUtils::getObjects( const QString &layerurl, const QList
   return queryServiceJSON( queryUrl, errorTitle, errorText );
 }
 
+QList<quint32> QgsArcGisRestUtils::getObjectIdsByExtent( const QString &layerurl, const QgsRectangle &filterRect, QString &errorTitle, QString &errorText )
+{
+  QUrl queryUrl( layerurl + "/query" );
+  queryUrl.addQueryItem( QStringLiteral( "f" ), QStringLiteral( "json" ) );
+  queryUrl.addQueryItem( QStringLiteral( "where" ), QStringLiteral( "objectid=objectid" ) );
+  queryUrl.addQueryItem( QStringLiteral( "returnIdsOnly" ), QStringLiteral( "true" ) );
+  queryUrl.addQueryItem( QStringLiteral( "geometry" ), QStringLiteral( "%1,%2,%3,%4" )
+                         .arg( filterRect.xMinimum(), 0, 'f', -1 ).arg( filterRect.yMinimum(), 0, 'f', -1 )
+                         .arg( filterRect.xMaximum(), 0, 'f', -1 ).arg( filterRect.yMaximum(), 0, 'f', -1 ) );
+  queryUrl.addQueryItem( QStringLiteral( "geometryType" ), QStringLiteral( "esriGeometryEnvelope" ) );
+  queryUrl.addQueryItem( QStringLiteral( "spatialRel" ), QStringLiteral( "esriSpatialRelEnvelopeIntersects" ) );
+  const QVariantMap objectIdData = queryServiceJSON( queryUrl, errorTitle, errorText );
+
+  if ( objectIdData.isEmpty() )
+  {
+    return QList<quint32>();
+  }
+
+  QList<quint32> ids;
+  foreach ( const QVariant &objectId, objectIdData["objectIds"].toList() )
+  {
+    ids << objectId.toInt();
+  }
+  return ids;
+}
+
 QByteArray QgsArcGisRestUtils::queryService( const QUrl &u, QString &errorTitle, QString &errorText )
 {
   QEventLoop loop;

diff --git a/src/providers/arcgisrest/qgsarcgisrestutils.h b/src/providers/arcgisrest/qgsarcgisrestutils.h
@@ -18,6 +18,7 @@
 #include <QStringList>
 #include <QVariant>
 #include "geometry/qgswkbtypes.h"
+#include "qgsfeature.h"
 
 class QNetworkReply;
 class QgsNetworkAccessManager;
@@ -40,6 +41,7 @@ class QgsArcGisRestUtils
     static QVariantMap getObjects( const QString &layerurl, const QList<quint32> &objectIds, const QString &crs,
                                    bool fetchGeometry, const QStringList &fetchAttributes, bool fetchM, bool fetchZ,
                                    const QgsRectangle &filterRect, QString &errorTitle, QString &errorText );
+    static QList<quint32> getObjectIdsByExtent( const QString &layerurl, const QgsRectangle &filterRect, QString &errorTitle, QString &errorText );
     static QByteArray queryService( const QUrl &url, QString &errorTitle, QString &errorText );
     static QVariantMap queryServiceJSON( const QUrl &url, QString &errorTitle, QString &errorText );
 

diff --git a/tests/src/python/test_provider_afs.py b/tests/src/python/test_provider_afs.py
@@ -312,6 +312,39 @@ def setUpClass(cls):
  ]
 }""".encode('UTF-8'))
 
+        with open(sanitize(endpoint, '/query?f=json&where=objectid=objectid&returnIdsOnly=true&geometry=-70.000000,67.000000,-60.000000,80.000000&geometryType=esriGeometryEnvelope&spatialRel=esriSpatialRelEnvelopeIntersects'), 'wb') as f:
+            f.write("""
+        {
+         "objectIdFieldName": "OBJECTID",
+         "objectIds": [
+          2,
+          4
+         ]
+        }
+        """.encode('UTF-8'))
+
+        with open(sanitize(endpoint, '/query?f=json&where=objectid=objectid&returnIdsOnly=true&geometry=-73.000000,70.000000,-63.000000,80.000000&geometryType=esriGeometryEnvelope&spatialRel=esriSpatialRelEnvelopeIntersects'), 'wb') as f:
+            f.write("""
+        {
+         "objectIdFieldName": "OBJECTID",
+         "objectIds": [
+          2,
+          4
+         ]
+        }
+        """.encode('UTF-8'))
+
+        with open(sanitize(endpoint, '/query?f=json&where=objectid=objectid&returnIdsOnly=true&geometry=-68.721119,68.177676,-64.678700,79.123755&geometryType=esriGeometryEnvelope&spatialRel=esriSpatialRelEnvelopeIntersects'), 'wb') as f:
+            f.write("""
+        {
+         "objectIdFieldName": "OBJECTID",
+         "objectIds": [
+          2,
+          4
+         ]
+        }
+        """.encode('UTF-8'))
+
     @classmethod
     def tearDownClass(cls):
         """Run after all tests"""