merge: #9668

9668: Escalate errors in output mapping evaluation r=pihme a=pihme

## Description

Changes the expression which merges the result of the output mapping to be consistent

## Review Hints
There are two versions of the expression. The last one is easier to comprehend, in my mind. If you like the first one better, we can just drop the last commit.

## Related issues

closes #9543



Co-authored-by: pihme <pihme@users.noreply.github.com>

engine/src/main/java/io/camunda/zeebe/engine/processing/deployment/model/transformer/VariableMappingTransformer.java

@@ -52,8 +52,8 @@
  * </pre>
  *
  * <p>Output variable mappings differ from input mappings that the result variables needs to be
- * merged with the existing variables if the variable is a JSON object. The merging is done by
- * calling the FEEL function 'put all()' and referencing the variable.
+ * merged with the existing variables if the variable is a JSON object. The merging is done, as a
+ * first draft, by calling the FEEL function 'put all()' and referencing the variable.
  *
  * <pre>
  *   {
@@ -69,6 +69,27 @@
  *       })
  *   }
  * </pre>
+ *
+ * <p>There is one edge case, though, which was revealed in #9543: At the time of this writing the
+ * two branches of the if statement behave differently with respect to evaluation errors. The first
+ * branch (if the target variable is null) will propagate any error that occurs to the result of the
+ * evaluation. However, the second branch uses the put all(...) method which will transform any
+ * error in one of its parameters into a null as result. Therefore, the two branches will produce
+ * inconsistent results if there are errors in the expression.
+ *
+ * <p>To account for this the expression, was amended to
+ *
+ * <pre>
+ *  if (targetVar != null and is defined(outputMappingResult))
+ *    then put all(targetVar,outputMappingResult)
+ *    else outputMappingResult
+ * </pre>
+ *
+ * <p>This is a workaround which hopefully can be removed in the future. It first checks whether the
+ * result of the output mapping is defined, and the target variable exists. If this is true, the put
+ * all(...) method is called to merge the result into the existing target variable. In all other
+ * cases the output mapping result is returned as is. If it evaluates to en error, this error is
+ * propagated to the final result of the evaluation
  */
 public final class VariableMappingTransformer {
 
@@ -185,8 +206,8 @@ private String mergeContextExpression(
     // example: x = 1 and a = {'c':2} results in a = {'b':1, 'c':2}
     final var existingContext = String.join(".", contextPath);
     return String.format(
-        "if (%s = null) then %s else put all(%s,%s)",
-        existingContext, nestedContext, existingContext, nestedContext);
+        "if (%s != null and is defined(%s)) then put all(%s,%s) else %s",
+        existingContext, nestedContext, existingContext, nestedContext, nestedContext);
   }
 
   private Expression parseExpression(
@@ -245,8 +266,7 @@ public <T> T visit(final MappingContextVisitor<T> visitor) {
                     final var key = entry.getKey();
                     final var value = entry.getValue();
 
-                    if (value instanceof MappingContext) {
-                      final var nestedContext = (MappingContext) value;
+                    if (value instanceof final MappingContext nestedContext) {
                       final var contextValue = nestedContext.visit(visitor);
 
                       return visitor.onContextEntry(key, contextValue, nestedContext.path);

engine/src/test/java/io/camunda/zeebe/engine/processing/variable/mapping/VariableOutputMappingTransformerTest.java

@@ -19,30 +19,17 @@
 import java.util.List;
 import java.util.Map;
 import org.agrona.DirectBuffer;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameter;
-import org.junit.runners.Parameterized.Parameters;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 
-@RunWith(Parameterized.class)
 public final class VariableOutputMappingTransformerTest {
 
-  @Parameter(0)
-  public List<ZeebeMapping> mappings;
-
-  @Parameter(1)
-  public Map<String, DirectBuffer> variables;
-
-  @Parameter(2)
-  public String expectedOutput;
-
   private final VariableMappingTransformer transformer = new VariableMappingTransformer();
   private final ExpressionLanguage expressionLanguage =
       ExpressionLanguageFactory.createExpressionLanguage();
 
-  @Parameters(name = "with {0} to {2}")
-  public static Object[][] parameters() {
+  public static Object[][] parametersSuccessfulEvaluationToObject() {
     return new Object[][] {
       // no mappings
       {List.of(), Map.of(), "{}"},
@@ -143,8 +130,22 @@ public static Object[][] parameters() {
     };
   }
 
-  @Test
-  public void shouldApplyMappings() {
+  public static Object[][] parametersEvaluationToFailure() {
+    return new Object[][] {
+      {
+        List.of(mapping("x", "a.b")),
+        Map.of(),
+        "failed to evaluate expression '{a:if (a != null and is defined({b: x})) then put all(a,{b: x}) else {b: x}}': no variable found for name 'x'"
+      }, // #9543
+    };
+  }
+
+  @ParameterizedTest(name = "{index}: with {0} to {2}")
+  @MethodSource("parametersSuccessfulEvaluationToObject")
+  public void shouldEvaluateToObject(
+      final List<ZeebeMapping> mappings,
+      final Map<String, DirectBuffer> variables,
+      final String expectedOutput) {
     // given
     final var expression = transformer.transformOutputMappings(mappings, expressionLanguage);
 
@@ -161,6 +162,28 @@ public void shouldApplyMappings() {
     MsgPackUtil.assertEquality(result.toBuffer(), expectedOutput);
   }
 
+  @ParameterizedTest(name = "{index}: mapping {0} fails with: {2}")
+  @MethodSource("parametersEvaluationToFailure")
+  public void shouldEvaluateToFailure(
+      final List<ZeebeMapping> mappings,
+      final Map<String, DirectBuffer> variables,
+      final String failureMessage) {
+    // given
+    final var expression = transformer.transformOutputMappings(mappings, expressionLanguage);
+
+    assertThat(expression.isValid())
+        .describedAs("Expected valid expression: %s", expression.getFailureMessage())
+        .isTrue();
+
+    // when
+    final var result = expressionLanguage.evaluateExpression(expression, variables::get);
+
+    // then
+    assertThat(result.isFailure()).isTrue();
+
+    Assertions.assertThat(result.getFailureMessage()).isEqualTo(failureMessage);
+  }
+
   private static ZeebeMapping mapping(final String source, final String target) {
     return new ZeebeMapping() {
       @Override