Fix aggregate function returns incorrect results

When the aggregate uses variables, we need to ensure that the cache key correctly considers the current value of ALL those variables. Otherwise we'll return incorrect results when an expression is re-evaluated after changing the variable value for the context. Fixes use of aggregate function with @symbol_label in legends. Fixes #58221 (cherry picked from commit 7b88103)
qgis · Oct 25, 2024 · e0fd657 · e0fd657
1 parent 0d458e3
commit e0fd657
Show file tree

Hide file tree

Showing 7 changed files with 159 additions and 13 deletions.
diff --git a/python/PyQt6/core/auto_generated/qgsexpressioncontext.sip.in b/python/PyQt6/core/auto_generated/qgsexpressioncontext.sip.in
@@ -992,6 +992,18 @@ if evaluation should be canceled, if set.
 .. seealso:: :py:func:`setFeedback`
 
 .. versionadded:: 3.20
+%End
+
+    QString uniqueHash( bool &ok /Out/, const QSet<QString> &variables = QSet<QString>() ) const;
+%Docstring
+Returns a unique hash representing the current state of the context.
+
+:param variables: optional names of a subset of variables to include in the hash. If not specified, all variables will be considered.
+
+:return: - calculated hash
+         - ok: ``True`` if the hash could be generated, or false if e.g. a variable value is of a type which cannot be hashed
+
+.. versionadded:: 3.40
 %End
 
     static const QString EXPR_FIELDS;

diff --git a/python/core/auto_generated/qgsexpressioncontext.sip.in b/python/core/auto_generated/qgsexpressioncontext.sip.in
@@ -992,6 +992,18 @@ if evaluation should be canceled, if set.
 .. seealso:: :py:func:`setFeedback`
 
 .. versionadded:: 3.20
+%End
+
+    QString uniqueHash( bool &ok /Out/, const QSet<QString> &variables = QSet<QString>() ) const;
+%Docstring
+Returns a unique hash representing the current state of the context.
+
+:param variables: optional names of a subset of variables to include in the hash. If not specified, all variables will be considered.
+
+:return: - calculated hash
+         - ok: ``True`` if the hash could be generated, or false if e.g. a variable value is of a type which cannot be hashed
+
+.. versionadded:: 3.40
 %End
 
     static const QString EXPR_FIELDS;

diff --git a/src/core/expression/qgsexpressionfunction.cpp b/src/core/expression/qgsexpressionfunction.cpp
@@ -714,18 +714,21 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
     QgsExpression subExp( subExpression );
     QgsExpression filterExp( parameters.filter );
 
+    const QSet< QString > filterVars = filterExp.referencedVariables();
+    const QSet< QString > subExpVars = subExp.referencedVariables();
+    QSet<QString> allVars = filterVars + subExpVars;
+
     bool isStatic = true;
-    if ( filterExp.referencedVariables().contains( QStringLiteral( "parent" ) )
-         || filterExp.referencedVariables().contains( QString() )
-         || subExp.referencedVariables().contains( QStringLiteral( "parent" ) )
-         || subExp.referencedVariables().contains( QString() ) )
+    if ( filterVars.contains( QStringLiteral( "parent" ) )
+         || filterVars.contains( QString() )
+         || subExpVars.contains( QStringLiteral( "parent" ) )
+         || subExpVars.contains( QString() ) )
     {
       isStatic = false;
     }
     else
     {
-      const QSet<QString> refVars = filterExp.referencedVariables() + subExp.referencedVariables();
-      for ( const QString &varName : refVars )
+      for ( const QString &varName : allVars )
       {
         const QgsExpressionContextScope *scope = context->activeScopeForVariable( varName );
         if ( scope && !scope->isStatic( varName ) )
@@ -738,15 +741,20 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
 
     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::number( qHash( context->feature() ) ), orderBy );
+      bool ok = false;
+      const QString contextHash = context->uniqueHash( ok, allVars );
+      if ( ok )
+      {
+        cacheKey = QStringLiteral( "aggfcn:%1:%2:%3:%4:%5:%6" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter,
+                   orderBy, contextHash );
+      }
     }
     else
     {
       cacheKey = QStringLiteral( "aggfcn:%1:%2:%3:%4:%5" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter, orderBy );
     }
 
