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
qgis · Apr 14, 2016 · 92a1808 · 92a1808
1 parent 99210ec
commit 92a1808
Show file tree

Hide file tree

Showing 13 changed files with 147 additions and 2 deletions.
diff --git a/python/core/qgsfeatureiterator.sip b/python/core/qgsfeatureiterator.sip
@@ -4,6 +4,15 @@ class QgsAbstractFeatureIterator
 #include <qgsfeatureiterator.h>
 %End
   public:
+
+    //! Status of expression compilation for filter expression requests
+    enum CompileStatus
+    {
+      NoCompilation, /*!< Expression could not be compiled or not attempt was made to compile expression */
+      PartiallyCompiled, /*!< Expression was partially compiled, but extra checks need to be applied to features*/
+      Compiled, /*!< Expression was fully compiled and delegated to data provider source*/
+    };
+
     //! base class constructor - stores the iteration parameters
     QgsAbstractFeatureIterator( const QgsFeatureRequest& request );
 
@@ -18,6 +27,11 @@ class QgsAbstractFeatureIterator
     //! end of iterating: free the resources / lock
     virtual bool close() = 0;
 
+    /** Returns the status of expression compilation for filter expression requests.
+     * @note added in QGIS 2.16
+     */
+    CompileStatus compileStatus() const;
+
   protected:
     /**
      * If you write a feature iterator for your provider, this is the method you
@@ -84,6 +98,7 @@ class QgsFeatureIterator
     PyErr_SetString(PyExc_StopIteration,"");
   }
 %End
+
     //! construct invalid iterator
     QgsFeatureIterator();
     //! construct a valid iterator
@@ -101,4 +116,10 @@ class QgsFeatureIterator
 
     //! find out whether the iterator is still valid or closed already
     bool isClosed() const;
+
+    /** Returns the status of expression compilation for filter expression requests.
+     * @note added in QGIS 2.16
+     */
+    QgsAbstractFeatureIterator::CompileStatus compileStatus() const;
+
 };
