Handle type conversion failures for compiled expressions

qgis · Dec 10, 2015 · 89b9b67 · 89b9b67
1 parent 0386461
commit 89b9b67
Show file tree

Hide file tree

Showing 10 changed files with 74 additions and 44 deletions.
diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp
@@ -61,11 +61,19 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
 
   bool limitAtProvider = ( mRequest.limit() >= 0 );
 
+  bool useFallbackWhereClause = false;
+  QString fallbackWhereClause;
+
   if ( !request.filterRect().isNull() && !mSource->mGeometryColumn.isNull() )
   {
     whereClause = whereClauseRect();
   }
 
+  if ( !mSource->mSqlWhereClause.isEmpty() )
+  {
+    whereClause = QgsPostgresUtils::andWhereClauses( whereClause, '(' + mSource->mSqlWhereClause + ')' );
+  }
+
   if ( request.filterType() == QgsFeatureRequest::FilterFid )
   {
     QString fidWhereClause = QgsPostgresUtils::whereClause( mRequest.filterFid(), mSource->mFields, mConn, mSource->mPrimaryKeyType, mSource->mPrimaryKeyAttrs, mSource->mShared );
@@ -82,10 +90,13 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
   {
     if ( QSettings().value( "/qgis/compileExpressions", true ).toBool() )
     {
+      //IMPORTANT - this MUST be the last clause added!
       QgsPostgresExpressionCompiler compiler = QgsPostgresExpressionCompiler( source );
 
       if ( compiler.compile( request.filterExpression() ) == QgsSqlExpressionCompiler::Complete )
       {
+        useFallbackWhereClause = true;
+        fallbackWhereClause = whereClause;
         whereClause = QgsPostgresUtils::andWhereClauses( whereClause, compiler.result() );
         mExpressionCompiled = true;
       }
@@ -100,19 +111,19 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
     }
   }
 
-  if ( !mSource->mSqlWhereClause.isEmpty() )
+  bool success = declareCursor( whereClause, limitAtProvider ? mRequest.limit() : -1, false );
+  if ( !success && useFallbackWhereClause )
   {
-    if ( !whereClause.isEmpty() )
-      whereClause += " AND ";
-
-    whereClause += '(' + mSource->mSqlWhereClause + ')';
+    //try with the fallback where clause, eg for cases when using compiled expression failed to prepare
+    mExpressionCompiled = false;
+    success = declareCursor( fallbackWhereClause, -1, false );
   }
 
-  if ( !declareCursor( whereClause, limitAtProvider ? mRequest.limit() : -1 ) )
+  if ( !success )
   {
+    close();
     mClosed = true;
     iteratorClosed();
-    return;
   }
 
   mFetched = 0;
@@ -336,7 +347,7 @@ QString QgsPostgresFeatureIterator::whereClauseRect()
 
 
 
-bool QgsPostgresFeatureIterator::declareCursor( const QString& whereClause, long limit )
+bool QgsPostgresFeatureIterator::declareCursor( const QString& whereClause, long limit, bool closeOnFail )
 {
   mFetchGeometry = !( mRequest.flags() & QgsFeatureRequest::NoGeometry ) && !mSource->mGeometryColumn.isNull();
 #if 0
@@ -503,7 +514,8 @@ bool QgsPostgresFeatureIterator::declareCursor( const QString& whereClause, long
 
     // reloading the fields might help next time around
     // TODO how to cleanly force reload of fields?  P->loadFields();
-    close();
+    if ( closeOnFail )
+      close();
     return false;
   }
 

diff --git a/src/providers/postgres/qgspostgresfeatureiterator.h b/src/providers/postgres/qgspostgresfeatureiterator.h
@@ -97,7 +97,7 @@ class QgsPostgresFeatureIterator : public QgsAbstractFeatureIteratorFromSource<Q
     QString whereClauseRect();
     bool getFeature( QgsPostgresResult &queryResult, int row, QgsFeature &feature );
     void getFeatureAttribute( int idx, QgsPostgresResult& queryResult, int row, int& col, QgsFeature& feature );
-    bool declareCursor( const QString& whereClause, long limit = -1 );
+    bool declareCursor( const QString& whereClause, long limit = -1, bool closeOnFail = true );
 
     QString mCursorName;
 

diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp
@@ -37,6 +37,8 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
   mRowNumber = 0;
 
   QStringList whereClauses;
+  bool useFallbackWhereClause = false;
+  QString fallbackWhereClause;
   QString whereClause;
 
   //beware - limitAtProvider needs to be set to false if the request cannot be completely handled
@@ -53,6 +55,15 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
     }
   }
 
