Skip to content

Commit

Permalink
Provide more descriptive error messages when converting a field value
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@nyalldawson
nyalldawson committed Jul 22, 2020
1 parent c912564 commit aa93872
Show file tree
Hide file tree
Showing 8 changed files with 157 additions and 60 deletions.
28 changes: 17 additions & 11 deletions python/core/auto_generated/qgsfield.sip.in
View file
Expand Up @@ -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
Expand All @@ -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 ( ... )
{
Expand All @@ -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;
Expand Down
10 changes: 6 additions & 4 deletions src/app/qgisapp.cpp
View file
Expand Up @@ -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 );
}
}
Expand Down Expand Up @@ -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 );
Expand All @@ -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;
Expand Down
14 changes: 8 additions & 6 deletions src/core/providers/memory/qgsmemoryprovider.cpp
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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() );
Expand All @@ -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;
Expand Down
33 changes: 32 additions & 1 deletion src/core/qgsfield.cpp
View file
Expand Up @@ -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 );
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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 ) ) );
Expand All @@ -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;
}

Expand All @@ -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 ) ) );
Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down
41 changes: 29 additions & 12 deletions src/core/qgsfield.h
View file
Expand Up @@ -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
*
Expand All @@ -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;

Expand All @@ -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 ( ... )
{
Expand All @@ -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

Expand Down
9 changes: 4 additions & 5 deletions src/core/qgsvectorfilewriter.cpp
View file
Expand Up @@ -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;
Expand Down

0 comments on commit aa93872

Please sign in to comment.