From a0cd72436cd1586423c19236b1aa0d05babec010 Mon Sep 17 00:00:00 2001 From: Vadim Kibana <82822460+vadimkibana@users.noreply.github.com> Date: Thu, 27 Apr 2023 15:24:57 +0200 Subject: [PATCH] [Files] Adds bulk get method (#155636) ## Summary Closes https://github.com/elastic/kibana/issues/155369 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .../file_metadata_client/adapters/es_index.ts | 20 ++++++ .../adapters/saved_objects.ts | 23 +++++- .../file_metadata_client.ts | 25 +++++++ .../server/file_service/file_action_types.ts | 20 ++++++ .../files/server/file_service/file_service.ts | 18 +++++ .../file_service/file_service_factory.ts | 29 +++++++- .../file_service/internal_file_service.ts | 72 +++++++++++++++++-- .../integration_tests/file_service.test.ts | 50 +++++++++++++ src/plugins/files/server/mocks.ts | 1 + 9 files changed, 251 insertions(+), 7 deletions(-) diff --git a/src/plugins/files/server/file_client/file_metadata_client/adapters/es_index.ts b/src/plugins/files/server/file_client/file_metadata_client/adapters/es_index.ts index d139c7cf6330f..f5d1aa336e78e 100644 --- a/src/plugins/files/server/file_client/file_metadata_client/adapters/es_index.ts +++ b/src/plugins/files/server/file_client/file_metadata_client/adapters/es_index.ts @@ -12,6 +12,8 @@ import { Logger } from '@kbn/core/server'; import { toElasticsearchQuery } from '@kbn/es-query'; import { ElasticsearchClient } from '@kbn/core-elasticsearch-server'; import { MappingProperty, SearchTotalHits } from '@elastic/elasticsearch/lib/api/types'; +import pLimit from 'p-limit'; + import type { FilesMetrics, FileMetadata, Pagination } from '../../../../common'; import type { FindFileArgs } from '../../../file_service'; import type { @@ -19,6 +21,7 @@ import type { FileDescriptor, FileMetadataClient, GetArg, + BulkGetArg, GetUsageMetricsArgs, UpdateArgs, } from '../file_metadata_client'; @@ -26,6 +29,7 @@ import { filterArgsToKuery } from './query_filters'; import { fileObjectType } from '../../../saved_objects/file'; const filterArgsToESQuery = pipe(filterArgsToKuery, toElasticsearchQuery); +const bulkGetConcurrency = pLimit(10); const fileMappings: MappingProperty = { dynamic: false, @@ -120,6 +124,22 @@ export class EsIndexFilesMetadataClient implements FileMetadataClie }; } + async bulkGet(arg: { ids: string[]; throwIfNotFound?: true }): Promise; + async bulkGet({ ids, throwIfNotFound }: BulkGetArg): Promise> { + const promises = ids.map((id) => + bulkGetConcurrency(() => + this.get({ id }).catch((e) => { + if (throwIfNotFound) { + throw e; + } + return null; + }) + ) + ); + const result = await Promise.all(promises); + return result; + } + async delete({ id }: DeleteArg): Promise { await this.esClient.delete({ index: this.index, id }); } diff --git a/src/plugins/files/server/file_client/file_metadata_client/adapters/saved_objects.ts b/src/plugins/files/server/file_client/file_metadata_client/adapters/saved_objects.ts index f0f547bb4704f..84274d695d292 100644 --- a/src/plugins/files/server/file_client/file_metadata_client/adapters/saved_objects.ts +++ b/src/plugins/files/server/file_client/file_metadata_client/adapters/saved_objects.ts @@ -21,6 +21,8 @@ import type { FileMetadata, FilesMetrics, FileStatus } from '../../../../common/ import type { FileMetadataClient, UpdateArgs, + GetArg, + BulkGetArg, FileDescriptor, GetUsageMetricsArgs, } from '../file_metadata_client'; @@ -54,7 +56,8 @@ export class SavedObjectsFileMetadataClient implements FileMetadataClient { metadata: result.attributes as FileDescriptor['metadata'], }; } - async get({ id }: { id: string }): Promise { + + async get({ id }: GetArg): Promise { const result = await this.soClient.get(this.soType, id); return { id: result.id, @@ -62,6 +65,24 @@ export class SavedObjectsFileMetadataClient implements FileMetadataClient { }; } + async bulkGet(arg: { ids: string[]; throwIfNotFound?: true }): Promise; + async bulkGet({ ids, throwIfNotFound }: BulkGetArg): Promise> { + const result = await this.soClient.bulkGet(ids.map((id) => ({ id, type: this.soType }))); + return result.saved_objects.map((so) => { + if (so.error) { + if (throwIfNotFound) { + throw new Error(`File [${so.id}] not found`); + } + return null; + } + + return { + id: so.id, + metadata: so.attributes as FileDescriptor['metadata'], + }; + }); + } + async find({ page, perPage, ...filterArgs }: FindFileArgs = {}): Promise<{ total: number; files: Array>; diff --git a/src/plugins/files/server/file_client/file_metadata_client/file_metadata_client.ts b/src/plugins/files/server/file_client/file_metadata_client/file_metadata_client.ts index 2738d75d8222c..33117ad842afc 100644 --- a/src/plugins/files/server/file_client/file_metadata_client/file_metadata_client.ts +++ b/src/plugins/files/server/file_client/file_metadata_client/file_metadata_client.ts @@ -63,6 +63,21 @@ export interface GetArg { id: string; } +/** + * Bulk get files + */ +export interface BulkGetArg { + /** + * Unique IDs of file metadata + */ + ids: string[]; + /** + * Flag to indicate if an Error is thrown if any of the file id is not found. If set to `false`, "null" will be returned. + * @default true + */ + throwIfNotFound?: boolean; +} + export interface DeleteArg { /** * Unique ID of file metadata to delete @@ -98,6 +113,16 @@ export interface FileMetadataClient { */ get(arg: GetArg): Promise; + /** + * Bulk get file metadata + * + * @param arg - Arguments to bulk retrieve file metadata + */ + bulkGet(arg: { ids: string[]; throwIfNotFound?: true }): Promise; + bulkGet( + arg: BulkGetArg | { ids: string[]; throwIfNotFound: false } + ): Promise>; + /** * The file metadata to update * diff --git a/src/plugins/files/server/file_service/file_action_types.ts b/src/plugins/files/server/file_service/file_action_types.ts index f0aad5e86e8eb..e27d5f9af3fb0 100644 --- a/src/plugins/files/server/file_service/file_action_types.ts +++ b/src/plugins/files/server/file_service/file_action_types.ts @@ -82,6 +82,26 @@ export interface GetByIdArgs { id: string; } +/** + * Arguments to bulk get multiple files by their IDs. + */ +export interface BulkGetByIdArgs { + /** + * File IDs. + */ + ids: string[]; + /** + * Flag to indicate if an Error is thrown if any of the file id is not found. If set to `false`, "null" will be returned. + * @default true + */ + throwIfNotFound?: boolean; + /** + * Format of the response, either a list of File[] (sorted by id passed) or a map `{[fileId: string]: File}` + * @default "array" + */ + format?: 'array' | 'map'; +} + /** * Arguments to filter for files. * diff --git a/src/plugins/files/server/file_service/file_service.ts b/src/plugins/files/server/file_service/file_service.ts index 0828d03ada7fd..0fe24002f99d4 100644 --- a/src/plugins/files/server/file_service/file_service.ts +++ b/src/plugins/files/server/file_service/file_service.ts @@ -14,6 +14,7 @@ import type { DeleteFileArgs, BulkDeleteFilesArgs, GetByIdArgs, + BulkGetByIdArgs, FindFileArgs, } from './file_action_types'; @@ -58,6 +59,23 @@ export interface FileServiceStart { */ getById(args: GetByIdArgs): Promise>; + /** + * Bulk get files by IDs. Will throw if any of the files fail to load (set `throwIfNotFound` to `false` to not throw and return `null` instead) + * + * @param args - bulk get files by IDs args + */ + bulkGetById(args: Pick & { throwIfNotFound?: true }): Promise; + bulkGetById( + args: Pick & { throwIfNotFound?: true; format: 'map' } + ): Promise<{ [id: string]: File }>; + bulkGetById( + args: Pick & { throwIfNotFound: false } + ): Promise>; + bulkGetById( + args: Pick & { throwIfNotFound: false; format: 'map' } + ): Promise<{ [id: string]: File | null }>; + bulkGetById(args: BulkGetByIdArgs): Promise | { [id: string]: File | null }>>; + /** * Find files given a set of parameters. * diff --git a/src/plugins/files/server/file_service/file_service_factory.ts b/src/plugins/files/server/file_service/file_service_factory.ts index bebb5b72c2b3d..eb384f2e5cbe7 100644 --- a/src/plugins/files/server/file_service/file_service_factory.ts +++ b/src/plugins/files/server/file_service/file_service_factory.ts @@ -20,7 +20,13 @@ import { fileObjectType, fileShareObjectType, hiddenTypes } from '../saved_objec import { BlobStorageService } from '../blob_storage_service'; import { FileClientImpl } from '../file_client/file_client'; import { InternalFileShareService } from '../file_share_service'; -import { CreateFileArgs, FindFileArgs, GetByIdArgs, UpdateFileArgs } from './file_action_types'; +import { + CreateFileArgs, + FindFileArgs, + GetByIdArgs, + BulkGetByIdArgs, + UpdateFileArgs, +} from './file_action_types'; import { InternalFileService } from './internal_file_service'; import { FileServiceStart } from './file_service'; import { FileKindsRegistry } from '../../common/file_kinds_registry'; @@ -86,6 +92,26 @@ export class FileServiceFactoryImpl implements FileServiceFactory { this.logger ); + function bulkGetById( + args: Pick & { throwIfNotFound?: true } + ): Promise>>; + function bulkGetById( + args: Pick & { throwIfNotFound?: true; format: 'map' } + ): Promise<{ [id: string]: File }>; + function bulkGetById( + args: Pick & { throwIfNotFound: false } + ): Promise | null>>; + function bulkGetById( + args: Pick & { throwIfNotFound: false; format: 'map' } + ): Promise<{ [id: string]: File | null }>; + function bulkGetById( + args: BulkGetByIdArgs + ): Promise | null> | { [id: string]: File | null }> { + return internalFileService.bulkGetById( + args as Parameters[0] + ); + } + return { async create(args: CreateFileArgs) { return internalFileService.createFile(args) as Promise>; @@ -102,6 +128,7 @@ export class FileServiceFactoryImpl implements FileServiceFactory { async getById(args: GetByIdArgs) { return internalFileService.getById(args) as Promise>; }, + bulkGetById, async find(args: FindFileArgs) { return internalFileService.findFilesJSON(args) as Promise<{ files: Array>; diff --git a/src/plugins/files/server/file_service/internal_file_service.ts b/src/plugins/files/server/file_service/internal_file_service.ts index 653929f7b1236..b224a28313a70 100644 --- a/src/plugins/files/server/file_service/internal_file_service.ts +++ b/src/plugins/files/server/file_service/internal_file_service.ts @@ -24,6 +24,7 @@ import type { BulkDeleteFilesArgs, FindFileArgs, GetByIdArgs, + BulkGetByIdArgs, } from './file_action_types'; import { createFileClient, FileClientImpl } from '../file_client/file_client'; @@ -93,10 +94,71 @@ export class InternalFileService { } } + private async bulkGet( + ids: string[], + { + throwIfNotFound = true, + format = 'array', + }: { throwIfNotFound?: boolean; format?: BulkGetByIdArgs['format'] } = {} + ): Promise | null> | { [id: string]: IFile | null }> { + try { + const metadatas = await this.metadataClient.bulkGet({ ids, throwIfNotFound: false }); + const result = metadatas.map((fileMetadata, i) => { + const notFound = !fileMetadata || !fileMetadata.metadata; + const deleted = fileMetadata?.metadata?.Status === 'DELETED'; + + if (notFound || deleted) { + if (!throwIfNotFound) { + return null; + } + throw new FileNotFoundError( + deleted ? 'File has been deleted' : `File [${fileMetadata?.id ?? ids[i]}] not found` + ); + } + + const { id, metadata } = fileMetadata; + return this.toFile(id, metadata as FileMetadata, metadata.FileKind); + }); + + return format === 'array' + ? result + : ids.reduce<{ [id: string]: IFile | null }>( + (acc, id, i) => ({ + ...acc, + [id]: result[i], + }), + {} + ); + } catch (e) { + this.logger.error(`Could not retrieve files: ${e}`); + throw e; + } + } + public async getById({ id }: GetByIdArgs): Promise { return await this.get(id); } + public async bulkGetById( + args: Pick & { throwIfNotFound?: true } + ): Promise>>; + public async bulkGetById( + args: Pick & { throwIfNotFound?: true; format: 'map' } + ): Promise<{ [id: string]: IFile }>; + public async bulkGetById( + args: Pick & { throwIfNotFound: false } + ): Promise | null>>; + public async bulkGetById( + args: Pick & { throwIfNotFound: false; format: 'map' } + ): Promise<{ [id: string]: IFile | null }>; + public async bulkGetById({ + ids, + throwIfNotFound, + format, + }: BulkGetByIdArgs): Promise | null> | { [id: string]: IFile | null }> { + return await this.bulkGet(ids, { throwIfNotFound, format }); + } + public getFileKind(id: string): FileKind { return this.fileKindRegistry.get(id); } @@ -122,15 +184,15 @@ export class InternalFileService { return this.get(fileId); } - private toFile( + private toFile( id: string, - fileMetadata: FileMetadata, + fileMetadata: FileMetadata, fileKind: string, fileClient?: FileClientImpl - ): IFile { - return new File( + ): IFile { + return new File( id, - toJSON(id, fileMetadata), + toJSON(id, fileMetadata), fileClient ?? this.createFileClient(fileKind), this.logger.get(`file-${id}`) ); diff --git a/src/plugins/files/server/integration_tests/file_service.test.ts b/src/plugins/files/server/integration_tests/file_service.test.ts index 5ca1fa00bdd0a..4c5933d61c9e6 100644 --- a/src/plugins/files/server/integration_tests/file_service.test.ts +++ b/src/plugins/files/server/integration_tests/file_service.test.ts @@ -140,6 +140,56 @@ describe('FileService', () => { expect(myFile?.id).toMatch(id); }); + it('retrieves a file using the bulk method', async () => { + const { id } = await createDisposableFile({ fileKind, name: 'test' }); + const [myFile] = await fileService.bulkGetById({ ids: [id] }); + expect(myFile?.id).toMatch(id); + }); + + it('retrieves multiple files using the bulk method', async () => { + const file1 = await createDisposableFile({ fileKind, name: 'test' }); + const file2 = await createDisposableFile({ fileKind, name: 'test' }); + const [myFile1, myFile2] = await fileService.bulkGetById({ ids: [file1.id, file2.id] }); + expect(myFile1?.id).toMatch(file1.id); + expect(myFile2?.id).toMatch(file2.id); + }); + + it('throws if one of the file does not exists', async () => { + const file1 = await createDisposableFile({ fileKind, name: 'test' }); + const unknownID = 'foo'; + + expect(async () => { + await fileService.bulkGetById({ ids: [file1.id, unknownID] }); + }).rejects.toThrowError(`File [${unknownID}] not found`); + }); + + it('does not throw if one of the file does not exists', async () => { + const file1 = await createDisposableFile({ fileKind, name: 'test' }); + const unknownID = 'foo'; + + const [myFile1, myFile2] = await fileService.bulkGetById({ + ids: [file1.id, unknownID], + throwIfNotFound: false, + }); + + expect(myFile1?.id).toBe(file1?.id); + expect(myFile2).toBe(null); + }); + + it('returns the files under a map of id/File', async () => { + const file1 = await createDisposableFile({ fileKind, name: 'test' }); + const unknownID = 'foo'; + + const myFiles = await fileService.bulkGetById({ + ids: [file1.id, unknownID], + throwIfNotFound: false, + format: 'map', + }); + + expect(myFiles[file1?.id]?.id).toBe(file1?.id); + expect(myFiles[unknownID]).toBe(null); + }); + it('lists files', async () => { await Promise.all([ createDisposableFile({ fileKind, name: 'test-1' }), diff --git a/src/plugins/files/server/mocks.ts b/src/plugins/files/server/mocks.ts index da94d95b273c4..4da690b99240e 100644 --- a/src/plugins/files/server/mocks.ts +++ b/src/plugins/files/server/mocks.ts @@ -19,6 +19,7 @@ export const createFileServiceMock = (): DeeplyMockedKeys => ( deleteShareObject: jest.fn(), find: jest.fn(), getById: jest.fn(), + bulkGetById: jest.fn(), getByToken: jest.fn(), getShareObject: jest.fn(), getUsageMetrics: jest.fn(),