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.

Sign up for GitHub

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

Jump to bottom

Prevent legacy url conflicts #116007

Merged
merged 15 commits into from
Oct 26, 2021
Merged

Prevent legacy url conflicts #116007

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1cb26fc
Refactor
jportner Oct 20, 2021
8954672
Add preflightCheckForCreate
jportner Oct 21, 2021
f696125
Change repository to use preflightCheckForCreate
jportner Oct 21, 2021
5b38264
Merge branch 'master' into issue-113335-prevent-legacy-url-conflicts
jportner Oct 25, 2021
3850deb
Address preliminary PR review feedback
jportner Oct 25, 2021
585e537
Update dev docs
jportner Oct 25, 2021
286d3ff
Add integration tests
jportner Oct 25, 2021
86ebd8c
More PR review feedback
jportner Oct 25, 2021
2d3b344
Change perPage for findLegacyUrlAliases
jportner Oct 25, 2021
d3383bd
Include alias checks for upserts
jportner Oct 25, 2021
0777f12
Merge branch 'master' into issue-113335-prevent-legacy-url-conflicts
jportner Oct 25, 2021
327de44
Revert change to export utils getObjKey delimiter
jportner Oct 25, 2021
393f714
Add update API integration tests
jportner Oct 25, 2021
8137c87
Merge branch 'master' into issue-113335-prevent-legacy-url-conflicts
jportner Oct 26, 2021
6c0f95a
Fix types
jportner Oct 26, 2021
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
9 changes: 9 additions & 0 deletions src/core/server/saved_objects/service/lib/collect_multi_namespace_references.test.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,17 @@
* Side Public License, v 1.
*/

import type { findLegacyUrlAliases } from './find_legacy_url_aliases';
import type * as InternalUtils from './internal_utils';

export const mockFindLegacyUrlAliases = jest.fn() as jest.MockedFunction<
typeof findLegacyUrlAliases
>;

jest.mock('./find_legacy_url_aliases', () => {
return { findLegacyUrlAliases: mockFindLegacyUrlAliases };
});

export const mockRawDocExistsInNamespace = jest.fn() as jest.MockedFunction<
typeof InternalUtils['rawDocExistsInNamespace']
>;
Expand Down
148 changes: 24 additions & 124 deletions src/core/server/saved_objects/service/lib/collect_multi_namespace_references.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@
* Side Public License, v 1.
*/

