Merge pull request #9527 from manisandro/master

manisandro · web-flow · commit ba416d2eefe8 · 2019-03-20T00:05:53.000+01:00
Misc crash fixes
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/qgsvectorlayer.cpp b/src/core/qgsvectorlayer.cpp
@@ -1594,6 +1594,7 @@ bool QgsVectorLayer::setDataProvider( QString const &provider, const QgsDataProv
   if ( !mValid )
   {
     QgsDebugMsgLevel( QStringLiteral( "Invalid provider plugin %1" ).arg( QString( mDataSource.toUtf8() ) ), 2 );
+    return false;
   }
 
   if ( mDataProvider->capabilities() & QgsVectorDataProvider::ReadLayerMetadata )
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 );
       }
     }
 

Original file line number	Diff line number	Diff line change
`@@ -1594,6 +1594,7 @@ bool QgsVectorLayer::setDataProvider( QString const &provider, const QgsDataProv`
`1594`	`1594`	`if ( !mValid )`
`1595`	`1595`	`{`
`1596`	`1596`	`QgsDebugMsgLevel( QStringLiteral( "Invalid provider plugin %1" ).arg( QString( mDataSource.toUtf8() ) ), 2 );`
	`1597`	`+ return false;`
`1597`	`1598`	`}`
`1598`	`1599`
`1599`	`1600`	`if ( mDataProvider->capabilities() & QgsVectorDataProvider::ReadLayerMetadata )`
Original file line number	Diff line number	Diff line change
`@@ -189,9 +189,10 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat`
`189`	`189`	`// and only modify the subset if we cannot.`
`190`	`190`	`if ( !mProviderRequest.orderBy().isEmpty() )`
`191`	`191`	`{`
`192`		`- Q_FOREACH ( const QString &attr, mProviderRequest.orderBy().usedAttributes() )`
	`192`	`+ const auto usedAttributeIndices = mProviderRequest.orderBy().usedAttributeIndices( mSource->mFields );`
	`193`	`+ for ( int attrIndex : usedAttributeIndices )`
`193`	`194`	`{`
`194`		`- providerSubset << mSource->mFields.lookupField( attr );`
	`195`	`+ providerSubset << attrIndex;`
`195`	`196`	`}`
`196`	`197`	`}`
`197`	`198`
Original file line number	Diff line number	Diff line change
`@@ -161,9 +161,9 @@ QgsDelimitedTextFeatureIterator::QgsDelimitedTextFeatureIterator( QgsDelimitedTe`
`161`	`161`	`if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )`
`162`	`162`	`{`
`163`	`163`	`QgsAttributeList attrs = request.subsetOfAttributes();`
`164`		`- Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )`
	`164`	`+ const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );`
	`165`	`+ for ( int attrIndex : usedAttributeIndices )`
`165`	`166`	`{`
`166`		`- int attrIndex = mSource->mFields.lookupField( attr );`
`167`	`167`	`if ( !attrs.contains( attrIndex ) )`
`168`	`168`	`attrs << attrIndex;`
`169`	`169`	`}`
Original file line number	Diff line number	Diff line change
`@@ -95,10 +95,9 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest &request )`
`95`	`95`	`}`
`96`	`96`
`97`	`97`	`// ensure that all attributes required for order by are fetched`
`98`		`- const QSet< QString > orderByAttributes = mRequest.orderBy().usedAttributes();`
`99`		`- for ( const QString &attr : orderByAttributes )`
	`98`	`+ const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );`
	`99`	`+ for ( int attrIndex : usedAttributeIndices )`
`100`	`100`	`{`
`101`		`- int attrIndex = mSource->mFields.lookupField( attr );`
`102`	`101`	`if ( !attrs.contains( attrIndex ) )`
`103`	`102`	`attrs << attrIndex;`
`104`	`103`	`}`
Original file line number	Diff line number	Diff line change
`@@ -136,9 +136,10 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool`
`136`	`136`	`if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )`
`137`	`137`	`{`
`138`	`138`	`QSet<int> attributeIndexes;`
`139`		`- Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )`
	`139`	`+ const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );`
	`140`	`+ for ( int attrIdx : usedAttributeIndices )`
`140`	`141`	`{`
`141`		`- attributeIndexes << mSource->mFields.lookupField( attr );`
	`142`	`+ attributeIndexes << attrIdx;`
`142`	`143`	`}`
`143`	`144`	`attributeIndexes += attrs.toSet();`
`144`	`145`	`attrs = attributeIndexes.toList();`
Original file line number	Diff line number	Diff line change
`@@ -186,9 +186,9 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource`
`186`	`186`	`if ( !mOrderByCompiled && mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )`
`187`	`187`	`{`
`188`	`188`	`QgsAttributeList attrs = mRequest.subsetOfAttributes();`
`189`		`- Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )`
	`189`	`+ const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );`
	`190`	`+ for ( int attrIndex : usedAttributeIndices )`
