Avoid loss of attributes when calculating fields

Correctly bail out when field calculation bar expression is invalid instead of nulling fields. Fixes #47385
qgis · Oct 9, 2023 · f35ba24 · f35ba24
1 parent 5cff1f4
commit f35ba24
Showing 1 changed file with 42 additions and 37 deletions.
diff --git a/src/app/qgsattributetabledialog.cpp b/src/app/qgsattributetabledialog.cpp
@@ -549,56 +549,61 @@ void QgsAttributeTableDialog::runFieldCalculation( QgsVectorLayer *layer, const
   int rownum = 1;
 
   QgsExpressionContext context( QgsExpressionContextUtils::globalProjectLayerScopes( layer ) );
-  exp.prepare( &context );
-
-  QgsField fld = layer->fields().at( fieldindex );
+  if ( !exp.prepare( &context ) )
+  {
+    calculationSuccess = false;
+    error = exp.evalErrorString();
+  }
+  else
+  {
+    QgsField fld = layer->fields().at( fieldindex );
 
-  QSet< QString >referencedColumns = exp.referencedColumns();
-  referencedColumns.insert( fld.name() ); // need existing column value to store old attribute when changing field values
-  request.setSubsetOfAttributes( referencedColumns, layer->fields() );
+    QSet< QString >referencedColumns = exp.referencedColumns();
+    referencedColumns.insert( fld.name() ); // need existing column value to store old attribute when changing field values
+    request.setSubsetOfAttributes( referencedColumns, layer->fields() );
 
-  //go through all the features and change the new attributes
-  QgsFeatureIterator fit = layer->getFeatures( request );
+    //go through all the features and change the new attributes
+    QgsFeatureIterator fit = layer->getFeatures( request );
 
-  std::unique_ptr< QgsScopedProxyProgressTask > task = std::make_unique< QgsScopedProxyProgressTask >( tr( "Calculating field" ) );
+    std::unique_ptr< QgsScopedProxyProgressTask > task = std::make_unique< QgsScopedProxyProgressTask >( tr( "Calculating field" ) );
 
-  long long count = !filteredIds.isEmpty() ? filteredIds.size() : layer->featureCount();
-  long long i = 0;
+    long long count = !filteredIds.isEmpty() ? filteredIds.size() : layer->featureCount();
+    long long i = 0;
 
-  QgsFeature feature;
-  while ( fit.nextFeature( feature ) )
-  {
-    if ( !filteredIds.isEmpty() && !filteredIds.contains( feature.id() ) )
+    QgsFeature feature;
+    while ( fit.nextFeature( feature ) )
     {
-      continue;
-    }
+      if ( !filteredIds.isEmpty() && !filteredIds.contains( feature.id() ) )
+      {
+        continue;
+      }
 
-    i++;
-    task->setProgress( i / static_cast< double >( count ) * 100 );
+      i++;
+      task->setProgress( i / static_cast< double >( count ) * 100 );
 
-    context.setFeature( feature );
-    context.lastScope()->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "row_number" ), rownum, true ) );
+      context.setFeature( feature );
+      context.lastScope()->addVariable( QgsExpressionContextScope::StaticVariable( QStringLiteral( "row_number" ), rownum, true ) );
 
-    QVariant value = exp.evaluate( &context );
-    ( void )fld.convertCompatible( value );
-    // Bail if we have a update error
-    if ( exp.hasEvalError() )
-    {
-      calculationSuccess = false;
-      error = exp.evalErrorString();
-      break;
-    }
-    else
-    {
-      QVariant oldvalue = feature.attributes().value( fieldindex );
-      mLayer->changeAttributeValue( feature.id(), fieldindex, value, oldvalue );
-    }
+      QVariant value = exp.evaluate( &context );
+      ( void )fld.convertCompatible( value );
+      // Bail if we have a update error
+      if ( exp.hasEvalError() )
+      {
+        calculationSuccess = false;
+        error = exp.evalErrorString();
+        break;
+      }
+      else
+      {
+        QVariant oldvalue = feature.attributes().value( fieldindex );
+        mLayer->changeAttributeValue( feature.id(), fieldindex, value, oldvalue );
+      }
 
-    rownum++;
+      rownum++;
+    }
   }
 
   cursorOverride.release();
-  task.reset();
 
   if ( !calculationSuccess )
   {