Provide more descriptive error messages when converting a field value

fails

And fix Python exception handling in QgsField::convertCompatible to
avoid Python "returned an object with the error set" error message
and instead just use the proper ValueError exception with a descriptive
exception message
This commit is contained in:
Nyall Dawson 2020-07-22 17:29:26 +10:00
parent c912564452
commit aa93872f0d
8 changed files with 157 additions and 60 deletions

View File

@ -288,6 +288,7 @@ Sets the alias for the field (the friendly displayed name of the field ).
Formats string for display
%End
bool convertCompatible( QVariant &v ) const;
%Docstring
Converts the provided variant to a compatible format
@ -307,11 +308,12 @@ Converts the provided variant to a compatible format
if ( sipParseArgs( &sipParseErr, sipArgs, "BJ1", &sipSelf, sipType_QgsField, &sipCpp, sipType_QVariant, &a0, &a0State ) )
{
bool sipRes;
QString errorMessage;
Py_BEGIN_ALLOW_THREADS
try
{
sipRes = sipCpp->convertCompatible( *a0 );
sipRes = sipCpp->convertCompatible( *a0, &errorMessage );
}
catch ( ... )
{
@ -324,24 +326,28 @@ Converts the provided variant to a compatible format
Py_END_ALLOW_THREADS
PyObject *res = sipConvertFromType( a0, sipType_QVariant, NULL );
sipReleaseType( a0, sipType_QVariant, a0State );
if ( !sipRes )
{
PyErr_SetString( PyExc_ValueError,
QString( "Value %1 (%2) could not be converted to field type %3." ).arg( a0->toString(), a0->typeName() ).arg( sipCpp->type() ).toUtf8().constData() );
sipError = sipErrorFail;
QString( "Value could not be converted to field type %1: %2" ).arg( QMetaType::typeName( sipCpp->type() ), errorMessage ).toUtf8().constData() );
sipIsErr = 1;
}
else
{
PyObject *res = sipConvertFromType( a0, sipType_QVariant, NULL );
sipReleaseType( a0, sipType_QVariant, a0State );
return res;
}
}
else
{
// Raise an exception if the arguments couldn't be parsed.
sipNoMethod( sipParseErr, sipName_QgsField, sipName_convertCompatible, doc_QgsField_convertCompatible );
return res;
return nullptr;
}
}
// Raise an exception if the arguments couldn't be parsed.
sipNoMethod( sipParseErr, sipName_QgsField, sipName_convertCompatible, doc_QgsField_convertCompatible );
return nullptr;
%End
operator QVariant() const;

View File

