Partial fix for virtual layer iterator depending on provider

The proper fix is more involved and requires reworking of the sqlite handle to utilise a shared pointer. Without this the iterator will return no features if the provider is removed while a source is active (i.e. removing virtual layer while map is rendering). But at least it avoids a crash in this circumstance...
qgis · Apr 23, 2017 · c85a437 · c85a437
1 parent 0dfe687
commit c85a437
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 33 deletions.
diff --git a/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp b/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp
@@ -29,43 +29,52 @@ static QString quotedColumn( QString name )
 QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
   : QgsAbstractFeatureIteratorFromSource<QgsVirtualLayerFeatureSource>( source, ownSource, request )
 {
-  try
+
+  // NOTE: this is really bad and should be removed.
+  // it's only here to guard mSource->mSqlite - because if the provider is removed
+  // then mSqlite will be meaningless.
+  // this needs to be totally reworked so that mSqlite no longer depends on the provider
+  // and can be fully encapsulated in the source
+  if ( !mSource->mProvider )
   {
-    mSqlite = mSource->provider()->mSqlite.get();
-    mDefinition = mSource->provider()->mDefinition;
+    close();
+    return;
+  }
 
-    QString tableName = mSource->provider()->mTableName;
+  try
+  {
+    QString tableName = mSource->mTableName;
 
     QStringList wheres;
-    QString subset = mSource->provider()->mSubset;
+    QString subset = mSource->mSubset;
     if ( !subset.isNull() )
     {
       wheres << subset;
     }
 
-    if ( !mDefinition.uid().isNull() )
+    if ( !mSource->mDefinition.uid().isNull() )
     {
       // filters are only available when a column with unique id exists
-      if ( mDefinition.hasDefinedGeometry() && !request.filterRect().isNull() )
+      if ( mSource->mDefinition.hasDefinedGeometry() && !request.filterRect().isNull() )
       {
         bool do_exact = request.flags() & QgsFeatureRequest::ExactIntersect;
         QgsRectangle rect( request.filterRect() );
         QString mbr = QStringLiteral( "%1,%2,%3,%4" ).arg( rect.xMinimum() ).arg( rect.yMinimum() ).arg( rect.xMaximum() ).arg( rect.yMaximum() );
-        wheres << quotedColumn( mDefinition.geometryField() ) + " is not null";
+        wheres << quotedColumn( mSource->mDefinition.geometryField() ) + " is not null";
         wheres <<  QStringLiteral( "%1Intersects(%2,BuildMbr(%3))" )
                .arg( do_exact ? "" : "Mbr",
-                     quotedColumn( mDefinition.geometryField() ),
+                     quotedColumn( mSource->mDefinition.geometryField() ),
                      mbr );
       }
       else if ( request.filterType() == QgsFeatureRequest::FilterFid )
       {
         wheres << QStringLiteral( "%1=%2" )
-               .arg( quotedColumn( mDefinition.uid() ) )
+               .arg( quotedColumn( mSource->mDefinition.uid() ) )
                .arg( request.filterFid() );
       }
       else if ( request.filterType() == QgsFeatureRequest::FilterFids )
       {
-        QString values = quotedColumn( mDefinition.uid() ) + " IN (";
+        QString values = quotedColumn( mSource->mDefinition.uid() ) + " IN (";
         bool first = true;
         Q_FOREACH ( QgsFeatureId v, request.filterFids() )
         {
@@ -81,7 +90,6 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
       }
     }
 
-    mFields = mSource->provider()->fields();
     if ( request.flags() & QgsFeatureRequest::SubsetOfAttributes )
     {
       // copy only selected fields
@@ -95,23 +103,23 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
       {
         Q_FOREACH ( const QString &field, request.filterExpression()->referencedColumns() )
         {
-          int attrIdx = mFields.lookupField( field );
+          int attrIdx = mSource->mFields.lookupField( field );
           if ( !mAttributes.contains( attrIdx ) )
             mAttributes << attrIdx;
         }
       }
     }
     else
     {
-      mAttributes = mFields.allAttributesList();
+      mAttributes = mSource->mFields.allAttributesList();
     }
 
     QString columns;
     {
       // the first column is always the uid (or 0)
-      if ( !mDefinition.uid().isNull() )
+      if ( !mSource->mDefinition.uid().isNull() )
       {
-        columns = quotedColumn( mDefinition.uid() );
+        columns = quotedColumn( mSource->mDefinition.uid() );
       }
       else
       {
@@ -120,16 +128,16 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
       Q_FOREACH ( int i, mAttributes )
       {
         columns += QLatin1String( "," );
-        QString cname = mFields.at( i ).name().toLower();
+        QString cname = mSource->mFields.at( i ).name().toLower();
         columns += quotedColumn( cname );
       }
     }
     // the last column is the geometry, if any
     if ( ( !( request.flags() & QgsFeatureRequest::NoGeometry )
            || ( request.filterType() == QgsFeatureRequest::FilterExpression && request.filterExpression()->needsGeometry() ) )
-         && !mDefinition.geometryField().isNull() && mDefinition.geometryField() != QLatin1String( "*no*" ) )
+         && !mSource->mDefinition.geometryField().isNull() && mSource->mDefinition.geometryField() != QLatin1String( "*no*" ) )
     {
-      columns += "," + quotedColumn( mDefinition.geometryField() );
+      columns += "," + quotedColumn( mSource->mDefinition.geometryField() );
     }
 
     mSqlQuery = "SELECT " + columns + " FROM " + tableName;
@@ -138,7 +146,7 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
       mSqlQuery += " WHERE " + wheres.join( QStringLiteral( " AND " ) );
     }
 
-    mQuery.reset( new Sqlite::Query( mSqlite, mSqlQuery ) );
+    mQuery.reset( new Sqlite::Query( mSource->mSqlite, mSqlQuery ) );
 
     mFid = 0;
   }
@@ -193,9 +201,9 @@ bool QgsVirtualLayerFeatureIterator::fetchFeature( QgsFeature &feature )
     return false;
   }
 
