Merge pull request #6000 from elpaso/bugfix-17795-ogr-filtered-readonly

[bugfix][ogr] Recompute capabilities when subsetfilter is set
qgis · Jan 6, 2018 · a343570 · a343570
2 parents 7605fde + 248ad5f
commit a343570
Show file tree

Hide file tree

Showing 5 changed files with 159 additions and 86 deletions.
diff --git 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
@@ -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
@@ -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):
@@ -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()
diff --git a/tests/src/python/test_provider_shapefile.py b/tests/src/python/test_provider_shapefile.py
@@ -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()
diff --git a/tests/testdata/provider/bug_17795.gpkg b/tests/testdata/provider/bug_17795.gpkg