Skip to content
Permalink
Browse files

Memory provider: roll back on errors

Long story short: calling provider's addFeatures
is implemented for some providers in a way that
will roll back all changes on errors, leaving
the backend storage unchanged.

Adding a QgsFeatureSink flag to control this
behavior allows certain providers to support
partial feature addition.

The issue comes from QgsVectorDataProvider::commitChanges
that is documented to leave the provider unchanged (roll
back) on any error, giving the client code the possibility
to fix errors (in the editing buffer) and re-commit.

Without a full rollback implementation in the memory
provider and after the type check introduction in this
PR we ended up with situations like this:

vl = ... an empty memory layer
self.assertTrue(vl.addFeatures([valid, invalid]))
self.assertFalse(vl.commitChanges())
self.assertEqual(vl.featureCount(), 1)  <--- fails!
We actually had 3 features from vl.getFeatures():
[valid, invalid, valid] (the first from the provider
the second and third from the editing buffer).

On the other hand, QgsFeatureSink would probably assume
that addFeatures will allow partial additions.

BTW: This is for sure the longest commit message I've ever
     written.
  • Loading branch information
elpaso authored and nyalldawson committed Jun 19, 2020
1 parent 64675ca commit 7fa6f386893318cb3e94a7dacff2daa07c5dc56d
@@ -34,6 +34,8 @@ An interface for objects which accept features via addFeature(s) methods.
{

FastInsert,

RollBackOnErrors,
};
typedef QFlags<QgsFeatureSink::Flag> Flags;

@@ -22,7 +22,6 @@
#include "qgslogger.h"
#include "qgsspatialindex.h"
#include "qgscoordinatereferencesystem.h"
#include "qgsvectorlayerutils.h"

#include <QUrl>
#include <QUrlQuery>
@@ -373,16 +372,21 @@ void QgsMemoryProvider::handlePostCloneOperations( QgsVectorDataProvider *source
}
}


bool QgsMemoryProvider::addFeatures( QgsFeatureList &flist, Flags )
// returns TRUE if all features were added successfully, or FALSE if any feature could not be added
bool QgsMemoryProvider::addFeatures( QgsFeatureList &flist, Flags flags )
{
bool result = true;
// whether or not to update the layer extent on the fly as we add features
bool updateExtent = mFeatures.isEmpty() || !mExtent.isEmpty();

int fieldCount = mFields.count();

for ( QgsFeatureList::iterator it = flist.begin(); it != flist.end(); ++it )
// For rollback
const auto oldExtent { mExtent };
const auto oldNextFeatureId { mNextFeatureId };
QgsFeatureIds addedFids ;

for ( QgsFeatureList::iterator it = flist.begin(); it != flist.end() && result ; ++it )
{
it->setId( mNextFeatureId );
it->setValid( true );
@@ -421,23 +425,31 @@ bool QgsMemoryProvider::addFeatures( QgsFeatureList &flist, Flags )

// Check attribute conversion
bool conversionError { false };
for ( int i = 0; i < mFields.count() && ! conversionError; ++i )
for ( int i = 0; i < mFields.count(); ++i )
{
QVariant attrValue { it->attribute( i ) };
if ( ! attrValue.isNull() && ! mFields.at( i ).convertCompatible( attrValue ) )
{
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() ) );
// 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() ) );
}
result = false;
conversionError = true;
continue;
}
}

// Skip the feature if there is at least one conversion error
if ( conversionError )
{
continue;
}

mFeatures.insert( mNextFeatureId, *it );
addedFids.insert( mNextFeatureId );

if ( it->hasGeometry() )
{
@@ -452,7 +464,21 @@ bool QgsMemoryProvider::addFeatures( QgsFeatureList &flist, Flags )
mNextFeatureId++;
}

