Handle type conversion failures for compiled expressions

nyalldawson · nyalldawson · commit 89b9b679742c · 2015-12-10T20:49:17.000+11:00
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')
 ;
 
 

Original file line number	Diff line number	Diff line change
`@@ -61,11 +61,19 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource`
`61`	`61`
`62`	`62`	`bool limitAtProvider = ( mRequest.limit() >= 0 );`
`63`	`63`
	`64`	`+ bool useFallbackWhereClause = false;`
	`65`	`+ QString fallbackWhereClause;`
	`66`	`+`
`64`	`67`	`if ( !request.filterRect().isNull() && !mSource->mGeometryColumn.isNull() )`
`65`	`68`	`{`
`66`	`69`	`whereClause = whereClauseRect();`
`67`	`70`	`}`
`68`	`71`
	`72`	`+ if ( !mSource->mSqlWhereClause.isEmpty() )`
	`73`	`+ {`
	`74`	`+ whereClause = QgsPostgresUtils::andWhereClauses( whereClause, '(' + mSource->mSqlWhereClause + ')' );`
	`75`	`+ }`
	`76`	`+`
`69`	`77`	`if ( request.filterType() == QgsFeatureRequest::FilterFid )`
`70`	`78`	`{`
`71`	`79`	`QString fidWhereClause = QgsPostgresUtils::whereClause( mRequest.filterFid(), mSource->mFields, mConn, mSource->mPrimaryKeyType, mSource->mPrimaryKeyAttrs, mSource->mShared );`
`@@ -82,10 +90,13 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource`
`82`	`90`	`{`
`83`	`91`	`if ( QSettings().value( "/qgis/compileExpressions", true ).toBool() )`
`84`	`92`	`{`
	`93`	`+ //IMPORTANT - this MUST be the last clause added!`
`85`	`94`	`QgsPostgresExpressionCompiler compiler = QgsPostgresExpressionCompiler( source );`
`86`	`95`
`87`	`96`	`if ( compiler.compile( request.filterExpression() ) == QgsSqlExpressionCompiler::Complete )`
`88`	`97`	`{`
	`98`	`+ useFallbackWhereClause = true;`
	`99`	`+ fallbackWhereClause = whereClause;`
`89`	`100`	`whereClause = QgsPostgresUtils::andWhereClauses( whereClause, compiler.result() );`
`90`	`101`	`mExpressionCompiled = true;`
`91`	`102`	`}`
`@@ -100,19 +111,19 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource`
`100`	`111`	`}`
`101`	`112`	`}`
`102`	`113`
`103`		`- if ( !mSource->mSqlWhereClause.isEmpty() )`
	`114`	`+ bool success = declareCursor( whereClause, limitAtProvider ? mRequest.limit() : -1, false );`
	`115`	`+ if ( !success && useFallbackWhereClause )`
`104`	`116`	`{`
`105`		`- if ( !whereClause.isEmpty() )`
`106`		`- whereClause += " AND ";`
`107`		`-`
`108`		`- whereClause += '(' + mSource->mSqlWhereClause + ')';`
	`117`	`+ //try with the fallback where clause, eg for cases when using compiled expression failed to prepare`
	`118`	`+ mExpressionCompiled = false;`
	`119`	`+ success = declareCursor( fallbackWhereClause, -1, false );`
`109`	`120`	`}`
`110`	`121`
`111`		`- if ( !declareCursor( whereClause, limitAtProvider ? mRequest.limit() : -1 ) )`
	`122`	`+ if ( !success )`
`112`	`123`	`{`
	`124`	`+ close();`
`113`	`125`	`mClosed = true;`
`114`	`126`	`iteratorClosed();`
`115`		`- return;`
`116`	`127`	`}`
`117`	`128`
`118`	`129`	`mFetched = 0;`
`@@ -336,7 +347,7 @@ QString QgsPostgresFeatureIterator::whereClauseRect()`
`336`	`347`
`337`	`348`
`338`	`349`
`339`		`-bool QgsPostgresFeatureIterator::declareCursor( const QString& whereClause, long limit )`
	`350`	`+bool QgsPostgresFeatureIterator::declareCursor( const QString& whereClause, long limit, bool closeOnFail )`
`340`	`351`	`{`
`341`	`352`	`mFetchGeometry = !( mRequest.flags() & QgsFeatureRequest::NoGeometry ) && !mSource->mGeometryColumn.isNull();`
`342`	`353`	`#if 0`
`@@ -503,7 +514,8 @@ bool QgsPostgresFeatureIterator::declareCursor( const QString& whereClause, long`
`503`	`514`
`504`	`515`	`// reloading the fields might help next time around`
`505`	`516`	`// TODO how to cleanly force reload of fields? P->loadFields();`
`506`		`- close();`
	`517`	`+ if ( closeOnFail )`
	`518`	`+ close();`
`507`	`519`	`return false;`
`508`	`520`	`}`
`509`	`521`