From d1f3f9d4f14784b98fc041f9eb488dc277e31f62 Mon Sep 17 00:00:00 2001
From: Denis Rouzaud <denis.rouzaud@gmail.com>
Date: Thu, 15 Oct 2020 14:43:33 +0200
Subject: [PATCH] correctly determine if variables are static in aggregate
 expression and filter

fixes #33382
---
 src/core/expression/qgsexpressionfunction.cpp | 23 ++++++++++++++++++-
 tests/src/core/testqgsexpression.cpp          | 19 +++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/src/core/expression/qgsexpressionfunction.cpp b/src/core/expression/qgsexpressionfunction.cpp
index ca88d79e1c74..3e7dac838ea4 100644
--- a/src/core/expression/qgsexpressionfunction.cpp
+++ b/src/core/expression/qgsexpressionfunction.cpp
@@ -637,10 +637,30 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
     QString cacheKey;
     QgsExpression subExp( subExpression );
     QgsExpression filterExp( parameters.filter );
+
+    bool isStatic = true;
     if ( filterExp.referencedVariables().contains( QStringLiteral( "parent" ) )
          || filterExp.referencedVariables().contains( QString() )
          || subExp.referencedVariables().contains( QStringLiteral( "parent" ) )
          || subExp.referencedVariables().contains( QString() ) )
+    {
+      isStatic = false;
+    }
+    else
+    {
+      const QSet<QString> refVars = filterExp.referencedVariables() + subExp.referencedVariables();
+      for ( const QString &varName : refVars )
+      {
+        const QgsExpressionContextScope *scope = context->activeScopeForVariable( varName );
+        if ( scope && !scope->isStatic( varName ) )
+        {
+          isStatic = false;
+          break;
+        }
+      }
+    }
+
+    if ( !isStatic )
     {
       cacheKey = QStringLiteral( "aggfcn:%1:%2:%3:%4:%5%6:%7" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter,
                  QString::number( context->feature().id() ), QString( qHash( context->feature() ) ), orderBy );
@@ -651,8 +671,9 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
     }
 
     if ( context && context->hasCachedValue( cacheKey ) )
+    {
       return context->cachedValue( cacheKey );
-
+    }
     QgsExpressionContext subContext( *context );
     QgsExpressionContextScope *subScope = new QgsExpressionContextScope();
     subScope->setVariable( QStringLiteral( "parent" ), context->feature() );
diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
index 445033b7b363..ec3bc3307546 100644
--- a/tests/src/core/testqgsexpression.cpp
+++ b/tests/src/core/testqgsexpression.cpp
@@ -2029,6 +2029,25 @@ class TestQgsExpression: public QObject
       QCOMPARE( res.toInt(), 1 );
     }
 
+    void test_aggregate_with_variable()
+    {
+      // this checks that a variable can be non static in a aggregate, i.e. the result will change across the fetched features
+      // see https://github.com/qgis/QGIS/issues/33382
+      QgsExpressionContext context;
+      context.appendScope( QgsExpressionContextUtils::layerScope( mAggregatesLayer ) );
+      QgsFeature f;
+
+      QgsFeatureIterator it = mAggregatesLayer->getFeatures();
+
+      while ( it.nextFeature( f ) )
+      {
+        context.setFeature( f );
+        QgsExpression exp( QString( "with_variable('my_var',\"col1\", aggregate(layer:='aggregate_layer', aggregate:='concatenate_unique', expression:=\"col2\", filter:=\"col1\"=@my_var))" ) );
+        QString res = exp.evaluate( &context ).toString();
+        QCOMPARE( res, f.attribute( "col2" ) );
+      }
+    }
+
     void aggregate_data()
     {
       QTest::addColumn<QString>( "string" );