Skip to content

Commit 92a1808

Browse files
nyalldawsonnyalldawson
committed
Add safeguard tests to ensure no regressions in expression compilation
(ie check that expressions are successfully compiled where expected) Add compilation support for "NOT..." type expressions
1 parent 99210ec commit 92a1808

13 files changed

+147
-2
lines changed

‎python/core/qgsfeatureiterator.sip

+21
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,15 @@ class QgsAbstractFeatureIterator
44
#include <qgsfeatureiterator.h>
55
%End
66
public:
7+
8+
//! Status of expression compilation for filter expression requests
9+
enum CompileStatus
10+
{
11+
NoCompilation, /*!< Expression could not be compiled or not attempt was made to compile expression */
12+
PartiallyCompiled, /*!< Expression was partially compiled, but extra checks need to be applied to features*/
13+
Compiled, /*!< Expression was fully compiled and delegated to data provider source*/
14+
};
15+
716
//! base class constructor - stores the iteration parameters
817
QgsAbstractFeatureIterator( const QgsFeatureRequest& request );
918

@@ -18,6 +27,11 @@ class QgsAbstractFeatureIterator
1827
//! end of iterating: free the resources / lock
1928
virtual bool close() = 0;
2029

30+
/** Returns the status of expression compilation for filter expression requests.
31+
* @note added in QGIS 2.16
32+
*/
33+
CompileStatus compileStatus() const;
34+
2135
protected:
2236
/**
2337
* If you write a feature iterator for your provider, this is the method you
@@ -84,6 +98,7 @@ class QgsFeatureIterator
8498
PyErr_SetString(PyExc_StopIteration,"");
8599
}
86100
%End
101+
87102
//! construct invalid iterator
88103
QgsFeatureIterator();
89104
//! construct a valid iterator
@@ -101,4 +116,10 @@ class QgsFeatureIterator
101116

102117
//! find out whether the iterator is still valid or closed already
103118
bool isClosed() const;
119+
120+
/** Returns the status of expression compilation for filter expression requests.
121+
* @note added in QGIS 2.16
122+
*/
123+
QgsAbstractFeatureIterator::CompileStatus compileStatus() const;
124+
104125
};