-    if ( context->hasCachedValue( cacheKey ) )
+    if ( !cacheKey.isEmpty() && context->hasCachedValue( cacheKey ) )
     {
       return context->cachedValue( cacheKey );
     }
@@ -757,7 +765,7 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
     subContext.appendScope( subScope );
     result = vl->aggregate( aggregate, subExpression, parameters, &subContext, &ok, nullptr, context->feedback(), &aggregateError );
 
-    if ( ok )
+    if ( ok && !cacheKey.isEmpty() )
     {
       // important -- we should only store cached values when the expression is successfully calculated. Otherwise subsequent
       // use of the expression context will happily grab the invalid QVariant cached value without realising that there was actually an error
@@ -1004,8 +1012,13 @@ static QVariant fcnAggregateGeneric( QgsAggregateCalculator::Aggregate aggregate
   QString cacheKey;
   if ( !isStatic )
   {
-    cacheKey = QStringLiteral( "agg:%1:%2:%3:%4:%5%6:%7" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter,
-               QString::number( context->feature().id() ), QString::number( qHash( context->feature() ) ), orderBy );
+    bool ok = false;
+    const QString contextHash = context->uniqueHash( ok, refVars );
+    if ( ok )
+    {
+      cacheKey = QStringLiteral( "agg:%1:%2:%3:%4:%5:%6" ).arg( vl->id(), QString::number( static_cast< int >( aggregate ) ), subExpression, parameters.filter,
+                 orderBy, contextHash );
+    }
   }
   else
   {

diff --git a/src/core/qgsexpressioncontext.cpp b/src/core/qgsexpressioncontext.cpp
@@ -765,3 +765,45 @@ QgsFeedback *QgsExpressionContext::feedback() const
 {
   return mFeedback;
 }
+
+QString QgsExpressionContext::uniqueHash( bool &ok, const QSet<QString> &variables ) const
+{
+  QString hash;
+  ok = true;
+  const QString delimiter( QStringLiteral( "||~~||" ) );
+
+  if ( hasFeature() )
+  {
+    hash.append( QString::number( feature().id() ) + delimiter + QString::number( qHash( feature() ) ) + delimiter );
+  }
+
+  QStringList sortedVars = qgis::setToList( variables );
+  if ( sortedVars.empty() )
+    sortedVars = variableNames();
+  std::sort( sortedVars.begin(), sortedVars.end() );
+
+  for ( const QString &variableName : std::as_const( sortedVars ) )
+  {
+    const QVariant value = variable( variableName );
+    if ( QgsVariantUtils::isNull( value ) )
+    {
+      hash.append( delimiter );
+    }
+    else if ( value.type() == QVariant::String )
+    {
+      hash.append( value.toString() + delimiter );
+    }
+    else
+    {
+      const QString variantString = value.toString();
+      if ( variantString.isEmpty() )
+      {
+        ok = false;
+        return QString();
+      }
+      hash.append( variantString + delimiter );
+    }
+  }
+
+  return hash;
+}
diff --git a/src/core/qgsexpressioncontext.h b/src/core/qgsexpressioncontext.h
@@ -902,6 +902,18 @@ class CORE_EXPORT QgsExpressionContext
      */
     QgsFeedback *feedback() const;
 
+    /**
+     * Returns a unique hash representing the current state of the context.
+     *
+     * \param ok will be set to TRUE if the hash could be generated, or false if e.g. a variable value is of a type which cannot be hashed
+     * \param variables optional names of a subset of variables to include in the hash. If not specified, all variables will be considered.
+     *
+     * \returns calculated hash
+     *
+     * \since QGIS 3.40
+     */
+    QString uniqueHash( bool &ok SIP_OUT, const QSet<QString> &variables = QSet<QString>() ) const;
+
     //! Inbuilt variable name for fields storage
     static const QString EXPR_FIELDS;
     //! Inbuilt variable name for value original value variable

diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
@@ -2634,7 +2634,7 @@ class TestQgsExpression: public QObject
       QCOMPARE( res.toInt(), 1 );
     }
 
-    void test_aggregate_with_variable()
+    void test_aggregate_with_variable_feature()
     {
       // 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
@@ -2658,6 +2658,23 @@ class TestQgsExpression: public QObject
       }
     }
 
+    void test_aggregate_with_variables()
+    {
+      // 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/58221
+      QgsExpressionContext context;
+      context.appendScope( QgsExpressionContextUtils::layerScope( mAggregatesLayer ) );
+      context.lastScope()->setVariable( QStringLiteral( "my_var" ), QStringLiteral( "3" ) );
+
+      QgsExpression exp( QString( "aggregate(layer:='aggregate_layer', aggregate:='concatenate_unique', expression:=\"col2\", filter:=\"col1\"=@my_var)" ) );
+      QString res = exp.evaluate( &context ).toString();
+      QCOMPARE( res, QStringLiteral( "test333" ) );
+
+      context.lastScope()->setVariable( QStringLiteral( "my_var" ), QStringLiteral( "4" ) );
+      res = exp.evaluate( &context ).toString();
+      QCOMPARE( res, QStringLiteral( "test" ) );
+    }
+
     void aggregate_data()
     {
       QTest::addColumn<QString>( "string" );

diff --git a/tests/src/core/testqgsexpressioncontext.cpp b/tests/src/core/testqgsexpressioncontext.cpp
@@ -61,6 +61,7 @@ class TestQgsExpressionContext : public QObject
     void description();
     void readWriteScope();
     void layerStores();
+    void uniqueHash();
 
   private:
 
@@ -1043,5 +1044,42 @@ void TestQgsExpressionContext::layerStores()
   QCOMPARE( c3.loadedLayerStore(), &store4 );
 }
 
+void TestQgsExpressionContext::uniqueHash()
+{
+  QgsExpressionContext context;
+  bool ok = true;
+  // the actual hash values aren't important, just that they are unique. Feel free to change the expected results accordingly
+  QSet< QString > vars;
+  vars.insert( QStringLiteral( "var1" ) );
+  vars.insert( QStringLiteral( "var2" ) );
+  QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "||~~||||~~||" ) );
+  QVERIFY( ok );
+
+  QgsExpressionContextScope *scope1 = new QgsExpressionContextScope();
+  context.appendScope( scope1 );
+  scope1->setVariable( QStringLiteral( "var1" ), QStringLiteral( "a string" ) );
+  scope1->setVariable( QStringLiteral( "var2" ), 5 );
+  QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "a string||~~||5||~~||" ) );
+  QVERIFY( ok );
+
+  QgsExpressionContextScope *scope2 = new QgsExpressionContextScope();
+  context.appendScope( scope2 );
+  scope2->setVariable( QStringLiteral( "var1" ), QStringLiteral( "b string" ) );
+  QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "b string||~~||5||~~||" ) );
+  QVERIFY( ok );
+
+  QgsFeature feature;
+  feature.setId( 11 );
+  feature.setAttributes( QgsAttributes() << 5 << 11 );
+  context.setFeature( feature );
+  QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "11||~~||1566||~~||b string||~~||5||~~||" ) );
+  QVERIFY( ok );
+
+  // a value which can't be converted to string
+  scope2->setVariable( QStringLiteral( "var1" ), QVariant::fromValue( QgsCoordinateReferenceSystem( "EPSG:3857" ) ) );
+  context.uniqueHash( ok, vars );
+  QVERIFY( !ok );
+}
+
 QGSTEST_MAIN( TestQgsExpressionContext )
 #include "testqgsexpressioncontext.moc"