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.

Sign up for GitHub

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

Jump to bottom

[Files] Adds bulk delete method #155628

Merged
merged 4 commits into from
Apr 26, 2023
Merged

[Files] Adds bulk delete method #155628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9380cdf
implement bulkDelete in files service
vadimkibana Apr 24, 2023
017d025
add bulk deletion tests
vadimkibana Apr 24, 2023
9852ddd
Merge branch 'main' into bulk-delete
vadimkibana Apr 24, 2023
d430f5b
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/plugins/files/server/file_service/file_action_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ export interface DeleteFileArgs {
id: string;
}

/**
* Arguments to delete files in a bulk request.
*/
export interface BulkDeleteFilesArgs {
/**
* File IDs.
*/
ids: string[];
}

/**
* Arguments to get a file by ID.
*/
Expand Down
8 changes: 8 additions & 0 deletions src/plugins/files/server/file_service/file_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {
CreateFileArgs,
UpdateFileArgs,
DeleteFileArgs,
BulkDeleteFilesArgs,
GetByIdArgs,
FindFileArgs,
} from './file_action_types';
Expand Down Expand Up @@ -43,6 +44,13 @@ export interface FileServiceStart {
*/
delete(args: DeleteFileArgs): Promise<void>;

/**
* Delete multiple files at once.
*
* @param args - delete files args
*/
bulkDelete(args: BulkDeleteFilesArgs): Promise<Array<PromiseSettledResult<void>>>;

/**
* Get a file by ID. Will throw if file cannot be found.
*
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/files/server/file_service/file_service_factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ export class FileServiceFactoryImpl implements FileServiceFactory {
await internalFileService.updateFile(args);
},
async delete(args) {
return internalFileService.deleteFile(args);
return await internalFileService.deleteFile(args);
},
async bulkDelete(args) {
return await internalFileService.bulkDeleteFiles(args);
},
async getById<M>(args: GetByIdArgs) {
return internalFileService.getById(args) as Promise<File<M>>;
Expand Down
13 changes: 13 additions & 0 deletions src/plugins/files/server/file_service/internal_file_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import { Logger, SavedObjectsErrorHelpers } from '@kbn/core/server';
import { AuditEvent, AuditLogger } from '@kbn/security-plugin/server';
import pLimit from 'p-limit';

import { BlobStorageService } from '../blob_storage_service';
import { InternalFileShareService } from '../file_share_service';
Expand All @@ -20,10 +21,14 @@ import type {
CreateFileArgs,
UpdateFileArgs,
DeleteFileArgs,
BulkDeleteFilesArgs,
FindFileArgs,
GetByIdArgs,
} from './file_action_types';
import { createFileClient, FileClientImpl } from '../file_client/file_client';

const bulkDeleteConcurrency = pLimit(10);

/**
* Service containing methods for working with files.
*
Expand Down Expand Up @@ -64,6 +69,14 @@ export class InternalFileService {
await file.delete();
}

public async bulkDeleteFiles({
ids,
}: BulkDeleteFilesArgs): Promise<Array<PromiseSettledResult<void>>> {
const promises = ids.map((id) => bulkDeleteConcurrency(() => this.deleteFile({ id })));
Copy link
Contributor

Choose a reason for hiding this comment

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

I know there's complexities here that I'm not aware of but would it be possible to use the saved object client's bulkDelete functionality?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the private get below you can see that there is some logic encapsulated. It converts the id to a File instance and calls its delete() method which then delete the content from the blob storage, calls the updateFileState to change the state, delete the SO and finally logs some events logAuditEvent 😊

That's what we hook into.

Copy link
Contributor Author

@vadimkibana vadimkibana Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

+1 to what @sebelga mentioned, essentially, before even deleting the saved object metadata document, we need to delete the blob contents first, which is stored in a different place.

const result = await Promise.allSettled(promises);
return result;
}

private async get(id: string) {
try {
const { metadata } = await this.metadataClient.get({ id });
Expand Down
23 changes: 21 additions & 2 deletions src/plugins/files/server/integration_tests/file_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ describe('FileService', () => {
return file;
}
afterEach(async () => {
await Promise.all(disposables.map((file) => file.delete()));
await fileService.bulkDelete({ ids: disposables.map((d) => d.id) });
const { files } = await fileService.find({ kind: [fileKind] });
expect(files.length).toBe(0);
disposables = [];
Expand Down Expand Up @@ -233,14 +233,33 @@ describe('FileService', () => {
expect(result3.files.length).toBe(2);
});

it('deletes files', async () => {
it('deletes a single file', async () => {
const file = await fileService.create({ fileKind, name: 'test' });
const result = await fileService.find({ kind: [fileKind] });
expect(result.files.length).toBe(1);
await file.delete();
expect(await fileService.find({ kind: [fileKind] })).toEqual({ files: [], total: 0 });
});

it('deletes a single file using the bulk method', async () => {
const file = await fileService.create({ fileKind, name: 'test' });
const result = await fileService.find({ kind: [fileKind] });
expect(result.files.length).toBe(1);
await fileService.bulkDelete({ ids: [file.id] });
expect(await fileService.find({ kind: [fileKind] })).toEqual({ files: [], total: 0 });
});

it('deletes multiple files using the bulk method', async () => {
const promises = Array.from({ length: 15 }, (v, i) =>
fileService.create({ fileKind, name: 'test ' + i })
);
const files = await Promise.all(promises);
const result = await fileService.find({ kind: [fileKind] });
expect(result.files.length).toBe(15);
await fileService.bulkDelete({ ids: files.map((file) => file.id) });
expect(await fileService.find({ kind: [fileKind] })).toEqual({ files: [], total: 0 });
});

interface CustomMeta {
some: string;
}
Expand Down
1 change: 1 addition & 0 deletions src/plugins/files/server/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { FileClient, FileServiceFactory, FileServiceStart, FilesSetup } from '.'
export const createFileServiceMock = (): DeeplyMockedKeys<FileServiceStart> => ({
create: jest.fn(),
delete: jest.fn(),
bulkDelete: jest.fn(),
deleteShareObject: jest.fn(),
find: jest.fn(),
getById: jest.fn(),
Expand Down