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

[SOM] Fix UI and export when handling more than 10k objects #118335

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ interface TableState {
activeAction?: SavedObjectsManagementAction;
}

const MAX_PAGINATED_ITEM = 10000;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a big fan of this hardcoded limit, OTOH that's how it's done in other parts of Kibana too, and it's way easier than fetching the setting from all the SO indices targeted by the request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can an end-user face the problem when requesting /api/kibana/management/saved_objects/scroll/counts? If so, maybe this logic belongs to the server-side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can't when requesting /api/kibana/management/saved_objects/scroll/counts, or when performing the export, only when manually calling /api/kibana/management/saved_objects/_find if the requested page/perPage exceeds the threshold (but even if prefixed by /api, there shouldn't be a case where an integration uses this endpoint)


export class Table extends PureComponent<TableProps, TableState> {
state: TableState = {
isSearchTextValid: true,
Expand Down Expand Up @@ -150,10 +152,12 @@ export class Table extends PureComponent<TableProps, TableState> {
allowedTypes,
} = this.props;

const cappedTotalItemCount = Math.min(totalItemCount, MAX_PAGINATED_ITEM);

const pagination = {
pageIndex,
pageSize,
totalItemCount,
totalItemCount: cappedTotalItemCount,
pageSizeOptions: [5, 10, 20, 50],
};

Expand Down Expand Up @@ -321,6 +325,7 @@ export class Table extends PureComponent<TableProps, TableState> {
);

const activeActionContents = this.state.activeAction?.render() ?? null;
const exceededResultCount = totalItemCount > MAX_PAGINATED_ITEM;

return (
<Fragment>
Expand Down Expand Up @@ -392,6 +397,18 @@ export class Table extends PureComponent<TableProps, TableState> {
/>
{queryParseError}
<EuiSpacer size="s" />
{exceededResultCount && (
<>
<EuiText color="subdued" size="s" data-test-subj="savedObjectsTableTooManyResultsLabel">
<FormattedMessage
id="savedObjectsManagement.objectsTable.table.tooManyResultsLabel"
defaultMessage="Showing {limit} of {totalItemCount, plural, one {# object} other {# objects}}"
values={{ totalItemCount, limit: MAX_PAGINATED_ITEM }}
/>
</EuiText>
<EuiSpacer size="s" />
</>
)}
<div data-test-subj="savedObjectsTable">
<EuiBasicTable
loading={isSearching}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,11 @@ export class SavedObjectsTable extends Component<SavedObjectsTableProps, SavedOb
blob = await fetchExportObjects(http, objectsToExport, includeReferencesDeep);
} catch (e) {
notifications.toasts.addDanger({
title: i18n.translate('savedObjectsManagement.objectsTable.export.dangerNotification', {
defaultMessage: 'Unable to generate export',
title: i18n.translate('savedObjectsManagement.objectsTable.export.toastErrorMessage', {
defaultMessage: 'Unable to generate export: {error}',
values: {
error: e.body?.message ?? e,
},
}),
});
throw e;
Expand Down Expand Up @@ -423,8 +426,11 @@ export class SavedObjectsTable extends Component<SavedObjectsTableProps, SavedOb
});
} catch (e) {
notifications.toasts.addDanger({
title: i18n.translate('savedObjectsManagement.objectsTable.export.dangerNotification', {
defaultMessage: 'Unable to generate export',
title: i18n.translate('savedObjectsManagement.objectsTable.export.toastErrorMessage', {
defaultMessage: 'Unable to generate export: {error}',
values: {
error: e.body?.message ?? e,
},
}),
});
throw e;
Expand Down
61 changes: 37 additions & 24 deletions src/plugins/saved_objects_management/server/lib/find_all.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,25 @@ describe('findAll', () => {
savedObjectsClient = savedObjectsClientMock.create();
});

it('calls the saved object client with the correct parameters', async () => {
it('calls `client.createPointInTimeFinder` with the correct parameters', async () => {
const query: SavedObjectsFindOptions = {
type: ['some-type', 'another-type'],
};

savedObjectsClient.find.mockResolvedValue({
saved_objects: [],
total: 1,
per_page: 20,
page: 1,
});

await findAll(savedObjectsClient, query);

expect(savedObjectsClient.createPointInTimeFinder).toHaveBeenCalledTimes(1);
expect(savedObjectsClient.createPointInTimeFinder).toHaveBeenCalledWith(query);
});

it('returns the results from the PIT search', async () => {
const query: SavedObjectsFindOptions = {
type: ['some-type', 'another-type'],
};
Expand All @@ -41,45 +59,40 @@ describe('findAll', () => {
const results = await findAll(savedObjectsClient, query);

expect(savedObjectsClient.find).toHaveBeenCalledTimes(1);
expect(savedObjectsClient.find).toHaveBeenCalledWith({
...query,
page: 1,
});
expect(savedObjectsClient.find).toHaveBeenCalledWith(
expect.objectContaining({
...query,
})
);

expect(results).toEqual([createObj(1), createObj(2)]);
});

it('recursively call find until all objects are fetched', async () => {
it('works when the PIT search returns multiple batches', async () => {
const query: SavedObjectsFindOptions = {
type: ['some-type', 'another-type'],
perPage: 2,
};
const objPerPage = 2;

savedObjectsClient.find.mockImplementation(({ page }) => {
const firstInPage = (page! - 1) * objPerPage + 1;
let callCount = 0;
savedObjectsClient.find.mockImplementation(({}) => {
callCount++;
const firstInPage = (callCount - 1) * objPerPage + 1;
return Promise.resolve({
saved_objects: [createObj(firstInPage), createObj(firstInPage + 1)],
saved_objects:
callCount > 3
? [createObj(firstInPage)]
: [createObj(firstInPage), createObj(firstInPage + 1)],
total: objPerPage * 3,
per_page: objPerPage,
page: page!,
page: callCount!,
});
});

const results = await findAll(savedObjectsClient, query);
expect(savedObjectsClient.find).toHaveBeenCalledTimes(3);
expect(savedObjectsClient.find).toHaveBeenCalledWith({
...query,
page: 1,
});
expect(savedObjectsClient.find).toHaveBeenCalledWith({
...query,
page: 2,
});
expect(savedObjectsClient.find).toHaveBeenCalledWith({
...query,
page: 3,
});

expect(results).toEqual(times(6, (num) => createObj(num + 1)));
expect(savedObjectsClient.find).toHaveBeenCalledTimes(4);
expect(results).toEqual(times(7, (num) => createObj(num + 1)));
});
});
32 changes: 11 additions & 21 deletions src/plugins/saved_objects_management/server/lib/find_all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,20 @@
* Side Public License, v 1.
*/

import { SavedObjectsClientContract, SavedObject, SavedObjectsFindOptions } from 'src/core/server';
import {
SavedObjectsClientContract,
SavedObject,
SavedObjectsCreatePointInTimeFinderOptions,
} from 'src/core/server';

export const findAll = async (
client: SavedObjectsClientContract,
findOptions: SavedObjectsFindOptions
findOptions: SavedObjectsCreatePointInTimeFinderOptions
): Promise<SavedObject[]> => {
return recursiveFind(client, findOptions, 1, []);
};

const recursiveFind = async (
client: SavedObjectsClientContract,
findOptions: SavedObjectsFindOptions,
page: number,
allObjects: SavedObject[]
): Promise<SavedObject[]> => {
const objects = await client.find({
...findOptions,
page,
});

allObjects.push(...objects.saved_objects);
if (allObjects.length < objects.total) {
return recursiveFind(client, findOptions, page + 1, allObjects);
const finder = client.createPointInTimeFinder(findOptions);
const results: SavedObject[] = [];
for await (const result of finder.find()) {
results.push(...result.saved_objects);
}

return allObjects;
return results;
};
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('registerRoutes', () => {

expect(httpSetup.createRouter).toHaveBeenCalledTimes(1);
expect(router.get).toHaveBeenCalledTimes(3);
expect(router.post).toHaveBeenCalledTimes(3);
expect(router.post).toHaveBeenCalledTimes(2);

expect(router.get).toHaveBeenCalledWith(
expect.objectContaining({
Expand Down Expand Up @@ -56,11 +56,5 @@ describe('registerRoutes', () => {
}),
expect.any(Function)
);
expect(router.post).toHaveBeenCalledWith(
expect.objectContaining({
path: '/api/kibana/management/saved_objects/scroll/export',
}),
expect.any(Function)
);
});
});
2 changes: 0 additions & 2 deletions src/plugins/saved_objects_management/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { ISavedObjectsManagement } from '../services';
import { registerFindRoute } from './find';
import { registerBulkGetRoute } from './bulk_get';
import { registerScrollForCountRoute } from './scroll_count';
import { registerScrollForExportRoute } from './scroll_export';
import { registerRelationshipsRoute } from './relationships';
import { registerGetAllowedTypesRoute } from './get_allowed_types';

Expand All @@ -25,7 +24,6 @@ export function registerRoutes({ http, managementServicePromise }: RegisterRoute
registerFindRoute(router, managementServicePromise);
registerBulkGetRoute(router, managementServicePromise);
registerScrollForCountRoute(router);
registerScrollForExportRoute(router);
registerRelationshipsRoute(router, managementServicePromise);
registerGetAllowedTypesRoute(router);
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { schema } from '@kbn/config-schema';
import { IRouter, SavedObjectsFindOptions } from 'src/core/server';
import { IRouter, SavedObjectsCreatePointInTimeFinderOptions } from 'src/core/server';
import { chain } from 'lodash';
import { findAll } from '../lib';

Expand Down Expand Up @@ -42,7 +42,7 @@ export const registerScrollForCountRoute = (router: IRouter) => {
.value();

const client = getClient({ includedHiddenTypes });
const findOptions: SavedObjectsFindOptions = {
const findOptions: SavedObjectsCreatePointInTimeFinderOptions = {
type: typesToInclude,
perPage: 1000,
};
Expand Down

This file was deleted.

Loading