Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change deleteByNamespace to include legacy URL aliases #115459

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/core/server/saved_objects/object_types/registration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const legacyUrlAliasType: SavedObjectsType = {
properties: {
sourceId: { type: 'keyword' },
targetType: { type: 'keyword' },
targetNamespace: { type: 'keyword' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this field to the mappings because we are now querying on it in the SOR

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)
Expand Down
7 changes: 6 additions & 1 deletion src/core/server/saved_objects/service/lib/repository.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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(),
});
});
});
Expand Down
16 changes: 14 additions & 2 deletions src/core/server/saved_objects/service/lib/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -55,6 +56,7 @@ import {
SavedObjectsBulkResolveObject,
SavedObjectsBulkResolveResponse,
} from '../saved_objects_client';
import { LEGACY_URL_ALIAS_TYPE } from '../../object_types';
import {
SavedObject,
SavedObjectsBaseOptions,
Expand Down Expand Up @@ -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,
];
Comment on lines -783 to +788
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Legacy URL aliases are space-agnostic by design (this made it easier for us to query/etc), but each one is only designed to be used in a single space.

TL;DR I had to add this in typesToUpdate.


// 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]);
Comment on lines +791 to +794
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈 (combined with the script param that removes documents not having a namespaces property)


const { body, statusCode, headers } = await this.client.updateByQuery(
{
Expand All @@ -803,8 +814,9 @@ export class SavedObjectsRepository {
},
conflicts: 'proceed',
...getSearchDsl(this._mappings, this._registry, {
namespaces: namespace ? [namespace] : undefined,
namespaces: [namespace],
Comment on lines -806 to +817
Copy link
Contributor Author

Choose a reason for hiding this comment

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

namespace is always defined (see L780-L782 above), so I removed this unnecessary ternary operator

type: typesToUpdate,
kueryNode,
}),
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
`,
},
Expand Down
53 changes: 24 additions & 29 deletions x-pack/test/spaces_api_integration/common/suites/delete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 = [
{
Expand All @@ -65,47 +70,37 @@ export function deleteTestSuiteFactory(
doc_count_error_upper_bound: 0,
sum_other_doc_count: 0,
buckets: [
Copy link
Contributor Author

@jportner jportner Oct 18, 2021

Choose a reason for hiding this comment

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

The spaces API integration test fixtures already include three legacy URL aliases:

  1. One in space_1
  2. One in space_2
  3. One in other_space (that doesn't actually exist)

Each test deletes space_2, so the agg buckets should reflect that aliases (1) and (3) still exist afterwards 👍

Copy link
Member

Choose a reason for hiding this comment

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

It might be good to memorialize these remarks as code comments so the next soul to wander in here has this context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done in 9be4f61

{
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);
Expand Down