Skip to content

Commit

Permalink
Fixes conflict checks for sharing saved objects (elastic#168655)
Browse files Browse the repository at this point in the history 
Closes elastic#168049

## Summary

This PR adjusts the KQL filters when collecting references to saved
objects for the purpose of updating their spaces (i.e. sharing saved
objects to spaces). An additional filter is added to specifically
exclude ID -> ID matches - an ID match would mean the object has already
been shared to the destination space and there is no conflict. Filters
to match the shared object's ID or origin ID to the destination space's
objects' origin ID, and to match the shared object's origin ID to the
destination space's objects' IDs remain in place to properly check for
conflicts with potential copies.

### Manual Testing
- Create 2 spaces: A and B
- Add a sample data set (e.g. flight) to space A
- In Discover, create a saved query called "s1" with a filter pill that
uses the sample data logs data view
- Create another saved query called "s2" with a filter pill that uses
the sample data logs data view
- Go to `Stack Management->Saved` Objects and share the "s1" query to
space B
- Now share the "s2" query to space B. From the main branch you will see
that there is a conflict that disallows sharing the second query. This
is because it is also attempting to share the referenced data view,
which is already in space B. However, this should not be a conflict -
from this PR you will be able to successfully share both queries.

### Automated Testing
-
packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.test.ts
-
packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.test.ts
-
x-pack/test/spaces_api_integration/common/suites/get_shareable_references.ts

---------

Co-authored-by: kibanamachine <[email protected]>
  • Loading branch information
@jeramysoucy @kibanamachine
jeramysoucy and kibanamachine authored Oct 16, 2023
1 parent cb3ed24 commit 460f5e3
Show file tree
Hide file tree
Showing 7 changed files with 325 additions and 31 deletions.
33 changes: 27 additions & 6 deletions ...cts-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -411,11 +411,12 @@ describe('collectMultiNamespaceReferences', () => {
expect(mockFindSharedOriginObjects).toHaveBeenCalledWith(
expect.anything(),
[
{ type: obj1.type, origin: obj1.id },
{ type: obj2.type, origin: obj2.originId }, // If the found object has an `originId`, that is used instead of the object's `id`.
{ type: obj3.type, origin: obj3.id },
{ type: obj1.type, id: obj1.id },
{ type: obj2.type, id: obj2.id, origin: obj2.originId },
{ type: obj3.type, id: obj3.id },
],
ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE
ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE,
undefined
);
expect(result.objects).toEqual([
// Note: in a realistic scenario, `spacesWithMatchingOrigins` would be a superset of `spaces`. But for the purposes of this unit
Expand All @@ -441,8 +442,9 @@ describe('collectMultiNamespaceReferences', () => {
expect(mockFindSharedOriginObjects).toHaveBeenCalledTimes(1);
expect(mockFindSharedOriginObjects).toHaveBeenCalledWith(
expect.anything(),
[{ type: obj1.type, origin: obj1.id }],
ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE
[{ type: obj1.type, id: obj1.id }],
ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE,
undefined
);
});

Expand All @@ -458,6 +460,25 @@ describe('collectMultiNamespaceReferences', () => {
'Failed to retrieve shared origin objects: Oh no!'
);
});

it('passes options to findSharedOriginObjects', 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,
options: { purpose: 'updateObjectsSpaces' },
});
expect(mockFindSharedOriginObjects).toHaveBeenCalledTimes(1);
expect(mockFindSharedOriginObjects).toHaveBeenCalledWith(
expect.anything(),
[{ type: obj1.type, id: obj1.id }],
ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE,
'updateObjectsSpaces'
);
});
});

