[expressions] Don't cache aggregate results if there's an evaluation error.

nyalldawson · nyalldawson · commit 49c150cc2896 · 2021-08-18T08:41:55.000+10:00
Otherwise subsequent evaluations of the expression will happily grab the cached null variant, unaware that it's actually a result of a broken expression and not a valid null aggregate calculation. (cherry picked from commit 97f0242)
diff --git a/src/core/expression/qgsexpressionfunction.cpp b/src/core/expression/qgsexpressionfunction.cpp
@@ -681,7 +681,13 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
     subContext.appendScope( subScope );
     result = vl->aggregate( aggregate, subExpression, parameters, &subContext, &ok );
 
-    context->setCachedValue( cacheKey, result );
+    if ( ok )
+    {
+      // 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
+      // associated with it's calculation!
+      context->setCachedValue( cacheKey, result );
+    }
   }
   else
   {
diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
@@ -2154,6 +2154,8 @@ class TestQgsExpression: public QObject
 
       // check again - make sure value was correctly cached
       res = exp.evaluate( &context );
+      // if first evaluation has an eval error, so should any subsequent evaluations!
+      QCOMPARE( exp.hasEvalError(), evalError );
       QCOMPARE( res, result );
     }
 

Original file line number	Diff line number	Diff line change
`@@ -681,7 +681,13 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon`
`681`	`681`	`subContext.appendScope( subScope );`
`682`	`682`	`result = vl->aggregate( aggregate, subExpression, parameters, &subContext, &ok );`
`683`	`683`
`684`		`- context->setCachedValue( cacheKey, result );`
	`684`	`+ if ( ok )`
	`685`	`+ {`
	`686`	`+ // important -- we should only store cached values when the expression is successfully calculated. Otherwise subsequent`
	`687`	`+ // use of the expression context will happily grab the invalid QVariant cached value without realising that there was actually an error`
	`688`	`+ // associated with it's calculation!`
	`689`	`+ context->setCachedValue( cacheKey, result );`
	`690`	`+ }`
`685`	`691`	`}`
`686`	`692`	`else`
`687`	`693`	`{`
Original file line number	Diff line number	Diff line change
`@@ -2154,6 +2154,8 @@ class TestQgsExpression: public QObject`
`2154`	`2154`
`2155`	`2155`	`// check again - make sure value was correctly cached`
`2156`	`2156`	`res = exp.evaluate( &context );`
	`2157`	`+ // if first evaluation has an eval error, so should any subsequent evaluations!`
	`2158`	`+ QCOMPARE( exp.hasEvalError(), evalError );`
`2157`	`2159`	`QCOMPARE( res, result );`
`2158`	`2160`	`}`
`2159`	`2161`