Merge pull request #6002 from elpaso/bugfix-17795-2-18-backport

[bugfix][ogr] Recompute capabilities when subsetfilter is set
qgis · Jan 6, 2018 · 4bcd2e4 · 4bcd2e4
2 parents 8127c4c + 0f2a520
commit 4bcd2e4
Show file tree

Hide file tree

Showing 5 changed files with 158 additions and 86 deletions.
diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -503,89 +503,8 @@ QgsAbstractFeatureSource* QgsOgrProvider::featureSource() const
 
 bool QgsOgrProvider::setSubsetString( const QString& theSQL, bool updateFeatureCount )
 {
-  QgsCPLErrorHandler handler;
-
-  if ( !ogrDataSource )
-    return false;
-
-  if ( theSQL == mSubsetString && mFeaturesCounted >= 0 )
-    return true;
-
-  OGRLayerH prevLayer = ogrLayer;
-  QString prevSubsetString = mSubsetString;
-  mSubsetString = theSQL;
-
-  if ( !mSubsetString.isEmpty() )
-  {
-
-    ogrLayer = setSubsetString( ogrOrigLayer, ogrDataSource );
-    if ( !ogrLayer )
-    {
-      pushError( tr( "OGR[%1] error %2: %3" ).arg( CPLGetLastErrorType() ).arg( CPLGetLastErrorNo() ).arg( CPLGetLastErrorMsg() ) );
-      ogrLayer = prevLayer;
-      mSubsetString = prevSubsetString;
-      return false;
-    }
-  }
-  else
-  {
-    ogrLayer = ogrOrigLayer;
-  }
-
-  if ( prevLayer != ogrOrigLayer )
-  {
-    OGR_DS_ReleaseResultSet( ogrDataSource, prevLayer );
-  }
-
-  QString uri = mFilePath;
-  if ( !mLayerName.isNull() )
-  {
-    uri += QString( "|layername=%1" ).arg( mLayerName );
-  }
-  else if ( mLayerIndex >= 0 )
-  {
-    uri += QString( "|layerid=%1" ).arg( mLayerIndex );
-  }
-
-  if ( !mSubsetString.isEmpty() )
-  {
-    uri += QString( "|subset=%1" ).arg( mSubsetString );
-  }
-
-  if ( mOgrGeometryTypeFilter != wkbUnknown )
-  {
-    uri += QString( "|geometrytype=%1" ).arg( ogrWkbGeometryTypeName( mOgrGeometryTypeFilter ) );
-  }
-
-  if ( uri != dataSourceUri() )
-  {
-    QgsOgrConnPool::instance()->unref( dataSourceUri() );
-    setDataSourceUri( uri );
-    QgsOgrConnPool::instance()->ref( dataSourceUri() );
-  }
-
-  OGR_L_ResetReading( ogrLayer );
-
-  // 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
-  QgsDebugMsg( "checking validity" );
-  loadFields();
-  QgsDebugMsg( "Done checking validity" );
-
-  invalidateCachedExtent( false );
-
-  // Changing the filter may change capabilities
-  computeCapabilities();
-
-  emit dataChanged();
-
-  return true;
+  // Always update capabilities
+  return _setSubsetString( theSQL, updateFeatureCount, true );
 }
 
 QString QgsOgrProvider::subsetString()
@@ -1640,6 +1559,94 @@ bool QgsOgrProvider::commitTransaction()
   return true;
 }
 
