diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index efd8193ee2e..1df6f2b46a7 100644 --- a/src/providers/ogr/qgsogrprovider.cpp +++ b/src/providers/ogr/qgsogrprovider.cpp @@ -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 @@ -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 ) { @@ -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 @@ -2318,6 +2334,9 @@ void QgsOgrProvider::computeCapabilities() } } + if ( updateModeActivated ) + leaveUpdateMode(); + mCapabilities = ability; } @@ -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 ) { @@ -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; } } diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h index b9d43d5fe0f..c25a68401b3 100644 --- a/src/providers/ogr/qgsogrprovider.h +++ b/src/providers/ogr/qgsogrprovider.h @@ -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; diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py index 2e01cd6bc53..8d42bed554e 100644 --- a/tests/src/python/test_provider_ogr_gpkg.py +++ b/tests/src/python/test_provider_ogr_gpkg.py @@ -39,6 +39,9 @@ from qgis.core import (QgsFeature, 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): @@ -834,6 +837,29 @@ class TestPyQgsOGRProviderGpkg(unittest.TestCase): 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() diff --git a/tests/src/python/test_provider_shapefile.py b/tests/src/python/test_provider_shapefile.py index f6d48fe3cda..37fe110102a 100644 --- a/tests/src/python/test_provider_shapefile.py +++ b/tests/src/python/test_provider_shapefile.py @@ -612,6 +612,29 @@ class TestPyQgsShapefileProvider(unittest.TestCase, ProviderTestCase): 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() diff --git a/tests/testdata/provider/bug_17795.gpkg b/tests/testdata/provider/bug_17795.gpkg new file mode 100644 index 00000000000..ea914cdf5d5 Binary files /dev/null and b/tests/testdata/provider/bug_17795.gpkg differ