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

[Files] Adds bulk get method #155636

Merged
merged 12 commits into from
Apr 27, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,24 @@ 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 {
DeleteArg,
FileDescriptor,
FileMetadataClient,
GetArg,
BulkGetArg,
GetUsageMetricsArgs,
UpdateArgs,
} from '../file_metadata_client';
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,
Expand Down Expand Up @@ -120,6 +124,12 @@ export class EsIndexFilesMetadataClient<M = unknown> implements FileMetadataClie
};
}

async bulkGet({ ids }: BulkGetArg): Promise<FileDescriptor[]> {
const promises = ids.map((id) => bulkGetConcurrency(() => this.get({ id })));
const result = await Promise.all(promises);
return result;
}

async delete({ id }: DeleteArg): Promise<void> {
await this.esClient.delete({ index: this.index, id });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import type { FileMetadata, FilesMetrics, FileStatus } from '../../../../common/
import type {
FileMetadataClient,
UpdateArgs,
GetArg,
BulkGetArg,
FileDescriptor,
GetUsageMetricsArgs,
} from '../file_metadata_client';
Expand Down Expand Up @@ -54,14 +56,23 @@ export class SavedObjectsFileMetadataClient implements FileMetadataClient {
metadata: result.attributes as FileDescriptor['metadata'],
};
}
async get({ id }: { id: string }): Promise<FileDescriptor> {

async get({ id }: GetArg): Promise<FileDescriptor> {
const result = await this.soClient.get(this.soType, id);
return {
id: result.id,
metadata: result.attributes as FileDescriptor['metadata'],
};
}

async bulkGet({ ids }: BulkGetArg): Promise<FileDescriptor[]> {
const result = await this.soClient.bulkGet(ids.map((id) => ({ id, type: this.soType })));
return result.saved_objects.map((so) => ({
id: so.id,
metadata: so.attributes as FileDescriptor['metadata'],
}));
}

async find({ page, perPage, ...filterArgs }: FindFileArgs = {}): Promise<{
total: number;
files: Array<FileDescriptor<unknown>>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ export interface GetArg {
id: string;
}

/**
* Bulk get files
*/
export interface BulkGetArg {
/**
* Unique IDs of file metadata
*/
ids: string[];
}

export interface DeleteArg {
/**
* Unique ID of file metadata to delete
Expand Down Expand Up @@ -98,6 +108,13 @@ export interface FileMetadataClient {
*/
get(arg: GetArg): Promise<FileDescriptor>;

/**
* Bulk get file metadata
*
* @param arg - Arguments to bulk retrieve file metadata
*/
bulkGet(arg: BulkGetArg): Promise<FileDescriptor[]>;

/**
* The file metadata to update
*
Expand Down
10 changes: 10 additions & 0 deletions src/plugins/files/server/file_service/file_action_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ export interface GetByIdArgs {
id: string;
}

/**
* Arguments to bulk get multiple files by their IDs.
*/
export interface BulkGetByIdArgs {
/**
* File IDs.
*/
ids: string[];
}

/**
* Arguments to filter for files.
*
Expand Down
8 changes: 8 additions & 0 deletions src/plugins/files/server/file_service/file_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
UpdateFileArgs,
DeleteFileArgs,
GetByIdArgs,
BulkGetByIdArgs,
FindFileArgs,
} from './file_action_types';

Expand Down Expand Up @@ -50,6 +51,13 @@ export interface FileServiceStart {
*/
getById<M>(args: GetByIdArgs): Promise<File<M>>;

/**
* Bulk get files by IDs. Will throw if any of the files fail to load.
*
* @param args - bulk get files by IDs args
*/
bulkGetById<M>(args: BulkGetByIdArgs): Promise<Array<File<M>>>;

/**
* Find files given a set of parameters.
*
Expand Down
11 changes: 10 additions & 1 deletion src/plugins/files/server/file_service/file_service_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -99,6 +105,9 @@ export class FileServiceFactoryImpl implements FileServiceFactory {
async getById<M>(args: GetByIdArgs) {
return internalFileService.getById(args) as Promise<File<M>>;
},
async bulkGetById<M>(args: BulkGetByIdArgs) {
return internalFileService.bulkGetById(args) as Promise<Array<File<M>>>;
},
async find<M>(args: FindFileArgs) {
return internalFileService.findFilesJSON(args) as Promise<{
files: Array<FileJSON<M>>;
Expand Down
24 changes: 24 additions & 0 deletions src/plugins/files/server/file_service/internal_file_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {
DeleteFileArgs,
FindFileArgs,
GetByIdArgs,
BulkGetByIdArgs,
} from './file_action_types';
import { createFileClient, FileClientImpl } from '../file_client/file_client';
/**
Expand Down Expand Up @@ -80,10 +81,33 @@ export class InternalFileService {
}
}

private async bulkGet(ids: string[]): Promise<IFile[]> {
try {
const metadatas = await this.metadataClient.bulkGet({ ids });
const result = metadatas.map(({ id, metadata }) => {
if (metadata.Status === 'DELETED') {
throw new FileNotFoundError('File has been deleted');
Copy link
Contributor

@jonathan-buttner jonathan-buttner Apr 25, 2023

Choose a reason for hiding this comment

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

I don't think this will provide the functionality we need in cases. Cases is intentionally doing a promise settled to allow for some files to not exist but still not have the entire api call throw an error.

https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/server/client/attachments/bulk_delete.ts#L137

This way we can continue with our deletion logic even when some files don't exist. Could we still return normally here but provide a way to see which ids failed and which succeeded?

Copy link
Contributor

@sebelga sebelga Apr 26, 2023

Choose a reason for hiding this comment

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

Good points 👍
In c1030a0 I've added 2 options:

  • throwIfNotFound - If set to false then null is returned
  • format ("array" | "map") - If set to map then a map of id/File (or null if throwIfNotFound is false) is returned
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, // do not throw
    format: 'map', // returns a map
  });

  expect(myFiles[file1?.id]?.id).toBe(file1?.id); // true
  expect(myFiles[unknownID]).toBe(null); // true
});

would that work?

}
return this.toFile(id, metadata, metadata.FileKind);
});
return result;
} catch (e) {
if (SavedObjectsErrorHelpers.isNotFoundError(e)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the saved object client throw an error if one of the files does not exist? I think it will just mark that item in the array with the error field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right, that's how it is returned from the saved object bulkGet

if (docNotFound) {
  return {
    id,
    type,
    error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)),
  } as any as SavedObject<T>;
}

In b97a0f1 I removed this condition and made sure we return null in case of an error when fetching one of the saved object.

throw new FileNotFoundError('Files not found');
}
this.logger.error(`Could not retrieve files: ${e}`);
throw e;
}
}

public async getById({ id }: GetByIdArgs): Promise<IFile> {
return await this.get(id);
}

public async bulkGetById({ ids }: BulkGetByIdArgs): Promise<IFile[]> {
return await this.bulkGet(ids);
}

public getFileKind(id: string): FileKind {
return this.fileKindRegistry.get(id);
}
Expand Down
14 changes: 14 additions & 0 deletions src/plugins/files/server/integration_tests/file_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,20 @@ 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('lists files', async () => {
await Promise.all([
createDisposableFile({ fileKind, name: 'test-1' }),
Expand Down
1 change: 1 addition & 0 deletions src/plugins/files/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const createFileServiceMock = (): DeeplyMockedKeys<FileServiceStart> => (
deleteShareObject: jest.fn(),
find: jest.fn(),
getById: jest.fn(),
bulkGetById: jest.fn(),
getByToken: jest.fn(),
getShareObject: jest.fn(),
getUsageMetrics: jest.fn(),
Expand Down