+bool QgsOgrProvider::_setSubsetString( const QString &theSQL, bool updateFeatureCount, bool updateCapabilities )
+{
+  QgsCPLErrorHandler handler;
+
+  if ( !ogrDataSource )
+    return false;
+
+  if ( theSQL == mSubsetString && mFeaturesCounted >= 0 )
+    return true;
+
+  OGRLayerH prevLayer = ogrLayer;
+  QString prevSubsetString = mSubsetString;
+  mSubsetString = theSQL;
+
+  if ( !mSubsetString.isEmpty() )
+  {
+
+    ogrLayer = setSubsetString( ogrOrigLayer, ogrDataSource );
+    if ( !ogrLayer )
+    {
+      pushError( tr( "OGR[%1] error %2: %3" ).arg( CPLGetLastErrorType() ).arg( CPLGetLastErrorNo() ).arg( CPLGetLastErrorMsg() ) );
+      ogrLayer = prevLayer;
+      mSubsetString = prevSubsetString;
+      return false;
+    }
+  }
+  else
+  {
+    ogrLayer = ogrOrigLayer;
+  }
+
+  if ( prevLayer != ogrOrigLayer )
+  {
+    OGR_DS_ReleaseResultSet( ogrDataSource, prevLayer );
+  }
+
+  QString uri = mFilePath;
+  if ( !mLayerName.isNull() )
+  {
+    uri += QString( "|layername=%1" ).arg( mLayerName );
+  }
+  else if ( mLayerIndex >= 0 )
+  {
+    uri += QString( "|layerid=%1" ).arg( mLayerIndex );
+  }
+
+  if ( !mSubsetString.isEmpty() )
+  {
+    uri += QString( "|subset=%1" ).arg( mSubsetString );
+  }
+
+  if ( mOgrGeometryTypeFilter != wkbUnknown )
+  {
+    uri += QString( "|geometrytype=%1" ).arg( ogrWkbGeometryTypeName( mOgrGeometryTypeFilter ) );
+  }
+
+  if ( uri != dataSourceUri() )
+  {
+    QgsOgrConnPool::instance()->unref( dataSourceUri() );
+    setDataSourceUri( uri );
+    QgsOgrConnPool::instance()->ref( dataSourceUri() );
+  }
+
+  OGR_L_ResetReading( ogrLayer );
+
+  // 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
+  QgsDebugMsg( "checking validity" );
+  loadFields();
+  QgsDebugMsg( "Done checking validity" );
+
+  invalidateCachedExtent( false );
+
+  // Changing the filter may change capabilities
+  if ( updateCapabilities )
+    computeCapabilities();
+
+  emit dataChanged();
+
+  return true;
+}
+
 
 bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_map )
 {
@@ -2008,10 +2015,18 @@ int QgsOgrProvider::capabilities() const
 void QgsOgrProvider::computeCapabilities()
 {
   int ability = 0;
+  bool updateModeActivated = false;
 
   // collect abilities reported by OGR
   if ( ogrLayer )
   {
+    // 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( );
+    }
+
     // 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
@@ -2138,6 +2153,10 @@ void QgsOgrProvider::computeCapabilities()
     }
   }
 
+  if ( updateModeActivated )
+    leaveUpdateMode();
+
+
   mCapabilities = ability;
 }
 
@@ -3620,7 +3639,8 @@ void QgsOgrProvider::open( OpenMode mode )
       mSubsetString = "";
       // 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 )
       {
@@ -3696,7 +3716,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
@@ -312,6 +312,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 );
+
     QgsFields mAttributeFields;
     bool mFirstFieldIsFid;
     OGRDataSourceH ogrDataSource;

diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py
@@ -21,10 +21,14 @@
 from osgeo import gdal, ogr
 
 from qgis.PyQt.QtCore import QCoreApplication, QSettings
-from qgis.core import QgsVectorLayer, QgsVectorLayerImport, QgsFeature, QgsGeometry, QgsFeatureRequest, QgsRectangle
+from qgis.core import QgsVectorLayer, QgsVectorLayerImport, QgsFeature, QgsGeometry, QgsFeatureRequest, QgsRectangle, QgsVectorDataProvider
 from qgis.testing import start_app, unittest
 from utilities import unitTestDataPath
 
+from utilities import unitTestDataPath
+
+TEST_DATA_DIR = unitTestDataPath()
+
 start_app()
 
 
@@ -419,6 +423,26 @@ def testGeopackageLargeFID(self):
         self.assertTrue(vl.deleteFeature(1234567890123))
         self.assertTrue(vl.commitChanges())
 
+    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)
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/src/python/test_provider_shapefile.py b/tests/src/python/test_provider_shapefile.py
@@ -481,5 +481,29 @@ def testRepackAtFirstSave(self):
         self.assertTrue(ds.GetLayer(0).GetFeatureCount(), original_feature_count - 1)
         ds = None
 
+    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