diff --git a/src/core/qgsfeatureiterator.cpp b/src/core/qgsfeatureiterator.cpp
@@ -26,6 +26,7 @@ QgsAbstractFeatureIterator::QgsAbstractFeatureIterator( const QgsFeatureRequest&
     , mZombie( false )
     , refs( 0 )
     , mFetchedCount( 0 )
+    , mCompileStatus( NoCompilation )
     , mGeometrySimplifier( nullptr )
     , mLocalSimplification( false )
     , mUseCachedFeatures( false )

diff --git a/src/core/qgsfeatureiterator.h b/src/core/qgsfeatureiterator.h
@@ -40,6 +40,15 @@ class CORE_EXPORT QgsInterruptionChecker
 class CORE_EXPORT QgsAbstractFeatureIterator
 {
   public:
+
+    //! Status of expression compilation for filter expression requests
+    enum CompileStatus
+    {
+      NoCompilation, /*!< Expression could not be compiled or not attempt was made to compile expression */
+      PartiallyCompiled, /*!< Expression was partially compiled, but extra checks need to be applied to features*/
+      Compiled, /*!< Expression was fully compiled and delegated to data provider source*/
+    };
+
     //! base class constructor - stores the iteration parameters
     QgsAbstractFeatureIterator( const QgsFeatureRequest& request );
 
@@ -64,6 +73,11 @@ class CORE_EXPORT QgsAbstractFeatureIterator
      */
     virtual void setInterruptionChecker( QgsInterruptionChecker* interruptionChecker );
 
+    /** Returns the status of expression compilation for filter expression requests.
+     * @note added in QGIS 2.16
+     */
+    CompileStatus compileStatus() const { return mCompileStatus; }
+
   protected:
     /**
      * If you write a feature iterator for your provider, this is the method you
@@ -124,6 +138,9 @@ class CORE_EXPORT QgsAbstractFeatureIterator
     //! Number of features already fetched by iterator
     long mFetchedCount;
 
+    //! Status of compilation of filter expression
+    CompileStatus mCompileStatus;
+
     //! Setup the simplification of geometries to fetch using the specified simplify method
     virtual bool prepareSimplification( const QgsSimplifyMethod& simplifyMethod );
 
@@ -225,6 +242,11 @@ class CORE_EXPORT QgsFeatureIterator
      */
     void setInterruptionChecker( QgsInterruptionChecker* interruptionChecker );
 
+    /** Returns the status of expression compilation for filter expression requests.
+     * @note added in QGIS 2.16
+     */
+    QgsAbstractFeatureIterator::CompileStatus compileStatus() const { return mIter->compileStatus(); }
+
     friend bool operator== ( const QgsFeatureIterator &fi1, const QgsFeatureIterator &fi2 );
     friend bool operator!= ( const QgsFeatureIterator &fi1, const QgsFeatureIterator &fi2 );
 

diff --git a/src/core/qgssqlexpressioncompiler.cpp b/src/core/qgssqlexpressioncompiler.cpp
@@ -81,7 +81,16 @@ QgsSqlExpressionCompiler::Result QgsSqlExpressionCompiler::compileNode( const Qg
       switch ( n->op() )
       {
         case QgsExpression::uoNot:
-          break;
+        {
+          QString right;
+          if ( compileNode( n->operand(), right ) == Complete )
+          {
+            result = "( NOT " + right + ')';
+            return Complete;
+          }
+
+          return Fail;
+        }
 
         case QgsExpression::uoMinus:
           break;

diff --git a/src/providers/db2/qgsdb2featureiterator.cpp b/src/providers/db2/qgsdb2featureiterator.cpp
@@ -179,6 +179,7 @@ void QgsDb2FeatureIterator::BuildStatement( const QgsFeatureRequest& request )
   }
 
   mExpressionCompiled = false;
+  mCompileStatus = NoCompilation;
   if ( request.filterType() == QgsFeatureRequest::FilterExpression )
   {
     QgsDebugMsg( QString( "compileExpressions: %1" ).arg( QSettings().value( "/qgis/compileExpressions", true ).toString() ) );
@@ -199,6 +200,7 @@ void QgsDb2FeatureIterator::BuildStatement( const QgsFeatureRequest& request )
 
         //if only partial success when compiling expression, we need to double-check results using QGIS' expressions
         mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
+        mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
         limitAtProvider = mExpressionCompiled;
       }
       else

diff --git a/src/providers/mssql/qgsmssqlfeatureiterator.cpp b/src/providers/mssql/qgsmssqlfeatureiterator.cpp
@@ -175,6 +175,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
 
   //NOTE - must be last added!
   mExpressionCompiled = false;
+  mCompileStatus = NoCompilation;
   if ( request.filterType() == QgsFeatureRequest::FilterExpression )
   {
     if ( QSettings().value( "/qgis/compileExpressions", true ).toBool() )
@@ -192,6 +193,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
 
         //if only partial success when compiling expression, we need to double-check results using QGIS' expressions
         mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
+        mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
         limitAtProvider = mExpressionCompiled;
       }
       else
@@ -369,7 +371,10 @@ bool QgsMssqlFeatureIterator::rewind()
     //try with fallback statement
     result = mQuery->exec( mOrderByClause.isEmpty() ? mFallbackStatement : mFallbackStatement + mOrderByClause );
     if ( result )
+    {
       mExpressionCompiled = false;
+      mCompileStatus = NoCompilation;
+    }
   }
 
   if ( !result && !mOrderByClause.isEmpty() )
@@ -388,6 +393,7 @@ bool QgsMssqlFeatureIterator::rewind()
     {
       mExpressionCompiled = false;
       mOrderByCompiled = false;
+      mCompileStatus = NoCompilation;
     }
   }
 

diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -111,6 +111,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
       {
         //if only partial success when compiling expression, we need to double-check results using QGIS' expressions
         mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
+        mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
       }
     }
     else

diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp
@@ -112,6 +112,7 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
         fallbackWhereClause = whereClause;
         whereClause = QgsPostgresUtils::andWhereClauses( whereClause, compiler.result() );
         mExpressionCompiled = true;
+        mCompileStatus = Compiled;
       }
       else
       {

diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp
@@ -112,6 +112,7 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
           whereClauses.append( whereClause );
           //if only partial success when compiling expression, we need to double-check results using QGIS' expressions
           mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
+          mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
         }
       }
       if ( result != QgsSqlExpressionCompiler::Complete )

diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py
@@ -12,7 +12,7 @@
 # This will get replaced with a git SHA1 when you do a git archive
 __revision__ = '$Format:%H$'
 
-from qgis.core import QgsRectangle, QgsFeatureRequest, QgsFeature, QgsGeometry, NULL
+from qgis.core import QgsRectangle, QgsFeatureRequest, QgsFeature, QgsGeometry, QgsAbstractFeatureIterator, NULL
 
 from utilities import(
     compareWkt
@@ -61,10 +61,31 @@ def testGetFeatures(self):
             else:
                 self.assertFalse(geometries[pk], 'Expected null geometry for {}'.format(pk))
 
+    def uncompiledFilters(self):
+        """ Individual derived provider tests should override this to return a list of expressions which
+        cannot be compiled """
+        return set()
+
+    def partiallyCompiledFilters(self):
+        """ Individual derived provider tests should override this to return a list of expressions which
+        should be partially compiled """
+        return set()
+
     def assert_query(self, provider, expression, expected):
         result = set([f['pk'] for f in provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression))])
         assert set(expected) == result, 'Expected {} and got {} when testing expression "{}"'.format(set(expected), result, expression)
 