clearMinMaxCache();
// Roll back
if ( ! result && flags.testFlag( QgsFeatureSink::Flag::RollBackOnErrors ) )
{
for ( const QgsFeatureId &addedFid : addedFids )
{
mFeatures.remove( addedFid );
}
mExtent = oldExtent;
mNextFeatureId = oldNextFeatureId;
}
else
{
clearMinMaxCache();
}

return result;
}

@@ -564,18 +590,54 @@ bool QgsMemoryProvider::deleteAttributes( const QgsAttributeIds &attributes )

bool QgsMemoryProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_map )
{
bool result { true };

QgsChangedAttributesMap rollBackMap;

for ( QgsChangedAttributesMap::const_iterator it = attr_map.begin(); it != attr_map.end(); ++it )
{
QgsFeatureMap::iterator fit = mFeatures.find( it.key() );
if ( fit == mFeatures.end() )
continue;

const QgsAttributeMap &attrs = it.value();
QgsAttributeMap rollBackAttrs;

// Break on errors
for ( QgsAttributeMap::const_iterator it2 = attrs.constBegin(); it2 != attrs.constEnd(); ++it2 )
{
QVariant attrValue { it2.value() };
// Check attribute conversion
const bool conversionError { ! attrValue.isNull()
&& ! mFields.at( it2.key() ).convertCompatible( attrValue ) };
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" )
.arg( mFields.at( it2.key() ).name(), it2.value( ).typeName(),
mFields.at( it2.key() ).typeName() ).arg( it.key() ) );
}
result = false;
break;
}
rollBackAttrs.insert( it2.key(), fit->attribute( it2.key() ) );
fit->setAttribute( it2.key(), it2.value() );
}
rollBackMap.insert( it.key(), rollBackAttrs );
}
clearMinMaxCache();
return true;

// Roll back
if ( ! result )
{
changeAttributeValues( rollBackMap );
}
else
{
clearMinMaxCache();
}
return result;
}

bool QgsMemoryProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
@@ -68,6 +68,14 @@ class CORE_EXPORT QgsFeatureSink
* skipping this update represents a significant speed boost for the operation.
*/
FastInsert = 1 << 1,

/**
* Roll back the whole transaction if a single add feature operation fails.
* Individual sink subclasses may choose to ignore this flag and always roll back
* while other providers will respect the flag and accept partial additions if
* this flag is not set.
*/
RollBackOnErrors = 1 << 2,
};
Q_DECLARE_FLAGS( Flags, Flag )

@@ -35,7 +35,6 @@
#include "qgsgeometryengine.h"
#include "qgsproviderregistry.h"
#include "qgsexpressioncontextutils.h"
#include "qgsvectorlayerutils.h"

#include <QFile>
#include <QFileInfo>
@@ -628,7 +628,7 @@ bool QgsVectorLayerEditBuffer::commitChanges( QStringList &commitErrors )
QgsVectorLayerUtils::matchAttributesToFields( featuresToAdd[i], provider->fields() );
}

if ( provider->addFeatures( featuresToAdd ) )
if ( provider->addFeatures( featuresToAdd, QgsFeatureSink::Flag::RollBackOnErrors ) )
{
commitErrors << tr( "SUCCESS: %n feature(s) added.", "added features count", featuresToAdd.size() );

@@ -29,7 +29,8 @@
QgsRectangle,
QgsTestUtils,
QgsFeatureSource,
QgsProjUtils
QgsProjUtils,
QgsFeatureSink,
)

from qgis.testing import (
@@ -698,15 +699,111 @@ def testTypeValidation(self):
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)'))
invalid = QgsFeature(vl.fields())
invalid.setAttribute('int', 'A string')
invalid.setGeometry(QgsGeometry.fromWkt('point(9 45)'))
self.assertTrue(vl.startEditing())
# Validation happens on commit
self.assertTrue(vl.addFeatures([f]))
self.assertTrue(vl.addFeatures([invalid]))
self.assertFalse(vl.commitChanges())
self.assertTrue(vl.rollBack())
self.assertFalse(vl.hasFeatures())

