Skip to content

Commit

Permalink
Merge pull request #6000 from elpaso/bugfix-17795-ogr-filtered-readonly
Browse files Browse the repository at this point in the history
 [bugfix][ogr] Recompute capabilities when subsetfilter is set
  • Loading branch information
elpaso authored Jan 6, 2018
2 parents 7605fde + 248ad5f commit a343570
Show file tree
Hide file tree
Showing 5 changed files with 159 additions and 86 deletions.
193 changes: 107 additions & 86 deletions src/providers/ogr/qgsogrprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -528,90 +528,7 @@ QgsAbstractFeatureSource *QgsOgrProvider::featureSource() const

bool QgsOgrProvider::setSubsetString( const QString &theSQL, bool updateFeatureCount )
{
QgsCPLErrorHandler handler;

if ( !mOgrOrigLayer )
return false;

if ( theSQL == mSubsetString && mFeaturesCounted != QgsVectorDataProvider::Uncounted )
return true;

if ( !theSQL.isEmpty() )
{
bool origFidAdded = false;
QMutex *mutex = nullptr;
OGRLayerH layer = mOgrOrigLayer->getHandleAndMutex( mutex );
GDALDatasetH ds = mOgrOrigLayer->getDatasetHandleAndMutex( mutex );
OGRLayerH subsetLayerH;
{
QMutexLocker locker( mutex );
subsetLayerH = QgsOgrProviderUtils::setSubsetString( layer, ds, textEncoding(), theSQL, origFidAdded );
}
if ( !subsetLayerH )
{
pushError( tr( "OGR[%1] error %2: %3" ).arg( CPLGetLastErrorType() ).arg( CPLGetLastErrorNo() ).arg( CPLGetLastErrorMsg() ) );
return false;
}
mOgrSqlLayer = QgsOgrProviderUtils::getSqlLayer( mOgrOrigLayer.get(), subsetLayerH, theSQL );
Q_ASSERT( mOgrSqlLayer.get() );
mOgrLayer = mOgrSqlLayer.get();
}
else
{
mOgrSqlLayer.reset();
mOgrLayer = mOgrOrigLayer.get();
}
mSubsetString = theSQL;

QString uri = mFilePath;
if ( !mLayerName.isNull() )
{
uri += QStringLiteral( "|layername=%1" ).arg( mLayerName );
}
else if ( mLayerIndex >= 0 )
{
uri += QStringLiteral( "|layerid=%1" ).arg( mLayerIndex );
}

if ( !mSubsetString.isEmpty() )
{
uri += QStringLiteral( "|subset=%1" ).arg( mSubsetString );
}

if ( mOgrGeometryTypeFilter != wkbUnknown )
{
uri += QStringLiteral( "|geometrytype=%1" ).arg( ogrWkbGeometryTypeName( mOgrGeometryTypeFilter ) );
}

if ( uri != dataSourceUri() )
{
QgsOgrConnPool::instance()->unref( QgsOgrProviderUtils::connectionPoolId( dataSourceUri( true ) ) );
setDataSourceUri( uri );
QgsOgrConnPool::instance()->ref( QgsOgrProviderUtils::connectionPoolId( dataSourceUri( true ) ) );
}

mOgrLayer->ResetReading();

// getting the total number of features in the layer
// TODO: This can be expensive, do we really need it!
if ( updateFeatureCount )
{
recalculateFeatureCount();
}

// check the validity of the layer
QgsDebugMsgLevel( "checking validity", 4 );
loadFields();
QgsDebugMsgLevel( "Done checking validity", 4 );

invalidateCachedExtent( false );

// Changing the filter may change capabilities
computeCapabilities();

emit dataChanged();

return true;
return _setSubsetString( theSQL, updateFeatureCount, true );
}

QString QgsOgrProvider::subsetString() const
Expand Down Expand Up @@ -1763,6 +1680,96 @@ bool QgsOgrProvider::commitTransaction()
return true;
}

