[Cases] Delete file attachments when files do not exist (#155088)

This PR modifies the bulk delete files API to support deleting the case
attachments even when the file does not exist. We could run into this
scenario if a user deleted the file outside of cases first and then
attempts to delete the case attachment.

package.json

@@ -850,6 +850,7 @@
     "p-limit": "^3.0.1",
     "p-map": "^4.0.0",
     "p-retry": "^4.2.0",
+    "p-settle": "4.1.1",
     "papaparse": "^5.2.0",
     "pbf": "3.2.1",
     "pdfjs-dist": "^2.13.216",
@@ -1426,6 +1427,7 @@
     "nyc": "^15.1.0",
     "oboe": "^2.1.4",
     "openapi-types": "^10.0.0",
+    "p-reflect": "2.1.0",
     "pbf": "3.2.1",
     "peggy": "^1.2.0",
     "picomatch": "^2.3.1",

x-pack/plugins/cases/server/client/attachments/bulk_delete.test.ts

@@ -0,0 +1,99 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import { loggerMock } from '@kbn/logging-mocks';
+import pReflect from 'p-reflect';
+import type { File } from '@kbn/files-plugin/common';
+import { FileNotFoundError } from '@kbn/files-plugin/server/file_service/errors';
+import { retrieveFilesIgnoringNotFound } from './bulk_delete';
+
+describe('bulk_delete', () => {
+  describe('retrieveFilesIgnoringNotFound', () => {
+    const mockLogger = loggerMock.create();
+
+    beforeEach(() => {
+      jest.clearAllMocks();
+    });
+
+    it('returns an empty array when the results is an empty array', () => {
+      expect(retrieveFilesIgnoringNotFound([], [], mockLogger)).toEqual([]);
+    });
+
+    it('returns a fulfilled file', async () => {
+      expect(retrieveFilesIgnoringNotFound([await createFakeFile()], ['abc'], mockLogger)).toEqual([
+        {},
+      ]);
+    });
+
+    it('logs a warning when encountering a file not found error', async () => {
+      const fileNotFound = await pReflect(Promise.reject(new FileNotFoundError('not found')));
+
+      expect(retrieveFilesIgnoringNotFound([fileNotFound], ['abc'], mockLogger)).toEqual([]);
+      expect(mockLogger.warn).toBeCalledTimes(1);
+      expect(mockLogger.warn.mock.calls[0]).toMatchInlineSnapshot(`
+        Array [
+          "Failed to find file id: abc: Error: not found",
+        ]
+      `);
+    });
+
+    it('logs a warning without the fileId when the results length is different from the file ids', async () => {
+      const fileNotFound = await pReflect(Promise.reject(new FileNotFoundError('not found')));
+
+      expect(retrieveFilesIgnoringNotFound([fileNotFound], ['abc', '123'], mockLogger)).toEqual([]);
+      expect(mockLogger.warn).toBeCalledTimes(1);
+      expect(mockLogger.warn.mock.calls[0]).toMatchInlineSnapshot(`
+        Array [
+          "Failed to find file: Error: not found",
+        ]
+      `);
+    });
+
+    it('throws when encountering an error that is not a file not found', async () => {
+      const otherError = new Error('other error');
+      const otherErrorResult = await pReflect(Promise.reject(new Error('other error')));
+
+      expect.assertions(2);
+
+      expect(() =>
+        retrieveFilesIgnoringNotFound([otherErrorResult], ['abc'], mockLogger)
+      ).toThrowError(otherError);
+      expect(mockLogger.warn).not.toBeCalled();
+    });
+
+    it('throws when encountering an error that is not a file not found after a valid file', async () => {
+      const otherError = new Error('other error');
+      const otherErrorResult = await pReflect(Promise.reject(otherError));
+      const fileResult = await createFakeFile();
+
+      expect.assertions(2);
+
+      expect(() =>
+        retrieveFilesIgnoringNotFound([fileResult, otherErrorResult], ['1', '2'], mockLogger)
+      ).toThrowError(otherError);
+      expect(mockLogger.warn).not.toBeCalled();
+    });
+
+    it('throws a new error when encountering an error that is a string', async () => {
+      // this produces an error because .reject() must be passed an error but I want to test a string just in case
+      // eslint-disable-next-line prefer-promise-reject-errors
+      const otherErrorResult = await pReflect(Promise.reject('string error'));
+      const fileResult = await createFakeFile();
+
+      expect.assertions(2);
+
+      expect(() =>
+        retrieveFilesIgnoringNotFound([fileResult, otherErrorResult], ['1', '2'], mockLogger)
+      ).toThrowErrorMatchingInlineSnapshot(`"Failed to retrieve file id: 2: string error"`);
+      expect(mockLogger.warn).not.toBeCalled();
+    });
+  });
+});
+
+const createFakeFile = () => {
+  return pReflect(Promise.resolve({} as File));
+};

x-pack/plugins/cases/server/client/attachments/bulk_delete.ts

@@ -10,8 +10,10 @@ import { pipe } from 'fp-ts/lib/pipeable';
 import { fold } from 'fp-ts/lib/Either';
 import { identity } from 'fp-ts/lib/function';
 
-import pMap from 'p-map';
+import type { PromiseResult, PromiseRejectedResult } from 'p-settle';
+import pSettle from 'p-settle';
 import { partition } from 'lodash';
+import type { Logger } from '@kbn/core/server';
 import type { File, FileJSON } from '@kbn/files-plugin/common';
 import type { FileServiceStart } from '@kbn/files-plugin/server';
 import { FileNotFoundError } from '@kbn/files-plugin/server/file_service/errors';
@@ -46,7 +48,12 @@ export const bulkDeleteFileAttachments = async (
 
     await casesClient.cases.resolve({ id: caseId, includeComments: false });
 
-    const fileEntities = await getFileEntities(caseId, request.ids, fileService);
+    const fileEntities = await getFileEntities({
+      caseId,
+      fileIds: request.ids,
+      fileService,
+      logger,
+    });
 
     // It's possible for this to return an empty array if there was an error creating file attachments in which case the
     // file would be present but the case attachment would not
@@ -67,7 +74,10 @@ export const bulkDeleteFileAttachments = async (
     });
 
     await Promise.all([
-      deleteFiles(request.ids, fileService),
+      deleteFiles(
+        fileEntities.map((entity) => entity.id),
+        fileService
+      ),
       attachmentService.bulkDelete({
         attachmentIds: fileAttachments.map((so) => so.id),
         refresh: false,
@@ -84,43 +94,53 @@ export const bulkDeleteFileAttachments = async (
       user,
     });
   } catch (error) {
-    let errorToTrack = error;
-
-    // if it's an error from the file service let's put it in a boom so we don't loose the status code of a 404
-    if (error instanceof FileNotFoundError) {
-      errorToTrack = Boom.notFound(error.message);
-    }
-
     throw createCaseError({
       message: `Failed to delete file attachments for case: ${caseId}: ${error}`,
-      error: errorToTrack,
+      error,
       logger,
     });
   }
 };
 
-const getFileEntities = async (
-  caseId: BulkDeleteFileArgs['caseId'],
-  fileIds: BulkDeleteFileArgs['fileIds'],
-  fileService: FileServiceStart
-) => {
-  const files = await getFiles(caseId, fileIds, fileService);
+const getFileEntities = async ({
+  caseId,
+  fileIds,
+  fileService,
+  logger,
+}: {
+  caseId: BulkDeleteFileArgs['caseId'];
+  fileIds: BulkDeleteFileArgs['fileIds'];
+  fileService: FileServiceStart;
+  logger: Logger;
+}) => {
+  const files = await getFiles({ caseId, fileIds, fileService, logger });
 
   const fileEntities = createFileEntities(files);
 
   return fileEntities;
 };
 
-const getFiles = async (
-  caseId: BulkDeleteFileArgs['caseId'],
-  fileIds: BulkDeleteFileArgs['fileIds'],
-  fileService: FileServiceStart
-): Promise<FileJSON[]> => {
+const getFiles = async ({
+  caseId,
+  fileIds,
+  fileService,
+  logger,
+}: {
+  caseId: BulkDeleteFileArgs['caseId'];
+  fileIds: BulkDeleteFileArgs['fileIds'];
+  fileService: FileServiceStart;
+  logger: Logger;
+}): Promise<FileJSON[]> => {
   // it's possible that we're trying to delete a file when an attachment wasn't created (for example if the create
   // attachment request failed)
-  const files = await pMap(fileIds, async (fileId: string) => fileService.getById({ id: fileId }), {
-    concurrency: MAX_CONCURRENT_SEARCHES,
-  });
+  const fileSettleResults = await pSettle(
+    fileIds.map(async (fileId) => fileService.getById({ id: fileId })),
+    {
+      concurrency: MAX_CONCURRENT_SEARCHES,
+    }
+  );
+
+  const files = retrieveFilesIgnoringNotFound(fileSettleResults, fileIds, logger);
 
   const [validFiles, invalidFiles] = partition(files, (file) => {
     return (
@@ -137,9 +157,52 @@ const getFiles = async (
     throw Boom.badRequest(`Failed to delete files because filed ids were invalid: ${invalidIds}`);
   }
 
-  if (validFiles.length <= 0) {
-    throw Boom.badRequest('Failed to find files to delete');
-  }
-
   return validFiles.map((fileInfo) => fileInfo.data);
 };
+
+export const retrieveFilesIgnoringNotFound = (
+  results: Array<PromiseResult<File<unknown>>>,
+  fileIds: BulkDeleteFileArgs['fileIds'],
+  logger: Logger
+) => {
+  const files: File[] = [];
+
+  results.forEach((result, index) => {
+    if (result.isFulfilled) {
+      files.push(result.value);
+    } else if (result.reason instanceof FileNotFoundError) {
+      const warningMessage = getFileNotFoundErrorMessage({
+        resultsLength: results.length,
+        fileIds,
+        index,
+        result,
+      });
+
+      logger.warn(warningMessage);
+    } else if (result.reason instanceof Error) {
+      throw result.reason;
+    } else {
+      throw new Error(`Failed to retrieve file id: ${fileIds[index]}: ${result.reason}`);
+    }
+  });
+
+  return files;
+};
+
+const getFileNotFoundErrorMessage = ({
+  resultsLength,
+  fileIds,
+  index,
+  result,
+}: {
+  resultsLength: number;
+  fileIds: BulkDeleteFileArgs['fileIds'];
+  index: number;
+  result: PromiseRejectedResult;
+}) => {
+  if (resultsLength === fileIds.length) {
+    return `Failed to find file id: ${fileIds[index]}: ${result.reason}`;
+  }
+
+  return `Failed to find file: ${result.reason}`;
+};

x-pack/plugins/cases/server/client/files/index.test.ts

@@ -51,6 +51,15 @@ describe('server files', () => {
   });
 
   describe('deleteFiles', () => {
+    it('does not call delete when the file ids is empty', async () => {
+      const fileServiceMock = createFileServiceMock();
+
+      expect.assertions(1);
+      await deleteFiles([], fileServiceMock);
+
+      expect(fileServiceMock.delete).not.toBeCalled();
+    });
+
     it('calls delete twice with the ids passed in', async () => {
       const fileServiceMock = createFileServiceMock();
 

x-pack/plugins/cases/server/client/files/index.ts

@@ -33,7 +33,12 @@ export const createFileEntities = (files: FileEntityInfo[]): OwnerEntity[] => {
   return fileEntities;
 };
 
-export const deleteFiles = async (fileIds: string[], fileService: FileServiceStart) =>
-  pMap(fileIds, async (fileId: string) => fileService.delete({ id: fileId }), {
+export const deleteFiles = async (fileIds: string[], fileService: FileServiceStart) => {
+  if (fileIds.length <= 0) {
+    return;
+  }
+
+  return pMap(fileIds, async (fileId: string) => fileService.delete({ id: fileId }), {
     concurrency: MAX_CONCURRENT_SEARCHES,
   });
+};

x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_delete_file_attachments.ts

@@ -121,15 +121,6 @@ export default ({ getService }: FtrProviderContext): void => {
         });
       });
 
-      it('fails to delete a file when the file does not exist', async () => {
-        await bulkDeleteFileAttachments({
-          supertest,
-          caseId: postedCase.id,
-          fileIds: ['abc'],
-          expectedHttpCode: 404,
-        });
-      });
-
       it('returns a 400 when the fileIds is an empty array', async () => {
         await bulkDeleteFileAttachments({
           supertest,
@@ -254,6 +245,17 @@ export default ({ getService }: FtrProviderContext): void => {
         await deleteAllCaseItems(es);
       });
 
+      it('returns a 204 when the file does not exist', async () => {
+        const postedCase = await createCase(supertest, getPostCaseRequest());
+
+        await bulkDeleteFileAttachments({
+          supertest,
+          caseId: postedCase.id,
+          fileIds: ['abc'],
+          expectedHttpCode: 204,
+        });
+      });
+
       it('deletes a file when the owner is not formatted as an array of strings', async () => {
         const postedCase = await createCase(supertest, getPostCaseRequest());
 
@@ -412,6 +414,37 @@ export default ({ getService }: FtrProviderContext): void => {
         await deleteAllCaseItems(es);
       });
 
+      it('deletes the attachment even when the file does not exist', async () => {
+        const postedCase = await createCase(
+          supertest,
+          getPostCaseRequest({ owner: 'securitySolution' })
+        );
+
+        const caseWithAttachments = await bulkCreateAttachments({
+          supertest,
+          caseId: postedCase.id,
+          params: [
+            getFilesAttachmentReq({
+              externalReferenceId: 'abc',
+              owner: 'securitySolution',
+            }),
+          ],
+        });
+
+        await bulkDeleteFileAttachments({
+          supertest,
+          caseId: postedCase.id,
+          fileIds: ['abc'],
+        });
+
+        await getComment({
+          supertest,
+          caseId: postedCase.id,
+          commentId: caseWithAttachments.comments![0].id,
+          expectedHttpCode: 404,
+        });
+      });
+
       it('deletes a single file', async () => {
         const postedCase = await createCase(
           supertest,

yarn.lock

@@ -22312,7 +22312,7 @@ p-limit@^1.1.0:
   dependencies:
     p-try "^1.0.0"
 
-p-limit@^2.0.0, p-limit@^2.2.0, p-limit@^2.3.0:
+p-limit@^2.0.0, p-limit@^2.2.0, p-limit@^2.2.2, p-limit@^2.3.0:
   version "2.3.0"
   resolved "https://registry.yarnpkg.com/p-limit/-/p-limit-2.3.0.tgz#3dd33c647a214fdfffd835933eb086da0dc21db1"
   integrity sha512-//88mFWSJx8lxCzwdAABTJL2MyWB12+eIY7MDL2SqLmAkeKU9qxRvWuSyTjm3FUmpBEMuFfckAIqEaVGUDxb6w==
@@ -22373,6 +22373,11 @@ p-map@^4.0.0:
   dependencies:
     aggregate-error "^3.0.0"
 
+p-reflect@2.1.0, p-reflect@^2.1.0:
+  version "2.1.0"
+  resolved "https://registry.yarnpkg.com/p-reflect/-/p-reflect-2.1.0.tgz#5d67c7b3c577c4e780b9451fc9129675bd99fe67"
+  integrity sha512-paHV8NUz8zDHu5lhr/ngGWQiW067DK/+IbJ+RfZ4k+s8y4EKyYCz8pGYWjxCg35eHztpJAt+NUgvN4L+GCbPlg==
+
 p-retry@^4.2.0, p-retry@^4.5.0:
   version "4.5.0"
   resolved "https://registry.yarnpkg.com/p-retry/-/p-retry-4.5.0.tgz#6685336b3672f9ee8174d3769a660cb5e488521d"
@@ -22381,6 +22386,14 @@ p-retry@^4.2.0, p-retry@^4.5.0:
     "@types/retry" "^0.12.0"
     retry "^0.12.0"
 
+p-settle@4.1.1:
+  version "4.1.1"
+  resolved "https://registry.yarnpkg.com/p-settle/-/p-settle-4.1.1.tgz#37fbceb2b02c9efc28658fc8d36949922266035f"
+  integrity sha512-6THGh13mt3gypcNMm0ADqVNCcYa3BK6DWsuJWFCuEKP1rpY+OKGp7gaZwVmLspmic01+fsg/fN57MfvDzZ/PuQ==
+  dependencies:
+    p-limit "^2.2.2"
+    p-reflect "^2.1.0"
+
 p-timeout@^2.0.1:
   version "2.0.1"
   resolved "https://registry.yarnpkg.com/p-timeout/-/p-timeout-2.0.1.tgz#d8dd1979595d2dc0139e1fe46b8b646cb3cdf038"