Skip to content

Commit df0d596

Browse files
nyalldawsonnyalldawson
committed
Fix virtual fields which depend on other virtual fields may not be
calculated in some circumstances (fix #14939)
1 parent 1bc17e6 commit df0d596

8 files changed

+101
-36
lines changed

‎python/core/qgsfeaturerequest.sip

+1-1
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ class QgsFeatureRequest
302302
* Return the subset of attributes which at least need to be fetched
303303
* @return A list of attributes to be fetched
304304
*/
305-
const QgsAttributeList& subsetOfAttributes() const;
305+
QgsAttributeList subsetOfAttributes() const;
306306

307307
//! Set a subset of attributes by names that will be fetched
308308
QgsFeatureRequest& setSubsetOfAttributes( const QStringList& attrNames, const QgsFields& fields );

‎src/core/qgsfeaturerequest.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ class CORE_EXPORT QgsFeatureRequest
370370
* Return the subset of attributes which at least need to be fetched
371371
* @return A list of attributes to be fetched
372372
*/
373-
const QgsAttributeList& subsetOfAttributes() const { return mAttrs; }
373+
QgsAttributeList subsetOfAttributes() const { return mAttrs; }
374374

375375
//! Set a subset of attributes by names that will be fetched
376376
QgsFeatureRequest& setSubsetOfAttributes( const QStringList& attrNames, const QgsFields& fields );

‎src/core/qgsvectorlayerfeatureiterator.cpp

+45-29
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,7 @@ void QgsVectorLayerFeatureIterator::prepareExpressions()
527527
mExpressionContext->appendScope( QgsExpressionContextUtils::projectScope() );
528528
mExpressionContext->setFields( mSource->mFields );
529529

530+
QList< int > virtualFieldsToFetch;
530531
for ( int i = 0; i < mSource->mFields.count(); i++ )
531532
{
532533
if ( mSource->mFields.fieldOrigin( i ) == QgsFields::OriginExpression )
@@ -535,38 +536,53 @@ void QgsVectorLayerFeatureIterator::prepareExpressions()
535536
if ( !( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
536537
|| mRequest.subsetOfAttributes().contains( i ) )
537538
{
538-
int oi = mSource->mFields.fieldOriginIndex( i );
539-
QgsExpression* exp = new QgsExpression( exps[oi].cachedExpression );
540-
541-
QgsDistanceArea da;
542-
da.setSourceCrs( mSource->mCrsId );
543-
da.setEllipsoidalMode( true );
544-
da.setEllipsoid( QgsProject::instance()->readEntry( "Measure", "/Ellipsoid", GEO_NONE ) );
545-
exp->setGeomCalculator( da );
546-
exp->setDistanceUnits( QgsProject::instance()->distanceUnits() );
547-
exp->setAreaUnits( QgsProject::instance()->areaUnits() );
548-
549-
exp->prepare( mExpressionContext.data() );
550-
mExpressionFieldInfo.insert( i, exp );
551-
552-
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
553-
{
554-
QgsAttributeList attrs;
555-
Q_FOREACH ( const QString& col, exp->referencedColumns() )
556-
{
557-
attrs.append( mSource->mFields.fieldNameIndex( col ) );
558-
}
559-
560-
mRequest.setSubsetOfAttributes( mRequest.subsetOfAttributes() + attrs );
561-
}
562-
563-
if ( exp->needsGeometry() )
564-
{
565-
mRequest.setFlags( mRequest.flags() & ~QgsFeatureRequest::NoGeometry );
566-
}
539+
virtualFieldsToFetch << i;
567540
}
568541
}
569542
}
543+
544+
QList< int > virtualFieldsProcessed;
545+
while ( !virtualFieldsToFetch.isEmpty() )
546+
{
547+
int fieldIdx = virtualFieldsToFetch.takeFirst();
548+
if ( virtualFieldsProcessed.contains( fieldIdx ) )
549+
continue;
550+
551+
virtualFieldsProcessed << fieldIdx;
552+
553+
554+
int oi = mSource->mFields.fieldOriginIndex( fieldIdx );
555+
QgsExpression* exp = new QgsExpression( exps[oi].cachedExpression );
556+
557+
QgsDistanceArea da;
558+
da.setSourceCrs( mSource->mCrsId );
559+
da.setEllipsoidalMode( true );
560+
da.setEllipsoid( QgsProject::instance()->readEntry( "Measure", "/Ellipsoid", GEO_NONE ) );
561+
exp->setGeomCalculator( da );
562+
exp->setDistanceUnits( QgsProject::instance()->distanceUnits() );
563+
exp->setAreaUnits( QgsProject::instance()->areaUnits() );
564+
565+
exp->prepare( mExpressionContext.data() );
566+
mExpressionFieldInfo.insert( fieldIdx, exp );
567+
568+
Q_FOREACH ( const QString& col, exp->referencedColumns() )
569+
{
570+
int dependantFieldIdx = mSource->mFields.fieldNameIndex( col );
571+
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
572+
{
573+
mRequest.setSubsetOfAttributes( mRequest.subsetOfAttributes() << dependantFieldIdx );
574+
}
575+
// also need to fetch this dependant field
576+
if ( mSource->mFields.fieldOrigin( dependantFieldIdx ) == QgsFields::OriginExpression )
577+
virtualFieldsToFetch << dependantFieldIdx;
578+
}
579+
580+
if ( exp->needsGeometry() )
581+
{
582+
mRequest.setFlags( mRequest.flags() & ~QgsFeatureRequest::NoGeometry );
583+
}
584+
}
585+
570586
}
571587