bool QgsOgrProvider::_setSubsetString( const QString &theSQL, bool updateFeatureCount, bool updateCapabilities )
{
QgsCPLErrorHandler handler;

if ( !mOgrOrigLayer )
return false;

if ( theSQL == mSubsetString && mFeaturesCounted != QgsVectorDataProvider::Uncounted )
return true;

if ( !theSQL.isEmpty() )
{
bool origFidAdded = false;
QMutex *mutex = nullptr;
OGRLayerH layer = mOgrOrigLayer->getHandleAndMutex( mutex );
GDALDatasetH ds = mOgrOrigLayer->getDatasetHandleAndMutex( mutex );
OGRLayerH subsetLayerH;
{
QMutexLocker locker( mutex );
subsetLayerH = QgsOgrProviderUtils::setSubsetString( layer, ds, textEncoding(), theSQL, origFidAdded );
}
if ( !subsetLayerH )
{
pushError( tr( "OGR[%1] error %2: %3" ).arg( CPLGetLastErrorType() ).arg( CPLGetLastErrorNo() ).arg( CPLGetLastErrorMsg() ) );
return false;
}
mOgrSqlLayer = QgsOgrProviderUtils::getSqlLayer( mOgrOrigLayer.get(), subsetLayerH, theSQL );
Q_ASSERT( mOgrSqlLayer.get() );
mOgrLayer = mOgrSqlLayer.get();
}
else
{
mOgrSqlLayer.reset();
mOgrLayer = mOgrOrigLayer.get();
}
mSubsetString = theSQL;

QString uri = mFilePath;
if ( !mLayerName.isNull() )
{
uri += QStringLiteral( "|layername=%1" ).arg( mLayerName );
}
else if ( mLayerIndex >= 0 )
{
uri += QStringLiteral( "|layerid=%1" ).arg( mLayerIndex );
}

if ( !mSubsetString.isEmpty() )
{
uri += QStringLiteral( "|subset=%1" ).arg( mSubsetString );
}

if ( mOgrGeometryTypeFilter != wkbUnknown )
{
uri += QStringLiteral( "|geometrytype=%1" ).arg( ogrWkbGeometryTypeName( mOgrGeometryTypeFilter ) );
}

if ( uri != dataSourceUri() )
{
QgsOgrConnPool::instance()->unref( QgsOgrProviderUtils::connectionPoolId( dataSourceUri( true ) ) );
setDataSourceUri( uri );
QgsOgrConnPool::instance()->ref( QgsOgrProviderUtils::connectionPoolId( dataSourceUri( true ) ) );
}

mOgrLayer->ResetReading();

// getting the total number of features in the layer
// TODO: This can be expensive, do we really need it!
if ( updateFeatureCount )
{
recalculateFeatureCount();
}

// check the validity of the layer
QgsDebugMsgLevel( "checking validity", 4 );
loadFields();
QgsDebugMsgLevel( "Done checking validity", 4 );

invalidateCachedExtent( false );

// Changing the filter may change capabilities
if ( updateCapabilities )
computeCapabilities();

emit dataChanged();

return true;

}


bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_map )
{
Expand Down Expand Up @@ -2186,10 +2193,19 @@ QgsVectorDataProvider::Capabilities QgsOgrProvider::capabilities() const
void QgsOgrProvider::computeCapabilities()
{
QgsVectorDataProvider::Capabilities ability = nullptr;
bool updateModeActivated = false;

// collect abilities reported by OGR
if ( mOgrLayer )
{

// We want the layer in rw mode or capabilities will be wrong
// If mUpdateModeStackDepth > 0, it means that an updateMode is already active and that we have write access
if ( mUpdateModeStackDepth == 0 )
{
updateModeActivated = _enterUpdateMode( true );
}

// Whilst the OGR documentation (e.g. at
// http://www.gdal.org/ogr/classOGRLayer.html#a17) states "The capability
// codes that can be tested are represented as strings, but #defined
Expand Down Expand Up @@ -2318,6 +2334,9 @@ void QgsOgrProvider::computeCapabilities()
}
}

if ( updateModeActivated )
leaveUpdateMode();

mCapabilities = ability;
}

