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

…ty or nonexisting taskId related to #3545
camunda · Nov 28, 2024 · 00965bc · 00965bc
1 parent 736692f
commit 00965bc
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 20 deletions.
diff --git a/engine/pom.xml b/engine/pom.xml
@@ -333,6 +333,10 @@
       <version>${project.version}</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-lang3</artifactId>
+    </dependency>
   </dependencies>
 
 

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 org.apache.commons.lang3.StringUtils;
 import org.camunda.bpm.engine.history.UserOperationLogEntry;
 import org.camunda.bpm.engine.impl.interceptor.Command;
 import org.camunda.bpm.engine.impl.interceptor.CommandContext;
@@ -49,16 +50,16 @@ public DeleteAttachmentCmd(String taskId, String attachmentId) {
 
   public Object execute(CommandContext commandContext) {
     AttachmentEntity attachment = null;
-    if (taskId != null) {
+    if (StringUtils.isNotBlank(taskId)) {
       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
@@ -71,17 +72,19 @@ public Object execute(CommandContext commandContext) {
         .deleteByteArrayById(attachment.getContentId());
     }
 
-    if (attachment.getTaskId()!=null) {
+    if (StringUtils.isNotBlank(attachment.getTaskId())) {
       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
@@ -552,7 +552,6 @@ public void testTaskAssignee() {
     taskService.deleteTask(task.getId(), true);
   }
 
-
   @Test
   public void testSaveTaskNullTask() {
     try {
@@ -773,7 +772,6 @@ public void testCompleteTaskWithParametersEmptyParameters() {
     assertNull(task);
   }
 
-
   @Deployment(resources = TWO_TASKS_PROCESS)
   @Test
   public void testCompleteWithParametersTask() {
@@ -2228,14 +2226,48 @@ 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) {}
-    }
+  @RequiredHistoryLevel(ProcessEngineConfiguration.HISTORY_NONE)
+  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" })
+  @RequiredHistoryLevel(ProcessEngineConfiguration.HISTORY_NONE)
+  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" })
+  @RequiredHistoryLevel(ProcessEngineConfiguration.HISTORY_AUDIT)
+  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
@@ -2297,7 +2329,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");
@@ -2998,7 +3030,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() {