572588
void QgsVectorLayerFeatureIterator::addJoinedAttributes( QgsFeature &f )

‎src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,7 @@ bool QgsDelimitedTextFeatureIterator::nextFeatureInternal( QgsFeature& feature )
342342

343343
if ( ! mTestSubset && ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes ) )
344344
{
345-
const QgsAttributeList& attrs = mRequest.subsetOfAttributes();
345+
QgsAttributeList attrs = mRequest.subsetOfAttributes();
346346
for ( QgsAttributeList::const_iterator i = attrs.begin(); i != attrs.end(); ++i )
347347
{
348348
int fieldIdx = *i;

‎src/providers/ogr/qgsogrfeatureiterator.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ bool QgsOgrFeatureIterator::readFeature( OGRFeatureH fet, QgsFeature& feature )
299299
// fetch attributes
300300
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
301301
{
302-
const QgsAttributeList& attrs = mRequest.subsetOfAttributes();
302+
QgsAttributeList attrs = mRequest.subsetOfAttributes();
303303
for ( QgsAttributeList::const_iterator it = attrs.begin(); it != attrs.end(); ++it )
304304
{
305305
getFeatureAttribute( fet, feature, *it );

‎src/providers/postgres/qgspostgresfeatureiterator.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,7 @@ bool QgsPostgresFeatureIterator::getFeature( QgsPostgresResult &queryResult, int
706706
QgsFeatureId fid = 0;
707707

708708
bool subsetOfAttributes = mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes;
709-
const QgsAttributeList& fetchAttributes = mRequest.subsetOfAttributes();
709+
QgsAttributeList fetchAttributes = mRequest.subsetOfAttributes();
710710

711711
switch ( mSource->mPrimaryKeyType )
712712
{

‎src/providers/spatialite/qgsspatialitefeatureiterator.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ bool QgsSpatiaLiteFeatureIterator::prepareStatement( const QString& whereClause,
292292

293293
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
294294
{
295-
const QgsAttributeList& fetchAttributes = mRequest.subsetOfAttributes();
295+
QgsAttributeList fetchAttributes = mRequest.subsetOfAttributes();
296296
for ( QgsAttributeList::const_iterator it = fetchAttributes.constBegin(); it != fetchAttributes.constEnd(); ++it )
297297
{
298298
sql += ',' + fieldName( mSource->mFields.field( *it ) );

‎tests/src/python/test_qgsfeatureiterator.py

+50-1
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,10 @@
1616

1717
import os
1818

19-
from qgis.core import QgsVectorLayer, QgsFeatureRequest, QgsFeature
19+
from qgis.core import QgsVectorLayer, QgsFeatureRequest, QgsFeature, QgsField, NULL
2020
from qgis.testing import start_app, unittest
21+
from qgis.PyQt.QtCore import QVariant
22+
2123
from utilities import unitTestDataPath
2224
start_app()
2325
TEST_DATA_DIR = unitTestDataPath()
@@ -108,5 +110,52 @@ def addFeatures(self, vl):
108110
feat['Staff'] = 2
109111
vl.addFeature(feat)
110112

113+
def test_ExpressionFieldNested(self):
114+
myShpFile = os.path.join(TEST_DATA_DIR, 'points.shp')
115+
layer = QgsVectorLayer(myShpFile, 'Points', 'ogr')
116+
self.assertTrue(layer.isValid())
117+
118+
cnt = layer.pendingFields().count()
119+
idx = layer.addExpressionField('"Staff"*2', QgsField('exp1', QVariant.LongLong))
120+
idx = layer.addExpressionField('"exp1"-1', QgsField('exp2', QVariant.LongLong))
121+
122+
fet = next(layer.getFeatures(QgsFeatureRequest().setSubsetOfAttributes(['exp2'], layer.fields())))
123+
self.assertEqual(fet['Class'], NULL)
124+
# nested virtual fields should make all these attributes be fetched
125+
self.assertEqual(fet['Staff'], 2)
126+
self.assertEqual(fet['exp2'], 3)
127+
self.assertEqual(fet['exp1'], 4)
128+
129+
def test_ExpressionFieldNestedGeometry(self):
130+
myShpFile = os.path.join(TEST_DATA_DIR, 'points.shp')
131+
layer = QgsVectorLayer(myShpFile, 'Points', 'ogr')
132+
self.assertTrue(layer.isValid())
133+
134+
cnt = layer.pendingFields().count()
135+
idx = layer.addExpressionField('$x*2', QgsField('exp1', QVariant.LongLong))
136+
idx = layer.addExpressionField('"exp1"/1.5', QgsField('exp2', QVariant.LongLong))
137+
138+
fet = next(layer.getFeatures(QgsFeatureRequest().setFlags(QgsFeatureRequest.NoGeometry).setSubsetOfAttributes(['exp2'], layer.fields())))
139+
# nested virtual fields should have made geometry be fetched
140+
self.assertEqual(fet['exp2'], -156)
141+
self.assertEqual(fet['exp1'], -234)
142+
143+
def test_ExpressionFieldNestedCircular(self):
144+
""" test circular virtual field definitions """
145+
146+
myShpFile = os.path.join(TEST_DATA_DIR, 'points.shp')
147+
layer = QgsVectorLayer(myShpFile, 'Points', 'ogr')
148+
self.assertTrue(layer.isValid())
149+
150+
cnt = layer.pendingFields().count()
151+
idx = layer.addExpressionField('"exp3"*2', QgsField('exp1', QVariant.LongLong))
152+
idx = layer.addExpressionField('"exp1"-1', QgsField('exp2', QVariant.LongLong))
153+
idx = layer.addExpressionField('"exp2"*3', QgsField('exp3', QVariant.LongLong))
154+
155+
# really just testing that this doesn't hang/crash... there's no good result here!
156+
fet = next(layer.getFeatures(QgsFeatureRequest().setSubsetOfAttributes(['exp2'], layer.fields())))
157+
self.assertEqual(fet['Class'], NULL)
158+
159+
111160
if __name__ == '__main__':
112161
unittest.main()

0 commit comments

Comments
 (0)