# Add a valid feature
valid = QgsFeature(vl.fields())
valid.setAttribute('int', 123)
self.assertTrue(vl.startEditing())
self.assertTrue(vl.addFeatures([valid]))
self.assertTrue(vl.commitChanges())
self.assertEqual(vl.featureCount(), 1)

f = vl.getFeature(1)
self.assertEqual(f.attribute('int'), 123)

# Add both
vl = QgsVectorLayer(
'Point?crs=epsg:4326&field=int:integer',
'test', 'memory')
self.assertEqual(vl.featureCount(), 0)
self.assertTrue(vl.startEditing())
self.assertTrue(vl.addFeatures([valid, invalid]))
self.assertFalse(vl.commitChanges())
self.assertEqual(vl.featureCount(), 2)
self.assertTrue(vl.rollBack())
self.assertEqual(vl.featureCount(), 0)

# Add both swapped
vl = QgsVectorLayer(
'Point?crs=epsg:4326&field=int:integer',
'test', 'memory')
self.assertTrue(vl.startEditing())
self.assertTrue(vl.addFeatures([invalid, valid]))
self.assertFalse(vl.commitChanges())
self.assertEqual(vl.featureCount(), 2)
self.assertTrue(vl.rollBack())
self.assertEqual(vl.featureCount(), 0)

# Change attribute value
vl = QgsVectorLayer(
'Point?crs=epsg:4326&field=int:integer',
'test', 'memory')
self.assertTrue(vl.startEditing())
self.assertTrue(vl.addFeatures([valid]))
self.assertTrue(vl.commitChanges())
self.assertTrue(vl.startEditing())
self.assertTrue(vl.changeAttributeValue(1, 0, 'A string'))
self.assertFalse(vl.commitChanges())
f = vl.getFeature(1)
self.assertEqual(f.attribute('int'), 'A string')
self.assertTrue(vl.rollBack())

f = vl.getFeature(1)
self.assertEqual(f.attribute('int'), 123)

# Change attribute values
vl = QgsVectorLayer(
'Point?crs=epsg:4326&field=int:integer',
'test', 'memory')
self.assertTrue(vl.startEditing())
self.assertTrue(vl.addFeatures([valid]))
self.assertTrue(vl.commitChanges())
self.assertTrue(vl.startEditing())
self.assertTrue(vl.changeAttributeValues(1, {0: 'A string'}))
self.assertFalse(vl.commitChanges())
f = vl.getFeature(1)
self.assertEqual(f.attribute('int'), 'A string')
self.assertTrue(vl.rollBack())

f = vl.getFeature(1)
self.assertEqual(f.attribute('int'), 123)

##############################################
# Test direct data provider calls

# No rollback (old behavior)
vl = QgsVectorLayer(
'Point?crs=epsg:4326&field=int:integer',
'test', 'memory')
dp = vl.dataProvider()
self.assertFalse(dp.addFeatures([valid, invalid])[0])
self.assertEqual([f.attributes() for f in dp.getFeatures()], [[123]])
f = vl.getFeature(1)
self.assertEqual(f.attribute('int'), 123)

# Roll back
vl = QgsVectorLayer(
'Point?crs=epsg:4326&field=int:integer',
'test', 'memory')
dp = vl.dataProvider()
self.assertFalse(dp.addFeatures([valid, invalid], QgsFeatureSink.RollBackOnErrors)[0])
self.assertFalse(dp.hasFeatures())

# Expected behavior for changeAttributeValues is to always roll back
self.assertTrue(dp.addFeatures([valid])[0])
self.assertFalse(dp.changeAttributeValues({1: {0: 'A string'}}))
f = vl.getFeature(1)
self.assertEqual(f.attribute('int'), 123)


class TestPyQgsMemoryProviderIndexed(unittest.TestCase, ProviderTestCase):

0 comments on commit 7fa6f38

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