From 391ec8a5dd4508f75b6538ab7be309379695add4 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Thu, 18 Oct 2018 00:54:27 +0200 Subject: [PATCH] [OGR provider] Revise significantly the way we handle subset filter to avoid issues with FID (fixes #20136) Some rationale on this change... Previously when applying a "regular" subset string, ie. one that is only the content of a where clause, we issued a full "SELECT * FROM layer WHERE subsetstring", resulting in a OGR SQL layer. The caveat of that is that most OGR drivers will have issues retaining the original FID. A hack consisting in adding a {original_fid_name} as orig_ogc_fid to the select columns was introduced in https://github.com/qgis/QGIS/commit/4ce2cf1744b008043403b18b8def8f18c99d14f1 to try to retain the original FID, but this added a lot of complexity. And actually, in the case of the OGR GPKG driver, it caused it to still be confused when analyzing the column definition of the resulting layer, since it sees 2 FID columns despite the renaming (one included in the '*' wildcard, and the one of orig_ogc_fid), which caused it to use sequential FID numbering (the driver when seeing more than once a column that is the FID column assumes that some cross join is done, and thus that FID are unreliable) A simpler and more robust (crossing fingers!) approach in that case is just to use OGR_L_SetAttributeFilter() instead of GDALDatasetExecuteSQL(). Some care must be taken to cancel the filter when removing the subset filter, or in QgsOgrFeatureIterator when combining with the filter expression coming from the request, but besides that, this is more straightforward, and actually solves #20136 --- src/providers/ogr/qgsogrfeatureiterator.cpp | 88 ++++++--------- src/providers/ogr/qgsogrfeatureiterator.h | 7 +- src/providers/ogr/qgsogrprovider.cpp | 117 ++++++++------------ src/providers/ogr/qgsogrprovider.h | 14 ++- tests/src/python/providertestbase.py | 27 +++++ tests/src/python/test_provider_ogr_gpkg.py | 37 ++++++- tests/src/python/test_provider_shapefile.py | 6 +- 7 files changed, 147 insertions(+), 149 deletions(-) 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)