Fix some providers returning valid features when iterator is closed

Also add some additional tests to ProviderTestCase for closed iterators
and subset string handling.

src/providers/spatialite/qgsspatialitefeatureiterator.cpp

@@ -184,11 +184,11 @@ QgsSpatiaLiteFeatureIterator::~QgsSpatiaLiteFeatureIterator()
 
 bool QgsSpatiaLiteFeatureIterator::fetchFeature( QgsFeature& feature )
 {
+  feature.setValid( false );
+
   if ( mClosed )
     return false;
 
-  feature.setValid( false );
-
   if ( !sqliteStatement )
   {
     QgsDebugMsg( "Invalid current SQLite statement" );

src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp

@@ -166,6 +166,8 @@ bool QgsVirtualLayerFeatureIterator::close()
 
 bool QgsVirtualLayerFeatureIterator::fetchFeature( QgsFeature& feature )
 {
+  feature.setValid( false );
+
   if ( mClosed )
   {
     return false;
@@ -218,6 +220,7 @@ bool QgsVirtualLayerFeatureIterator::fetchFeature( QgsFeature& feature )
     }
   }
 
+  feature.setValid( true );
   return true;
 }
 

tests/src/python/providertestbase.py

@@ -120,6 +120,47 @@ def testGetFeaturesCompiled(self):
         except AttributeError:
             print 'Provider does not support compiling'
 
+    def testSubsetString(self):
+        if not self.provider.supportsSubsetString():
+            print 'Provider does not support subset strings'
+            return
+
+        subset = self.getSubsetString()
+        self.provider.setSubsetString(subset)
+        self.assertEqual(self.provider.subsetString(), subset)
+        result = set([f['pk'] for f in self.provider.getFeatures()])
+        self.provider.setSubsetString(None)
+
+        expected = set([2, 3, 4])
+        assert set(expected) == result, 'Expected {} and got {} when testing subset string {}'.format(set(expected), result, subset)
+
+        # Subset string AND filter rect
+        self.provider.setSubsetString(subset)
+        extent = QgsRectangle(-70, 70, -60, 75)
+        result = set([f['pk'] for f in self.provider.getFeatures(QgsFeatureRequest().setFilterRect(extent))])
+        self.provider.setSubsetString(None)
+        expected = set([2])
+        assert set(expected) == result, 'Expected {} and got {} when testing subset string {}'.format(set(expected), result, subset)
+
+        # Subset string AND filter rect, version 2
+        self.provider.setSubsetString(subset)
+        extent = QgsRectangle(-71, 65, -60, 80)
+        result = set([f['pk'] for f in self.provider.getFeatures(QgsFeatureRequest().setFilterRect(extent))])
+        self.provider.setSubsetString(None)
+        expected = set([2, 4])
+        assert set(expected) == result, 'Expected {} and got {} when testing subset string {}'.format(set(expected), result, subset)
+
+        # Subset string AND expression
+        self.provider.setSubsetString(subset)
+        result = set([f['pk'] for f in self.provider.getFeatures(QgsFeatureRequest().setFilterExpression('length("name")=5'))])
+        self.provider.setSubsetString(None)
+        expected = set([2, 4])
+        assert set(expected) == result, 'Expected {} and got {} when testing subset string {}'.format(set(expected), result, subset)
+
+    def getSubsetString(self):
+        """Individual providers may need to override this depending on their subset string formats"""
+        return '"cnt" > 100 and "cnt" < 410'
+
     def testOrderBy(self):
         request = QgsFeatureRequest().addOrderBy('cnt')
         values = [f['cnt'] for f in self.provider.getFeatures(request)]
@@ -285,3 +326,18 @@ def testUnique(self):
 
     def testFeatureCount(self):
         assert self.provider.featureCount() == 5, 'Got {}'.format(self.provider.featureCount())
+
+    def testClosedIterators(self):
+        """ Test behaviour of closed iterators """
+
+        # Test retrieving feature after closing iterator
+        f_it = self.provider.getFeatures(QgsFeatureRequest())
+        fet = QgsFeature()
+        assert f_it.nextFeature(fet), 'Could not fetch feature'
+        assert fet.isValid(), 'Feature is not valid'
+        assert f_it.close(), 'Could not close iterator'
+        self.assertFalse(f_it.nextFeature(fet), 'Fetched feature after iterator closed, expected nextFeature() to return False')
+        self.assertFalse(fet.isValid(), 'Valid feature fetched from closed iterator, should be invalid')
+
+        # Test rewinding closed iterator
+        self.assertFalse(f_it.rewind(), 'Rewinding closed iterator successful, should not be allowed')

tests/src/python/test_provider_memory.py

@@ -218,5 +218,42 @@ def testSaveFields(self):
             assert f == importedFields.field(f.name())
 
 
+class TestPyQgsMemoryProviderIndexed(TestCase, ProviderTestCase):
+    """Runs the provider test suite against an indexed memory layer"""
+
+    @classmethod
+    def setUpClass(cls):
+        """Run before all tests"""
+        # Create test layer
+        cls.vl = QgsVectorLayer(u'Point?crs=epsg:4326&index=yes&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', '5'])
+        f1.setGeometry(QgsGeometry.fromWkt('Point (-71.123 78.23)'))
+
+        f2 = QgsFeature()
+        f2.setAttributes([3, 300, 'Pear', 'PEaR', '3'])
+
+        f3 = QgsFeature()
+        f3.setAttributes([1, 100, 'Orange', 'oranGe', '1'])
+        f3.setGeometry(QgsGeometry.fromWkt('Point (-70.332 66.33)'))
+
+        f4 = QgsFeature()
+        f4.setAttributes([2, 200, 'Apple', 'Apple', '2'])
+        f4.setGeometry(QgsGeometry.fromWkt('Point (-68.2 70.8)'))
+
+        f5 = QgsFeature()
+        f5.setAttributes([4, 400, 'Honey', 'Honey', '4'])
+        f5.setGeometry(QgsGeometry.fromWkt('Point (-65.32 78.3)'))
+
+        cls.provider.addFeatures([f1, f2, f3, f4, f5])
+
+    @classmethod
+    def tearDownClass(cls):
+        """Run after all tests"""
+
 if __name__ == '__main__':
     unittest.main()