Avoid lazy evaluation for substr expression function

Better solution is to set handlesNull to true to avoid the default null parameter = null result behaviour, and handle null values in params 1 and 2 manually
qgis · Nov 7, 2016 · ec55161 · ec55161
1 parent 0fbcc4b
commit ec55161
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 20 deletions.
diff --git a/src/core/qgsexpression.cpp b/src/core/qgsexpression.cpp
@@ -1398,30 +1398,19 @@ static QVariant fcnUuid( const QVariantList&, const QgsExpressionContext*, QgsEx
   return QUuid::createUuid().toString();
 }
 
-static QVariant fcnSubstr( const QVariantList& values, const QgsExpressionContext* context, QgsExpression* parent )
+static QVariant fcnSubstr( const QVariantList& values, const QgsExpressionContext*, QgsExpression* parent )
 {
+  if ( !values.at( 0 ).isValid() || !values.at( 1 ).isValid() )
+    return QVariant();
 
-  QgsExpression::Node* node;
-
-  node = getNode( values.at( 0 ), parent );
-  ENSURE_NO_EVAL_ERROR;
-  QString str = node->eval( parent, context ).toString();
-
-  node = getNode( values.at( 1 ), parent );
-  ENSURE_NO_EVAL_ERROR;
-  int from = node->eval( parent, context ).toInt();
+  QString str = getStringValue( values.at( 0 ), parent );
+  int from = getIntValue( values.at( 1 ), parent );
 
-  node = getNode( values.at( 2 ), parent );
-  QVariant value = node->eval( parent, context );
-  int len;
-  if ( value.isValid() )
-  {
-    len = value.toInt();
-  }
+  int len = 0;
+  if ( values.at( 2 ).isValid() )
+    len = getIntValue( values.at( 2 ), parent );
   else
-  {
     len = str.size();
-  }
 
   if ( from < 0 )
   {
@@ -3847,7 +3836,8 @@ const QList<QgsExpression::Function*>& QgsExpression::Functions()
     << new StaticFunction( QStringLiteral( "replace" ), -1, fcnReplace, QStringLiteral( "String" ) )
     << new StaticFunction( QStringLiteral( "regexp_replace" ), 3, fcnRegexpReplace, QStringLiteral( "String" ) )
     << new StaticFunction( QStringLiteral( "regexp_substr" ), 2, fcnRegexpSubstr, QStringLiteral( "String" ) )
-    << new StaticFunction( QStringLiteral( "substr" ), ParameterList() << Parameter( QStringLiteral( "string" ) ) << Parameter( QStringLiteral( "start " ) ) << Parameter( QStringLiteral( "length" ), true ), fcnSubstr, QStringLiteral( "String" ), QString(), False, QSet<QString>(), true )
+    << new StaticFunction( QStringLiteral( "substr" ), ParameterList() << Parameter( QStringLiteral( "string" ) ) << Parameter( QStringLiteral( "start " ) ) << Parameter( QStringLiteral( "length" ), true ), fcnSubstr, QStringLiteral( "String" ), QString(),
+                           false, QSet< QString >(), false, QStringList(), true )
     << new StaticFunction( QStringLiteral( "concat" ), -1, fcnConcat, QStringLiteral( "String" ), QString(), false, QSet<QString>(), false, QStringList(), true )
     << new StaticFunction( QStringLiteral( "strpos" ), 2, fcnStrpos, QStringLiteral( "String" ) )
     << new StaticFunction( QStringLiteral( "left" ), 2, fcnLeft, QStringLiteral( "String" ) )

diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
@@ -840,6 +840,8 @@ class TestQgsExpression: public QObject
       QTest::newRow( "substr negative length" ) << "substr('HeLLo', 1,-3)" << false << QVariant( "He" );
       QTest::newRow( "substr positive start and negative length" ) << "substr('HeLLo', 3,-1)" << false << QVariant( "LL" );
       QTest::newRow( "substr start only" ) << "substr('HeLLo', 3)" << false << QVariant( "LLo" );
+      QTest::newRow( "substr null value" ) << "substr(NULL, 3,2)" << false << QVariant();
+      QTest::newRow( "substr null start" ) << "substr('Hello',NULL,2)" << false << QVariant();
       QTest::newRow( "regexp_substr" ) << "regexp_substr('abc123','(\\\\d+)')" << false << QVariant( "123" );
       QTest::newRow( "regexp_substr non-greedy" ) << "regexp_substr('abc123','(\\\\d+?)')" << false << QVariant( "1" );
       QTest::newRow( "regexp_substr no hit" ) << "regexp_substr('abcdef','(\\\\d+)')" << false << QVariant( "" );