Skip to content
Permalink
Browse files

Skip potential reset reading calls

  • Loading branch information
elpaso committed Oct 8, 2020
1 parent 2f6c33c commit 505ef99328f8d1ba63919aeb89d815a6c4ce679b
@@ -41,12 +41,22 @@
///@cond PRIVATE


QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request, QgsTransaction *transaction )
: QgsAbstractFeatureIteratorFromSource<QgsOgrFeatureSource>( source, ownSource, request )
, mSharedDS( source->mSharedDS )
, mFirstFieldIsFid( source->mFirstFieldIsFid )
, mFieldsWithoutFid( source->mFieldsWithoutFid )
{

/* When inside a transaction for GPKG/SQLite and fetching fid(s) we might be nested inside an outer fetching loop,
* (see GH #39178) so we need to skip all calls that might reset the reading (rewind) to avoid an endless loop in the
* outer fetching iterator that uses the same connection.
*/
mDoNotResetReading = transaction &&
( source->mDriverName == QLatin1String( "GPKG" ) || source->mDriverName == QLatin1String( "SQLite" ) ) &&
( mRequest.filterType() == QgsFeatureRequest::FilterType::FilterFid
|| mRequest.filterType() == QgsFeatureRequest::FilterType::FilterFids );

for ( const auto &id : mRequest.filterFids() )
{
mFilterFids.insert( id );
@@ -85,7 +95,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
return;
}

if ( !mSource->mSubsetString.isEmpty() )
if ( ! mDoNotResetReading && !mSource->mSubsetString.isEmpty() )
{
mOgrLayerOri = mOgrLayer;
mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString );
@@ -165,17 +175,20 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
}

// spatial query to select features
if ( !mFilterRect.isNull() )
if ( ! mDoNotResetReading )
{
OGR_L_SetSpatialFilterRect( mOgrLayer, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilterRect( mOgrLayerOri, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
}
else
{
OGR_L_SetSpatialFilter( mOgrLayer, nullptr );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilter( mOgrLayerOri, nullptr );
if ( !mFilterRect.isNull() )
{
OGR_L_SetSpatialFilterRect( mOgrLayer, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilterRect( mOgrLayerOri, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
}
else
{
OGR_L_SetSpatialFilter( mOgrLayer, nullptr );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilter( mOgrLayerOri, nullptr );
}
}

if ( request.filterType() == QgsFeatureRequest::FilterExpression
@@ -201,33 +214,38 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
QStringLiteral( ") AND (" ) + whereClause +
QStringLiteral( ")" );
}
if ( OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( whereClause ).constData() ) == OGRERR_NONE )
{
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
}
else if ( !mSource->mSubsetString.isEmpty() )

if ( ! mDoNotResetReading )
{
// OGR rejected the compiled expression. Make sure we restore the original subset string if set (and do the filtering on QGIS' side)
OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( mSource->mSubsetString ).constData() );
if ( OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( whereClause ).constData() ) == OGRERR_NONE )
{
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
}
else if ( !mSource->mSubsetString.isEmpty() )
{
// OGR rejected the compiled expression. Make sure we restore the original subset string if set (and do the filtering on QGIS' side)
OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( mSource->mSubsetString ).constData() );
}
}

}
else if ( mSource->mSubsetString.isEmpty() )
else if ( mSource->mSubsetString.isEmpty() && ! mDoNotResetReading )
{
OGR_L_SetAttributeFilter( mOgrLayer, nullptr );
}

delete compiler;
}
else if ( mSource->mSubsetString.isEmpty() )
else if ( mSource->mSubsetString.isEmpty() && ! mDoNotResetReading )
{
OGR_L_SetAttributeFilter( mOgrLayer, nullptr );
}


//start with first feature
rewind();

}

QgsOgrFeatureIterator::~QgsOgrFeatureIterator()
@@ -249,7 +267,7 @@ bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature &fea
gdal::ogr_feature_unique_ptr fet;

