fix(taskAttachment): prevent NPE for deletion of attachments with empty or nonexisting taskId

[PHWaechtler]

related to #3545

• adds checks for empty or nonExistent taskIds when deleting a taskAttachment

Behaviour is now:
Deleting a task attachment providing an empty taskID:

• it will try to find the attachment via attachmentId. If no attachment found, exception is returned stating no attachment exists
• if an attachment with the given attachmentId exists, it will delete this attachment.
• no attempt is made to update the nonexisting task, avoiding the NPE

Deleting a task attachment with an ID of a task that has already been completed:

• if the attachment still exists, the attachment will be deleted
• no attempt is made to update the nonexisting task, avoiding the NPE

Note:
Due to this existing implementation, the NullValueException thrown in the Delete command will always be returned as an InvalidRequestException as the API response. I am assuming this is expected behaviour, but lmk if that should be adjusted in any way.

[PHWaechtler]

Suggested follow up:
Based on my understanding of the issue, I think it would also make sense to look into prohibiting the creation of attachments with empty taskId and null (or empty) processInstanceId. At the moment, this is still allowed because the creation only checks for the taskId not being null. I can open a follow up ticket if you agree that this makes sense

[joaquinfelici]

So basically without your fix, we can create a taskless attachment but not delete it, but with your changes, we'll be able to do both, is that correct?

[PHWaechtler]

Correct, but its more about creating an attachment without taskId and without processInstanceId. My understanding is that it should have either one or the other, but that its fine for it to not have a taskId as long as it still has a processInstanceId.

[PHWaechtler]

@joaquinfelici can you let me know about whether to open a follow up for this?

[joaquinfelici]

From what we can see in the create attachment documentation, what you say makes sense.

@param processInstanceId - id of a process to use if task id is null

I guess we can create a follow-up ticket to prevent that both taskId and processInstanceId are null on creation.

[PHWaechtler]

Here is the follow up ticket

[joaquinfelici]

Look good! Added some question for clarification and minor suggestions.

[joaquinfelici]

So basically without your fix, we can create a taskless attachment but not delete it, but with your changes, we'll be able to do both, is that correct?