‎src/core/qgsfeatureiterator.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ QgsAbstractFeatureIterator::QgsAbstractFeatureIterator( const QgsFeatureRequest&
2626
, mZombie( false )
2727
, refs( 0 )
2828
, mFetchedCount( 0 )
29+
, mCompileStatus( NoCompilation )
2930
, mGeometrySimplifier( nullptr )
3031
, mLocalSimplification( false )
3132
, mUseCachedFeatures( false )

‎src/core/qgsfeatureiterator.h

+22
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,15 @@ class CORE_EXPORT QgsInterruptionChecker
4040
class CORE_EXPORT QgsAbstractFeatureIterator
4141
{
4242
public:
43+
44+
//! Status of expression compilation for filter expression requests
45+
enum CompileStatus
46+
{
47+
NoCompilation, /*!< Expression could not be compiled or not attempt was made to compile expression */
48+
PartiallyCompiled, /*!< Expression was partially compiled, but extra checks need to be applied to features*/
49+
Compiled, /*!< Expression was fully compiled and delegated to data provider source*/
50+
};
51+
4352
//! base class constructor - stores the iteration parameters
4453
QgsAbstractFeatureIterator( const QgsFeatureRequest& request );
4554

@@ -64,6 +73,11 @@ class CORE_EXPORT QgsAbstractFeatureIterator
6473
*/
6574
virtual void setInterruptionChecker( QgsInterruptionChecker* interruptionChecker );
6675

76+
/** Returns the status of expression compilation for filter expression requests.
77+
* @note added in QGIS 2.16
78+
*/
79+
CompileStatus compileStatus() const { return mCompileStatus; }
80+
6781
protected:
6882
/**
6983
* If you write a feature iterator for your provider, this is the method you
@@ -124,6 +138,9 @@ class CORE_EXPORT QgsAbstractFeatureIterator
124138
//! Number of features already fetched by iterator
125139
long mFetchedCount;
126140

141+
//! Status of compilation of filter expression
142+
CompileStatus mCompileStatus;
143+
127144
//! Setup the simplification of geometries to fetch using the specified simplify method
128145
virtual bool prepareSimplification( const QgsSimplifyMethod& simplifyMethod );
129146

@@ -225,6 +242,11 @@ class CORE_EXPORT QgsFeatureIterator
225242
*/
226243
void setInterruptionChecker( QgsInterruptionChecker* interruptionChecker );
227244

245+
/** Returns the status of expression compilation for filter expression requests.
246+
* @note added in QGIS 2.16
247+
*/
248+
QgsAbstractFeatureIterator::CompileStatus compileStatus() const { return mIter->compileStatus(); }
249+
228250
friend bool operator== ( const QgsFeatureIterator &fi1, const QgsFeatureIterator &fi2 );
229251
friend bool operator!= ( const QgsFeatureIterator &fi1, const QgsFeatureIterator &fi2 );
230252

‎src/core/qgssqlexpressioncompiler.cpp

+10-1
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,16 @@ QgsSqlExpressionCompiler::Result QgsSqlExpressionCompiler::compileNode( const Qg
8181
switch ( n->op() )
8282
{
8383
case QgsExpression::uoNot:
84-
break;
84+
{
85+
QString right;
86+
if ( compileNode( n->operand(), right ) == Complete )
87+
{
88+
result = "( NOT " + right + ')';
89+
return Complete;
90+
}
91+
92+
return Fail;
93+
}
8594

8695
case QgsExpression::uoMinus:
8796
break;

‎src/providers/db2/qgsdb2featureiterator.cpp

+2
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,7 @@ void QgsDb2FeatureIterator::BuildStatement( const QgsFeatureRequest& request )
179179
}
180180

181181
mExpressionCompiled = false;
182+
mCompileStatus = NoCompilation;
182183
if ( request.filterType() == QgsFeatureRequest::FilterExpression )
183184
{
184185
QgsDebugMsg( QString( "compileExpressions: %1" ).arg( QSettings().value( "/qgis/compileExpressions", true ).toString() ) );
@@ -199,6 +200,7 @@ void QgsDb2FeatureIterator::BuildStatement( const QgsFeatureRequest& request )
199200

200201
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
201202
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
203+
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
202204
limitAtProvider = mExpressionCompiled;
203205
}
204206
else

‎src/providers/mssql/qgsmssqlfeatureiterator.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
175175

176176
//NOTE - must be last added!
177177
mExpressionCompiled = false;
178+
mCompileStatus = NoCompilation;
178179
if ( request.filterType() == QgsFeatureRequest::FilterExpression )
179180
{
180181
if ( QSettings().value( "/qgis/compileExpressions", true ).toBool() )
@@ -192,6 +193,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
192193

193194
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
194195
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
196+
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
195197
limitAtProvider = mExpressionCompiled;
196198
}
197199
else
@@ -369,7 +371,10 @@ bool QgsMssqlFeatureIterator::rewind()
369371
//try with fallback statement
370372
result = mQuery->exec( mOrderByClause.isEmpty() ? mFallbackStatement : mFallbackStatement + mOrderByClause );
371373
if ( result )
374+
{
372375
mExpressionCompiled = false;
376+
mCompileStatus = NoCompilation;
377+
}
373378
}
374379

375380
if ( !result && !mOrderByClause.isEmpty() )
@@ -388,6 +393,7 @@ bool QgsMssqlFeatureIterator::rewind()
388393
{
389394
mExpressionCompiled = false;
390395
mOrderByCompiled = false;
396+
mCompileStatus = NoCompilation;
391397
}
392398
}
393399

‎src/providers/ogr/qgsogrfeatureiterator.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
111111
{
112112
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
113113
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
114+
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
114115
}
115116
}
116117
else

‎src/providers/postgres/qgspostgresfeatureiterator.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
112112
fallbackWhereClause = whereClause;
113113
whereClause = QgsPostgresUtils::andWhereClauses( whereClause, compiler.result() );
114114
mExpressionCompiled = true;
115+
mCompileStatus = Compiled;
115116
}
116117
else
117118
{

‎src/providers/spatialite/qgsspatialitefeatureiterator.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
112112
whereClauses.append( whereClause );
113113
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
114114
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
115+
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
115116
}
116117
}
117118
if ( result != QgsSqlExpressionCompiler::Complete )

‎tests/src/python/providertestbase.py

+24-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
# This will get replaced with a git SHA1 when you do a git archive
1313
__revision__ = '$Format:%H$'
1414

15-
from qgis.core import QgsRectangle, QgsFeatureRequest, QgsFeature, QgsGeometry, NULL
15+
from qgis.core import QgsRectangle, QgsFeatureRequest, QgsFeature, QgsGeometry, QgsAbstractFeatureIterator, NULL
1616

1717
from utilities import(
1818
compareWkt
@@ -61,10 +61,31 @@ def testGetFeatures(self):
6161
else:
6262
self.assertFalse(geometries[pk], 'Expected null geometry for {}'.format(pk))
6363

64+
def uncompiledFilters(self):
65+
""" Individual derived provider tests should override this to return a list of expressions which
66+
cannot be compiled """
67+
return set()
68+
69+
def partiallyCompiledFilters(self):
70+
""" Individual derived provider tests should override this to return a list of expressions which
71+
should be partially compiled """
72+
return set()
73+
6474
def assert_query(self, provider, expression, expected):
6575
result = set([f['pk'] for f in provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression))])
6676
assert set(expected) == result, 'Expected {} and got {} when testing expression "{}"'.format(set(expected), result, expression)
6777