#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,2,0)
if ( !QgsOgrProviderUtils::canDriverShareSameDatasetAmongLayers( mSource->mDriverName ) )
if ( ! mDoNotResetReading && !QgsOgrProviderUtils::canDriverShareSameDatasetAmongLayers( mSource->mDriverName ) )
{
OGRLayerH nextFeatureBelongingLayer;
bool found = false;
@@ -381,6 +399,10 @@ bool QgsOgrFeatureIterator::fetchFeature( QgsFeature &feature )

void QgsOgrFeatureIterator::resetReading()
{
if ( mDoNotResetReading )
{
return;
}
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,2,0)
if ( !QgsOgrProviderUtils::canDriverShareSameDatasetAmongLayers( mSource->mDriverName ) )
{
@@ -561,6 +583,7 @@ QgsOgrFeatureSource::QgsOgrFeatureSource( const QgsOgrProvider *p )
{
if ( p->mTransaction )
{
mTransaction = p->mTransaction;
mSharedDS = p->mTransaction->sharedDS();
}
for ( int i = ( p->mFirstFieldIsFid ) ? 1 : 0; i < mFields.size(); i++ )
@@ -575,7 +598,7 @@ QgsOgrFeatureSource::~QgsOgrFeatureSource()

QgsFeatureIterator QgsOgrFeatureSource::getFeatures( const QgsFeatureRequest &request )
{
return QgsFeatureIterator( new QgsOgrFeatureIterator( this, false, request ) );
return QgsFeatureIterator( new QgsOgrFeatureIterator( this, false, request, mTransaction ) );
}

///@endcond
@@ -56,6 +56,7 @@ class QgsOgrFeatureSource final: public QgsAbstractFeatureSource
QgsCoordinateReferenceSystem mCrs;
QgsWkbTypes::Type mWkbType = QgsWkbTypes::Unknown;
QgsOgrDatasetSharedPtr mSharedDS = nullptr;
QgsTransaction *mTransaction = nullptr;

friend class QgsOgrFeatureIterator;
friend class QgsOgrExpressionCompiler;
@@ -64,7 +65,7 @@ class QgsOgrFeatureSource final: public QgsAbstractFeatureSource
class QgsOgrFeatureIterator final: public QgsAbstractFeatureIteratorFromSource<QgsOgrFeatureSource>
{
public:
QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request );
QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request, QgsTransaction *transaction );

~QgsOgrFeatureIterator() override;

@@ -102,6 +103,11 @@ class QgsOgrFeatureIterator final: public QgsAbstractFeatureIteratorFromSource<Q
bool mFirstFieldIsFid = false;
QgsFields mFieldsWithoutFid;

/* This flag tells the iterator to skip all calls that might reset the reading (rewind),
* to be used when the request is for a single fid or for a list of fids and we are inside
* a transaction for SQLITE-based layers */
bool mDoNotResetReading = false;

bool fetchFeatureWithId( QgsFeatureId id, QgsFeature &feature ) const;

void resetReading();
@@ -1340,7 +1340,7 @@ void QgsOgrProviderUtils::setRelevantFields( OGRLayerH ogrLayer, int fieldCount,

QgsFeatureIterator QgsOgrProvider::getFeatures( const QgsFeatureRequest &request ) const
{
return QgsFeatureIterator( new QgsOgrFeatureIterator( static_cast<QgsOgrFeatureSource *>( featureSource() ), true, request ) );
return QgsFeatureIterator( new QgsOgrFeatureIterator( static_cast<QgsOgrFeatureSource *>( featureSource() ), true, request, mTransaction ) );
}


@@ -1768,6 +1768,37 @@ def testTransactionGroup(self):
self.assertFalse(vl1_1.isEditable())
self.assertFalse(vl1_2.isEditable())

def testTransactionGroupIterator(self):
"""Test issue GH #39178: the bug is that this test hangs
forever in an endless loop"""

project = QgsProject()
project.setAutoTransaction(True)
tmpfile = os.path.join(
self.basetestpath, 'tempGeoPackageTransactionGroupIterator.gpkg')
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
lyr.CreateField(ogr.FieldDefn('str_field', ogr.OFTString))

f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometry(ogr.CreateGeometryFromWkt('POINT (1 1)'))
f.SetField('str_field', 'one')
lyr.CreateFeature(f)

del lyr
del ds

vl = QgsVectorLayer(tmpfile + '|layername=test', 'test', 'ogr')
project.addMapLayers([vl])

self.assertTrue(vl.startEditing())

for f in vl.getFeatures():
self.assertTrue(vl.changeAttributeValue(1, 1, 'new value'))

# Test that QGIS sees the new changes
self.assertEqual(next(vl.getFeatures()).attribute(1), 'new value')


if __name__ == '__main__':
unittest.main()

0 comments on commit 505ef99

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