describe('with security enabled', () => {
Expand Down
6 changes: 4 additions & 2 deletions ...-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,14 @@ export async function collectMultiNamespaceReferences(
);
const objectOriginsToSearchFor = foundObjects.map(({ type, id, originId }) => ({
type,
origin: originId || id,
id,
origin: originId,
}));
const originsMap = await findSharedOriginObjects(
createPointInTimeFinder,
objectOriginsToSearchFor,
ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE
ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE,
options?.purpose
);
const results = objectsWithContext.map((obj) => {
const aliasesVal = aliasesMap.get(getObjectKey(obj));
Expand Down
224 changes: 220 additions & 4 deletions ...e-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@ describe('findSharedOriginObjects', () => {
});
}

const obj1 = { type: 'type-1', origin: 'id-1' };
const obj2 = { type: 'type-2', origin: 'id-2' };
const obj3 = { type: 'type-3', origin: 'id-3' };
const obj4 = { type: 'type-4', origin: 'id-4' };
const obj1 = { type: 'type-1', id: 'id-1', origin: 'origin-1' };
const obj2 = { type: 'type-2', id: 'id-2', origin: 'origin-2' };
const obj3 = { type: 'type-3', id: 'id-3', origin: 'origin-3' };
const obj4 = { type: 'type-4', id: 'id-4', origin: 'origin-4' };

it('uses the PointInTimeFinder to search for legacy URL aliases', async () => {
mockFindResults(
Expand Down Expand Up @@ -156,4 +156,220 @@ describe('findSharedOriginObjects', () => {
expect(pointInTimeFinder.find).toHaveBeenCalledTimes(1);
expect(pointInTimeFinder.close).toHaveBeenCalledTimes(2);
});

describe(`when options.purpose is 'updateObjectsSpaces'`, () => {
it('calls createPointInTimeFinder with filter to ignore direct ID matches', async () => {
const objects = [obj1, obj2, obj3];
await findSharedOriginObjects(createPointInTimeFinder, objects, 999, 'updateObjectsSpaces');
expect(createPointInTimeFinder).toHaveBeenCalledTimes(1);
expect(createPointInTimeFinder).toHaveBeenCalledWith(
expect.objectContaining({
filter: expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: [
{
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-1.id',
},
{
isQuoted: false,
type: 'literal',
value: 'type-1:id-1',
},
],
function: 'is',
type: 'function',
},
],
function: 'not',
type: 'function',
}),
]),
}),
expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: [
{
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-2.id',
},
{
isQuoted: false,
type: 'literal',
value: 'type-2:id-2',
},
],
function: 'is',
type: 'function',
},
],
function: 'not',
type: 'function',
}),
]),
}),
expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: [
{
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-3.id',
},
{
isQuoted: false,
type: 'literal',
value: 'type-3:id-3',
},
],
function: 'is',
type: 'function',
},
],
function: 'not',
type: 'function',
}),
]),
}),
]),
}),
}),
undefined,
{ disableExtensions: true }
);
});

it('calls createPointInTimeFinder without redundant filter when object does not have an origin ID', async () => {
const objects = [obj1, { ...obj2, origin: undefined }, obj3];
await findSharedOriginObjects(createPointInTimeFinder, objects, 999, 'updateObjectsSpaces');
expect(createPointInTimeFinder).toHaveBeenCalledTimes(1);
expect(createPointInTimeFinder).toHaveBeenCalledWith(
expect.objectContaining({
filter: expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: [
{
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-1.id',
},
{
isQuoted: false,
type: 'literal',
value: 'type-1:origin-1',
},
],
function: 'is',
type: 'function',
},
{
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-1.originId',
},
{
isQuoted: false,
type: 'literal',
value: 'origin-1',
},
],
function: 'is',
type: 'function',
},
],
function: 'or',
type: 'function',
}),
]),
}),
expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-2.originId',
},
{
isQuoted: false,
type: 'literal',
value: 'id-2',
},
],
function: 'is',
type: 'function',
}),
]),
}),
expect.objectContaining({
arguments: expect.arrayContaining([
expect.objectContaining({
arguments: [
{
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-3.id',
},
{
isQuoted: false,
type: 'literal',
value: 'type-3:origin-3',
},
],
function: 'is',
type: 'function',
},
{
arguments: [
{
isQuoted: false,
type: 'literal',
value: 'type-3.originId',
},
{
isQuoted: false,
type: 'literal',
value: 'origin-3',
},
],
function: 'is',
type: 'function',
},
],
function: 'or',
type: 'function',
}),
]),
}),
]),
}),
}),
undefined,
{ disableExtensions: true }
);
});
});
});
Loading

0 comments on commit 460f5e3

Please sign in to comment.