78+
if self.compiled:
79+
# Check compilation status
80+
it = provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression))
81+
82+
if expression in self.uncompiledFilters():
83+
self.assertEqual(it.compileStatus(), QgsAbstractFeatureIterator.NoCompilation)
84+
elif expression in self.partiallyCompiledFilters():
85+
self.assertEqual(it.compileStatus(), QgsAbstractFeatureIterator.PartiallyCompiled)
86+
else:
87+
self.assertEqual(it.compileStatus(), QgsAbstractFeatureIterator.Compiled)
88+
6889
# Also check that filter works when referenced fields are not being retrieved by request
6990
result = set([f['pk'] for f in provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression).setSubsetOfAttributes([0]))])
7091
assert set(expected) == result, 'Expected {} and got {} when testing expression "{}" using empty attribute subset'.format(set(expected), result, expression)
@@ -155,6 +176,7 @@ def runGetFeatureTests(self, provider):
155176
self.assert_query(provider, 'num_char IN (2, 4, 5)', [2, 4, 5])
156177

157178
def testGetFeaturesUncompiled(self):
179+
self.compiled = False
158180
try:
159181
self.disableCompiler()
160182
except AttributeError:
@@ -164,6 +186,7 @@ def testGetFeaturesUncompiled(self):
164186
def testGetFeaturesCompiled(self):
165187
try:
166188
self.enableCompiler()
189+
self.compiled = True
167190
self.runGetFeatureTests(self.provider)
168191
except AttributeError:
169192
print('Provider does not support compiling')

‎tests/src/python/test_provider_postgres.py

+6
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ def enableCompiler(self):
5858
def disableCompiler(self):
5959
QSettings().setValue(u'/qgis/compileExpressions', False)
6060

61+
def uncompiledFilters(self):
62+
return set([])
63+
64+
def partiallyCompiledFilters(self):
65+
return set([])
66+
6167
# HERE GO THE PROVIDER SPECIFIC TESTS
6268
def testDefaultValue(self):
6369
self.assertEqual(self.provider.defaultValue(0), u'nextval(\'qgis_test."someData_pk_seq"\'::regclass)')

‎tests/src/python/test_provider_shapefile.py

+42
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,48 @@ def enableCompiler(self):
6666
def disableCompiler(self):
6767
QSettings().setValue(u'/qgis/compileExpressions', False)
6868

69+
def uncompiledFilters(self):
70+
return set(['name ILIKE \'QGIS\'',
71+
'"name" NOT LIKE \'Ap%\'',
72+
'"name" NOT ILIKE \'QGIS\'',
73+
'"name" NOT ILIKE \'pEAR\'',
74+
'name <> \'Apple\'',
75+
'"name" <> \'apple\'',
76+
'(name = \'Apple\') is not null',
77+
'name ILIKE \'aPple\'',
78+
'name ILIKE \'%pp%\'',
79+
'cnt = 1100 % 1000',
80+
'"name" || \' \' || "name" = \'Orange Orange\'',
81+
'"name" || \' \' || "cnt" = \'Orange 100\'',
82+
'\'x\' || "name" IS NOT NULL',
83+
'\'x\' || "name" IS NULL',
84+
'cnt = 10 ^ 2',
85+
'"name" ~ \'[OP]ra[gne]+\'',
86+
'false and NULL',
87+
'true and NULL',
88+
'NULL and false',
89+
'NULL and true',
90+
'NULL and NULL',
91+
'false or NULL',
92+
'true or NULL',
93+
'NULL or false',
94+
'NULL or true',
95+
'NULL or NULL',
96+
'not null',
97+
'not name = \'Apple\'',
98+
'not name = \'Apple\' or name = \'Apple\'',
99+
'not name = \'Apple\' or not name = \'Apple\'',
100+
'not name = \'Apple\' and pk = 4',
101+
'not name = \'Apple\' and not pk = 4',
102+
'num_char IN (2, 4, 5)'])
103+
104+
def partiallyCompiledFilters(self):
105+
return set(['name = \'Apple\'',
106+
'name = \'apple\'',
107+
'name LIKE \'Apple\'',
108+
'name LIKE \'aPple\'',
109+
'"name"="name2"'])
110+
69111
def testRepack(self):
70112
vl = QgsVectorLayer(u'{}|layerid=0'.format(self.repackfile), u'test', u'ogr')
71113

‎tests/src/python/test_provider_spatialite.py

+10
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,16 @@ def enableCompiler(self):
131131
def disableCompiler(self):
132132
QSettings().setValue(u'/qgis/compileExpressions', False)
133133

134+
def uncompiledFilters(self):
135+
return set(['cnt = 10 ^ 2',
136+
'"name" ~ \'[OP]ra[gne]+\''])
137+
138+
def partiallyCompiledFilters(self):
139+
return set(['"name" NOT LIKE \'Ap%\'',
140+
'name LIKE \'Apple\'',
141+
'name LIKE \'aPple\''
142+
])
143+
134144
def test_SplitFeature(self):
135145
"""Create spatialite database"""
136146
layer = QgsVectorLayer("dbname=%s table=test_pg (geometry)" % self.dbname, "test_pg", "spatialite")

0 commit comments

Comments
 (0)