Skip to content

Commit

Permalink
Merge pull request #6002 from elpaso/bugfix-17795-2-18-backport
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 8127c4c + 0f2a520 commit 4bcd2e4
Show file tree
Hide file tree
Showing 5 changed files with 158 additions and 86 deletions.
191 changes: 106 additions & 85 deletions src/providers/ogr/qgsogrprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 )
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2138,6 +2153,10 @@ void QgsOgrProvider::computeCapabilities()
}
}

if ( updateModeActivated )
leaveUpdateMode();


mCapabilities = ability;
}

Expand Down Expand Up @@ -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 )
{
Expand Down Expand Up @@ -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;
}
}
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 @@ -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;
Expand Down
26 changes: 25 additions & 1 deletion tests/src/python/test_provider_ogr_gpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()


Expand Down Expand Up @@ -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()
24 changes: 24 additions & 0 deletions tests/src/python/test_provider_shapefile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Binary file added tests/testdata/provider/bug_17795.gpkg
Binary file not shown.

0 comments on commit 4bcd2e4

Please sign in to comment.