Fix other feature's geometries are shown instead of null geometry

in delimited text provider (fix #14666) Add tests, also fix virtual layer, mssql and db2 providers which suffered the same bug
qgis · Apr 13, 2016 · 02e0e3f · 02e0e3f
1 parent ecc85a9
commit 02e0e3f
Show file tree

Hide file tree

Showing 9 changed files with 402 additions and 273 deletions.
diff --git a/src/providers/db2/qgsdb2featureiterator.cpp b/src/providers/db2/qgsdb2featureiterator.cpp
@@ -360,8 +360,13 @@ bool QgsDb2FeatureIterator::fetchFeature( QgsFeature& feature )
       else
       {
         QgsDebugMsg( "Geometry is empty" );
+        feature.setGeometry( nullptr );
       }
     }
+    else
+    {
+      feature.setGeometry( nullptr );
+    }
     feature.setValid( true );
     mFetchCount++;
     if ( mFetchCount % 100 == 0 )

diff --git a/src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp b/src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp
@@ -56,19 +56,20 @@ QgsDelimitedTextFeatureIterator::QgsDelimitedTextFeatureIterator( QgsDelimitedTe
     QgsRectangle rect = request.filterRect();
 
     // If request doesn't overlap extents, then nothing to return
-    if ( ! rect.intersects( mSource->mExtent ) )
+    if ( ! rect.intersects( mSource->mExtent ) && !mTestSubset )
     {
       QgsDebugMsg( "Rectangle outside layer extents - no features to return" );
       mMode = FeatureIds;
     }
     // If the request extents include the entire layer, then revert to
     // a file scan
 
-    else if ( rect.contains( mSource->mExtent ) )
+    else if ( rect.contains( mSource->mExtent ) && !mTestSubset )
     {
       QgsDebugMsg( "Rectangle contains layer extents - bypass spatial filter" );
       mTestGeometry = false;
     }
+
     // If we have a spatial index then use it.  The spatial index already accounts
     // for the subset.  Also means we don't have to test geometries unless doing exact
     // intersection
@@ -332,9 +333,7 @@ bool QgsDelimitedTextFeatureIterator::nextFeatureInternal( QgsFeature& feature )
     feature.setFields( mSource->mFields ); // allow name-based attribute lookups
     feature.setFeatureId( fid );
     feature.initAttributes( mSource->mFields.count() );
-
-    if ( geom )
-      feature.setGeometry( geom );
+    feature.setGeometry( geom );
 
     // If we are testing subset expression, then need all attributes just in case.
     // Could be more sophisticated, but probably not worth it!

diff --git a/src/providers/delimitedtext/qgsdelimitedtextprovider.cpp b/src/providers/delimitedtext/qgsdelimitedtextprovider.cpp
@@ -779,7 +779,7 @@ void QgsDelimitedTextProvider::rescanFile()
   bool foundFirstGeometry = false;
   while ( fi.nextFeature( f ) )
   {
-    if ( mGeometryType != QGis::NoGeometry )
+    if ( mGeometryType != QGis::NoGeometry && f.constGeometry() )
     {
       if ( !foundFirstGeometry )
       {

diff --git a/src/providers/mssql/qgsmssqlfeatureiterator.cpp b/src/providers/mssql/qgsmssqlfeatureiterator.cpp
@@ -315,6 +315,14 @@ bool QgsMssqlFeatureIterator::fetchFeature( QgsFeature& feature )
         g->fromWkb( wkb, mParser.GetWkbLen() );
         feature.setGeometry( g );
       }
+      else
+      {
+        feature.setGeometry( nullptr );
+      }
+    }
+    else
+    {
+      feature.setGeometry( nullptr );
     }
 
     feature.setValid( true );

diff --git a/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp b/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp
@@ -228,6 +228,10 @@ bool QgsVirtualLayerFeatureIterator::fetchFeature( QgsFeature& feature )
     {
       feature.setGeometry( spatialiteBlobToQgsGeometry( blob.constData(), blob.size() ) );
     }
+    else
+    {
+      feature.setGeometry( nullptr );
+    }
   }
 
   feature.setValid( true );

diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py
@@ -14,9 +14,53 @@
 
 from qgis.core import QgsRectangle, QgsFeatureRequest, QgsFeature, QgsGeometry, NULL
 
+from utilities import(
+    compareWkt
+)
+
 
 class ProviderTestCase(object):
 
+    def testGetFeatures(self):
+        """ Test that expected results are returned when fetching all features """
+
+        # IMPORTANT - we do not use `for f in provider.getFeatures()` as we are also
+        # testing that existing attributes & geometry in f are overwritten correctly
+        # (for f in ... uses a new QgsFeature for every iteration)
+
+        it = self.provider.getFeatures()
+        f = QgsFeature()
+        attributes = {}
+        geometries = {}
+        while it.nextFeature(f):
+            # split off the first 5 attributes only - some provider test datasets will include
+            # additional attributes which we ignore
+            attrs = f.attributes()[0:5]
+            # force the num_char attribute to be text - some providers (eg delimited text) will
+            # automatically detect that this attribute contains numbers and set it as a numeric
+            # field
+            attrs[4] = str(attrs[4])
+            attributes[f['pk']] = attrs
+            geometries[f['pk']] = f.constGeometry() and f.constGeometry().exportToWkt()
+
+        expected_attributes = {5: [5, -200, NULL, 'NuLl', '5'],
+                               3: [3, 300, 'Pear', 'PEaR', '3'],
+                               1: [1, 100, 'Orange', 'oranGe', '1'],
+                               2: [2, 200, 'Apple', 'Apple', '2'],
+                               4: [4, 400, 'Honey', 'Honey', '4']}
+        self.assertEqual(attributes, expected_attributes, 'Expected {}, got {}'.format(expected_attributes, attributes))
+
+        expected_geometries = {1: 'Point (-70.332 66.33)',
+                               2: 'Point (-68.2 70.8)',
+                               3: None,
+                               4: 'Point(-65.32 78.3)',
+                               5: 'Point(-71.123 78.23)'}
+        for pk, geom in expected_geometries.iteritems():
+            if geom:
+                assert compareWkt(geom, geometries[pk]), "Geometry {} mismatch Expected:\n{}\nGot:\n{}\n".format(pk, geom, geometries[pk].exportToWkt())
+            else:
+                self.assertFalse(geometries[pk], 'Expected null geometry for {}'.format(pk))
+
     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)