Skip to content

Commit 060a36b

Browse files
m-kuhnm-kuhn
authoredDec 27, 2017
Merge pull request #5955 from m-kuhn/noCacheEvalErrors
Expressions: do not cache results when there is an eval error
·
ltr-3_40final-3_0_0
2 parents e552b9b + d01f94f commit 060a36b

File tree

3 files changed

+65
-43
lines changed

3 files changed

+65
-43
lines changed
 

‎src/core/expression/qgsexpressionfunction.cpp

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3446,42 +3446,50 @@ static QVariant fcnRepresentValue( const QVariantList &values, const QgsExpressi
34463446
{
34473447
QVariant result;
34483448
QString fieldName;
3449-
if ( !values.isEmpty() )
3450-
{
3451-
QgsExpressionNodeColumnRef *col = dynamic_cast<QgsExpressionNodeColumnRef *>( node->args()->at( 0 ) );
3452-
if ( col && ( values.size() == 1 || !values.at( 1 ).isValid() ) )
3453-
fieldName = col->name();
3454-
else if ( values.size() == 2 )
3455-
fieldName = QgsExpressionUtils::getStringValue( values.at( 1 ), parent );
3456-
}
3457-
3458-
QVariant value = values.at( 0 );
3459-
3460-
const QgsFields fields = context->fields();
3461-
int fieldIndex = fields.lookupField( fieldName );
34623449

3463-
if ( fieldIndex == -1 )
3464-
{
3465-
parent->setEvalErrorString( QCoreApplication::translate( "expression", "%1: Field not found %2" ).arg( QStringLiteral( "represent_value" ), fieldName ) );
3466-
}
3467-
else
3450+
if ( context )
34683451
{
3469-
QgsVectorLayer *layer = QgsExpressionUtils::getVectorLayer( context->variable( "layer" ), parent );
3470-
const QgsEditorWidgetSetup setup = fields.at( fieldIndex ).editorWidgetSetup();
3471-
const QgsFieldFormatter *formatter = QgsApplication::fieldFormatterRegistry()->fieldFormatter( setup.type() );
3452+
if ( !values.isEmpty() )
3453+
{
3454+
QgsExpressionNodeColumnRef *col = dynamic_cast<QgsExpressionNodeColumnRef *>( node->args()->at( 0 ) );
3455+
if ( col && ( values.size() == 1 || !values.at( 1 ).isValid() ) )
3456+
fieldName = col->name();
3457+
else if ( values.size() == 2 )
3458+
fieldName = QgsExpressionUtils::getStringValue( values.at( 1 ), parent );
3459+
}
34723460

3473-
const QString cacheKey = QStringLiteral( "repvalfcn:%1:%2" ).arg( layer ? layer->id() : QStringLiteral( "[None]" ), fieldName );
3461+
QVariant value = values.at( 0 );
34743462

3475-
QVariant cache;
3476-
if ( !context->hasCachedValue( cacheKey ) )
3463+
const QgsFields fields = context->fields();
3464+
int fieldIndex = fields.lookupField( fieldName );
3465+
3466+
if ( fieldIndex == -1 )
34773467
{
3478-
cache = formatter->createCache( layer, fieldIndex, setup.config() );
3479-
context->setCachedValue( cacheKey, cache );
3468+
parent->setEvalErrorString( QCoreApplication::translate( "expression", "%1: Field not found %2" ).arg( QStringLiteral( "represent_value" ), fieldName ) );
34803469
}
34813470
else
3482-
cache = context->cachedValue( cacheKey );
3471+
{
3472+
QgsVectorLayer *layer = QgsExpressionUtils::getVectorLayer( context->variable( "layer" ), parent );
3473+
const QgsEditorWidgetSetup setup = fields.at( fieldIndex ).editorWidgetSetup();
3474+
const QgsFieldFormatter *formatter = QgsApplication::fieldFormatterRegistry()->fieldFormatter( setup.type() );
3475+
3476+
const QString cacheKey = QStringLiteral( "repvalfcn:%1:%2" ).arg( layer ? layer->id() : QStringLiteral( "[None]" ), fieldName );
34833477

3484-
result = formatter->representValue( layer, fieldIndex, setup.config(), cache, value );
3478+
QVariant cache;
3479+
if ( !context->hasCachedValue( cacheKey ) )
3480+
{
3481+
cache = formatter->createCache( layer, fieldIndex, setup.config() );
3482+
context->setCachedValue( cacheKey, cache );
3483+
}
3484+
else
3485+
cache = context->cachedValue( cacheKey );
3486+
3487+
result = formatter->representValue( layer, fieldIndex, setup.config(), cache, value );
3488+
}
3489+
}
3490+
else
3491+
{
3492+
parent->setEvalErrorString( QCoreApplication::translate( "expression", "%1: function cannot be evaluated without a context." ).arg( QStringLiteral( "represent_value" ), fieldName ) );
34853493
}
34863494

34873495
return result;

‎src/core/expression/qgsexpressionnode.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
***************************************************************************/
1515

1616
#include "qgsexpressionnode.h"
17+
#include "qgsexpression.h"
1718

1819

1920
QVariant QgsExpressionNode::eval( QgsExpression *parent, const QgsExpressionContext *context )
@@ -34,7 +35,10 @@ bool QgsExpressionNode::prepare( QgsExpression *parent, const QgsExpressionConte
3435
if ( isStatic( parent, context ) )
3536
{
3637
mCachedStaticValue = evalNode( parent, context );
37-
mHasCachedValue = true;
38+
if ( !parent->hasEvalError() )
39+
mHasCachedValue = true;
40+
else
41+
mHasCachedValue = false;
3842
return true;
3943
}
4044
else

‎tests/src/core/testqgsexpression.cpp

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ class TestQgsExpression: public QObject
359359
}
360360

361361
QgsExpressionContext context;
362-
Q_ASSERT( exp.prepare( &context ) );
362+
QVERIFY( exp.prepare( &context ) );
363363

364364
QVariant res = exp.evaluate();
365365
if ( exp.hasEvalError() )
@@ -388,10 +388,10 @@ class TestQgsExpression: public QObject
388388
QgsExpression expression( "represent_value(\"Pilots\", 'Pilots')" );
389389
if ( expression.hasParserError() )
390390
qDebug() << expression.parserErrorString();
391-
Q_ASSERT( !expression.hasParserError() );
391+
QVERIFY( !expression.hasParserError() );
392392
if ( expression.hasEvalError() )
393393
qDebug() << expression.evalErrorString();
394-
Q_ASSERT( !expression.hasEvalError() );
394+
QVERIFY( !expression.hasEvalError() );
395395
expression.prepare( &context );
396396

397397
QgsFeature feature;
@@ -403,10 +403,10 @@ class TestQgsExpression: public QObject
403403
QgsExpression expression2( "represent_value(\"Class\", 'Class')" );
404404
if ( expression2.hasParserError() )
405405
qDebug() << expression2.parserErrorString();
406-
Q_ASSERT( !expression2.hasParserError() );
406+
QVERIFY( !expression2.hasParserError() );
407407
if ( expression2.hasEvalError() )
408408
qDebug() << expression2.evalErrorString();
409-
Q_ASSERT( !expression2.hasEvalError() );
409+
QVERIFY( !expression2.hasEvalError() );
410410
expression2.prepare( &context );
411411
mPointsLayer->getFeatures( QgsFeatureRequest().setFilterExpression( "Class = 'Jet'" ) ).nextFeature( feature );
412412
context.setFeature( feature );
@@ -416,10 +416,10 @@ class TestQgsExpression: public QObject
416416
QgsExpression expression3( "represent_value(\"Pilots\")" );
417417
if ( expression3.hasParserError() )
418418
qDebug() << expression.parserErrorString();
419-
Q_ASSERT( !expression3.hasParserError() );
419+
QVERIFY( !expression3.hasParserError() );
420420
if ( expression3.hasEvalError() )
421421
qDebug() << expression3.evalErrorString();
422-
Q_ASSERT( !expression3.hasEvalError() );
422+
QVERIFY( !expression3.hasEvalError() );
423423

424424
mPointsLayer->getFeatures( QgsFeatureRequest().setFilterExpression( "Pilots = 1" ) ).nextFeature( feature );
425425
context.setFeature( feature );
@@ -428,12 +428,22 @@ class TestQgsExpression: public QObject
428428
expression3.prepare( &context );
429429
QCOMPARE( expression.evaluate( &context ).toString(), QStringLiteral( "one" ) );
430430

431-
432431
QgsExpression expression4( "represent_value('Class')" );
432+
expression4.evaluate();
433+
if ( expression4.hasParserError() )
434+
qDebug() << expression4.parserErrorString();
435+
QVERIFY( !expression4.hasParserError() );
436+
if ( expression4.hasEvalError() )
437+
qDebug() << expression4.evalErrorString();
438+
QVERIFY( expression4.hasEvalError() );
439+
440+
expression4.prepare( &context );
433441
if ( expression4.hasParserError() )
434442
qDebug() << expression4.parserErrorString();
435-
Q_ASSERT( !expression4.hasParserError() );
436-
Q_ASSERT( expression4.hasEvalError() );
443+
QVERIFY( !expression4.hasParserError() );
444+
if ( expression4.hasEvalError() )
445+
qDebug() << expression4.evalErrorString();
446+
QVERIFY( expression4.hasEvalError() );
437447
}
438448

439449
void evaluation_data()
@@ -1224,7 +1234,7 @@ class TestQgsExpression: public QObject
12241234

12251235
QgsExpressionContext context;
12261236

1227-
Q_ASSERT( exp.prepare( &context ) );
1237+
QVERIFY( exp.prepare( &context ) );
12281238

12291239
QVariant::Type resultType = result.type();
12301240
QVariant::Type expectedType = expected.type();
@@ -1278,7 +1288,7 @@ class TestQgsExpression: public QObject
12781288
break;
12791289
}
12801290
default:
1281-
Q_ASSERT( false ); // should never happen
1291+
QVERIFY( false ); // should never happen
12821292
}
12831293
}
12841294

@@ -2417,7 +2427,7 @@ class TestQgsExpression: public QObject
24172427
QgsExpression exp1( QStringLiteral( "eval()" ) );
24182428
QVariant v1 = exp1.evaluate( &context );
24192429

2420-
Q_ASSERT( !v1.isValid() );
2430+
QVERIFY( !v1.isValid() );
24212431

24222432
QgsExpression exp2( QStringLiteral( "eval('4')" ) );
24232433
QVariant v2 = exp2.evaluate( &context );
@@ -2903,7 +2913,7 @@ class TestQgsExpression: public QObject
29032913
QgsExpression e3( QStringLiteral( "env('TESTENV_I_DO_NOT_EXIST')" ) );
29042914
QVariant result3 = e3.evaluate( &context );
29052915

2906-
Q_ASSERT( result3.isNull() );
2916+
QVERIFY( result3.isNull() );
29072917
}
29082918

29092919
void test_formatPreviewString()

0 commit comments

Comments
 (0)
Please sign in to comment.