From e4fb118fee9302d65e696c40adc3c44ff44c5a27 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Tue, 19 Oct 2021 10:44:15 -0400 Subject: [PATCH] Change deleteByNamespace to include legacy URL aliases (#115459) --- .../object_types/registration.ts | 1 + .../service/lib/repository.test.js | 7 ++- .../saved_objects/service/lib/repository.ts | 16 +++++- .../common/lib/space_test_utils.ts | 2 + .../common/suites/delete.ts | 53 +++++++++---------- 5 files changed, 47 insertions(+), 32 deletions(-) diff --git a/src/core/server/saved_objects/object_types/registration.ts b/src/core/server/saved_objects/object_types/registration.ts index 6ef4f79ef77c9..ce10896747178 100644 --- a/src/core/server/saved_objects/object_types/registration.ts +++ b/src/core/server/saved_objects/object_types/registration.ts @@ -17,6 +17,7 @@ const legacyUrlAliasType: SavedObjectsType = { properties: { sourceId: { type: 'keyword' }, targetType: { type: 'keyword' }, + targetNamespace: { type: 'keyword' }, resolveCounter: { type: 'long' }, disabled: { type: 'boolean' }, // other properties exist, but we aren't querying or aggregating on those, so we don't need to specify them (because we use `dynamic: false` above) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 82a0dd71700f6..84359147fccbc 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -26,6 +26,7 @@ import { encodeHitVersion } from '../../version'; import { SavedObjectTypeRegistry } from '../../saved_objects_type_registry'; import { DocumentMigrator } from '../../migrations/core/document_migrator'; import { mockKibanaMigrator } from '../../migrations/kibana/kibana_migrator.mock'; +import { LEGACY_URL_ALIAS_TYPE } from '../../object_types'; import { elasticsearchClientMock } from '../../../elasticsearch/client/mocks'; import * as esKuery from '@kbn/es-query'; import { errors as EsErrors } from '@elastic/elasticsearch'; @@ -2714,7 +2715,11 @@ describe('SavedObjectsRepository', () => { const allTypes = registry.getAllTypes().map((type) => type.name); expect(getSearchDslNS.getSearchDsl).toHaveBeenCalledWith(mappings, registry, { namespaces: [namespace], - type: allTypes.filter((type) => !registry.isNamespaceAgnostic(type)), + type: [ + ...allTypes.filter((type) => !registry.isNamespaceAgnostic(type)), + LEGACY_URL_ALIAS_TYPE, + ], + kueryNode: expect.anything(), }); }); }); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index e522d770b3f58..c081c59911405 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -8,6 +8,7 @@ import { omit, isObject } from 'lodash'; import type { estypes } from '@elastic/elasticsearch'; +import * as esKuery from '@kbn/es-query'; import type { ElasticsearchClient } from '../../../elasticsearch/'; import { isSupportedEsServer, isNotFoundFromUnsupportedServer } from '../../../elasticsearch'; import type { Logger } from '../../../logging'; @@ -55,6 +56,7 @@ import { SavedObjectsBulkResolveObject, SavedObjectsBulkResolveResponse, } from '../saved_objects_client'; +import { LEGACY_URL_ALIAS_TYPE } from '../../object_types'; import { SavedObject, SavedObjectsBaseOptions, @@ -780,7 +782,16 @@ export class SavedObjectsRepository { } const allTypes = Object.keys(getRootPropertiesObjects(this._mappings)); - const typesToUpdate = allTypes.filter((type) => !this._registry.isNamespaceAgnostic(type)); + const typesToUpdate = [ + ...allTypes.filter((type) => !this._registry.isNamespaceAgnostic(type)), + LEGACY_URL_ALIAS_TYPE, + ]; + + // Construct kueryNode to filter legacy URL aliases (these space-agnostic objects do not use root-level "namespace/s" fields) + const { buildNode } = esKuery.nodeTypes.function; + const match1 = buildNode('is', `${LEGACY_URL_ALIAS_TYPE}.targetNamespace`, namespace); + const match2 = buildNode('not', buildNode('is', 'type', LEGACY_URL_ALIAS_TYPE)); + const kueryNode = buildNode('or', [match1, match2]); const { body, statusCode, headers } = await this.client.updateByQuery( { @@ -803,8 +814,9 @@ export class SavedObjectsRepository { }, conflicts: 'proceed', ...getSearchDsl(this._mappings, this._registry, { - namespaces: namespace ? [namespace] : undefined, + namespaces: [namespace], type: typesToUpdate, + kueryNode, }), }, }, diff --git a/x-pack/test/spaces_api_integration/common/lib/space_test_utils.ts b/x-pack/test/spaces_api_integration/common/lib/space_test_utils.ts index 28b19d5db20b6..c047a741e35da 100644 --- a/x-pack/test/spaces_api_integration/common/lib/space_test_utils.ts +++ b/x-pack/test/spaces_api_integration/common/lib/space_test_utils.ts @@ -50,6 +50,8 @@ export function getAggregatedSpaceData(es: KibanaClient, objectTypes: string[]) emit(doc["namespaces"].value); } else if (doc["namespace"].size() > 0) { emit(doc["namespace"].value); + } else if (doc["legacy-url-alias.targetNamespace"].size() > 0) { + emit(doc["legacy-url-alias.targetNamespace"].value); } `, }, diff --git a/x-pack/test/spaces_api_integration/common/suites/delete.ts b/x-pack/test/spaces_api_integration/common/suites/delete.ts index aaca4fa843d67..4bf44d88db8e0 100644 --- a/x-pack/test/spaces_api_integration/common/suites/delete.ts +++ b/x-pack/test/spaces_api_integration/common/suites/delete.ts @@ -48,6 +48,7 @@ export function deleteTestSuiteFactory( 'dashboard', 'space', 'index-pattern', + 'legacy-url-alias', // TODO: add assertions for config objects -- these assertions were removed because of flaky behavior in #92358, but we should // consider adding them again at some point, especially if we convert config objects to `namespaceType: 'multiple-isolated'` in // the future. @@ -56,6 +57,10 @@ export function deleteTestSuiteFactory( // @ts-expect-error @elastic/elasticsearch doesn't defined `count.buckets`. const buckets = response.aggregations?.count.buckets; + // The test fixture contains three legacy URL aliases: + // (1) one for "space_1", (2) one for "space_2", and (3) one for "other_space", which is a non-existent space. + // Each test deletes "space_2", so the agg buckets should reflect that aliases (1) and (3) still exist afterwards. + // Space 2 deleted, all others should exist const expectedBuckets = [ { @@ -65,47 +70,37 @@ export function deleteTestSuiteFactory( doc_count_error_upper_bound: 0, sum_other_doc_count: 0, buckets: [ - { - key: 'visualization', - doc_count: 3, - }, - { - key: 'dashboard', - doc_count: 2, - }, - { - key: 'space', - doc_count: 2, - }, - { - key: 'index-pattern', - doc_count: 1, - }, + { key: 'visualization', doc_count: 3 }, + { key: 'dashboard', doc_count: 2 }, + { key: 'space', doc_count: 2 }, // since space objects are namespace-agnostic, they appear in the "default" agg bucket + { key: 'index-pattern', doc_count: 1 }, + // legacy-url-alias objects cannot exist for the default space ], }, }, { - doc_count: 6, + doc_count: 7, key: 'space_1', countByType: { doc_count_error_upper_bound: 0, sum_other_doc_count: 0, buckets: [ - { - key: 'visualization', - doc_count: 3, - }, - { - key: 'dashboard', - doc_count: 2, - }, - { - key: 'index-pattern', - doc_count: 1, - }, + { key: 'visualization', doc_count: 3 }, + { key: 'dashboard', doc_count: 2 }, + { key: 'index-pattern', doc_count: 1 }, + { key: 'legacy-url-alias', doc_count: 1 }, // alias (1) ], }, }, + { + doc_count: 1, + key: 'other_space', + countByType: { + doc_count_error_upper_bound: 0, + sum_other_doc_count: 0, + buckets: [{ key: 'legacy-url-alias', doc_count: 1 }], // alias (3) + }, + }, ]; expect(buckets).to.eql(expectedBuckets);