From dd60fcd4fc23371f39bbb83b9b82473e80b158d7 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 5 Jan 2018 11:06:40 +0100 Subject: [PATCH 1/5] [bugfix] Add failing test for OGR setSubsetString Fails to revert to rw after a subset string is set and cleared --- tests/src/python/test_provider_ogr.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/src/python/test_provider_ogr.py b/tests/src/python/test_provider_ogr.py index 38e42bdb18e..ef4c21c40fa 100644 --- a/tests/src/python/test_provider_ogr.py +++ b/tests/src/python/test_provider_ogr.py @@ -318,6 +318,25 @@ class PyQgsOGRProvider(unittest.TestCase): self.assertEqual(gdal.GetConfigOption("GDAL_HTTP_PROXY"), "myproxyhostname.com") self.assertEqual(gdal.GetConfigOption("GDAL_HTTP_PROXYUSERPWD"), "username") + def testSubSetStringEditable(self): + """Test that a shapefile is editable after setting a subset""" + vl = QgsVectorLayer(TEST_DATA_DIR + '/' + 'lines.shp', 'subset_test', 'ogr') + self.assertTrue(vl.isValid()) + self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) + + vl = QgsVectorLayer(TEST_DATA_DIR + '/' + 'lines.shp', 'subset_test', 'ogr') + vl.setSubsetString('') + self.assertTrue(vl.isValid()) + self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) + + vl = QgsVectorLayer(TEST_DATA_DIR + '/' + 'lines.shp', 'subset_test', 'ogr') + vl.setSubsetString('"Name" = \'Arterial\'') + self.assertTrue(vl.isValid()) + self.assertFalse(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) + + vl.setSubsetString('') + self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) + if __name__ == '__main__': unittest.main() From 3edc5472bb18b3cb1e12a50b40c76abfb1e899ca Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 5 Jan 2018 15:41:56 +0100 Subject: [PATCH 2/5] [bugfix][ogr] Recompute capabilities when subsetfilter is set Need to re-open in rw mode if mDynamicWriteAccess --- src/providers/ogr/qgsogrprovider.cpp | 21 ++++++++++++++++++++- src/providers/ogr/qgsogrprovider.h | 5 +++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index efd8193ee2e..30da22c7100 100644 --- a/src/providers/ogr/qgsogrprovider.cpp +++ b/src/providers/ogr/qgsogrprovider.cpp @@ -607,7 +607,7 @@ bool QgsOgrProvider::setSubsetString( const QString &theSQL, bool updateFeatureC invalidateCachedExtent( false ); // Changing the filter may change capabilities - computeCapabilities(); + emit capabilitiesNeedUpdate(); emit dataChanged(); @@ -2186,10 +2186,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 +2327,9 @@ void QgsOgrProvider::computeCapabilities() } } + if ( updateModeActivated ) + leaveUpdateMode(); + mCapabilities = ability; } @@ -4038,6 +4050,13 @@ void QgsOgrProvider::open( OpenMode mode ) } } + // Connect + if ( mode == OpenModeInitial ) + connect( this, &QgsOgrProvider::capabilitiesNeedUpdate, this, [ = ] + { + computeCapabilities(); + } ); + // For debug/testing purposes if ( !mValid ) setProperty( "_debug_open_mode", "invalid" ); diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h index b9d43d5fe0f..8f77dc443c7 100644 --- a/src/providers/ogr/qgsogrprovider.h +++ b/src/providers/ogr/qgsogrprovider.h @@ -309,6 +309,11 @@ class QgsOgrProvider : public QgsVectorDataProvider void setupProxy(); #endif + signals: + + //! Emitted when capabilities need to be re-computed + void capabilitiesNeedUpdate( ); + }; /** From 9c8533455fe8bfb544734e860bac1237c548bed8 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 5 Jan 2018 15:43:28 +0100 Subject: [PATCH 3/5] [bugfix][ogr] Tests for capabilities update on filter changes Fixes #17795 --- tests/src/python/test_provider_ogr.py | 19 -------------- tests/src/python/test_provider_ogr_gpkg.py | 26 ++++++++++++++++++++ tests/src/python/test_provider_shapefile.py | 23 +++++++++++++++++ tests/testdata/provider/bug_17795.gpkg | Bin 0 -> 118784 bytes 4 files changed, 49 insertions(+), 19 deletions(-) create mode 100644 tests/testdata/provider/bug_17795.gpkg diff --git a/tests/src/python/test_provider_ogr.py b/tests/src/python/test_provider_ogr.py index ef4c21c40fa..38e42bdb18e 100644 --- a/tests/src/python/test_provider_ogr.py +++ b/tests/src/python/test_provider_ogr.py @@ -318,25 +318,6 @@ class PyQgsOGRProvider(unittest.TestCase): self.assertEqual(gdal.GetConfigOption("GDAL_HTTP_PROXY"), "myproxyhostname.com") self.assertEqual(gdal.GetConfigOption("GDAL_HTTP_PROXYUSERPWD"), "username") - def testSubSetStringEditable(self): - """Test that a shapefile is editable after setting a subset""" - vl = QgsVectorLayer(TEST_DATA_DIR + '/' + 'lines.shp', 'subset_test', 'ogr') - self.assertTrue(vl.isValid()) - self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) - - vl = QgsVectorLayer(TEST_DATA_DIR + '/' + 'lines.shp', 'subset_test', 'ogr') - vl.setSubsetString('') - self.assertTrue(vl.isValid()) - self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) - - vl = QgsVectorLayer(TEST_DATA_DIR + '/' + 'lines.shp', 'subset_test', 'ogr') - vl.setSubsetString('"Name" = \'Arterial\'') - self.assertTrue(vl.isValid()) - self.assertFalse(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) - - vl.setSubsetString('') - self.assertTrue(vl.dataProvider().capabilities() & QgsVectorDataProvider.DeleteFeatures) - if __name__ == '__main__': unittest.main() 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 0000000000000000000000000000000000000000..ea914cdf5d5ac26d083f17b985bf44d48312de95 GIT binary patch literal 118784 zcmeI5OK=;>dB*{XB1nl3?L!Oia%Hv<%OEU}B1lmbrBxhA96_*1fC4~kxr#Cw0S$00 zV1^3}m*iSbnbfYf@;D`xq{>Du?9Y zbkBPR0}^~B5>8?=7XXl+HSEpyMrKV?-Q*+lQreyxP z#*(v?6O*agD>Ktmb5k>^xw$LTK{Cvxrlx{rxMIjqSCyKqtyk#fx}@m`#NpW@vO+4f zEH|V&`{mbd5mh1DDkUb(*4ipfB*=900@8p2!H?xfB*=900@8p z2)vvGz8XF=dVX~9CYSESZ_<_>Fg2OFHa?jepPVVBu3VqIc75vV z#PsCM2WNkBnq>?R|3`@Z;R6C700JNY0w4eaAOHd&00JNY0wD0}5EwW)+Hda+VEzB< zG%RWY0w4eaAOHd&00JNY0w4eaAOHd`0-p81@&Ern4-J3rN`XTV009sH0T2KI5C8!X z009sH0T2LzV@_Zod~&o8_y3Q1D?)q_009sH0T2KI5C8!X009sH0T9@OK*0U~ABTp2 zyhkoXg8&GC00@8p2!H?xfB*=900@8p2)wKWM#GWO^U)K1&JBO|8ifYcnp;cxY_4Sa z|NCiZ_@^&x6(AP~fB*=900@8p2!H?xfB*=900@9U_XN)MjGlM={#n}q@c-96L8Jr$ z5C8!X009sH0T2KI5C8!X0D)sfpzZ(vht3`&=^-izfB*=900@8p2!H?xfB*=900@A< zz6tRA|5*R;8yhl!00@8p2!H?xfB*=900@8p2!Oz`BEaAOhxPxlYBC550w4eaAOHd& z00JNY0w4eaAOHgUCV>0@`$mQgAOHd&00JNY0w4eaAOHd&00JOz>0dqbmuH?teyxA>)DKQ=^jtgfZ!E#$v8&oEBk<1r*~rN9c;6?l zX-&CSqs`jdlbWc>bt*O_7PM*oY{(UDRg{$$ZEF5Bw+lkLB#=@eo5=_T68Fakl@TYm zggg0yK(e`_P$-dnu03CzuoOf{-@Z!<`3EdRCYvKgVL`ZEB87CeD2%0V?|f}IGIF0`ywC~b$Ew;8>+~a97uVM+ObkDyJt4N0 zU?W^!n&r5~v~LL4dkM2*Nq(#pz1@9+@;@zBgpRJkV>}OfN)Kqy{A=;r$ZH<=MaGg@vT6s_cC+0yiCYoTr<0ie060ZB=6y z#Ftq&Uq+7(j_Z_hP2Obvb5m@|A5$`)U*;9EMKU6{f+KI|bHx&~Nx2ePdm?IDgDe%Y zi|N7&xhJf+86;hOG?8>=tFmm3hUlWj&0{u`^Ouky1>p{}N4eWV(ZJ2@nMM_@&-WXw#M$eiho zKAh>gpEC08eU-Pzz$c+dBsx1A{%q9gua*yHv8>kD8%j%Wet%$Jdt3XHbZ+dVQF2hn zwhPg=)@V?IPP*|iDP?mjyc5~IuF)RkYH8Nj_{DfFTW;-hi=AZSJ!hZ`;^}hhEO*(h zCCj<&{be@LJDt#zo^?^4Uhy`(D{)sn)2~fs@$kBo?eC2~n%z~G>_#y)LYxZJiPPD> z{TF47A^t1$Z@+aa61{vmy#3HoOtsczt!3CNT>y7LZ@WUeqh0N?i{YtKNn3AHc9nI- z864(?JL%;GW8_JsdwZkHmv_bGzOLEt+(D6N@1BT6O`` zXDnqGoe_>$3CGy%xT?twIu^gUGJdf!ez8)zc=!6n#p@TVAHGfOhHYit zQLN*r%?cZ;0*CJkt;&k57vWP!zuqPI|NlRIq7*t&8v2XDm4U(2-|we={k{L$``0J` zB=i~Q6#8BL8($#MC4rsNYsP*>s?*xgy>d9{Qahjwdwpmww)1zqTL*{Rzc~1E+bwF~ zim|Kxi|1$FiHv08eZM*F#>!siDd~exqawD-Dx1r>R@+vjWwSh+KUSpZ%V|^eud=NS z2jBA14XRa4hrJ%N8lrm3#4*_$wI1TeO>EHrF~Z7gmBVS~LJ2s?GIX#!zk1`u{&0*SL7ITsOLh{1;) z*#tNd_c=S}MA(l`gqL9>VSxq`p2$SP!%Qgr(14=nvYX7J!s3IduwXwGV*_gL?LB7z zmZF(cL+kYQo!7shPYa)wosr*)^l0d7R=;s}O{{ce%(ZTIEp{5>+7tJHPSVBO_Ckr6 zvp=iDc?V~EYUeZ&PpC;sg=vns-ju!R+1#!{1LmAgIU3#6E^jvK@uV%bXLrX*8Bgr2 zL=B^T!);yMo6PRLm4h1XfHG`z;}~sssu#9L>Jh>+MztpEBy29P?>amCe0+|kcTrEK zU$i$%vu_z}N4jF`GT<)$mM(g1xGP?CU;8E7Fm;P&8}J@)QJR|lel00 zbmp*;EM)I7CuH%9aq{$OTPv_27L!OAt_q(0>K7v;@{PXd@jbefPXXM|gtlvZx7{kZ zP`hh6&xpo-DdWy_qU~4jzQ6h1rtC}Id?MM;j+;(F0(%$hW~O4XBi_r5AFp=lUX_@# z_%G18&$+eEt$Vs}*=JunuQ&0h-`r;tyBTP_Jf+LfL18-tzhO06yU!5U;4dY67 z=|45ey}>beA&s>oa2r<!Zo#~W4 z2?`p{of()Ve(=EFxwxP)-o9kOAn#o9h=zNoz~F)37RDa=CBmkOY$C^Fh^(+Jf;_o) zbuvX_TkJF5XpC1XrPbBzjmGs>Yoc5(e-I;VDc`U+Cv5L#PT84-{4ElHm>QpY^l)-~ z&h<6U&Qr!YKb$rWOYG20dYv847@@Ai2^Q)u=7YGt26s6*ZpD-O>fV~ zoc#quL*59SqnMoW7< zg6-APN7Db@a=o-Bz%Ar)YS-t#LX5i+m-aB~hO+6qzDLu6+Y1cnzpg|t!Dgu;&C7dp z!qsUz-J3YnW^qvcac{zZhsb+x0{{PikChOjK>!3m00ck)1V8`;KmY_l00cnb6(E51 z|0~dlr~(Lp00@8p2!H?xfB*=900@8p2<$%*5&6=>L~K+WU8>e*5H~pZGS5 z#^?W+z;^XYWaQpx-%i7P@2TP7!G4@Ww7frv6vyMixi=o_%YtiW;v5y|VwBni;LCE-%feJ5<#cja;}%_>k3fD<4Q=+eD3#H}s0x9j@$}zHB%;qvgFKtY&BuPV7Hrcnd$-iY~OF!O9 z-26&!^!}xANcT}0R6tWrK{bJo^w@hg8g20&bn=@#guUcy|2^7|VT8k8g67h(|ru*LW zags9E0(EtRHeF4UU}=r8xLt*~IU|b1gGm@x|G*@6&u)>h%)NIoIW&n&2O+Wa Date: Fri, 5 Jan 2018 19:55:01 +0100 Subject: [PATCH 4/5] Lambda indentation --- src/providers/ogr/qgsogrprovider.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index 30da22c7100..4e5e9c6a3fe 100644 --- a/src/providers/ogr/qgsogrprovider.cpp +++ b/src/providers/ogr/qgsogrprovider.cpp @@ -4050,12 +4050,15 @@ void QgsOgrProvider::open( OpenMode mode ) } } - // Connect + // Updates capabilities when the subset string changes, it needs to be + // asynchronous because if called directly if will recurse if ( mode == OpenModeInitial ) - connect( this, &QgsOgrProvider::capabilitiesNeedUpdate, this, [ = ] { - computeCapabilities(); - } ); + connect( this, &QgsOgrProvider::capabilitiesNeedUpdate, this, [ = ] + { + computeCapabilities(); + } ); + } // For debug/testing purposes if ( !mValid ) From 248ad5f763adbaa09aa752607a081ce053384e48 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Sat, 6 Jan 2018 01:31:23 +0100 Subject: [PATCH 5/5] Get rid of the connect for capabilities update --- src/providers/ogr/qgsogrprovider.cpp | 191 +++++++++++++-------------- src/providers/ogr/qgsogrprovider.h | 8 +- 2 files changed, 98 insertions(+), 101 deletions(-) diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index 4e5e9c6a3fe..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 - emit capabilitiesNeedUpdate(); - - 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 ) { @@ -3981,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 ) { @@ -4044,22 +4052,13 @@ 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; } } } - // Updates capabilities when the subset string changes, it needs to be - // asynchronous because if called directly if will recurse - if ( mode == OpenModeInitial ) - { - connect( this, &QgsOgrProvider::capabilitiesNeedUpdate, this, [ = ] - { - computeCapabilities(); - } ); - } - // For debug/testing purposes if ( !mValid ) setProperty( "_debug_open_mode", "invalid" ); diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h index 8f77dc443c7..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; @@ -309,11 +312,6 @@ class QgsOgrProvider : public QgsVectorDataProvider void setupProxy(); #endif - signals: - - //! Emitted when capabilities need to be re-computed - void capabilitiesNeedUpdate( ); - }; /**