+        if self.compiled:
+            # Check compilation status
+            it = provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression))
+
+            if expression in self.uncompiledFilters():
+                self.assertEqual(it.compileStatus(), QgsAbstractFeatureIterator.NoCompilation)
+            elif expression in self.partiallyCompiledFilters():
+                self.assertEqual(it.compileStatus(), QgsAbstractFeatureIterator.PartiallyCompiled)
+            else:
+                self.assertEqual(it.compileStatus(), QgsAbstractFeatureIterator.Compiled)
+
         # Also check that filter works when referenced fields are not being retrieved by request
         result = set([f['pk'] for f in provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression).setSubsetOfAttributes([0]))])
         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):
         self.assert_query(provider, 'num_char IN (2, 4, 5)', [2, 4, 5])
 
     def testGetFeaturesUncompiled(self):
+        self.compiled = False
         try:
             self.disableCompiler()
         except AttributeError:
@@ -164,6 +186,7 @@ def testGetFeaturesUncompiled(self):
     def testGetFeaturesCompiled(self):
         try:
             self.enableCompiler()
+            self.compiled = True
             self.runGetFeatureTests(self.provider)
         except AttributeError:
             print('Provider does not support compiling')

diff --git a/tests/src/python/test_provider_postgres.py b/tests/src/python/test_provider_postgres.py
@@ -58,6 +58,12 @@ def enableCompiler(self):
     def disableCompiler(self):
         QSettings().setValue(u'/qgis/compileExpressions', False)
 
+    def uncompiledFilters(self):
+        return set([])
+
+    def partiallyCompiledFilters(self):
+        return set([])
+
     # HERE GO THE PROVIDER SPECIFIC TESTS
     def testDefaultValue(self):
         self.assertEqual(self.provider.defaultValue(0), u'nextval(\'qgis_test."someData_pk_seq"\'::regclass)')

diff --git a/tests/src/python/test_provider_shapefile.py b/tests/src/python/test_provider_shapefile.py
@@ -66,6 +66,48 @@ def enableCompiler(self):
     def disableCompiler(self):
         QSettings().setValue(u'/qgis/compileExpressions', False)
 
+    def uncompiledFilters(self):
+        return set(['name ILIKE \'QGIS\'',
+                    '"name" NOT LIKE \'Ap%\'',
+                    '"name" NOT ILIKE \'QGIS\'',
+                    '"name" NOT ILIKE \'pEAR\'',
+                    'name <> \'Apple\'',
+                    '"name" <> \'apple\'',
+                    '(name = \'Apple\') is not null',
+                    'name ILIKE \'aPple\'',
+                    'name ILIKE \'%pp%\'',
+                    'cnt = 1100 % 1000',
+                    '"name" || \' \' || "name" = \'Orange Orange\'',
+                    '"name" || \' \' || "cnt" = \'Orange 100\'',
+                    '\'x\' || "name" IS NOT NULL',
+                    '\'x\' || "name" IS NULL',
+                    'cnt = 10 ^ 2',
+                    '"name" ~ \'[OP]ra[gne]+\'',
+                    'false and NULL',
+                    'true and NULL',
+                    'NULL and false',
+                    'NULL and true',
+                    'NULL and NULL',
+                    'false or NULL',
+                    'true or NULL',
+                    'NULL or false',
+                    'NULL or true',
+                    'NULL or NULL',
+                    'not null',
+                    'not name = \'Apple\'',
+                    'not name = \'Apple\' or name = \'Apple\'',
+                    'not name = \'Apple\' or not name = \'Apple\'',
+                    'not name = \'Apple\' and pk = 4',
+                    'not name = \'Apple\' and not pk = 4',
+                    'num_char IN (2, 4, 5)'])
+
+    def partiallyCompiledFilters(self):
+        return set(['name = \'Apple\'',
+                    'name = \'apple\'',
+                    'name LIKE \'Apple\'',
+                    'name LIKE \'aPple\'',
+                    '"name"="name2"'])
+
     def testRepack(self):
         vl = QgsVectorLayer(u'{}|layerid=0'.format(self.repackfile), u'test', u'ogr')
 

diff --git a/tests/src/python/test_provider_spatialite.py b/tests/src/python/test_provider_spatialite.py
@@ -131,6 +131,16 @@ def enableCompiler(self):
     def disableCompiler(self):
         QSettings().setValue(u'/qgis/compileExpressions', False)
 
+    def uncompiledFilters(self):
+        return set(['cnt = 10 ^ 2',
+                    '"name" ~ \'[OP]ra[gne]+\''])
+
+    def partiallyCompiledFilters(self):
+        return set(['"name" NOT LIKE \'Ap%\'',
+                    'name LIKE \'Apple\'',
+                    'name LIKE \'aPple\''
+                    ])
+
     def test_SplitFeature(self):
         """Create spatialite database"""
         layer = QgsVectorLayer("dbname=%s table=test_pg (geometry)" % self.dbname, "test_pg", "spatialite")