Safer approach to handling reprojection with distance within requests

We can't safely handle a distance within query when transforming, as we cannot transform the static within tolerance distance from one CRS to a static distance in a different CRS
qgis · Jan 31, 2022 · 8e04498 · 8e04498
1 parent 3004fde
commit 8e04498
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 22 deletions.
diff --git a/python/core/auto_generated/qgsfeatureiterator.sip.in b/python/core/auto_generated/qgsfeatureiterator.sip.in
@@ -75,6 +75,12 @@ This indicates that there is something wrong with the expression compiler.
 .. versionadded:: 3.2
 %End
 
+    enum class RequestToSourceCrsResult
+    {
+      Success,
+      DistanceWithinMustBeCheckedManually,
+    };
+
   protected:
 
     virtual bool fetchFeature( QgsFeature &f ) = 0;
@@ -138,13 +144,17 @@ Will throw a :py:class:`QgsCsException` if the rect cannot be transformed from t
 .. versionadded:: 3.0
 %End
 
-    void updateRequestToSourceCrs( QgsFeatureRequest &request, const QgsCoordinateTransform &transform ) const throw( QgsCsException );
+    RequestToSourceCrsResult updateRequestToSourceCrs( QgsFeatureRequest &request, const QgsCoordinateTransform &transform ) const throw( QgsCsException );
 %Docstring
 Update a :py:class:`QgsFeatureRequest` so that spatial filters are
 transformed to the source's coordinate reference system.
 Iterators should call this method against the request used for filtering
 features to ensure that any :py:func:`QgsFeatureRequest.destinationCrs()` set on the request is respected.
-Will throw a :py:class:`QgsCsException` if the rect cannot be transformed from the destination CRS.
+
+:return: result of operation. See QgsAbstractFeatureIterator.RequestToSourceCrsResult for interpretation.
+
+:raises QgsCsException: if the rect cannot be transformed from the destination CRS.
+
 
 .. versionadded:: 3.22
 %End

diff --git a/src/core/qgsfeatureiterator.cpp b/src/core/qgsfeatureiterator.cpp
@@ -121,35 +121,37 @@ void QgsAbstractFeatureIterator::geometryToDestinationCrs( QgsFeature &feature,
   }
 }
 
-void QgsAbstractFeatureIterator::updateRequestToSourceCrs( QgsFeatureRequest &request, const QgsCoordinateTransform &transform ) const
+QgsAbstractFeatureIterator::RequestToSourceCrsResult QgsAbstractFeatureIterator::updateRequestToSourceCrs( QgsFeatureRequest &request, const QgsCoordinateTransform &transform ) const
 {
+  if ( transform.isShortCircuited() )
+    return RequestToSourceCrsResult::Success; // nothing to do
+
   switch ( request.spatialFilterType() )
   {
     case Qgis::SpatialFilterType::NoFilter:
-      break;
+      return RequestToSourceCrsResult::Success;
+
     case Qgis::SpatialFilterType::BoundingBox:
     {
       QgsRectangle newRect = transform.transformBoundingBox( request.filterRect(), Qgis::TransformDirection::Reverse );
       request.setFilterRect( newRect );
-      break;
+      return RequestToSourceCrsResult::Success;
     }
     case Qgis::SpatialFilterType::DistanceWithin:
     {
-      QgsGeometry geom = request.referenceGeometry();
-      QgsRectangle bbox = geom.boundingBox();
-      double x0 = bbox.xMaximum();
-      double y0 = bbox.yMaximum();
-      double dist = request.distanceWithin();
-      QgsPoint p0( x0, y0 );
-      QgsPoint p1( x0 + dist, y0 );
-      QgsLineString newDistLine( p0, p1 );
-      newDistLine.transform( transform, Qgis::TransformDirection::Reverse );
-      dist = newDistLine.length();
-      geom.transform( transform, Qgis::TransformDirection::Reverse );
-      request.setDistanceWithin( geom, dist );
-      break;
+      // we can't safely handle a distance within query, as we cannot transform the
+      // static within tolerance distance from one CRS to a static distance in a different CRS.
+
+      // in this case we transform the request's distance within requirement to a "worst case" bounding box filter, so
+      // that the request itself can still take advantage of spatial indices even when we have to do the distance within check locally
+      QgsRectangle newRect = transform.transformBoundingBox( request.filterRect(), Qgis::TransformDirection::Reverse );
+      request.setFilterRect( newRect );
+
+      return RequestToSourceCrsResult::DistanceWithinMustBeCheckedManually;
     }
   }
+
+  BUILTIN_UNREACHABLE
 }
 
 QgsRectangle QgsAbstractFeatureIterator::filterRectToSourceCrs( const QgsCoordinateTransform &transform ) const

diff --git a/src/core/qgsfeatureiterator.h b/src/core/qgsfeatureiterator.h
@@ -91,6 +91,17 @@ class CORE_EXPORT QgsAbstractFeatureIterator
      */
     bool compileFailed() const;
 