@ -9690,14 +9690,15 @@ void QgisApp::mergeAttributesOfSelectedFeatures()
vl->dataProvider()->defaultValueClause( vl->fields().fieldOriginIndex( i ) ) == val;
// convert to destination data type
if ( !isDefaultValue && !fld.convertCompatible( val ) )
QString errorMessage;
if ( !isDefaultValue && !fld.convertCompatible( val, &errorMessage ) )
{
if ( firstFeature )
{
//only warn on first feature
visibleMessageBar()->pushMessage(
tr( "Invalid result" ),
tr( "Could not store value '%1' in field of type %2" ).arg( merged.at( i ).toString(), fld.typeName() ),
tr( "Could not store value '%1' in field of type %2: %3" ).arg( merged.at( i ).toString(), fld.typeName(), errorMessage ),
Qgis::Warning );
}
}
@ -9862,6 +9863,7 @@ void QgisApp::mergeSelectedFeatures()
newFeature.setGeometry( unionGeom );
QgsAttributes attrs = d.mergedAttributes();
QString errorMessage;
for ( int i = 0; i < attrs.count(); ++i )
{
QVariant val = attrs.at( i );
@ -9870,11 +9872,11 @@ void QgisApp::mergeSelectedFeatures()
vl->dataProvider()->defaultValueClause( vl->fields().fieldOriginIndex( i ) ) == val;
// convert to destination data type
if ( !isDefaultValue && !vl->fields().at( i ).convertCompatible( val ) )
if ( !isDefaultValue && !vl->fields().at( i ).convertCompatible( val, &errorMessage ) )
{
visibleMessageBar()->pushMessage(
tr( "Invalid result" ),
tr( "Could not store value '%1' in field of type %2." ).arg( attrs.at( i ).toString(), vl->fields().at( i ).typeName() ),
tr( "Could not store value '%1' in field of type %2: %3" ).arg( attrs.at( i ).toString(), vl->fields().at( i ).typeName(), errorMessage ),
Qgis::Warning );
}
attrs[i] = val;

View File

@ -425,16 +425,17 @@ bool QgsMemoryProvider::addFeatures( QgsFeatureList &flist, Flags flags )
// Check attribute conversion
bool conversionError { false };
QString errorMessage;
for ( int i = 0; i < mFields.count(); ++i )
{
QVariant attrValue { it->attribute( i ) };
if ( ! attrValue.isNull() && ! mFields.at( i ).convertCompatible( attrValue ) )
if ( ! attrValue.isNull() && ! mFields.at( i ).convertCompatible( attrValue, &errorMessage ) )
{
// Push first conversion error only
if ( result )
{
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() ) );
pushError( tr( "Could not store attribute \"%1\": %2" )
.arg( mFields.at( i ).name(), errorMessage ) );
}
result = false;
conversionError = true;
@ -598,6 +599,7 @@ bool QgsMemoryProvider::changeAttributeValues( const QgsChangedAttributesMap &at
QgsChangedAttributesMap rollBackMap;
QString errorMessage;
for ( QgsChangedAttributesMap::const_iterator it = attr_map.begin(); it != attr_map.end(); ++it )
{
QgsFeatureMap::iterator fit = mFeatures.find( it.key() );
@ -613,15 +615,15 @@ bool QgsMemoryProvider::changeAttributeValues( const QgsChangedAttributesMap &at
QVariant attrValue { it2.value() };
// Check attribute conversion
const bool conversionError { ! attrValue.isNull()
&& ! mFields.at( it2.key() ).convertCompatible( attrValue ) };
&& ! mFields.at( it2.key() ).convertCompatible( attrValue, &errorMessage ) };
if ( conversionError )
{
// Push first conversion error only
if ( result )
{
pushError( tr( "Could not change attribute %1 having type %2 for feature %4, cannot convert to type %3" )
pushError( tr( "Could not change attribute %1 having type %2 for feature %4: %3" )
.arg( mFields.at( it2.key() ).name(), it2.value( ).typeName(),
mFields.at( it2.key() ).typeName() ).arg( it.key() ) );
errorMessage ).arg( it.key() ) );
}
result = false;
break;

View File

@ -336,8 +336,12 @@ QString QgsField::displayString( const QVariant &v ) const
* See details in QEP #17
****************************************************************************/
bool QgsField::convertCompatible( QVariant &v ) const
bool QgsField::convertCompatible( QVariant &v, QString *errorMessage ) const
{
const QVariant original = v;
if ( errorMessage )
errorMessage->clear();
if ( v.isNull() )
{
v.convert( d->type );
@ -347,6 +351,8 @@ bool QgsField::convertCompatible( QVariant &v ) const
if ( d->type == QVariant::Int && v.toInt() != v.toLongLong() )
{
v = QVariant( d->type );
if ( errorMessage )
*errorMessage = QObject::tr( "Value \"%1\" is too large for integer field" ).arg( original.toLongLong() );
return false;
}
@ -422,6 +428,10 @@ bool QgsField::convertCompatible( QVariant &v ) const
{
//couldn't convert to number
v = QVariant( d->type );
if ( errorMessage )
*errorMessage = QObject::tr( "Value \"%1\" is not a number" ).arg( original.toString() );
return false;
}
@ -430,6 +440,10 @@ bool QgsField::convertCompatible( QVariant &v ) const
{
//double too large to fit in int
v = QVariant( d->type );
if ( errorMessage )
*errorMessage = QObject::tr( "Value \"%1\" is too large for integer field" ).arg( original.toDouble() );
return false;
}
v = QVariant( static_cast< int >( std::round( dbl ) ) );
@ -450,6 +464,10 @@ bool QgsField::convertCompatible( QVariant &v ) const
{
//couldn't convert to number
v = QVariant( d->type );
if ( errorMessage )
*errorMessage = QObject::tr( "Value \"%1\" is not a number" ).arg( original.toString() );
return false;
}
@ -458,6 +476,10 @@ bool QgsField::convertCompatible( QVariant &v ) const
{
//double too large to fit in longlong
v = QVariant( d->type );
if ( errorMessage )
*errorMessage = QObject::tr( "Value \"%1\" is too large for long long field" ).arg( original.toDouble() );
return false;
}
v = QVariant( static_cast< long long >( std::round( dbl ) ) );
@ -468,6 +490,10 @@ bool QgsField::convertCompatible( QVariant &v ) const
if ( !v.convert( d->type ) )
{
v = QVariant( d->type );
if ( errorMessage )
*errorMessage = QObject::tr( "Could not convert value \"%1\" to target type" ).arg( original.toString() );
return false;
}
@ -481,7 +507,12 @@ bool QgsField::convertCompatible( QVariant &v ) const
if ( d->type == QVariant::String && d->length > 0 && v.toString().length() > d->length )
{
const int length = v.toString().length();
v = v.toString().left( d->length );
if ( errorMessage )
*errorMessage = QObject::tr( "String of length %1 exceeds maximum field length (%2)" ).arg( length ).arg( d->length );
return false;
}

View File

@ -285,6 +285,19 @@ class CORE_EXPORT QgsField
//! Formats string for display
QString displayString( const QVariant &v ) const;
#ifndef SIP_RUN
/**
* Converts the provided variant to a compatible format
*
* \param v The value to convert
* \param errorMessage if specified, will be set to a descriptive error when a conversion failure occurs
*
* \returns TRUE if the conversion was successful
*/
bool convertCompatible( QVariant &v, QString *errorMessage = nullptr ) const;
#else
/**
* Converts the provided variant to a compatible format
*
@ -293,7 +306,6 @@ class CORE_EXPORT QgsField
* \returns TRUE if the conversion was successful
*/
bool convertCompatible( QVariant &v ) const;
#ifdef SIP_RUN
% MethodCode
PyObject *sipParseErr = NULL;
@ -305,11 +317,12 @@ class CORE_EXPORT QgsField
if ( sipParseArgs( &sipParseErr, sipArgs, "BJ1", &sipSelf, sipType_QgsField, &sipCpp, sipType_QVariant, &a0, &a0State ) )
{
bool sipRes;
QString errorMessage;
Py_BEGIN_ALLOW_THREADS
try
{
sipRes = sipCpp->convertCompatible( *a0 );
sipRes = sipCpp->convertCompatible( *a0, &errorMessage );
}
catch ( ... )
{
@ -322,24 +335,28 @@ class CORE_EXPORT QgsField
Py_END_ALLOW_THREADS
PyObject *res = sipConvertFromType( a0, sipType_QVariant, NULL );
sipReleaseType( a0, sipType_QVariant, a0State );
if ( !sipRes )
{
PyErr_SetString( PyExc_ValueError,
QString( "Value %1 (%2) could not be converted to field type %3." ).arg( a0->toString(), a0->typeName() ).arg( sipCpp->type() ).toUtf8().constData() );
sipError = sipErrorFail;
QString( "Value could not be converted to field type %1: %2" ).arg( QMetaType::typeName( sipCpp->type() ), errorMessage ).toUtf8().constData() );
sipIsErr = 1;
}
else
{
PyObject *res = sipConvertFromType( a0, sipType_QVariant, NULL );
sipReleaseType( a0, sipType_QVariant, a0State );
return res;
}
}
else
{
// Raise an exception if the arguments couldn't be parsed.
sipNoMethod( sipParseErr, sipName_QgsField, sipName_convertCompatible, doc_QgsField_convertCompatible );
return res;
return nullptr;
}
}
// Raise an exception if the arguments couldn't be parsed.
sipNoMethod( sipParseErr, sipName_QgsField, sipName_convertCompatible, doc_QgsField_convertCompatible );
return nullptr;
% End
#endif

View File

@ -2419,13 +2419,12 @@ gdal::ogr_feature_unique_ptr QgsVectorFileWriter::createFeature( const QgsFeatur
}
// Check type compatibility before passing attribute value to OGR
if ( ! field.convertCompatible( attrValue ) )
QString errorMessage;
if ( ! field.convertCompatible( attrValue, &errorMessage ) )
{
mErrorMessage = QObject::tr( "Error converting value (%1) from %2 to %3 for attribute field %4" )
mErrorMessage = QObject::tr( "Error converting value (%1) for attribute field %2: %3" )
.arg( feature.attribute( fldIdx ).toString(),
mFields.at( fldIdx ).typeName(),
attrValue.typeName(),
mFields.at( fldIdx ).name() );
mFields.at( fldIdx ).name(), errorMessage );
QgsMessageLog::logMessage( mErrorMessage, QObject::tr( "OGR" ) );
mError = ErrFeatureWriteFailed;
return nullptr;

View File

@ -478,11 +478,16 @@ void TestQgsField::convertCompatible()
QgsField doubleField( QStringLiteral( "double" ), QVariant::Double, QStringLiteral( "double" ) );
stringVar = QVariant( "test string" );
QVERIFY( !doubleField.convertCompatible( stringVar ) );
QString error;
QVERIFY( !doubleField.convertCompatible( stringVar, &error ) );
QCOMPARE( stringVar.type(), QVariant::Double );
QCOMPARE( error, QStringLiteral( "Could not convert value \"test string\" to target type" ) );
stringVar = QVariant( "test string" );
QVERIFY( !doubleField.convertCompatible( stringVar ) );
QVERIFY( stringVar.isNull() );
nullString = QVariant( QVariant::String );
QVERIFY( doubleField.convertCompatible( nullString ) );
QVERIFY( doubleField.convertCompatible( nullString, &error ) );
QVERIFY( error.isEmpty() );
QCOMPARE( nullString.type(), QVariant::Double );
QVERIFY( nullString.isNull() );
intVar = QVariant( 5 );
@ -517,12 +522,18 @@ void TestQgsField::convertCompatible()
QCOMPARE( negativeSmallDouble, QVariant( -9346 ) );
//large double, cannot be converted
QVariant largeDouble( 9999999999.99 );
QVERIFY( !intField.convertCompatible( largeDouble, &error ) );
QCOMPARE( error, QStringLiteral( "Value \"10000000000\" is too large for integer field" ) );
largeDouble = QVariant( 9999999999.99 );
QVERIFY( !intField.convertCompatible( largeDouble ) );
QCOMPARE( largeDouble.type(), QVariant::Int );
QVERIFY( largeDouble.isNull() );
//conversion of string double value to int
QVariant notNumberString( "notanumber" );
QVERIFY( !intField.convertCompatible( notNumberString, &error ) );
QCOMPARE( error, QStringLiteral( "Value \"notanumber\" is not a number" ) );
notNumberString = QVariant( "notanumber" );
QVERIFY( !intField.convertCompatible( notNumberString ) );
QCOMPARE( notNumberString.type(), QVariant::Int );
QVERIFY( notNumberString.isNull() );
@ -537,13 +548,17 @@ void TestQgsField::convertCompatible()
QCOMPARE( negativeSmallDoubleString, QVariant( -9346 ) );
//large double, cannot be converted
QVariant largeDoubleString( "9999999999.99" );
QVERIFY( !intField.convertCompatible( largeDoubleString, &error ) );
QCOMPARE( error, QStringLiteral( "Value \"1e+10\" is too large for integer field" ) );
largeDoubleString = QVariant( "9999999999.99" );
QVERIFY( !intField.convertCompatible( largeDoubleString ) );
QCOMPARE( largeDoubleString.type(), QVariant::Int );
QVERIFY( largeDoubleString.isNull() );
//conversion of longlong to int
QVariant longlong( 99999999999999999LL );
QVERIFY( !intField.convertCompatible( longlong ) );
QVERIFY( !intField.convertCompatible( longlong, &error ) );
QCOMPARE( error, QStringLiteral( "Value \"99999999999999999\" is too large for integer field" ) );
QCOMPARE( longlong.type(), QVariant::Int );
QVERIFY( longlong.isNull() );
QVariant smallLonglong( 99LL );
@ -552,7 +567,8 @@ void TestQgsField::convertCompatible()
QCOMPARE( smallLonglong, QVariant( 99 ) );
// negative longlong to int
QVariant negativeLonglong( -99999999999999999LL );
QVERIFY( !intField.convertCompatible( negativeLonglong ) );
QVERIFY( !intField.convertCompatible( negativeLonglong, &error ) );
QCOMPARE( error, QStringLiteral( "Value \"-99999999999999999\" is too large for integer field" ) );
QCOMPARE( negativeLonglong.type(), QVariant::Int );
QVERIFY( negativeLonglong.isNull() );
// small negative longlong to int
@ -592,7 +608,8 @@ void TestQgsField::convertCompatible()
//conversion of string double value to longlong
notNumberString = QVariant( "notanumber" );
QVERIFY( !longlongField.convertCompatible( notNumberString ) );
QVERIFY( !longlongField.convertCompatible( notNumberString, &error ) );
QCOMPARE( error, QStringLiteral( "Value \"notanumber\" is not a number" ) );
QCOMPARE( notNumberString.type(), QVariant::LongLong );
QVERIFY( notNumberString.isNull() );
//small double, should be rounded
@ -611,7 +628,8 @@ void TestQgsField::convertCompatible()
QCOMPARE( largeDoubleString, QVariant( 10000000000LL ) );
//extra large double, cannot be converted
largeDoubleString = QVariant( "999999999999999999999.99" );
QVERIFY( !longlongField.convertCompatible( largeDoubleString ) );
QVERIFY( !longlongField.convertCompatible( largeDoubleString, &error ) );
QCOMPARE( error, QStringLiteral( "Value \"1e+21\" is too large for long long field" ) );
QCOMPARE( largeDoubleString.type(), QVariant::LongLong );
QVERIFY( largeDoubleString.isNull() );
@ -627,8 +645,8 @@ void TestQgsField::convertCompatible()
QCOMPARE( stringDouble, QVariant( 1223456.012345 ) );
// This should not convert
stringDouble = QVariant( "1.223.456,012345" );
QVERIFY( ! doubleField.convertCompatible( stringDouble ) );
QVERIFY( ! doubleField.convertCompatible( stringDouble, &error ) );
QCOMPARE( error, QStringLiteral( "Could not convert value \"1.223.456,012345\" to target type" ) );
//double with precision
QgsField doubleWithPrecField( QStringLiteral( "double" ), QVariant::Double, QStringLiteral( "double" ), 10, 3 );
@ -641,7 +659,8 @@ void TestQgsField::convertCompatible()
//truncating string length
QgsField stringWithLen( QStringLiteral( "string" ), QVariant::String, QStringLiteral( "string" ), 3 );
stringVar = QVariant( "longstring" );
QVERIFY( !stringWithLen.convertCompatible( stringVar ) );
QVERIFY( !stringWithLen.convertCompatible( stringVar, &error ) );
QCOMPARE( error, QStringLiteral( "String of length 10 exceeds maximum field length (3)" ) );
QCOMPARE( stringVar.type(), QVariant::String );
QCOMPARE( stringVar.toString(), QString( "lon" ) );

View File

@ -92,10 +92,10 @@ class TestQgsFields(unittest.TestCase):
def test_names(self):
ml = QgsVectorLayer(
"Point?crs=epsg:4236" +
"&field=id:integer" +
"&field=value:double" +
"&field=crazy:double",
"Point?crs=epsg:4236"
+ "&field=id:integer"
+ "&field=value:double"
+ "&field=crazy:double",
"test_data",
"memory")
@ -121,30 +121,47 @@ class TestQgsFields(unittest.TestCase):
self.assertIsNone(vl.fields()[0].convertCompatible(None))
self.assertEqual(vl.fields()[0].convertCompatible(QVariant(QVariant.Int)), NULL)
# Not valid
with self.assertRaises(Exception):
with self.assertRaises(ValueError) as cm:
vl.fields()[0].convertCompatible('QGIS Rocks!')
with self.assertRaises(Exception):
self.assertEqual(str(cm.exception), 'Value could not be converted to field type int: Value "QGIS Rocks!" is not a number')
with self.assertRaises(ValueError) as cm:
self.assertFalse(vl.fields()[0].convertCompatible(QDate(2020, 6, 30)))
self.assertEqual(str(cm.exception),
'Value could not be converted to field type int: Could not convert value "2020-06-30" to target type')
# Not valid: overflow
with self.assertRaises(Exception):
with self.assertRaises(ValueError) as cm:
self.assertFalse(vl.fields()[0].convertCompatible(2147483647 + 1))
self.assertEqual(str(cm.exception),
'Value could not be converted to field type int: Value "2147483648" is too large for integer field')
# Valid: narrow cast with loss of precision (!)
self.assertTrue(vl.fields()[0].convertCompatible(123.123))
vl = QgsVectorLayer('Point?crs=epsg:4326&field=date:date', 'test', 'memory')
self.assertTrue(vl.fields()[0].convertCompatible(QDate(2020, 6, 30)))
# Not valid
with self.assertRaises(Exception):
with self.assertRaises(ValueError) as cm:
self.assertFalse(vl.fields()[0].convertCompatible('QGIS Rocks!'))
with self.assertRaises(Exception):
self.assertEqual(str(cm.exception),
'Value could not be converted to field type QDate: Could not convert value "QGIS Rocks!" to target type')
with self.assertRaises(ValueError) as cm:
self.assertFalse(vl.fields()[0].convertCompatible(123))
self.assertEqual(str(cm.exception),
'Value could not be converted to field type QDate: Could not convert value "123" to target type')
# Strings can store almost anything
vl = QgsVectorLayer('Point?crs=epsg:4326&field=text:text', 'test', 'memory')
vl = QgsVectorLayer('Point?crs=epsg:4326&field=text:string(30)', 'test', 'memory')
self.assertTrue(vl.fields()[0].convertCompatible(QDate(2020, 6, 30)))
self.assertTrue(vl.fields()[0].convertCompatible('QGIS Rocks!'))
self.assertTrue(vl.fields()[0].convertCompatible(123))
self.assertTrue(vl.fields()[0].convertCompatible(123.456))
# string overflow
self.assertEqual(vl.fields()[0].length(), 30)
with self.assertRaises(ValueError) as cm:
self.assertTrue(vl.fields()[0].convertCompatible('x' * 31))
self.assertEqual(str(cm.exception),
'Value could not be converted to field type QString: String of length 31 exceeds maximum field length (30)')
vl = QgsVectorLayer('Point?crs=epsg:4326&field=double:double', 'test', 'memory')
@ -156,10 +173,14 @@ class TestQgsFields(unittest.TestCase):
self.assertEqual(vl.fields()[0].convertCompatible(NULL), NULL)
self.assertTrue(vl.fields()[0].convertCompatible(QVariant.Double))
# Not valid
with self.assertRaises(Exception):
with self.assertRaises(ValueError) as cm:
self.assertFalse(vl.fields()[0].convertCompatible('QGIS Rocks!'))
with self.assertRaises(Exception):
self.assertEqual(str(cm.exception),
'Value could not be converted to field type double: Could not convert value "QGIS Rocks!" to target type')
with self.assertRaises(ValueError) as cm:
self.assertFalse(vl.fields()[0].convertCompatible(QDate(2020, 6, 30)))
self.assertEqual(str(cm.exception),
'Value could not be converted to field type double: Could not convert value "2020-06-30" to target type')
if __name__ == '__main__':