-  feature.setFields( mFields, /* init */ true );
+  feature.setFields( mSource->mFields, /* init */ true );
 
-  if ( mDefinition.uid().isNull() )
+  if ( mSource->mDefinition.uid().isNull() )
   {
     // no id column => autoincrement
     feature.setId( mFid++ );
@@ -246,6 +254,11 @@ bool QgsVirtualLayerFeatureIterator::fetchFeature( QgsFeature &feature )
 
 QgsVirtualLayerFeatureSource::QgsVirtualLayerFeatureSource( const QgsVirtualLayerProvider *p )
   : mProvider( p )
+  , mDefinition( p->mDefinition )
+  , mFields( p->fields() )
+  , mSqlite( p->mSqlite.get() )
+  , mTableName( p->mTableName )
+  , mSubset( p->mSubset )
 {
 }
 

diff --git a/src/providers/virtual/qgsvirtuallayerfeatureiterator.h b/src/providers/virtual/qgsvirtuallayerfeatureiterator.h
@@ -21,6 +21,7 @@ email                : hugo dot mercier at oslandia dot com
 #include <qgsvirtuallayerprovider.h>
 #include <qgsfeatureiterator.h>
 #include <memory>
+#include <QPointer>
 
 class QgsVirtualLayerFeatureSource : public QgsAbstractFeatureSource
 {
@@ -30,9 +31,23 @@ class QgsVirtualLayerFeatureSource : public QgsAbstractFeatureSource
 
     virtual QgsFeatureIterator getFeatures( const QgsFeatureRequest &request ) override;
 
-    const QgsVirtualLayerProvider *provider() const { return mProvider; }
   private:
-    const QgsVirtualLayerProvider *mProvider = nullptr;
+
+    // NOTE: this is really bad and should be removed.
+    // it's only here to guard mSqlite - because if the provider is removed
+    // then mSqlite will be meaningless.
+    // this needs to be totally reworked so that mSqlite no longer depends on the provider
+    // and can be fully encapsulated here
+    QPointer< const QgsVirtualLayerProvider  > mProvider;
+
+    QString mPath;
+    QgsVirtualLayerDefinition mDefinition;
+    QgsFields mFields;
+    sqlite3 *mSqlite = nullptr;
+    QString mTableName;
+    QString mSubset;
+
+    friend class QgsVirtualLayerFeatureIterator;
 };
 
 class QgsVirtualLayerFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsVirtualLayerFeatureSource>
@@ -50,16 +65,10 @@ class QgsVirtualLayerFeatureIterator : public QgsAbstractFeatureIteratorFromSour
 
     std::unique_ptr<Sqlite::Query> mQuery;
 
-    QgsFeatureId mFid;
-
-    QString mPath;
-    sqlite3 *mSqlite = nullptr;
-    QgsVirtualLayerDefinition mDefinition;
-    QgsFields mFields;
-
+    QgsAttributeList mAttributes;
     QString mSqlQuery;
+    QgsFeatureId mFid;
 
-    QgsAttributeList mAttributes;
 };
 
 #endif
diff --git a/src/providers/virtual/qgsvirtuallayerprovider.h b/src/providers/virtual/qgsvirtuallayerprovider.h
@@ -113,7 +113,7 @@ class QgsVirtualLayerProvider: public QgsVectorDataProvider
     bool createIt();
     bool loadSourceLayers();
 
-    friend class QgsVirtualLayerFeatureIterator;
+    friend class QgsVirtualLayerFeatureSource;
 
   private slots:
     void invalidateStatistics();