[Files] Refactor list and find and improve support for pagination (#143283)

* make list endpoint paginate properly

* consolidate find and list endpoints to use the same service method. also remove the list method because it does the same thing

* send body with post in tests
This commit is contained in:
Jean-Louis Leysens 2022-10-14 12:52:54 +02:00 committed by GitHub
parent 3cf9778127
commit af7c336824
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
21 changed files with 160 additions and 182 deletions

View file

@ -102,9 +102,11 @@ export function createFilesClient({
getById: ({ kind, ...args }) => {
return http.get(apiRoutes.getByIdRoute(scopedFileKind ?? kind, args.id));
},
list({ kind, page, perPage } = { kind: '' }) {
return http.get(apiRoutes.getListRoute(scopedFileKind ?? kind), {
list: ({ kind, page, perPage, ...body } = { kind: '' }) => {
return http.post(apiRoutes.getListRoute(scopedFileKind ?? kind), {
headers: commonBodyHeaders,
query: { page, perPage },
body: JSON.stringify(body),
});
},
update: ({ kind, id, ...body }) => {

View file

@ -154,12 +154,16 @@ export class FileClientImpl implements FileClient {
await this.internalUpdate(id, payload);
}
public async find<M = unknown>(arg: P1<FileMetadataClient['find']>): Promise<Array<File<M>>> {
return this.metadataClient
.find(arg)
.then((r) =>
r.map(({ id, metadata }) => this.instantiateFile(id, metadata as FileMetadata<M>))
);
public async find<M = unknown>(
arg: P1<FileMetadataClient['find']>
): Promise<{ files: File[]; total: number }> {
const result = await this.metadataClient.find(arg);
return {
total: result.total,
files: result.files.map(({ id, metadata }) =>
this.instantiateFile(id, metadata as FileMetadata<M>)
),
};
}
public async delete({ id, hasContent = true }: DeleteArgs) {
@ -192,12 +196,6 @@ export class FileClientImpl implements FileClient {
return this.blobStorageClient.delete(arg);
};
public async list(arg?: P1<FileMetadataClient['list']>): Promise<File[]> {
return this.metadataClient
.list(arg)
.then((r) => r.map(({ id, metadata }) => this.instantiateFile(id, metadata)));
}
/**
* Upload a blob
* @param id - The ID of the file content is associated with

View file

@ -10,7 +10,7 @@ import { pipe } from 'lodash/fp';
import { Logger } from '@kbn/core/server';
import { toElasticsearchQuery } from '@kbn/es-query';
import { ElasticsearchClient } from '@kbn/core-elasticsearch-server';
import { MappingProperty } from '@elastic/elasticsearch/lib/api/types';
import { MappingProperty, SearchTotalHits } from '@elastic/elasticsearch/lib/api/types';
import type { FilesMetrics, FileMetadata, Pagination } from '../../../../common';
import type { FindFileArgs } from '../../../file_service';
import type {
@ -19,10 +19,9 @@ import type {
FileMetadataClient,
GetArg,
GetUsageMetricsArgs,
ListArg,
UpdateArgs,
} from '../file_metadata_client';
import { filterArgsToKuery, filterDeletedFiles } from './query_filters';
import { filterArgsToKuery } from './query_filters';
import { fileObjectType } from '../../../saved_objects/file';
const filterArgsToESQuery = pipe(filterArgsToKuery, toElasticsearchQuery);
@ -114,25 +113,12 @@ export class EsIndexFilesMetadataClient<M = unknown> implements FileMetadataClie
private attrPrefix: keyof FileDocument = 'file';
async list({ page, perPage }: ListArg = {}): Promise<Array<FileDescriptor<M>>> {
const result = await this.esClient.search<FileDocument<M>>({
index: this.index,
expand_wildcards: 'hidden',
query: toElasticsearchQuery(filterDeletedFiles({ attrPrefix: this.attrPrefix })),
...this.paginationToES({ page, perPage }),
sort: 'file.created',
});
return result.hits.hits.map((hit) => {
return {
id: hit._id,
metadata: hit._source?.file!,
};
});
}
async find({ page, perPage, ...filterArgs }: FindFileArgs): Promise<Array<FileDescriptor<M>>> {
async find({ page, perPage, ...filterArgs }: FindFileArgs = {}): Promise<{
total: number;
files: Array<FileDescriptor<unknown>>;
}> {
const result = await this.esClient.search<FileDocument<M>>({
track_total_hits: true,
index: this.index,
expand_wildcards: 'hidden',
query: filterArgsToESQuery({ ...filterArgs, attrPrefix: this.attrPrefix }),
@ -140,7 +126,10 @@ export class EsIndexFilesMetadataClient<M = unknown> implements FileMetadataClie
sort: 'file.created',
});
return result.hits.hits.map((r) => ({ id: r._id, metadata: r._source?.file! }));
return {
total: (result.hits.total as SearchTotalHits).value,
files: result.hits.hits.map((r) => ({ id: r._id, metadata: r._source?.file! })),
};
}
async getUsageMetrics(arg: GetUsageMetricsArgs): Promise<FilesMetrics> {

View file

@ -12,12 +12,10 @@ import { getFlattenedObject } from '@kbn/std';
import { FileMetadata, FileStatus } from '../../../../common/types';
import { FindFileArgs } from '../../../file_service';
const { buildNode } = nodeTypes.function;
const deletedStatus: FileStatus = 'DELETED';
export function filterDeletedFiles({ attrPrefix }: { attrPrefix: string }): KueryNode {
return buildNode('not', nodeBuilder.is(`${attrPrefix}.Status`, deletedStatus));
return nodeTypes.function.buildNode('not', nodeBuilder.is(`${attrPrefix}.Status`, deletedStatus));
}
export function filterArgsToKuery({
@ -30,16 +28,25 @@ export function filterArgsToKuery({
}: Omit<FindFileArgs, 'page' | 'perPage'> & { attrPrefix?: string }): KueryNode {
const kueryExpressions: KueryNode[] = [filterDeletedFiles({ attrPrefix })];
const addFilters = (fieldName: keyof FileMetadata, values: string[] = []): void => {
const addFilters = (
fieldName: keyof FileMetadata,
values: string[] = [],
isWildcard = false
): void => {
if (values.length) {
const orExpressions = values
.filter(Boolean)
.map((value) => nodeBuilder.is(`${attrPrefix}.${fieldName}`, escapeKuery(value)));
.map((value) =>
nodeBuilder.is(
`${attrPrefix}.${fieldName}`,
isWildcard ? nodeTypes.wildcard.buildNode(value) : escapeKuery(value)
)
);
kueryExpressions.push(nodeBuilder.or(orExpressions));
}
};
addFilters('name', name);
addFilters('name', name, true);
addFilters('FileKind', kind);
addFilters('Status', status);
addFilters('extension', extension);
@ -51,7 +58,8 @@ export function filterArgsToKuery({
forEach(([fieldName, value]) => {
addFilters(
`Meta.${fieldName}` as keyof FileMetadata,
Array.isArray(value) ? value : [value]
Array.isArray(value) ? value : [value],
true
);
})
);

View file

@ -13,11 +13,10 @@ import type {
SavedObjectsOpenPointInTimeResponse,
} from '@kbn/core-saved-objects-api-server';
import { AggregationsSumAggregate } from '@elastic/elasticsearch/lib/api/types';
import { escapeKuery } from '@kbn/es-query';
import { FindFileArgs } from '../../../file_service/file_action_types';
import { ES_FIXED_SIZE_INDEX_BLOB_STORE } from '../../../../common/constants';
import type { FileMetadata, FilesMetrics, FileStatus, Pagination } from '../../../../common/types';
import type { FileMetadata, FilesMetrics, FileStatus } from '../../../../common/types';
import type {
FileMetadataClient,
UpdateArgs,
@ -61,26 +60,11 @@ export class SavedObjectsFileMetadataClient implements FileMetadataClient {
metadata: result.attributes as FileDescriptor['metadata'],
};
}
async list({ fileKind, page, perPage }: { fileKind?: string } & Pagination = {}): Promise<
FileDescriptor[]
> {
let filter = `NOT ${this.soType}.attributes.Status: DELETED`;
if (fileKind) {
filter = `${this.soType}.attributes.FileKind: ${escapeKuery(fileKind)} AND ${filter}`;
}
const result = await this.soClient.find({
type: this.soType,
filter,
page,
perPage,
});
return result.saved_objects.map((file) => ({
id: file.id,
metadata: file.attributes as FileDescriptor['metadata'],
}));
}
async find({ page, perPage, ...filterArgs }: FindFileArgs): Promise<FileDescriptor[]> {
async find({ page, perPage, ...filterArgs }: FindFileArgs = {}): Promise<{
total: number;
files: Array<FileDescriptor<unknown>>;
}> {
const result = await this.soClient.find({
type: this.soType,
filter: filterArgsToKuery({ ...filterArgs, attrPrefix: `${this.soType}.attributes` }),
@ -89,10 +73,13 @@ export class SavedObjectsFileMetadataClient implements FileMetadataClient {
sortOrder: 'desc',
sortField: 'created',
});
return result.saved_objects.map((so) => ({
id: so.id,
metadata: so.attributes as FileMetadata,
}));
return {
files: result.saved_objects.map((so) => ({
id: so.id,
metadata: so.attributes as FileMetadata,
})),
total: result.total,
};
}
async delete({ id }: { id: string }): Promise<void> {

View file

@ -72,13 +72,6 @@ export interface DeleteArg {
id: string;
}
export interface ListArg extends Pagination {
/**
* The file kind to scope this query to
*/
fileKind?: string;
}
export interface FindArg extends Pagination {
/**
* The file kind to scope this query to
@ -116,18 +109,12 @@ export interface FileMetadataClient {
* @param arg - Arguments to delete file metadata
*/
delete(arg: DeleteArg): Promise<void>;
/**
* List all instances of metadata for a file kind.
*
* @param arg - Arguments to list file metadata
*/
list(arg?: ListArg): Promise<FileDescriptor[]>;
/**
* Search for a set of file kind instances that match the filters.
*
* @param arg - Filters and other settings to match against
*/
find(arg: FindFileArgs): Promise<FileDescriptor[]>;
find(arg?: FindFileArgs): Promise<{ total: number; files: FileDescriptor[] }>;
/**
* Prepare a set of metrics based on the file metadata.
*

View file

@ -12,7 +12,6 @@ export type {
FindArg as FindMetadataArg,
GetArg as GetMetadataArg,
GetUsageMetricsArgs,
ListArg as ListMetadataArg,
UpdateArgs as UpdateMetadataArg,
} from './file_metadata_client';
export { SavedObjectsFileMetadataClient, EsIndexFilesMetadataClient } from './adapters';

View file

@ -13,7 +13,6 @@ export type {
FindMetadataArg,
GetMetadataArg,
GetUsageMetricsArgs,
ListMetadataArg,
UpdateMetadataArg,
} from './file_metadata_client';
export { FileClientImpl } from './file_client';

View file

@ -106,7 +106,7 @@ describe('ES-index-backed file client', () => {
await file3.uploadContent(Readable.from(['test']));
{
const results = await fileClient.find({
const { files: results } = await fileClient.find({
status: ['READY'],
meta: { test: '3' },
});
@ -121,7 +121,7 @@ describe('ES-index-backed file client', () => {
}
{
const results = await fileClient.find({
const { files: results } = await fileClient.find({
status: ['READY', 'AWAITING_UPLOAD'],
});
@ -178,10 +178,10 @@ describe('ES-index-backed file client', () => {
},
});
const list = await fileClient.list();
const { files } = await fileClient.find();
expect(list).toHaveLength(1);
expect(list[0].toJSON()).toEqual(
expect(files).toHaveLength(1);
expect(files[0].toJSON()).toEqual(
expect.objectContaining({
id: '123',
fileKind: 'none',

View file

@ -93,19 +93,12 @@ export interface FileClient {
*/
delete(arg: DeleteArgs): Promise<void>;
/**
* See {@link FileMetadataClient.list}
*
* @param arg - Argument to list files
*/
list(arg?: P1<FileMetadataClient['list']>): Promise<File[]>;
/**
* See {@link FileMetadataClient.find}.
*
* @param arg - Argument to find files
*/
find: (arg: P1<FileMetadataClient['find']>) => Promise<File[]>;
find: (arg?: P1<FileMetadataClient['find']>) => Promise<{ files: File[]; total: number }>;
/**
* Create a file share instance for this file.

View file

@ -65,16 +65,6 @@ export interface DeleteFileArgs {
fileKind: string;
}
/**
* Arguments list files.
*/
export interface ListFilesArgs extends Pagination {
/**
* File kind, must correspond to a registered {@link FileKind}.
*/
fileKind: string;
}
/**
* Arguments to get a file by ID.
*/

View file

@ -5,14 +5,13 @@
* 2.0.
*/
import type { FileJSON, FilesMetrics, File } from '../../common';
import type { FilesMetrics, File, FileJSON } from '../../common';
import type { FileShareServiceStart } from '../file_share_service/types';
import type {
CreateFileArgs,
UpdateFileArgs,
DeleteFileArgs,
GetByIdArgs,
ListFilesArgs,
FindFileArgs,
} from './file_action_types';
@ -55,14 +54,7 @@ export interface FileServiceStart {
*
* @param args - find files args
*/
find<M>(args: FindFileArgs): Promise<Array<FileJSON<M>>>;
/**
* List all files of specific file kind.
*
* @param args - list files args
*/
list<M>(args: ListFilesArgs): Promise<Array<File<M>>>;
find<M>(args: FindFileArgs): Promise<{ files: Array<FileJSON<M>>; total: number }>;
/**
* Get an instance of a share object

View file

@ -19,13 +19,7 @@ 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,
ListFilesArgs,
UpdateFileArgs,
} from './file_action_types';
import { CreateFileArgs, FindFileArgs, GetByIdArgs, UpdateFileArgs } from './file_action_types';
import { InternalFileService } from './internal_file_service';
import { FileServiceStart } from './file_service';
import { FileKindsRegistry } from '../../common/file_kinds_registry';
@ -105,10 +99,10 @@ export class FileServiceFactoryImpl implements FileServiceFactory {
return internalFileService.getById(args) as Promise<File<M>>;
},
async find<M>(args: FindFileArgs) {
return internalFileService.findFilesJSON(args) as Promise<Array<FileJSON<M>>>;
},
async list<M>(args: ListFilesArgs) {
return internalFileService.list(args) as Promise<Array<File<M>>>;
return internalFileService.findFilesJSON(args) as Promise<{
files: Array<FileJSON<M>>;
total: number;
}>;
},
async getUsageMetrics() {
return internalFileService.getUsageMetrics();

View file

@ -11,7 +11,6 @@ export type {
DeleteFileArgs,
FindFileArgs,
GetByIdArgs,
ListFilesArgs,
UpdateFileArgs,
} from './file_action_types';
export type { FileServiceStart } from './file_service';

View file

@ -21,7 +21,6 @@ import type {
DeleteFileArgs,
FindFileArgs,
GetByIdArgs,
ListFilesArgs,
} from './file_action_types';
import { createFileClient, FileClientImpl } from '../file_client/file_client';
/**
@ -88,28 +87,16 @@ export class InternalFileService {
return file;
}
public async list({
fileKind: fileKindId,
page = 1,
perPage = 100,
}: ListFilesArgs): Promise<IFile[]> {
const fileKind = this.getFileKind(fileKindId);
const result = await this.metadataClient.list({
fileKind: fileKind.id,
page,
perPage,
});
const fileClient = this.createFileClient(fileKind.id);
return result.map((file) => this.toFile(file.id, file.metadata, fileKind.id, fileClient));
}
public getFileKind(id: string): FileKind {
return this.fileKindRegistry.get(id);
}
public async findFilesJSON(args: FindFileArgs): Promise<FileJSON[]> {
const result = await this.metadataClient.find(args);
return result.map((r) => toJSON(r.id, r.metadata));
public async findFilesJSON(args: FindFileArgs): Promise<{ files: FileJSON[]; total: number }> {
const { total, files } = await this.metadataClient.find(args);
return {
total,
files: files.map(({ id, metadata }) => toJSON(id, metadata)),
};
}
public async getUsageMetrics(): Promise<FilesMetrics> {

View file

@ -13,7 +13,6 @@ export type {
FileDescriptor,
GetMetadataArg,
FindMetadataArg,
ListMetadataArg,
UpdateMetadataArg,
DeleteMetedataArg,
FileMetadataClient,
@ -35,7 +34,6 @@ export type {
export type {
GetByIdArgs,
FindFileArgs,
ListFilesArgs,
CreateFileArgs,
DeleteFileArgs,
UpdateFileArgs,

View file

@ -99,8 +99,8 @@ describe('FileService', () => {
}
afterEach(async () => {
await Promise.all(disposables.map((file) => file.delete()));
const results = await fileService.list({ fileKind });
expect(results.length).toBe(0);
const { files } = await fileService.find({ kind: [fileKind] });
expect(files.length).toBe(0);
disposables = [];
});
@ -146,16 +146,46 @@ describe('FileService', () => {
createDisposableFile({ fileKind, name: 'test-3' }),
createDisposableFile({ fileKind, name: 'test-3' /* Also test file with same name */ }),
]);
const result = await fileService.list({ fileKind });
expect(result.length).toBe(4);
const result = await fileService.find({ kind: [fileKind] });
expect(result.files.length).toBe(4);
});
it('lists files and filters', async () => {
await Promise.all([
createDisposableFile({ fileKind, name: 'foo-1' }),
createDisposableFile({ fileKind, name: 'foo-2' }),
createDisposableFile({ fileKind, name: 'foo-3' }),
createDisposableFile({ fileKind, name: 'test-3' }),
]);
{
const { files, total } = await fileService.find({
kind: [fileKind],
name: ['foo*'],
perPage: 2,
page: 1,
});
expect(files.length).toBe(2);
expect(total).toBe(3);
}
{
const { files, total } = await fileService.find({
kind: [fileKind],
name: ['foo*'],
perPage: 2,
page: 2,
});
expect(files.length).toBe(1);
expect(total).toBe(3);
}
});
it('deletes files', async () => {
const file = await fileService.create({ fileKind, name: 'test' });
const files = await fileService.list({ fileKind });
expect(files.length).toBe(1);
const result = await fileService.find({ kind: [fileKind] });
expect(result.files.length).toBe(1);
await file.delete();
expect(await fileService.list({ fileKind })).toEqual([]);
expect(await fileService.find({ kind: [fileKind] })).toEqual({ files: [], total: 0 });
});
interface CustomMeta {

View file

@ -17,7 +17,6 @@ export const createFileServiceMock = (): DeeplyMockedKeys<FileServiceStart> => (
getByToken: jest.fn(),
getShareObject: jest.fn(),
getUsageMetrics: jest.fn(),
list: jest.fn(),
listShareObjects: jest.fn(),
update: jest.fn(),
updateShareObject: jest.fn(),

View file

@ -110,14 +110,17 @@ describe('File kind HTTP API', () => {
const {
body: { files },
} = await request.get(root, `/api/files/files/${fileKind}/list`).expect(200);
} = await request.post(root, `/api/files/files/${fileKind}/list`).send({}).expect(200);
expect(files).toHaveLength(nrOfFiles);
expect(files[0]).toEqual(expect.objectContaining({ name: 'test' }));
const {
body: { files: files2 },
} = await request.get(root, `/api/files/files/${fileKind}/list?page=1&perPage=5`).expect(200);
} = await request
.post(root, `/api/files/files/${fileKind}/list?page=1&perPage=5`)
.send({})
.expect(200);
expect(files2).toHaveLength(5);
});

View file

@ -8,27 +8,47 @@ import { schema } from '@kbn/config-schema';
import type { FileJSON, FileKind } from '../../../common/types';
import { CreateRouteDefinition, FILES_API_ROUTES } from '../api_routes';
import type { CreateHandler, FileKindRouter } from './types';
import {
stringOrArrayOfStrings,
nameStringOrArrayOfNameStrings,
toArrayOrUndefined,
} from '../find';
export const method = 'get' as const;
export const method = 'post' as const;
const rt = {
body: schema.object({
status: schema.maybe(stringOrArrayOfStrings),
extension: schema.maybe(stringOrArrayOfStrings),
name: schema.maybe(nameStringOrArrayOfNameStrings),
meta: schema.maybe(schema.object({}, { unknowns: 'allow' })),
}),
query: schema.object({
page: schema.maybe(schema.number({ defaultValue: 1 })),
page: schema.maybe(schema.number()),
perPage: schema.maybe(schema.number({ defaultValue: 100 })),
}),
};
export type Endpoint<M = unknown> = CreateRouteDefinition<typeof rt, { files: Array<FileJSON<M>> }>;
export type Endpoint<M = unknown> = CreateRouteDefinition<
typeof rt,
{ files: Array<FileJSON<M>>; total: number }
>;
export const handler: CreateHandler<Endpoint> = async ({ files, fileKind }, req, res) => {
const {
body: { name, status, extension, meta },
query: { page, perPage },
} = req;
const { fileService } = await files;
const response = await fileService.asCurrentUser().list({ fileKind, page, perPage });
const body: Endpoint['output'] = {
files: response.map((result) => result.toJSON()),
};
const body: Endpoint['output'] = await fileService.asCurrentUser().find({
kind: [fileKind],
name: toArrayOrUndefined(name),
status: toArrayOrUndefined(status),
extension: toArrayOrUndefined(extension),
page,
perPage,
meta,
});
return res.ok({ body });
};

View file

@ -14,8 +14,13 @@ const method = 'post' as const;
const string64 = schema.string({ maxLength: 64 });
const string256 = schema.string({ maxLength: 256 });
const stringOrArrayOfStrings = schema.oneOf([string64, schema.arrayOf(string64)]);
const nameStringOrArrayOfNameStrings = schema.oneOf([string256, schema.arrayOf(string256)]);
export const stringOrArrayOfStrings = schema.oneOf([string64, schema.arrayOf(string64)]);
export const nameStringOrArrayOfNameStrings = schema.oneOf([string256, schema.arrayOf(string256)]);
export function toArrayOrUndefined(val?: string | string[]): undefined | string[] {
if (val == null) return undefined;
return Array.isArray(val) ? val : [val];
}
const rt = {
body: schema.object({
@ -31,11 +36,7 @@ const rt = {
}),
};
export type Endpoint = CreateRouteDefinition<typeof rt, { files: FileJSON[] }>;
function toArray(val: string | string[]) {
return Array.isArray(val) ? val : [val];
}
export type Endpoint = CreateRouteDefinition<typeof rt, { files: FileJSON[]; total: number }>;
const handler: CreateHandler<Endpoint> = async ({ files }, req, res) => {
const { fileService } = await files;
@ -44,15 +45,18 @@ const handler: CreateHandler<Endpoint> = async ({ files }, req, res) => {
query,
} = req;
const { files: results, total } = await fileService.asCurrentUser().find({
kind: toArrayOrUndefined(kind),
name: toArrayOrUndefined(name),
status: toArrayOrUndefined(status),
extension: toArrayOrUndefined(extension),
meta,
...query,
});
const body: Endpoint['output'] = {
files: await fileService.asCurrentUser().find({
kind: kind ? toArray(kind) : undefined,
name: name ? toArray(name) : undefined,
status: status ? toArray(status) : undefined,
extension: extension ? toArray(extension) : undefined,
meta,
...query,
}),
total,
files: results,
};
return res.ok({
body,