diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp index 07e98ee41dca..e4e7878a6c07 100644 --- a/src/providers/ogr/qgsogrfeatureiterator.cpp +++ b/src/providers/ogr/qgsogrfeatureiterator.cpp @@ -51,7 +51,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool mRequest.setTimeout( -1 ); if ( mSharedDS ) { - mOgrLayer = mSharedDS->createSQLResultLayer( mSource->mEncoding, mSource->mLayerName, mSource->mLayerIndex ); + mOgrLayer = mSharedDS->getLayerFromNameOrIndex( mSource->mLayerName, mSource->mLayerIndex ); if ( !mOgrLayer ) { return; @@ -81,13 +81,9 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool if ( !mSource->mSubsetString.isEmpty() ) { - mOgrOrigLayer = mOgrLayer; - mOgrLayerWithFid = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, QString(), true, &mOrigFidAdded ); - mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString, true, &mOrigFidAdded ); - - OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( mOgrLayer ); - QByteArray fidColumn( OGR_L_GetFIDColumn( mOgrLayer ) ); - mFirstFieldIsFid = !fidColumn.isEmpty() && OGR_FD_GetFieldIndex( fdef, fidColumn ) < 0; + mOgrLayerOri = mOgrLayer; + mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString ); + // If the mSubsetString was a full SELECT ...., then mOgrLayer will be a OGR SQL layer != mOgrLayerOri mFieldsWithoutFid.clear(); for ( int i = ( mFirstFieldIsFid ) ? 1 : 0; i < mSource->mFields.size(); i++ ) @@ -122,8 +118,6 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ( mSource->mOgrGeometryTypeFilter != wkbUnknown ); QgsAttributeList attrs = ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes ) ? mRequest.subsetOfAttributes() : mSource->mFields.allAttributesList(); - if ( mOrigFidAdded ) - attrs << mSource->mFields.count(); // ensure that all attributes required for expression filter are being fetched if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && request.filterType() == QgsFeatureRequest::FilterExpression ) @@ -158,23 +152,23 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool // filter if we choose to ignore them (fixes #11223) if ( ( mSource->mDriverName != QLatin1String( "VRT" ) && mSource->mDriverName != QLatin1String( "OGR_VRT" ) ) || mFilterRect.isNull() ) { - QgsOgrProviderUtils::setRelevantFields( mOgrLayer, mSource->mFields.count() + ( mOrigFidAdded ? 1 : 0 ), mFetchGeometry, attrs, mSource->mFirstFieldIsFid ); - if ( mOgrLayerWithFid ) - QgsOgrProviderUtils::setRelevantFields( mOgrLayerWithFid, mSource->mFields.count() + ( mOrigFidAdded ? 1 : 0 ), mFetchGeometry, attrs, mSource->mFirstFieldIsFid ); + QgsOgrProviderUtils::setRelevantFields( mOgrLayer, mSource->mFields.count(), mFetchGeometry, attrs, mSource->mFirstFieldIsFid, mSource->mSubsetString ); + if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer ) + QgsOgrProviderUtils::setRelevantFields( mOgrLayerOri, mSource->mFields.count(), mFetchGeometry, attrs, mSource->mFirstFieldIsFid, mSource->mSubsetString ); } // spatial query to select features if ( !mFilterRect.isNull() ) { OGR_L_SetSpatialFilterRect( mOgrLayer, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() ); - if ( mOgrLayerWithFid ) - OGR_L_SetSpatialFilterRect( mOgrLayerWithFid, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() ); + if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer ) + OGR_L_SetSpatialFilterRect( mOgrLayerOri, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() ); } else { OGR_L_SetSpatialFilter( mOgrLayer, nullptr ); - if ( mOgrLayerWithFid ) - OGR_L_SetSpatialFilter( mOgrLayerWithFid, nullptr ); + if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer ) + OGR_L_SetSpatialFilter( mOgrLayerOri, nullptr ); } if ( request.filterType() == QgsFeatureRequest::FilterExpression @@ -194,6 +188,12 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool if ( result == QgsSqlExpressionCompiler::Complete || result == QgsSqlExpressionCompiler::Partial ) { QString whereClause = compiler->result(); + if ( !mSource->mSubsetString.isEmpty() && mOgrLayer == mOgrLayerOri ) + { + whereClause = QStringLiteral( "(" ) + mSource->mSubsetString + + QStringLiteral( ") AND (" ) + whereClause + + QStringLiteral( ")" ); + } if ( OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( whereClause ).constData() ) == OGRERR_NONE ) { //if only partial success when compiling expression, we need to double-check results using QGIS' expressions @@ -201,14 +201,14 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled ); } } - else + else if ( mSource->mSubsetString.isEmpty() ) { OGR_L_SetAttributeFilter( mOgrLayer, nullptr ); } delete compiler; } - else + else if ( mSource->mSubsetString.isEmpty() ) { OGR_L_SetAttributeFilter( mOgrLayer, nullptr ); } @@ -235,19 +235,7 @@ bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature &fea { feature.setValid( false ); gdal::ogr_feature_unique_ptr fet; - if ( mOrigFidAdded ) - { - // if we have been using the original fid as feature IDs (e.g. as a result of a subset - // string in place), then we need to request features from the original, unfiltered - // OGR layer. Otherwise the feature IDs we request will correspond to features in the - // filtered layer, not the original layer! - OGR_L_ResetReading( mOgrLayerWithFid ); - fet.reset( OGR_L_GetFeature( mOgrLayerWithFid, FID_TO_NUMBER( id ) ) ); - } - else - { - fet.reset( OGR_L_GetFeature( mOgrLayer, FID_TO_NUMBER( id ) ) ); - } + fet.reset( OGR_L_GetFeature( mOgrLayer, FID_TO_NUMBER( id ) ) ); if ( !fet ) { @@ -375,11 +363,7 @@ bool QgsOgrFeatureIterator::close() { iteratorClosed(); - if ( mOgrLayer ) - { - mSharedDS->releaseResultSet( mOgrLayer ); - mOgrLayer = nullptr; - } + mOgrLayer = nullptr; mSharedDS.reset(); mClosed = true; return true; @@ -396,13 +380,14 @@ bool QgsOgrFeatureIterator::close() resetReading(); } - if ( mOgrOrigLayer ) + if ( mOgrLayerOri ) { - GDALDatasetReleaseResultSet( mConn->ds, mOgrLayer ); - GDALDatasetReleaseResultSet( mConn->ds, mOgrLayerWithFid ); - mOgrLayer = mOgrOrigLayer; - mOgrOrigLayer = nullptr; - mOgrLayerWithFid = nullptr; + if ( mOgrLayer != mOgrLayerOri ) + { + GDALDatasetReleaseResultSet( mConn->ds, mOgrLayer ); + } + mOgrLayer = nullptr; + mOgrLayerOri = nullptr; } if ( mConn ) @@ -438,19 +423,7 @@ void QgsOgrFeatureIterator::getFeatureAttribute( OGRFeatureH ogrFet, QgsFeature bool QgsOgrFeatureIterator::readFeature( gdal::ogr_feature_unique_ptr fet, QgsFeature &feature ) const { - if ( mOrigFidAdded ) - { - OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( mOgrLayer ); - int lastField = OGR_FD_GetFieldCount( fdef ) - 1; - if ( lastField >= 0 ) - feature.setId( OGR_F_GetFieldAsInteger64( fet.get(), lastField ) ); - else - feature.setId( OGR_F_GetFID( fet.get() ) ); - } - else - { - feature.setId( OGR_F_GetFID( fet.get() ) ); - } + feature.setId( OGR_F_GetFID( fet.get() ) ); feature.initAttributes( mSource->mFields.count() ); feature.setFields( mSource->mFields ); // allow name-based attribute lookups @@ -508,7 +481,8 @@ bool QgsOgrFeatureIterator::readFeature( gdal::ogr_feature_unique_ptr fet, QgsFe else { // all attributes - for ( int idx = 0; idx < mSource->mFields.count(); ++idx ) + const auto fieldCount = mSource->mFields.count(); + for ( int idx = 0; idx < fieldCount; ++idx ) { getFeatureAttribute( fet.get(), feature, idx ); } diff --git a/src/providers/ogr/qgsogrfeatureiterator.h b/src/providers/ogr/qgsogrfeatureiterator.h index 2f09d776c31a..5d19a614bdd2 100644 --- a/src/providers/ogr/qgsogrfeatureiterator.h +++ b/src/providers/ogr/qgsogrfeatureiterator.h @@ -79,11 +79,8 @@ class QgsOgrFeatureIterator : public QgsAbstractFeatureIteratorFromSourcetoUnicode( OGR_Fld_GetNameRef( fldDef ) ); #endif - if ( name == ORIG_OGC_FID ) - { - // don't ever report this field, it's for internal purposes only! - continue; - } - if ( mAttributeFields.indexFromName( name ) != -1 ) { @@ -1114,11 +1106,15 @@ void QgsOgrProvider::setRelevantFields( bool fetchGeometry, const QgsAttributeLi QMutex *mutex = nullptr; OGRLayerH ogrLayer = mOgrLayer->getHandleAndMutex( mutex ); QMutexLocker locker( mutex ); - QgsOgrProviderUtils::setRelevantFields( ogrLayer, mAttributeFields.count(), fetchGeometry, fetchAttributes, mFirstFieldIsFid ); + QgsOgrProviderUtils::setRelevantFields( ogrLayer, mAttributeFields.count(), fetchGeometry, fetchAttributes, mFirstFieldIsFid, mSubsetString ); } -void QgsOgrProviderUtils::setRelevantFields( OGRLayerH ogrLayer, int fieldCount, bool fetchGeometry, const QgsAttributeList &fetchAttributes, bool firstAttrIsFid ) +void QgsOgrProviderUtils::setRelevantFields( OGRLayerH ogrLayer, int fieldCount, + bool fetchGeometry, + const QgsAttributeList &fetchAttributes, + bool firstAttrIsFid, + const QString &subsetString ) { if ( OGR_L_TestCapability( ogrLayer, OLCIgnoreFields ) ) { @@ -1132,7 +1128,17 @@ void QgsOgrProviderUtils::setRelevantFields( OGRLayerH ogrLayer, int fieldCount, if ( OGRFieldDefnH field = OGR_FD_GetFieldDefn( featDefn, firstAttrIsFid ? i - 1 : i ) ) { const char *fieldName = OGR_Fld_GetNameRef( field ); - if ( qstrcmp( fieldName, ORIG_OGC_FID ) != 0 ) + // This is implemented a bit in a hacky way, but in case we are acting on a layer + // with a subset filter, do not ignore fields that are found in the + // where clause. We do this in a rough way, by looking, in a case + // insensitive way, if the current field name is in the subsetString, + // so we potentially don't ignore fields we could, in situations like + // subsetFilter == "foobar = 2", and there's a "foo" or "bar" field. + // Better be safe than sorry. + // We could argue that OGR_L_SetIgnoredFields() should be aware of + // the fields of the attribute filter, and do not ignore them. + if ( subsetString.isEmpty() || + subsetString.indexOf( QString::fromUtf8( fieldName ), 0, Qt::CaseInsensitive ) < 0 ) { ignoredFields.append( fieldName ); } @@ -1196,7 +1202,7 @@ QgsRectangle QgsOgrProvider::extent() const mExtent->MaxY = -std::numeric_limits::max(); // TODO: This can be expensive, do we really need it! - if ( mOgrLayer == mOgrOrigLayer.get() ) + if ( mOgrLayer == mOgrOrigLayer.get() && mSubsetString.isEmpty() ) { mOgrLayer->GetExtent( mExtent.get(), true ); } @@ -1853,14 +1859,28 @@ bool QgsOgrProvider::_setSubsetString( const QString &theSQL, bool updateFeature 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(); + if ( layer != subsetLayerH ) + { + mOgrSqlLayer = QgsOgrProviderUtils::getSqlLayer( mOgrOrigLayer.get(), subsetLayerH, theSQL ); + Q_ASSERT( mOgrSqlLayer.get() ); + mOgrLayer = mOgrSqlLayer.get(); + } + else + { + mOgrSqlLayer.reset(); + mOgrLayer = mOgrOrigLayer.get(); + } } else { mOgrSqlLayer.reset(); mOgrLayer = mOgrOrigLayer.get(); + QMutex *mutex = nullptr; + OGRLayerH layer = mOgrOrigLayer->getHandleAndMutex( mutex ); + { + QMutexLocker locker( mutex ); + OGR_L_SetAttributeFilter( layer, nullptr ); + } } mSubsetString = theSQL; @@ -4012,14 +4032,11 @@ OGRwkbGeometryType QgsOgrProvider::ogrWkbSingleFlatten( OGRwkbGeometryType type } } -OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, GDALDatasetH ds, QTextCodec *encoding, const QString &subsetString, bool addOriginalFid, bool *origFidAdded ) +OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, GDALDatasetH ds, QTextCodec *encoding, const QString &subsetString ) { QByteArray layerName = OGR_FD_GetName( OGR_L_GetLayerDefn( layer ) ); GDALDriverH driver = GDALGetDatasetDriver( ds ); QString driverName = GDALGetDriverShortName( driver ); - bool origFidAddAttempted = false; - if ( origFidAdded ) - *origFidAdded = false; if ( driverName == QLatin1String( "ODBC" ) ) //the odbc driver does not like schema names for subset { @@ -4041,56 +4058,8 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, GDALDatasetH ds } else { - QByteArray sqlPart1 = "SELECT *"; - QByteArray sqlPart3 = " FROM " + quotedIdentifier( layerName, driverName ); - if ( !subsetString.isEmpty() ) - sqlPart3 += " WHERE " + encoding->fromUnicode( subsetString ); - - if ( addOriginalFid ) - { - origFidAddAttempted = true; - - QByteArray fidColumn = OGR_L_GetFIDColumn( layer ); - // Fallback to FID if OGR_L_GetFIDColumn returns nothing - if ( fidColumn.isEmpty() ) - { - fidColumn = "FID"; - } - - QByteArray sql = sqlPart1 + ", " + fidColumn + " as \"" + ORIG_OGC_FID + '"' + sqlPart3; - QgsDebugMsg( QStringLiteral( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) ); - subsetLayer = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr ); - - // See https://lists.osgeo.org/pipermail/qgis-developer/2017-September/049802.html - // If execute SQL fails because it did not find the fidColumn, retry with hardcoded FID - if ( !subsetLayer ) - { - QByteArray sql = sqlPart1 + ", " + "FID as \"" + ORIG_OGC_FID + '"' + sqlPart3; - QgsDebugMsg( QStringLiteral( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) ); - subsetLayer = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr ); - } - } - // If that also fails (or we never wanted to add the original FID), just continue without the orig_ogc_fid - if ( !subsetLayer ) - { - QByteArray sql = sqlPart1 + sqlPart3; - QgsDebugMsg( QStringLiteral( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) ); - subsetLayer = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr ); - origFidAddAttempted = false; - } - } - - // Check if last column is orig_ogc_fid - if ( origFidAddAttempted && subsetLayer ) - { - OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( subsetLayer ); - int fieldCount = OGR_FD_GetFieldCount( fdef ); - if ( fieldCount > 0 ) - { - OGRFieldDefnH fldDef = OGR_FD_GetFieldDefn( fdef, fieldCount - 1 ); - if ( origFidAdded ) - *origFidAdded = qstrcmp( OGR_Fld_GetNameRef( fldDef ), ORIG_OGC_FID ) == 0; - } + OGR_L_SetAttributeFilter( layer, encoding->fromUnicode( subsetString ).constData() ); + subsetLayer = layer; } return subsetLayer; @@ -4511,6 +4480,7 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName, ds->hDS, layerIndex ); if ( hLayer ) { + OGR_L_SetAttributeFilter( hLayer, nullptr ); layerName = QString::fromUtf8( OGR_L_GetName( hLayer ) ); } } @@ -4566,6 +4536,7 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName, ds->hDS, layerIndex ); if ( hLayer ) { + OGR_L_SetAttributeFilter( hLayer, nullptr ); layerName = QString::fromUtf8( OGR_L_GetName( hLayer ) ); } } @@ -4649,6 +4620,7 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName, errCause = QObject::tr( "Cannot find layer %1." ).arg( layerName ); return nullptr; } + OGR_L_SetAttributeFilter( hLayer, nullptr ); QgsOgrLayerUniquePtr layer = QgsOgrLayer::CreateForLayer( iter.key(), layerName, ds, hLayer ); @@ -5092,6 +5064,7 @@ QgsOgrLayerUniquePtr QgsOgrProviderUtils::getLayer( const QString &dsName, errCause = QObject::tr( "Cannot find layer %1." ).arg( layerName ); return nullptr; } + OGR_L_SetAttributeFilter( hLayer, nullptr ); QgsOgrLayerUniquePtr layer = QgsOgrLayer::CreateForLayer( ident, layerName, ds, hLayer ); @@ -5239,7 +5212,7 @@ bool QgsOgrDataset::executeSQLNoReturn( const QString &sql ) } -OGRLayerH QgsOgrDataset::createSQLResultLayer( QTextCodec *encoding, const QString &layerName, int layerIndex ) +OGRLayerH QgsOgrDataset::getLayerFromNameOrIndex( const QString &layerName, int layerIndex ) { QMutexLocker locker( &mutex() ); @@ -5252,9 +5225,7 @@ OGRLayerH QgsOgrDataset::createSQLResultLayer( QTextCodec *encoding, const QStri { layer = GDALDatasetGetLayer( mDs->hDS, layerIndex ); } - if ( !layer ) - return nullptr; - return QgsOgrProviderUtils::setSubsetString( layer, mDs->hDS, encoding, QString(), false, nullptr ); + return layer; } void QgsOgrDataset::releaseResultSet( OGRLayerH hSqlLayer ) diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h index 1a061022a7df..92b851727e71 100644 --- a/src/providers/ogr/qgsogrprovider.h +++ b/src/providers/ogr/qgsogrprovider.h @@ -406,15 +406,17 @@ class QgsOgrProviderUtils //! Inject credentials into the dsName (if any) static QString expandAuthConfig( const QString &dsName ); - static void setRelevantFields( OGRLayerH ogrLayer, int fieldCount, bool fetchGeometry, const QgsAttributeList &fetchAttributes, bool firstAttrIsFid ); + static void setRelevantFields( OGRLayerH ogrLayer, int fieldCount, + bool fetchGeometry, + const QgsAttributeList &fetchAttributes, + bool firstAttrIsFid, + const QString &subsetString ); /** * Sets a subset string for an OGR \a layer. - * - * If \a addOriginalFid is specified, then the original OGR feature ID field will be added. If this is successful, - * \a origFidAdded will be set to true. + * Might return either layer, or a new OGR SQL result layer */ - static OGRLayerH setSubsetString( OGRLayerH layer, GDALDatasetH ds, QTextCodec *encoding, const QString &subsetString, bool addOriginalFid = false, bool *origFidAdded = nullptr ); + static OGRLayerH setSubsetString( OGRLayerH layer, GDALDatasetH ds, QTextCodec *encoding, const QString &subsetString ); static QByteArray quotedIdentifier( QByteArray field, const QString &driverName ); /** @@ -509,7 +511,7 @@ class QgsOgrDataset bool executeSQLNoReturn( const QString &sql ); - OGRLayerH createSQLResultLayer( QTextCodec *encoding, const QString &layerName, int layerIndex ); + OGRLayerH getLayerFromNameOrIndex( const QString &layerName, int layerIndex ); void releaseResultSet( OGRLayerH hSqlLayer ); }; diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py index ab9956bbdfcf..88a76f9cbb6b 100644 --- a/tests/src/python/providertestbase.py +++ b/tests/src/python/providertestbase.py @@ -365,6 +365,7 @@ def testExtent(self): self.source.setSubsetString(None) self.assertEqual(count, 0) self.assertTrue(provider_extent.isNull()) + self.assertEqual(self.source.featureCount(), 5) def testUnique(self): self.assertEqual(self.source.uniqueValues(-1), set()) @@ -921,3 +922,29 @@ def testConcurrency(self): context.setFeature(feat) exp = QgsExpression('get_feature(\'{layer}\', \'pk\', 5)'.format(layer=self.vl.id())) exp.evaluate(context) + + def testEmptySubsetOfAttributesWithSubsetString(self): + + if self.source.supportsSubsetString(): + try: + # Add a subset string + subset = self.getSubsetString() + self.source.setSubsetString(subset) + + # First test, in a regular way + features = [f for f in self.source.getFeatures()] + count = len(features) + self.assertEqual(count, 3) + has_geometry = features[0].hasGeometry() + + # Ask for no attributes + request = QgsFeatureRequest().setSubsetOfAttributes([]) + # Make sure we still retrieve features ! + features = [f for f in self.source.getFeatures(request)] + count = len(features) + self.assertEqual(count, 3) + # Check that we still get a geometry if we add one before + self.assertEqual(features[0].hasGeometry(), has_geometry) + + finally: + self.source.setSubsetString(None) diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py index b3d9cc34fcfd..3298d2d3bb6e 100644 --- a/tests/src/python/test_provider_ogr_gpkg.py +++ b/tests/src/python/test_provider_ogr_gpkg.py @@ -279,17 +279,44 @@ def testSelectSubsetString(self): vl.setSubsetString("SELECT fid, foo FROM test WHERE foo = 'baz'") got = [feat for feat in vl.getFeatures()] self.assertEqual(len(got), 1) + del vl testdata_path = unitTestDataPath('provider') - gpkg = os.path.join(testdata_path, 'bug_19826.gpkg') - vl = QgsVectorLayer('{}|layerid=0'.format(gpkg, 'test', 'ogr')) + shutil.copy(os.path.join(testdata_path, 'bug_19826.gpkg'), tmpfile) + vl = QgsVectorLayer('{}|layerid=0'.format(tmpfile, 'test', 'ogr')) vl.setSubsetString("name = 'two'") got = [feat for feat in vl.getFeatures()] self.assertEqual(len(got), 1) attributes = got[0].attributes() + self.assertEqual(got[0].id(), 2) self.assertEqual(attributes[0], 2) self.assertEqual(attributes[1], 'two') + self.assertNotEqual(attributes[2], None) + + # Request by FeatureId on a subset layer + got = [feat for feat in vl.getFeatures(QgsFeatureRequest(2))] + self.assertEqual(len(got), 1) + attributes = got[0].attributes() + self.assertEqual(got[0].id(), 2) + self.assertEqual(attributes[0], 2) + self.assertEqual(attributes[1], 'two') + self.assertNotEqual(attributes[2], None) + + request = QgsFeatureRequest(2).setSubsetOfAttributes([0]) + got = [feat for feat in vl.getFeatures(request)] + self.assertEqual(len(got), 1) + attributes = got[0].attributes() + self.assertEqual(got[0].id(), 2) + self.assertEqual(attributes[0], 2) + self.assertEqual(attributes[1], None) + self.assertEqual(attributes[2], None) + + # Request by FeatureId on a subset layer. The name = 'two' filter + # only returns FID 2, so requesting on FID 1 should return nothing + # but this is broken now. + got = [feat for feat in vl.getFeatures(QgsFeatureRequest(1))] + self.assertEqual(len(got), 1) # this is the current behavior, broken def testStyle(self): @@ -896,8 +923,8 @@ 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""" + def testSubSetStringEditable_bug17795_but_with_modified_behavior(self): + """Test that a layer is editable after setting a subset""" tmpfile = os.path.join(self.basetestpath, 'testSubSetStringEditable_bug17795.gpkg') shutil.copy(TEST_DATA_DIR + '/' + 'provider/bug_17795.gpkg', tmpfile) @@ -917,7 +944,7 @@ def testSubSetStringEditable_bug17795(self): vl = QgsVectorLayer(testPath, 'subset_test', 'ogr') vl.setSubsetString('"category" = \'one\'') self.assertTrue(vl.isValid()) - self.assertFalse(vl.dataProvider().capabilities() & isEditable) + self.assertTrue(vl.dataProvider().capabilities() & isEditable) vl.setSubsetString('') self.assertTrue(vl.dataProvider().capabilities() & isEditable) diff --git a/tests/src/python/test_provider_shapefile.py b/tests/src/python/test_provider_shapefile.py index d833c8d6402b..322b26c63e61 100644 --- a/tests/src/python/test_provider_shapefile.py +++ b/tests/src/python/test_provider_shapefile.py @@ -614,8 +614,8 @@ 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""" + def testSubSetStringEditable_bug17795_but_with_modified_behavior(self): + """Test that a layer is still editable after setting a subset""" testPath = TEST_DATA_DIR + '/' + 'lines.shp' isEditable = QgsVectorDataProvider.ChangeAttributeValues @@ -632,7 +632,7 @@ def testSubSetStringEditable_bug17795(self): vl = QgsVectorLayer(testPath, 'subset_test', 'ogr') vl.setSubsetString('"Name" = \'Arterial\'') self.assertTrue(vl.isValid()) - self.assertFalse(vl.dataProvider().capabilities() & isEditable) + self.assertTrue(vl.dataProvider().capabilities() & isEditable) vl.setSubsetString('') self.assertTrue(vl.dataProvider().capabilities() & isEditable)