diff --git a/engine/pom.xml b/engine/pom.xml index 8f79fe1a029..d7f2228a746 100644 --- a/engine/pom.xml +++ b/engine/pom.xml @@ -333,6 +333,10 @@ ${project.version} test + + org.apache.commons + commons-lang3 + 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 index c90fe3cf051..0985375816e 100644 --- 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 index 38395799c3f..78c07b4eb2c 100644 --- 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 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() {