Skip to content

Commit

Permalink
Fix virtual fields which depend on other virtual fields may not be
Browse files Browse the repository at this point in the history
calculated in some circumstances (fix #14939)

(cherry-picked from df0d596)
  • Loading branch information
nyalldawson committed Jul 1, 2016
1 parent 248ba38 commit 91a1d33
Show file tree
Hide file tree
Showing 8 changed files with 98 additions and 35 deletions.
2 changes: 1 addition & 1 deletion python/core/qgsfeaturerequest.sip
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
2 changes: 1 addition & 1 deletion src/core/qgsfeaturerequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
74 changes: 45 additions & 29 deletions src/core/qgsvectorlayerfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Expand All @@ -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 )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/providers/ogr/qgsogrfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
2 changes: 1 addition & 1 deletion src/providers/postgres/qgspostgresfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
{
Expand Down
2 changes: 1 addition & 1 deletion src/providers/spatialite/qgsspatialitefeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
Expand Down
47 changes: 47 additions & 0 deletions tests/src/python/test_qgsfeatureiterator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()

0 comments on commit 91a1d33

Please sign in to comment.