Change saved object delete API to use deleteLegacyUrlAliases

src/core/server/saved_objects/service/lib/repository.test.js

@@ -14,6 +14,7 @@ import {
   mockUpdateObjectsSpaces,
   mockGetCurrentTime,
   mockPreflightCheckForCreate,
+  mockDeleteLegacyUrlAliases,
 } from './repository.test.mock';
 
 import { SavedObjectsRepository } from './repository';
@@ -2394,9 +2395,11 @@ describe('SavedObjectsRepository', () => {
     const id = 'logstash-*';
     const namespace = 'foo-namespace';
 
-    const deleteSuccess = async (type, id, options) => {
+    const deleteSuccess = async (type, id, options, internalOptions = {}) => {
+      const { mockGetResponseValue } = internalOptions;
       if (registry.isMultiNamespace(type)) {
-        const mockGetResponse = getMockGetResponse({ type, id }, options?.namespace);
+        const mockGetResponse =
+          mockGetResponseValue ?? getMockGetResponse({ type, id }, options?.namespace);
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(mockGetResponse)
         );
@@ -2409,6 +2412,11 @@ describe('SavedObjectsRepository', () => {
       return result;
     };
 
+    beforeEach(() => {
+      mockDeleteLegacyUrlAliases.mockClear();
+      mockDeleteLegacyUrlAliases.mockResolvedValue();
+    });
+
     describe('client calls', () => {
       it(`should use the ES delete action when not using a multi-namespace type`, async () => {
         await deleteSuccess(type, id);
@@ -2482,6 +2490,62 @@ describe('SavedObjectsRepository', () => {
       });
     });
 
+    describe('legacy URL aliases', () => {
+      it(`doesn't delete legacy URL aliases for single-namespace object types`, async () => {
+        await deleteSuccess(type, id, { namespace });
+        expect(mockDeleteLegacyUrlAliases).not.toHaveBeenCalled();
+      });
+
+      // We intentionally do not include a test case for a multi-namespace object with a "not found" preflight result, because that throws
+      // an error (without deleting aliases) and we already have a test case for that
+
+      it(`deletes legacy URL aliases for multi-namespace object types (all spaces)`, async () => {
+        const internalOptions = {
+          mockGetResponseValue: getMockGetResponse(
+            { type: MULTI_NAMESPACE_TYPE, id },
+            ALL_NAMESPACES_STRING
+          ),
+        };
+        await deleteSuccess(MULTI_NAMESPACE_TYPE, id, { namespace, force: true }, internalOptions);
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledWith(
+          expect.objectContaining({
+            type: MULTI_NAMESPACE_TYPE,
+            id,
+            namespaces: [],
+            deleteBehavior: 'exclusive',
+          })
+        );
+      });
+
+      it(`deletes legacy URL aliases for multi-namespace object types (specific spaces)`, async () => {
+        await deleteSuccess(MULTI_NAMESPACE_TYPE, id, { namespace }); // this function mocks a preflight response with the given namespace by default
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledWith(
+          expect.objectContaining({
+            type: MULTI_NAMESPACE_TYPE,
+            id,
+            namespaces: [namespace],
+            deleteBehavior: 'inclusive',
+          })
+        );
+      });
+
+      it(`logs a message when deleteLegacyUrlAliases returns an error`, async () => {
+        client.get.mockResolvedValueOnce(
+          elasticsearchClientMock.createSuccessTransportRequestPromise(
+            getMockGetResponse({ type: MULTI_NAMESPACE_ISOLATED_TYPE, id, namespace })
+          )
+        );
+        client.delete.mockResolvedValueOnce(
+          elasticsearchClientMock.createSuccessTransportRequestPromise({ result: 'deleted' })
+        );
+        mockDeleteLegacyUrlAliases.mockRejectedValueOnce(new Error('Oh no!'));
+        await savedObjectsRepository.delete(MULTI_NAMESPACE_ISOLATED_TYPE, id, { namespace });
+        expect(client.get).toHaveBeenCalledTimes(1);
+        expect(logger.error).toHaveBeenCalledTimes(1);
+        expect(logger.error).toHaveBeenCalledWith('Oh no!');
+      });
+    });
+
     describe('errors', () => {
       const expectNotFoundError = async (type, id, options) => {
         await expect(savedObjectsRepository.delete(type, id, options)).rejects.toThrowError(

src/core/server/saved_objects/service/lib/repository.test.mock.ts

@@ -11,6 +11,7 @@ import type { internalBulkResolve } from './internal_bulk_resolve';
 import type * as InternalUtils from './internal_utils';
 import type { preflightCheckForCreate } from './preflight_check_for_create';
 import type { updateObjectsSpaces } from './update_objects_spaces';
+import type { deleteLegacyUrlAliases } from './legacy_url_aliases';
 
 export const mockCollectMultiNamespaceReferences = jest.fn() as jest.MockedFunction<
   typeof collectMultiNamespaceReferences
@@ -60,3 +61,10 @@ export const pointInTimeFinderMock = jest.fn();
 jest.doMock('./point_in_time_finder', () => ({
   PointInTimeFinder: pointInTimeFinderMock,
 }));
+
+export const mockDeleteLegacyUrlAliases = jest.fn() as jest.MockedFunction<
+  typeof deleteLegacyUrlAliases
+>;
+jest.mock('./legacy_url_aliases', () => ({
+  deleteLegacyUrlAliases: mockDeleteLegacyUrlAliases,
+}));

src/core/server/saved_objects/service/lib/repository.ts

@@ -100,6 +100,7 @@ import {
   preflightCheckForCreate,
   PreflightCheckForCreateObject,
 } from './preflight_check_for_create';
+import { deleteLegacyUrlAliases } from './legacy_url_aliases';
 
 // 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.
@@ -717,6 +718,25 @@ export class SavedObjectsRepository {
 
     const deleted = body.result === 'deleted';
     if (deleted) {
+      const namespaces = preflightResult?.savedObjectNamespaces;
+      if (namespaces) {
+        // This is a multi-namespace object type, and it might have legacy URL aliases that need to be 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
+            : { 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(err.message);
+        });
+      }
       return {};
     }
 
@@ -1344,10 +1364,12 @@ export class SavedObjectsRepository {
     options?: SavedObjectsUpdateObjectsSpacesOptions
   ) {
     return updateObjectsSpaces({
+      mappings: this._mappings,
       registry: this._registry,
       allowedTypes: this._allowedTypes,
       client: this.client,
       serializer: this._serializer,
+      logger: this._logger,
       getIndexForType: this.getIndexForType.bind(this),
       objects,
       spacesToAdd,

src/core/server/saved_objects/service/lib/update_objects_spaces.test.mock.ts

@@ -7,6 +7,7 @@
  */
 
 import type * as InternalUtils from './internal_utils';
+import type { deleteLegacyUrlAliases } from './legacy_url_aliases';
 
 export const mockGetBulkOperationError = jest.fn() as jest.MockedFunction<
   typeof InternalUtils['getBulkOperationError']
@@ -27,3 +28,10 @@ jest.mock('./internal_utils', () => {
     rawDocExistsInNamespace: mockRawDocExistsInNamespace,
   };
 });
+
+export const mockDeleteLegacyUrlAliases = jest.fn() as jest.MockedFunction<
+  typeof deleteLegacyUrlAliases
+>;
+jest.mock('./legacy_url_aliases', () => ({
+  deleteLegacyUrlAliases: mockDeleteLegacyUrlAliases,
+}));

src/core/server/saved_objects/service/lib/update_objects_spaces.test.ts

@@ -10,19 +10,22 @@ import {
   mockGetBulkOperationError,
   mockGetExpectedVersionProperties,
   mockRawDocExistsInNamespace,
+  mockDeleteLegacyUrlAliases,
 } from './update_objects_spaces.test.mock';
 
 import type { DeeplyMockedKeys } from '@kbn/utility-types/jest';
 import type { ElasticsearchClient } from 'src/core/server/elasticsearch';
 import { elasticsearchClientMock } from 'src/core/server/elasticsearch/client/mocks';
 
+import { loggerMock } from '../../../logging/logger.mock';
 import { typeRegistryMock } from '../../saved_objects_type_registry.mock';
 import { SavedObjectsSerializer } from '../../serialization';
 import type {
   SavedObjectsUpdateObjectsSpacesObject,
   UpdateObjectsSpacesParams,
 } from './update_objects_spaces';
 import { updateObjectsSpaces } from './update_objects_spaces';
+import { ALL_NAMESPACES_STRING } from './utils';
 
 type SetupParams = Partial<
   Pick<UpdateObjectsSpacesParams, 'objects' | 'spacesToAdd' | 'spacesToRemove' | 'options'>
@@ -53,6 +56,8 @@ beforeEach(() => {
   mockGetExpectedVersionProperties.mockReturnValue(EXPECTED_VERSION_PROPS);
   mockRawDocExistsInNamespace.mockReset();
   mockRawDocExistsInNamespace.mockReturnValue(true); // return true by default
+  mockDeleteLegacyUrlAliases.mockReset();
+  mockDeleteLegacyUrlAliases.mockResolvedValue(); // resolve an empty promise by default
 });
 
 afterAll(() => {
@@ -71,10 +76,12 @@ describe('#updateObjectsSpaces', () => {
     client = elasticsearchClientMock.createElasticsearchClient();
     const serializer = new SavedObjectsSerializer(registry);
     return {
+      mappings: { properties: {} }, // doesn't matter, only used as an argument to deleteLegacyUrlAliases which is mocked
       registry,
       allowedTypes: [SHAREABLE_OBJ_TYPE, NON_SHAREABLE_OBJ_TYPE], // SHAREABLE_HIDDEN_OBJ_TYPE is excluded
       client,
       serializer,
+      logger: loggerMock.create(),
       getIndexForType: (type: string) => `index-for-${type}`,
       objects,
       spacesToAdd,
@@ -382,6 +389,146 @@ describe('#updateObjectsSpaces', () => {
         expect(client.bulk).not.toHaveBeenCalled();
       });
     });
+
+    describe('legacy URL aliases', () => {
+      it('does not delete aliases for objects that were not removed from any spaces', async () => {
+        const space1 = 'space-to-add';
+        const space2 = 'space-to-remove';
+        const space3 = 'other-space';
+        const obj1 = { type: SHAREABLE_OBJ_TYPE, id: 'id-1', spaces: [space1] }; // will not be changed
+        const obj2 = { type: SHAREABLE_OBJ_TYPE, id: 'id-2', spaces: [space3] }; // will be updated
+
+        const objects = [obj1, obj2];
+        const spacesToAdd = [space1];
+        const spacesToRemove = [space2];
+        const params = setup({ objects, spacesToAdd, spacesToRemove });
+        // this test case does not call mget
+        mockBulkResults({ error: false }); // result for obj2
+
+        await updateObjectsSpaces(params);
+        expect(client.bulk).toHaveBeenCalledTimes(1);
+        expectBulkArgs({ action: 'update', object: { ...obj2, namespaces: [space3, space1] } });
+        expect(mockDeleteLegacyUrlAliases).not.toHaveBeenCalled();
+        expect(params.logger.error).not.toHaveBeenCalled();
+      });
+
+      it('does not delete aliases for objects that were removed from spaces but were also added to All Spaces (*)', async () => {
+        const space2 = 'space-to-remove';
+        const obj1 = { type: SHAREABLE_OBJ_TYPE, id: 'id-1', spaces: [space2] };
+
+        const objects = [obj1];
+        const spacesToAdd = [ALL_NAMESPACES_STRING];
+        const spacesToRemove = [space2];
+        const params = setup({ objects, spacesToAdd, spacesToRemove });
+        // this test case does not call mget
+        mockBulkResults({ error: false }); // result for obj1
+
+        await updateObjectsSpaces(params);
+        expect(client.bulk).toHaveBeenCalledTimes(1);
+        expectBulkArgs({
+          action: 'update',
+          object: { ...obj1, namespaces: [ALL_NAMESPACES_STRING] },
+        });
+        expect(mockDeleteLegacyUrlAliases).not.toHaveBeenCalled();
+        expect(params.logger.error).not.toHaveBeenCalled();
+      });
+
+      it('deletes aliases for objects that were removed from specific spaces using "deleteBehavior: exclusive"', async () => {
+        const space1 = 'space-to-remove';
+        const space2 = 'other-space';
+        const obj1 = { type: SHAREABLE_OBJ_TYPE, id: 'id-1', spaces: [space2] }; // will not be changed
+        const obj2 = { type: SHAREABLE_OBJ_TYPE, id: 'id-1', spaces: [space1, space2] }; // will be updated
+        const obj3 = { type: SHAREABLE_OBJ_TYPE, id: 'id-1', spaces: [space1] }; // will be deleted
+
+        const objects = [obj1, obj2, obj3];
+        const spacesToRemove = [space1];
+        const params = setup({ objects, spacesToRemove });
+        // this test case does not call mget
+        mockBulkResults({ error: false }, { error: false }); // result2 for obj2 and obj3
+
+        await updateObjectsSpaces(params);
+        expect(client.bulk).toHaveBeenCalledTimes(1);
+        expectBulkArgs(
+          { action: 'update', object: { ...obj2, namespaces: [space2] } },
+          { action: 'delete', object: obj3 }
+        );
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledTimes(2);
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenNthCalledWith(
+          1, // the first call resulted in an error which generated a log message (see assertion below)
+          expect.objectContaining({
+            type: obj2.type,
+            id: obj2.id,
+            namespaces: [space1],
+            deleteBehavior: 'inclusive',
+          })
+        );
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenNthCalledWith(
+          2,
+          expect.objectContaining({
+            type: obj3.type,
+            id: obj3.id,
+            namespaces: [space1],
+            deleteBehavior: 'inclusive',
+          })
+        );
+        expect(params.logger.error).not.toHaveBeenCalled();
+      });
+
+      it('deletes aliases for objects that were removed from all spaces using "deleteBehavior: inclusive"', async () => {
+        const space1 = 'space-to-add';
+        const space2 = 'other-space';
+        const obj1 = { type: SHAREABLE_OBJ_TYPE, id: 'id-1', spaces: [space2] }; // will be updated to add space1
+        const obj2 = {
+          type: SHAREABLE_OBJ_TYPE,
+          id: 'id-2',
+          spaces: [space2, ALL_NAMESPACES_STRING], // will be updated to add space1 and remove *
+        };
+
+        const objects = [obj1, obj2];
+        const spacesToAdd = [space1];
+        const spacesToRemove = [ALL_NAMESPACES_STRING];
+        const params = setup({ objects, spacesToAdd, spacesToRemove });
+        // this test case does not call mget
+        mockBulkResults({ error: false }); // result for obj1
+
+        await updateObjectsSpaces(params);
+        expect(client.bulk).toHaveBeenCalledTimes(1);
+        expectBulkArgs(
+          { action: 'update', object: { ...obj1, namespaces: [space2, space1] } },
+          { action: 'update', object: { ...obj2, namespaces: [space2, space1] } }
+        );
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledTimes(1);
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledWith(
+          expect.objectContaining({
+            type: obj2.type,
+            id: obj2.id,
+            namespaces: [space2, space1],
+            deleteBehavior: 'exclusive',
+          })
+        );
+        expect(params.logger.error).not.toHaveBeenCalled();
+      });
+
+      it('logs a message when deleteLegacyUrlAliases returns an error', async () => {
+        const space1 = 'space-to-remove';
+        const space2 = 'other-space';
+        const obj1 = { type: SHAREABLE_OBJ_TYPE, id: 'id-1', spaces: [space1, space2] }; // will be updated
+
+        const objects = [obj1];
+        const spacesToRemove = [space1];
+        const params = setup({ objects, spacesToRemove });
+        // this test case does not call mget
+        mockBulkResults({ error: false }); // result for obj1
+        mockDeleteLegacyUrlAliases.mockRejectedValueOnce(new Error('Oh no!')); // result for deleting aliases for obj1
+
+        await updateObjectsSpaces(params);
+        expect(client.bulk).toHaveBeenCalledTimes(1);
+        expectBulkArgs({ action: 'update', object: { ...obj1, namespaces: [space2] } });
+        expect(mockDeleteLegacyUrlAliases).toHaveBeenCalledTimes(1); // don't assert deleteLegacyUrlAliases args, we have tests for that above
+        expect(params.logger.error).toHaveBeenCalledTimes(1);
+        expect(params.logger.error).toHaveBeenCalledWith('Oh no!');
+      });
+    });
   });
 
   describe('returns expected results', () => {