fix(taskAttachment): prevent NPE for deletion of attachments with emp…

…ty or nonexisting taskId (camunda#4817) related to camunda#3545
xmgdtc · Dec 4, 2024 · 7e974cf · 7e974cf
1 parent 824b35f
commit 7e974cf
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 24 deletions.
diff --git a/engine/src/main/java/org/camunda/bpm/engine/impl/cmd/DeleteAttachmentCmd.java b/engine/src/main/java/org/camunda/bpm/engine/impl/cmd/DeleteAttachmentCmd.java
@@ -20,6 +20,7 @@
 
 import java.io.Serializable;
 
+import java.util.Objects;
 import org.camunda.bpm.engine.history.UserOperationLogEntry;
 import org.camunda.bpm.engine.impl.interceptor.Command;
 import org.camunda.bpm.engine.impl.interceptor.CommandContext;
@@ -49,39 +50,41 @@ public DeleteAttachmentCmd(String taskId, String attachmentId) {
 
   public Object execute(CommandContext commandContext) {
     AttachmentEntity attachment = null;
-    if (taskId != null) {
+    if (taskId != null && !taskId.isBlank()) {
       attachment = (AttachmentEntity) commandContext
           .getAttachmentManager()
           .findAttachmentByTaskIdAndAttachmentId(taskId, attachmentId);
-      ensureNotNull("No attachment exist for task id '" + taskId + " and attachmentId '" + attachmentId + "'.", "attachment", attachment);
+      ensureNotNull("No attachment exists for task id '" + taskId + " and attachmentId '" + attachmentId + "'.", "attachment", attachment);
     } else {
       attachment = commandContext
           .getDbEntityManager()
           .selectById(AttachmentEntity.class, attachmentId);
-      ensureNotNull("No attachment exist with attachmentId '" + attachmentId + "'.", "attachment", attachment);
+      ensureNotNull("No attachment exists with attachmentId '" + attachmentId + "'.", "attachment", attachment);
     }
 
     commandContext
-      .getDbEntityManager()
-      .delete(attachment);
+        .getDbEntityManager()
+        .delete(attachment);
 
     if (attachment.getContentId() != null) {
       commandContext
-        .getByteArrayManager()
-        .deleteByteArrayById(attachment.getContentId());
+          .getByteArrayManager()
+          .deleteByteArrayById(attachment.getContentId());
     }
 
-    if (attachment.getTaskId()!=null) {
+    if (attachment.getTaskId() != null && !attachment.getTaskId().isBlank()) {
       TaskEntity task = commandContext
           .getTaskManager()
           .findTaskById(attachment.getTaskId());
 
-      PropertyChange propertyChange = new PropertyChange("name", null, attachment.getName());
+      if (task != null) {
+        PropertyChange propertyChange = new PropertyChange("name", null, attachment.getName());
 
-      commandContext.getOperationLogManager()
-          .logAttachmentOperation(UserOperationLogEntry.OPERATION_TYPE_DELETE_ATTACHMENT, task, propertyChange);
+        commandContext.getOperationLogManager()
+            .logAttachmentOperation(UserOperationLogEntry.OPERATION_TYPE_DELETE_ATTACHMENT, task, propertyChange);
 
-      task.triggerUpdateEvent();
+        task.triggerUpdateEvent();
+      }
     }
 
     return null;

diff --git a/engine/src/test/java/org/camunda/bpm/engine/test/api/task/TaskServiceTest.java b/engine/src/test/java/org/camunda/bpm/engine/test/api/task/TaskServiceTest.java
@@ -1026,7 +1026,6 @@ public void testTaskAssignee() {
     taskService.deleteTask(task.getId(), true);
   }
 
-
   @Test
   public void testSaveTaskNullTask() {
     try {
@@ -1247,7 +1246,6 @@ public void testCompleteTaskWithParametersEmptyParameters() {
     assertNull(task);
   }
 
-
   @Deployment(resources = TWO_TASKS_PROCESS)
   @Test
   public void testCompleteWithParametersTask() {
@@ -2702,14 +2700,45 @@ public void testDeleteTaskAttachmentWithNullParameters() {
   }
 
   @Test
-  public void testDeleteTaskAttachmentWithTaskIdNull() {
-    int historyLevel = processEngineConfiguration.getHistoryLevel().getId();
-    if (historyLevel> ProcessEngineConfigurationImpl.HISTORYLEVEL_NONE) {
-      try {
-        taskService.deleteTaskAttachment(null, "myAttachmentId");
-        fail("expected process engine exception");
-      } catch(ProcessEngineException e) {}
-    }
+  public void testDeleteTaskAttachmentThatDoesNotExist() {
+    assertThatThrownBy(() -> taskService.deleteTaskAttachment(null, "attachmentDoesNotExist")).isInstanceOf(
+            NullValueException.class)
+        .hasMessageContaining("No attachment exists with attachmentId 'attachmentDoesNotExist'");
+  }
+
+  @Test
+  @Deployment(resources = { "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml" })
+  public void testDeleteTaskAttachmentWithTaskIdEmpty() {
+    // given
+    runtimeService.startProcessInstanceByKey("oneTaskProcess");
+    Attachment attachment = taskService.createAttachment("web page", "", null, "weatherforcast",
+        "temperatures and more", new ByteArrayInputStream("someContent".getBytes()));
+    final String attachmentId = attachment.getId();
+    assertThat(taskService.getAttachment(attachmentId)).isNotNull();
+
+    // when
+    taskService.deleteTaskAttachment("", attachmentId);
+
+    // then
+    assertThat(taskService.getAttachment(attachmentId)).isNull();
+  }
+
+  @Test
+  @Deployment(resources = { "org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml" })
+  public void testDeleteTaskAttachmentWithTaskIdNoLongerExists() {
+    // given
+    runtimeService.startProcessInstanceByKey("oneTaskProcess");
+    final String taskId = taskService.createTaskQuery().singleResult().getId();
+    final Attachment attachment = taskService.createAttachment("web page", taskId, null, "weatherforcast",
+        "temperatures and more", "http://weather.com");
+    taskService.complete(taskId);
+    final String attachmentId = attachment.getId();
+
+    // when
+    taskService.deleteTaskAttachment(taskId, attachmentId);
+
+    // then
+    assertThat(taskService.getAttachment(attachmentId)).isNull();
   }
 
   @Test
@@ -2771,7 +2800,7 @@ public void testUpdateVariablesLocalForNonExistingTaskId() {
   }
 
   @Test
-  public void testUpdateVariablesLocaForNullTaskId() {
+  public void testUpdateVariablesLocalForNullTaskId() {
     Map<String, Object> modifications = new HashMap<>();
     modifications.put("variable1", "anotherValue1");
     modifications.put("variable2", "anotherValue2");
@@ -3472,7 +3501,6 @@ public void testHandleEscalationInterruptInEventSubprocess() {
     assertEquals("bar",runtimeService.createVariableInstanceQuery().variableName("foo").singleResult().getValue());
   }
 
-
   @Test
   @Deployment(resources = { "org/camunda/bpm/engine/test/api/task/TaskServiceTest.handleUserTaskEscalation.bpmn20.xml" })
   public void testHandleEscalationNonInterruptEmbeddedSubprocess() {