Expand Down Expand Up @@ -3969,7 +3988,8 @@ void QgsOgrProvider::open( OpenMode mode )
mSubsetString.clear();
// Block signals to avoid endless recusion reloadData -> emit dataChanged -> reloadData
blockSignals( true );
mValid = setSubsetString( origSubsetString );
// Do not update capabilities: it will be done later
mValid = _setSubsetString( origSubsetString, true, false );
blockSignals( false );
if ( mValid )
{
Expand Down Expand Up @@ -4032,7 +4052,8 @@ void QgsOgrProvider::open( OpenMode mode )
{
int featuresCountedBackup = mFeaturesCounted;
mFeaturesCounted = -1;
mValid = setSubsetString( mSubsetString, false );
// Do not update capabilities here
mValid = _setSubsetString( mSubsetString, false, false );
mFeaturesCounted = featuresCountedBackup;
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/providers/ogr/qgsogrprovider.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@ class QgsOgrProvider : public QgsVectorDataProvider
//! Commits a transaction
bool commitTransaction();

//! Does the real job of settings the subset string and adds an argument to disable update capabilities
bool _setSubsetString( const QString &theSQL, bool updateFeatureCount = true, bool updateCapabilities = true );

void addSubLayerDetailsToSubLayerList( int i, QgsOgrLayer *layer ) const;

QgsFields mAttributeFields;
Expand Down
26 changes: 26 additions & 0 deletions tests/src/python/test_provider_ogr_gpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
from qgis.PyQt.QtCore import QCoreApplication, QVariant
from qgis.testing import start_app, unittest
from qgis.utils import spatialite_connect
from utilities import unitTestDataPath

TEST_DATA_DIR = unitTestDataPath()


def GDAL_COMPUTE_VERSION(maj, min, rev):
Expand Down Expand Up @@ -834,6 +837,29 @@ def testCreateSpatialIndex(self):
self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.CreateSpatialIndex)
self.assertTrue(vl.dataProvider().createSpatialIndex())

def testSubSetStringEditable_bug17795(self):
"""Test that a layer is not editable after setting a subset and it's reverted to editable after the filter is removed"""

isEditable = QgsVectorDataProvider.ChangeAttributeValues
testPath = TEST_DATA_DIR + '/' + 'provider/bug_17795.gpkg|layername=bug_17795'

vl = QgsVectorLayer(testPath, 'subset_test', 'ogr')
self.assertTrue(vl.isValid())
self.assertTrue(vl.dataProvider().capabilities() & isEditable)

vl = QgsVectorLayer(testPath, 'subset_test', 'ogr')
vl.setSubsetString('')
self.assertTrue(vl.isValid())
self.assertTrue(vl.dataProvider().capabilities() & isEditable)

vl = QgsVectorLayer(testPath, 'subset_test', 'ogr')
vl.setSubsetString('"category" = \'one\'')
self.assertTrue(vl.isValid())
self.assertFalse(vl.dataProvider().capabilities() & isEditable)

vl.setSubsetString('')
self.assertTrue(vl.dataProvider().capabilities() & isEditable)


if __name__ == '__main__':
unittest.main()
23 changes: 23 additions & 0 deletions tests/src/python/test_provider_shapefile.py
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,29 @@ def testCreateSpatialIndex(self):
self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.CreateSpatialIndex)
self.assertTrue(vl.dataProvider().createSpatialIndex())

def testSubSetStringEditable_bug17795(self):
"""Test that a layer is not editable after setting a subset and it's reverted to editable after the filter is removed"""

testPath = TEST_DATA_DIR + '/' + 'lines.shp'
isEditable = QgsVectorDataProvider.ChangeAttributeValues

vl = QgsVectorLayer(testPath, 'subset_test', 'ogr')
self.assertTrue(vl.isValid())
self.assertTrue(vl.dataProvider().capabilities() & isEditable)

vl = QgsVectorLayer(testPath, 'subset_test', 'ogr')
vl.setSubsetString('')
self.assertTrue(vl.isValid())
self.assertTrue(vl.dataProvider().capabilities() & isEditable)

vl = QgsVectorLayer(testPath, 'subset_test', 'ogr')
vl.setSubsetString('"Name" = \'Arterial\'')
self.assertTrue(vl.isValid())
self.assertFalse(vl.dataProvider().capabilities() & isEditable)

vl.setSubsetString('')
self.assertTrue(vl.dataProvider().capabilities() & isEditable)


if __name__ == '__main__':
unittest.main()
Binary file added tests/testdata/provider/bug_17795.gpkg
Binary file not shown.

0 comments on commit a343570

Please sign in to comment.