Skip to content

Commit

Permalink
Merge pull request #5955 from m-kuhn/noCacheEvalErrors
Browse files Browse the repository at this point in the history 
Expressions: do not cache results when there is an eval error
  • Loading branch information
@m-kuhn
m-kuhn committed Dec 27, 2017
2 parents e552b9b + d01f94f commit 060a36b
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 43 deletions.
64 changes: 36 additions & 28 deletions src/core/expression/qgsexpressionfunction.cpp
View file
Expand Up @@ -3446,42 +3446,50 @@ static QVariant fcnRepresentValue( const QVariantList &values, const QgsExpressi
{
QVariant result;
QString fieldName;
if ( !values.isEmpty() )
{
QgsExpressionNodeColumnRef *col = dynamic_cast<QgsExpressionNodeColumnRef *>( node->args()->at( 0 ) );
if ( col && ( values.size() == 1 || !values.at( 1 ).isValid() ) )
fieldName = col->name();
else if ( values.size() == 2 )
fieldName = QgsExpressionUtils::getStringValue( values.at( 1 ), parent );
}

QVariant value = values.at( 0 );

const QgsFields fields = context->fields();
int fieldIndex = fields.lookupField( fieldName );

if ( fieldIndex == -1 )
{
parent->setEvalErrorString( QCoreApplication::translate( "expression", "%1: Field not found %2" ).arg( QStringLiteral( "represent_value" ), fieldName ) );
}
else
if ( context )
{
QgsVectorLayer *layer = QgsExpressionUtils::getVectorLayer( context->variable( "layer" ), parent );
const QgsEditorWidgetSetup setup = fields.at( fieldIndex ).editorWidgetSetup();
const QgsFieldFormatter *formatter = QgsApplication::fieldFormatterRegistry()->fieldFormatter( setup.type() );
if ( !values.isEmpty() )
{
QgsExpressionNodeColumnRef *col = dynamic_cast<QgsExpressionNodeColumnRef *>( node->args()->at( 0 ) );
if ( col && ( values.size() == 1 || !values.at( 1 ).isValid() ) )
fieldName = col->name();
else if ( values.size() == 2 )
fieldName = QgsExpressionUtils::getStringValue( values.at( 1 ), parent );
}

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

QVariant cache;
if ( !context->hasCachedValue( cacheKey ) )
const QgsFields fields = context->fields();
int fieldIndex = fields.lookupField( fieldName );

if ( fieldIndex == -1 )
{
cache = formatter->createCache( layer, fieldIndex, setup.config() );
context->setCachedValue( cacheKey, cache );
parent->setEvalErrorString( QCoreApplication::translate( "expression", "%1: Field not found %2" ).arg( QStringLiteral( "represent_value" ), fieldName ) );
}
else
cache = context->cachedValue( cacheKey );
{
QgsVectorLayer *layer = QgsExpressionUtils::getVectorLayer( context->variable( "layer" ), parent );
const QgsEditorWidgetSetup setup = fields.at( fieldIndex ).editorWidgetSetup();
const QgsFieldFormatter *formatter = QgsApplication::fieldFormatterRegistry()->fieldFormatter( setup.type() );

const QString cacheKey = QStringLiteral( "repvalfcn:%1:%2" ).arg( layer ? layer->id() : QStringLiteral( "[None]" ), fieldName );

result = formatter->representValue( layer, fieldIndex, setup.config(), cache, value );
QVariant cache;
if ( !context->hasCachedValue( cacheKey ) )
{
cache = formatter->createCache( layer, fieldIndex, setup.config() );
context->setCachedValue( cacheKey, cache );
}
else
cache = context->cachedValue( cacheKey );

result = formatter->representValue( layer, fieldIndex, setup.config(), cache, value );
}
}
else
{
parent->setEvalErrorString( QCoreApplication::translate( "expression", "%1: function cannot be evaluated without a context." ).arg( QStringLiteral( "represent_value" ), fieldName ) );
}

return result;
Expand Down
6 changes: 5 additions & 1 deletion src/core/expression/qgsexpressionnode.cpp
View file
Expand Up @@ -14,6 +14,7 @@
***************************************************************************/

#include "qgsexpressionnode.h"
#include "qgsexpression.h"


QVariant QgsExpressionNode::eval( QgsExpression *parent, const QgsExpressionContext *context )
Expand All @@ -34,7 +35,10 @@ bool QgsExpressionNode::prepare( QgsExpression *parent, const QgsExpressionConte
if ( isStatic( parent, context ) )
{
mCachedStaticValue = evalNode( parent, context );
mHasCachedValue = true;
if ( !parent->hasEvalError() )
mHasCachedValue = true;
else
mHasCachedValue = false;
return true;
}
else
Expand Down
38 changes: 24 additions & 14 deletions tests/src/core/testqgsexpression.cpp
View file
Expand Up @@ -359,7 +359,7 @@ class TestQgsExpression: public QObject
}

QgsExpressionContext context;
Q_ASSERT( exp.prepare( &context ) );
QVERIFY( exp.prepare( &context ) );

QVariant res = exp.evaluate();
if ( exp.hasEvalError() )
Expand Down Expand Up @@ -388,10 +388,10 @@ class TestQgsExpression: public QObject
QgsExpression expression( "represent_value(\"Pilots\", 'Pilots')" );
if ( expression.hasParserError() )
qDebug() << expression.parserErrorString();
Q_ASSERT( !expression.hasParserError() );
QVERIFY( !expression.hasParserError() );
if ( expression.hasEvalError() )
qDebug() << expression.evalErrorString();
Q_ASSERT( !expression.hasEvalError() );
QVERIFY( !expression.hasEvalError() );
expression.prepare( &context );

QgsFeature feature;
Expand All @@ -403,10 +403,10 @@ class TestQgsExpression: public QObject
QgsExpression expression2( "represent_value(\"Class\", 'Class')" );
if ( expression2.hasParserError() )
qDebug() << expression2.parserErrorString();
Q_ASSERT( !expression2.hasParserError() );
QVERIFY( !expression2.hasParserError() );
if ( expression2.hasEvalError() )
qDebug() << expression2.evalErrorString();
Q_ASSERT( !expression2.hasEvalError() );
QVERIFY( !expression2.hasEvalError() );
expression2.prepare( &context );
mPointsLayer->getFeatures( QgsFeatureRequest().setFilterExpression( "Class = 'Jet'" ) ).nextFeature( feature );
context.setFeature( feature );
Expand All @@ -416,10 +416,10 @@ class TestQgsExpression: public QObject
QgsExpression expression3( "represent_value(\"Pilots\")" );
if ( expression3.hasParserError() )
qDebug() << expression.parserErrorString();
Q_ASSERT( !expression3.hasParserError() );
QVERIFY( !expression3.hasParserError() );
if ( expression3.hasEvalError() )
qDebug() << expression3.evalErrorString();
Q_ASSERT( !expression3.hasEvalError() );
QVERIFY( !expression3.hasEvalError() );

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


QgsExpression expression4( "represent_value('Class')" );
expression4.evaluate();
if ( expression4.hasParserError() )
qDebug() << expression4.parserErrorString();
QVERIFY( !expression4.hasParserError() );
if ( expression4.hasEvalError() )
qDebug() << expression4.evalErrorString();
QVERIFY( expression4.hasEvalError() );

expression4.prepare( &context );
if ( expression4.hasParserError() )
qDebug() << expression4.parserErrorString();
Q_ASSERT( !expression4.hasParserError() );
Q_ASSERT( expression4.hasEvalError() );
QVERIFY( !expression4.hasParserError() );
if ( expression4.hasEvalError() )
qDebug() << expression4.evalErrorString();
QVERIFY( expression4.hasEvalError() );
}

void evaluation_data()
Expand Down Expand Up @@ -1224,7 +1234,7 @@ class TestQgsExpression: public QObject

QgsExpressionContext context;

Q_ASSERT( exp.prepare( &context ) );
QVERIFY( exp.prepare( &context ) );

QVariant::Type resultType = result.type();
QVariant::Type expectedType = expected.type();
Expand Down Expand Up @@ -1278,7 +1288,7 @@ class TestQgsExpression: public QObject
break;
}
default:
Q_ASSERT( false ); // should never happen
QVERIFY( false ); // should never happen
}
}

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

Q_ASSERT( !v1.isValid() );
QVERIFY( !v1.isValid() );

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

Q_ASSERT( result3.isNull() );
QVERIFY( result3.isNull() );
}

void test_formatPreviewString()
Expand Down

0 comments on commit 060a36b

Please sign in to comment.