refactor(engine): incorporate review feedback

engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/MultiInstanceOutputCollectionBehavior.java

@@ -14,10 +14,12 @@
 import io.camunda.zeebe.engine.processing.common.Failure;
 import io.camunda.zeebe.engine.processing.deployment.model.element.ExecutableMultiInstanceBody;
 import io.camunda.zeebe.msgpack.spec.MsgPackReader;
+import io.camunda.zeebe.msgpack.spec.MsgPackToken;
 import io.camunda.zeebe.msgpack.spec.MsgPackType;
 import io.camunda.zeebe.msgpack.spec.MsgPackWriter;
 import io.camunda.zeebe.protocol.record.value.ErrorType;
 import io.camunda.zeebe.util.Either;
+import java.util.Optional;
 import org.agrona.DirectBuffer;
 import org.agrona.ExpandableArrayBuffer;
 import org.agrona.concurrent.UnsafeBuffer;
@@ -116,24 +118,13 @@ private Either<Failure, DirectBuffer> replaceAt(
 
     outputCollectionReader.wrap(array, 0, array.capacity());
     final var token = outputCollectionReader.readToken();
-    if (token.getType() != MsgPackType.ARRAY) {
-      return Either.left(
-          new Failure(
-              "Unable to update item in output collection '%s' because the type of the variable is: %s. This happens when multiple BPMN elements write to the same variable."
-                  .formatted(bufferAsString(variableName), token.getType()),
-              ErrorType.EXTRACT_VALUE_ERROR,
-              variableScopeKey));
-    }
 
-    final int size = token.getSize();
-    if (index > size) {
-      return Either.left(
-          new Failure(
-              "Unable to update item in output collection '%s' at position %d because the size of the collection is: %d. This happens when multiple BPMN elements write to the same variable."
-                  .formatted(bufferAsString(variableName), index, size),
-              ErrorType.EXTRACT_VALUE_ERROR,
-              variableScopeKey));
+    final var optValidationFailure =
+        validateIsCollectionAndHasAppropriateSIze(index, variableScopeKey, variableName, token);
+    if (optValidationFailure.isPresent()) {
+      return Either.left(optValidationFailure.get());
     }
+
     outputCollectionReader.skipValues((long) index - 1L);
 
     final var offsetBefore = outputCollectionReader.getOffset();
@@ -150,4 +141,30 @@ private Either<Failure, DirectBuffer> replaceAt(
     updatedOutputCollectionBuffer.wrap(outputCollectionBuffer, 0, length);
     return Either.right(updatedOutputCollectionBuffer);
   }
+
+  private Optional<Failure> validateIsCollectionAndHasAppropriateSIze(
+      final int index,
+      final long variableScopeKey,
+      final DirectBuffer variableName,
+      final MsgPackToken token) {
+    if (token.getType() != MsgPackType.ARRAY) {
+      return Optional.of(
+          new Failure(
+              "Unable to update an item in output collection '%s' because the type of the output collection is: %s. This may happen when multiple BPMN elements write to the same variable."
+                  .formatted(bufferAsString(variableName), token.getType()),
+              ErrorType.EXTRACT_VALUE_ERROR,
+              variableScopeKey));
+    }
+
+    final int size = token.getSize();
+    if (index > size) {
+      return Optional.of(
+          new Failure(
+              "Unable to update an item in output collection '%s' at position %d because the size of the collection is: %d. This may happen when multiple BPMN elements write to the same variable."
+                  .formatted(bufferAsString(variableName), index, size),
+              ErrorType.EXTRACT_VALUE_ERROR,
+              variableScopeKey));
+    }
+    return Optional.empty();
+  }
 }

engine/src/test/java/io/camunda/zeebe/engine/processing/bpmn/behavior/MultiInstanceOutputCollectionBehaviorTest.java

@@ -36,6 +36,14 @@
 
 @ExtendWith(MockitoExtension.class)
 public class MultiInstanceOutputCollectionBehaviorTest {
+  /*
+   * Note: this test class caused some controversy between the author and the reviewer. The reviewer
+   * feared that it will too costly to maintain and doesn't provide much value in return. The author
+   * felt it will be alright. We didn't manage to come to an agreement, so we leave it to you, future
+   * developer. If you had to maintain the test and feel it was too cumbersome and didn't add value,
+   * feel free to remove this test. If you felt maintaining it was ok, and the test does have value,
+   * feel free to remove this comment.
+   */
 
   @Test // regression test for #9143
   void shouldReturnFailureWhenWritingToOutputCollectionOutOfBounds() {
@@ -81,7 +89,7 @@ void shouldReturnFailureWhenWritingToOutputCollectionOutOfBounds() {
     assertThat(failure.getErrorType()).isEqualTo(ErrorType.EXTRACT_VALUE_ERROR);
     assertThat(failure.getMessage())
         .isEqualTo(
-            "Unable to update item in output collection 'OUTPUT_COLLECTION' at position 2 because the size of the collection is: 1. This happens when multiple BPMN elements write to the same variable.");
+            "Unable to update an item in output collection 'OUTPUT_COLLECTION' at position 2 because the size of the collection is: 1. This may happen when multiple BPMN elements write to the same variable.");
     assertThat(failure.getVariableScopeKey()).isEqualTo(flowScopeContextKey);
   }
 
@@ -129,7 +137,7 @@ void shouldReturnFailureWhenWritingToOutputCollectionWhichIsNotArray() {
     assertThat(failure.getErrorType()).isEqualTo(ErrorType.EXTRACT_VALUE_ERROR);
     assertThat(failure.getMessage())
         .isEqualTo(
-            "Unable to update item in output collection 'OUTPUT_COLLECTION' because the type of the variable is: STRING. This happens when multiple BPMN elements write to the same variable.");
+            "Unable to update an item in output collection 'OUTPUT_COLLECTION' because the type of the output collection is: STRING. This may happen when multiple BPMN elements write to the same variable.");
     assertThat(failure.getVariableScopeKey()).isEqualTo(flowScopeContextKey);
   }
 

engine/src/test/java/io/camunda/zeebe/engine/processing/incident/MultiInstanceIncidentTest.java

@@ -636,10 +636,10 @@ public void shouldResolveIncidentDueToCompletionCondition() {
    * size.
    */
   @Test // regression test for #9143
-  public void shouldCreateIncidentIfOutputElementCannotBeReplacedInOutputCollection() {
+  public void shouldCreateAndResolveIncidentIfOutputElementCannotBeReplacedInOutputCollection() {
     // given
     final var processId = "index-out-of-bounds-in-output-collection";
-    final var collectionWithThreeElements = "=[1,2,3]";
+    final var collectionWithThreeElements = "=[1]";
     final var collectionWithNoElements = "=[]";
     final var outputCollectionName = "outputItems";
 
@@ -649,10 +649,10 @@ public void shouldCreateIncidentIfOutputElementCannotBeReplacedInOutputCollectio
 
     ENGINE.deployment().withXmlResource(process).deploy();
 
-    // when
+    // when (raise incident)
     final var processInstanceKey = ENGINE.processInstance().ofBpmnProcessId(processId).create();
 
-    // then
+    // then (incident is raised)
     final Record<IncidentRecordValue> incidentEvent =
         RecordingExporter.incidentRecords(IncidentIntent.CREATED)
             .withProcessInstanceKey(processInstanceKey)
@@ -661,38 +661,18 @@ public void shouldCreateIncidentIfOutputElementCannotBeReplacedInOutputCollectio
     Assertions.assertThat(incidentEvent.getValue())
         .hasErrorType(ErrorType.EXTRACT_VALUE_ERROR)
         .hasErrorMessage(
-            "Unable to update item in output collection 'outputItems' at position 1 because the size of the collection is: 0. This happens when multiple BPMN elements write to the same variable.")
+            "Unable to update an item in output collection 'outputItems' at position 1 because the size of the collection is: 0. This may happen when multiple BPMN elements write to the same variable.")
         .hasProcessInstanceKey(processInstanceKey);
-  }
 
-  @Test
-  public void shouldResolveIncidentRaisedByOutputElementCannotBeReplacedInOutputCollection() {
-    // given
-    final var processId = "index-out-of-bounds-in-output-collection";
-    final var collectionWithOneElement = "=[1]";
-    final var collectionWithNoElements = "=[]";
-    final var outputCollectionName = "outputItems";
-
-    final var process =
-        createProcessThatModifiesOutputCollection(
-            processId, collectionWithOneElement, collectionWithNoElements, outputCollectionName);
-
-    ENGINE.deployment().withXmlResource(process).deploy();
-    final var processInstanceKey = ENGINE.processInstance().ofBpmnProcessId(processId).create();
-
-    final Record<IncidentRecordValue> incidentEvent =
-        RecordingExporter.incidentRecords(IncidentIntent.CREATED)
-            .withProcessInstanceKey(processInstanceKey)
-            .getFirst();
-    // when
+    // when (resolve incident)
     ENGINE
         .variables()
         .ofScope(incidentEvent.getValue().getVariableScopeKey())
         .withDocument(Maps.of(entry(outputCollectionName, List.of(1))))
         .update();
     ENGINE.incident().ofInstance(processInstanceKey).withKey(incidentEvent.getKey()).resolve();
 
-    // then the process is able to complete
+    // then (incident is resolved)
     assertThat(
             RecordingExporter.processInstanceRecords(ProcessInstanceIntent.ELEMENT_COMPLETED)
                 .withElementType(BpmnElementType.PROCESS)
@@ -701,17 +681,24 @@ public void shouldResolveIncidentRaisedByOutputElementCannotBeReplacedInOutputCo
                 .exists())
         .describedAs("the process has completed")
         .isTrue();
+    org.assertj.core.api.Assertions.assertThat(
+            RecordingExporter.variableRecords(VariableIntent.UPDATED)
+                .withProcessInstanceKey(processInstanceKey)
+                .withName(outputCollectionName)
+                .withValue("[1]")
+                .exists())
+        .isTrue();
   }
 
   /**
    * This test covers the scenario where a multi instance cannot be completed, because updating the
    * output collection fails because the output collection is not an array.
    */
   @Test
-  public void shouldCreateIncidentIfOutputCollectionHasWrongType() {
+  public void shouldCreateAndResolveIncidentIfOutputCollectionHasWrongType() {
     // given
     final var processId = "output-collection-is-overwritten-by-string";
-    final var collectionWithThreeElements = "=[1,2,3]";
+    final var collectionWithThreeElements = "=[1]";
     final var overwriteWithString = "=\"String overwrite\"";
     final var outputCollectionName = "outputItems";
 
@@ -721,10 +708,10 @@ public void shouldCreateIncidentIfOutputCollectionHasWrongType() {
 
     ENGINE.deployment().withXmlResource(process).deploy();
 
-    // when
+    // when (raise incident)
     final var processInstanceKey = ENGINE.processInstance().ofBpmnProcessId(processId).create();
 
-    // then
+    // then (incident is raised)
     final Record<IncidentRecordValue> incidentEvent =
         RecordingExporter.incidentRecords(IncidentIntent.CREATED)
             .withProcessInstanceKey(processInstanceKey)
@@ -733,38 +720,18 @@ public void shouldCreateIncidentIfOutputCollectionHasWrongType() {
     Assertions.assertThat(incidentEvent.getValue())
         .hasErrorType(ErrorType.EXTRACT_VALUE_ERROR)
         .hasErrorMessage(
-            "Unable to update item in output collection 'outputItems' because the type of the variable is: STRING. This happens when multiple BPMN elements write to the same variable.")
+            "Unable to update an item in output collection 'outputItems' because the type of the output collection is: STRING. This may happen when multiple BPMN elements write to the same variable.")
         .hasProcessInstanceKey(processInstanceKey);
-  }
-
-  @Test
-  public void shouldResolveIncidentRaisedByOutputCollectionHasWrongType() {
-    // given
-    final var processId = "output-collection-is-overwritten-by-string";
-    final var collectionWithOneElement = "=[1]";
-    final var overwriteWithString = "=\"String overwrite\"";
-    final var outputCollectionName = "outputItems";
-
-    final var process =
-        createProcessThatModifiesOutputCollection(
-            processId, collectionWithOneElement, overwriteWithString, outputCollectionName);
-
-    ENGINE.deployment().withXmlResource(process).deploy();
-    final var processInstanceKey = ENGINE.processInstance().ofBpmnProcessId(processId).create();
 
-    final Record<IncidentRecordValue> incidentEvent =
-        RecordingExporter.incidentRecords(IncidentIntent.CREATED)
-            .withProcessInstanceKey(processInstanceKey)
-            .getFirst();
-    // when
+    // when (resolve incident)
     ENGINE
         .variables()
         .ofScope(incidentEvent.getValue().getVariableScopeKey())
         .withDocument(Maps.of(entry(outputCollectionName, List.of(1))))
         .update();
     ENGINE.incident().ofInstance(processInstanceKey).withKey(incidentEvent.getKey()).resolve();
 
-    // then the process is able to complete
+    // then (incident is resolved)
     assertThat(
             RecordingExporter.processInstanceRecords(ProcessInstanceIntent.ELEMENT_COMPLETED)
                 .withElementType(BpmnElementType.PROCESS)
@@ -773,6 +740,14 @@ public void shouldResolveIncidentRaisedByOutputCollectionHasWrongType() {
                 .exists())
         .describedAs("the process has completed")
         .isTrue();
+
+    org.assertj.core.api.Assertions.assertThat(
+            RecordingExporter.variableRecords(VariableIntent.UPDATED)
+                .withProcessInstanceKey(processInstanceKey)
+                .withName(outputCollectionName)
+                .withValue("[1]")
+                .exists())
+        .isTrue();
   }
 
   private BpmnModelInstance createProcessThatModifiesOutputCollection(