Mark thread-unsafe methods as deprecated, to avoid their further

use in expression functions
qgis · Dec 13, 2022 · e6f1254 · e6f1254
1 parent 7dfdfb2
commit e6f1254
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 11 deletions.
diff --git a/src/core/expression/qgsexpressioncontextutils.cpp b/src/core/expression/qgsexpressioncontextutils.cpp
@@ -34,6 +34,7 @@
 #include "qgsmarkersymbol.h"
 #include "qgstriangularmesh.h"
 #include "qgsvectortileutils.h"
+#include "qgsmeshlayer.h"
 
 QgsExpressionContextScope *QgsExpressionContextUtils::globalScope()
 {
@@ -965,7 +966,9 @@ QVariant QgsExpressionContextUtils::GetLayerVisibility::func( const QVariantList
   }
 
   bool isVisible = false;
+  Q_NOWARN_DEPRECATED_PUSH
   QgsMapLayer *layer = QgsExpressionUtils::getMapLayer( values.at( 0 ), context, parent );
+  Q_NOWARN_DEPRECATED_POP
   if ( layer && mLayers.contains( layer ) )
   {
     isVisible = true;

diff --git a/src/core/expression/qgsexpressionfunction.cpp b/src/core/expression/qgsexpressionfunction.cpp
@@ -601,7 +601,11 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
   ENSURE_NO_EVAL_ERROR
   QVariant value = node->eval( parent, context );
   ENSURE_NO_EVAL_ERROR
+
+  // TODO this expression function is NOT thread safe
+  Q_NOWARN_DEPRECATED_PUSH
   QgsVectorLayer *vl = QgsExpressionUtils::getVectorLayer( value, context, parent );
+  Q_NOWARN_DEPRECATED_POP
   if ( !vl )
   {
     parent->setEvalErrorString( QObject::tr( "Cannot find layer with name or ID '%1'" ).arg( value.toString() ) );
@@ -745,7 +749,11 @@ static QVariant fcnAggregateRelation( const QVariantList &values, const QgsExpre
   }
 
   // first step - find current layer
+
+  // TODO this expression function is NOT thread safe
+  Q_NOWARN_DEPRECATED_PUSH
   QgsVectorLayer *vl = QgsExpressionUtils::getVectorLayer( context->variable( QStringLiteral( "layer" ) ), context, parent );
+  Q_NOWARN_DEPRECATED_POP
   if ( !vl )
   {
     parent->setEvalErrorString( QObject::tr( "Cannot use relation aggregate function in this context" ) );
@@ -868,7 +876,11 @@ static QVariant fcnAggregateGeneric( QgsAggregateCalculator::Aggregate aggregate
   }
 
   // first step - find current layer
+
+  // TODO this expression function is NOT thread safe
+  Q_NOWARN_DEPRECATED_PUSH
   QgsVectorLayer *vl = QgsExpressionUtils::getVectorLayer( context->variable( QStringLiteral( "layer" ) ), context, parent );
+  Q_NOWARN_DEPRECATED_POP
   if ( !vl )
   {
     parent->setEvalErrorString( QObject::tr( "Cannot use aggregate function in this context" ) );
@@ -1905,6 +1917,8 @@ static QVariant fcnRepresentAttributes( const QVariantList &values, const QgsExp
   QgsVectorLayer *layer = nullptr;
   QgsFeature feature;
 
+  // TODO this expression function is NOT thread safe
+  Q_NOWARN_DEPRECATED_PUSH
   if ( values.isEmpty() )
   {
     feature = context->feature();
@@ -1925,6 +1939,7 @@ static QVariant fcnRepresentAttributes( const QVariantList &values, const QgsExp
     parent->setEvalErrorString( QObject::tr( "Function `represent_attributes` requires no more than two parameters. %n given.", nullptr, values.length() ) );
     return QVariant();
   }
+  Q_NOWARN_DEPRECATED_POP
 
   if ( !layer )
   {
@@ -1988,6 +2003,8 @@ static QVariant fcnCoreFeatureMaptipDisplay( const QVariantList &values, const Q
   QgsFeature feature;
   bool evaluate = true;
 
+  // TODO this expression function is NOT thread safe
+  Q_NOWARN_DEPRECATED_PUSH
   if ( values.isEmpty() )
   {
     feature = context->feature();
@@ -2027,6 +2044,7 @@ static QVariant fcnCoreFeatureMaptipDisplay( const QVariantList &values, const Q
     parent->setEvalErrorString( QObject::tr( "The layer is not valid." ) );
     return QVariant( );
   }
+  Q_NOWARN_DEPRECATED_POP
 
   if ( !feature.isValid() )
   {
@@ -6063,7 +6081,10 @@ static QVariant fcnRepresentValue( const QVariantList &values, const QgsExpressi
     }
     else
     {
+      // TODO this function is NOT thread safe
+      Q_NOWARN_DEPRECATED_PUSH
       QgsVectorLayer *layer = QgsExpressionUtils::getVectorLayer( context->variable( QStringLiteral( "layer" ) ), context, parent );
+      Q_NOWARN_DEPRECATED_POP
 
       const QString cacheValueKey = QStringLiteral( "repvalfcnval:%1:%2:%3" ).arg( layer ? layer->id() : QStringLiteral( "[None]" ), fieldName, value.toString() );
       if ( context->hasCachedValue( cacheValueKey ) )
@@ -7142,7 +7163,10 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress
 {
 
   const QVariant sourceLayerRef = context->variable( QStringLiteral( "layer" ) ); //used to detect if sourceLayer and targetLayer are the same
+  // TODO this function is NOT thread safe
+  Q_NOWARN_DEPRECATED_PUSH
   QgsVectorLayer *sourceLayer = QgsExpressionUtils::getVectorLayer( sourceLayerRef, context, parent );
+  Q_NOWARN_DEPRECATED_POP
 
   QgsFeatureRequest request;
   request.setTimeout( 10000 );
@@ -7163,7 +7187,10 @@ static QVariant executeGeomOverlay( const QVariantList &values, const QgsExpress
   QString subExpString = node->dump();
 
   bool testOnly = ( subExpString == "NULL" );
+  // TODO this function is NOT thread safe
+  Q_NOWARN_DEPRECATED_PUSH
   QgsVectorLayer *targetLayer = QgsExpressionUtils::getVectorLayer( targetLayerValue, context, parent );
+  Q_NOWARN_DEPRECATED_POP
   if ( !targetLayer ) // No layer, no joy
   {
     parent->setEvalErrorString( QObject::tr( "Layer '%1' could not be loaded." ).arg( targetLayerValue.toString() ) );

diff --git a/src/core/expression/qgsexpressionutils.cpp b/src/core/expression/qgsexpressionutils.cpp
@@ -19,6 +19,7 @@
 #include "qgsproviderregistry.h"
 #include "qgsvariantutils.h"
 #include "qgsproject.h"
+#include "qgsvectorlayerfeatureiterator.h"
 
 ///@cond PRIVATE
 
@@ -51,7 +52,12 @@ QgsGradientColorRamp QgsExpressionUtils::getRamp( const QVariant &value, QgsExpr
   return QgsGradientColorRamp();
 }
 
-QgsMapLayer *QgsExpressionUtils::getMapLayer( const QVariant &value, const QgsExpressionContext *, QgsExpression * )
+QgsMapLayer *QgsExpressionUtils::getMapLayer( const QVariant &value, const QgsExpressionContext *context, QgsExpression *parent )
+{
+  return getMapLayerPrivate( value, context, parent );
+}
+
+QgsMapLayer *QgsExpressionUtils::getMapLayerPrivate( const QVariant &value, const QgsExpressionContext *, QgsExpression * )
 {
   // First check if we already received a layer pointer
   QPointer< QgsMapLayer > ml = value.value< QgsWeakMapLayerPointer >().data();
@@ -133,7 +139,7 @@ void QgsExpressionUtils::executeLambdaForMapLayer( const QVariant &value, const
   // if no layer stores, then this is only for layers in project and therefore associated with the main thread
   auto runFunction = [ value, context, expression, &function, &foundLayer ]
   {
-    if ( QgsMapLayer *layer = getMapLayer( value, context, expression ) )
+    if ( QgsMapLayer *layer = getMapLayerPrivate( value, context, expression ) )
     {
       foundLayer = true;
       function( layer );
@@ -181,15 +187,22 @@ std::unique_ptr<QgsVectorLayerFeatureSource> QgsExpressionUtils::getFeatureSourc
   return featureSource;
 }
 
+QgsVectorLayer *QgsExpressionUtils::getVectorLayer( const QVariant &value, const QgsExpressionContext *context, QgsExpression *e )
+{
+  return qobject_cast<QgsVectorLayer *>( getMapLayerPrivate( value, context, e ) );
+}
+
 QString QgsExpressionUtils::getFilePathValue( const QVariant &value, const QgsExpressionContext *context, QgsExpression *parent )
 {
   // if it's a map layer, return the file path of that layer...
   QString res;
+  Q_NOWARN_DEPRECATED_PUSH
   if ( QgsMapLayer *layer = getMapLayer( value, context, parent ) )
   {
     const QVariantMap parts = QgsProviderRegistry::instance()->decodeUri( layer->providerType(), layer->source() );
     res = parts.value( QStringLiteral( "path" ) ).toString();
   }
+  Q_NOWARN_DEPRECATED_POP
 
   if ( res.isEmpty() )
     res = value.toString();

diff --git a/src/core/expression/qgsexpressionutils.h b/src/core/expression/qgsexpressionutils.h
@@ -21,17 +21,15 @@
 
 #include "qgsfeature.h"
 #include "qgsexpression.h"
-#include "qgsvectorlayerfeatureiterator.h"
-#include "qgsrasterlayer.h"
-#include "qgsvectorlayer.h"
-#include "qgsmeshlayer.h"
 #include "qgsvariantutils.h"
+#include "qgsfeaturerequest.h"
 
 #include <QThread>
 #include <QLocale>
 #include <functional>
 
 class QgsGradientColorRamp;
+class QgsVectorLayerFeatureSource;
 
 #define ENSURE_NO_EVAL_ERROR   {  if ( parent->hasEvalError() ) return QVariant(); }
 #define SET_EVAL_ERROR(x)   { parent->setEvalErrorString( x ); return QVariant(); }
@@ -352,7 +350,10 @@ class CORE_EXPORT QgsExpressionUtils
       return nullptr;
     }
 
-    static QgsMapLayer *getMapLayer( const QVariant &value, const QgsExpressionContext *context, QgsExpression * );
+    /**
+     * \deprecated Not actually deprecated, but this method is not thread safe -- use with extreme caution only when the thread safety has already been taken care of by the caller!
+     */
+    Q_DECL_DEPRECATED static QgsMapLayer *getMapLayer( const QVariant &value, const QgsExpressionContext *context, QgsExpression * );
 
     /**
      * Executes a lambda \a function for a \a value which corresponds to a map layer, in a thread-safe way.
@@ -373,10 +374,10 @@ class CORE_EXPORT QgsExpressionUtils
      */
     static std::unique_ptr<QgsVectorLayerFeatureSource> getFeatureSource( const QVariant &value, const QgsExpressionContext *context, QgsExpression *e, bool &foundLayer );
 
-    static QgsVectorLayer *getVectorLayer( const QVariant &value, const QgsExpressionContext *context, QgsExpression *e )
-    {
-      return qobject_cast<QgsVectorLayer *>( getMapLayer( value, context, e ) );
-    }
+    /**
+     * \deprecated Not actually deprecated, but this method is not thread safe -- use with extreme caution only when the thread safety has already been taken care of by the caller!
+     */
+    Q_DECL_DEPRECATED static QgsVectorLayer *getVectorLayer( const QVariant &value, const QgsExpressionContext *context, QgsExpression *e );
 
     /**
      * Tries to convert a \a value to a file path.
@@ -478,6 +479,13 @@ class CORE_EXPORT QgsExpressionUtils
      */
     static std::tuple<QVariant::Type, int> determineResultType( const QString &expression, const QgsVectorLayer *layer, QgsFeatureRequest request = QgsFeatureRequest(), QgsExpressionContext context = QgsExpressionContext(), bool *foundFeatures = nullptr );
 
+  private:
+
+    /**
+     * \warning Only call when thread safety has been taken care of by the caller!
+     */
+    static QgsMapLayer *getMapLayerPrivate( const QVariant &value, const QgsExpressionContext *context, QgsExpression * );
+
 };
 
 

diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
@@ -36,6 +36,7 @@
 #include "qgsvectorlayerutils.h"
 #include "qgsexpressioncontextutils.h"
 #include "qgsexpressionutils.h"
+#include "qgsmeshlayer.h"
 #include <geos_c.h>
 
 static void _parseAndEvalExpr( int arg )
@@ -4581,6 +4582,7 @@ class TestQgsExpression: public QObject
       QgsExpression exp;
       // NULL value
       QgsExpressionContext context;
+      Q_NOWARN_DEPRECATED_PUSH
       QgsMapLayer *res = QgsExpressionUtils::getMapLayer( QVariant(), &context, &exp );
       QVERIFY( !res );
       QVERIFY( !exp.hasEvalError() );
@@ -4626,6 +4628,7 @@ class TestQgsExpression: public QObject
       // TODO -- probably should flag an error here?
       QVERIFY( !exp.hasEvalError() );
 #endif
+      Q_NOWARN_DEPRECATED_POP
     }
 
     void testGetFilePathValue()

diff --git a/tests/src/core/testqgslayoutcontext.cpp b/tests/src/core/testqgslayoutcontext.cpp
@@ -121,7 +121,9 @@ void TestQgsLayoutContext::layer()
   l.reportContext().setLayer( layer );
   //test that expression context created for layout contains report context layer scope
   const QgsExpressionContext expContext  = l.createExpressionContext();
+  Q_NOWARN_DEPRECATED_PUSH
   QCOMPARE( QgsExpressionUtils::getVectorLayer( expContext.variable( "layer" ), &expContext, nullptr ), layer );
+  Q_NOWARN_DEPRECATED_POP
 
   delete layer;
 }