Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PHWaechtler
Copy link
Contributor

@PHWaechtler PHWaechtler commented Nov 27, 2024

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 PHWaechtler force-pushed the 3545-npe-thrown-during-deletion-of-attachment branch from 9a89b08 to cd73812 Compare November 27, 2024 12:43
Comment on lines +336 to +339
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>
Copy link
Contributor Author

@PHWaechtler PHWaechtler Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lmk if we prefer not to introduce this dependency for some reason, can also just do the StringUtils.isNotBlank check manually.

@PHWaechtler PHWaechtler force-pushed the 3545-npe-thrown-during-deletion-of-attachment branch 2 times, most recently from 3859a8a to 997d9a0 Compare November 27, 2024 13:08
@@ -2228,14 +2226,47 @@ public void testDeleteTaskAttachmentWithNullParameters() {
}

@Test
public void testDeleteTaskAttachmentWithTaskIdNull() {
Copy link
Contributor Author

@PHWaechtler PHWaechtler Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context:
I changed the name for this test case because to me it looks like the exception thrown here is thrown because the attachment was never created, not because the taskId is null (also made the exception assertion more specific for this reason)

Comment on lines +2242 to +2243
Attachment attachment = taskService.createAttachment("web page", "", null, "weatherforcast",
"temperatures and more", new ByteArrayInputStream("someContent".getBytes()));
Copy link
Contributor Author

@PHWaechtler PHWaechtler Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@PHWaechtler PHWaechtler force-pushed the 3545-npe-thrown-during-deletion-of-attachment branch 3 times, most recently from 7194bf2 to 00965bc Compare November 28, 2024 11:16
@PHWaechtler PHWaechtler force-pushed the 3545-npe-thrown-during-deletion-of-attachment branch from 00965bc to a42a645 Compare November 28, 2024 11:27
@PHWaechtler PHWaechtler self-assigned this Nov 28, 2024
@PHWaechtler PHWaechtler requested review from joaquinfelici and removed request for tasso94 November 28, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant