[processing] When a model algorithm parameter value is set to be taken

from an expression, abort the model execution if the expression evaluation fails instead of treating the value as null
qgis · Feb 7, 2022 · e6379fb · e6379fb
1 parent c74c934
commit e6379fb
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 10 deletions.
diff --git a/src/core/processing/models/qgsprocessingmodelalgorithm.cpp b/src/core/processing/models/qgsprocessingmodelalgorithm.cpp
@@ -98,9 +98,10 @@ QgsProcessingAlgorithm::Flags QgsProcessingModelAlgorithm::flags() const
   return QgsProcessingAlgorithm::flags() | QgsProcessingAlgorithm::FlagNoThreading;
 }
 
-QVariantMap QgsProcessingModelAlgorithm::parametersForChildAlgorithm( const QgsProcessingModelChildAlgorithm &child, const QVariantMap &modelParameters, const QVariantMap &results, const QgsExpressionContext &expressionContext ) const
+QVariantMap QgsProcessingModelAlgorithm::parametersForChildAlgorithm( const QgsProcessingModelChildAlgorithm &child, const QVariantMap &modelParameters, const QVariantMap &results, const QgsExpressionContext &expressionContext, QString &error ) const
 {
-  auto evaluateSources = [ = ]( const QgsProcessingParameterDefinition * def )->QVariant
+  error.clear();
+  auto evaluateSources = [ =, &error ]( const QgsProcessingParameterDefinition * def )->QVariant
   {
     const QgsProcessingModelChildParameterSources paramSources = child.parameterSources().value( def->name() );
 
@@ -129,6 +130,10 @@ QVariantMap QgsProcessingModelAlgorithm::parametersForChildAlgorithm( const QgsP
         {
           QgsExpression exp( source.expression() );
           paramParts << exp.evaluate( &expressionContext );
+          if ( exp.hasEvalError() )
+          {
+            error = QObject::tr( "Could not evaluate expression for parameter %1 for %2: %3" ).arg( def->name(), child.description(), exp.evalErrorString() );
+          }
           break;
         }
         case QgsProcessingModelChildParameterSource::ExpressionText:
@@ -329,7 +334,11 @@ QVariantMap QgsProcessingModelAlgorithm::processAlgorithm( const QVariantMap &pa
                  << createExpressionContextScopeForChildAlgorithm( childId, context, parameters, childResults );
       context.setExpressionContext( expContext );
 
-      QVariantMap childParams = parametersForChildAlgorithm( child, parameters, childResults, expContext );
+      QString error;
+      QVariantMap childParams = parametersForChildAlgorithm( child, parameters, childResults, expContext, error );
+      if ( !error.isEmpty() )
+        throw QgsProcessingException( error );
+
       if ( feedback && !skipGenericLogging )
         feedback->setProgressText( QObject::tr( "Running %1 [%2/%3]" ).arg( child.description() ).arg( executed.count() + 1 ).arg( toExecute.count() ) );
 

diff --git a/src/core/processing/models/qgsprocessingmodelalgorithm.h b/src/core/processing/models/qgsprocessingmodelalgorithm.h
@@ -549,7 +549,7 @@ class CORE_EXPORT QgsProcessingModelAlgorithm : public QgsProcessingAlgorithm
     void dependsOnChildAlgorithmsRecursive( const QString &childId, QSet<QString> &depends ) const;
     void dependentChildAlgorithmsRecursive( const QString &childId, QSet<QString> &depends, const QString &branch ) const;
 
-    QVariantMap parametersForChildAlgorithm( const QgsProcessingModelChildAlgorithm &child, const QVariantMap &modelParameters, const QVariantMap &results, const QgsExpressionContext &expressionContext ) const;
+    QVariantMap parametersForChildAlgorithm( const QgsProcessingModelChildAlgorithm &child, const QVariantMap &modelParameters, const QVariantMap &results, const QgsExpressionContext &expressionContext, QString &error ) const;
 
     /**
      * Returns TRUE if an output from a child algorithm is required elsewhere in

diff --git a/tests/src/analysis/testqgsprocessingmodelalgorithm.cpp b/tests/src/analysis/testqgsprocessingmodelalgorithm.cpp
@@ -1163,7 +1163,9 @@ void TestQgsProcessingModelAlgorithm::modelExecution()
   layerDef.destinationName = "my_dest";
   modelInputs.insert( "cx3:MY_OUT", QVariant::fromValue( layerDef ) );
   QVariantMap childResults;
-  QVariantMap params = model2.parametersForChildAlgorithm( model2.childAlgorithm( QStringLiteral( "cx1" ) ), modelInputs, childResults, expContext );
+  QString error;
+  QVariantMap params = model2.parametersForChildAlgorithm( model2.childAlgorithm( QStringLiteral( "cx1" ) ), modelInputs, childResults, expContext, error );
+  QVERIFY( error.isEmpty() );
   QCOMPARE( params.value( "DISSOLVE" ).toBool(), false );
   QCOMPARE( params.value( "DISTANCE" ).toInt(), 271 );
   QCOMPARE( params.value( "SEGMENTS" ).toInt(), 16 );
@@ -1218,7 +1220,8 @@ void TestQgsProcessingModelAlgorithm::modelExecution()
   alg2c2.setAlgorithmId( "native:centroids" );
   alg2c2.addParameterSources( "INPUT", QgsProcessingModelChildParameterSources() << QgsProcessingModelChildParameterSource::fromChildOutput( QStringLiteral( "cx1" ), "OUTPUT" ) );
   model2.addChildAlgorithm( alg2c2 );
-  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx2" ), modelInputs, childResults, expContext );
+  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx2" ), modelInputs, childResults, expContext, error );
+  QVERIFY( error.isEmpty() );
   QCOMPARE( params.value( "INPUT" ).toString(), QStringLiteral( "dest.shp" ) );
   QCOMPARE( params.value( "OUTPUT" ).toString(), QStringLiteral( "memory:Centroids" ) );
   QCOMPARE( params.count(), 2 );
@@ -1269,7 +1272,8 @@ void TestQgsProcessingModelAlgorithm::modelExecution()
   alg2c3.setModelOutputs( outputs3 );
 
   model2.addChildAlgorithm( alg2c3 );
-  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx3" ), modelInputs, childResults, expContext );
+  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx3" ), modelInputs, childResults, expContext, error );
+  QVERIFY( error.isEmpty() );
   QCOMPARE( params.value( "INPUT" ).toString(), QStringLiteral( "dest.shp" ) );
   QCOMPARE( params.value( "EXPRESSION" ).toString(), QStringLiteral( "true" ) );
   QVERIFY( params.value( "OUTPUT" ).canConvert<QgsProcessingOutputLayerDefinition>() );
@@ -1278,23 +1282,38 @@ void TestQgsProcessingModelAlgorithm::modelExecution()
   QCOMPARE( outDef.sink.staticValue().toString(), QStringLiteral( "memory:" ) );
   QCOMPARE( params.count(), 3 ); // don't want FAIL_OUTPUT set!
 
+  // a child with an expression which is invalid
+  QgsProcessingModelChildAlgorithm alg2c3a;
+  alg2c3a.setChildId( "cx3a" );
+  alg2c3a.setAlgorithmId( "native:extractbyexpression" );
+  alg2c3a.setDescription( "Extract by expression" );
+  alg2c3a.addParameterSources( "INPUT", QgsProcessingModelChildParameterSources() << QgsProcessingModelChildParameterSource::fromExpression( QStringLiteral( "1/'a'" ) ) );
+  alg2c3a.addParameterSources( "EXPRESSION", QgsProcessingModelChildParameterSources() << QgsProcessingModelChildParameterSource::fromStaticValue( "true" ) );
+  alg2c3a.addParameterSources( "OUTPUT", QgsProcessingModelChildParameterSources() << QgsProcessingModelChildParameterSource::fromModelParameter( "MY_OUT" ) );
+
+  model2.addChildAlgorithm( alg2c3a );
+  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx3a" ), modelInputs, childResults, expContext, error );
+  QCOMPARE( error, QStringLiteral( "Could not evaluate expression for parameter INPUT for Extract by expression: Cannot convert 'a' to double" ) );
+  model2.removeChildAlgorithm( "cx3a" );
+
   // a child with an static output value
   QgsProcessingModelChildAlgorithm alg2c4;
   alg2c4.setChildId( "cx4" );
   alg2c4.setAlgorithmId( "native:extractbyexpression" );
   alg2c4.addParameterSources( "OUTPUT", QgsProcessingModelChildParameterSources() << QgsProcessingModelChildParameterSource::fromStaticValue( "STATIC" ) );
   model2.addChildAlgorithm( alg2c4 );
-  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx4" ), modelInputs, childResults, expContext );
+  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx4" ), modelInputs, childResults, expContext, error );
+  QVERIFY( error.isEmpty() );
   QCOMPARE( params.value( "OUTPUT" ).toString(), QStringLiteral( "STATIC" ) );
   model2.removeChildAlgorithm( "cx4" );
   // expression based output value
   alg2c4.addParameterSources( "OUTPUT", QgsProcessingModelChildParameterSources() << QgsProcessingModelChildParameterSource::fromExpression( "'A' || 'B'" ) );
   model2.addChildAlgorithm( alg2c4 );
-  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx4" ), modelInputs, childResults, expContext );
+  params = model2.parametersForChildAlgorithm( model2.childAlgorithm( "cx4" ), modelInputs, childResults, expContext, error );
+  QVERIFY( error.isEmpty() );
   QCOMPARE( params.value( "OUTPUT" ).toString(), QStringLiteral( "AB" ) );
   model2.removeChildAlgorithm( "cx4" );
 
-
   variables = model2.variablesForChildAlgorithm( "cx3", context );
   QCOMPARE( variables.count(), 17 );
   QCOMPARE( variables.value( "DIST" ).source.source(), QgsProcessingModelChildParameterSource::ModelParameter );