+  if ( !mSource->mSubsetString.isEmpty() )
+  {
+    whereClause = "( " + mSource->mSubsetString + ')';
+    if ( ! whereClause.isEmpty() )
+    {
+      whereClauses.append( whereClause );
+    }
+  }
+
   if ( request.filterType() == QgsFeatureRequest::FilterFid )
   {
     whereClause = whereClauseFid();
@@ -69,6 +80,7 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
       whereClauses.append( whereClause );
     }
   }
+  //IMPORTANT - this MUST be the last clause added!
   else if ( request.filterType() == QgsFeatureRequest::FilterExpression )
   {
     if ( QSettings().value( "/qgis/compileExpressions", true ).toBool() )
@@ -82,6 +94,8 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
         whereClause = compiler.result();
         if ( !whereClause.isEmpty() )
         {
+          useFallbackWhereClause = true;
+          fallbackWhereClause = whereClauses.join( " AND " );
           whereClauses.append( whereClause );
           //if only partial success when compiling expression, we need to double-check results using QGIS' expressions
           mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
@@ -99,24 +113,23 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
     }
   }
 
-  if ( !mSource->mSubsetString.isEmpty() )
-  {
-    whereClause = "( " + mSource->mSubsetString + ')';
-    if ( ! whereClause.isEmpty() )
-    {
-      whereClauses.append( whereClause );
-    }
-  }
 
   whereClause = whereClauses.join( " AND " );
 
   // preparing the SQL statement
-  if ( !prepareStatement( whereClause, limitAtProvider ? mRequest.limit() : -1 ) )
+  bool success = prepareStatement( whereClause, limitAtProvider ? mRequest.limit() : -1 );
+  if ( !success && useFallbackWhereClause )
+  {
+    //try with the fallback where clause, eg for cases when using compiled expression failed to prepare
+    mExpressionCompiled = false;
+    success = prepareStatement( fallbackWhereClause, -1 );
+  }
+
+  if ( !success )
   {
     // some error occurred
     sqliteStatement = NULL;
     close();
-    return;
   }
 }
 

diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py
@@ -99,6 +99,10 @@ def runGetFeatureTests(self, provider):
         self.assert_query(provider, 'not name = \'Apple\' and not pk = 4', [1, 3])
         self.assert_query(provider, 'not pk IN (1, 2, 4, 8)', [3, 5])
 
+        #type conversion - QGIS expressions do not mind that we are comparing a string
+        #against numeric literals
+        self.assert_query(provider, 'num_char IN (2, 4, 5)', [2, 4, 5])
+
     def testGetFeaturesUncompiled(self):
         try:
             self.disableCompiler()

diff --git a/tests/src/python/test_provider_memory.py b/tests/src/python/test_provider_memory.py
@@ -39,28 +39,28 @@ class TestPyQgsMemoryProvider(TestCase, ProviderTestCase):
     def setUpClass(cls):
         """Run before all tests"""
         # Create test layer
