Fix virtual fields which depend on other virtual fields may not be

calculated in some circumstances (fix #14939) (cherry-picked from df0d596)
qgis · Jul 1, 2016 · 91a1d33 · 91a1d33
1 parent 248ba38
commit 91a1d33
Show file tree

Hide file tree

Showing 8 changed files with 98 additions and 35 deletions.
diff --git a/python/core/qgsfeaturerequest.sip b/python/core/qgsfeaturerequest.sip
@@ -302,7 +302,7 @@ class QgsFeatureRequest
      * Return the subset of attributes which at least need to be fetched
      * @return A list of attributes to be fetched
      */
-    const QgsAttributeList& subsetOfAttributes() const;
+    QgsAttributeList subsetOfAttributes() const;
 
     //! Set a subset of attributes by names that will be fetched
     QgsFeatureRequest& setSubsetOfAttributes( const QStringList& attrNames, const QgsFields& fields );

diff --git a/src/core/qgsfeaturerequest.h b/src/core/qgsfeaturerequest.h
@@ -370,7 +370,7 @@ class CORE_EXPORT QgsFeatureRequest
      * Return the subset of attributes which at least need to be fetched
      * @return A list of attributes to be fetched
      */
-    const QgsAttributeList& subsetOfAttributes() const { return mAttrs; }
+    QgsAttributeList subsetOfAttributes() const { return mAttrs; }
 
     //! Set a subset of attributes by names that will be fetched
     QgsFeatureRequest& setSubsetOfAttributes( const QStringList& attrNames, const QgsFields& fields );

diff --git a/src/core/qgsvectorlayerfeatureiterator.cpp b/src/core/qgsvectorlayerfeatureiterator.cpp
@@ -538,6 +538,7 @@ void QgsVectorLayerFeatureIterator::prepareExpressions()
   mExpressionContext->appendScope( QgsExpressionContextUtils::projectScope() );
   mExpressionContext->setFields( mSource->mFields );
 
+  QList< int > virtualFieldsToFetch;
   for ( int i = 0; i < mSource->mFields.count(); i++ )
   {
     if ( mSource->mFields.fieldOrigin( i ) == QgsFields::OriginExpression )
@@ -546,38 +547,53 @@ void QgsVectorLayerFeatureIterator::prepareExpressions()
       if ( !( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
            || mRequest.subsetOfAttributes().contains( i ) )
       {
-        int oi = mSource->mFields.fieldOriginIndex( i );
-        QgsExpression* exp = new QgsExpression( exps[oi].cachedExpression );
-
-        QgsDistanceArea da;
-        da.setSourceCrs( mSource->mCrsId );
-        da.setEllipsoidalMode( true );
-        da.setEllipsoid( QgsProject::instance()->readEntry( "Measure", "/Ellipsoid", GEO_NONE ) );
-        exp->setGeomCalculator( da );
-        exp->setDistanceUnits( QgsProject::instance()->distanceUnits() );
-        exp->setAreaUnits( QgsProject::instance()->areaUnits() );
-
-        exp->prepare( mExpressionContext.data() );
-        mExpressionFieldInfo.insert( i, exp );
-
-        if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
-        {
-          QgsAttributeList attrs;
-          Q_FOREACH ( const QString& col, exp->referencedColumns() )
-          {
-            attrs.append( mSource->mFields.fieldNameIndex( col ) );
-          }
-
-          mRequest.setSubsetOfAttributes( mRequest.subsetOfAttributes() + attrs );
-        }
-
-        if ( exp->needsGeometry() )
-        {
-          mRequest.setFlags( mRequest.flags() & ~QgsFeatureRequest::NoGeometry );
-        }
+        virtualFieldsToFetch << i;
       }
     }
   }
+
+  QList< int > virtualFieldsProcessed;
+  while ( !virtualFieldsToFetch.isEmpty() )
+  {
+    int fieldIdx = virtualFieldsToFetch.takeFirst();
+    if ( virtualFieldsProcessed.contains( fieldIdx ) )
+      continue;
+
+    virtualFieldsProcessed << fieldIdx;
+
+
+    int oi = mSource->mFields.fieldOriginIndex( fieldIdx );
+    QgsExpression* exp = new QgsExpression( exps[oi].cachedExpression );
+
+    QgsDistanceArea da;
+    da.setSourceCrs( mSource->mCrsId );
+    da.setEllipsoidalMode( true );
+    da.setEllipsoid( QgsProject::instance()->readEntry( "Measure", "/Ellipsoid", GEO_NONE ) );
+    exp->setGeomCalculator( da );
+    exp->setDistanceUnits( QgsProject::instance()->distanceUnits() );
+    exp->setAreaUnits( QgsProject::instance()->areaUnits() );
+
+    exp->prepare( mExpressionContext.data() );
+    mExpressionFieldInfo.insert( fieldIdx, exp );
+
+    Q_FOREACH ( const QString& col, exp->referencedColumns() )
+    {
+      int dependantFieldIdx = mSource->mFields.fieldNameIndex( col );
+      if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
+      {
+        mRequest.setSubsetOfAttributes( mRequest.subsetOfAttributes() << dependantFieldIdx );
+      }
+      // also need to fetch this dependant field
+      if ( mSource->mFields.fieldOrigin( dependantFieldIdx ) == QgsFields::OriginExpression )
+        virtualFieldsToFetch << dependantFieldIdx;
+    }
+
+    if ( exp->needsGeometry() )
+    {
+      mRequest.setFlags( mRequest.flags() & ~QgsFeatureRequest::NoGeometry );
+    }
+  }
+
 }
 
 void QgsVectorLayerFeatureIterator::addJoinedAttributes( QgsFeature &f )