+    /**
+     * Possible results from the updateRequestToSourceCrs() method.
+     *
+     * \since QGIS 3.22
+     */
+    enum class RequestToSourceCrsResult : int
+    {
+      Success, //!< Request was successfully updated to the source CRS, or no changes were required
+      DistanceWithinMustBeCheckedManually, //!< The distance within request cannot be losslessly updated to the source CRS, and callers will need to take appropriate steps to handle the distance within requirement manually during feature iteration
+    };
+
   protected:
 
     /**
@@ -154,10 +165,14 @@ class CORE_EXPORT QgsAbstractFeatureIterator
      * transformed to the source's coordinate reference system.
      * Iterators should call this method against the request used for filtering
      * features to ensure that any QgsFeatureRequest::destinationCrs() set on the request is respected.
-     * Will throw a QgsCsException if the rect cannot be transformed from the destination CRS.
+     *
+     * \returns result of operation. See QgsAbstractFeatureIterator::RequestToSourceCrsResult for interpretation.
+     *
+     * \throws QgsCsException if the rect cannot be transformed from the destination CRS.
+     *
      * \since QGIS 3.22
      */
-    void updateRequestToSourceCrs( QgsFeatureRequest &request, const QgsCoordinateTransform &transform ) const SIP_THROW( QgsCsException );
+    RequestToSourceCrsResult updateRequestToSourceCrs( QgsFeatureRequest &request, const QgsCoordinateTransform &transform ) const SIP_THROW( QgsCsException );
 
     //! A copy of the feature request.
     QgsFeatureRequest mRequest;

diff --git a/src/core/vector/qgsvectorlayerfeatureiterator.cpp b/src/core/vector/qgsvectorlayerfeatureiterator.cpp
@@ -130,6 +130,9 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
   }
 
   // prepare spatial filter geometries for optimal speed
+  // since the mDistanceWithin* constraint member variables are all in the DESTINATION CRS,
+  // we set all these upfront before any transformation to the source CRS is done.
+
   switch ( mRequest.spatialFilterType() )
   {
     case Qgis::SpatialFilterType::NoFilter:
@@ -139,6 +142,10 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
     case Qgis::SpatialFilterType::DistanceWithin:
       if ( !mRequest.referenceGeometry().isEmpty() )
       {
+        // Note that regardless of whether or not we'll ultimately be able to handoff this check to the underlying provider,
+        // we still need these reference geometry constraints in the vector layer iterator as we need them to check against
+        // the features from the vector layer's edit buffer! (In other words, we cannot completely hand off responsibility for
+        // these checks to the provider and ignore them locally)
         mDistanceWithinGeom = mRequest.referenceGeometry();
         mDistanceWithinEngine = mRequest.referenceGeometryEngine();
         mDistanceWithinEngine->prepareGeometry();
@@ -147,9 +154,22 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
       break;
   }
 
+  bool canDelegateLimitToProvider = true;
   try
   {
-    updateRequestToSourceCrs( mRequest, mTransform );
+    switch ( updateRequestToSourceCrs( mRequest, mTransform ) )
+    {
+      case QgsAbstractFeatureIterator::RequestToSourceCrsResult::Success:
+        break;
+
+      case QgsAbstractFeatureIterator::RequestToSourceCrsResult::DistanceWithinMustBeCheckedManually:
+        // we have to disable any limit on the provider's request -- since that request may be returning features which are outside the
+        // distance tolerance, we'll have to fetch them all and then handle the limit check manually only after testing for the distance within constraint
+        canDelegateLimitToProvider = false;
+        break;
+    }
+
+    // mFilterRect is in the source CRS, so we set that now (after request transformation has been done)
     mFilterRect = mRequest.filterRect();
   }
   catch ( QgsCsException & )
@@ -159,7 +179,6 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
     return;
   }
 
-
   // check whether the order by clause(s) can be delegated to the provider
   mDelegatedOrderByToProvider = !mSource->mHasEditBuffer;
   if ( !mRequest.orderBy().isEmpty() )
@@ -214,6 +233,11 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
     mProviderRequest.setOrderBy( QgsFeatureRequest::OrderBy() );
   }
 
+  if ( !canDelegateLimitToProvider )
+  {
+    mProviderRequest.setLimit( -1 );
+  }
+
   if ( mProviderRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
   {
     // prepare list of attributes to match provider fields

diff --git a/src/core/vector/qgsvectorlayerfeatureiterator.h b/src/core/vector/qgsvectorlayerfeatureiterator.h
@@ -268,9 +268,11 @@ class CORE_EXPORT QgsVectorLayerFeatureIterator : public QgsAbstractFeatureItera
     QgsFeatureRequest mChangedFeaturesRequest;
     QgsFeatureIterator mChangedFeaturesIterator;
 
+    // filter bounding box constraint, in SOURCE CRS
     QgsRectangle mFilterRect;
     QgsCoordinateTransform mTransform;
 
+    // distance within constraint reference geometry and distance IN DESTINATION CRS
     QgsGeometry mDistanceWithinGeom;
     std::shared_ptr< QgsGeometryEngine > mDistanceWithinEngine;
     double mDistanceWithin = 0;