-        cls.vl = QgsVectorLayer(u'Point?crs=epsg:4326&field=pk:integer&field=cnt:integer&field=name:string(0)&field=name2:string(0)&key=pk',
+        cls.vl = QgsVectorLayer(u'Point?crs=epsg:4326&field=pk:integer&field=cnt:integer&field=name:string(0)&field=name2:string(0)&field=num_char:string&key=pk',
                                 u'test', u'memory')
         assert (cls.vl.isValid())
         cls.provider = cls.vl.dataProvider()
 
         f1 = QgsFeature()
-        f1.setAttributes([5, -200, NULL, 'NuLl'])
+        f1.setAttributes([5, -200, NULL, 'NuLl', '5'])
         f1.setGeometry(QgsGeometry.fromWkt('Point (-71.123 78.23)'))
 
         f2 = QgsFeature()
-        f2.setAttributes([3, 300, 'Pear', 'PEaR'])
+        f2.setAttributes([3, 300, 'Pear', 'PEaR', '3'])
 
         f3 = QgsFeature()
-        f3.setAttributes([1, 100, 'Orange', 'oranGe'])
+        f3.setAttributes([1, 100, 'Orange', 'oranGe', '1'])
         f3.setGeometry(QgsGeometry.fromWkt('Point (-70.332 66.33)'))
 
         f4 = QgsFeature()
-        f4.setAttributes([2, 200, 'Apple', 'Apple'])
+        f4.setAttributes([2, 200, 'Apple', 'Apple', '2'])
         f4.setGeometry(QgsGeometry.fromWkt('Point (-68.2 70.8)'))
 
         f5 = QgsFeature()
-        f5.setAttributes([4, 400, 'Honey', 'Honey'])
+        f5.setAttributes([4, 400, 'Honey', 'Honey', '4'])
         f5.setGeometry(QgsGeometry.fromWkt('Point (-65.32 78.3)'))
 
         cls.provider.addFeatures([f1, f2, f3, f4, f5])

diff --git a/tests/testdata/provider/delimited_wkt.csv b/tests/testdata/provider/delimited_wkt.csv
@@ -1,6 +1,6 @@
-pk,cnt,name,name2,wkt
-5,-200,,NuLl,Point(-71.123 78.23)
-3,300,Pear,PEaR,
-1,100,Orange,oranGe,Point(-70.332 66.33)
-2,200,Apple,Apple,Point(-68.2 70.8)
-4,400,Honey,Honey,Point(-65.32 78.3)
+pk,cnt,name,name2,num_char,wkt
+5,-200,,NuLl,5,Point(-71.123 78.23)
+3,300,Pear,3,PEaR,
+1,100,Orange,oranGe,1,Point(-70.332 66.33)
+2,200,Apple,Apple,2,Point(-68.2 70.8)
+4,400,Honey,Honey,4,Point(-65.32 78.3)
diff --git a/tests/testdata/provider/delimited_xy.csv b/tests/testdata/provider/delimited_xy.csv
@@ -1,6 +1,6 @@
-pk,cnt,name,name2,X,Y
-5,-200,,NuLl,-71.123,78.23
-3,300,Pear,PEaR,,
-1,100,Orange,oranGe,-70.332,66.33
-2,200,Apple,Apple,-68.2,70.8
-4,400,Honey,Honey,-65.32,78.3
+pk,cnt,name,name2,num_char,X,Y
+5,-200,,NuLl,5,-71.123,78.23
+3,300,Pear,PEaR,3,,
+1,100,Orange,oranGe,1,-70.332,66.33
+2,200,Apple,Apple,2,-68.2,70.8
+4,400,Honey,Honey,4,-65.32,78.3
diff --git a/tests/testdata/provider/shapefile.dbf b/tests/testdata/provider/shapefile.dbf
diff --git a/tests/testdata/provider/spatialite.db b/tests/testdata/provider/spatialite.db
diff --git a/tests/testdata/provider/testdata.sql b/tests/testdata/provider/testdata.sql
@@ -38,6 +38,7 @@ CREATE TABLE qgis_test."someData" (
     cnt integer,
     name text DEFAULT 'qgis',
     name2 text DEFAULT 'qgis',
+    num_char text,
     geom public.geometry(Point,4326)
 );
 
@@ -48,12 +49,12 @@ CREATE TABLE qgis_test."someData" (
 -- Data for Name: someData; Type: TABLE DATA; Schema: qgis_test; Owner: postgres
 --
 
-INSERT INTO qgis_test."someData" (pk, cnt, name, name2, geom) VALUES
-(5, -200, NULL, 'NuLl', '0101000020E61000001D5A643BDFC751C01F85EB51B88E5340'),
-(3,  300, 'Pear', 'PEaR', NULL),
-(1,  100, 'Orange', 'oranGe', '0101000020E61000006891ED7C3F9551C085EB51B81E955040'),
-(2,  200, 'Apple', 'Apple', '0101000020E6100000CDCCCCCCCC0C51C03333333333B35140'),
-(4,  400, 'Honey', 'Honey', '0101000020E610000014AE47E17A5450C03333333333935340')
+INSERT INTO qgis_test."someData" (pk, cnt, name, name2, num_char, geom) VALUES
+(5, -200, NULL, 'NuLl', '5', '0101000020E61000001D5A643BDFC751C01F85EB51B88E5340'),
+(3,  300, 'Pear', 'PEaR', '3', NULL),
+(1,  100, 'Orange', 'oranGe', '1', '0101000020E61000006891ED7C3F9551C085EB51B81E955040'),
+(2,  200, 'Apple', 'Apple', '2', '0101000020E6100000CDCCCCCCCC0C51C03333333333B35140'),
+(4,  400, 'Honey', 'Honey', '4', '0101000020E610000014AE47E17A5450C03333333333935340')
 ;