From 61b7970157d686e04c721555d8f35f2ba5499343 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 8 Oct 2024 14:16:33 +1000 Subject: [PATCH 1/3] 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 7b88103128db0cc33c5718ad273ddf51cb192bf9) --- .../qgsexpressioncontext.sip.in | 12 ++++++ .../qgsexpressioncontext.sip.in | 12 ++++++ src/core/expression/qgsexpressionfunction.cpp | 37 ++++++++++------ src/core/qgsexpressioncontext.cpp | 42 +++++++++++++++++++ src/core/qgsexpressioncontext.h | 12 ++++++ tests/src/core/testqgsexpression.cpp | 19 ++++++++- tests/src/core/testqgsexpressioncontext.cpp | 38 +++++++++++++++++ 7 files changed, 159 insertions(+), 13 deletions(-) diff --git a/python/PyQt6/core/auto_generated/qgsexpressioncontext.sip.in b/python/PyQt6/core/auto_generated/qgsexpressioncontext.sip.in index dfbd1a6cf797..362e8331d271 100644 --- 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 &variables = QSet() ) 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 index dfbd1a6cf797..362e8331d271 100644 --- 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 &variables = QSet() ) 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 index c9defb41c305..1fe1bf8ad49c 100644 --- 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 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 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 index b05f99490a5e..1301ebc98436 100644 --- 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 &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 index 4763fcdb1070..b93c7aa554aa 100644 --- 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 &variables = QSet() ) 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 index 88bb577c1f32..16108d34b1c2 100644 --- 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( "string" ); diff --git a/tests/src/core/testqgsexpressioncontext.cpp b/tests/src/core/testqgsexpressioncontext.cpp index 51785e0b4892..392d298d0783 100644 --- 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" From 0f5796d3608f898bbaf1d82db094155671dfda2b Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Fri, 11 Oct 2024 11:06:15 +1000 Subject: [PATCH 2/3] Include variable name in result (cherry picked from commit f618993f774a2421b7fcda3136530b98aa9526d3) --- src/core/qgsexpressioncontext.cpp | 1 + tests/src/core/testqgsexpressioncontext.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/core/qgsexpressioncontext.cpp b/src/core/qgsexpressioncontext.cpp index 1301ebc98436..c2d852d20365 100644 --- a/src/core/qgsexpressioncontext.cpp +++ b/src/core/qgsexpressioncontext.cpp @@ -785,6 +785,7 @@ QString QgsExpressionContext::uniqueHash( bool &ok, const QSet &variabl for ( const QString &variableName : std::as_const( sortedVars ) ) { const QVariant value = variable( variableName ); + hash.append( variableName + "=" ); if ( QgsVariantUtils::isNull( value ) ) { hash.append( delimiter ); diff --git a/tests/src/core/testqgsexpressioncontext.cpp b/tests/src/core/testqgsexpressioncontext.cpp index 392d298d0783..0751b59b5c4f 100644 --- a/tests/src/core/testqgsexpressioncontext.cpp +++ b/tests/src/core/testqgsexpressioncontext.cpp @@ -1052,27 +1052,27 @@ void TestQgsExpressionContext::uniqueHash() QSet< QString > vars; vars.insert( QStringLiteral( "var1" ) ); vars.insert( QStringLiteral( "var2" ) ); - QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "||~~||||~~||" ) ); + QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "var1=||~~||var2=||~~||" ) ); 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||~~||" ) ); + QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "var1=a string||~~||var2=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||~~||" ) ); + QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "var1=b string||~~||var2=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||~~||" ) ); + QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "11||~~||1566||~~||var1=b string||~~||var2=5||~~||" ) ); QVERIFY( ok ); // a value which can't be converted to string From 863d0334da88705df7431544b3c821c420c940f9 Mon Sep 17 00:00:00 2001 From: Nyall Dawson Date: Tue, 15 Oct 2024 11:17:30 +1000 Subject: [PATCH 3/3] Sipify --- python/core/auto_generated/qgsexpressioncontext.sip.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/core/auto_generated/qgsexpressioncontext.sip.in b/python/core/auto_generated/qgsexpressioncontext.sip.in index 362e8331d271..f20cbc003908 100644 --- a/python/core/auto_generated/qgsexpressioncontext.sip.in +++ b/python/core/auto_generated/qgsexpressioncontext.sip.in @@ -1001,7 +1001,8 @@ 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 + - 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 + .. versionadded:: 3.40 %End