From 7f98688a7f8cb2a908ae4f0a26fac00741d8730b Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Fri, 26 Aug 2022 10:41:12 -0700 Subject: [PATCH 01/49] Updates public api types with bulkDelete types --- .../src/apis/bulk_delete.ts | 44 +++++++++++++++++++ .../src/apis/index.ts | 6 +++ .../src/index.ts | 4 ++ .../src/saved_objects_client.ts | 13 ++++++ .../src/saved_objects_repository.ts | 14 ++++++ 5 files changed, 81 insertions(+) create mode 100644 packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts new file mode 100644 index 0000000000000..053de53a8b815 --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts @@ -0,0 +1,44 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { SavedObjectError } from '@kbn/core-saved-objects-common'; +import type { MutatingOperationRefreshSetting, SavedObjectsBaseOptions } from './base'; + +/** + * + * @public + */ +export interface SavedObjectsBulkDeleteObject { + type: string; + id: string; +} + +/** + * @public + */ +export interface SavedObjectsBulkDeleteOptions extends SavedObjectsBaseOptions { + refresh?: MutatingOperationRefreshSetting; + force?: boolean; +} + +/** + * @public + */ +export interface SavedObjectsBulkDeleteStatus { + id: string; + type: string; + success: boolean; + error?: SavedObjectError; +} + +/** + * @public + */ +export interface SavedObjectsBulkDeleteResponse { + statuses: SavedObjectsBulkDeleteStatus[]; +} diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts index 7dc8e7ab09fc6..d311f2316885d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts @@ -72,3 +72,9 @@ export type { SavedObjectsUpdateObjectsSpacesOptions, SavedObjectsUpdateObjectsSpacesResponseObject, } from './update_objects_spaces'; +export type { + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteStatus, + SavedObjectsBulkDeleteResponse, +} from './bulk_delete'; diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/index.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/index.ts index 93e35d01a6a3f..b48fd957e96a0 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/index.ts @@ -52,4 +52,8 @@ export type { SavedObjectsCreatePointInTimeFinderOptions, SavedObjectsFindOptions, SavedObjectsPointInTimeFinderClient, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteStatus, + SavedObjectsBulkDeleteResponse, } from './apis'; diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_client.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_client.ts index 82808e4024c73..cfe1f4e6a146b 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_client.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_client.ts @@ -41,6 +41,9 @@ import type { SavedObjectsRemoveReferencesToResponse, SavedObjectsCollectMultiNamespaceReferencesOptions, SavedObjectsBulkResponse, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteResponse, } from './apis'; /** @@ -151,6 +154,16 @@ export interface SavedObjectsClientContract { */ delete(type: string, id: string, options?: SavedObjectsDeleteOptions): Promise<{}>; + /** + * Deletes multiple SavedObjects batched together as a single request + * + * @param objects + * @param options + */ + bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options?: SavedObjectsBulkDeleteOptions + ): Promise; /** * Find all SavedObjects matching the search query * diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts index 102afce9dd73d..d789acb7621c9 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts @@ -44,6 +44,9 @@ import type { SavedObjectsDeleteByNamespaceOptions, SavedObjectsIncrementCounterField, SavedObjectsIncrementCounterOptions, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteResponse, } from './apis'; /** @@ -105,6 +108,17 @@ export interface ISavedObjectsRepository { */ delete(type: string, id: string, options?: SavedObjectsDeleteOptions): Promise<{}>; + /** + * Deletes multiple documents at once + * @param {array} objects - an array of objects containing id and type + * @param {object} [options={}] + * @returns {promise} - { statuses: [{ success, error }] } + */ + bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options: SavedObjectsBulkDeleteOptions + ): Promise; + /** * Deletes all objects from the provided namespace. * From 1b9c5c46efe6d8147aae7047bf709cae4466fcff Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Fri, 26 Aug 2022 11:12:26 -0700 Subject: [PATCH 02/49] Adds bulkDelete server side implementation --- .../src/lib/repository.ts | 243 ++++++++++++++++++ .../src/mocks/repository.mock.ts | 1 + .../src/saved_objects_client.test.ts | 18 ++ .../src/saved_objects_client.ts | 11 + .../src/repository.mock.ts | 1 + .../src/saved_objects_client.mock.ts | 1 + src/core/server/types.ts | 3 + ...ecure_saved_objects_client_wrapper.test.ts | 11 + .../secure_saved_objects_client_wrapper.ts | 11 + 9 files changed, 300 insertions(+) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 961b44a1cd688..cad2c046c4d7d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -9,6 +9,7 @@ import { omit, isObject } from 'lodash'; import type { Payload } from '@hapi/boom'; import * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; +import { BulkResponseItem, ErrorCause } from '@elastic/elasticsearch/lib/api/types'; import * as esKuery from '@kbn/es-query'; import type { Logger } from '@kbn/logging'; import type { ElasticsearchClient } from '@kbn/core-elasticsearch-server'; @@ -54,6 +55,9 @@ import type { SavedObjectsClosePointInTimeOptions, SavedObjectsCreatePointInTimeFinderOptions, SavedObjectsFindOptions, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteResponse, } from '@kbn/core-saved-objects-api-server'; import type { SavedObjectSanitizedDoc, @@ -762,6 +766,245 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { ); } + /** + * {@inheritDoc ISvedObjectsRepository.bulkDelete} + */ + async bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options: SavedObjectsBulkDeleteOptions + ): Promise { + const { refresh = DEFAULT_REFRESH_SETTING, force } = options; + const namespace = normalizeNamespace(options.namespace); + let bulkGetRequestIndexCounter = 0; + // TODO: move to internal types + type ExpectedBulkGetResult = Either< + { type: string; id: string; error: Payload }, + { type: string; id: string; version?: string; esRequestIndex?: number } + >; + const expectedBulkGetResults = objects.map((object) => { + const { type, id } = object; + if (!this._allowedTypes.includes(type)) { + return { + tag: 'Left', + value: { + id, + type, + error: errorContent(SavedObjectsErrorHelpers.createUnsupportedTypeError(type)), + }, + }; + } + const requiresNamespacesCheck = this._registry.isMultiNamespace(type); + return { + tag: 'Right', + value: { + type, + id, + ...(requiresNamespacesCheck && { esRequestIndex: bulkGetRequestIndexCounter++ }), + }, + }; + }); + + // get multi-namespace saved objects. + const bulkGetDocs = expectedBulkGetResults + .filter(isRight) + .filter(({ value }) => value.esRequestIndex !== undefined) + .map(({ value: { type, id } }) => ({ + _id: this._serializer.generateRawId(namespace, type, id), + _index: this.getIndexForType(type), + _source: ['type', 'namespaces'], + })); + + const bulkGetResponse = bulkGetDocs.length + ? await this.client.mget({ body: { docs: bulkGetDocs } }, { ignore: [404], meta: true }) + : undefined; + // fail fast if we can't verify a 404 response is from Elasticsearch + if ( + bulkGetResponse && + isNotFoundFromUnsupportedServer({ + statusCode: bulkGetResponse.statusCode, + headers: bulkGetResponse.headers, + }) + ) { + throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(); + } + // TODO: move to internal types + interface BulkDeleteParams { + delete: BulkDeleteParamsItem; + } + // TODO: move to internal types + interface BulkDeleteParamsItem { + if_seq_no?: number; + if_primary_term?: number; + _id: string; + _index: string; + } + // TODO: move to internal types + type ExpectedBulkDeleteResult = Either< + { type: string; id: string; error: Payload }, + { + type: string; + id: string; + namespaces: string[]; + esRequestIndex: number; + } + >; + + let bulkDeleteRequestIndexCounter = 0; + const bulkDeleteParams: BulkDeleteParams[] = []; + + const expectedBulkDeleteResults = await Promise.all( + expectedBulkGetResults.map>( + async (expectedBulkGetResult) => { + if (isLeft(expectedBulkGetResult)) { + return { ...expectedBulkGetResult }; + } + const { esRequestIndex, id, type, version } = expectedBulkGetResult.value; + + let namespaces; + let versionProperties; + + if (esRequestIndex !== undefined) { + const indexFound = bulkGetResponse?.statusCode !== 404; + + const actualResult = indexFound + ? bulkGetResponse?.body.docs[esRequestIndex] + : undefined; + + const docFound = indexFound && isMgetDoc(actualResult) && actualResult.found; + + if (!docFound) { + return { + tag: 'Left', + value: { + id, + type, + error: errorContent( + SavedObjectsErrorHelpers.createGenericNotFoundError(type, id) + ), + }, + }; + } + if ( + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + !this.rawDocExistsInNamespace(actualResult, namespace) && + !force + ) { + // the document exists but not in the namespace for this client. Deleting the doc is still possible but one needs to force the action. + return { + tag: 'Left', + value: { + success: false, + id, + type, + error: errorContent( + SavedObjectsErrorHelpers.createBadRequestError( + `Unable to delete saved object id: ${id}, type: ${type} that exists in multiple namespaces, use the "force" option to delete all saved objects` + ) + ), + }, + }; + } + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + namespaces = actualResult!._source.namespaces ?? [ + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + SavedObjectsUtils.namespaceIdToString(actualResult!._source.namespace), + ]; + versionProperties = getExpectedVersionProperties(version); + } else { + if (this._registry.isSingleNamespace(type)) { + namespaces = [SavedObjectsUtils.namespaceIdToString(namespace)]; + } + versionProperties = getExpectedVersionProperties(version); + } + + const expectedResult = { + type, + id, + namespaces, + esRequestIndex: bulkDeleteRequestIndexCounter++, + }; + + bulkDeleteParams.push({ + delete: { + _id: this._serializer.generateRawId(namespace, type, id), + _index: this.getIndexForType(type), + ...versionProperties, + }, + }); + + return { tag: 'Right', value: expectedResult }; + } + ) + ); + + const bulkDeleteResponse = bulkDeleteParams.length + ? await this.client.bulk({ + refresh, + body: bulkDeleteParams, + _source_includes: ['originId'], + require_alias: true, + }) + : undefined; + + // extracted to ensure consistency in the error results returned + let errorResult: { success: boolean; type: string; id: string; error: Payload }; + + const savedObjects = await Promise.all( + expectedBulkDeleteResults.map(async (expectedResult) => { + if (isLeft(expectedResult)) { + errorResult = { ...expectedResult.value, success: false }; + return errorResult; + } + const { type, id, namespaces, esRequestIndex } = expectedResult.value; + + // TODO: move to internal types + type NewBulkItemResponse = BulkResponseItem & { error: ErrorCause & { index: string } }; + + // we assume this wouldn't happen but is needed to circumvent type issues + if (bulkDeleteResponse === undefined) throw new Error(); + + const rawResponse = Object.values( + bulkDeleteResponse.items[esRequestIndex] + )[0] as NewBulkItemResponse; + + const error = getBulkOperationError(type, id, rawResponse); + if (error) { + errorResult = { success: false, type, id, error }; + return errorResult; + } + const deleted = rawResponse.result === 'deleted'; + if (deleted) { + if (namespaces) { + await deleteLegacyUrlAliases({ + mappings: this._mappings, + registry: this._registry, + client: this.client, + getIndexForType: this.getIndexForType.bind(this), + type, + id, + ...(namespaces.includes(ALL_NAMESPACES_STRING) + ? { namespaces: [], deleteBehavior: 'exclusive' } // delete legacy URL aliases for this type/ID for all spaces + : { namespaces, deleteBehavior: 'inclusive' }), // delete legacy URL aliases for this type/ID for these specific spaces + }).catch((err) => { + // The object has already been deleted, but we caught an error when attempting to delete aliases. + // A consumer cannot attempt to delete the object again, so just log the error and swallow it. + this._logger.error( + `Unable to delete aliases when deleting an object: ${err.message}` + ); + }); + } + } + const successfulResult = { + success: true, + id, + type, + }; + return successfulResult; + }) + ); + return { statuses: [...savedObjects] }; + } + /** * {@inheritDoc ISavedObjectsRepository.deleteByNamespace} */ diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/mocks/repository.mock.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/mocks/repository.mock.ts index 2cdfcf1710ad4..dc6c06c0c828d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/mocks/repository.mock.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/mocks/repository.mock.ts @@ -16,6 +16,7 @@ const createRepositoryMock = () => { create: jest.fn(), bulkCreate: jest.fn(), bulkUpdate: jest.fn(), + bulkDelete: jest.fn(), delete: jest.fn(), bulkGet: jest.fn(), find: jest.fn(), diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.test.ts index 5829f34a6ba79..38d4e75a0c528 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.test.ts @@ -23,6 +23,8 @@ import type { SavedObjectsFindOptions, SavedObjectsUpdateObjectsSpacesObject, SavedObjectsUpdateObjectsSpacesOptions, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteObject, } from '@kbn/core-saved-objects-api-server'; import { SavedObjectsClient } from './saved_objects_client'; import { repositoryMock, savedObjectsPointInTimeFinderMock } from './mocks'; @@ -119,6 +121,22 @@ describe('SavedObjectsClient', () => { }); }); + test(`#bulkDelete`, async () => { + const returnValue: any = Symbol(); + mockRepository.bulkDelete.mockResolvedValueOnce(returnValue); + const client = new SavedObjectsClient(mockRepository); + + const objects: SavedObjectsBulkDeleteObject[] = [ + { type: 'foo', id: '1' }, + { type: 'bar', id: '2' }, + ]; + const options: SavedObjectsBulkDeleteOptions = { namespace: 'ns-1', refresh: true }; + const result = await client.bulkDelete(objects, options); + + expect(mockRepository.bulkDelete).toHaveBeenCalledWith(objects, options); + expect(result).toBe(returnValue); + }); + test(`#delete`, async () => { const returnValue: any = Symbol(); mockRepository.delete.mockResolvedValueOnce(returnValue); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.ts index 7c2b3a205b76d..50f78f09dd684 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/saved_objects_client.ts @@ -39,6 +39,9 @@ import type { SavedObjectsClosePointInTimeOptions, SavedObjectsCreatePointInTimeFinderOptions, SavedObjectsFindOptions, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteResponse, } from '@kbn/core-saved-objects-api-server'; import { SavedObjectsErrorHelpers } from '@kbn/core-saved-objects-utils-server'; @@ -83,6 +86,14 @@ export class SavedObjectsClient implements SavedObjectsClientContract { return await this._repository.delete(type, id, options); } + /** {@inheritDoc SavedObjectsClientContract.bulkDelete} */ + async bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options: SavedObjectsBulkDeleteOptions = {} + ): Promise { + return await this._repository.bulkDelete(objects, options); + } + /** {@inheritDoc SavedObjectsClientContract.find} */ async find( options: SavedObjectsFindOptions diff --git a/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/repository.mock.ts b/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/repository.mock.ts index d950b041d2432..168f4c8de6b59 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/repository.mock.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/repository.mock.ts @@ -15,6 +15,7 @@ const create = () => { create: jest.fn(), bulkCreate: jest.fn(), bulkUpdate: jest.fn(), + bulkDelete: jest.fn(), delete: jest.fn(), bulkGet: jest.fn(), find: jest.fn(), diff --git a/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/saved_objects_client.mock.ts b/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/saved_objects_client.mock.ts index 75ee540cb7d8a..523e5003e650f 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/saved_objects_client.mock.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-mocks/src/saved_objects_client.mock.ts @@ -18,6 +18,7 @@ const create = () => { checkConflicts: jest.fn(), bulkUpdate: jest.fn(), delete: jest.fn(), + bulkDelete: jest.fn(), bulkGet: jest.fn(), find: jest.fn(), get: jest.fn(), diff --git a/src/core/server/types.ts b/src/core/server/types.ts index 966380ba99c8f..88717746843eb 100644 --- a/src/core/server/types.ts +++ b/src/core/server/types.ts @@ -40,6 +40,9 @@ export type { SavedObjectsClientContract, SavedObjectReferenceWithContext, SavedObjectsCollectMultiNamespaceReferencesResponse, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteResponse, } from '@kbn/core-saved-objects-api-server'; export type { DomainDeprecationDetails, diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 52702c014f0cb..c2e65f8a4701a 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -591,6 +591,17 @@ describe('#bulkUpdate', () => { }); }); +describe('#bulkDelete', () => { + test.todo(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => {}); + test.todo(`throws decorated ForbiddenError when unauthorized`, async () => {}); + test.todo(`returns result of baseClient.bulkUpdate when authorized`, async () => {}); + test.todo(`checks privileges for user, actions, and namespace`, async () => {}); + test.todo(`checks privileges for object namespaces if present`, async () => {}); + test.todo(`filters namespaces that the user doesn't have access to`, async () => {}); + test.todo(`adds audit event when successful`, async () => {}); + test.todo(`adds audit event when not successful`, async () => {}); +}); + describe('#checkConflicts', () => { const obj1 = Object.freeze({ type: 'foo', id: 'foo-id' }); const obj2 = Object.freeze({ type: 'bar', id: 'bar-id' }); diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index db0645992b2f7..b87fc005a63b9 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -9,6 +9,9 @@ import type { SavedObjectReferenceWithContext, SavedObjectsBaseOptions, SavedObjectsBulkCreateObject, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteResponse, SavedObjectsBulkGetObject, SavedObjectsBulkResolveObject, SavedObjectsBulkUpdateObject, @@ -224,6 +227,14 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra return await this.baseClient.delete(type, id, options); } + // TODO: Implement + public async bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options?: SavedObjectsBulkDeleteOptions + ): Promise { + throw new Error('Method not implemented.'); + } + public async find(options: SavedObjectsFindOptions) { if ( this.getSpacesService() == null && From 254edb356d4a13656e7ee7571dfc1afe941b605f Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Fri, 26 Aug 2022 11:55:57 -0700 Subject: [PATCH 03/49] Adds bulk_delete server-side route, updates core usage data accordingly --- .../src/apis/bulk_delete.ts | 34 +++++++++ .../src/apis/index.ts | 6 ++ .../src/index.ts | 4 + .../src/saved_objects_client.ts | 16 ++++ .../src/saved_objects_client.ts | 27 +++++++ .../src/saved_objects_service.mock.ts | 1 + .../src/routes/bulk_delete.ts | 48 ++++++++++++ .../src/routes/index.ts | 2 + .../src/usage_stats_client.ts | 2 + .../src/core_usage_stats.ts | 7 ++ .../core_usage_stats_client.mock.ts | 1 + .../core_usage_stats_client.test.ts | 76 +++++++++++++++++++ .../core_usage_stats_client.ts | 6 ++ src/core/server/index.ts | 3 + src/core/server/types.ts | 3 - .../spaces_saved_objects_client.test.ts | 30 ++++++++ .../spaces_saved_objects_client.ts | 12 +++ 17 files changed, 275 insertions(+), 3 deletions(-) create mode 100644 packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts create mode 100644 packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_delete.ts diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts new file mode 100644 index 0000000000000..69bf43fb10ba1 --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts @@ -0,0 +1,34 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { SavedObjectError } from '@kbn/core-saved-objects-common'; + +/** @public */ +export interface SavedObjectsBulkDeleteObject { + type: string; + id: string; +} + +/** @public */ +export interface SavedObjectsBulkDeleteOptions { + namespace?: string; + force?: boolean; +} + +/** @public */ +export interface SavedObjectsBulkDeleteItemResponse { + id: string; + type: string; + success: boolean; + error?: SavedObjectError | Error; +} + +/** @public */ +export interface SavedObjectsBulkDeleteResponse { + statuses: SavedObjectsBulkDeleteItemResponse[]; +} diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts index 4652facb972cc..11f977438b873 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts @@ -19,3 +19,9 @@ export type { } from './find'; export type { ResolvedSimpleSavedObject } from './resolve'; export type { SavedObjectsUpdateOptions } from './update'; +export type { + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteItemResponse, + SavedObjectsBulkDeleteResponse, +} from './bulk_delete'; diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts index 82a93cf091c77..372a4fdc95113 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts @@ -22,4 +22,8 @@ export type { SavedObjectsBulkUpdateOptions, SavedObjectsBulkResolveResponse, SavedObjectsBulkCreateObject, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteItemResponse, + SavedObjectsBulkDeleteResponse, } from './apis'; diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts index 123b5c81d4064..2ec370c9777e9 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts @@ -20,6 +20,11 @@ import type { SavedObjectsUpdateOptions, SavedObjectsDeleteOptions, } from './apis'; +import { + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteResponse, +} from './apis/bulk_delete'; import type { SimpleSavedObject } from './simple_saved_object'; /** @@ -52,6 +57,17 @@ export interface SavedObjectsClientContract { */ delete(type: string, id: string, options?: SavedObjectsDeleteOptions): Promise<{}>; + /** + * Deletes multiple documents at once + * @param objects - an array of objects containing id, type + * @param options - optional force argument to force deletion of objects in a namespace other than the scoped client + * @returns The bulk delete result for the saved objects for the given types and ids. + */ + bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options?: SavedObjectsBulkDeleteOptions + ): Promise; + /** * Search for objects * diff --git a/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts b/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts index 3a16030983fa9..db2469d7663ad 100644 --- a/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts +++ b/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts @@ -11,6 +11,7 @@ import type { HttpSetup, HttpFetchOptions } from '@kbn/core-http-browser'; import type { SavedObject, SavedObjectTypeIdTuple } from '@kbn/core-saved-objects-common'; import type { SavedObjectsBulkResolveResponse as SavedObjectsBulkResolveResponseServer, + SavedObjectsBulkDeleteResponse as SavedObjectsBulkDeleteResponseServer, SavedObjectsClientContract as SavedObjectsApi, SavedObjectsFindResponse as SavedObjectsFindResponseServer, SavedObjectsResolveResponse, @@ -28,6 +29,7 @@ import type { SavedObjectsBulkCreateOptions, SavedObjectsBulkCreateObject, SimpleSavedObject, + // SavedObjectsBulkDeleteObjects } from '@kbn/core-saved-objects-api-browser'; import { SimpleSavedObjectImpl } from './simple_saved_object'; @@ -255,6 +257,31 @@ export class SavedObjectsClient implements SavedObjectsClientContract { return this.savedObjectsFetch(this.getPath([type, id]), { method: 'DELETE', query }); }; + public bulkDelete = async ( + objects: SavedObjectTypeIdTuple[], + options?: { force?: boolean } + ): ReturnType => { + const filteredObjects = objects.map(({ type, id }) => ({ type, id })); + const queryOptions = { force: !!options?.force }; + const response = await this.performBulkDelete(filteredObjects, queryOptions); + return { + statuses: response.statuses, + }; + }; + + private async performBulkDelete( + objects: SavedObjectTypeIdTuple[], + queryOptions: { force: boolean } + ) { + const path = this.getPath(['_bulk_delete']); + const request: Promise = this.savedObjectsFetch(path, { + method: 'POST', + body: JSON.stringify(objects), + query: queryOptions, + }); + return request; + } + public find = ( options: SavedObjectsFindOptions ): Promise> => { diff --git a/packages/core/saved-objects/core-saved-objects-browser-mocks/src/saved_objects_service.mock.ts b/packages/core/saved-objects/core-saved-objects-browser-mocks/src/saved_objects_service.mock.ts index 0caa572238807..2239b94d7e2eb 100644 --- a/packages/core/saved-objects/core-saved-objects-browser-mocks/src/saved_objects_service.mock.ts +++ b/packages/core/saved-objects/core-saved-objects-browser-mocks/src/saved_objects_service.mock.ts @@ -19,6 +19,7 @@ const createStartContractMock = () => { bulkCreate: jest.fn(), bulkResolve: jest.fn(), bulkUpdate: jest.fn(), + bulkDelete: jest.fn(), delete: jest.fn(), bulkGet: jest.fn(), find: jest.fn(), diff --git a/packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_delete.ts b/packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_delete.ts new file mode 100644 index 0000000000000..f435eadebd066 --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-server-internal/src/routes/bulk_delete.ts @@ -0,0 +1,48 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import { schema } from '@kbn/config-schema'; +import type { InternalCoreUsageDataSetup } from '@kbn/core-usage-data-base-server-internal'; +import type { InternalSavedObjectRouter } from '../internal_types'; +import { catchAndReturnBoomErrors } from './utils'; + +interface RouteDependencies { + coreUsageData: InternalCoreUsageDataSetup; +} + +export const registerBulkDeleteRoute = ( + router: InternalSavedObjectRouter, + { coreUsageData }: RouteDependencies +) => { + router.post( + { + path: '/_bulk_delete', + validate: { + body: schema.arrayOf( + schema.object({ + type: schema.string(), + id: schema.string(), + }) + ), + query: schema.object({ + force: schema.maybe(schema.boolean()), + }), + }, + }, + catchAndReturnBoomErrors(async (context, req, res) => { + const { force } = req.query; + const usageStatsClient = coreUsageData.getClient(); + usageStatsClient.incrementSavedObjectsBulkDelete({ request: req }).catch(() => {}); + + const { savedObjects } = await context.core; + + const statuses = await savedObjects.client.bulkDelete(req.body, { force }); + return res.ok({ body: statuses }); + }) + ); +}; diff --git a/packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts b/packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts index 528793e8539bd..89d5b41dd8885 100644 --- a/packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts +++ b/packages/core/saved-objects/core-saved-objects-server-internal/src/routes/index.ts @@ -23,6 +23,7 @@ import { registerUpdateRoute } from './update'; import { registerBulkGetRoute } from './bulk_get'; import { registerBulkCreateRoute } from './bulk_create'; import { registerBulkUpdateRoute } from './bulk_update'; +import { registerBulkDeleteRoute } from './bulk_delete'; import { registerExportRoute } from './export'; import { registerImportRoute } from './import'; import { registerResolveImportErrorsRoute } from './resolve_import_errors'; @@ -62,6 +63,7 @@ export function registerRoutes({ registerBulkCreateRoute(router, { coreUsageData }); registerBulkResolveRoute(router, { coreUsageData }); registerBulkUpdateRoute(router, { coreUsageData }); + registerBulkDeleteRoute(router, { coreUsageData }); registerExportRoute(router, { config, coreUsageData }); registerImportRoute(router, { config, coreUsageData }); registerResolveImportErrorsRoute(router, { config, coreUsageData }); diff --git a/packages/core/usage-data/core-usage-data-base-server-internal/src/usage_stats_client.ts b/packages/core/usage-data/core-usage-data-base-server-internal/src/usage_stats_client.ts index 3a603ebfdf5f0..735a4ab261658 100644 --- a/packages/core/usage-data/core-usage-data-base-server-internal/src/usage_stats_client.ts +++ b/packages/core/usage-data/core-usage-data-base-server-internal/src/usage_stats_client.ts @@ -43,6 +43,8 @@ export interface ICoreUsageStatsClient { incrementSavedObjectsBulkUpdate(options: BaseIncrementOptions): Promise; + incrementSavedObjectsBulkDelete(options: BaseIncrementOptions): Promise; + incrementSavedObjectsCreate(options: BaseIncrementOptions): Promise; incrementSavedObjectsDelete(options: BaseIncrementOptions): Promise; diff --git a/packages/core/usage-data/core-usage-data-server/src/core_usage_stats.ts b/packages/core/usage-data/core-usage-data-server/src/core_usage_stats.ts index aef5b657fb6f7..279d5c68cd733 100644 --- a/packages/core/usage-data/core-usage-data-server/src/core_usage_stats.ts +++ b/packages/core/usage-data/core-usage-data-server/src/core_usage_stats.ts @@ -42,6 +42,13 @@ export interface CoreUsageStats { 'apiCalls.savedObjectsBulkUpdate.namespace.custom.total'?: number; 'apiCalls.savedObjectsBulkUpdate.namespace.custom.kibanaRequest.yes'?: number; 'apiCalls.savedObjectsBulkUpdate.namespace.custom.kibanaRequest.no'?: number; + 'apiCalls.savedObjectsBulkDelete.total'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.default.total'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.yes'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.no'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.custom.total'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.yes'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.no'?: number; 'apiCalls.savedObjectsCreate.total'?: number; 'apiCalls.savedObjectsCreate.namespace.default.total'?: number; 'apiCalls.savedObjectsCreate.namespace.default.kibanaRequest.yes'?: number; diff --git a/src/core/server/core_usage_data/core_usage_stats_client.mock.ts b/src/core/server/core_usage_data/core_usage_stats_client.mock.ts index ac5855c3a4ba0..49b7a7b041298 100644 --- a/src/core/server/core_usage_data/core_usage_stats_client.mock.ts +++ b/src/core/server/core_usage_data/core_usage_stats_client.mock.ts @@ -15,6 +15,7 @@ const createUsageStatsClientMock = () => incrementSavedObjectsBulkGet: jest.fn().mockResolvedValue(null), incrementSavedObjectsBulkResolve: jest.fn().mockResolvedValue(null), incrementSavedObjectsBulkUpdate: jest.fn().mockResolvedValue(null), + incrementSavedObjectsBulkDelete: jest.fn().mockResolvedValue(null), incrementSavedObjectsCreate: jest.fn().mockResolvedValue(null), incrementSavedObjectsDelete: jest.fn().mockResolvedValue(null), incrementSavedObjectsFind: jest.fn().mockResolvedValue(null), diff --git a/src/core/server/core_usage_data/core_usage_stats_client.test.ts b/src/core/server/core_usage_data/core_usage_stats_client.test.ts index a06a239217601..425954847ad73 100644 --- a/src/core/server/core_usage_data/core_usage_stats_client.test.ts +++ b/src/core/server/core_usage_data/core_usage_stats_client.test.ts @@ -19,6 +19,7 @@ import { BULK_CREATE_STATS_PREFIX, BULK_GET_STATS_PREFIX, BULK_UPDATE_STATS_PREFIX, + BULK_DELETE_STATS_PREFIX, CREATE_STATS_PREFIX, DELETE_STATS_PREFIX, FIND_STATS_PREFIX, @@ -451,6 +452,81 @@ describe('CoreUsageStatsClient', () => { }); }); + describe('#incrementSavedObjectsBulkDelete', () => { + it('does not throw an error if repository incrementCounter operation fails', async () => { + const { usageStatsClient, repositoryMock } = setup(); + repositoryMock.incrementCounter.mockRejectedValue(new Error('Oh no!')); + + const request = httpServerMock.createKibanaRequest(); + await expect( + usageStatsClient.incrementSavedObjectsBulkDelete({ + request, + } as BaseIncrementOptions) + ).resolves.toBeUndefined(); + expect(repositoryMock.incrementCounter).toHaveBeenCalled(); + }); + + it('handles falsy options appropriately', async () => { + const { usageStatsClient, repositoryMock } = setup(); + + const request = httpServerMock.createKibanaRequest(); + await usageStatsClient.incrementSavedObjectsBulkDelete({ + request, + } as BaseIncrementOptions); + expect(repositoryMock.incrementCounter).toHaveBeenCalledTimes(1); + expect(repositoryMock.incrementCounter).toHaveBeenCalledWith( + CORE_USAGE_STATS_TYPE, + CORE_USAGE_STATS_ID, + [ + `${BULK_DELETE_STATS_PREFIX}.total`, + `${BULK_DELETE_STATS_PREFIX}.namespace.default.total`, + `${BULK_DELETE_STATS_PREFIX}.namespace.default.kibanaRequest.no`, + ], + incrementOptions + ); + }); + + it('handles truthy options and the default namespace string appropriately', async () => { + const { usageStatsClient, repositoryMock } = setup(DEFAULT_NAMESPACE_STRING); + + const request = httpServerMock.createKibanaRequest({ headers: firstPartyRequestHeaders }); + await usageStatsClient.incrementSavedObjectsBulkDelete({ + request, + } as BaseIncrementOptions); + expect(repositoryMock.incrementCounter).toHaveBeenCalledTimes(1); + expect(repositoryMock.incrementCounter).toHaveBeenCalledWith( + CORE_USAGE_STATS_TYPE, + CORE_USAGE_STATS_ID, + [ + `${BULK_DELETE_STATS_PREFIX}.total`, + `${BULK_DELETE_STATS_PREFIX}.namespace.default.total`, + `${BULK_DELETE_STATS_PREFIX}.namespace.default.kibanaRequest.yes`, + ], + incrementOptions + ); + }); + + it('handles a non-default space appropriately', async () => { + const { usageStatsClient, repositoryMock } = setup('foo'); + + const request = httpServerMock.createKibanaRequest(); + await usageStatsClient.incrementSavedObjectsBulkDelete({ + request, + } as BaseIncrementOptions); + expect(repositoryMock.incrementCounter).toHaveBeenCalledTimes(1); + expect(repositoryMock.incrementCounter).toHaveBeenCalledWith( + CORE_USAGE_STATS_TYPE, + CORE_USAGE_STATS_ID, + [ + `${BULK_DELETE_STATS_PREFIX}.total`, + `${BULK_DELETE_STATS_PREFIX}.namespace.custom.total`, + `${BULK_DELETE_STATS_PREFIX}.namespace.custom.kibanaRequest.no`, + ], + incrementOptions + ); + }); + }); + describe('#incrementSavedObjectsDelete', () => { it('does not throw an error if repository incrementCounter operation fails', async () => { const { usageStatsClient, repositoryMock } = setup(); diff --git a/src/core/server/core_usage_data/core_usage_stats_client.ts b/src/core/server/core_usage_data/core_usage_stats_client.ts index f7c5709afa52d..fb572d5d91b0e 100644 --- a/src/core/server/core_usage_data/core_usage_stats_client.ts +++ b/src/core/server/core_usage_data/core_usage_stats_client.ts @@ -24,6 +24,7 @@ export const BULK_CREATE_STATS_PREFIX = 'apiCalls.savedObjectsBulkCreate'; export const BULK_GET_STATS_PREFIX = 'apiCalls.savedObjectsBulkGet'; export const BULK_RESOLVE_STATS_PREFIX = 'apiCalls.savedObjectsBulkResolve'; export const BULK_UPDATE_STATS_PREFIX = 'apiCalls.savedObjectsBulkUpdate'; +export const BULK_DELETE_STATS_PREFIX = 'apiCalls.savedObjectsBulkDelete'; export const CREATE_STATS_PREFIX = 'apiCalls.savedObjectsCreate'; export const DELETE_STATS_PREFIX = 'apiCalls.savedObjectsDelete'; export const FIND_STATS_PREFIX = 'apiCalls.savedObjectsFind'; @@ -42,6 +43,7 @@ const ALL_COUNTER_FIELDS = [ ...getFieldsForCounter(BULK_GET_STATS_PREFIX), ...getFieldsForCounter(BULK_RESOLVE_STATS_PREFIX), ...getFieldsForCounter(BULK_UPDATE_STATS_PREFIX), + ...getFieldsForCounter(BULK_DELETE_STATS_PREFIX), ...getFieldsForCounter(CREATE_STATS_PREFIX), ...getFieldsForCounter(DELETE_STATS_PREFIX), ...getFieldsForCounter(FIND_STATS_PREFIX), @@ -113,6 +115,10 @@ export class CoreUsageStatsClient implements ICoreUsageStatsClient { await this.updateUsageStats([], BULK_UPDATE_STATS_PREFIX, options); } + public async incrementSavedObjectsBulkDelete(options: BaseIncrementOptions) { + await this.updateUsageStats([], BULK_DELETE_STATS_PREFIX, options); + } + public async incrementSavedObjectsCreate(options: BaseIncrementOptions) { await this.updateUsageStats([], CREATE_STATS_PREFIX, options); } diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 98fd84e6fc70f..f058f9e670ce1 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -343,6 +343,9 @@ export type { SavedObjectsFindOptions, SavedObjectsFindOptionsReference, SavedObjectsPitParams, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, + SavedObjectsBulkDeleteResponse, } from '@kbn/core-saved-objects-api-server'; export type { SavedObjectsServiceSetup, diff --git a/src/core/server/types.ts b/src/core/server/types.ts index 88717746843eb..966380ba99c8f 100644 --- a/src/core/server/types.ts +++ b/src/core/server/types.ts @@ -40,9 +40,6 @@ export type { SavedObjectsClientContract, SavedObjectReferenceWithContext, SavedObjectsCollectMultiNamespaceReferencesResponse, - SavedObjectsBulkDeleteObject, - SavedObjectsBulkDeleteOptions, - SavedObjectsBulkDeleteResponse, } from '@kbn/core-saved-objects-api-server'; export type { DomainDeprecationDetails, diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index 0060039f6f2ca..bcf3e3deb3be5 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -586,6 +586,36 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); }); + describe('#bulkDelete', () => { + test(`throws error if options.namespace is specified`, async () => { + const { client } = createSpacesSavedObjectsClient(); + + await expect( + // @ts-expect-error + client.bulkDelete(null, { namespace: 'bar' }) + ).rejects.toThrow(ERROR_NAMESPACE_SPECIFIED); + }); + + test(`supplements options with the current namespace`, async () => { + const { client, baseClient } = createSpacesSavedObjectsClient(); + const expectedReturnValue = { statuses: [{ id: 'id', type: 'type', success: true }] }; + baseClient.bulkDelete.mockReturnValue(Promise.resolve(expectedReturnValue)); + + const actualReturnValue = await client.bulkDelete([{ id: 'id', type: 'foo' }]); + + expect(actualReturnValue).toBe(expectedReturnValue); + expect(baseClient.bulkDelete).toHaveBeenCalledWith( + [ + { + id: 'id', + type: 'foo', + }, + ], + { namespace: currentSpace.expectedNamespace } + ); + }); + }); + describe('#removeReferencesTo', () => { test(`throws error if options.namespace is specified`, async () => { const { client } = createSpacesSavedObjectsClient(); diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index a7ef2dae5b386..ece3da264d939 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -12,6 +12,7 @@ import type { SavedObject, SavedObjectsBaseOptions, SavedObjectsBulkCreateObject, + SavedObjectsBulkDeleteObject, SavedObjectsBulkGetObject, SavedObjectsBulkResolveObject, SavedObjectsBulkUpdateObject, @@ -139,6 +140,17 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { }); } + async bulkDelete( + objects: SavedObjectsBulkDeleteObject[] = [], + options: SavedObjectsBaseOptions = {} + ) { + throwErrorIfNamespaceSpecified(options); + return await this.client.bulkDelete(objects, { + ...options, + namespace: spaceIdToNamespace(this.spaceId), + }); + } + async find(options: SavedObjectsFindOptions) { let namespaces: string[]; try { From bc5c98fd42ddf13ced8969ae4bd7a24232c066f7 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Fri, 26 Aug 2022 16:54:46 -0700 Subject: [PATCH 04/49] Adds bulkDelete to client side api, client and contract, adds implementation --- .../src/apis/bulk_delete.ts | 9 +-- .../src/apis/index.ts | 1 - .../src/index.ts | 1 - .../src/saved_objects_client.ts | 10 ++-- .../src/lib/repository.ts | 44 ++++---------- .../repository_bulk_delete_internal_types.ts | 57 +++++++++++++++++++ .../src/saved_objects_client.test.ts | 39 +++++++++++++ .../src/saved_objects_client.ts | 7 ++- 8 files changed, 116 insertions(+), 52 deletions(-) create mode 100644 packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts index 69bf43fb10ba1..e4e72b978cc9e 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts @@ -8,15 +8,8 @@ import { SavedObjectError } from '@kbn/core-saved-objects-common'; -/** @public */ -export interface SavedObjectsBulkDeleteObject { - type: string; - id: string; -} - /** @public */ export interface SavedObjectsBulkDeleteOptions { - namespace?: string; force?: boolean; } @@ -25,7 +18,7 @@ export interface SavedObjectsBulkDeleteItemResponse { id: string; type: string; success: boolean; - error?: SavedObjectError | Error; + error?: SavedObjectError; } /** @public */ diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts index 11f977438b873..95ae5f16e722d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts @@ -20,7 +20,6 @@ export type { export type { ResolvedSimpleSavedObject } from './resolve'; export type { SavedObjectsUpdateOptions } from './update'; export type { - SavedObjectsBulkDeleteObject, SavedObjectsBulkDeleteOptions, SavedObjectsBulkDeleteItemResponse, SavedObjectsBulkDeleteResponse, diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts index 372a4fdc95113..5eb09e12a69ab 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/index.ts @@ -22,7 +22,6 @@ export type { SavedObjectsBulkUpdateOptions, SavedObjectsBulkResolveResponse, SavedObjectsBulkCreateObject, - SavedObjectsBulkDeleteObject, SavedObjectsBulkDeleteOptions, SavedObjectsBulkDeleteItemResponse, SavedObjectsBulkDeleteResponse, diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts index 2ec370c9777e9..d222770a8579d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/saved_objects_client.ts @@ -19,12 +19,10 @@ import type { SavedObjectsFindOptions, SavedObjectsUpdateOptions, SavedObjectsDeleteOptions, -} from './apis'; -import { - SavedObjectsBulkDeleteObject, - SavedObjectsBulkDeleteOptions, SavedObjectsBulkDeleteResponse, -} from './apis/bulk_delete'; + SavedObjectsBulkDeleteOptions, +} from './apis'; + import type { SimpleSavedObject } from './simple_saved_object'; /** @@ -64,7 +62,7 @@ export interface SavedObjectsClientContract { * @returns The bulk delete result for the saved objects for the given types and ids. */ bulkDelete( - objects: SavedObjectsBulkDeleteObject[], + objects: SavedObjectTypeIdTuple[], options?: SavedObjectsBulkDeleteOptions ): Promise; diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index cad2c046c4d7d..53603131dcef0 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -9,7 +9,6 @@ import { omit, isObject } from 'lodash'; import type { Payload } from '@hapi/boom'; import * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; -import { BulkResponseItem, ErrorCause } from '@elastic/elasticsearch/lib/api/types'; import * as esKuery from '@kbn/es-query'; import type { Logger } from '@kbn/logging'; import type { ElasticsearchClient } from '@kbn/core-elasticsearch-server'; @@ -113,6 +112,13 @@ import { PreflightCheckForCreateObject, } from './preflight_check_for_create'; import { deleteLegacyUrlAliases } from './legacy_url_aliases'; +import type { + BulkDeleteParams, + ExpectedBulkDeleteResult, + BulkDeleteItemErrorResult, + NewBulkItemResponse, + BulkDeleteExpectedBulkGetResult, +} from './repository_bulk_delete_internal_types'; // BEWARE: The SavedObjectClient depends on the implementation details of the SavedObjectsRepository // so any breaking changes to this repository are considered breaking changes to the SavedObjectsClient. @@ -776,12 +782,8 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { const { refresh = DEFAULT_REFRESH_SETTING, force } = options; const namespace = normalizeNamespace(options.namespace); let bulkGetRequestIndexCounter = 0; - // TODO: move to internal types - type ExpectedBulkGetResult = Either< - { type: string; id: string; error: Payload }, - { type: string; id: string; version?: string; esRequestIndex?: number } - >; - const expectedBulkGetResults = objects.map((object) => { + + const expectedBulkGetResults = objects.map((object) => { const { type, id } = object; if (!this._allowedTypes.includes(type)) { return { @@ -827,27 +829,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { ) { throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(); } - // TODO: move to internal types - interface BulkDeleteParams { - delete: BulkDeleteParamsItem; - } - // TODO: move to internal types - interface BulkDeleteParamsItem { - if_seq_no?: number; - if_primary_term?: number; - _id: string; - _index: string; - } - // TODO: move to internal types - type ExpectedBulkDeleteResult = Either< - { type: string; id: string; error: Payload }, - { - type: string; - id: string; - namespaces: string[]; - esRequestIndex: number; - } - >; let bulkDeleteRequestIndexCounter = 0; const bulkDeleteParams: BulkDeleteParams[] = []; @@ -947,7 +928,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { : undefined; // extracted to ensure consistency in the error results returned - let errorResult: { success: boolean; type: string; id: string; error: Payload }; + let errorResult: BulkDeleteItemErrorResult; const savedObjects = await Promise.all( expectedBulkDeleteResults.map(async (expectedResult) => { @@ -957,10 +938,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } const { type, id, namespaces, esRequestIndex } = expectedResult.value; - // TODO: move to internal types - type NewBulkItemResponse = BulkResponseItem & { error: ErrorCause & { index: string } }; - - // we assume this wouldn't happen but is needed to circumvent type issues + // we assume this wouldn't happen but is needed to ensure type consistency if (bulkDeleteResponse === undefined) throw new Error(); const rawResponse = Object.values( diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts new file mode 100644 index 0000000000000..2504ea524b07e --- /dev/null +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts @@ -0,0 +1,57 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ +import type { Payload } from '@hapi/boom'; +import { + BulkOperationBase, + BulkResponseItem, + ErrorCause, +} from '@elastic/elasticsearch/lib/api/types'; +import { Either } from './internal_utils'; + +/** + * @internal + */ +export interface BulkDeleteParams { + delete: Omit; +} + +/** + * @internal + */ +export type ExpectedBulkDeleteResult = Either< + { type: string; id: string; error: Payload }, + { + type: string; + id: string; + namespaces: string[]; + esRequestIndex: number; + } +>; + +/** + * @internal + */ +export interface BulkDeleteItemErrorResult { + success: boolean; + type: string; + id: string; + error: Payload; +} + +/** + * @internal + */ +export type NewBulkItemResponse = BulkResponseItem & { error: ErrorCause & { index: string } }; + +/** + * @internal + */ +export type BulkDeleteExpectedBulkGetResult = Either< + { type: string; id: string; error: Payload }, + { type: string; id: string; version?: string; esRequestIndex?: number } +>; diff --git a/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.test.ts b/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.test.ts index 77391343cd033..6c2966ee9775f 100644 --- a/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.test.ts +++ b/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.test.ts @@ -308,6 +308,45 @@ describe('SavedObjectsClient', () => { }); }); + describe('#bulk_delete', () => { + const bulkDeleteDoc = { + id: 'AVwSwFxtcMV38qjDZoQg', + type: 'config', + }; + beforeEach(() => { + http.fetch.mockResolvedValue({ + statuses: [{ id: bulkDeleteDoc.id, type: bulkDeleteDoc.type, success: true }], + }); + }); + + test('deletes with an array of id, type and success status for deleted docs', async () => { + const response = savedObjectsClient.bulkDelete([bulkDeleteDoc]); + await expect(response).resolves.toHaveProperty('statuses'); + + const result = await response; + expect(result.statuses).toHaveLength(1); + expect(result.statuses[0]).toHaveProperty('success'); + }); + + test('makes HTTP call', async () => { + await savedObjectsClient.bulkDelete([bulkDeleteDoc]); + expect(http.fetch.mock.calls).toMatchInlineSnapshot(` + Array [ + Array [ + "/api/saved_objects/_bulk_delete", + Object { + "body": "[{\\"type\\":\\"config\\",\\"id\\":\\"AVwSwFxtcMV38qjDZoQg\\"}]", + "method": "POST", + "query": Object { + "force": false, + }, + }, + ], + ] + `); + }); + }); + describe('#update', () => { const attributes = { foo: 'Foo', bar: 'Bar' }; const options = { version: '1' }; diff --git a/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts b/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts index db2469d7663ad..dd2feed58123f 100644 --- a/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts +++ b/packages/core/saved-objects/core-saved-objects-browser-internal/src/saved_objects_client.ts @@ -15,6 +15,7 @@ import type { SavedObjectsClientContract as SavedObjectsApi, SavedObjectsFindResponse as SavedObjectsFindResponseServer, SavedObjectsResolveResponse, + SavedObjectsBulkDeleteOptions, } from '@kbn/core-saved-objects-api-server'; import type { SavedObjectsClientContract, @@ -29,7 +30,7 @@ import type { SavedObjectsBulkCreateOptions, SavedObjectsBulkCreateObject, SimpleSavedObject, - // SavedObjectsBulkDeleteObjects + SavedObjectsBulkDeleteResponse, } from '@kbn/core-saved-objects-api-browser'; import { SimpleSavedObjectImpl } from './simple_saved_object'; @@ -259,8 +260,8 @@ export class SavedObjectsClient implements SavedObjectsClientContract { public bulkDelete = async ( objects: SavedObjectTypeIdTuple[], - options?: { force?: boolean } - ): ReturnType => { + options?: SavedObjectsBulkDeleteOptions + ): Promise => { const filteredObjects = objects.map(({ type, id }) => ({ type, id })); const queryOptions = { force: !!options?.force }; const response = await this.performBulkDelete(filteredObjects, queryOptions); From a32022d50e11595d5f8d8f2a514edcd3b59577ef Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Sat, 27 Aug 2022 12:11:33 -0700 Subject: [PATCH 05/49] Adds core jest integration tests for bulkDelete --- .../index.ts | 1 + .../saved_objects/routes/bulk_delete.test.ts | 95 +++++++++++++++++++ .../service/lib/repository_with_proxy.test.ts | 40 ++++++++ .../lib/repository_with_proxy_utils.ts | 3 +- 4 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts diff --git a/packages/core/saved-objects/core-saved-objects-server-internal/index.ts b/packages/core/saved-objects/core-saved-objects-server-internal/index.ts index caeb029e037f7..f7d6fa7918031 100644 --- a/packages/core/saved-objects/core-saved-objects-server-internal/index.ts +++ b/packages/core/saved-objects/core-saved-objects-server-internal/index.ts @@ -22,6 +22,7 @@ export { registerBulkCreateRoute } from './src/routes/bulk_create'; export { registerBulkGetRoute } from './src/routes/bulk_get'; export { registerBulkResolveRoute } from './src/routes/bulk_resolve'; export { registerBulkUpdateRoute } from './src/routes/bulk_update'; +export { registerBulkDeleteRoute } from './src/routes/bulk_delete'; export { registerCreateRoute } from './src/routes/create'; export { registerDeleteRoute } from './src/routes/delete'; export { registerExportRoute } from './src/routes/export'; diff --git a/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts b/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts new file mode 100644 index 0000000000000..1e3569e06f577 --- /dev/null +++ b/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts @@ -0,0 +1,95 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import supertest from 'supertest'; +import { savedObjectsClientMock } from '../../../mocks'; +import { CoreUsageStatsClient } from '../../../core_usage_data'; +import { coreUsageStatsClientMock } from '../../../core_usage_data/core_usage_stats_client.mock'; +import { coreUsageDataServiceMock } from '../../../core_usage_data/core_usage_data_service.mock'; +import { setupServer } from './test_utils'; +import { + registerBulkDeleteRoute, + type InternalSavedObjectsRequestHandlerContext, +} from '@kbn/core-saved-objects-server-internal'; + +type SetupServerReturn = Awaited>; + +describe('POST /api/saved_objects/_bulk_delete', () => { + let server: SetupServerReturn['server']; + let httpSetup: SetupServerReturn['httpSetup']; + let handlerContext: SetupServerReturn['handlerContext']; + let savedObjectsClient: ReturnType; + let coreUsageStatsClient: jest.Mocked; + + beforeEach(async () => { + ({ server, httpSetup, handlerContext } = await setupServer()); + savedObjectsClient = handlerContext.savedObjects.client; + + savedObjectsClient.bulkDelete.mockResolvedValue({ + statuses: [], + }); + const router = + httpSetup.createRouter('/api/saved_objects/'); + coreUsageStatsClient = coreUsageStatsClientMock.create(); + coreUsageStatsClient.incrementSavedObjectsBulkDelete.mockRejectedValue(new Error('Oh no!')); // intentionally throw this error, which is swallowed, so we can assert that the operation does not fail + const coreUsageData = coreUsageDataServiceMock.createSetupContract(coreUsageStatsClient); + registerBulkDeleteRoute(router, { coreUsageData }); + + await server.start(); + }); + + afterEach(async () => { + await server.stop(); + }); + + it('formats successful response and records usage stats', async () => { + const clientResponse = { + statuses: [ + { + id: 'abc123', + type: 'index-pattern', + success: true, + }, + ], + }; + savedObjectsClient.bulkDelete.mockImplementation(() => Promise.resolve(clientResponse)); + + const result = await supertest(httpSetup.server.listener) + .post('/api/saved_objects/_bulk_delete') + .send([ + { + id: 'abc123', + type: 'index-pattern', + }, + ]) + .expect(200); + + expect(result.body).toEqual(clientResponse); + expect(coreUsageStatsClient.incrementSavedObjectsBulkDelete).toHaveBeenCalledWith({ + request: expect.anything(), + }); + }); + + it('calls upon savedObjectClient.bulkDelete', async () => { + const docs = [ + { + id: 'abc123', + type: 'index-pattern', + }, + ]; + + await supertest(httpSetup.server.listener) + .post('/api/saved_objects/_bulk_delete') + .send(docs) + .query({ force: true }) + .expect(200); + + expect(savedObjectsClient.bulkDelete).toHaveBeenCalledTimes(1); + expect(savedObjectsClient.bulkDelete).toHaveBeenCalledWith(docs, { force: true }); + }); +}); diff --git a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts index 7581e5f3639a2..6c77d8fd7b64a 100644 --- a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts +++ b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts @@ -54,6 +54,17 @@ const registerSOTypes = (setup: InternalCoreSetup) => { }, namespaceType: 'single', }); + setup.savedObjects.registerType({ + name: 'my_bulk_delete_type', + hidden: false, + mappings: { + dynamic: false, + properties: { + title: { type: 'text' }, + }, + }, + namespaceType: 'single', + }); }; describe('404s from proxies', () => { @@ -123,7 +134,9 @@ describe('404s from proxies', () => { describe('requests when a proxy relays request/responses with the correct product header', () => { let repository: ISavedObjectsRepository; let myOtherType: SavedObject; + let myBulkDeleteType: SavedObject; const myOtherTypeDocs: SavedObject[] = []; + const myBulkDeleteTypeDocs: SavedObject[] = []; beforeAll(async () => { repository = start.savedObjects.createInternalRepository(); @@ -145,6 +158,24 @@ describe('404s from proxies', () => { overwrite: true, namespace: 'default', }); + + myBulkDeleteType = await repository.create( + 'my_bulk_delete_type', + { title: 'my_bulk_delete_type1' }, + { overwrite: false, references: [] } + ); + for (let i = 1; i < 11; i++) { + myBulkDeleteTypeDocs.push({ + type: 'my_bulk_delete_type', + id: `myOtherTypeId${i}`, + attributes: { title: `MyOtherTypeTitle${i}` }, + references: [], + }); + } + await repository.bulkCreate(myBulkDeleteTypeDocs, { + overwrite: true, + namespace: 'default', + }); }); beforeEach(() => { @@ -237,6 +268,15 @@ describe('404s from proxies', () => { ); }); + it('handles `bulkDelete` requests that are successful when the proxy passes through the product header', async () => { + const docsToGet = myBulkDeleteTypeDocs; + const bulkDeleteDocs = docsToGet.map((doc) => ({ id: doc.id, type: 'my_bulk_delete_type' })); + + const docsFound = await repository.bulkDelete(bulkDeleteDocs, { force: false }); + expect(docsFound.statuses.length).toBeGreaterThan(0); + expect(docsFound.statuses[0].success).toBe(true); + }); + it('handles `bulkGet` requests that are successful when the proxy passes through the product header', async () => { const docsToGet = myOtherTypeDocs; const docsFound = await repository.bulkGet( diff --git a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy_utils.ts b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy_utils.ts index 6f1c2c523226c..251c7608b6299 100644 --- a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy_utils.ts +++ b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy_utils.ts @@ -122,7 +122,8 @@ export const declarePostMgetRoute = (hapiServer: Hapi.Server, hostname: string, if ( proxyInterrupt === 'bulkGetMyType' || proxyInterrupt === 'checkConficts' || - proxyInterrupt === 'internalBulkResolve' + proxyInterrupt === 'internalBulkResolve' || + proxyInterrupt === 'bulkDeleteMyDocs' ) { return proxyResponseHandler(h, hostname, port); } else { From a07d4d690ff7e34232bc49ed98adaf63f6dbb0bc Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Sat, 27 Aug 2022 16:54:48 -0700 Subject: [PATCH 06/49] Adds bulkDelete to secure_saved_objects_client_wrapper --- ...ecure_saved_objects_client_wrapper.test.ts | 49 +++++++++++++++---- .../secure_saved_objects_client_wrapper.ts | 44 +++++++++++++++-- 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index c2e65f8a4701a..a10d5e1d91277 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -11,6 +11,7 @@ import type { EcsEventOutcome, SavedObject, SavedObjectReferenceWithContext, + SavedObjectsBulkDeleteResponse, SavedObjectsErrorHelpers, SavedObjectsResolveResponse, SavedObjectsUpdateObjectsSpacesResponseObject, @@ -551,7 +552,7 @@ describe('#bulkUpdate', () => { test(`checks privileges for user, actions, and namespace`, async () => { const objects = [obj1, obj2]; const options = { namespace }; - const namespaces = [options.namespace]; // the bulkUpdate function always checks privileges as an array + const namespaces = [options.namespace]; // the bulkDelete function always checks privileges as an array?? await expectPrivilegeCheck(client.bulkUpdate, { objects, options }, namespaces); }); @@ -592,14 +593,44 @@ describe('#bulkUpdate', () => { }); describe('#bulkDelete', () => { - test.todo(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => {}); - test.todo(`throws decorated ForbiddenError when unauthorized`, async () => {}); - test.todo(`returns result of baseClient.bulkUpdate when authorized`, async () => {}); - test.todo(`checks privileges for user, actions, and namespace`, async () => {}); - test.todo(`checks privileges for object namespaces if present`, async () => {}); - test.todo(`filters namespaces that the user doesn't have access to`, async () => {}); - test.todo(`adds audit event when successful`, async () => {}); - test.todo(`adds audit event when not successful`, async () => {}); + const obj1 = Object.freeze({ type: 'foo', id: 'foo-id' }); + const obj2 = Object.freeze({ type: 'bar', id: 'bar-id' }); + const namespace = 'some-ns'; + + test(`throws decorated GeneralError when hasPrivileges rejects promise`, async () => { + const objects = [obj1]; + await expectGeneralError(client.bulkDelete, { objects }); + }); + + test(`throws decorated ForbiddenError when unauthorized`, async () => { + const objects = [obj1, obj2]; + const options = { namespace }; + await expectForbiddenError(client.bulkDelete, { objects, options }); + }); + + test(`returns result of baseClient.bulkDelete when authorized`, async () => { + const apiCallReturnValue = { + statuses: [obj1, obj2].map((obj) => { + return { ...obj, success: true }; + }), + }; + clientOpts.baseClient.bulkDelete.mockReturnValue(apiCallReturnValue as any); + + const objects = [obj1, obj2]; + const options = { namespace }; + const result = await expectSuccess(client.bulkDelete, { objects, options }); + expect(result).toEqual(apiCallReturnValue); + }); + + test(`checks privileges for user, actions, and namespace`, async () => { + const objects = [obj1, obj2]; + const options = { namespace }; + await expectPrivilegeCheck(client.bulkDelete, { objects, options }, namespace); + }); + // test.todo(`checks privileges for object namespaces if present`, async () => {}); + // test.todo(`filters namespaces that the user doesn't have access to`, async () => {}); + // test.todo(`adds audit event when successful`, async () => {}); + // test.todo(`adds audit event when not successful`, async () => {}); }); describe('#checkConflicts', () => { diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index b87fc005a63b9..649ddcd950b49 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -227,12 +227,50 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra return await this.baseClient.delete(type, id, options); } - // TODO: Implement public async bulkDelete( objects: SavedObjectsBulkDeleteObject[], - options?: SavedObjectsBulkDeleteOptions + options: SavedObjectsBulkDeleteOptions ): Promise { - throw new Error('Method not implemented.'); + try { + const args = { objects, options }; + await this.legacyEnsureAuthorized( + this.getUniqueObjectTypes(objects), + 'bulk_delete', + options?.namespace, + { + args, + } + ); + } catch (error) { + objects.forEach(({ type, id }) => + this.auditLogger.log( + savedObjectEvent({ + action: SavedObjectAction.DELETE, + savedObject: { type, id }, + error, + }) + ) + ); + throw error; + } + const response = await this.baseClient.bulkDelete(objects, options); + + response.statuses.forEach(({ id, type, success, error }) => { + if (!error) { + const auditEventOutcome = success === true ? 'success' : 'failure'; + this.auditLogger.log( + savedObjectEvent({ + action: SavedObjectAction.DELETE, + savedObject: { type, id }, + outcome: auditEventOutcome, + error: error ? error : undefined, + }) + ); + } + }); + // the response only contains saved objects' type and id and there's no direct object namespace to delete. + // However, the id might contain a reference to a space and we may need to redact that. + return response; } public async find(options: SavedObjectsFindOptions) { From 54f5bcbc80739590cdfae993ea66dea4d7a1a5a9 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Sun, 28 Aug 2022 13:55:24 -0700 Subject: [PATCH 07/49] adds bulkDelete to secure saved objects client, updates client unit tests --- ...ecure_saved_objects_client_wrapper.test.ts | 27 ++++++++++++++++--- .../secure_saved_objects_client_wrapper.ts | 7 +++-- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index a10d5e1d91277..a88eb1da728ed 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -627,10 +627,29 @@ describe('#bulkDelete', () => { const options = { namespace }; await expectPrivilegeCheck(client.bulkDelete, { objects, options }, namespace); }); - // test.todo(`checks privileges for object namespaces if present`, async () => {}); - // test.todo(`filters namespaces that the user doesn't have access to`, async () => {}); - // test.todo(`adds audit event when successful`, async () => {}); - // test.todo(`adds audit event when not successful`, async () => {}); + + test(`adds audit event when successful`, async () => { + const apiCallReturnValue = { + statuses: [obj1, obj2].map((obj) => { + return { ...obj, success: true }; + }), + }; + clientOpts.baseClient.bulkDelete.mockReturnValue(apiCallReturnValue as any); + + const objects = [obj1, obj2]; + const options = { namespace }; + await expectSuccess(client.bulkDelete, { objects, options }); + expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2); + expectAuditEvent('saved_object_delete', 'success', { type: obj1.type, id: obj1.id }); + expectAuditEvent('saved_object_delete', 'success', { type: obj2.type, id: obj2.id }); + }); + test(`adds audit event when not successful`, async () => { + clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error()); + await expect(() => client.bulkDelete([obj1, obj2], { namespace })).rejects.toThrow(); + expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2); + expectAuditEvent('saved_object_delete', 'failure', { type: obj1.type, id: obj1.id }); + expectAuditEvent('saved_object_delete', 'failure', { type: obj2.type, id: obj2.id }); + }); }); describe('#checkConflicts', () => { diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index 649ddcd950b49..205de9b9b24b9 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -231,6 +231,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra objects: SavedObjectsBulkDeleteObject[], options: SavedObjectsBulkDeleteOptions ): Promise { + // check if the user is authorized to bulk_delete the object types, exit early if not allowed to try { const args = { objects, options }; await this.legacyEnsureAuthorized( @@ -253,9 +254,10 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra ); throw error; } + // call the core saved objects client const response = await this.baseClient.bulkDelete(objects, options); - - response.statuses.forEach(({ id, type, success, error }) => { + // log the result of each object's delete outcome + response?.statuses.forEach(({ id, type, success, error }) => { if (!error) { const auditEventOutcome = success === true ? 'success' : 'failure'; this.auditLogger.log( @@ -270,6 +272,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra }); // the response only contains saved objects' type and id and there's no direct object namespace to delete. // However, the id might contain a reference to a space and we may need to redact that. + // FOLLOW UP: figure out how to remove the saved object space from the id return response; } From e1d538379d6c31f979dc474b822b378a09f98718 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Sun, 28 Aug 2022 14:39:46 -0700 Subject: [PATCH 08/49] Adds bulkDelete to encrypted saved objects wrapper --- ...ypted_saved_objects_client_wrapper.test.ts | 38 +++++++++++++++++++ .../encrypted_saved_objects_client_wrapper.ts | 9 +++++ 2 files changed, 47 insertions(+) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 75dd3fdfe8dce..ab8b03840c819 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -870,6 +870,44 @@ describe('#delete', () => { }); }); +describe('#bulkDelete', () => { + const obj1 = Object.freeze({ type: 'unknown-type', id: 'unknown-type-id-1' }); + const obj2 = Object.freeze({ type: 'unknown-type', id: 'unknown-type-id-2' }); + const namespace = 'some-ns'; + + it('redirects request to underlying base client if type is not registered', async () => { + await wrapper.bulkDelete([obj1, obj2], { namespace }); + expect(mockBaseClient.bulkDelete).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkDelete).toHaveBeenCalledWith([obj1, obj2], { namespace }); + }); + + it('redirects request to underlying base client if type is registered', async () => { + const knownObj1 = Object.freeze({ type: 'known-type', id: 'known-type-id-1' }); + const knownObj2 = Object.freeze({ type: 'known-type', id: 'known-type-id-2' }); + const options = { namespace: 'some-ns' }; + + await wrapper.bulkDelete([knownObj1, knownObj2], options); + + expect(mockBaseClient.bulkDelete).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkDelete).toHaveBeenCalledWith([knownObj1, knownObj2], { namespace }); + }); + + it('fails if base client fails', async () => { + const failureReason = new Error('Something bad happened...'); + mockBaseClient.bulkDelete.mockRejectedValue(failureReason); + + await expect(wrapper.bulkDelete([{ type: 'known-type', id: 'some-id' }])).rejects.toThrowError( + failureReason + ); + + expect(mockBaseClient.bulkDelete).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkDelete).toHaveBeenCalledWith( + [{ type: 'known-type', id: 'some-id' }], + undefined + ); + }); +}); + describe('#find', () => { it('redirects request to underlying base client and does not alter response if type is not registered', async () => { const mockedResponse = { diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 0b9c81cc33334..e2fcfd2a6ef25 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -10,6 +10,8 @@ import type { SavedObject, SavedObjectsBaseOptions, SavedObjectsBulkCreateObject, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, SavedObjectsBulkGetObject, SavedObjectsBulkResolveObject, SavedObjectsBulkResponse, @@ -166,6 +168,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return await this.options.baseClient.delete(type, id, options); } + public async bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options?: SavedObjectsBulkDeleteOptions + ) { + return await this.options.baseClient.bulkDelete(objects, options); + } + public async find(options: SavedObjectsFindOptions) { return await this.handleEncryptedAttributesInBulkResponse( await this.options.baseClient.find(options), From 4aa5a6c474f8afa865884d5389d8c1633da14f42 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 29 Aug 2022 16:10:33 -0700 Subject: [PATCH 09/49] Cleans up types --- .../saved_objects/service/lib/repository_with_proxy.test.ts | 6 ------ .../secure_saved_objects_client_wrapper.test.ts | 4 ++-- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts index 6c77d8fd7b64a..50698183ca667 100644 --- a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts +++ b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts @@ -134,7 +134,6 @@ describe('404s from proxies', () => { describe('requests when a proxy relays request/responses with the correct product header', () => { let repository: ISavedObjectsRepository; let myOtherType: SavedObject; - let myBulkDeleteType: SavedObject; const myOtherTypeDocs: SavedObject[] = []; const myBulkDeleteTypeDocs: SavedObject[] = []; @@ -159,11 +158,6 @@ describe('404s from proxies', () => { namespace: 'default', }); - myBulkDeleteType = await repository.create( - 'my_bulk_delete_type', - { title: 'my_bulk_delete_type1' }, - { overwrite: false, references: [] } - ); for (let i = 1; i < 11; i++) { myBulkDeleteTypeDocs.push({ type: 'my_bulk_delete_type', diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index a88eb1da728ed..59d9860b62253 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -11,7 +11,6 @@ import type { EcsEventOutcome, SavedObject, SavedObjectReferenceWithContext, - SavedObjectsBulkDeleteResponse, SavedObjectsErrorHelpers, SavedObjectsResolveResponse, SavedObjectsUpdateObjectsSpacesResponseObject, @@ -643,9 +642,10 @@ describe('#bulkDelete', () => { expectAuditEvent('saved_object_delete', 'success', { type: obj1.type, id: obj1.id }); expectAuditEvent('saved_object_delete', 'success', { type: obj2.type, id: obj2.id }); }); + test(`adds audit event when not successful`, async () => { clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockRejectedValue(new Error()); - await expect(() => client.bulkDelete([obj1, obj2], { namespace })).rejects.toThrow(); + await expect(() => client.bulkDelete([obj1, obj2], { namespace })).rejects.toThrow(); expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(2); expectAuditEvent('saved_object_delete', 'failure', { type: obj1.type, id: obj1.id }); expectAuditEvent('saved_object_delete', 'failure', { type: obj2.type, id: obj2.id }); From 95f144af0991949b677ba673770638a2cde4a4a0 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 29 Aug 2022 16:41:51 -0700 Subject: [PATCH 10/49] Adds api integration tests for saved objects bulk delete --- .../apis/saved_objects/bulk_delete.ts | 81 +++++++++++++++++++ .../apis/saved_objects/index.ts | 1 + 2 files changed, 82 insertions(+) create mode 100644 test/api_integration/apis/saved_objects/bulk_delete.ts diff --git a/test/api_integration/apis/saved_objects/bulk_delete.ts b/test/api_integration/apis/saved_objects/bulk_delete.ts new file mode 100644 index 0000000000000..6ea880b53d505 --- /dev/null +++ b/test/api_integration/apis/saved_objects/bulk_delete.ts @@ -0,0 +1,81 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import expect from '@kbn/expect'; +import { FtrProviderContext } from '../../ftr_provider_context'; + +export default function ({ getService }: FtrProviderContext) { + const supertest = getService('supertest'); + const kibanaServer = getService('kibanaServer'); + + describe('bulk_delete', () => { + before(async () => { + await kibanaServer.importExport.load( + 'test/api_integration/fixtures/kbn_archiver/saved_objects/basic.json' + ); + }); + after(async () => { + await kibanaServer.importExport.unload( + 'test/api_integration/fixtures/kbn_archiver/saved_objects/basic.json' + ); + }); + + it('should return 200 with individual responses when deleting many docs', async () => + await supertest + .post(`/api/saved_objects/_bulk_delete`) + .send([ + { + type: 'visualization', + id: 'dd7caf20-9efd-11e7-acb3-3dab96693fab', + }, + { + type: 'dashboard', + id: 'be3733a0-9efe-11e7-acb3-3dab96693fab', + }, + ]) + .expect(200) + .then((resp) => { + expect(resp.body).to.eql({ + statuses: [ + { + success: true, + type: 'visualization', + id: 'dd7caf20-9efd-11e7-acb3-3dab96693fab', + }, + { + success: true, + type: 'dashboard', + id: 'be3733a0-9efe-11e7-acb3-3dab96693fab', + }, + ], + }); + })); + + it('should return generic 404 when deleting an unknown doc', async () => + await supertest + .post(`/api/saved_objects/_bulk_delete`) + .send([{ type: 'dashboard', id: 'not-a-real-id' }]) + .expect(200) + .then((resp) => { + expect(resp.body).to.eql({ + statuses: [ + { + error: { + error: 'Not Found', + message: 'Saved object [dashboard/not-a-real-id] not found', + statusCode: 404, + }, + id: 'not-a-real-id', + type: 'dashboard', + success: false, + }, + ], + }); + })); + }); +} diff --git a/test/api_integration/apis/saved_objects/index.ts b/test/api_integration/apis/saved_objects/index.ts index 44ee3d8d7d76b..c981add4540b3 100644 --- a/test/api_integration/apis/saved_objects/index.ts +++ b/test/api_integration/apis/saved_objects/index.ts @@ -11,6 +11,7 @@ import { FtrProviderContext } from '../../ftr_provider_context'; export default function ({ loadTestFile }: FtrProviderContext) { describe('saved_objects', () => { loadTestFile(require.resolve('./bulk_create')); + loadTestFile(require.resolve('./bulk_delete')); loadTestFile(require.resolve('./bulk_get')); loadTestFile(require.resolve('./bulk_update')); loadTestFile(require.resolve('./create')); From 3cbfe86779af64e56736d70dadfb417a131fc5b4 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 29 Aug 2022 18:57:30 -0700 Subject: [PATCH 11/49] Updates core saved objects usage stats --- .../collectors/core/core_usage_collector.ts | 40 +++++++++++++++++++ .../server/collectors/core/core_usage_data.ts | 7 ++++ 2 files changed, 47 insertions(+) diff --git a/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_collector.ts b/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_collector.ts index ce8e27f318cfd..28a921dd20162 100644 --- a/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_collector.ts +++ b/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_collector.ts @@ -822,6 +822,46 @@ export function getCoreUsageCollector( 'How many times this API has been called by a non-Kibana client in a custom space.', }, }, + 'apiCalls.savedObjectsBulkDelete.total': { + type: 'long', + _meta: { description: 'How many times this API has been called.' }, + }, + 'apiCalls.savedObjectsBulkDelete.namespace.default.total': { + type: 'long', + _meta: { description: 'How many times this API has been called in the Default space.' }, + }, + 'apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.yes': { + type: 'long', + _meta: { + description: + 'How many times this API has been called by the Kibana client in the Default space.', + }, + }, + 'apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.no': { + type: 'long', + _meta: { + description: + 'How many times this API has been called by a non-Kibana client in the Default space.', + }, + }, + 'apiCalls.savedObjectsBulkDelete.namespace.custom.total': { + type: 'long', + _meta: { description: 'How many times this API has been called in a custom space.' }, + }, + 'apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.yes': { + type: 'long', + _meta: { + description: + 'How many times this API has been called by the Kibana client in a custom space.', + }, + }, + 'apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.no': { + type: 'long', + _meta: { + description: + 'How many times this API has been called by a non-Kibana client in a custom space.', + }, + }, // Saved Objects Management APIs 'apiCalls.savedObjectsImport.total': { type: 'long', diff --git a/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_data.ts b/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_data.ts index 36aac8ec0511a..2e5084f1b9ae5 100644 --- a/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_data.ts +++ b/src/plugins/kibana_usage_collection/server/collectors/core/core_usage_data.ts @@ -38,6 +38,13 @@ export interface CoreUsageStats { 'apiCalls.savedObjectsBulkResolve.namespace.custom.total'?: number; 'apiCalls.savedObjectsBulkResolve.namespace.custom.kibanaRequest.yes'?: number; 'apiCalls.savedObjectsBulkResolve.namespace.custom.kibanaRequest.no'?: number; + 'apiCalls.savedObjectsBulkDelete.total'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.default.total'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.yes'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.no'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.custom.total'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.yes'?: number; + 'apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.no'?: number; 'apiCalls.savedObjectsBulkUpdate.total'?: number; 'apiCalls.savedObjectsBulkUpdate.namespace.default.total'?: number; 'apiCalls.savedObjectsBulkUpdate.namespace.default.kibanaRequest.yes'?: number; From 1e03ff7a5608355746a222e3715cc76c80627dda Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 29 Aug 2022 18:59:32 -0700 Subject: [PATCH 12/49] updates telemetry schema --- src/plugins/telemetry/schema/oss_plugins.json | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/plugins/telemetry/schema/oss_plugins.json b/src/plugins/telemetry/schema/oss_plugins.json index e59b192442133..f176420509cdc 100644 --- a/src/plugins/telemetry/schema/oss_plugins.json +++ b/src/plugins/telemetry/schema/oss_plugins.json @@ -7216,6 +7216,48 @@ "description": "How many times this API has been called by a non-Kibana client in a custom space." } }, + "apiCalls.savedObjectsBulkDelete.total": { + "type": "long", + "_meta": { + "description": "How many times this API has been called." + } + }, + "apiCalls.savedObjectsBulkDelete.namespace.default.total": { + "type": "long", + "_meta": { + "description": "How many times this API has been called in the Default space." + } + }, + "apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.yes": { + "type": "long", + "_meta": { + "description": "How many times this API has been called by the Kibana client in the Default space." + } + }, + "apiCalls.savedObjectsBulkDelete.namespace.default.kibanaRequest.no": { + "type": "long", + "_meta": { + "description": "How many times this API has been called by a non-Kibana client in the Default space." + } + }, + "apiCalls.savedObjectsBulkDelete.namespace.custom.total": { + "type": "long", + "_meta": { + "description": "How many times this API has been called in a custom space." + } + }, + "apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.yes": { + "type": "long", + "_meta": { + "description": "How many times this API has been called by the Kibana client in a custom space." + } + }, + "apiCalls.savedObjectsBulkDelete.namespace.custom.kibanaRequest.no": { + "type": "long", + "_meta": { + "description": "How many times this API has been called by a non-Kibana client in a custom space." + } + }, "apiCalls.savedObjectsImport.total": { "type": "long", "_meta": { From 941e5684d6a81b92b2cf63ffc586f6f130e9e8b0 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 31 Aug 2022 12:46:31 -0700 Subject: [PATCH 13/49] Fix incorrect conditional to apply force flag --- .../src/lib/repository.ts | 93 ++++++++++--------- 1 file changed, 48 insertions(+), 45 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 53603131dcef0..49765ddf81df6 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -783,48 +783,53 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { const namespace = normalizeNamespace(options.namespace); let bulkGetRequestIndexCounter = 0; - const expectedBulkGetResults = objects.map((object) => { - const { type, id } = object; - if (!this._allowedTypes.includes(type)) { + const expectedBulkGetAllDocsWithNamespaceChecksFlag = + objects.map((object) => { + const { type, id } = object; + if (!this._allowedTypes.includes(type)) { + return { + tag: 'Left', + value: { + id, + type, + error: errorContent(SavedObjectsErrorHelpers.createUnsupportedTypeError(type)), + }, + }; + } + const requiresNamespacesCheck = this._registry.isMultiNamespace(type); return { - tag: 'Left', + tag: 'Right', value: { - id, type, - error: errorContent(SavedObjectsErrorHelpers.createUnsupportedTypeError(type)), + id, + ...(requiresNamespacesCheck && { esRequestIndex: bulkGetRequestIndexCounter++ }), }, }; - } - const requiresNamespacesCheck = this._registry.isMultiNamespace(type); - return { - tag: 'Right', - value: { - type, - id, - ...(requiresNamespacesCheck && { esRequestIndex: bulkGetRequestIndexCounter++ }), - }, - }; - }); + }); - // get multi-namespace saved objects. - const bulkGetDocs = expectedBulkGetResults + // get multi-namespace saved objects only to minimise the number of docs to fetch for the preflight multinamespace objects. + // Create params for an mget request only from docs that need a multi-namespace preflight check + const bulkGetMultiNamespaceDocs = expectedBulkGetAllDocsWithNamespaceChecksFlag .filter(isRight) - .filter(({ value }) => value.esRequestIndex !== undefined) + .filter(({ value }) => value.esRequestIndex !== undefined) // only get docs that need multinamespace checks, I don't think we want to do this filtering .map(({ value: { type, id } }) => ({ _id: this._serializer.generateRawId(namespace, type, id), _index: this.getIndexForType(type), _source: ['type', 'namespaces'], })); - const bulkGetResponse = bulkGetDocs.length - ? await this.client.mget({ body: { docs: bulkGetDocs } }, { ignore: [404], meta: true }) + const bulkGetMultiNamespaceDocsResponse = bulkGetMultiNamespaceDocs.length + ? await this.client.mget( + { body: { docs: bulkGetMultiNamespaceDocs } }, + { ignore: [404], meta: true } + ) : undefined; // fail fast if we can't verify a 404 response is from Elasticsearch if ( - bulkGetResponse && + bulkGetMultiNamespaceDocsResponse && isNotFoundFromUnsupportedServer({ - statusCode: bulkGetResponse.statusCode, - headers: bulkGetResponse.headers, + statusCode: bulkGetMultiNamespaceDocsResponse.statusCode, + headers: bulkGetMultiNamespaceDocsResponse.headers, }) ) { throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(); @@ -833,8 +838,8 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { let bulkDeleteRequestIndexCounter = 0; const bulkDeleteParams: BulkDeleteParams[] = []; - const expectedBulkDeleteResults = await Promise.all( - expectedBulkGetResults.map>( + const expectedBulkDeleteMultiNamespaceDocsResults = await Promise.all( + expectedBulkGetAllDocsWithNamespaceChecksFlag.map>( async (expectedBulkGetResult) => { if (isLeft(expectedBulkGetResult)) { return { ...expectedBulkGetResult }; @@ -845,15 +850,22 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { let versionProperties; if (esRequestIndex !== undefined) { - const indexFound = bulkGetResponse?.statusCode !== 404; + const indexFound = bulkGetMultiNamespaceDocsResponse?.statusCode !== 404; const actualResult = indexFound - ? bulkGetResponse?.body.docs[esRequestIndex] + ? bulkGetMultiNamespaceDocsResponse?.body.docs[esRequestIndex] : undefined; const docFound = indexFound && isMgetDoc(actualResult) && actualResult.found; + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + namespaces = actualResult!._source.namespaces ?? [ + SavedObjectsUtils.namespaceIdToString(namespace), + ]; - if (!docFound) { + versionProperties = getExpectedVersionProperties(version); + // return an error if the doc isnn't found at all or the doc doesn't exist in the namespaces + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + if (!docFound || !this.rawDocExistsInNamespaces(actualResult, namespaces)) { return { tag: 'Left', value: { @@ -865,12 +877,8 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { }, }; } - if ( - // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - !this.rawDocExistsInNamespace(actualResult, namespace) && - !force - ) { - // the document exists but not in the namespace for this client. Deleting the doc is still possible but one needs to force the action. + // the document exists but is multinamespace and there are more than one namespaces present and can only be deleted by force. + if (!force && (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING))) { return { tag: 'Left', value: { @@ -879,19 +887,14 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { type, error: errorContent( SavedObjectsErrorHelpers.createBadRequestError( - `Unable to delete saved object id: ${id}, type: ${type} that exists in multiple namespaces, use the "force" option to delete all saved objects` + `Unable to delete saved object that exists in multiple namespaces, use the "force" option to delete it anyway` ) ), }, }; } - // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - namespaces = actualResult!._source.namespaces ?? [ - // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - SavedObjectsUtils.namespaceIdToString(actualResult!._source.namespace), - ]; - versionProperties = getExpectedVersionProperties(version); } else { + // checking if the object is single namespace type should be redundant, since it wouldn't have an `esRequestIndex` but we do the check to be safe if (this._registry.isSingleNamespace(type)) { namespaces = [SavedObjectsUtils.namespaceIdToString(namespace)]; } @@ -902,7 +905,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { type, id, namespaces, - esRequestIndex: bulkDeleteRequestIndexCounter++, + esRequestIndex: bulkDeleteRequestIndexCounter++, // i think this is wrong }; bulkDeleteParams.push({ @@ -931,7 +934,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { let errorResult: BulkDeleteItemErrorResult; const savedObjects = await Promise.all( - expectedBulkDeleteResults.map(async (expectedResult) => { + expectedBulkDeleteMultiNamespaceDocsResults.map(async (expectedResult) => { if (isLeft(expectedResult)) { errorResult = { ...expectedResult.value, success: false }; return errorResult; From df876c9b4c8a02e931072ee8e91cdb8de6107f74 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 31 Aug 2022 14:15:31 -0700 Subject: [PATCH 14/49] Fix unknown documents deletion --- .../src/lib/repository.ts | 28 ++++++++++++++----- .../saved_objects/routes/bulk_delete.test.ts | 2 +- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 49765ddf81df6..c4c03c622f664 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -857,15 +857,23 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { : undefined; const docFound = indexFound && isMgetDoc(actualResult) && actualResult.found; - // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - namespaces = actualResult!._source.namespaces ?? [ - SavedObjectsUtils.namespaceIdToString(namespace), - ]; - versionProperties = getExpectedVersionProperties(version); // return an error if the doc isnn't found at all or the doc doesn't exist in the namespaces + if (!docFound) { + return { + tag: 'Left', + value: { + id, + type, + error: errorContent( + SavedObjectsErrorHelpers.createGenericNotFoundError(type, id) + ), + }, + }; + } + // the following check should be redundant since we're retrieving the docs from elasticsearch but we check anyway // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - if (!docFound || !this.rawDocExistsInNamespaces(actualResult, namespaces)) { + if (docFound && !this.rawDocExistsInNamespace(actualResult, namespace)) { return { tag: 'Left', value: { @@ -877,6 +885,12 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { }, }; } + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + namespaces = actualResult!._source.namespaces ?? [ + SavedObjectsUtils.namespaceIdToString(namespace), + ]; + + versionProperties = getExpectedVersionProperties(version); // the document exists but is multinamespace and there are more than one namespaces present and can only be deleted by force. if (!force && (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING))) { return { @@ -905,7 +919,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { type, id, namespaces, - esRequestIndex: bulkDeleteRequestIndexCounter++, // i think this is wrong + esRequestIndex: bulkDeleteRequestIndexCounter++, }; bulkDeleteParams.push({ diff --git a/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts b/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts index 1e3569e06f577..5eb8b9a874a19 100644 --- a/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts +++ b/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts @@ -75,7 +75,7 @@ describe('POST /api/saved_objects/_bulk_delete', () => { }); }); - it('calls upon savedObjectClient.bulkDelete', async () => { + it('calls upon savedObjectClient.bulkDelete with query options', async () => { const docs = [ { id: 'abc123', From 4ccd6ca2ebc816bcbd6f63a334c3e6755aadf49d Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 31 Aug 2022 15:13:01 -0700 Subject: [PATCH 15/49] renames esRequestIndex to help avoid confusion --- .../src/lib/repository.ts | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index c4c03c622f664..6d8ccd94a476b 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -844,16 +844,21 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { if (isLeft(expectedBulkGetResult)) { return { ...expectedBulkGetResult }; } - const { esRequestIndex, id, type, version } = expectedBulkGetResult.value; + const { + esRequestIndex: esBulkGetRequestIndex, + id, + type, + version, + } = expectedBulkGetResult.value; let namespaces; let versionProperties; - if (esRequestIndex !== undefined) { + if (esBulkGetRequestIndex !== undefined) { const indexFound = bulkGetMultiNamespaceDocsResponse?.statusCode !== 404; const actualResult = indexFound - ? bulkGetMultiNamespaceDocsResponse?.body.docs[esRequestIndex] + ? bulkGetMultiNamespaceDocsResponse?.body.docs[esBulkGetRequestIndex] : undefined; const docFound = indexFound && isMgetDoc(actualResult) && actualResult.found; @@ -953,13 +958,18 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { errorResult = { ...expectedResult.value, success: false }; return errorResult; } - const { type, id, namespaces, esRequestIndex } = expectedResult.value; + const { + type, + id, + namespaces, + esRequestIndex: esBulkDeleteRequestIndex, + } = expectedResult.value; // we assume this wouldn't happen but is needed to ensure type consistency if (bulkDeleteResponse === undefined) throw new Error(); const rawResponse = Object.values( - bulkDeleteResponse.items[esRequestIndex] + bulkDeleteResponse.items[esBulkDeleteRequestIndex] )[0] as NewBulkItemResponse; const error = getBulkOperationError(type, id, rawResponse); From 336634aa2cf34abfc83a14f008d7e01f1cd1d5ad Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 31 Aug 2022 15:56:02 -0700 Subject: [PATCH 16/49] Adds a bulkDelete test assertion for mix of valid and invalid documents --- .../src/lib/repository.ts | 7 ++-- .../apis/saved_objects/bulk_delete.ts | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 6d8ccd94a476b..ee3db3627ba27 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -944,7 +944,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { ? await this.client.bulk({ refresh, body: bulkDeleteParams, - _source_includes: ['originId'], require_alias: true, }) : undefined; @@ -966,7 +965,11 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } = expectedResult.value; // we assume this wouldn't happen but is needed to ensure type consistency - if (bulkDeleteResponse === undefined) throw new Error(); + if (bulkDeleteResponse === undefined) { + throw new Error( + `Unexpected error in bulkDelete saved objects: bulkDeleteResponse is undefined` + ); + } const rawResponse = Object.values( bulkDeleteResponse.items[esBulkDeleteRequestIndex] diff --git a/test/api_integration/apis/saved_objects/bulk_delete.ts b/test/api_integration/apis/saved_objects/bulk_delete.ts index 6ea880b53d505..5b5292b97ddde 100644 --- a/test/api_integration/apis/saved_objects/bulk_delete.ts +++ b/test/api_integration/apis/saved_objects/bulk_delete.ts @@ -77,5 +77,38 @@ export default function ({ getService }: FtrProviderContext) { ], }); })); + + it('should return the result of deleting valid and invalid objects in the same request', async () => + await supertest + .post(`/api/saved_objects/_bulk_delete`) + .send([ + { type: 'visualization', id: 'not-a-real-vis-id' }, + { + type: 'index-pattern', + id: '91200a00-9efd-11e7-acb3-3dab96693fab', + }, + ]) + .expect(200) + .then((resp) => { + expect(resp.body).to.eql({ + statuses: [ + { + error: { + error: 'Not Found', + message: 'Saved object [visualization/not-a-real-vis-id] not found', + statusCode: 404, + }, + id: 'not-a-real-vis-id', + type: 'visualization', + success: false, + }, + { + success: true, + type: 'index-pattern', + id: '91200a00-9efd-11e7-acb3-3dab96693fab', + }, + ], + }); + })); }); } From 40c54540017f526831635f184cbbb71cede5d239 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 31 Aug 2022 16:54:44 -0700 Subject: [PATCH 17/49] Handles nits from draft review --- .../core-saved-objects-api-browser/index.ts | 2 +- .../src/apis/bulk_delete.ts | 4 +-- .../src/apis/index.ts | 2 +- .../src/apis/bulk_delete.ts | 2 +- .../src/saved_objects_repository.ts | 2 +- ...ecure_saved_objects_client_wrapper.test.ts | 2 +- .../secure_saved_objects_client_wrapper.ts | 27 +++++++------------ .../spaces_saved_objects_client.test.ts | 9 +++++-- .../spaces_saved_objects_client.ts | 5 ++-- 9 files changed, 27 insertions(+), 28 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/index.ts b/packages/core/saved-objects/core-saved-objects-api-browser/index.ts index db719396f57f1..e78c56d76556c 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/index.ts @@ -23,6 +23,6 @@ export type { SavedObjectsBulkResolveResponse, SavedObjectsBulkCreateObject, SavedObjectsBulkDeleteOptions, - SavedObjectsBulkDeleteItemResponse, + SavedObjectsBulkDeleteResponseItem, SavedObjectsBulkDeleteResponse, } from './src/apis'; diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts index e4e72b978cc9e..1e4b5d2268dea 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_delete.ts @@ -14,7 +14,7 @@ export interface SavedObjectsBulkDeleteOptions { } /** @public */ -export interface SavedObjectsBulkDeleteItemResponse { +export interface SavedObjectsBulkDeleteResponseItem { id: string; type: string; success: boolean; @@ -23,5 +23,5 @@ export interface SavedObjectsBulkDeleteItemResponse { /** @public */ export interface SavedObjectsBulkDeleteResponse { - statuses: SavedObjectsBulkDeleteItemResponse[]; + statuses: SavedObjectsBulkDeleteResponseItem[]; } diff --git a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts index 95ae5f16e722d..afee0a01494e1 100644 --- a/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-browser/src/apis/index.ts @@ -21,6 +21,6 @@ export type { ResolvedSimpleSavedObject } from './resolve'; export type { SavedObjectsUpdateOptions } from './update'; export type { SavedObjectsBulkDeleteOptions, - SavedObjectsBulkDeleteItemResponse, + SavedObjectsBulkDeleteResponseItem, SavedObjectsBulkDeleteResponse, } from './bulk_delete'; diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts index 053de53a8b815..7279703c1cf5d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -import { SavedObjectError } from '@kbn/core-saved-objects-common'; +import type { SavedObjectError } from '@kbn/core-saved-objects-common'; import type { MutatingOperationRefreshSetting, SavedObjectsBaseOptions } from './base'; /** diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts index d789acb7621c9..9f5d6e5f80b65 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts @@ -116,7 +116,7 @@ export interface ISavedObjectsRepository { */ bulkDelete( objects: SavedObjectsBulkDeleteObject[], - options: SavedObjectsBulkDeleteOptions + options?: SavedObjectsBulkDeleteOptions ): Promise; /** diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 59d9860b62253..26a0285658e3a 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -551,7 +551,7 @@ describe('#bulkUpdate', () => { test(`checks privileges for user, actions, and namespace`, async () => { const objects = [obj1, obj2]; const options = { namespace }; - const namespaces = [options.namespace]; // the bulkDelete function always checks privileges as an array?? + const namespaces = [options.namespace]; await expectPrivilegeCheck(client.bulkUpdate, { objects, options }, namespaces); }); diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index 205de9b9b24b9..b8e253b7f3160 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -231,7 +231,6 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra objects: SavedObjectsBulkDeleteObject[], options: SavedObjectsBulkDeleteOptions ): Promise { - // check if the user is authorized to bulk_delete the object types, exit early if not allowed to try { const args = { objects, options }; await this.legacyEnsureAuthorized( @@ -254,25 +253,19 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra ); throw error; } - // call the core saved objects client const response = await this.baseClient.bulkDelete(objects, options); - // log the result of each object's delete outcome response?.statuses.forEach(({ id, type, success, error }) => { - if (!error) { - const auditEventOutcome = success === true ? 'success' : 'failure'; - this.auditLogger.log( - savedObjectEvent({ - action: SavedObjectAction.DELETE, - savedObject: { type, id }, - outcome: auditEventOutcome, - error: error ? error : undefined, - }) - ); - } + const auditEventOutcome = success === true ? 'success' : 'failure'; + const auditEventOutcomeError = error ? (error as unknown as Error) : undefined; + this.auditLogger.log( + savedObjectEvent({ + action: SavedObjectAction.DELETE, + savedObject: { type, id }, + outcome: auditEventOutcome, + error: auditEventOutcomeError, + }) + ); }); - // the response only contains saved objects' type and id and there's no direct object namespace to delete. - // However, the id might contain a reference to a space and we may need to redact that. - // FOLLOW UP: figure out how to remove the saved object space from the id return response; } diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index bcf3e3deb3be5..70a8628246b71 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -601,7 +601,9 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; const expectedReturnValue = { statuses: [{ id: 'id', type: 'type', success: true }] }; baseClient.bulkDelete.mockReturnValue(Promise.resolve(expectedReturnValue)); - const actualReturnValue = await client.bulkDelete([{ id: 'id', type: 'foo' }]); + const actualReturnValue = await client.bulkDelete([{ id: 'id', type: 'foo' }], { + force: true, + }); expect(actualReturnValue).toBe(expectedReturnValue); expect(baseClient.bulkDelete).toHaveBeenCalledWith( @@ -611,7 +613,10 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; type: 'foo', }, ], - { namespace: currentSpace.expectedNamespace } + { + namespace: currentSpace.expectedNamespace, + force: true, + } ); }); }); diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index ece3da264d939..30935a675c0f6 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -13,6 +13,7 @@ import type { SavedObjectsBaseOptions, SavedObjectsBulkCreateObject, SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, SavedObjectsBulkGetObject, SavedObjectsBulkResolveObject, SavedObjectsBulkUpdateObject, @@ -142,7 +143,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { async bulkDelete( objects: SavedObjectsBulkDeleteObject[] = [], - options: SavedObjectsBaseOptions = {} + options: SavedObjectsBulkDeleteOptions = {} ) { throwErrorIfNamespaceSpecified(options); return await this.client.bulkDelete(objects, { @@ -285,7 +286,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { async bulkUpdate( objects: Array> = [], - options: SavedObjectsBaseOptions = {} + options: SavedObjectsBulkDeleteOptions = {} ) { throwErrorIfNamespaceSpecified(options); return await this.client.bulkUpdate(objects, { From 2d79405e6a61e8dfa724acf58f6c8186de300994 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 31 Aug 2022 17:41:10 -0700 Subject: [PATCH 18/49] Fixes typo --- .../src/lib/repository.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index ee3db3627ba27..c9974e2bfafb8 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -773,7 +773,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } /** - * {@inheritDoc ISvedObjectsRepository.bulkDelete} + * {@inheritDoc ISavedObjectsRepository.bulkDelete} */ async bulkDelete( objects: SavedObjectsBulkDeleteObject[], From 39638d895b50ba22aae3cfc774675cc62c2c941d Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Sat, 3 Sep 2022 18:29:07 -0700 Subject: [PATCH 19/49] Starts adding bulkDelete repository unit tests --- .../src/lib/repository.test.ts | 219 ++++++++++++++++++ .../src/lib/repository.ts | 2 +- 2 files changed, 220 insertions(+), 1 deletion(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index c86945c6acb5c..05034341227f9 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -46,6 +46,8 @@ import type { SavedObjectsCollectMultiNamespaceReferencesResponse, SavedObjectsUpdateObjectsSpacesObject, SavedObjectsUpdateObjectsSpacesOptions, + SavedObjectsBulkDeleteObject, + SavedObjectsBulkDeleteOptions, } from '@kbn/core-saved-objects-api-server'; import type { SavedObjectsType, @@ -2044,6 +2046,223 @@ describe('SavedObjectsRepository', () => { }); }); + describe.only('#bulkDelete', () => { + const obj1: SavedObjectsBulkDeleteObject = { + type: 'config', + id: '6.0.0-alpha1', + }; + const obj2: SavedObjectsBulkDeleteObject = { + type: 'index-pattern', + id: 'logstash-*', + }; + + const namespace = 'foo-namespace'; + + const getMockEsBulkDeleteResponse = ( + objects: TypeIdTuple[], + options?: SavedObjectsBulkDeleteOptions + ) => + ({ + items: objects.map(({ type, id }) => ({ + // es response returns more fields than what we're interested in. + delete: { + _id: `${ + registry.isSingleNamespace(type) && options?.namespace ? `${options?.namespace}:` : '' + }${type}:${id}`, + ...mockVersionProps, + result: 'deleted', + }, + })), + } as estypes.BulkResponse); + + const repositoryBulkDeleteSuccess = async ( + objects: SavedObjectsBulkDeleteObject[], + options?: SavedObjectsBulkDeleteOptions + ) => { + // + const multiNamespaceObjects = objects.filter(({ type }) => registry.isMultiNamespace(type)); + if (multiNamespaceObjects?.length) { + const response = getMockMgetResponse(multiNamespaceObjects, options?.namespace); + client.mget.mockResponseOnce(response); + } + const response = getMockEsBulkDeleteResponse(objects, options); // we're not passing the version stuff along here. + + client.bulk.mockResponseOnce(response); + const result = await savedObjectsRepository.bulkDelete(objects, options); + // + expect(client.mget).toHaveBeenCalledTimes(multiNamespaceObjects?.length ? 1 : 0); + return result; + }; + + // bulk delete calls only has one object for each source -- the action + const expectClientCallBulkDeleteArgsAction = ( + objects: TypeIdTuple[], + { + method, + _index = expect.any(String), + getId = () => expect.any(String), + overrides = {}, + }: { + method: string; + _index?: string; + getId?: (type: string, id: string) => string; + overrides?: Record; + } + ) => { + const body = []; + for (const { type, id } of objects) { + body.push({ + [method]: { + _index, + _id: getId(type, id), + ...overrides, + }, + }); + } + + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }; + + const expectBulkDeleteObjArgs = ({ type, id }: { type: string; id: string }) => [ + { + delete: expect.objectContaining({ + _id: `${ + registry.isSingleNamespace(type) && namespace ? `${namespace}:` : '' + }${type}:${id}`, + _index: expect.any(String), + }), + }, + ]; + + beforeEach(() => { + mockDeleteLegacyUrlAliases.mockClear(); + mockDeleteLegacyUrlAliases.mockResolvedValue(); + }); + + describe('client calls', () => { + it(`should use the ES bulk action by default`, async () => { + await repositoryBulkDeleteSuccess([obj1, obj2]); + expect(client.bulk).toHaveBeenCalled(); + }); + + it(`should use the ES mget action before bulk action for any types that are multi-namespace`, async () => { + const objects = [obj1, { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }]; + await repositoryBulkDeleteSuccess(objects); + expect(client.bulk).toHaveBeenCalled(); + expect(client.mget).toHaveBeenCalled(); + + const docs = [ + expect.objectContaining({ _id: `${MULTI_NAMESPACE_ISOLATED_TYPE}:${obj2.id}` }), + ]; + expect(client.mget).toHaveBeenCalledWith( + expect.objectContaining({ body: { docs } }), + expect.anything() + ); + }); + + it(`should not use the ES bulk action when there are no valid documents to delete`, async () => { + const objects = [obj1, obj2].map((x) => ({ ...x, type: 'unknownType' })); + await savedObjectsRepository.bulkDelete(objects); + expect(client.bulk).toHaveBeenCalledTimes(0); + }); + + it(`formats the ES request`, async () => { + await repositoryBulkDeleteSuccess([obj1, obj2], { namespace }); + const body = [...expectBulkDeleteObjArgs(obj1), ...expectBulkDeleteObjArgs(obj2)]; + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }); + + it(`formats the ES request for any types that are multi-namespace`, async () => { + const _obj2 = { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }; + await repositoryBulkDeleteSuccess([obj1, _obj2], { namespace }); + const body = [...expectBulkDeleteObjArgs(obj1), ...expectBulkDeleteObjArgs(_obj2)]; + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }); + + it(`defaults to a refresh setting of wait_for`, async () => { + await repositoryBulkDeleteSuccess([obj1, obj2]); + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ refresh: 'wait_for' }), + expect.anything() + ); + }); + + // we don't call mget for single namespace docs so we can't get the version properties + it(`defaults to no version for types that are not multi-namespace`, async () => { + const objects = [obj1, { ...obj2, type: NAMESPACE_AGNOSTIC_TYPE }]; + await repositoryBulkDeleteSuccess(objects); + expectClientCallBulkDeleteArgsAction(objects, { method: 'delete' }); + }); + + // we only call mget for multinamespace docs and the implementation isn't handling the doc version correctly. I need help here. + it.skip(`accepts version for types that are multi-namespace`, async () => { + const version = encodeHitVersion({ _seq_no: 100, _primary_term: 200 }); + // test with both non-multi-namespace and multi-namespace types + const objects = [ + { ...obj1, version }, + { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE, version }, + ]; + await repositoryBulkDeleteSuccess(objects); + const overrides = { if_seq_no: 100, if_primary_term: 200 }; + expectClientCallBulkDeleteArgsAction(objects, { method: 'delete', overrides }); + }); + + it.todo(`prepends namespace to the id when providing namespace for single-namespace type`); + it.todo( + `doesn't prepend namespace to the id when providing no namespace for single-namespace type` + ); + it.todo(`normalizes options.namespace from 'default' to undefined`); + it.todo(`doesn't prepend namespace to the id when not using single-namespace type`); + it.todo(`defaults to not using the force option`); + it.todo(`accepts force option`); + }); + describe('legacy URL aliases', () => { + it.todo(`doesn't delete legacy URL aliases for single-namespace object types`); + it.todo(`deletes legacy URL aliases for multi-namespace object types (all spaces)`); + it.todo(`deletes legacy URL aliases for multi-namespace object types (specific spaces)`); + it.todo(`logs a message when deleteLegacyUrlAliases returns an error`); + }); + + describe('errors', () => { + it.todo(`returns an error when options.namespace is '*'`); + it.todo(`returns an error when type is invalid`); + it.todo(`returns an error when type is hidden`); + it.todo(`returns an error when ES is unable to find the document during get`); + it.todo(`returns an error when ES is unable to find the index during get`); + it.todo(`returns error when document is not found`); + it.todo( + `returns an error when the type is multi-namespace and the document exists, but not in this namespace` + ); + it.todo( + `returns an error when the type is multi-namespace and the document has multiple namespaces and the force option is not enabled` + ); + it.todo( + `returns an error when the type is multi-namespace and the document has all namespaces and the force option is not enabled` + ); + it.todo(`returns an error when ES is unable to find the document during delete`); + it.todo(`returns an error when ES is unable to find the index during delete`); + it.todo(`returns an error when ES returns an unexpected response`); + it.todo( + `returns error when type is multi-namespace and the document exists, but not in this namespace` + ); + }); + + describe('returns', () => { + it.todo(`returns early for empty objects argument`); + it.todo(`formats the ES response`); + it.todo(`handles a mix of successful deletes and errors`); + }); + }); + describe('#checkConflicts', () => { const obj1 = { type: 'dashboard', id: 'one' }; const obj2 = { type: 'dashboard', id: 'two' }; diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index c9974e2bfafb8..3119f45935f77 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -777,7 +777,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { */ async bulkDelete( objects: SavedObjectsBulkDeleteObject[], - options: SavedObjectsBulkDeleteOptions + options: SavedObjectsBulkDeleteOptions = {} ): Promise { const { refresh = DEFAULT_REFRESH_SETTING, force } = options; const namespace = normalizeNamespace(options.namespace); From d3dbcfcebd273e914de92f277c432a9da055fa14 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 5 Sep 2022 14:19:09 -0700 Subject: [PATCH 20/49] merge upstream main --- .../src/lib/repository.test.ts | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index 05034341227f9..6df0010aac1ed 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2196,26 +2196,12 @@ describe('SavedObjectsRepository', () => { ); }); - // we don't call mget for single namespace docs so we can't get the version properties - it(`defaults to no version for types that are not multi-namespace`, async () => { + it(`does not include the version of the existing document when using a multi-namespace type`, async () => { const objects = [obj1, { ...obj2, type: NAMESPACE_AGNOSTIC_TYPE }]; await repositoryBulkDeleteSuccess(objects); expectClientCallBulkDeleteArgsAction(objects, { method: 'delete' }); }); - // we only call mget for multinamespace docs and the implementation isn't handling the doc version correctly. I need help here. - it.skip(`accepts version for types that are multi-namespace`, async () => { - const version = encodeHitVersion({ _seq_no: 100, _primary_term: 200 }); - // test with both non-multi-namespace and multi-namespace types - const objects = [ - { ...obj1, version }, - { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE, version }, - ]; - await repositoryBulkDeleteSuccess(objects); - const overrides = { if_seq_no: 100, if_primary_term: 200 }; - expectClientCallBulkDeleteArgsAction(objects, { method: 'delete', overrides }); - }); - it.todo(`prepends namespace to the id when providing namespace for single-namespace type`); it.todo( `doesn't prepend namespace to the id when providing no namespace for single-namespace type` From d1fdc1e61d1700e0e65174f5f225d566883e9b07 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 5 Sep 2022 14:27:12 -0700 Subject: [PATCH 21/49] Cleans up incorrect version-handling --- .../src/lib/repository.ts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 3119f45935f77..b1ec43794ca0f 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -844,15 +844,9 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { if (isLeft(expectedBulkGetResult)) { return { ...expectedBulkGetResult }; } - const { - esRequestIndex: esBulkGetRequestIndex, - id, - type, - version, - } = expectedBulkGetResult.value; + const { esRequestIndex: esBulkGetRequestIndex, id, type } = expectedBulkGetResult.value; let namespaces; - let versionProperties; if (esBulkGetRequestIndex !== undefined) { const indexFound = bulkGetMultiNamespaceDocsResponse?.statusCode !== 404; @@ -895,7 +889,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { SavedObjectsUtils.namespaceIdToString(namespace), ]; - versionProperties = getExpectedVersionProperties(version); // the document exists but is multinamespace and there are more than one namespaces present and can only be deleted by force. if (!force && (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING))) { return { @@ -917,7 +910,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { if (this._registry.isSingleNamespace(type)) { namespaces = [SavedObjectsUtils.namespaceIdToString(namespace)]; } - versionProperties = getExpectedVersionProperties(version); } const expectedResult = { @@ -931,7 +923,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { delete: { _id: this._serializer.generateRawId(namespace, type, id), _index: this.getIndexForType(type), - ...versionProperties, + ...getExpectedVersionProperties(undefined), }, }); From 29fbfca5b20f50684df808dc051b8e46d919247e Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 5 Sep 2022 14:32:36 -0700 Subject: [PATCH 22/49] changes async to sync preflight check --- .../src/lib/repository.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index b1ec43794ca0f..99e918820b5e3 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -838,9 +838,9 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { let bulkDeleteRequestIndexCounter = 0; const bulkDeleteParams: BulkDeleteParams[] = []; - const expectedBulkDeleteMultiNamespaceDocsResults = await Promise.all( - expectedBulkGetAllDocsWithNamespaceChecksFlag.map>( - async (expectedBulkGetResult) => { + const expectedBulkDeleteMultiNamespaceDocsResults = + expectedBulkGetAllDocsWithNamespaceChecksFlag.map( + (expectedBulkGetResult) => { if (isLeft(expectedBulkGetResult)) { return { ...expectedBulkGetResult }; } @@ -929,8 +929,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { return { tag: 'Right', value: expectedResult }; } - ) - ); + ); const bulkDeleteResponse = bulkDeleteParams.length ? await this.client.bulk({ From 9edd64e75dd462ae0a44000820fe76c552ad72a2 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 5 Sep 2022 14:49:55 -0700 Subject: [PATCH 23/49] Update imports from core usage data service --- .../saved_objects/routes/bulk_delete.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts b/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts index 5eb8b9a874a19..2536915f6f068 100644 --- a/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts +++ b/src/core/server/integration_tests/saved_objects/routes/bulk_delete.test.ts @@ -8,9 +8,11 @@ import supertest from 'supertest'; import { savedObjectsClientMock } from '../../../mocks'; -import { CoreUsageStatsClient } from '../../../core_usage_data'; -import { coreUsageStatsClientMock } from '../../../core_usage_data/core_usage_stats_client.mock'; -import { coreUsageDataServiceMock } from '../../../core_usage_data/core_usage_data_service.mock'; +import type { ICoreUsageStatsClient } from '@kbn/core-usage-data-base-server-internal'; +import { + coreUsageStatsClientMock, + coreUsageDataServiceMock, +} from '@kbn/core-usage-data-server-mocks'; import { setupServer } from './test_utils'; import { registerBulkDeleteRoute, @@ -24,7 +26,7 @@ describe('POST /api/saved_objects/_bulk_delete', () => { let httpSetup: SetupServerReturn['httpSetup']; let handlerContext: SetupServerReturn['handlerContext']; let savedObjectsClient: ReturnType; - let coreUsageStatsClient: jest.Mocked; + let coreUsageStatsClient: jest.Mocked; beforeEach(async () => { ({ server, httpSetup, handlerContext } = await setupServer()); From 6c91b93bd3daca071b68286d2c8bd38e5887add0 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 5 Sep 2022 17:18:43 -0700 Subject: [PATCH 24/49] Adds more bulkDelete client calls unit tests --- .../src/lib/repository.test.ts | 48 +++++++++++++++---- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index 6df0010aac1ed..fed1e0549897a 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2046,7 +2046,7 @@ describe('SavedObjectsRepository', () => { }); }); - describe.only('#bulkDelete', () => { + describe('#bulkDelete', () => { const obj1: SavedObjectsBulkDeleteObject = { type: 'config', id: '6.0.0-alpha1', @@ -2202,13 +2202,45 @@ describe('SavedObjectsRepository', () => { expectClientCallBulkDeleteArgsAction(objects, { method: 'delete' }); }); - it.todo(`prepends namespace to the id when providing namespace for single-namespace type`); - it.todo( - `doesn't prepend namespace to the id when providing no namespace for single-namespace type` - ); - it.todo(`normalizes options.namespace from 'default' to undefined`); - it.todo(`doesn't prepend namespace to the id when not using single-namespace type`); - it.todo(`defaults to not using the force option`); + it(`prepends namespace to the id when providing namespace for single-namespace type`, async () => { + const getId = (type: string, id: string) => `${namespace}:${type}:${id}`; + await repositoryBulkDeleteSuccess([obj1, obj2], { namespace }); + expectClientCallBulkDeleteArgsAction([obj1, obj2], { method: 'delete', getId }); + }); + + it(`doesn't prepend namespace to the id when providing no namespace for single-namespace type`, async () => { + const getId = (type: string, id: string) => `${type}:${id}`; + await repositoryBulkDeleteSuccess([obj1, obj2]); + expectClientCallBulkDeleteArgsAction([obj1, obj2], { method: 'delete', getId }); + }); + + it(`normalizes options.namespace from 'default' to undefined`, async () => { + const getId = (type: string, id: string) => `${type}:${id}`; + await repositoryBulkDeleteSuccess([obj1, obj2], { namespace: 'default' }); + expectClientCallBulkDeleteArgsAction([obj1, obj2], { method: 'delete', getId }); + }); + + it(`doesn't prepend namespace to the id when not using single-namespace type`, async () => { + const getId = (type: string, id: string) => `${type}:${id}`; // not expecting namespace prefix; + const _obj1 = { ...obj1, type: NAMESPACE_AGNOSTIC_TYPE }; + const _obj2 = { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }; + + await repositoryBulkDeleteSuccess([_obj1], { namespace }); + expectClientCallBulkDeleteArgsAction([_obj1], { method: 'delete', getId }); + client.bulk.mockClear(); + await repositoryBulkDeleteSuccess([_obj2], { namespace }); + expectClientCallBulkDeleteArgsAction([_obj2], { method: 'delete', getId }); + client.bulk.mockClear(); + }); + + it(`does not pass on the force option`, async () => { + await repositoryBulkDeleteSuccess([obj1, obj2]); + expect(client.bulk).toHaveBeenCalledWith( + expect.not.objectContaining({ force: expect.anything() }), + expect.anything() + ); + }); + it.todo(`accepts force option`); }); describe('legacy URL aliases', () => { From 98303bbf6c7c12fb3e23916f936543677fdb6af2 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Tue, 6 Sep 2022 15:15:22 -0700 Subject: [PATCH 25/49] We only want namespaces in the expected result for multinamespace objects, otherwise we'll incorrectly assume there are legacy URL aliases to remove for all namespace types --- .../src/lib/repository.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 99e918820b5e3..54a0196a64383 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -905,11 +905,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { }, }; } - } else { - // checking if the object is single namespace type should be redundant, since it wouldn't have an `esRequestIndex` but we do the check to be safe - if (this._registry.isSingleNamespace(type)) { - namespaces = [SavedObjectsUtils.namespaceIdToString(namespace)]; - } } const expectedResult = { @@ -973,6 +968,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } const deleted = rawResponse.result === 'deleted'; if (deleted) { + // `namespaces` should only exist in the expectedResult.value if the type is multinamespace. if (namespaces) { await deleteLegacyUrlAliases({ mappings: this._mappings, From 717b834bd95308b0b05152c2eaecc448e6b38d56 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Tue, 6 Sep 2022 17:30:37 -0700 Subject: [PATCH 26/49] Adds repository unit tests for bulkDelete legacy URL aliases cleanup --- .../src/lib/repository.test.ts | 122 +++++++++++++++--- 1 file changed, 106 insertions(+), 16 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index fed1e0549897a..8b4871320842e 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2076,20 +2076,28 @@ describe('SavedObjectsRepository', () => { } as estypes.BulkResponse); const repositoryBulkDeleteSuccess = async ( - objects: SavedObjectsBulkDeleteObject[], - options?: SavedObjectsBulkDeleteOptions + objects: SavedObjectsBulkDeleteObject[] = [], + options?: SavedObjectsBulkDeleteOptions, + internalOptions: { + mockMGetResponseWithObject?: { initialNamespaces: string[]; type: string; id: string }; + } = {} ) => { - // - const multiNamespaceObjects = objects.filter(({ type }) => registry.isMultiNamespace(type)); - if (multiNamespaceObjects?.length) { - const response = getMockMgetResponse(multiNamespaceObjects, options?.namespace); - client.mget.mockResponseOnce(response); + const multiNamespaceObjects = objects.filter(({ type }) => { + return registry.isMultiNamespace(type); + }); + + const { mockMGetResponseWithObject } = internalOptions; + if (multiNamespaceObjects.length > 0) { + const mockedMGetResponse = mockMGetResponseWithObject + ? getMockMgetResponse([mockMGetResponseWithObject], options?.namespace) + : getMockMgetResponse(multiNamespaceObjects, options?.namespace); + client.mget.mockResponseOnce(mockedMGetResponse); } - const response = getMockEsBulkDeleteResponse(objects, options); // we're not passing the version stuff along here. + const mockedEsBulkDeleteResponse = getMockEsBulkDeleteResponse(objects, options); - client.bulk.mockResponseOnce(response); + client.bulk.mockResponseOnce(mockedEsBulkDeleteResponse); const result = await savedObjectsRepository.bulkDelete(objects, options); - // + expect(client.mget).toHaveBeenCalledTimes(multiNamespaceObjects?.length ? 1 : 0); return result; }; @@ -2240,14 +2248,96 @@ describe('SavedObjectsRepository', () => { expect.anything() ); }); - - it.todo(`accepts force option`); }); describe('legacy URL aliases', () => { - it.todo(`doesn't delete legacy URL aliases for single-namespace object types`); - it.todo(`deletes legacy URL aliases for multi-namespace object types (all spaces)`); - it.todo(`deletes legacy URL aliases for multi-namespace object types (specific spaces)`); - it.todo(`logs a message when deleteLegacyUrlAliases returns an error`); + it(`doesn't delete legacy URL aliases for single-namespace object types`, async () => { + await repositoryBulkDeleteSuccess([obj1, obj2]); + expect(mockDeleteLegacyUrlAliases).not.toHaveBeenCalled(); + }); + it(`deletes legacy URL aliases for multi-namespace object types (all spaces)`, async () => { + const testObject = { ...obj1, type: MULTI_NAMESPACE_TYPE }; + const internalOptions = { + mockMGetResponseWithObject: { + ...testObject, + initialNamespaces: [ALL_NAMESPACES_STRING], + }, + }; + await repositoryBulkDeleteSuccess( + [testObject], + { namespace, force: true }, + internalOptions + ); + expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledWith( + expect.objectContaining({ + type: MULTI_NAMESPACE_TYPE, + id: testObject.id, + namespaces: [], + deleteBehavior: 'exclusive', + }) + ); + }); + it(`deletes legacy URL aliases for multi-namespace object types (specific space)`, async () => { + const testObject = { ...obj1, type: MULTI_NAMESPACE_TYPE }; + const internalOptions = { + mockMGetResponseWithObject: { + ...testObject, + initialNamespaces: [namespace], + }, + }; + // specifically test against the current namespace + await repositoryBulkDeleteSuccess([testObject], { namespace }, internalOptions); + expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledWith( + expect.objectContaining({ + type: MULTI_NAMESPACE_TYPE, + id: testObject.id, + namespaces: [namespace], + deleteBehavior: 'inclusive', + }) + ); + }); + + it(`deletes legacy URL aliases for multi-namespace object types shared to many specific spaces`, async () => { + const testObject = { ...obj1, type: MULTI_NAMESPACE_TYPE }; + const initialTestObjectNamespaces = [namespace, 'bar-namespace']; + const internalOptions = { + mockMGetResponseWithObject: { + ...testObject, + initialNamespaces: initialTestObjectNamespaces, + }, + }; + // specifically test against named spaces ('*' is handled specifically, this assures we also take care of named spaces) + await repositoryBulkDeleteSuccess( + [testObject], + { namespace, force: true }, + internalOptions + ); + expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledWith( + expect.objectContaining({ + type: MULTI_NAMESPACE_TYPE, + id: testObject.id, + namespaces: initialTestObjectNamespaces, + deleteBehavior: 'inclusive', + }) + ); + }); + + it(`logs a message when deleteLegacyUrlAliases returns an error`, async () => { + const testObject = { type: MULTI_NAMESPACE_ISOLATED_TYPE, id: obj1.id }; + client.mget.mockResolvedValueOnce( + elasticsearchClientMock.createSuccessTransportRequestPromise( + getMockMgetResponse([testObject], namespace) + ) + ); + const mockedBulkResponse = getMockEsBulkDeleteResponse([testObject], { namespace }); + client.bulk.mockResolvedValueOnce(mockedBulkResponse); + mockDeleteLegacyUrlAliases.mockRejectedValueOnce(new Error('Oh no!')); + await savedObjectsRepository.bulkDelete([testObject], { namespace }); + expect(client.mget).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith( + 'Unable to delete aliases when deleting an object: Oh no!' + ); + }); }); describe('errors', () => { From 840303803d565c6f1d5291cb94e2a84be4ea79c9 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 7 Sep 2022 18:22:21 -0700 Subject: [PATCH 27/49] Adds unit tests for bulkDelete error cases, updates a few helpers --- .../src/lib/repository.test.ts | 237 ++++++++++++++++-- 1 file changed, 212 insertions(+), 25 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index 8b4871320842e..b958b15a46beb 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2134,7 +2134,10 @@ describe('SavedObjectsRepository', () => { ); }; - const expectBulkDeleteObjArgs = ({ type, id }: { type: string; id: string }) => [ + const expectBulkDeleteObjArgs = ( + { type, id }: { type: string; id: string }, + namespace: string + ) => [ { delete: expect.objectContaining({ _id: `${ @@ -2179,7 +2182,10 @@ describe('SavedObjectsRepository', () => { it(`formats the ES request`, async () => { await repositoryBulkDeleteSuccess([obj1, obj2], { namespace }); - const body = [...expectBulkDeleteObjArgs(obj1), ...expectBulkDeleteObjArgs(obj2)]; + const body = [ + ...expectBulkDeleteObjArgs(obj1, namespace), + ...expectBulkDeleteObjArgs(obj2, namespace), + ]; expect(client.bulk).toHaveBeenCalledWith( expect.objectContaining({ body }), expect.anything() @@ -2189,7 +2195,10 @@ describe('SavedObjectsRepository', () => { it(`formats the ES request for any types that are multi-namespace`, async () => { const _obj2 = { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }; await repositoryBulkDeleteSuccess([obj1, _obj2], { namespace }); - const body = [...expectBulkDeleteObjArgs(obj1), ...expectBulkDeleteObjArgs(_obj2)]; + const body = [ + ...expectBulkDeleteObjArgs(obj1, namespace), + ...expectBulkDeleteObjArgs(_obj2, namespace), + ]; expect(client.bulk).toHaveBeenCalledWith( expect.objectContaining({ body }), expect.anything() @@ -2204,7 +2213,7 @@ describe('SavedObjectsRepository', () => { ); }); - it(`does not include the version of the existing document when using a multi-namespace type`, async () => { + it(`does not include the version of the existing document when not using a multi-namespace type`, async () => { const objects = [obj1, { ...obj2, type: NAMESPACE_AGNOSTIC_TYPE }]; await repositoryBulkDeleteSuccess(objects); expectClientCallBulkDeleteArgsAction(objects, { method: 'delete' }); @@ -2249,6 +2258,7 @@ describe('SavedObjectsRepository', () => { ); }); }); + describe('legacy URL aliases', () => { it(`doesn't delete legacy URL aliases for single-namespace object types`, async () => { await repositoryBulkDeleteSuccess([obj1, obj2]); @@ -2341,27 +2351,204 @@ describe('SavedObjectsRepository', () => { }); describe('errors', () => { - it.todo(`returns an error when options.namespace is '*'`); - it.todo(`returns an error when type is invalid`); - it.todo(`returns an error when type is hidden`); - it.todo(`returns an error when ES is unable to find the document during get`); - it.todo(`returns an error when ES is unable to find the index during get`); - it.todo(`returns error when document is not found`); - it.todo( - `returns an error when the type is multi-namespace and the document exists, but not in this namespace` - ); - it.todo( - `returns an error when the type is multi-namespace and the document has multiple namespaces and the force option is not enabled` - ); - it.todo( - `returns an error when the type is multi-namespace and the document has all namespaces and the force option is not enabled` - ); - it.todo(`returns an error when ES is unable to find the document during delete`); - it.todo(`returns an error when ES is unable to find the index during delete`); - it.todo(`returns an error when ES returns an unexpected response`); - it.todo( - `returns error when type is multi-namespace and the document exists, but not in this namespace` - ); + const expectRepositoryBulkDeleteStatusError = ({ + type, + id, + error, + }: { + type: string; + id: string; + error?: any; + }) => ({ + type, + id, + success: false, + error: error ?? createBadRequestError(), + }); + const repositoryBulkDeleteError = async ( + obj: SavedObjectsBulkDeleteObject, + isBulkError: boolean, + expectedErrorResult: ExpectedErrorResult + ) => { + const objects = [obj1, obj, obj2]; + const mockedBulkDeleteResponse = getMockEsBulkDeleteResponse(objects); + if (isBulkError) { + mockGetBulkOperationError.mockReturnValueOnce(undefined); + mockGetBulkOperationError.mockReturnValueOnce(expectedErrorResult.error as Payload); + } + client.bulk.mockResponseOnce(mockedBulkDeleteResponse); + + const result = await savedObjectsRepository.bulkDelete(objects); + expect(client.bulk).toHaveBeenCalled(); + expect(result).toEqual({ + statuses: [ + { ...obj1, success: true }, + { ...expectedErrorResult, success: false }, + { ...obj2, success: true }, + ], + }); + }; + const expectClientCallArgsAction = ( + objects: TypeIdTuple[], + { + method, + _index = expect.any(String), + getId = () => expect.any(String), + overrides = {}, + }: { + method: string; + _index?: string; + getId?: (type: string, id: string) => string; + overrides?: Record; + } + ) => { + const body = []; + for (const { type, id } of objects) { + body.push({ + [method]: { + _index, + _id: getId(type, id), + ...overrides, + }, + }); + } + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }; + const bulkDeleteMultiError = async ( + [obj1, _obj, obj2]: SavedObjectsBulkDeleteObject[], + options: SavedObjectsBulkDeleteOptions | undefined, + mgetResponse: estypes.MgetResponse, + mgetOptions?: { statusCode?: number } + ) => { + const getId = (type: string, id: string) => `${options?.namespace}:${type}:${id}`; + // mock teh response for the not found doc + client.mget.mockResponseOnce(mgetResponse, { statusCode: mgetOptions?.statusCode }); + // get a mocked response for the valid docs + const bulkResponse = getMockEsBulkDeleteResponse([obj1, obj2], { namespace }); + client.bulk.mockResponseOnce(bulkResponse); + + const result = await savedObjectsRepository.bulkDelete([obj1, _obj, obj2], options); + expect(client.bulk).toHaveBeenCalledTimes(1); + expect(client.mget).toHaveBeenCalledTimes(1); + + expectClientCallArgsAction([obj1, obj2], { method: 'delete', getId }); + expect(result).toEqual({ + statuses: [ + { ...obj1, success: true }, + { ...expectErrorNotFound(_obj), success: false }, + { ...obj2, success: true }, + ], + }); + }; + + it(`throws an error when options.namespace is '*'`, async () => { + await expect( + savedObjectsRepository.bulkDelete([obj1], { namespace: ALL_NAMESPACES_STRING }) + ).rejects.toThrowError(createBadRequestError('"options.namespace" cannot be "*"')); + }); + + it(`throws an error when client bulk response is not defined`, async () => { + client.mget.mockResolvedValueOnce( + elasticsearchClientMock.createSuccessTransportRequestPromise( + getMockMgetResponse([obj1], namespace) + ) + ); + const mockedBulkResponse = undefined; + // we have to cast here to test the assumption we always get a response. + client.bulk.mockResponseOnce(mockedBulkResponse as unknown as estypes.BulkResponse); + await expect(savedObjectsRepository.bulkDelete([obj1], { namespace })).rejects.toThrowError( + 'Unexpected error in bulkDelete saved objects: bulkDeleteResponse is undefined' + ); + }); + + it(`returns an error for the object when the object's type is invalid`, async () => { + const unknownObjType = { ...obj1, type: 'unknownType' }; + await repositoryBulkDeleteError( + unknownObjType, + false, + expectErrorInvalidType(unknownObjType) + ); + }); + + it(`returns an for an object when the object's type is hidden`, async () => { + const hiddenObject = { ...obj1, type: HIDDEN_TYPE }; + await repositoryBulkDeleteError(hiddenObject, false, expectErrorInvalidType(hiddenObject)); + }); + + it(`returns an error when ES is unable to find the document during mget`, async () => { + const notFoundObj = { ...obj1, type: MULTI_NAMESPACE_ISOLATED_TYPE, found: false }; + const mgetResponse = getMockMgetResponse([notFoundObj], namespace); + await bulkDeleteMultiError([obj1, notFoundObj, obj2], { namespace }, mgetResponse); + }); + + it(`returns an error when ES is unable to find the index during mget`, async () => { + const notFoundObj = { ...obj1, type: MULTI_NAMESPACE_ISOLATED_TYPE, found: false }; + await bulkDeleteMultiError( + [obj1, notFoundObj, obj2], + { namespace }, + {} as estypes.MgetResponse, + { + statusCode: 404, + } + ); + }); + + it(`returns an error when the type is multi-namespace and the document exists, but not in this namespace`, async () => { + const obj = { + type: MULTI_NAMESPACE_ISOLATED_TYPE, + id: 'three', + namespace: 'bar-namespace', + }; + const mgetResponse = getMockMgetResponse([obj], namespace); + await bulkDeleteMultiError([obj1, obj, obj2], { namespace }, mgetResponse); + }); + + it(`returns an error when the type is multi-namespace and the document has multiple namespaces and the force option is not enabled`, async () => { + const testObject = { ...obj1, type: MULTI_NAMESPACE_TYPE }; + const internalOptions = { + mockMGetResponseWithObject: { + ...testObject, + initialNamespaces: [namespace, 'bar-namespace'], + }, + }; + const result = await repositoryBulkDeleteSuccess( + [testObject], + { namespace }, + internalOptions + ); + expect(result.statuses[0]).toStrictEqual( + expectRepositoryBulkDeleteStatusError({ + ...testObject, + error: createBadRequestError( + 'Unable to delete saved object that exists in multiple namespaces, use the "force" option to delete it anyway' + ), + }) + ); + }); + + it(`returns an error when the type is multi-namespace and the document has all namespaces and the force option is not enabled`, async () => { + const testObject = { ...obj1, type: ALL_NAMESPACES_STRING }; + const internalOptions = { + mockMGetResponseWithObject: { + ...testObject, + initialNamespaces: [namespace, 'bar-namespace'], + }, + }; + const result = await repositoryBulkDeleteSuccess( + [testObject], + { namespace }, + internalOptions + ); + expect(result.statuses[0]).toStrictEqual( + expectRepositoryBulkDeleteStatusError({ + ...testObject, + error: createBadRequestError("Unsupported saved object type: '*'"), + }) + ); + }); }); describe('returns', () => { From 643a12bad207b15e0a37ea58a143334e79894c46 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 7 Sep 2022 18:25:51 -0700 Subject: [PATCH 28/49] Fixes typo --- .../src/lib/repository.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index b958b15a46beb..b5837b9175c9e 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2424,7 +2424,7 @@ describe('SavedObjectsRepository', () => { mgetOptions?: { statusCode?: number } ) => { const getId = (type: string, id: string) => `${options?.namespace}:${type}:${id}`; - // mock teh response for the not found doc + // mock the response for the not found doc client.mget.mockResponseOnce(mgetResponse, { statusCode: mgetOptions?.statusCode }); // get a mocked response for the valid docs const bulkResponse = getMockEsBulkDeleteResponse([obj1, obj2], { namespace }); From a34ee765cd62dfc05a6b8b8320e43a30f105a28e Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 7 Sep 2022 18:51:45 -0700 Subject: [PATCH 29/49] Adds bulkDelete return assertions --- .../src/lib/repository.test.ts | 102 +++++++++++++++++- 1 file changed, 99 insertions(+), 3 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index b5837b9175c9e..71e497b077a81 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2552,9 +2552,105 @@ describe('SavedObjectsRepository', () => { }); describe('returns', () => { - it.todo(`returns early for empty objects argument`); - it.todo(`formats the ES response`); - it.todo(`handles a mix of successful deletes and errors`); + const expectSuccessResult = ({ type, id }: SavedObjectsBulkDeleteObject) => ({ + type, + id, + success: true, + }); + + const expectErrorResult = ({ + type, + id, + error, + }: { + type: string; + id: string; + error?: any; + }) => ({ + type, + id, + success: false, + error: error ?? createBadRequestError(), + }); + + const expectClientCallArgsAction = ( + objects: TypeIdTuple[], + { + method, + _index = expect.any(String), + getId = () => expect.any(String), + overrides = {}, + }: { + method: string; + _index?: string; + getId?: (type: string, id: string) => string; + overrides?: Record; + } + ) => { + const body = []; + for (const { type, id } of objects) { + body.push({ + [method]: { + _index, + _id: getId(type, id), + ...overrides, + }, + }); + } + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }; + + const bulkDeleteMultiError = async ( + [obj1, _obj, obj2]: SavedObjectsBulkDeleteObject[], + options: SavedObjectsBulkDeleteOptions | undefined, + mgetResponse: estypes.MgetResponse, + mgetOptions?: { statusCode?: number } + ) => { + const getId = (type: string, id: string) => `${options?.namespace}:${type}:${id}`; + // mock the response for the not found doc + client.mget.mockResponseOnce(mgetResponse, { statusCode: mgetOptions?.statusCode }); + // get a mocked response for the valid docs + const bulkResponse = getMockEsBulkDeleteResponse([obj1, obj2], { namespace }); + client.bulk.mockResponseOnce(bulkResponse); + + const result = await savedObjectsRepository.bulkDelete([obj1, _obj, obj2], options); + expect(client.bulk).toHaveBeenCalledTimes(1); + expect(client.mget).toHaveBeenCalledTimes(1); + + expectClientCallArgsAction([obj1, obj2], { method: 'delete', getId }); + expect(result).toEqual({ + statuses: [ + { ...obj1, success: true }, + { ...expectErrorNotFound(_obj), success: false }, + { ...obj2, success: true }, + ], + }); + }; + + it(`returns early for empty objects argument`, async () => { + await savedObjectsRepository.bulkDelete([], { namespace }); + expect(client.bulk).toHaveBeenCalledTimes(0); + }); + + it(`formats the ES response`, async () => { + const response = await repositoryBulkDeleteSuccess([obj1, obj2], { namespace }); + expect(response).toEqual({ + statuses: [obj1, obj2].map(expectSuccessResult), + }); + }); + + it(`handles a mix of successful deletes and errors`, async () => { + const notFoundObj = { ...obj1, type: MULTI_NAMESPACE_ISOLATED_TYPE, found: false }; + await bulkDeleteMultiError( + [obj1, notFoundObj, obj2], + { namespace }, + {} as estypes.MgetResponse, + { statusCode: 404 } + ); + }); }); }); From 17af04fc667618a20e02433a395358d7a0a0c0da Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Thu, 8 Sep 2022 07:24:44 -0700 Subject: [PATCH 30/49] Removes unused method --- .../src/lib/repository.test.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index 71e497b077a81..40beb2834ba7e 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2558,21 +2558,6 @@ describe('SavedObjectsRepository', () => { success: true, }); - const expectErrorResult = ({ - type, - id, - error, - }: { - type: string; - id: string; - error?: any; - }) => ({ - type, - id, - success: false, - error: error ?? createBadRequestError(), - }); - const expectClientCallArgsAction = ( objects: TypeIdTuple[], { From 93da2f8fe74c25e50653ca111eb9bb927fdf4a6a Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Thu, 8 Sep 2022 10:26:59 -0700 Subject: [PATCH 31/49] Cleans up repository bulk delete unit tests --- .../src/lib/repository.test.ts | 330 +++++++----------- 1 file changed, 124 insertions(+), 206 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index 40beb2834ba7e..1d663be0f18b4 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2058,6 +2058,9 @@ describe('SavedObjectsRepository', () => { const namespace = 'foo-namespace'; + const createNamespaceAwareGetId = (type: string, id: string) => + `${registry.isSingleNamespace(type) && namespace ? `${namespace}:` : ''}${type}:${id}`; + const getMockEsBulkDeleteResponse = ( objects: TypeIdTuple[], options?: SavedObjectsBulkDeleteOptions @@ -2134,19 +2137,108 @@ describe('SavedObjectsRepository', () => { ); }; - const expectBulkDeleteObjArgs = ( - { type, id }: { type: string; id: string }, - namespace: string - ) => [ + const createBulkDeleteFailStatus = ({ + type, + id, + error, + }: { + type: string; + id: string; + error?: ExpectedErrorResult['error']; + }) => ({ + type, + id, + success: false, + error: error ?? createBadRequestError(), + }); + + const createBulkDeleteSuccessStatus = ({ type, id }: { type: string; id: string }) => ({ + type, + id, + success: true, + }); + + // mocks a combination of success, error results for hidden and unknown object object types. + const repositoryBulkDeleteError = async ( + obj: SavedObjectsBulkDeleteObject, + isBulkError: boolean, + expectedErrorResult: ExpectedErrorResult + ) => { + const objects = [obj1, obj, obj2]; + const mockedBulkDeleteResponse = getMockEsBulkDeleteResponse(objects); + if (isBulkError) { + mockGetBulkOperationError.mockReturnValueOnce(undefined); + mockGetBulkOperationError.mockReturnValueOnce(expectedErrorResult.error as Payload); + } + client.bulk.mockResponseOnce(mockedBulkDeleteResponse); + + const result = await savedObjectsRepository.bulkDelete(objects); + expect(client.bulk).toHaveBeenCalled(); + expect(result).toEqual({ + statuses: [ + createBulkDeleteSuccessStatus(obj1), + createBulkDeleteFailStatus({ ...obj, error: expectedErrorResult.error }), + createBulkDeleteSuccessStatus(obj2), + ], + }); + }; + + const expectClientCallArgsAction = ( + objects: TypeIdTuple[], { - delete: expect.objectContaining({ - _id: `${ - registry.isSingleNamespace(type) && namespace ? `${namespace}:` : '' - }${type}:${id}`, - _index: expect.any(String), - }), - }, - ]; + method, + _index = expect.any(String), + getId = () => expect.any(String), + overrides = {}, + }: { + method: string; + _index?: string; + getId?: (type: string, id: string) => string; + overrides?: Record; + } + ) => { + const body = []; + for (const { type, id } of objects) { + body.push({ + [method]: { + _index, + _id: getId(type, id), + ...overrides, + }, + }); + } + expect(client.bulk).toHaveBeenCalledWith( + expect.objectContaining({ body }), + expect.anything() + ); + }; + + const bulkDeleteMultiNamespaceError = async ( + [obj1, _obj, obj2]: SavedObjectsBulkDeleteObject[], + options: SavedObjectsBulkDeleteOptions | undefined, + mgetResponse: estypes.MgetResponse, + mgetOptions?: { statusCode?: number } + ) => { + const getId = (type: string, id: string) => `${options?.namespace}:${type}:${id}`; + // mock the response for the not found doc + client.mget.mockResponseOnce(mgetResponse, { statusCode: mgetOptions?.statusCode }); + // get a mocked response for the valid docs + const bulkResponse = getMockEsBulkDeleteResponse([obj1, obj2], { namespace }); + client.bulk.mockResponseOnce(bulkResponse); + + const result = await savedObjectsRepository.bulkDelete([obj1, _obj, obj2], options); + expect(client.bulk).toHaveBeenCalledTimes(1); + expect(client.mget).toHaveBeenCalledTimes(1); + + expectClientCallArgsAction([obj1, obj2], { method: 'delete', getId }); + expect(result).toEqual({ + statuses: [ + createBulkDeleteSuccessStatus(obj1), + { ...expectErrorNotFound(_obj), success: false }, + createBulkDeleteSuccessStatus(obj2), + ], + }); + }; beforeEach(() => { mockDeleteLegacyUrlAliases.mockClear(); @@ -2181,28 +2273,16 @@ describe('SavedObjectsRepository', () => { }); it(`formats the ES request`, async () => { + const getId = createNamespaceAwareGetId; await repositoryBulkDeleteSuccess([obj1, obj2], { namespace }); - const body = [ - ...expectBulkDeleteObjArgs(obj1, namespace), - ...expectBulkDeleteObjArgs(obj2, namespace), - ]; - expect(client.bulk).toHaveBeenCalledWith( - expect.objectContaining({ body }), - expect.anything() - ); + expectClientCallBulkDeleteArgsAction([obj1, obj2], { method: 'delete', getId }); }); it(`formats the ES request for any types that are multi-namespace`, async () => { const _obj2 = { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }; + const getId = createNamespaceAwareGetId; await repositoryBulkDeleteSuccess([obj1, _obj2], { namespace }); - const body = [ - ...expectBulkDeleteObjArgs(obj1, namespace), - ...expectBulkDeleteObjArgs(_obj2, namespace), - ]; - expect(client.bulk).toHaveBeenCalledWith( - expect.objectContaining({ body }), - expect.anything() - ); + expectClientCallBulkDeleteArgsAction([obj1, _obj2], { method: 'delete', getId }); }); it(`defaults to a refresh setting of wait_for`, async () => { @@ -2220,7 +2300,7 @@ describe('SavedObjectsRepository', () => { }); it(`prepends namespace to the id when providing namespace for single-namespace type`, async () => { - const getId = (type: string, id: string) => `${namespace}:${type}:${id}`; + const getId = createNamespaceAwareGetId; await repositoryBulkDeleteSuccess([obj1, obj2], { namespace }); expectClientCallBulkDeleteArgsAction([obj1, obj2], { method: 'delete', getId }); }); @@ -2242,20 +2322,8 @@ describe('SavedObjectsRepository', () => { const _obj1 = { ...obj1, type: NAMESPACE_AGNOSTIC_TYPE }; const _obj2 = { ...obj2, type: MULTI_NAMESPACE_ISOLATED_TYPE }; - await repositoryBulkDeleteSuccess([_obj1], { namespace }); - expectClientCallBulkDeleteArgsAction([_obj1], { method: 'delete', getId }); - client.bulk.mockClear(); - await repositoryBulkDeleteSuccess([_obj2], { namespace }); - expectClientCallBulkDeleteArgsAction([_obj2], { method: 'delete', getId }); - client.bulk.mockClear(); - }); - - it(`does not pass on the force option`, async () => { - await repositoryBulkDeleteSuccess([obj1, obj2]); - expect(client.bulk).toHaveBeenCalledWith( - expect.not.objectContaining({ force: expect.anything() }), - expect.anything() - ); + await repositoryBulkDeleteSuccess([_obj1, _obj2], { namespace }); + expectClientCallBulkDeleteArgsAction([_obj1, _obj2], { method: 'delete', getId }); }); }); @@ -2264,6 +2332,7 @@ describe('SavedObjectsRepository', () => { await repositoryBulkDeleteSuccess([obj1, obj2]); expect(mockDeleteLegacyUrlAliases).not.toHaveBeenCalled(); }); + it(`deletes legacy URL aliases for multi-namespace object types (all spaces)`, async () => { const testObject = { ...obj1, type: MULTI_NAMESPACE_TYPE }; const internalOptions = { @@ -2286,6 +2355,7 @@ describe('SavedObjectsRepository', () => { }) ); }); + it(`deletes legacy URL aliases for multi-namespace object types (specific space)`, async () => { const testObject = { ...obj1, type: MULTI_NAMESPACE_TYPE }; const internalOptions = { @@ -2333,6 +2403,7 @@ describe('SavedObjectsRepository', () => { it(`logs a message when deleteLegacyUrlAliases returns an error`, async () => { const testObject = { type: MULTI_NAMESPACE_ISOLATED_TYPE, id: obj1.id }; + client.mget.mockResolvedValueOnce( elasticsearchClientMock.createSuccessTransportRequestPromise( getMockMgetResponse([testObject], namespace) @@ -2340,8 +2411,11 @@ describe('SavedObjectsRepository', () => { ); const mockedBulkResponse = getMockEsBulkDeleteResponse([testObject], { namespace }); client.bulk.mockResolvedValueOnce(mockedBulkResponse); + mockDeleteLegacyUrlAliases.mockRejectedValueOnce(new Error('Oh no!')); + await savedObjectsRepository.bulkDelete([testObject], { namespace }); + expect(client.mget).toHaveBeenCalledTimes(1); expect(logger.error).toHaveBeenCalledTimes(1); expect(logger.error).toHaveBeenCalledWith( @@ -2351,99 +2425,6 @@ describe('SavedObjectsRepository', () => { }); describe('errors', () => { - const expectRepositoryBulkDeleteStatusError = ({ - type, - id, - error, - }: { - type: string; - id: string; - error?: any; - }) => ({ - type, - id, - success: false, - error: error ?? createBadRequestError(), - }); - const repositoryBulkDeleteError = async ( - obj: SavedObjectsBulkDeleteObject, - isBulkError: boolean, - expectedErrorResult: ExpectedErrorResult - ) => { - const objects = [obj1, obj, obj2]; - const mockedBulkDeleteResponse = getMockEsBulkDeleteResponse(objects); - if (isBulkError) { - mockGetBulkOperationError.mockReturnValueOnce(undefined); - mockGetBulkOperationError.mockReturnValueOnce(expectedErrorResult.error as Payload); - } - client.bulk.mockResponseOnce(mockedBulkDeleteResponse); - - const result = await savedObjectsRepository.bulkDelete(objects); - expect(client.bulk).toHaveBeenCalled(); - expect(result).toEqual({ - statuses: [ - { ...obj1, success: true }, - { ...expectedErrorResult, success: false }, - { ...obj2, success: true }, - ], - }); - }; - const expectClientCallArgsAction = ( - objects: TypeIdTuple[], - { - method, - _index = expect.any(String), - getId = () => expect.any(String), - overrides = {}, - }: { - method: string; - _index?: string; - getId?: (type: string, id: string) => string; - overrides?: Record; - } - ) => { - const body = []; - for (const { type, id } of objects) { - body.push({ - [method]: { - _index, - _id: getId(type, id), - ...overrides, - }, - }); - } - expect(client.bulk).toHaveBeenCalledWith( - expect.objectContaining({ body }), - expect.anything() - ); - }; - const bulkDeleteMultiError = async ( - [obj1, _obj, obj2]: SavedObjectsBulkDeleteObject[], - options: SavedObjectsBulkDeleteOptions | undefined, - mgetResponse: estypes.MgetResponse, - mgetOptions?: { statusCode?: number } - ) => { - const getId = (type: string, id: string) => `${options?.namespace}:${type}:${id}`; - // mock the response for the not found doc - client.mget.mockResponseOnce(mgetResponse, { statusCode: mgetOptions?.statusCode }); - // get a mocked response for the valid docs - const bulkResponse = getMockEsBulkDeleteResponse([obj1, obj2], { namespace }); - client.bulk.mockResponseOnce(bulkResponse); - - const result = await savedObjectsRepository.bulkDelete([obj1, _obj, obj2], options); - expect(client.bulk).toHaveBeenCalledTimes(1); - expect(client.mget).toHaveBeenCalledTimes(1); - - expectClientCallArgsAction([obj1, obj2], { method: 'delete', getId }); - expect(result).toEqual({ - statuses: [ - { ...obj1, success: true }, - { ...expectErrorNotFound(_obj), success: false }, - { ...obj2, success: true }, - ], - }); - }; - it(`throws an error when options.namespace is '*'`, async () => { await expect( savedObjectsRepository.bulkDelete([obj1], { namespace: ALL_NAMESPACES_STRING }) @@ -2481,12 +2462,12 @@ describe('SavedObjectsRepository', () => { it(`returns an error when ES is unable to find the document during mget`, async () => { const notFoundObj = { ...obj1, type: MULTI_NAMESPACE_ISOLATED_TYPE, found: false }; const mgetResponse = getMockMgetResponse([notFoundObj], namespace); - await bulkDeleteMultiError([obj1, notFoundObj, obj2], { namespace }, mgetResponse); + await bulkDeleteMultiNamespaceError([obj1, notFoundObj, obj2], { namespace }, mgetResponse); }); it(`returns an error when ES is unable to find the index during mget`, async () => { const notFoundObj = { ...obj1, type: MULTI_NAMESPACE_ISOLATED_TYPE, found: false }; - await bulkDeleteMultiError( + await bulkDeleteMultiNamespaceError( [obj1, notFoundObj, obj2], { namespace }, {} as estypes.MgetResponse, @@ -2503,7 +2484,7 @@ describe('SavedObjectsRepository', () => { namespace: 'bar-namespace', }; const mgetResponse = getMockMgetResponse([obj], namespace); - await bulkDeleteMultiError([obj1, obj, obj2], { namespace }, mgetResponse); + await bulkDeleteMultiNamespaceError([obj1, obj, obj2], { namespace }, mgetResponse); }); it(`returns an error when the type is multi-namespace and the document has multiple namespaces and the force option is not enabled`, async () => { @@ -2520,7 +2501,7 @@ describe('SavedObjectsRepository', () => { internalOptions ); expect(result.statuses[0]).toStrictEqual( - expectRepositoryBulkDeleteStatusError({ + createBulkDeleteFailStatus({ ...testObject, error: createBadRequestError( 'Unable to delete saved object that exists in multiple namespaces, use the "force" option to delete it anyway' @@ -2543,7 +2524,7 @@ describe('SavedObjectsRepository', () => { internalOptions ); expect(result.statuses[0]).toStrictEqual( - expectRepositoryBulkDeleteStatusError({ + createBulkDeleteFailStatus({ ...testObject, error: createBadRequestError("Unsupported saved object type: '*'"), }) @@ -2552,69 +2533,6 @@ describe('SavedObjectsRepository', () => { }); describe('returns', () => { - const expectSuccessResult = ({ type, id }: SavedObjectsBulkDeleteObject) => ({ - type, - id, - success: true, - }); - - const expectClientCallArgsAction = ( - objects: TypeIdTuple[], - { - method, - _index = expect.any(String), - getId = () => expect.any(String), - overrides = {}, - }: { - method: string; - _index?: string; - getId?: (type: string, id: string) => string; - overrides?: Record; - } - ) => { - const body = []; - for (const { type, id } of objects) { - body.push({ - [method]: { - _index, - _id: getId(type, id), - ...overrides, - }, - }); - } - expect(client.bulk).toHaveBeenCalledWith( - expect.objectContaining({ body }), - expect.anything() - ); - }; - - const bulkDeleteMultiError = async ( - [obj1, _obj, obj2]: SavedObjectsBulkDeleteObject[], - options: SavedObjectsBulkDeleteOptions | undefined, - mgetResponse: estypes.MgetResponse, - mgetOptions?: { statusCode?: number } - ) => { - const getId = (type: string, id: string) => `${options?.namespace}:${type}:${id}`; - // mock the response for the not found doc - client.mget.mockResponseOnce(mgetResponse, { statusCode: mgetOptions?.statusCode }); - // get a mocked response for the valid docs - const bulkResponse = getMockEsBulkDeleteResponse([obj1, obj2], { namespace }); - client.bulk.mockResponseOnce(bulkResponse); - - const result = await savedObjectsRepository.bulkDelete([obj1, _obj, obj2], options); - expect(client.bulk).toHaveBeenCalledTimes(1); - expect(client.mget).toHaveBeenCalledTimes(1); - - expectClientCallArgsAction([obj1, obj2], { method: 'delete', getId }); - expect(result).toEqual({ - statuses: [ - { ...obj1, success: true }, - { ...expectErrorNotFound(_obj), success: false }, - { ...obj2, success: true }, - ], - }); - }; - it(`returns early for empty objects argument`, async () => { await savedObjectsRepository.bulkDelete([], { namespace }); expect(client.bulk).toHaveBeenCalledTimes(0); @@ -2623,13 +2541,13 @@ describe('SavedObjectsRepository', () => { it(`formats the ES response`, async () => { const response = await repositoryBulkDeleteSuccess([obj1, obj2], { namespace }); expect(response).toEqual({ - statuses: [obj1, obj2].map(expectSuccessResult), + statuses: [obj1, obj2].map(createBulkDeleteSuccessStatus), }); }); it(`handles a mix of successful deletes and errors`, async () => { const notFoundObj = { ...obj1, type: MULTI_NAMESPACE_ISOLATED_TYPE, found: false }; - await bulkDeleteMultiError( + await bulkDeleteMultiNamespaceError( [obj1, notFoundObj, obj2], { namespace }, {} as estypes.MgetResponse, From 7b31c8cc8999bff1c9f2c16f819a70066230b473 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Thu, 8 Sep 2022 16:58:01 -0700 Subject: [PATCH 32/49] Starts extracting some parts of bulkDelete implementation --- .../src/lib/repository.ts | 272 ++++++++++-------- 1 file changed, 155 insertions(+), 117 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 54a0196a64383..801c2c6ea48e1 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -773,43 +773,44 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } /** - * {@inheritDoc ISavedObjectsRepository.bulkDelete} + * performs initial checks on object type validity and flags multi-namespace objects for preflight checks + * @param objects SavedObjectsBulkDeleteObject[] + * @returns array BulkDeleteExpectedBulkGetResult[]: left (error) or right Either result */ - async bulkDelete( - objects: SavedObjectsBulkDeleteObject[], - options: SavedObjectsBulkDeleteOptions = {} - ): Promise { - const { refresh = DEFAULT_REFRESH_SETTING, force } = options; - const namespace = normalizeNamespace(options.namespace); + private presortObjectsByNamespaceType(objects: SavedObjectsBulkDeleteObject[]) { let bulkGetRequestIndexCounter = 0; - - const expectedBulkGetAllDocsWithNamespaceChecksFlag = - objects.map((object) => { - const { type, id } = object; - if (!this._allowedTypes.includes(type)) { - return { - tag: 'Left', - value: { - id, - type, - error: errorContent(SavedObjectsErrorHelpers.createUnsupportedTypeError(type)), - }, - }; - } - const requiresNamespacesCheck = this._registry.isMultiNamespace(type); + return objects.map((object) => { + const { type, id } = object; + if (!this._allowedTypes.includes(type)) { return { - tag: 'Right', + tag: 'Left', value: { - type, id, - ...(requiresNamespacesCheck && { esRequestIndex: bulkGetRequestIndexCounter++ }), + type, + error: errorContent(SavedObjectsErrorHelpers.createUnsupportedTypeError(type)), }, }; - }); + } + const requiresNamespacesCheck = this._registry.isMultiNamespace(type); + return { + tag: 'Right', + value: { + type, + id, + ...(requiresNamespacesCheck && { esRequestIndex: bulkGetRequestIndexCounter++ }), + }, + }; + }); + } - // get multi-namespace saved objects only to minimise the number of docs to fetch for the preflight multinamespace objects. - // Create params for an mget request only from docs that need a multi-namespace preflight check - const bulkGetMultiNamespaceDocs = expectedBulkGetAllDocsWithNamespaceChecksFlag + /** + * Fetch multi-namespace saved objects + * @returns MgetResponse + * @internalNotes multinamespace objects shared to more than one space require special handling. We fetch these docs to retrieve their namespaces. + */ + private async preflightCheckForBulkDelete(params: PreflightCheckForBulkDeleteParams) { + const { expectedBulkGetResults, namespace } = params; + const bulkGetMultiNamespaceDocs = expectedBulkGetResults .filter(isRight) .filter(({ value }) => value.esRequestIndex !== undefined) // only get docs that need multinamespace checks, I don't think we want to do this filtering .map(({ value: { type, id } }) => ({ @@ -834,97 +835,137 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { ) { throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(); } + return bulkGetMultiNamespaceDocsResponse; + } - let bulkDeleteRequestIndexCounter = 0; - const bulkDeleteParams: BulkDeleteParams[] = []; - + /** + * + * @param params: ExpectedBulkDeleteMultiNamespaceDocsParams + * @returns array of objects sorted by expected delete success or failure result + */ + private getExpectedBulkDeleteMultiNamespaceDocsResults( + params: ExpectedBulkDeleteMultiNamespaceDocsParams + ): ExpectedBulkDeleteResult[] { + const { expectedBulkGetResults, multiNamespaceDocsResponse, namespace, force } = params; + let indexCounter = 0; const expectedBulkDeleteMultiNamespaceDocsResults = - expectedBulkGetAllDocsWithNamespaceChecksFlag.map( - (expectedBulkGetResult) => { - if (isLeft(expectedBulkGetResult)) { - return { ...expectedBulkGetResult }; + expectedBulkGetResults.map((expectedBulkGetResult) => { + if (isLeft(expectedBulkGetResult)) { + return { ...expectedBulkGetResult }; + } + const { esRequestIndex: esBulkGetRequestIndex, id, type } = expectedBulkGetResult.value; + + let namespaces; + + if (esBulkGetRequestIndex !== undefined) { + const indexFound = multiNamespaceDocsResponse?.statusCode !== 404; + + const actualResult = indexFound + ? multiNamespaceDocsResponse?.body.docs[esBulkGetRequestIndex] + : undefined; + + const docFound = indexFound && isMgetDoc(actualResult) && actualResult.found; + + // return an error if the doc isnn't found at all or the doc doesn't exist in the namespaces + if (!docFound) { + return { + tag: 'Left', + value: { + id, + type, + error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), + }, + }; } - const { esRequestIndex: esBulkGetRequestIndex, id, type } = expectedBulkGetResult.value; - - let namespaces; - - if (esBulkGetRequestIndex !== undefined) { - const indexFound = bulkGetMultiNamespaceDocsResponse?.statusCode !== 404; - - const actualResult = indexFound - ? bulkGetMultiNamespaceDocsResponse?.body.docs[esBulkGetRequestIndex] - : undefined; - - const docFound = indexFound && isMgetDoc(actualResult) && actualResult.found; - - // return an error if the doc isnn't found at all or the doc doesn't exist in the namespaces - if (!docFound) { - return { - tag: 'Left', - value: { - id, - type, - error: errorContent( - SavedObjectsErrorHelpers.createGenericNotFoundError(type, id) - ), - }, - }; - } - // the following check should be redundant since we're retrieving the docs from elasticsearch but we check anyway - // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - if (docFound && !this.rawDocExistsInNamespace(actualResult, namespace)) { - return { - tag: 'Left', - value: { - id, - type, - error: errorContent( - SavedObjectsErrorHelpers.createGenericNotFoundError(type, id) - ), - }, - }; - } - // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - namespaces = actualResult!._source.namespaces ?? [ - SavedObjectsUtils.namespaceIdToString(namespace), - ]; - - // the document exists but is multinamespace and there are more than one namespaces present and can only be deleted by force. - if (!force && (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING))) { - return { - tag: 'Left', - value: { - success: false, - id, - type, - error: errorContent( - SavedObjectsErrorHelpers.createBadRequestError( - `Unable to delete saved object that exists in multiple namespaces, use the "force" option to delete it anyway` - ) - ), - }, - }; - } + // the following check should be redundant since we're retrieving the docs from elasticsearch but we check anyway + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + if (docFound && !this.rawDocExistsInNamespace(actualResult, namespace)) { + return { + tag: 'Left', + value: { + id, + type, + error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), + }, + }; } + // @ts-expect-error MultiGetHit is incorrectly missing _id, _source + namespaces = actualResult!._source.namespaces ?? [ + SavedObjectsUtils.namespaceIdToString(namespace), + ]; - const expectedResult = { - type, - id, - namespaces, - esRequestIndex: bulkDeleteRequestIndexCounter++, - }; + // the document is shared to more than one space and can only be deleted by force. + if (!force && (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING))) { + return { + tag: 'Left', + value: { + success: false, + id, + type, + error: errorContent( + SavedObjectsErrorHelpers.createBadRequestError( + `Unable to delete saved object that exists in multiple namespaces, use the "force" option to delete it anyway` + ) + ), + }, + }; + } + } - bulkDeleteParams.push({ - delete: { - _id: this._serializer.generateRawId(namespace, type, id), - _index: this.getIndexForType(type), - ...getExpectedVersionProperties(undefined), - }, - }); + const expectedResult = { + type, + id, + namespaces, + esRequestIndex: indexCounter++, + }; - return { tag: 'Right', value: expectedResult }; - } - ); + return { tag: 'Right', value: expectedResult }; + }); + return expectedBulkDeleteMultiNamespaceDocsResults; + } + + /** + * {@inheritDoc ISavedObjectsRepository.bulkDelete} + */ + async bulkDelete( + objects: SavedObjectsBulkDeleteObject[], + options: SavedObjectsBulkDeleteOptions = {} + ): Promise { + const { refresh = DEFAULT_REFRESH_SETTING, force } = options; + const namespace = normalizeNamespace(options.namespace); + const expectedBulkGetResults = this.presortObjectsByNamespaceType(objects); + + const multiNamespaceDocsResponse = await this.preflightCheckForBulkDelete({ + expectedBulkGetResults, + namespace, + }); + + const bulkDeleteParams: BulkDeleteParams[] = []; + + // @TINA expectedBulkDeleteMultiNamespaceDocsResults is result from sorting again into Left and Right responses. Use the Right responses to create the bulk params + const expectedBulkDeleteMultiNamespaceDocsResults = + this.getExpectedBulkDeleteMultiNamespaceDocsResults({ + expectedBulkGetResults, + multiNamespaceDocsResponse, + namespace, + force, + }); + + expectedBulkDeleteMultiNamespaceDocsResults.map((expectedResult) => { + if (isRight(expectedResult)) { + bulkDeleteParams.push({ + delete: { + _id: this._serializer.generateRawId( + namespace, + expectedResult.value.type, + expectedResult.value.id + ), + _index: this.getIndexForType(expectedResult.value.type), + ...getExpectedVersionProperties(undefined), + }, + }); + } + }); const bulkDeleteResponse = bulkDeleteParams.length ? await this.client.bulk({ @@ -940,8 +981,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { const savedObjects = await Promise.all( expectedBulkDeleteMultiNamespaceDocsResults.map(async (expectedResult) => { if (isLeft(expectedResult)) { - errorResult = { ...expectedResult.value, success: false }; - return errorResult; + return { ...expectedResult.value, success: false }; } const { type, @@ -949,14 +989,12 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { namespaces, esRequestIndex: esBulkDeleteRequestIndex, } = expectedResult.value; - // we assume this wouldn't happen but is needed to ensure type consistency if (bulkDeleteResponse === undefined) { throw new Error( `Unexpected error in bulkDelete saved objects: bulkDeleteResponse is undefined` ); } - const rawResponse = Object.values( bulkDeleteResponse.items[esBulkDeleteRequestIndex] )[0] as NewBulkItemResponse; From 41f769fbb53be3ad900187267e1f3388f065b404 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Thu, 8 Sep 2022 16:59:19 -0700 Subject: [PATCH 33/49] Update internal bulk Delete types --- .../src/lib/repository.ts | 2 ++ .../repository_bulk_delete_internal_types.ts | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 801c2c6ea48e1..f2bf411e1694c 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -118,6 +118,8 @@ import type { BulkDeleteItemErrorResult, NewBulkItemResponse, BulkDeleteExpectedBulkGetResult, + PreflightCheckForBulkDeleteParams, + ExpectedBulkDeleteMultiNamespaceDocsParams, } from './repository_bulk_delete_internal_types'; // BEWARE: The SavedObjectClient depends on the implementation details of the SavedObjectsRepository diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts index 2504ea524b07e..19dd11c71b66c 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts @@ -11,8 +11,26 @@ import { BulkResponseItem, ErrorCause, } from '@elastic/elasticsearch/lib/api/types'; +import type { estypes, TransportResult } from '@elastic/elasticsearch'; import { Either } from './internal_utils'; +/** + * @internal + */ +export interface PreflightCheckForBulkDeleteParams { + expectedBulkGetResults: BulkDeleteExpectedBulkGetResult[]; + namespace?: string; +} + +/** + * @internal + */ +export interface ExpectedBulkDeleteMultiNamespaceDocsParams { + expectedBulkGetResults: BulkDeleteExpectedBulkGetResult[]; + multiNamespaceDocsResponse: TransportResult, unknown> | undefined; + namespace: string | undefined; + force?: boolean; +} /** * @internal */ From 213c6ca51b9266d3e888ddcee989519bddc6a4d7 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Fri, 9 Sep 2022 10:15:23 -0700 Subject: [PATCH 34/49] Adds bulk_delete as a saved object write operation priviledge --- .../feature_privilege_builder/saved_object.ts | 1 + .../privileges/privileges.test.ts | 53 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/saved_object.ts b/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/saved_object.ts index 330eebc1602b4..3ba81321ca9ee 100644 --- a/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/saved_object.ts +++ b/x-pack/plugins/security/server/authorization/privileges/feature_privilege_builder/saved_object.ts @@ -24,6 +24,7 @@ const writeOperations: string[] = [ 'update', 'bulk_update', 'delete', + 'bulk_delete', 'share_to_space', ]; const allOperations: string[] = [...readOperations, ...writeOperations]; diff --git a/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts b/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts index d9c82403a582a..1a8d7814e5a38 100644 --- a/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts +++ b/x-pack/plugins/security/server/authorization/privileges/privileges.test.ts @@ -109,6 +109,7 @@ describe('features', () => { actions.savedObject.get('all-savedObject-all-1', 'update'), actions.savedObject.get('all-savedObject-all-1', 'bulk_update'), actions.savedObject.get('all-savedObject-all-1', 'delete'), + actions.savedObject.get('all-savedObject-all-1', 'bulk_delete'), actions.savedObject.get('all-savedObject-all-1', 'share_to_space'), actions.savedObject.get('all-savedObject-all-2', 'bulk_get'), actions.savedObject.get('all-savedObject-all-2', 'get'), @@ -120,6 +121,7 @@ describe('features', () => { actions.savedObject.get('all-savedObject-all-2', 'update'), actions.savedObject.get('all-savedObject-all-2', 'bulk_update'), actions.savedObject.get('all-savedObject-all-2', 'delete'), + actions.savedObject.get('all-savedObject-all-2', 'bulk_delete'), actions.savedObject.get('all-savedObject-all-2', 'share_to_space'), actions.savedObject.get('all-savedObject-read-1', 'bulk_get'), actions.savedObject.get('all-savedObject-read-1', 'get'), @@ -148,6 +150,7 @@ describe('features', () => { actions.savedObject.get('read-savedObject-all-1', 'update'), actions.savedObject.get('read-savedObject-all-1', 'bulk_update'), actions.savedObject.get('read-savedObject-all-1', 'delete'), + actions.savedObject.get('read-savedObject-all-1', 'bulk_delete'), actions.savedObject.get('read-savedObject-all-1', 'share_to_space'), actions.savedObject.get('read-savedObject-all-2', 'bulk_get'), actions.savedObject.get('read-savedObject-all-2', 'get'), @@ -159,6 +162,7 @@ describe('features', () => { actions.savedObject.get('read-savedObject-all-2', 'update'), actions.savedObject.get('read-savedObject-all-2', 'bulk_update'), actions.savedObject.get('read-savedObject-all-2', 'delete'), + actions.savedObject.get('read-savedObject-all-2', 'bulk_delete'), actions.savedObject.get('read-savedObject-all-2', 'share_to_space'), actions.savedObject.get('read-savedObject-read-1', 'bulk_get'), actions.savedObject.get('read-savedObject-read-1', 'get'), @@ -301,6 +305,7 @@ describe('features', () => { actions.savedObject.get('all-savedObject-all-1', 'update'), actions.savedObject.get('all-savedObject-all-1', 'bulk_update'), actions.savedObject.get('all-savedObject-all-1', 'delete'), + actions.savedObject.get('all-savedObject-all-1', 'bulk_delete'), actions.savedObject.get('all-savedObject-all-1', 'share_to_space'), actions.savedObject.get('all-savedObject-all-2', 'bulk_get'), actions.savedObject.get('all-savedObject-all-2', 'get'), @@ -312,6 +317,7 @@ describe('features', () => { actions.savedObject.get('all-savedObject-all-2', 'update'), actions.savedObject.get('all-savedObject-all-2', 'bulk_update'), actions.savedObject.get('all-savedObject-all-2', 'delete'), + actions.savedObject.get('all-savedObject-all-2', 'bulk_delete'), actions.savedObject.get('all-savedObject-all-2', 'share_to_space'), actions.savedObject.get('all-savedObject-read-1', 'bulk_get'), actions.savedObject.get('all-savedObject-read-1', 'get'), @@ -339,6 +345,7 @@ describe('features', () => { actions.savedObject.get('read-savedObject-all-1', 'update'), actions.savedObject.get('read-savedObject-all-1', 'bulk_update'), actions.savedObject.get('read-savedObject-all-1', 'delete'), + actions.savedObject.get('read-savedObject-all-1', 'bulk_delete'), actions.savedObject.get('read-savedObject-all-1', 'share_to_space'), actions.savedObject.get('read-savedObject-all-2', 'bulk_get'), actions.savedObject.get('read-savedObject-all-2', 'get'), @@ -350,6 +357,7 @@ describe('features', () => { actions.savedObject.get('read-savedObject-all-2', 'update'), actions.savedObject.get('read-savedObject-all-2', 'bulk_update'), actions.savedObject.get('read-savedObject-all-2', 'delete'), + actions.savedObject.get('read-savedObject-all-2', 'bulk_delete'), actions.savedObject.get('read-savedObject-all-2', 'share_to_space'), actions.savedObject.get('read-savedObject-read-1', 'bulk_get'), actions.savedObject.get('read-savedObject-read-1', 'get'), @@ -427,6 +435,7 @@ describe('features', () => { actions.savedObject.get('read-savedObject-all-1', 'update'), actions.savedObject.get('read-savedObject-all-1', 'bulk_update'), actions.savedObject.get('read-savedObject-all-1', 'delete'), + actions.savedObject.get('read-savedObject-all-1', 'bulk_delete'), actions.savedObject.get('read-savedObject-all-1', 'share_to_space'), actions.savedObject.get('read-savedObject-all-2', 'bulk_get'), actions.savedObject.get('read-savedObject-all-2', 'get'), @@ -438,6 +447,7 @@ describe('features', () => { actions.savedObject.get('read-savedObject-all-2', 'update'), actions.savedObject.get('read-savedObject-all-2', 'bulk_update'), actions.savedObject.get('read-savedObject-all-2', 'delete'), + actions.savedObject.get('read-savedObject-all-2', 'bulk_delete'), actions.savedObject.get('read-savedObject-all-2', 'share_to_space'), actions.savedObject.get('read-savedObject-read-1', 'bulk_get'), actions.savedObject.get('read-savedObject-read-1', 'get'), @@ -732,6 +742,7 @@ describe('reserved', () => { actions.savedObject.get('savedObject-all-1', 'update'), actions.savedObject.get('savedObject-all-1', 'bulk_update'), actions.savedObject.get('savedObject-all-1', 'delete'), + actions.savedObject.get('savedObject-all-1', 'bulk_delete'), actions.savedObject.get('savedObject-all-1', 'share_to_space'), actions.savedObject.get('savedObject-all-2', 'bulk_get'), actions.savedObject.get('savedObject-all-2', 'get'), @@ -743,6 +754,7 @@ describe('reserved', () => { actions.savedObject.get('savedObject-all-2', 'update'), actions.savedObject.get('savedObject-all-2', 'bulk_update'), actions.savedObject.get('savedObject-all-2', 'delete'), + actions.savedObject.get('savedObject-all-2', 'bulk_delete'), actions.savedObject.get('savedObject-all-2', 'share_to_space'), actions.savedObject.get('savedObject-read-1', 'bulk_get'), actions.savedObject.get('savedObject-read-1', 'get'), @@ -862,6 +874,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -993,6 +1006,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1015,6 +1029,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1044,6 +1059,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1081,6 +1097,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1104,6 +1121,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1127,6 +1145,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1149,6 +1168,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1227,6 +1247,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1249,6 +1270,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1278,6 +1300,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1384,6 +1407,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1406,6 +1430,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1455,6 +1480,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1484,6 +1510,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1567,6 +1594,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1589,6 +1617,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1709,6 +1738,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1738,6 +1768,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1775,6 +1806,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1798,6 +1830,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1821,6 +1854,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1843,6 +1877,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1941,6 +1976,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -1970,6 +2006,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2007,6 +2044,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2030,6 +2068,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2053,6 +2092,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2075,6 +2115,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2173,6 +2214,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_get'), actions.savedObject.get('all-licensed-sub-feature-type', 'get'), @@ -2184,6 +2226,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-licensed-sub-feature-type', 'update'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-licensed-sub-feature-type', 'delete'), + actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-licensed-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2219,6 +2262,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_get'), actions.savedObject.get('all-licensed-sub-feature-type', 'get'), @@ -2230,6 +2274,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-licensed-sub-feature-type', 'update'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-licensed-sub-feature-type', 'delete'), + actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-licensed-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2273,6 +2318,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_get'), actions.savedObject.get('all-licensed-sub-feature-type', 'get'), @@ -2284,6 +2330,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-licensed-sub-feature-type', 'update'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-licensed-sub-feature-type', 'delete'), + actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-licensed-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2313,6 +2360,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_get'), actions.savedObject.get('all-licensed-sub-feature-type', 'get'), @@ -2324,6 +2372,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-licensed-sub-feature-type', 'update'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-licensed-sub-feature-type', 'delete'), + actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-licensed-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2353,6 +2402,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_get'), actions.savedObject.get('all-licensed-sub-feature-type', 'get'), @@ -2364,6 +2414,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-licensed-sub-feature-type', 'update'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-licensed-sub-feature-type', 'delete'), + actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-licensed-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), @@ -2392,6 +2443,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-sub-feature-type', 'update'), actions.savedObject.get('all-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-sub-feature-type', 'delete'), + actions.savedObject.get('all-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-sub-feature-type', 'share_to_space'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_get'), actions.savedObject.get('all-licensed-sub-feature-type', 'get'), @@ -2403,6 +2455,7 @@ describe('subFeatures', () => { actions.savedObject.get('all-licensed-sub-feature-type', 'update'), actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_update'), actions.savedObject.get('all-licensed-sub-feature-type', 'delete'), + actions.savedObject.get('all-licensed-sub-feature-type', 'bulk_delete'), actions.savedObject.get('all-licensed-sub-feature-type', 'share_to_space'), actions.savedObject.get('read-sub-feature-type', 'bulk_get'), actions.savedObject.get('read-sub-feature-type', 'get'), From e775569340118df22e8bcded6b211e75c3af0359 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 12 Sep 2022 15:16:57 -0700 Subject: [PATCH 35/49] Fixes bug with deleting objects across spaces --- .../src/lib/repository.ts | 21 +++++++++++-------- .../repository_bulk_delete_internal_types.ts | 5 +++++ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index f2bf411e1694c..26e241b9a1a5b 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -775,12 +775,12 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } /** - * performs initial checks on object type validity and flags multi-namespace objects for preflight checks + * performs initial checks on object type validity and flags multi-namespace objects for preflight checks by adding an `esRequestIndex` * @param objects SavedObjectsBulkDeleteObject[] - * @returns array BulkDeleteExpectedBulkGetResult[]: left (error) or right Either result + * @returns array BulkDeleteExpectedBulkGetResult[]: left as 400 for objects that don't have a valid type or right Either result with the object and an `esRequestIndex` if the object is of a multinamespace type. */ private presortObjectsByNamespaceType(objects: SavedObjectsBulkDeleteObject[]) { - let bulkGetRequestIndexCounter = 0; + let bulkGetRequestIndexCounter = 0; // flag for multi-namespace objects return objects.map((object) => { const { type, id } = object; if (!this._allowedTypes.includes(type)) { @@ -816,7 +816,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { .filter(isRight) .filter(({ value }) => value.esRequestIndex !== undefined) // only get docs that need multinamespace checks, I don't think we want to do this filtering .map(({ value: { type, id } }) => ({ - _id: this._serializer.generateRawId(namespace, type, id), + _id: this._serializer.generateRawId(namespace, type, id), // prefixes the id with the space name in the api call's scope _index: this.getIndexForType(type), _source: ['type', 'namespaces'], })); @@ -835,6 +835,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { headers: bulkGetMultiNamespaceDocsResponse.headers, }) ) { + // I'm not sure if I want to throw here. throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(); } return bulkGetMultiNamespaceDocsResponse; @@ -868,7 +869,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { const docFound = indexFound && isMgetDoc(actualResult) && actualResult.found; - // return an error if the doc isnn't found at all or the doc doesn't exist in the namespaces + // return an error if the doc isn't found at all or the doc doesn't exist in the namespaces if (!docFound) { return { tag: 'Left', @@ -879,7 +880,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { }, }; } - // the following check should be redundant since we're retrieving the docs from elasticsearch but we check anyway + // the following check should be redundant since we're retrieving the docs from elasticsearch but we check just to make sure // @ts-expect-error MultiGetHit is incorrectly missing _id, _source if (docFound && !this.rawDocExistsInNamespace(actualResult, namespace)) { return { @@ -913,7 +914,8 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { }; } } - + // contains all objects that passed initial preflight checks, including single namespace objects that skipped the mget call + // single namespace objects will have namespaces:undefined const expectedResult = { type, id, @@ -1010,6 +1012,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { if (deleted) { // `namespaces` should only exist in the expectedResult.value if the type is multinamespace. if (namespaces) { + // in the bulk operation, one cannot specify a namespace from which to delete an object other than the namespace that the operation is performed in. If a multinamespace object exists in more than the current space (from which the call is made), force deleting the object will delete it from all namespaces it exists in. In that case, all legacy url aliases are deleted as well. If force isn't applied, the operation fails and the object isn't deleted. await deleteLegacyUrlAliases({ mappings: this._mappings, registry: this._registry, @@ -1019,7 +1022,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { id, ...(namespaces.includes(ALL_NAMESPACES_STRING) ? { namespaces: [], deleteBehavior: 'exclusive' } // delete legacy URL aliases for this type/ID for all spaces - : { namespaces, deleteBehavior: 'inclusive' }), // delete legacy URL aliases for this type/ID for these specific spaces + : { namespaces, deleteBehavior: 'inclusive' }), // delete legacy URL aliases for this type/ID for these specific spaces. In the bulk operation, this behavior is only applicable in the case of multi-namespace isolated types. }).catch((err) => { // The object has already been deleted, but we caught an error when attempting to delete aliases. // A consumer cannot attempt to delete the object again, so just log the error and swallow it. @@ -1030,7 +1033,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } } const successfulResult = { - success: true, + success: true, // could also use (rawResponse.result === 'deleted') id, type, }; diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts index 19dd11c71b66c..97c7650fbfb81 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts @@ -26,9 +26,13 @@ export interface PreflightCheckForBulkDeleteParams { * @internal */ export interface ExpectedBulkDeleteMultiNamespaceDocsParams { + // contains the type and id of all objects to delete expectedBulkGetResults: BulkDeleteExpectedBulkGetResult[]; + // subset of multi-namespace only expectedBulkGetResults multiNamespaceDocsResponse: TransportResult, unknown> | undefined; + // current namespace in which the bulkDelete call is made namespace: string | undefined; + // optional parameter used to force delete multinamespace objects that exist in more than the current space force?: boolean; } /** @@ -68,6 +72,7 @@ export type NewBulkItemResponse = BulkResponseItem & { error: ErrorCause & { ind /** * @internal + * contains all documents for bulk delete, regardless of namespace type. */ export type BulkDeleteExpectedBulkGetResult = Either< { type: string; id: string; error: Payload }, From 86dcd8cc3230e66dc6e081fb9a187c67807394ce Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 12 Sep 2022 15:31:09 -0700 Subject: [PATCH 36/49] Adds api integration tests for bulk_delete against spaces only --- .../common/suites/bulk_delete.ts | 150 ++++++++++++++++++ .../spaces_only/apis/bulk_delete.ts | 68 ++++++++ .../spaces_only/apis/index.ts | 1 + 3 files changed, 219 insertions(+) create mode 100644 x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts create mode 100644 x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_delete.ts diff --git a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts new file mode 100644 index 0000000000000..918a21d9fe944 --- /dev/null +++ b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts @@ -0,0 +1,150 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { SuperTest } from 'supertest'; +import type { Client } from '@elastic/elasticsearch'; +import expect from '@kbn/expect'; +import type { SearchTotalHits } from '@elastic/elasticsearch/lib/api/types'; +import { SAVED_OBJECT_TEST_CASES as CASES } from '../lib/saved_object_test_cases'; +import { SPACES } from '../lib/spaces'; +import { expectResponses, getUrlPrefix, getTestTitle } from '../lib/saved_object_test_utils'; +import { ExpectResponseBody, TestCase, TestDefinition, TestSuite, TestUser } from '../lib/types'; + +export interface BulkDeleteTestDefinition extends TestDefinition { + request: { type: string; id: string; force?: boolean }; + force?: boolean; +} +export type BulkDeleteTestSuite = TestSuite; + +export interface BulkDeleteTestCase extends TestCase { + force?: boolean; + failure?: 400 | 403 | 404; +} + +const ALIAS_DELETE_INCLUSIVE = Object.freeze({ type: 'resolvetype', id: 'alias-match-newid' }); // exists in three specific spaces; deleting this should also delete the aliases that target it in the default space and space_1 +const ALIAS_DELETE_EXCLUSIVE = Object.freeze({ type: 'resolvetype', id: 'all_spaces' }); // exists in all spaces; deleting this should also delete the aliases that target it in the default space and space_1 +const DOES_NOT_EXIST = Object.freeze({ type: 'dashboard', id: 'does-not-exist' }); +export const TEST_CASES: Record = Object.freeze({ + ...CASES, + ALIAS_DELETE_INCLUSIVE, + ALIAS_DELETE_EXCLUSIVE, + DOES_NOT_EXIST, +}); + +/** + * Test cases have additional properties that we don't want to send in HTTP Requests + */ +const createRequest = ({ type, id, force }: BulkDeleteTestCase) => ({ type, id, force }); + +export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertest: SuperTest) { + const expectSavedObjectForbidden = expectResponses.forbiddenTypes('bulk_delete'); + const expectResponseBody = + (testCase: BulkDeleteTestCase, statusCode: 200 | 403, user?: TestUser): ExpectResponseBody => + async (response: Record) => { + if (statusCode === 403) { + await expectSavedObjectForbidden(testCase.type)(response); + } else { + // permitted + const statuses = response.body.statuses; + expect(statuses).length([testCase].length); // hard coding the expectation now since I expect a response for each test case as a single request item in the array + for (let i = 0; i < statuses.length; i++) { + const object = statuses[i]; + expect(object).to.have.keys(['id', 'type', 'success']); + if (testCase.failure) { + const { type, id } = testCase; + expect(object.type).to.eql(type); + expect(object.id).to.eql(id); + await expectResponses.permitted(object, testCase); // takes the object (record) we're asserting against and uses the testCase's failure entry value to return the appropriate error. Asserts that the object is the expected error + } else { + await es.indices.refresh({ index: '.kibana' }); // alias deletion uses refresh: false, so we need to manually refresh the index before searching + const searchResponse = await es.search({ + index: '.kibana', + body: { + size: 0, + query: { terms: { type: ['legacy-url-alias'] } }, + track_total_hits: true, + }, + }); + + const numberOfAliasesDeleted = [ALIAS_DELETE_INCLUSIVE, ALIAS_DELETE_EXCLUSIVE].find( + ({ type, id }) => testCase.type === type && testCase.id === id + ); + // use a conditional check to guard against future exclusion of these test cases + // we also use a dynamically calculated numberic value to allow for changing the fixtures the tests run against. + if (Array.isArray(numberOfAliasesDeleted)) { + // Eight aliases exist and they are all deleted in the bulk operation since the delete behavior when using force is to delete the object from all spaces it exists in + expect((searchResponse.hits.total as SearchTotalHits).value).to.eql( + numberOfAliasesDeleted.length + ); + } + } + } + } + }; + + const createTestDefinitions = ( + testCases: BulkDeleteTestCase | BulkDeleteTestCase[], + forbidden: boolean, + options?: { + spaceId?: string; + responseBodyOverride?: ExpectResponseBody; + } + ): BulkDeleteTestDefinition[] => { + let cases = Array.isArray(testCases) ? testCases : [testCases]; + const responseStatusCode = forbidden ? 403 : 200; + if (forbidden) { + // override the expected result in each test case + cases = cases.map((x) => ({ ...x, failure: 403 })); + } + return cases.map((x) => ({ + title: getTestTitle(x, responseStatusCode), + responseStatusCode, + request: createRequest(x), + responseBody: options?.responseBodyOverride || expectResponseBody(x, responseStatusCode), + })); + }; + + const makeBulkDeleteTest = + (describeFn: Mocha.SuiteFunction) => (description: string, definition: BulkDeleteTestSuite) => { + const { user, spaceId = SPACES.DEFAULT.spaceId, tests } = definition; + + describeFn(description, () => { + before(() => + esArchiver.load( + 'x-pack/test/saved_object_api_integration/common/fixtures/es_archiver/saved_objects/spaces' + ) + ); + after(() => + esArchiver.unload( + 'x-pack/test/saved_object_api_integration/common/fixtures/es_archiver/saved_objects/spaces' + ) + ); + + for (const test of tests) { + it(`should return ${test.responseStatusCode} ${test.title} `, async () => { + const requestBody = [{ type: test.request.type, id: test.request.id }]; + const query = test.force && test.force === true ? '?force=true' : ''; + await supertest + .post(`${getUrlPrefix(spaceId)}/api/saved_objects/_bulk_delete${query}`) + .auth(user?.username, user?.password) + .send(requestBody) + .expect(test.responseStatusCode) + .then(test.responseBody); + }); + } + }); + }; + + const addTests = makeBulkDeleteTest(describe); + // @ts-ignore + addTests.only = makeBulkDeleteTest(describe.only); + + return { + addTests, + createTestDefinitions, + }; +} diff --git a/x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_delete.ts b/x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_delete.ts new file mode 100644 index 0000000000000..84f0c048f1a28 --- /dev/null +++ b/x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_delete.ts @@ -0,0 +1,68 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { SPACES } from '../../common/lib/spaces'; +import { testCaseFailures, getTestScenarios } from '../../common/lib/saved_object_test_utils'; +import { FtrProviderContext } from '../../common/ftr_provider_context'; +import { bulkDeleteTestSuiteFactory, TEST_CASES as CASES } from '../../common/suites/bulk_delete'; + +const { + DEFAULT: { spaceId: DEFAULT_SPACE_ID }, + SPACE_1: { spaceId: SPACE_1_ID }, + SPACE_2: { spaceId: SPACE_2_ID }, +} = SPACES; +const { fail400, fail404 } = testCaseFailures; + +const createTestCases = (spaceId: string) => [ + { ...CASES.SINGLE_NAMESPACE_DEFAULT_SPACE, ...fail404(spaceId !== DEFAULT_SPACE_ID) }, + { ...CASES.SINGLE_NAMESPACE_SPACE_1, ...fail404(spaceId !== SPACE_1_ID) }, + { ...CASES.SINGLE_NAMESPACE_SPACE_2, ...fail404(spaceId !== SPACE_2_ID) }, + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, ...fail400() }, + // // try to delete this object again, this time using the `force` option + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, force: true }, + { + ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, + ...fail400(spaceId === DEFAULT_SPACE_ID || spaceId === SPACE_1_ID), + ...fail404(spaceId !== DEFAULT_SPACE_ID && spaceId !== SPACE_1_ID), + }, + // try to delete this object again, this time using the `force` option + { + ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, + force: true, + }, + { ...CASES.MULTI_NAMESPACE_ONLY_SPACE_1, ...fail404(spaceId !== SPACE_1_ID) }, + { ...CASES.MULTI_NAMESPACE_ONLY_SPACE_2, ...fail404(spaceId !== SPACE_2_ID) }, + { + ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_DEFAULT_SPACE, + ...fail404(spaceId !== DEFAULT_SPACE_ID), + }, + { ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, ...fail404(spaceId !== SPACE_1_ID) }, + CASES.NAMESPACE_AGNOSTIC, + { ...CASES.ALIAS_DELETE_INCLUSIVE, force: true }, + { ...CASES.ALIAS_DELETE_EXCLUSIVE, force: true }, + { ...CASES.HIDDEN, ...fail400() }, + { ...CASES.DOES_NOT_EXIST, ...fail404() }, +]; + +export default function ({ getService }: FtrProviderContext) { + const supertest = getService('supertest'); + const esArchiver = getService('esArchiver'); + const es = getService('es'); + + const { addTests, createTestDefinitions } = bulkDeleteTestSuiteFactory(es, esArchiver, supertest); + const createTests = (spaceId: string) => { + const testCases = createTestCases(spaceId); + return createTestDefinitions(testCases, false, { spaceId }); + }; + + describe('_bulk_delete', () => { + getTestScenarios().spaces.forEach(({ spaceId }) => { + const tests = createTests(spaceId); + addTests(`within the ${spaceId} space`, { spaceId, tests }); + }); + }); +} diff --git a/x-pack/test/saved_object_api_integration/spaces_only/apis/index.ts b/x-pack/test/saved_object_api_integration/spaces_only/apis/index.ts index 1be7ed754a971..e17cdc43c16f9 100644 --- a/x-pack/test/saved_object_api_integration/spaces_only/apis/index.ts +++ b/x-pack/test/saved_object_api_integration/spaces_only/apis/index.ts @@ -15,6 +15,7 @@ export default function ({ loadTestFile }: FtrProviderContext) { loadTestFile(require.resolve('./bulk_update')); loadTestFile(require.resolve('./create')); loadTestFile(require.resolve('./delete')); + loadTestFile(require.resolve('./bulk_delete')); loadTestFile(require.resolve('./export')); loadTestFile(require.resolve('./find')); loadTestFile(require.resolve('./get')); From f9b19f8922ede08af715cd00d2f10f202208ea6a Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 12 Sep 2022 15:36:32 -0700 Subject: [PATCH 37/49] Actually adds the code to fix the bug --- .../src/lib/repository.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 26e241b9a1a5b..9af44e9f42480 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -1008,8 +1008,17 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { errorResult = { success: false, type, id, error }; return errorResult; } - const deleted = rawResponse.result === 'deleted'; - if (deleted) { + if (rawResponse.result === 'not_found') { + errorResult = { + success: false, + type, + id, + error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), + }; + return errorResult; + } + + if (rawResponse.result === 'deleted') { // `namespaces` should only exist in the expectedResult.value if the type is multinamespace. if (namespaces) { // in the bulk operation, one cannot specify a namespace from which to delete an object other than the namespace that the operation is performed in. If a multinamespace object exists in more than the current space (from which the call is made), force deleting the object will delete it from all namespaces it exists in. In that case, all legacy url aliases are deleted as well. If force isn't applied, the operation fails and the object isn't deleted. From 2d8a656b72cf335ece9269cb2c79f86ace2f4de2 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Tue, 13 Sep 2022 10:49:01 -0700 Subject: [PATCH 38/49] Adds bulk_delete integration tests for when security and spaces are enabled --- .../security_and_spaces/apis/bulk_delete.ts | 107 ++++++++++++++++++ .../security_and_spaces/apis/index.ts | 1 + 2 files changed, 108 insertions(+) create mode 100644 x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts diff --git a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts new file mode 100644 index 0000000000000..a6b6e1dd701b4 --- /dev/null +++ b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts @@ -0,0 +1,107 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { SPACES } from '../../common/lib/spaces'; +import { testCaseFailures, getTestScenarios } from '../../common/lib/saved_object_test_utils'; +import { TestUser } from '../../common/lib/types'; +import { FtrProviderContext } from '../../common/ftr_provider_context'; +import { + bulkDeleteTestSuiteFactory, + TEST_CASES as CASES, + BulkDeleteTestDefinition, +} from '../../common/suites/bulk_delete'; + +const { + DEFAULT: { spaceId: DEFAULT_SPACE_ID }, + SPACE_1: { spaceId: SPACE_1_ID }, + SPACE_2: { spaceId: SPACE_2_ID }, +} = SPACES; +const { fail400, fail404 } = testCaseFailures; + +const createTestCases = (spaceId: string) => { + // for each permitted (non-403) outcome, if failure !== undefined then we expect + // to receive an error; otherwise, we expect to receive a success result + const normalTypes = [ + { ...CASES.SINGLE_NAMESPACE_DEFAULT_SPACE, ...fail404(spaceId !== DEFAULT_SPACE_ID) }, + { ...CASES.SINGLE_NAMESPACE_SPACE_1, ...fail404(spaceId !== SPACE_1_ID) }, + { ...CASES.SINGLE_NAMESPACE_SPACE_2, ...fail404(spaceId !== SPACE_2_ID) }, + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, ...fail400() }, + // try to delete this object again, this time using the `force` option + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, force: true }, + { + ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, + ...fail400(spaceId === DEFAULT_SPACE_ID || spaceId === SPACE_1_ID), + ...fail404(spaceId !== DEFAULT_SPACE_ID && spaceId !== SPACE_1_ID), + }, + // try to delete this object again, this time using the `force` option + { + ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, + force: true, + ...fail404(spaceId !== DEFAULT_SPACE_ID && spaceId !== SPACE_1_ID), + }, + { ...CASES.MULTI_NAMESPACE_ONLY_SPACE_1, ...fail404(spaceId !== SPACE_1_ID) }, + { ...CASES.MULTI_NAMESPACE_ONLY_SPACE_2, ...fail404(spaceId !== SPACE_2_ID) }, + { + ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_DEFAULT_SPACE, + ...fail404(spaceId !== DEFAULT_SPACE_ID), + }, + { ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, ...fail404(spaceId !== SPACE_1_ID) }, + CASES.NAMESPACE_AGNOSTIC, + { ...CASES.ALIAS_DELETE_INCLUSIVE, force: true }, + { ...CASES.ALIAS_DELETE_EXCLUSIVE, force: true }, + { ...CASES.DOES_NOT_EXIST, ...fail404() }, + ]; + const hiddenType = [{ ...CASES.HIDDEN, ...fail400() }]; // this behavior diverges from `delete`, which throws 404 + const allTypes = normalTypes.concat(hiddenType); + return { normalTypes, hiddenType, allTypes }; +}; + +export default function ({ getService }: FtrProviderContext) { + const supertest = getService('supertestWithoutAuth'); + const esArchiver = getService('esArchiver'); + const es = getService('es'); + + const { addTests, createTestDefinitions } = bulkDeleteTestSuiteFactory(es, esArchiver, supertest); + const createTests = (spaceId: string) => { + const { normalTypes, hiddenType, allTypes } = createTestCases(spaceId); + return { + unauthorized: createTestDefinitions(allTypes, true, { spaceId }), + authorized: [ + createTestDefinitions(normalTypes, false, { spaceId }), + createTestDefinitions(hiddenType, true, { spaceId }), + ].flat(), + superuser: createTestDefinitions(allTypes, false, { spaceId }), + }; + }; + + describe('_bulk__delete', () => { + getTestScenarios().securityAndSpaces.forEach(({ spaceId, users }) => { + const suffix = ` within the ${spaceId} space`; + const { unauthorized, authorized, superuser } = createTests(spaceId); + const _addTests = (user: TestUser, tests: BulkDeleteTestDefinition[]) => { + addTests(`${user.description}${suffix}`, { user, spaceId, tests }); + }; + + [ + users.noAccess, // expect fail + users.legacyAll, // expect success + users.dualRead, // expect fail + users.readGlobally, // expect fail + users.readAtSpace, // expect fail + users.allAtOtherSpace, // expect fail if not within other_space + ].forEach((user) => { + _addTests(user, unauthorized); // expect fail + }); + // allAtSpace users only have access to a single space. If any multinamespace objects exist that are shared to some space they don't have access to, (or have legacy alias URLs in a space they don't have access to), I'd expect the request to get rejected. + [users.dualAll, users.allGlobally, users.allAtSpace].forEach((user) => { + // expect fail for allAtSpace if not within that space + _addTests(user, authorized); + }); + _addTests(users.superuser, superuser); // expect success + }); + }); +} diff --git a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/index.ts b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/index.ts index 4eb0b90480314..5cbb560427281 100644 --- a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/index.ts +++ b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/index.ts @@ -21,6 +21,7 @@ export default function ({ getService, loadTestFile }: FtrProviderContext) { loadTestFile(require.resolve('./bulk_get')); loadTestFile(require.resolve('./bulk_update')); loadTestFile(require.resolve('./bulk_resolve')); + loadTestFile(require.resolve('./bulk_delete')); loadTestFile(require.resolve('./create')); loadTestFile(require.resolve('./delete')); loadTestFile(require.resolve('./export')); From 6c9dc038007f40f69ac91a2de0b5fdc65a33f9c3 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Tue, 13 Sep 2022 10:50:51 -0700 Subject: [PATCH 39/49] typo --- .../security_and_spaces/apis/bulk_delete.ts | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts index a6b6e1dd701b4..7a40ea564fb82 100644 --- a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts +++ b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_delete.ts @@ -78,7 +78,7 @@ export default function ({ getService }: FtrProviderContext) { }; }; - describe('_bulk__delete', () => { + describe('_bulk_delete', () => { getTestScenarios().securityAndSpaces.forEach(({ spaceId, users }) => { const suffix = ` within the ${spaceId} space`; const { unauthorized, authorized, superuser } = createTests(spaceId); @@ -87,21 +87,19 @@ export default function ({ getService }: FtrProviderContext) { }; [ - users.noAccess, // expect fail - users.legacyAll, // expect success - users.dualRead, // expect fail - users.readGlobally, // expect fail - users.readAtSpace, // expect fail - users.allAtOtherSpace, // expect fail if not within other_space + users.noAccess, + users.legacyAll, + users.dualRead, + users.readGlobally, + users.readAtSpace, + users.allAtOtherSpace, ].forEach((user) => { - _addTests(user, unauthorized); // expect fail + _addTests(user, unauthorized); }); - // allAtSpace users only have access to a single space. If any multinamespace objects exist that are shared to some space they don't have access to, (or have legacy alias URLs in a space they don't have access to), I'd expect the request to get rejected. [users.dualAll, users.allGlobally, users.allAtSpace].forEach((user) => { - // expect fail for allAtSpace if not within that space _addTests(user, authorized); }); - _addTests(users.superuser, superuser); // expect success + _addTests(users.superuser, superuser); }); }); } From 18969cfa8316726503c9aff3da5ce02196c0f52b Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Tue, 13 Sep 2022 11:18:17 -0700 Subject: [PATCH 40/49] Changes back to hard coding the expected number of legacy aliases deleted --- .../common/suites/bulk_delete.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts index 918a21d9fe944..3a36c67ef1bbc 100644 --- a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts +++ b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts @@ -58,7 +58,7 @@ export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertes const { type, id } = testCase; expect(object.type).to.eql(type); expect(object.id).to.eql(id); - await expectResponses.permitted(object, testCase); // takes the object (record) we're asserting against and uses the testCase's failure entry value to return the appropriate error. Asserts that the object is the expected error + await expectResponses.permitted(object, testCase); } else { await es.indices.refresh({ index: '.kibana' }); // alias deletion uses refresh: false, so we need to manually refresh the index before searching const searchResponse = await es.search({ @@ -70,17 +70,14 @@ export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertes }, }); - const numberOfAliasesDeleted = [ALIAS_DELETE_INCLUSIVE, ALIAS_DELETE_EXCLUSIVE].find( + const expectAliasWasDeleted = !![ALIAS_DELETE_INCLUSIVE, ALIAS_DELETE_EXCLUSIVE].find( ({ type, id }) => testCase.type === type && testCase.id === id ); - // use a conditional check to guard against future exclusion of these test cases - // we also use a dynamically calculated numberic value to allow for changing the fixtures the tests run against. - if (Array.isArray(numberOfAliasesDeleted)) { - // Eight aliases exist and they are all deleted in the bulk operation since the delete behavior when using force is to delete the object from all spaces it exists in - expect((searchResponse.hits.total as SearchTotalHits).value).to.eql( - numberOfAliasesDeleted.length - ); - } + // Eight aliases exist and they are all deleted in the bulk operation. + // The delete behavior for multinamespace objects shared to more than one space when using force is to delete the object from all the spaces it is shared to. + expect((searchResponse.hits.total as SearchTotalHits).value).to.eql( + expectAliasWasDeleted ? 8 : 8 + ); } } } From 30099c1fb5291f93362c46bb1da3de215628993c Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Wed, 14 Sep 2022 18:24:33 -0700 Subject: [PATCH 41/49] Correctly applies force flag --- .../src/lib/repository.ts | 27 +++++++++---------- .../common/suites/bulk_delete.ts | 21 ++++++++++----- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 9af44e9f42480..36d66995428d8 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -237,7 +237,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { serializer, migrator, allowedTypes = [], - logger, + logger, // 'savedobjects-service.repository' } = options; // It's important that we migrate documents / mark them as up-to-date @@ -688,7 +688,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { if (!this._allowedTypes.includes(type)) { throw SavedObjectsErrorHelpers.createGenericNotFoundError(type, id); } - const { refresh = DEFAULT_REFRESH_SETTING, force } = options; const namespace = normalizeNamespace(options.namespace); @@ -780,7 +779,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { * @returns array BulkDeleteExpectedBulkGetResult[]: left as 400 for objects that don't have a valid type or right Either result with the object and an `esRequestIndex` if the object is of a multinamespace type. */ private presortObjectsByNamespaceType(objects: SavedObjectsBulkDeleteObject[]) { - let bulkGetRequestIndexCounter = 0; // flag for multi-namespace objects + let bulkGetRequestIndexCounter = 0; return objects.map((object) => { const { type, id } = object; if (!this._allowedTypes.includes(type)) { @@ -814,9 +813,9 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { const { expectedBulkGetResults, namespace } = params; const bulkGetMultiNamespaceDocs = expectedBulkGetResults .filter(isRight) - .filter(({ value }) => value.esRequestIndex !== undefined) // only get docs that need multinamespace checks, I don't think we want to do this filtering + .filter(({ value }) => value.esRequestIndex !== undefined) .map(({ value: { type, id } }) => ({ - _id: this._serializer.generateRawId(namespace, type, id), // prefixes the id with the space name in the api call's scope + _id: this._serializer.generateRawId(namespace, type, id), _index: this.getIndexForType(type), _source: ['type', 'namespaces'], })); @@ -835,7 +834,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { headers: bulkGetMultiNamespaceDocsResponse.headers, }) ) { - // I'm not sure if I want to throw here. throw SavedObjectsErrorHelpers.createGenericNotFoundEsUnavailableError(); } return bulkGetMultiNamespaceDocsResponse; @@ -896,9 +894,12 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { namespaces = actualResult!._source.namespaces ?? [ SavedObjectsUtils.namespaceIdToString(namespace), ]; - + const useForce = force && force === true ? true : false; // the document is shared to more than one space and can only be deleted by force. - if (!force && (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING))) { + if ( + useForce === false && + (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING)) + ) { return { tag: 'Left', value: { @@ -938,15 +939,12 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { const { refresh = DEFAULT_REFRESH_SETTING, force } = options; const namespace = normalizeNamespace(options.namespace); const expectedBulkGetResults = this.presortObjectsByNamespaceType(objects); - const multiNamespaceDocsResponse = await this.preflightCheckForBulkDelete({ expectedBulkGetResults, namespace, }); - const bulkDeleteParams: BulkDeleteParams[] = []; - // @TINA expectedBulkDeleteMultiNamespaceDocsResults is result from sorting again into Left and Right responses. Use the Right responses to create the bulk params const expectedBulkDeleteMultiNamespaceDocsResults = this.getExpectedBulkDeleteMultiNamespaceDocsResults({ expectedBulkGetResults, @@ -954,7 +952,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { namespace, force, }); - + // note that we're using a map here but we're only using it to add to the bulkDeleteParams expectedBulkDeleteMultiNamespaceDocsResults.map((expectedResult) => { if (isRight(expectedResult)) { bulkDeleteParams.push({ @@ -981,7 +979,6 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { // extracted to ensure consistency in the error results returned let errorResult: BulkDeleteItemErrorResult; - const savedObjects = await Promise.all( expectedBulkDeleteMultiNamespaceDocsResults.map(async (expectedResult) => { if (isLeft(expectedResult)) { @@ -1030,7 +1027,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { type, id, ...(namespaces.includes(ALL_NAMESPACES_STRING) - ? { namespaces: [], deleteBehavior: 'exclusive' } // delete legacy URL aliases for this type/ID for all spaces + ? { namespaces: [], deleteBehavior: 'exclusive' } // delete legacy URL aliases for this type/ID for all spaces not in []. Effectively, it's the same behavior ad inludisve with a defined array of namespaces. : { namespaces, deleteBehavior: 'inclusive' }), // delete legacy URL aliases for this type/ID for these specific spaces. In the bulk operation, this behavior is only applicable in the case of multi-namespace isolated types. }).catch((err) => { // The object has already been deleted, but we caught an error when attempting to delete aliases. @@ -1042,7 +1039,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } } const successfulResult = { - success: true, // could also use (rawResponse.result === 'deleted') + success: true, id, type, }; diff --git a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts index 3a36c67ef1bbc..8b50f9bf28a6d 100644 --- a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts +++ b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts @@ -25,8 +25,16 @@ export interface BulkDeleteTestCase extends TestCase { failure?: 400 | 403 | 404; } -const ALIAS_DELETE_INCLUSIVE = Object.freeze({ type: 'resolvetype', id: 'alias-match-newid' }); // exists in three specific spaces; deleting this should also delete the aliases that target it in the default space and space_1 -const ALIAS_DELETE_EXCLUSIVE = Object.freeze({ type: 'resolvetype', id: 'all_spaces' }); // exists in all spaces; deleting this should also delete the aliases that target it in the default space and space_1 +const ALIAS_DELETE_INCLUSIVE = Object.freeze({ + type: 'resolvetype', + id: 'alias-match-newid', + force: true, +}); // exists in three specific spaces; deleting this should also delete the aliases that target it in the default space and space_1 +const ALIAS_DELETE_EXCLUSIVE = Object.freeze({ + type: 'resolvetype', + id: 'all_spaces', + force: true, +}); // exists in all spaces; deleting this should also delete the aliases that target it in the default space and space_1 const DOES_NOT_EXIST = Object.freeze({ type: 'dashboard', id: 'does-not-exist' }); export const TEST_CASES: Record = Object.freeze({ ...CASES, @@ -64,7 +72,7 @@ export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertes const searchResponse = await es.search({ index: '.kibana', body: { - size: 0, + size: 10, query: { terms: { type: ['legacy-url-alias'] } }, track_total_hits: true, }, @@ -76,7 +84,7 @@ export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertes // Eight aliases exist and they are all deleted in the bulk operation. // The delete behavior for multinamespace objects shared to more than one space when using force is to delete the object from all the spaces it is shared to. expect((searchResponse.hits.total as SearchTotalHits).value).to.eql( - expectAliasWasDeleted ? 8 : 8 + expectAliasWasDeleted ? 6 : 8 ); } } @@ -123,8 +131,9 @@ export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertes for (const test of tests) { it(`should return ${test.responseStatusCode} ${test.title} `, async () => { - const requestBody = [{ type: test.request.type, id: test.request.id }]; - const query = test.force && test.force === true ? '?force=true' : ''; + const { type: testType, id: testId, force: testForce } = test.request; + const requestBody = [{ type: testType, id: testId }]; + const query = testForce && testForce === true ? '?force=true' : ''; await supertest .post(`${getUrlPrefix(spaceId)}/api/saved_objects/_bulk_delete${query}`) .auth(user?.username, user?.password) From 0352813011dfa7eb67596fec11fd04530bbf16a8 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Thu, 15 Sep 2022 07:15:25 -0700 Subject: [PATCH 42/49] removes force flag from bulk_delete's common suite --- .../saved_object_api_integration/common/suites/bulk_delete.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts index 8b50f9bf28a6d..a32ac15abb741 100644 --- a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts +++ b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts @@ -28,12 +28,10 @@ export interface BulkDeleteTestCase extends TestCase { const ALIAS_DELETE_INCLUSIVE = Object.freeze({ type: 'resolvetype', id: 'alias-match-newid', - force: true, }); // exists in three specific spaces; deleting this should also delete the aliases that target it in the default space and space_1 const ALIAS_DELETE_EXCLUSIVE = Object.freeze({ type: 'resolvetype', id: 'all_spaces', - force: true, }); // exists in all spaces; deleting this should also delete the aliases that target it in the default space and space_1 const DOES_NOT_EXIST = Object.freeze({ type: 'dashboard', id: 'does-not-exist' }); export const TEST_CASES: Record = Object.freeze({ @@ -72,7 +70,7 @@ export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertes const searchResponse = await es.search({ index: '.kibana', body: { - size: 10, + size: 0, query: { terms: { type: ['legacy-url-alias'] } }, track_total_hits: true, }, From cac082aee36d71d24cd88352746e93c43ea6d4a7 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Thu, 15 Sep 2022 08:49:41 -0700 Subject: [PATCH 43/49] Code cleanup --- .../src/lib/repository.ts | 19 +++++++++++-------- .../repository_bulk_delete_internal_types.ts | 2 +- .../src/apis/bulk_delete.ts | 6 ++++++ ...ecure_saved_objects_client_wrapper.test.ts | 2 +- .../common/suites/bulk_delete.ts | 2 +- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 36d66995428d8..8ef4b418d1093 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -774,9 +774,10 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } /** - * performs initial checks on object type validity and flags multi-namespace objects for preflight checks by adding an `esRequestIndex` + * Performs initial checks on object type validity and flags multi-namespace objects for preflight checks by adding an `esRequestIndex` * @param objects SavedObjectsBulkDeleteObject[] - * @returns array BulkDeleteExpectedBulkGetResult[]: left as 400 for objects that don't have a valid type or right Either result with the object and an `esRequestIndex` if the object is of a multinamespace type. + * @returns array BulkDeleteExpectedBulkGetResult[] + * @internal */ private presortObjectsByNamespaceType(objects: SavedObjectsBulkDeleteObject[]) { let bulkGetRequestIndexCounter = 0; @@ -807,7 +808,8 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { /** * Fetch multi-namespace saved objects * @returns MgetResponse - * @internalNotes multinamespace objects shared to more than one space require special handling. We fetch these docs to retrieve their namespaces. + * @notes multi-namespace objects shared to more than one space require special handling. We fetch these docs to retrieve their namespaces. + * @internal */ private async preflightCheckForBulkDelete(params: PreflightCheckForBulkDeleteParams) { const { expectedBulkGetResults, namespace } = params; @@ -840,9 +842,8 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } /** - * - * @param params: ExpectedBulkDeleteMultiNamespaceDocsParams * @returns array of objects sorted by expected delete success or failure result + * @internal */ private getExpectedBulkDeleteMultiNamespaceDocsResults( params: ExpectedBulkDeleteMultiNamespaceDocsParams @@ -952,7 +953,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { namespace, force, }); - // note that we're using a map here but we're only using it to add to the bulkDeleteParams + // bulk up the bulkDeleteParams expectedBulkDeleteMultiNamespaceDocsResults.map((expectedResult) => { if (isRight(expectedResult)) { bulkDeleteParams.push({ @@ -1016,9 +1017,11 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } if (rawResponse.result === 'deleted') { - // `namespaces` should only exist in the expectedResult.value if the type is multinamespace. + // `namespaces` should only exist in the expectedResult.value if the type is multi-namespace. if (namespaces) { - // in the bulk operation, one cannot specify a namespace from which to delete an object other than the namespace that the operation is performed in. If a multinamespace object exists in more than the current space (from which the call is made), force deleting the object will delete it from all namespaces it exists in. In that case, all legacy url aliases are deleted as well. If force isn't applied, the operation fails and the object isn't deleted. + // in the bulk operation, one cannot specify a namespace from which to delete an object other than the namespace that the operation is performed in. + // If a multinamespace object exists in more than the current space (from which the call is made), force deleting the object will delete it from all namespaces it exists in. + // In that case, all legacy url aliases are deleted as well. If force isn't applied, the operation fails and the object isn't deleted. await deleteLegacyUrlAliases({ mappings: this._mappings, registry: this._registry, diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts index 97c7650fbfb81..6aea829251c0f 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts @@ -72,7 +72,7 @@ export type NewBulkItemResponse = BulkResponseItem & { error: ErrorCause & { ind /** * @internal - * contains all documents for bulk delete, regardless of namespace type. + * @note Contains all documents for bulk delete, regardless of namespace type */ export type BulkDeleteExpectedBulkGetResult = Either< { type: string; id: string; error: Payload }, diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts index 7279703c1cf5d..76d490925c580 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/bulk_delete.ts @@ -22,7 +22,11 @@ export interface SavedObjectsBulkDeleteObject { * @public */ export interface SavedObjectsBulkDeleteOptions extends SavedObjectsBaseOptions { + /** The Elasticsearch Refresh setting for this operation */ refresh?: MutatingOperationRefreshSetting; + /** + * Force deletion of all objects that exists in multiple namespaces, applied to all objects. + */ force?: boolean; } @@ -32,7 +36,9 @@ export interface SavedObjectsBulkDeleteOptions extends SavedObjectsBaseOptions { export interface SavedObjectsBulkDeleteStatus { id: string; type: string; + /** The status of deleting the object: true for deleted, false for error */ success: boolean; + /** Reason the object could not be deleted (success is false) */ error?: SavedObjectError; } diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 26a0285658e3a..bce6786f06775 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -551,7 +551,7 @@ describe('#bulkUpdate', () => { test(`checks privileges for user, actions, and namespace`, async () => { const objects = [obj1, obj2]; const options = { namespace }; - const namespaces = [options.namespace]; + const namespaces = [options.namespace]; // the bulkUpdate function always checks privileges as an array await expectPrivilegeCheck(client.bulkUpdate, { objects, options }, namespaces); }); diff --git a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts index a32ac15abb741..d0c90ccc3c255 100644 --- a/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts +++ b/x-pack/test/saved_object_api_integration/common/suites/bulk_delete.ts @@ -56,7 +56,7 @@ export function bulkDeleteTestSuiteFactory(es: Client, esArchiver: any, supertes } else { // permitted const statuses = response.body.statuses; - expect(statuses).length([testCase].length); // hard coding the expectation now since I expect a response for each test case as a single request item in the array + expect(statuses).length([testCase].length); for (let i = 0; i < statuses.length; i++) { const object = statuses[i]; expect(object).to.have.keys(['id', 'type', 'success']); From 614caf62aebe635ee42b78c1c2c939e1e18979f6 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Thu, 15 Sep 2022 09:49:36 -0700 Subject: [PATCH 44/49] Update packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts --- .../src/saved_objects_repository.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts index 9f5d6e5f80b65..d7d2ca57ae3a2 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/saved_objects_repository.ts @@ -112,7 +112,7 @@ export interface ISavedObjectsRepository { * Deletes multiple documents at once * @param {array} objects - an array of objects containing id and type * @param {object} [options={}] - * @returns {promise} - { statuses: [{ success, error }] } + * @returns {promise} - { statuses: [{ id, type, success, error: { message } }] } */ bulkDelete( objects: SavedObjectsBulkDeleteObject[], From b0bd664baad4aa3120f3bb987bbaec39edce8d99 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Mon, 19 Sep 2022 09:39:43 -0700 Subject: [PATCH 45/49] Update x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts --- .../spaces/server/saved_objects/spaces_saved_objects_client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index 30935a675c0f6..52ca1f2604e88 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -286,7 +286,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { async bulkUpdate( objects: Array> = [], - options: SavedObjectsBulkDeleteOptions = {} + options: SavedObjectsBaseOptions = {} ) { throwErrorIfNamespaceSpecified(options); return await this.client.bulkUpdate(objects, { From 591a7b88e9e34d1a7dda85c8e88e80347b60a076 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 19 Sep 2022 14:11:13 -0700 Subject: [PATCH 46/49] limit number of concurrent legacy url alias deletions --- .../src/lib/repository.ts | 129 +++++++++--------- .../repository_bulk_delete_internal_types.ts | 6 + 2 files changed, 73 insertions(+), 62 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 8ef4b418d1093..cbac420c08a38 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -86,6 +86,7 @@ import { type IndexMapping, type IKibanaMigrator, } from '@kbn/core-saved-objects-base-server-internal'; +import pMap from 'p-map'; import { PointInTimeFinder } from './point_in_time_finder'; import { createRepositoryEsClient, RepositoryEsClient } from './repository_es_client'; import { getSearchDsl } from './search_dsl'; @@ -120,6 +121,7 @@ import type { BulkDeleteExpectedBulkGetResult, PreflightCheckForBulkDeleteParams, ExpectedBulkDeleteMultiNamespaceDocsParams, + ObjectToDeleteAliasesFor, } from './repository_bulk_delete_internal_types'; // BEWARE: The SavedObjectClient depends on the implementation details of the SavedObjectsRepository @@ -139,6 +141,7 @@ export interface SavedObjectsRepositoryOptions { export const DEFAULT_REFRESH_SETTING = 'wait_for'; export const DEFAULT_RETRY_COUNT = 3; +const MAX_CONCURRENT_ALIAS_DELETIONS = 10; /** * @internal */ @@ -980,75 +983,77 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { // extracted to ensure consistency in the error results returned let errorResult: BulkDeleteItemErrorResult; - const savedObjects = await Promise.all( - expectedBulkDeleteMultiNamespaceDocsResults.map(async (expectedResult) => { - if (isLeft(expectedResult)) { - return { ...expectedResult.value, success: false }; - } - const { + const objectsToDeleteAliasesFor: ObjectToDeleteAliasesFor[] = []; + + const savedObjects = expectedBulkDeleteMultiNamespaceDocsResults.map((expectedResult) => { + if (isLeft(expectedResult)) { + return { ...expectedResult.value, success: false }; + } + const { + type, + id, + namespaces, + esRequestIndex: esBulkDeleteRequestIndex, + } = expectedResult.value; + // we assume this wouldn't happen but is needed to ensure type consistency + if (bulkDeleteResponse === undefined) { + throw new Error( + `Unexpected error in bulkDelete saved objects: bulkDeleteResponse is undefined` + ); + } + const rawResponse = Object.values( + bulkDeleteResponse.items[esBulkDeleteRequestIndex] + )[0] as NewBulkItemResponse; + + const error = getBulkOperationError(type, id, rawResponse); + if (error) { + return { success: false, type, id, error }; + } + if (rawResponse.result === 'not_found') { + return { + success: false, type, id, - namespaces, - esRequestIndex: esBulkDeleteRequestIndex, - } = expectedResult.value; - // we assume this wouldn't happen but is needed to ensure type consistency - if (bulkDeleteResponse === undefined) { - throw new Error( - `Unexpected error in bulkDelete saved objects: bulkDeleteResponse is undefined` - ); - } - const rawResponse = Object.values( - bulkDeleteResponse.items[esBulkDeleteRequestIndex] - )[0] as NewBulkItemResponse; + error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), + }; + } - const error = getBulkOperationError(type, id, rawResponse); - if (error) { - errorResult = { success: false, type, id, error }; - return errorResult; - } - if (rawResponse.result === 'not_found') { - errorResult = { - success: false, + if (rawResponse.result === 'deleted') { + // `namespaces` should only exist in the expectedResult.value if the type is multi-namespace. + if (namespaces) { + objectsToDeleteAliasesFor.push({ type, id, - error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), - }; - return errorResult; + ...(namespaces.includes(ALL_NAMESPACES_STRING) + ? { namespaces: [], deleteBehavior: 'exclusive' } + : { namespaces, deleteBehavior: 'inclusive' }), + }); } + } + const successfulResult = { + success: true, + id, + type, + }; + return successfulResult; + }); + + // Delete aliases if necessary, ensuring we don't have too many concurrent operations running. + const mapper = async ({ type, id, namespaces, deleteBehavior }: ObjectToDeleteAliasesFor) => + await deleteLegacyUrlAliases({ + mappings: this._mappings, + registry: this._registry, + client: this.client, + getIndexForType: this.getIndexForType.bind(this), + type, + id, + namespaces, + deleteBehavior, + }).catch((err) => { + this._logger.error(`Unable to delete aliases when deleting an object: ${err.message}`); + }); + await pMap(objectsToDeleteAliasesFor, mapper, { concurrency: MAX_CONCURRENT_ALIAS_DELETIONS }); - if (rawResponse.result === 'deleted') { - // `namespaces` should only exist in the expectedResult.value if the type is multi-namespace. - if (namespaces) { - // in the bulk operation, one cannot specify a namespace from which to delete an object other than the namespace that the operation is performed in. - // If a multinamespace object exists in more than the current space (from which the call is made), force deleting the object will delete it from all namespaces it exists in. - // In that case, all legacy url aliases are deleted as well. If force isn't applied, the operation fails and the object isn't deleted. - await deleteLegacyUrlAliases({ - mappings: this._mappings, - registry: this._registry, - client: this.client, - getIndexForType: this.getIndexForType.bind(this), - type, - id, - ...(namespaces.includes(ALL_NAMESPACES_STRING) - ? { namespaces: [], deleteBehavior: 'exclusive' } // delete legacy URL aliases for this type/ID for all spaces not in []. Effectively, it's the same behavior ad inludisve with a defined array of namespaces. - : { namespaces, deleteBehavior: 'inclusive' }), // delete legacy URL aliases for this type/ID for these specific spaces. In the bulk operation, this behavior is only applicable in the case of multi-namespace isolated types. - }).catch((err) => { - // The object has already been deleted, but we caught an error when attempting to delete aliases. - // A consumer cannot attempt to delete the object again, so just log the error and swallow it. - this._logger.error( - `Unable to delete aliases when deleting an object: ${err.message}` - ); - }); - } - } - const successfulResult = { - success: true, - id, - type, - }; - return successfulResult; - }) - ); return { statuses: [...savedObjects] }; } diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts index 6aea829251c0f..93d4354d8d7e8 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository_bulk_delete_internal_types.ts @@ -13,6 +13,7 @@ import { } from '@elastic/elasticsearch/lib/api/types'; import type { estypes, TransportResult } from '@elastic/elasticsearch'; import { Either } from './internal_utils'; +import { DeleteLegacyUrlAliasesParams } from './legacy_url_aliases'; /** * @internal @@ -78,3 +79,8 @@ export type BulkDeleteExpectedBulkGetResult = Either< { type: string; id: string; error: Payload }, { type: string; id: string; version?: string; esRequestIndex?: number } >; + +export type ObjectToDeleteAliasesFor = Pick< + DeleteLegacyUrlAliasesParams, + 'type' | 'id' | 'namespaces' | 'deleteBehavior' +>; From 0ce4a5d7e2a34eddf40b04844db1802b43479b55 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 19 Sep 2022 14:25:23 -0700 Subject: [PATCH 47/49] reimplement error type consistency implementation --- .../src/lib/repository.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index cbac420c08a38..31b37a2961224 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -1007,15 +1007,17 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { const error = getBulkOperationError(type, id, rawResponse); if (error) { - return { success: false, type, id, error }; + errorResult = { success: false, type, id, error }; + return errorResult; } if (rawResponse.result === 'not_found') { - return { + errorResult = { success: false, type, id, error: errorContent(SavedObjectsErrorHelpers.createGenericNotFoundError(type, id)), }; + return errorResult; } if (rawResponse.result === 'deleted') { From 7aa18ba4ce3ea5974b01543ccf6f1efc234970f1 Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Mon, 19 Sep 2022 14:54:56 -0700 Subject: [PATCH 48/49] handles nits from PR review --- .../src/lib/repository.test.ts | 2 +- .../src/lib/repository.ts | 2 +- .../service/lib/repository_with_proxy.test.ts | 7 +++++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts index 1d663be0f18b4..0739c9acab8f5 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.test.ts @@ -2454,7 +2454,7 @@ describe('SavedObjectsRepository', () => { ); }); - it(`returns an for an object when the object's type is hidden`, async () => { + it(`returns an error for an object when the object's type is hidden`, async () => { const hiddenObject = { ...obj1, type: HIDDEN_TYPE }; await repositoryBulkDeleteError(hiddenObject, false, expectErrorInvalidType(hiddenObject)); }); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 31b37a2961224..6bfe12b3da925 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -240,7 +240,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { serializer, migrator, allowedTypes = [], - logger, // 'savedobjects-service.repository' + logger, } = options; // It's important that we migrate documents / mark them as up-to-date diff --git a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts index 50698183ca667..f0fdc609d8915 100644 --- a/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts +++ b/src/core/server/integration_tests/saved_objects/service/lib/repository_with_proxy.test.ts @@ -263,8 +263,11 @@ describe('404s from proxies', () => { }); it('handles `bulkDelete` requests that are successful when the proxy passes through the product header', async () => { - const docsToGet = myBulkDeleteTypeDocs; - const bulkDeleteDocs = docsToGet.map((doc) => ({ id: doc.id, type: 'my_bulk_delete_type' })); + const docsToDelete = myBulkDeleteTypeDocs; + const bulkDeleteDocs = docsToDelete.map((doc) => ({ + id: doc.id, + type: 'my_bulk_delete_type', + })); const docsFound = await repository.bulkDelete(bulkDeleteDocs, { force: false }); expect(docsFound.statuses.length).toBeGreaterThan(0); From cd5c7ffa768fdd601e24a5d855d2f2fdd709c0dd Mon Sep 17 00:00:00 2001 From: Christiane Heiligers Date: Tue, 20 Sep 2022 06:49:40 -0700 Subject: [PATCH 49/49] Addressed code review nits --- .../src/lib/repository.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts index 6bfe12b3da925..5569141c7fa0e 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/repository.ts @@ -884,7 +884,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { } // the following check should be redundant since we're retrieving the docs from elasticsearch but we check just to make sure // @ts-expect-error MultiGetHit is incorrectly missing _id, _source - if (docFound && !this.rawDocExistsInNamespace(actualResult, namespace)) { + if (!this.rawDocExistsInNamespace(actualResult, namespace)) { return { tag: 'Left', value: { @@ -900,10 +900,7 @@ export class SavedObjectsRepository implements ISavedObjectsRepository { ]; const useForce = force && force === true ? true : false; // the document is shared to more than one space and can only be deleted by force. - if ( - useForce === false && - (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING)) - ) { + if (!useForce && (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING))) { return { tag: 'Left', value: {