[Bugfix] Ensure order by attribute indices are valid

qgis · Mar 19, 2019 · 0996c93 · 0996c93
1 parent 61b2f91
commit 0996c93
Show file tree

Hide file tree

Showing 11 changed files with 69 additions and 25 deletions.
diff --git a/python/core/auto_generated/qgsfeaturerequest.sip.in b/python/core/auto_generated/qgsfeaturerequest.sip.in
@@ -243,6 +243,17 @@ Deserialize from XML
         QSet<QString> usedAttributes() const;
 %Docstring
 Returns a set of used attributes
+
+.. note::
+
+   The returned attributes names are NOT guaranteed to be valid.
+%End
+
+        QSet<int> usedAttributeIndices( const QgsFields &fields ) const;
+%Docstring
+Returns a set of used, validated attribute indices
+
+.. versionadded:: 3.8
 %End
 
         QString dump() const;

diff --git a/src/core/qgsfeaturerequest.cpp b/src/core/qgsfeaturerequest.cpp
@@ -485,6 +485,24 @@ QSet<QString> QgsFeatureRequest::OrderBy::usedAttributes() const
   return usedAttributes;
 }
 
+QSet<int> QgsFeatureRequest::OrderBy::usedAttributeIndices( const QgsFields &fields ) const
+{
+  QSet<int> usedAttributeIdx;
+  for ( const OrderByClause &clause : *this )
+  {
+    const auto referencedColumns = clause.expression().referencedColumns();
+    for ( const QString &fieldName : referencedColumns )
+    {
+      int idx = fields.lookupField( fieldName );
+      if ( idx >= 0 )
+      {
+        usedAttributeIdx.insert( idx );
+      }
+    }
+  }
+  return usedAttributeIdx;
+}
+
 QString QgsFeatureRequest::OrderBy::dump() const
 {
   QStringList results;

diff --git a/src/core/qgsfeaturerequest.h b/src/core/qgsfeaturerequest.h
@@ -257,9 +257,16 @@ class CORE_EXPORT QgsFeatureRequest
 
         /**
          * Returns a set of used attributes
+         * \note The returned attributes names are NOT guaranteed to be valid.
          */
         QSet<QString> CORE_EXPORT usedAttributes() const;
 
+        /**
+         * Returns a set of used, validated attribute indices
+         * \since QGIS 3.8
+         */
+        QSet<int> CORE_EXPORT usedAttributeIndices( const QgsFields &fields ) const;
+
         /**
          * Dumps the content to an SQL equivalent syntax
          */

diff --git a/src/core/qgsvectorlayerfeatureiterator.cpp b/src/core/qgsvectorlayerfeatureiterator.cpp
@@ -189,9 +189,10 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
     // and only modify the subset if we cannot.
     if ( !mProviderRequest.orderBy().isEmpty() )
     {
-      Q_FOREACH ( const QString &attr, mProviderRequest.orderBy().usedAttributes() )
+      const auto usedAttributeIndices = mProviderRequest.orderBy().usedAttributeIndices( mSource->mFields );
+      for ( int attrIndex : usedAttributeIndices )
       {
-        providerSubset << mSource->mFields.lookupField( attr );
+        providerSubset << attrIndex;
       }
     }
 

diff --git a/src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp b/src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp
@@ -161,9 +161,9 @@ QgsDelimitedTextFeatureIterator::QgsDelimitedTextFeatureIterator( QgsDelimitedTe
   if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
   {
     QgsAttributeList attrs = request.subsetOfAttributes();
-    Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
+    const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );
+    for ( int attrIndex : usedAttributeIndices )
     {
-      int attrIndex = mSource->mFields.lookupField( attr );
       if ( !attrs.contains( attrIndex ) )
         attrs << attrIndex;
     }

diff --git a/src/providers/mssql/qgsmssqlfeatureiterator.cpp b/src/providers/mssql/qgsmssqlfeatureiterator.cpp
@@ -95,10 +95,9 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest &request )
     }
 
     // ensure that all attributes required for order by are fetched
-    const QSet< QString > orderByAttributes = mRequest.orderBy().usedAttributes();
-    for ( const QString &attr : orderByAttributes )
+    const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );
+    for ( int attrIndex : usedAttributeIndices )
     {
-      int attrIndex = mSource->mFields.lookupField( attr );
       if ( !attrs.contains( attrIndex ) )
         attrs << attrIndex;
     }

diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -136,9 +136,10 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
   if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
   {
     QSet<int> attributeIndexes;
-    Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
+    const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );
+    for ( int attrIdx : usedAttributeIndices )
     {
-      attributeIndexes << mSource->mFields.lookupField( attr );
+      attributeIndexes << attrIdx;
     }
     attributeIndexes += attrs.toSet();
     attrs = attributeIndexes.toList();

diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp
@@ -186,9 +186,9 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
     if ( !mOrderByCompiled && mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
     {
       QgsAttributeList attrs = mRequest.subsetOfAttributes();
-      Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
+      const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );
+      for ( int attrIndex : usedAttributeIndices )
       {
-        int attrIndex = mSource->mFields.lookupField( attr );
         if ( !attrs.contains( attrIndex ) )
           attrs << attrIndex;
       }

diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp
@@ -196,9 +196,10 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
     if ( !mOrderByCompiled && mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
     {
       QSet<int> attributeIndexes;
-      Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
+      const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );
+      for ( int attrIdx : usedAttributeIndices )
       {
-        attributeIndexes << mSource->mFields.lookupField( attr );
+        attributeIndexes << attrIdx;
       }
       attributeIndexes += mRequest.subsetOfAttributes().toSet();
       mRequest.setSubsetOfAttributes( attributeIndexes.toList() );

diff --git a/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp b/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp
@@ -128,7 +128,8 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
     if ( request.flags() & QgsFeatureRequest::SubsetOfAttributes )
     {
       // copy only selected fields
-      Q_FOREACH ( int idx, request.subsetOfAttributes() )
+      const auto subsetOfAttributes = request.subsetOfAttributes();
+      for ( int idx : subsetOfAttributes )
       {
         mAttributes << idx;
       }
@@ -147,9 +148,9 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
       // also need attributes required by order by
       if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
       {
-        Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
+        const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );
+        for ( int attrIdx : usedAttributeIndices )
         {
-          int attrIdx = mSource->mFields.lookupField( attr );
           if ( !mAttributes.contains( attrIdx ) )
             mAttributes << attrIdx;
         }

diff --git a/src/providers/wfs/qgswfsfeatureiterator.cpp b/src/providers/wfs/qgswfsfeatureiterator.cpp
@@ -1030,7 +1030,8 @@ QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )
   {
     QgsFields dataProviderFields = mShared->mCacheDataProvider->fields();
     QgsAttributeList cacheSubSet;
-    Q_FOREACH ( int i, mRequest.subsetOfAttributes() )
+    const auto subsetOfAttributes = mRequest.subsetOfAttributes();
+    for ( int i : subsetOfAttributes )
     {
       int idx = dataProviderFields.indexFromName( mShared->mFields.at( i ).name() );
       if ( idx >= 0 )
@@ -1043,7 +1044,8 @@ QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )
     // ensure that all attributes required for expression filter are being fetched
     if ( mRequest.filterType() == QgsFeatureRequest::FilterExpression )
     {
-      Q_FOREACH ( const QString &field, mRequest.filterExpression()->referencedColumns() )
+      const auto referencedColumns = mRequest.filterExpression()->referencedColumns();
+      for ( const QString &field : referencedColumns )
       {
         int idx = dataProviderFields.indexFromName( field );
         if ( idx >= 0 && !cacheSubSet.contains( idx ) )
@@ -1057,15 +1059,18 @@ QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )
     // also need attributes required by order by
     if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
     {
-      Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
+      const auto usedProviderAttributeIndices = mRequest.orderBy().usedAttributeIndices( dataProviderFields );
+      for ( int attrIdx : usedProviderAttributeIndices )
       {
-        int idx = dataProviderFields.indexFromName( attr );
-        if ( idx >= 0 && !cacheSubSet.contains( idx ) )
-          cacheSubSet.append( idx );
+        if ( !cacheSubSet.contains( attrIdx ) )
+          cacheSubSet.append( attrIdx );
+      }
 
-        idx = mShared->mFields.indexFromName( attr );
-        if ( idx >= 0  && !mSubSetAttributes.contains( idx ) )
-          mSubSetAttributes.append( idx );
+      const auto usedSharedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mShared->mFields );
+      for ( int attrIdx : usedSharedAttributeIndices )
+      {
+        if ( !mSubSetAttributes.contains( attrIdx ) )
+          mSubSetAttributes.append( attrIdx );
       }
     }