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 (cherry-picked from 02e0e3f)
qgis · Apr 15, 2016 · 6880b53 · 6880b53
1 parent 0161497
commit 6880b53
Show file tree

Hide file tree

Showing 7 changed files with 122 additions and 6 deletions.
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)

diff --git a/tests/src/python/test_qgsdelimitedtextprovider_wanted.py b/tests/src/python/test_qgsdelimitedtextprovider_wanted.py
@@ -2395,3 +2395,57 @@ def test_039_issue_13749():
         u'1 records have missing geometry definitions',
     ]
     return wanted
+
+
+def test_040_issue_14666():
+    wanted = {}
+    wanted['uri'] = u'file://test14666.csv?yField=y&xField=x&type=csv&delimiter=\\t'
+    wanted['fieldTypes'] = ['integer', 'double', 'double']
+    wanted['geometryType'] = 0
+    wanted['data'] = {
+        2: {
+            'id': u'1',
+            'description': u'7.15417',
+            'x': u'7.15417',
+            'y': u'50.680622',
+            '#fid': 2,
+            '#geometry': 'Point (7.1541699999999997 50.68062199999999962)',
+        },
+        3: {
+            'id': u'2',
+            'description': u'7.119219',
+            'x': u'7.119219',
+            'y': u'50.739814',
+            '#fid': 3,
+            '#geometry': 'Point (7.11921900000000019 50.73981400000000264)',
+        },
+        4: {
+            'id': u'3',
+            'description': u'NULL',
+            'x': u'NULL',
+            'y': u'NULL',
+            '#fid': 4,
+            '#geometry': 'None',
+        },
+        5: {
+            'id': u'4',
+            'description': u'NULL',
+            'x': u'NULL',
+            'y': u'NULL',
+            '#fid': 5,
+            '#geometry': 'None',
+        },
+        6: {
+            'id': u'5',
+            'description': u'7.129229',
+            'x': u'7.129229',
+            'y': u'50.703692',
+            '#fid': 6,
+            '#geometry': 'Point (7.12922899999999959 50.70369199999999665)',
+        },
+    }
+    wanted['log'] = [
+        u'Errors in file test14666.csv',
+        u'2 records have missing geometry definitions',
+    ]
+    return wanted
diff --git a/tests/testdata/delimitedtext/test14666.csv b/tests/testdata/delimitedtext/test14666.csv
@@ -0,0 +1,7 @@
+id	x	y
+1	7.15417	50.680622
+2	7.119219	50.739814
+3		
+4		
+5	7.129229	50.703692
+