diff --git a/src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp b/src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp
@@ -341,7 +341,7 @@ bool QgsDelimitedTextFeatureIterator::nextFeatureInternal( QgsFeature& feature )
 
     if ( ! mTestSubset && ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes ) )
     {
-      const QgsAttributeList& attrs = mRequest.subsetOfAttributes();
+      QgsAttributeList attrs = mRequest.subsetOfAttributes();
       for ( QgsAttributeList::const_iterator i = attrs.begin(); i != attrs.end(); ++i )
       {
         int fieldIdx = *i;

diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -404,7 +404,7 @@ bool QgsOgrFeatureIterator::readFeature( OGRFeatureH fet, QgsFeature& feature )
   // fetch attributes
   if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
   {
-    const QgsAttributeList& attrs = mRequest.subsetOfAttributes();
+    QgsAttributeList attrs = mRequest.subsetOfAttributes();
     for ( QgsAttributeList::const_iterator it = attrs.begin(); it != attrs.end(); ++it )
     {
       getFeatureAttribute( fet, feature, *it );

diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp
@@ -685,7 +685,7 @@ bool QgsPostgresFeatureIterator::getFeature( QgsPostgresResult &queryResult, int
   QgsFeatureId fid = 0;
 
   bool subsetOfAttributes = mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes;
-  const QgsAttributeList& fetchAttributes = mRequest.subsetOfAttributes();
+  QgsAttributeList fetchAttributes = mRequest.subsetOfAttributes();
 
   switch ( mSource->mPrimaryKeyType )
   {

diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp
@@ -291,7 +291,7 @@ bool QgsSpatiaLiteFeatureIterator::prepareStatement( const QString& whereClause,
 
     if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
     {
-      const QgsAttributeList& fetchAttributes = mRequest.subsetOfAttributes();
+      QgsAttributeList fetchAttributes = mRequest.subsetOfAttributes();
       for ( QgsAttributeList::const_iterator it = fetchAttributes.constBegin(); it != fetchAttributes.constEnd(); ++it )
       {
         sql += ',' + fieldName( mSource->mFields.field( *it ) );

diff --git a/tests/src/python/test_qgsfeatureiterator.py b/tests/src/python/test_qgsfeatureiterator.py
@@ -111,5 +111,52 @@ def addFeatures(self, vl):
         feat['Staff'] = 2
         vl.addFeature(feat)
 
+    def test_ExpressionFieldNested(self):
+        myShpFile = os.path.join(TEST_DATA_DIR, 'points.shp')
+        layer = QgsVectorLayer(myShpFile, 'Points', 'ogr')
+        self.assertTrue(layer.isValid())
+
+        cnt = layer.pendingFields().count()
+        idx = layer.addExpressionField('"Staff"*2', QgsField('exp1', QVariant.LongLong))
+        idx = layer.addExpressionField('"exp1"-1', QgsField('exp2', QVariant.LongLong))
+
+        fet = next(layer.getFeatures(QgsFeatureRequest().setSubsetOfAttributes(['exp2'], layer.fields())))
+        self.assertEqual(fet['Class'], NULL)
+        # nested virtual fields should make all these attributes be fetched
+        self.assertEqual(fet['Staff'], 2)
+        self.assertEqual(fet['exp2'], 3)
+        self.assertEqual(fet['exp1'], 4)
+
+    def test_ExpressionFieldNestedGeometry(self):
+        myShpFile = os.path.join(TEST_DATA_DIR, 'points.shp')
+        layer = QgsVectorLayer(myShpFile, 'Points', 'ogr')
+        self.assertTrue(layer.isValid())
+
+        cnt = layer.pendingFields().count()
+        idx = layer.addExpressionField('$x*2', QgsField('exp1', QVariant.LongLong))
+        idx = layer.addExpressionField('"exp1"/1.5', QgsField('exp2', QVariant.LongLong))
+
+        fet = next(layer.getFeatures(QgsFeatureRequest().setFlags(QgsFeatureRequest.NoGeometry).setSubsetOfAttributes(['exp2'], layer.fields())))
+        # nested virtual fields should have made geometry be fetched
+        self.assertEqual(fet['exp2'], -156)
+        self.assertEqual(fet['exp1'], -234)
+
+    def test_ExpressionFieldNestedCircular(self):
+        """ test circular virtual field definitions """
+
+        myShpFile = os.path.join(TEST_DATA_DIR, 'points.shp')
+        layer = QgsVectorLayer(myShpFile, 'Points', 'ogr')
+        self.assertTrue(layer.isValid())
+
+        cnt = layer.pendingFields().count()
+        idx = layer.addExpressionField('"exp3"*2', QgsField('exp1', QVariant.LongLong))
+        idx = layer.addExpressionField('"exp1"-1', QgsField('exp2', QVariant.LongLong))
+        idx = layer.addExpressionField('"exp2"*3', QgsField('exp3', QVariant.LongLong))
+
+        # really just testing that this doesn't hang/crash... there's no good result here!
+        fet = next(layer.getFeatures(QgsFeatureRequest().setSubsetOfAttributes(['exp2'], layer.fields())))
+        self.assertEqual(fet['Class'], NULL)
+
+
 if __name__ == '__main__':
     unittest.main()