Add type check to file writer an memory layer

Fixes #36715 Adds a method to check for QVariant conversions, also check for integral type narrowing so that for example floating point 123.45 does not get down casted to integer without raising an error.
qgis · Jun 19, 2020 · 68baf74 · 68baf74
1 parent 9163100
commit 68baf74
Show file tree

Hide file tree

Showing 8 changed files with 155 additions and 2 deletions.
diff --git a/python/core/auto_generated/qgsvectorlayerutils.sip.in b/python/core/auto_generated/qgsvectorlayerutils.sip.in
@@ -175,6 +175,17 @@ The optional seed value can be used as a basis for generated values.
 Tests an attribute value to check whether it passes all constraints which are present on the corresponding field.
 Returns ``True`` if the attribute value is valid for the field. Any constraint failures will be reported in the errors argument.
 If the strength or origin parameter is set then only constraints with a matching strength/origin will be checked.
+%End
+
+    static bool canConvert( const QVariant &value, const QVariant::Type destinationType );
+%Docstring
+Tests an attribute ``value`` for type compatibility, i.e. checks whether it can be converted
+to the ``destinationType``.
+NULL values (and QVariant invalid values because they are usually converted to NULLs) are considered valid.
+
+.. seealso:: :py:func:`validateAttribute`
+
+.. versionadded:: 3.14
 %End
 
     static QgsFeature createFeature( const QgsVectorLayer *layer,

diff --git a/src/core/providers/memory/qgsmemoryprovider.cpp b/src/core/providers/memory/qgsmemoryprovider.cpp
@@ -22,6 +22,7 @@
 #include "qgslogger.h"
 #include "qgsspatialindex.h"
 #include "qgscoordinatereferencesystem.h"
+#include "qgsvectorlayerutils.h"
 
 #include <QUrl>
 #include <QUrlQuery>
@@ -381,7 +382,6 @@ bool QgsMemoryProvider::addFeatures( QgsFeatureList &flist, Flags )
 
   int fieldCount = mFields.count();
 
-  // TODO: sanity checks of fields
   for ( QgsFeatureList::iterator it = flist.begin(); it != flist.end(); ++it )
   {
     it->setId( mNextFeatureId );
@@ -419,6 +419,23 @@ bool QgsMemoryProvider::addFeatures( QgsFeatureList &flist, Flags )
       continue;
     }
 
+    // Check attribute types
+    bool conversionError { false };
+    for ( int i = 0; i < mFields.count() && ! conversionError; ++i )
+    {
+      if ( ! QgsVectorLayerUtils::canConvert( it->attribute( i ), mFields.at( i ).type() ) )
+      {
+        pushError( tr( "Could not add feature with attribute %1 having type %2, cannot convert to type %3" )
+                   .arg( mFields.at( i ).name(), it->attribute( i ).typeName(), mFields.at( i ).typeName() ) );
+        result = false;
+      }
+    }
+
+    if ( conversionError )
+    {
+      continue;
+    }
+
     mFeatures.insert( mNextFeatureId, *it );
 
     if ( it->hasGeometry() )

diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp
@@ -35,6 +35,7 @@
 #include "qgsgeometryengine.h"
 #include "qgsproviderregistry.h"
 #include "qgsexpressioncontextutils.h"
+#include "qgsvectorlayerutils.h"
 
 #include <QFile>
 #include <QFileInfo>
@@ -2413,6 +2414,19 @@ gdal::ogr_feature_unique_ptr QgsVectorFileWriter::createFeature( const QgsFeatur
       attrValue = mFieldValueConverter->convert( fldIdx, attrValue );
     }
 
+    // Check for QVariant conversion before passing attribute value to OGR
+    if ( ! QgsVectorLayerUtils::canConvert( attrValue, field.type() ) )
+    {
+      mErrorMessage = QObject::tr( "Invalid variant type for field %1[%2]: received %3 with type %4" )
+                      .arg( mFields.at( fldIdx ).name() )
+                      .arg( ogrField )
+                      .arg( attrValue.typeName(),
+                            attrValue.toString() );
+      QgsMessageLog::logMessage( mErrorMessage, QObject::tr( "OGR" ) );
+      mError = ErrFeatureWriteFailed;
+      return nullptr;
+    }
+
     switch ( field.type() )
     {
       case QVariant::Int:

diff --git a/src/core/qgsvectorlayerutils.cpp b/src/core/qgsvectorlayerutils.cpp
@@ -472,6 +472,18 @@ bool QgsVectorLayerUtils::validateAttribute( const QgsVectorLayer *layer, const
   return valid;
 }
 
+bool QgsVectorLayerUtils::canConvert( const QVariant &value, const QVariant::Type destinationType )
+{
+  if ( value.isNull() )
+  {
+    return true;
+  }
+
+  QVariant converted { value };
+  const bool ok { converted.canConvert( destinationType ) &&converted.convert( destinationType ) };
+  return ok && ( converted.toString() == value.toString() );
+}
+
 QgsFeature QgsVectorLayerUtils::createFeature( const QgsVectorLayer *layer, const QgsGeometry &geometry,
     const QgsAttributeMap &attributes, QgsExpressionContext *context )
 {

diff --git a/src/core/qgsvectorlayerutils.h b/src/core/qgsvectorlayerutils.h
@@ -176,6 +176,16 @@ class CORE_EXPORT QgsVectorLayerUtils
                                    QgsFieldConstraints::ConstraintStrength strength = QgsFieldConstraints::ConstraintStrengthNotSet,
                                    QgsFieldConstraints::ConstraintOrigin origin = QgsFieldConstraints::ConstraintOriginNotSet );
 
+    /**
+     * Tests an attribute \a value for type compatibility, i.e. checks whether it can be converted
+     * to the \a destinationType.
+     * NULL values (and QVariant invalid values because they are usually converted to NULLs) are considered valid.
+     *
+     * \see validateAttribute()
+     * \since QGIS 3.14
+     */
+    static bool canConvert( const QVariant &value, const QVariant::Type destinationType );
+
     /**
      * Creates a new feature ready for insertion into a layer. Default values and constraints
      * (e.g., unique constraints) will automatically be handled. An optional attribute map can be

diff --git a/tests/src/python/test_provider_memory.py b/tests/src/python/test_provider_memory.py
@@ -692,6 +692,22 @@ def testClone(self):
         self.assertEqual(len(parse_qs(vl2.publicSource())['uid']), 1)
         self.assertNotEqual(parse_qs(vl2.publicSource())['uid'][0], parse_qs(vl.publicSource())['uid'][0])
 
+    def testTypeValidation(self):
+        """Test that incompatible types in attributes raise errors"""
+
+        vl = QgsVectorLayer(
+            'Point?crs=epsg:4326&field=int:integer',
+            'test', 'memory')
+
+        self.assertTrue(vl.isValid())
+        f = QgsFeature(vl.fields())
+        f.setAttribute('int', 'A string')
+        f.setGeometry(QgsGeometry.fromWkt('point(9 45)'))
+        self.assertTrue(vl.startEditing())
+        # Validation happens on commit
+        self.assertTrue(vl.addFeatures([f]))
+        self.assertFalse(vl.commitChanges())
+
 
 class TestPyQgsMemoryProviderIndexed(unittest.TestCase, ProviderTestCase):
     """Runs the provider test suite against an indexed memory layer"""

diff --git a/tests/src/python/test_qgsvectorfilewriter.py b/tests/src/python/test_qgsvectorfilewriter.py
@@ -1237,6 +1237,36 @@ def testWriteTriangle(self):
         self.assertEqual(f.geometry().asWkt(), 'MultiPolygonZ (((1 2 3, 2 2 4, 2 3 4, 1 2 3)))')
         self.assertEqual(f.attributes(), [1, 'Johny'])
 
+    def testWriteConversionErrors(self):
+        """Test writing features with attribute values that cannot be
+        converted to the destination fields.
+        See: GH #36715"""
+
+        vl = QgsVectorLayer('Point?crs=epsg:4326&field=int:integer', 'test', 'memory')
+        self.assertTrue(vl.startEditing())
+        f = QgsFeature(vl.fields())
+        f.setGeometry(QgsGeometry.fromWkt('point(9 45)'))
+        f.setAttribute(0, 'QGIS Rocks!')  # not valid!
+        self.assertTrue(vl.addFeatures([f]))
+        f.setAttribute(0, 12345)  # valid!
+        self.assertTrue(vl.addFeatures([f]))
+
+        dest_file_name = os.path.join(str(QDir.tempPath()), 'writer_conversion_errors.shp')
+        write_result, error_message = QgsVectorFileWriter.writeAsVectorFormat(
+            vl,
+            dest_file_name,
+            'utf-8',
+            QgsCoordinateReferenceSystem(),
+            'ESRI Shapefile')
+        self.assertEqual(write_result, QgsVectorFileWriter.ErrFeatureWriteFailed, error_message)
+
+        # Open result and check
+        created_layer = QgsVectorLayer('{}|layerid=0'.format(dest_file_name), 'test', 'ogr')
+        self.assertEqual(created_layer.fields().count(), 1)
+        self.assertEqual(created_layer.featureCount(), 1)
+        f = next(created_layer.getFeatures(QgsFeatureRequest()))
+        self.assertEqual(f['int'], 12345)
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/src/python/test_qgsvectorlayerutils.py b/tests/src/python/test_qgsvectorlayerutils.py
@@ -15,7 +15,7 @@
 import shutil
 import tempfile
 
-from qgis.PyQt.QtCore import QVariant
+from qgis.PyQt.QtCore import QVariant, QDate
 from qgis.core import (QgsProject,
                        QgsVectorLayer,
                        QgsVectorLayerUtils,
@@ -690,6 +690,49 @@ def test_unique_pk_when_subset(self):
         vl.addFeatures(features)
         self.assertTrue(vl.commitChanges())
 
+    def test_check_attribute_type(self):
+        """Test checkAttributeType"""
+
+        vl = QgsVectorLayer('Point?crs=epsg:4326&field=int:integer', 'test', 'memory')
+
+        # Valid values
+        self.assertTrue(QgsVectorLayerUtils.canConvert(123.0, vl.fields()[0].type()))
+        self.assertTrue(QgsVectorLayerUtils.canConvert(123, vl.fields()[0].type()))
+        # Check NULL/invalid
+        self.assertTrue(QgsVectorLayerUtils.canConvert(None, vl.fields()[0].type()))
+        self.assertTrue(QgsVectorLayerUtils.canConvert(QVariant(QVariant.Int), vl.fields()[0].type()))
+        # Not valid
+        self.assertFalse(QgsVectorLayerUtils.canConvert('QGIS Rocks!', vl.fields()[0].type()))
+        self.assertFalse(QgsVectorLayerUtils.canConvert(QDate(2020, 6, 30), vl.fields()[0].type()))
+        # Not valid: overflow and narrow cast!
+        self.assertFalse(QgsVectorLayerUtils.canConvert(2147483647 + 1, vl.fields()[0].type()))
+        self.assertFalse(QgsVectorLayerUtils.canConvert(123.123, vl.fields()[0].type()))
+
+        vl = QgsVectorLayer('Point?crs=epsg:4326&field=date:date', 'test', 'memory')
+        self.assertTrue(QgsVectorLayerUtils.canConvert(QDate(2020, 6, 30), vl.fields()[0].type()))
+        # Not valid
+        self.assertFalse(QgsVectorLayerUtils.canConvert('QGIS Rocks!', vl.fields()[0].type()))
+        self.assertFalse(QgsVectorLayerUtils.canConvert(123, vl.fields()[0].type()))
+
+        # Strings can store almost anything
+        vl = QgsVectorLayer('Point?crs=epsg:4326&field=text:text', 'test', 'memory')
+        self.assertTrue(QgsVectorLayerUtils.canConvert(QDate(2020, 6, 30), vl.fields()[0].type()))
+        self.assertTrue(QgsVectorLayerUtils.canConvert('QGIS Rocks!', vl.fields()[0].type()))
+        self.assertTrue(QgsVectorLayerUtils.canConvert(123, vl.fields()[0].type()))
+        self.assertTrue(QgsVectorLayerUtils.canConvert(123.456, vl.fields()[0].type()))
+
+        vl = QgsVectorLayer('Point?crs=epsg:4326&field=double:double', 'test', 'memory')
+
+        # Valid values
+        self.assertTrue(QgsVectorLayerUtils.canConvert(123.0, vl.fields()[0].type()))
+        self.assertTrue(QgsVectorLayerUtils.canConvert(123, vl.fields()[0].type()))
+        # Check NULL/invalid
+        self.assertTrue(QgsVectorLayerUtils.canConvert(None, vl.fields()[0].type()))
+        self.assertTrue(QgsVectorLayerUtils.canConvert(QVariant.Double, vl.fields()[0].type()))
+        # Not valid
+        self.assertFalse(QgsVectorLayerUtils.canConvert('QGIS Rocks!', vl.fields()[0].type()))
+        self.assertFalse(QgsVectorLayerUtils.canConvert(QDate(2020, 6, 30), vl.fields()[0].type()))
+
 
 if __name__ == '__main__':
     unittest.main()