Skip to content
Permalink
Browse files

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.
  • Loading branch information
elpaso authored and nyalldawson committed Jun 19, 2020
1 parent 9163100 commit 68baf74906f3f511f2da7b93a2317b3512375201
@@ -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,
@@ -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() )
@@ -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:
@@ -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 )
{
@@ -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
@@ -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"""
@@ -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()
@@ -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()

0 comments on commit 68baf74

Please sign in to comment.
You can’t perform that action at this time.