`190`	`191`	`{`
`191`		`- int attrIndex = mSource->mFields.lookupField( attr );`
`192`	`192`	`if ( !attrs.contains( attrIndex ) )`
`193`	`193`	`attrs << attrIndex;`
`194`	`194`	`}`
Original file line number	Diff line number	Diff line change
`@@ -196,9 +196,10 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature`
`196`	`196`	`if ( !mOrderByCompiled && mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )`
`197`	`197`	`{`
`198`	`198`	`QSet<int> attributeIndexes;`
`199`		`- Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )`
	`199`	`+ const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );`
	`200`	`+ for ( int attrIdx : usedAttributeIndices )`
`200`	`201`	`{`
`201`		`- attributeIndexes << mSource->mFields.lookupField( attr );`
	`202`	`+ attributeIndexes << attrIdx;`
`202`	`203`	`}`
`203`	`204`	`attributeIndexes += mRequest.subsetOfAttributes().toSet();`
`204`	`205`	`mRequest.setSubsetOfAttributes( attributeIndexes.toList() );`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,8 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF`
`128`	`128`	`if ( request.flags() & QgsFeatureRequest::SubsetOfAttributes )`
`129`	`129`	`{`
`130`	`130`	`// copy only selected fields`
`131`		`- Q_FOREACH ( int idx, request.subsetOfAttributes() )`
	`131`	`+ const auto subsetOfAttributes = request.subsetOfAttributes();`
	`132`	`+ for ( int idx : subsetOfAttributes )`
`132`	`133`	`{`
`133`	`134`	`mAttributes << idx;`
`134`	`135`	`}`
`@@ -147,9 +148,9 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF`
`147`	`148`	`// also need attributes required by order by`
`148`	`149`	`if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )`
`149`	`150`	`{`
`150`		`- Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )`
	`151`	`+ const auto usedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mSource->mFields );`
	`152`	`+ for ( int attrIdx : usedAttributeIndices )`
`151`	`153`	`{`
`152`		`- int attrIdx = mSource->mFields.lookupField( attr );`
`153`	`154`	`if ( !mAttributes.contains( attrIdx ) )`
`154`	`155`	`mAttributes << attrIdx;`
`155`	`156`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1030,7 +1030,8 @@ QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )`
`1030`	`1030`	`{`
`1031`	`1031`	`QgsFields dataProviderFields = mShared->mCacheDataProvider->fields();`
`1032`	`1032`	`QgsAttributeList cacheSubSet;`
`1033`		`- Q_FOREACH ( int i, mRequest.subsetOfAttributes() )`
	`1033`	`+ const auto subsetOfAttributes = mRequest.subsetOfAttributes();`
	`1034`	`+ for ( int i : subsetOfAttributes )`
`1034`	`1035`	`{`
`1035`	`1036`	`int idx = dataProviderFields.indexFromName( mShared->mFields.at( i ).name() );`
`1036`	`1037`	`if ( idx >= 0 )`
`@@ -1043,7 +1044,8 @@ QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )`
`1043`	`1044`	`// ensure that all attributes required for expression filter are being fetched`
`1044`	`1045`	`if ( mRequest.filterType() == QgsFeatureRequest::FilterExpression )`
`1045`	`1046`	`{`
`1046`		`- Q_FOREACH ( const QString &field, mRequest.filterExpression()->referencedColumns() )`
	`1047`	`+ const auto referencedColumns = mRequest.filterExpression()->referencedColumns();`
	`1048`	`+ for ( const QString &field : referencedColumns )`
`1047`	`1049`	`{`
`1048`	`1050`	`int idx = dataProviderFields.indexFromName( field );`
`1049`	`1051`	`if ( idx >= 0 && !cacheSubSet.contains( idx ) )`
`@@ -1057,15 +1059,18 @@ QgsFeatureRequest QgsWFSFeatureIterator::buildRequestCache( int genCounter )`
`1057`	`1059`	`// also need attributes required by order by`
`1058`	`1060`	`if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )`
`1059`	`1061`	`{`
`1060`		`- Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )`
	`1062`	`+ const auto usedProviderAttributeIndices = mRequest.orderBy().usedAttributeIndices( dataProviderFields );`
	`1063`	`+ for ( int attrIdx : usedProviderAttributeIndices )`
`1061`	`1064`	`{`
`1062`		`- int idx = dataProviderFields.indexFromName( attr );`
`1063`		`- if ( idx >= 0 && !cacheSubSet.contains( idx ) )`
`1064`		`- cacheSubSet.append( idx );`
	`1065`	`+ if ( !cacheSubSet.contains( attrIdx ) )`
	`1066`	`+ cacheSubSet.append( attrIdx );`
	`1067`	`+ }`
`1065`	`1068`
`1066`		`- idx = mShared->mFields.indexFromName( attr );`
`1067`		`- if ( idx >= 0 && !mSubSetAttributes.contains( idx ) )`
`1068`		`- mSubSetAttributes.append( idx );`
	`1069`	`+ const auto usedSharedAttributeIndices = mRequest.orderBy().usedAttributeIndices( mShared->mFields );`
	`1070`	`+ for ( int attrIdx : usedSharedAttributeIndices )`
	`1071`	`+ {`
	`1072`	`+ if ( !mSubSetAttributes.contains( attrIdx ) )`
	`1073`	`+ mSubSetAttributes.append( attrIdx );`
`1069`	`1074`	`}`
`1070`	`1075`	`}`
`1071`	`1076`