import { mockRawDocExistsInNamespace } from './collect_multi_namespace_references.test.mock';
import {
mockFindLegacyUrlAliases,
mockRawDocExistsInNamespace,
} from './collect_multi_namespace_references.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 { LegacyUrlAlias, LEGACY_URL_ALIAS_TYPE } from '../../object_types';
import type { ElasticsearchClient } from '../../../elasticsearch';
import { elasticsearchClientMock } from '../../../elasticsearch/client/mocks';
import { typeRegistryMock } from '../../saved_objects_type_registry.mock';
import { SavedObjectsSerializer } from '../../serialization';
import type {
Expand All @@ -21,11 +23,8 @@ import type {
SavedObjectsCollectMultiNamespaceReferencesOptions,
} from './collect_multi_namespace_references';
import { collectMultiNamespaceReferences } from './collect_multi_namespace_references';
import { savedObjectsPointInTimeFinderMock } from './point_in_time_finder.mock';
import { savedObjectsRepositoryMock } from './repository.mock';
import { PointInTimeFinder } from './point_in_time_finder';
import { ISavedObjectsRepository } from './repository';
import { SavedObjectsErrorHelpers } from './errors';
import type { CreatePointInTimeFinderFn } from './point_in_time_finder';

const SPACES = ['default', 'another-space'];
const VERSION_PROPS = { _seq_no: 1, _primary_term: 1 };
Expand All @@ -36,17 +35,14 @@ const NON_MULTI_NAMESPACE_OBJ_TYPE = 'type-c';
const MULTI_NAMESPACE_HIDDEN_OBJ_TYPE = 'type-d';

beforeEach(() => {
mockFindLegacyUrlAliases.mockReset();
mockFindLegacyUrlAliases.mockResolvedValue(new Map()); // return an empty map by default
mockRawDocExistsInNamespace.mockReset();
mockRawDocExistsInNamespace.mockReturnValue(true); // return true by default
});

describe('collectMultiNamespaceReferences', () => {
let client: DeeplyMockedKeys<ElasticsearchClient>;
let savedObjectsMock: jest.Mocked<ISavedObjectsRepository>;
let createPointInTimeFinder: jest.MockedFunction<
CollectMultiNamespaceReferencesParams['createPointInTimeFinder']
>;
let pointInTimeFinder: DeeplyMockedKeys<PointInTimeFinder>;

/** Sets up the type registry, saved objects client, etc. and return the full parameters object to be passed to `collectMultiNamespaceReferences` */
function setup(
Expand All @@ -68,20 +64,6 @@ describe('collectMultiNamespaceReferences', () => {
client = elasticsearchClientMock.createElasticsearchClient();

const serializer = new SavedObjectsSerializer(registry);
savedObjectsMock = savedObjectsRepositoryMock.create();
savedObjectsMock.find.mockResolvedValue({
pit_id: 'foo',
saved_objects: [],
// the rest of these fields don't matter but are included for type safety
total: 0,
page: 1,
per_page: 100,
});
createPointInTimeFinder = jest.fn();
createPointInTimeFinder.mockImplementation((params) => {
pointInTimeFinder = savedObjectsPointInTimeFinderMock.create({ savedObjectsMock })(params);
return pointInTimeFinder;
});
return {
registry,
allowedTypes: [
Expand All @@ -92,7 +74,7 @@ describe('collectMultiNamespaceReferences', () => {
client,
serializer,
getIndexForType: (type: string) => `index-for-${type}`,
createPointInTimeFinder,
createPointInTimeFinder: jest.fn() as CreatePointInTimeFinderFn,
objects,
options,
};
Expand Down Expand Up @@ -131,23 +113,6 @@ describe('collectMultiNamespaceReferences', () => {
);
}

function mockFindResults(...results: LegacyUrlAlias[]) {
savedObjectsMock.find.mockResolvedValueOnce({
pit_id: 'foo',
saved_objects: results.map((attributes) => ({
id: 'doesnt-matter',
type: LEGACY_URL_ALIAS_TYPE,
attributes,
references: [],
score: 0, // doesn't matter
})),
// the rest of these fields don't matter but are included for type safety
total: 0,
page: 1,
per_page: 100,
});
}

/** Asserts that mget is called for the given objects */
function expectMgetArgs(
n: number,
Expand Down Expand Up @@ -338,45 +303,26 @@ describe('collectMultiNamespaceReferences', () => {
});

describe('legacy URL aliases', () => {
it('uses the PointInTimeFinder to search for legacy URL aliases', async () => {
it('uses findLegacyUrlAliases to search for legacy URL aliases', async () => {
const obj1 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-1' };
const obj2 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-2' };
const obj3 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-3' };
const params = setup([obj1, obj2], {});
mockMgetResults({ found: true, references: [obj3] }, { found: true, references: [] }); // results for obj1 and obj2
mockMgetResults({ found: true, references: [] }); // results for obj3
mockFindResults(
// mock search results for four aliases for obj1, and none for obj2 or obj3
...[1, 2, 3, 4].map((i) => ({
sourceId: obj1.id,
targetId: 'doesnt-matter',
targetType: obj1.type,
targetNamespace: `space-${i}`,
}))
mockFindLegacyUrlAliases.mockResolvedValue(
new Map([
[`${obj1.type}:${obj1.id}`, new Set(['space-1', 'space-2', 'space-3', 'space-4'])],
// the result map does not contain keys for obj2 or obj3 because we did not find any aliases for those objects
])
);

const result = await collectMultiNamespaceReferences(params);
expect(client.mget).toHaveBeenCalledTimes(2);
expectMgetArgs(1, obj1, obj2);
expectMgetArgs(2, obj3); // obj3 is retrieved in a second cluster call
expect(createPointInTimeFinder).toHaveBeenCalledTimes(1);
const kueryFilterArgs = createPointInTimeFinder.mock.calls[0][0].filter.arguments;
expect(kueryFilterArgs).toHaveLength(2);
const typeAndIdFilters = kueryFilterArgs[1].arguments;
expect(typeAndIdFilters).toHaveLength(3);
[obj1, obj2, obj3].forEach(({ type, id }, i) => {
const typeAndIdFilter = typeAndIdFilters[i].arguments;
expect(typeAndIdFilter).toEqual([
expect.objectContaining({
arguments: expect.arrayContaining([{ type: 'literal', value: type }]),
}),
expect.objectContaining({
arguments: expect.arrayContaining([{ type: 'literal', value: id }]),
}),
]);
});
expect(pointInTimeFinder.find).toHaveBeenCalledTimes(1);
expect(pointInTimeFinder.close).toHaveBeenCalledTimes(2);
expect(mockFindLegacyUrlAliases).toHaveBeenCalledTimes(1);
expect(mockFindLegacyUrlAliases).toHaveBeenCalledWith(expect.anything(), [obj1, obj2, obj3]);
expect(result.objects).toEqual([
{
...obj1,
Expand All @@ -389,74 +335,28 @@ describe('collectMultiNamespaceReferences', () => {
]);
});

it('does not create a PointInTimeFinder if no objects are passed in', async () => {
const params = setup([]);

await collectMultiNamespaceReferences(params);
expect(params.createPointInTimeFinder).not.toHaveBeenCalled();
});

it('does not search for objects that have an empty spaces array (the object does not exist, or we are not sure)', async () => {
it('omits objects that have an empty spaces array (the object does not exist, or we are not sure)', async () => {
const obj1 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-1' };
const obj2 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-2' };
const params = setup([obj1, obj2]);
mockMgetResults({ found: true }, { found: false }); // results for obj1 and obj2

await collectMultiNamespaceReferences(params);
expect(createPointInTimeFinder).toHaveBeenCalledTimes(1);

const kueryFilterArgs = createPointInTimeFinder.mock.calls[0][0].filter.arguments;
expect(kueryFilterArgs).toHaveLength(2);
const typeAndIdFilters = kueryFilterArgs[1].arguments;
expect(typeAndIdFilters).toHaveLength(1);
const typeAndIdFilter = typeAndIdFilters[0].arguments;
expect(typeAndIdFilter).toEqual([
expect.objectContaining({
arguments: expect.arrayContaining([{ type: 'literal', value: obj1.type }]),
}),
expect.objectContaining({
arguments: expect.arrayContaining([{ type: 'literal', value: obj1.id }]),
}),
]);
expect(pointInTimeFinder.find).toHaveBeenCalledTimes(1);
expect(pointInTimeFinder.close).toHaveBeenCalledTimes(2);
});

it('does not search at all if all objects that have an empty spaces array (the object does not exist, or we are not sure)', async () => {
const obj1 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-1' };
const params = setup([obj1]);
mockMgetResults({ found: false }); // results for obj1

await collectMultiNamespaceReferences(params);
expect(params.createPointInTimeFinder).not.toHaveBeenCalled();
expect(mockFindLegacyUrlAliases).toHaveBeenCalledTimes(1);
expect(mockFindLegacyUrlAliases).toHaveBeenCalledWith(expect.anything(), [obj1]);
});

it('handles PointInTimeFinder.find errors', async () => {
it('handles findLegacyUrlAliases errors', async () => {
const obj1 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-1' };
const params = setup([obj1]);
mockMgetResults({ found: true }); // results for obj1
savedObjectsMock.find.mockRejectedValue(new Error('Oh no!'));

await expect(() => collectMultiNamespaceReferences(params)).rejects.toThrow(
'Failed to retrieve legacy URL aliases: Oh no!'
mockFindLegacyUrlAliases.mockRejectedValue(
new Error('Failed to retrieve legacy URL aliases: Oh no!')
);
expect(createPointInTimeFinder).toHaveBeenCalledTimes(1);
expect(pointInTimeFinder.find).toHaveBeenCalledTimes(1);
expect(pointInTimeFinder.close).toHaveBeenCalledTimes(2); // we still close the point-in-time, even though the search failed
});

it('handles PointInTimeFinder.close errors', async () => {
const obj1 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-1' };
const params = setup([obj1]);
mockMgetResults({ found: true }); // results for obj1
savedObjectsMock.closePointInTime.mockRejectedValue(new Error('Oh no!'));

await expect(() => collectMultiNamespaceReferences(params)).rejects.toThrow(
'Failed to retrieve legacy URL aliases: Oh no!'
);
expect(createPointInTimeFinder).toHaveBeenCalledTimes(1);
expect(pointInTimeFinder.find).toHaveBeenCalledTimes(1);
expect(pointInTimeFinder.close).toHaveBeenCalledTimes(2);
});
});
});
Loading