From 9e9c8122482a664e4d2a6994df0c485c52101659 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 20 Jan 2020 18:25:26 +0100 Subject: [PATCH] QgsExpression::referencedAttributeIndexes(): only report valid indices If the expression was referencing a non-existing field, -1 was returned in the result set, which caused later crashed in various providers, including the Spatialite, Postgres, etc..., due to tried to dereference mFields.at(-1) Discarding invalid indices is what is also done in QgsFeatureRequest::OrderBy::usedAttributeIndices() Fixes #33878 --- src/core/expression/qgsexpression.cpp | 6 +++++- tests/src/python/featuresourcetestbase.py | 7 +++++++ tests/src/python/test_qgsexpression.py | 8 +++++++- tests/testdata/provider/spatialite.db | Bin 5012480 -> 5014528 bytes 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/core/expression/qgsexpression.cpp b/src/core/expression/qgsexpression.cpp index f0b204c153e8..3396b3c8d678 100644 --- a/src/core/expression/qgsexpression.cpp +++ b/src/core/expression/qgsexpression.cpp @@ -250,7 +250,11 @@ QSet QgsExpression::referencedAttributeIndexes( const QgsFields &fields ) c referencedIndexes = fields.allAttributesList().toSet(); break; } - referencedIndexes << fields.lookupField( fieldName ); + const int idx = fields.lookupField( fieldName ); + if ( idx >= 0 ) + { + referencedIndexes << idx; + } } return referencedIndexes; diff --git a/tests/src/python/featuresourcetestbase.py b/tests/src/python/featuresourcetestbase.py index c56a8f896193..2ef866ad1190 100644 --- a/tests/src/python/featuresourcetestbase.py +++ b/tests/src/python/featuresourcetestbase.py @@ -715,3 +715,10 @@ def testMaximumValue(self): def testAllFeatureIds(self): ids = set([f.id() for f in self.source.getFeatures()]) self.assertEqual(set(self.source.allFeatureIds()), ids) + + def testSubsetOfAttributesWithFilterExprWithNonExistingColumn(self): + """ Test fix for https://github.com/qgis/QGIS/issues/33878 """ + request = QgsFeatureRequest().setSubsetOfAttributes([0]) + request.setFilterExpression("non_existing = 1") + features = [f for f in self.source.getFeatures(request)] + self.assertEqual(len(features), 0) diff --git a/tests/src/python/test_qgsexpression.py b/tests/src/python/test_qgsexpression.py index 0f679bfe7dd8..8fe42aa74419 100644 --- a/tests/src/python/test_qgsexpression.py +++ b/tests/src/python/test_qgsexpression.py @@ -15,7 +15,7 @@ from qgis.PyQt.QtCore import QVariant from qgis.testing import unittest from qgis.utils import qgsfunction -from qgis.core import QgsExpression, QgsFeatureRequest, QgsExpressionContext, NULL +from qgis.core import QgsExpression, QgsFeatureRequest, QgsFields, QgsExpressionContext, NULL class TestQgsExpressionCustomFunctions(unittest.TestCase): @@ -264,6 +264,12 @@ def testCreateFieldEqualityExpression(self): res = '"my\'field" = TRUE' self.assertEqual(e.createFieldEqualityExpression(field, value), res) + def testReferencedAttributeIndexesNonExistingField(self): + e = QgsExpression() + e.setExpression("foo = 1") + self.assertTrue(e.isValid()) + self.assertEqual(len(e.referencedAttributeIndexes(QgsFields())), 0) + if __name__ == "__main__": unittest.main() diff --git a/tests/testdata/provider/spatialite.db b/tests/testdata/provider/spatialite.db index e37acc2da191f034f8c592183776ee9d8099528c..33a3f86b9f36e8977a06970b194ec2ecbfcd5fd9 100644 GIT binary patch delta 1459 zcmb8tKTH#G6bJC%-Mdn*fKZAc6zG9sL1>Qx{sI3{N~=JrRcNBo1WS8jlX6F{#egv> zjf0wKB4L0SMh7Pn9At99!No-f2RBC-2XSHa+w#v!N$`@-@Vnpd_wwHRy}8X*e(vd8 z{-xEt!I-6i?R}f`ZJpf4>fP6+#R9D``az#*omSS&GuAm;l$yDEi&-E$7Y!%FWARu- zQ4>l+e7tUXvTosk2Lb^MPyj|K1QAS71jSGSN1znSzzpS(iB$ndp%SX#7*sK?5|xaX0}@a1!j$3@va9TA>Xb-~<`k!37=A30-g+&cInX2i?#E=ivfegiCN4 zdch5S;DLVdf)B31RTzLl7+PDm_mhHONu1W>*{znI^k6~Nu$^C+|5DYM6K~9wv&_znxR%iUFT~msjE{+1V)To? z(tDbsX8t=r&-ZaZxur}|kGRx`Xke}BB9BMvyX6>hI|3f5%h`TEnVd@W$Z|wi!pYbJ zC8AAEYHGr%=@atKY{}!Z4mILPgk@Zi@@o=?~dboiY1Cp1I` zX3acpyX4B>eN9bmz3J5SIG#1CO@?Et{7}&~88>pfU&#y1#e)5kLkeb3Oa0m7HJre@ zSy0!e^q7(i>$5lz&{WB%D@qANB`l;ym3uK&iJCIwNI^z#=oLMq8H$n^q<8dzzR~Z@ zJa$paLap4+3vUmj`0qb7rLc^ecF$$nZOr+B69pw?+d7fiHtemiClrwtd)dBFCAqlW XvRtvqc3pfmb&L7SDkfXuZc%>#LBPwu delta 392 zcmYMwIab0@0EOX~_naVN3~>%h92nF%#(4q-1%)M8xBx4vlsU&jO0C?08^9*@t8Lr> zJv+eCpUQjA*WTif+FvsI?Z`LR?h4^##sB*6eqeMvPoGv&DJ@h_{U{yVp&O|WuV@4> z+IiKE0SSd63}XbN7()U{m>5S26R167NQDgXcg