Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[expressions] Don't cache aggregate results if there's an evaluation …
…error.

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)
  • Loading branch information
nyalldawson committed Aug 17, 2021
1 parent 1cb8df8 commit 49c150c
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 1 deletion.
8 changes: 7 additions & 1 deletion src/core/expression/qgsexpressionfunction.cpp
Expand Up @@ -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
{
Expand Down
2 changes: 2 additions & 0 deletions tests/src/core/testqgsexpression.cpp
Expand Up @@ -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 );
}

Expand Down

0 comments on commit 49c150c

Please sign in to comment.