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..f20cbc003908 100644 --- a/python/core/auto_generated/qgsexpressioncontext.sip.in +++ b/python/core/auto_generated/qgsexpressioncontext.sip.in @@ -992,6 +992,19 @@ 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: 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 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..c2d852d20365 100644 --- a/src/core/qgsexpressioncontext.cpp +++ b/src/core/qgsexpressioncontext.cpp @@ -765,3 +765,46 @@ 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 ); + hash.append( 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..0751b59b5c4f 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( "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( "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( "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||~~||var1=b string||~~||var2=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"