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

[Saved Objects] Adds managed to import options #155677

Merged
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -293,11 +293,5 @@ export function setManaged({
optionsManaged?: boolean;
objectManaged?: boolean;
}): boolean {
if (optionsManaged !== undefined) {
return optionsManaged;
} else if (optionsManaged === undefined && objectManaged !== undefined) {
return objectManaged;
} else {
return false;
}
return optionsManaged ?? objectManaged ?? false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,10 @@ export interface LegacyUrlAlias {
* created because of saved object conversion, then we will display a toast telling the user that the object has a new URL.
*/
purpose?: 'savedObjectConversion' | 'savedObjectImport';
/**
* Flag indicating if a saved object is managed by Kibana (default=false).
* Only used when upserting a saved object. If the saved object already
* exist this option has no effect.
*/
managed?: boolean;
Comment on lines +62 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of surfacing this managed flag on our legacy url alias definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to set all objects created during import as managed if the option is provided. Legacy alias get created if they need to and the repository will set the default for managed as false if it's not specified.
In short: to bypass adding a default.

}
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export interface SavedObjectsImportFailure {
* If `overwrite` is specified, an attempt was made to overwrite an existing object.
*/
overwrite?: boolean;
managed?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here, I don't see it as being a problem, but just so I understand, why do we need to surface the managed flag on SO import failures/successes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a reference, that's all.

error:
| SavedObjectsImportConflictError
| SavedObjectsImportAmbiguousConflictError
Expand Down Expand Up @@ -125,6 +126,14 @@ export interface SavedObjectsImportSuccess {
* If `overwrite` is specified, this object overwrote an existing one (or will do so, in the case of a pending resolution).
*/
overwrite?: boolean;
/**
* Flag indicating if a saved object is managed by Kibana (default=false)
*
* This can be leveraged by applications to e.g. prevent edits to a managed
* saved object. Instead, users can be guided to create a copy first and
* make their edits to the copy.
*/
managed?: boolean;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,12 @@ describe('#importSavedObjectsFromStream', () => {
management: { icon: `${type}-icon` },
} as any),
importHooks = {},
managed,
}: {
createNewCopies?: boolean;
getTypeImpl?: (name: string) => any;
importHooks?: Record<string, SavedObjectsImportHook[]>;
managed?: boolean;
} = {}): ImportSavedObjectsOptions => {
readStream = new Readable();
savedObjectsClient = savedObjectsClientMock.create();
Expand All @@ -98,19 +100,23 @@ describe('#importSavedObjectsFromStream', () => {
namespace,
createNewCopies,
importHooks,
managed,
};
};
const createObject = ({
type = 'foo-type',
title = 'some-title',
}: { type?: string; title?: string } = {}): SavedObject<{
managed = undefined, // explicitly declare undefined so as not set to test against existing objects
}: { type?: string; title?: string; managed?: boolean } = {}): SavedObject<{
title: string;
managed?: boolean;
}> => {
return {
type,
id: uuidv4(),
references: [],
attributes: { title },
managed,
};
};
const createError = (): SavedObjectsImportFailure => {
Expand Down Expand Up @@ -320,6 +326,55 @@ describe('#importSavedObjectsFromStream', () => {
importStateMap,
overwrite,
namespace,
managed: options.managed,
};
expect(mockCreateSavedObjects).toHaveBeenCalledWith(createSavedObjectsParams);
});

test('creates managed saved objects', async () => {
const options = setupOptions({ managed: true });
const collectedObjects = [createObject({ managed: true })];
const filteredObjects = [createObject({ managed: false })];
const errors = [createError(), createError(), createError(), createError()];
mockCollectSavedObjects.mockResolvedValue({
errors: [errors[0]],
collectedObjects,
importStateMap: new Map([
['foo', {}],
['bar', {}],
['baz', { isOnlyReference: true }],
]),
});
mockCheckReferenceOrigins.mockResolvedValue({
importStateMap: new Map([['baz', { isOnlyReference: true, destinationId: 'newId1' }]]),
});
mockValidateReferences.mockResolvedValue([errors[1]]);
mockCheckConflicts.mockResolvedValue({
errors: [errors[2]],
filteredObjects,
importStateMap: new Map([['foo', { destinationId: 'newId2' }]]),
pendingOverwrites: new Set(),
});
mockCheckOriginConflicts.mockResolvedValue({
errors: [errors[3]],
importStateMap: new Map([['bar', { destinationId: 'newId3' }]]),
pendingOverwrites: new Set(),
});

await importSavedObjectsFromStream(options);
const importStateMap = new Map([
['foo', { destinationId: 'newId2' }],
['bar', { destinationId: 'newId3' }],
['baz', { isOnlyReference: true, destinationId: 'newId1' }],
]);
const createSavedObjectsParams = {
objects: collectedObjects,
accumulatedErrors: errors,
savedObjectsClient,
importStateMap,
overwrite,
namespace,
managed: options.managed,
};
expect(mockCreateSavedObjects).toHaveBeenCalledWith(createSavedObjectsParams);
});
Expand Down Expand Up @@ -382,6 +437,214 @@ describe('#importSavedObjectsFromStream', () => {
};
expect(mockCreateSavedObjects).toHaveBeenCalledWith(createSavedObjectsParams);
});

test('creates managed saved objects', async () => {
const options = setupOptions({ createNewCopies: true, managed: true });
const collectedObjects = [createObject({ managed: true })];
const errors = [createError(), createError()];
mockCollectSavedObjects.mockResolvedValue({
errors: [errors[0]],
collectedObjects,
importStateMap: new Map([
['foo', {}],
['bar', { isOnlyReference: true }],
]),
});
mockCheckReferenceOrigins.mockResolvedValue({
importStateMap: new Map([['bar', { isOnlyReference: true, destinationId: 'newId' }]]),
});
mockValidateReferences.mockResolvedValue([errors[1]]);
mockRegenerateIds.mockReturnValue(new Map([['foo', { destinationId: `randomId1` }]]));

await importSavedObjectsFromStream(options);
// assert that the importStateMap is correctly composed of the results from the three modules
const importStateMap: ImportStateMap = new Map([
['foo', { destinationId: `randomId1` }],
['bar', { isOnlyReference: true, destinationId: 'newId' }],
]);
const createSavedObjectsParams = {
objects: collectedObjects,
accumulatedErrors: errors,
savedObjectsClient,
importStateMap,
overwrite,
namespace,
managed: options.managed,
};
expect(mockCreateSavedObjects).toHaveBeenCalledWith(createSavedObjectsParams);
});
});

describe('managed option', () => {
test('if not provided, does not override existing property when already present, defaults to false for others', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looked briefly, but I think this test should focus on asserting the paramaters that we send to the create API

expect(mockCreateSavedObjects).toHaveBeenCalledWith(createSavedObjectsParams)

We already test createSavedObjects function's behaviour in other places, so mocking ES responses and checking the success results are less useful and I think these are already covered in existing tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already indirectly test that the params don't include the new option in creates saved objects.

I'll be more explicit in this one then.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Apr 27, 2023

Choose a reason for hiding this comment

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

I refactored all of them in this group of tests to assert on the params rather than the objects getting created.

const obj1 = createObject({ type: 'foo', managed: true });
const obj2 = createObject({ type: 'bar', title: 'bar-title', managed: false });

const options = setupOptions({
createNewCopies: false,
getTypeImpl: (type) => {
if (type === 'foo') {
return {
management: { getTitle: () => 'getTitle-foo', icon: `${type}-icon` },
};
}
return {
management: { icon: `${type}-icon` },
};
},
});

mockCheckConflicts.mockResolvedValue({
errors: [],
filteredObjects: [],
importStateMap: new Map(),
pendingOverwrites: new Set(),
});
mockCreateSavedObjects.mockResolvedValue({
errors: [],
createdObjects: [obj1, { ...obj2, managed: false }], // default applied in createSavedObjects
});

const result = await importSavedObjectsFromStream(options);
// successResults only includes the imported object's type, id, managed, and destinationId (if a new one was generated)
const successResults = [
{
type: obj1.type,
id: obj1.id,
meta: { title: 'getTitle-foo', icon: `${obj1.type}-icon` },
managed: true,
},
{
type: obj2.type,
id: obj2.id,
meta: { title: 'bar-title', icon: `${obj2.type}-icon` },
managed: false,
},
];

expect(result).toEqual({
success: true,
successCount: 2,
successResults,
warnings: [],
});
}); // assert that the documents being imported retain their prop or have the default applied

test('creates and converts objects from unmanaged to managed', async () => {
const obj1 = createObject({ type: 'foo', managed: false });
const obj2 = createObject({ type: 'bar', title: 'bar-title' });

const options = setupOptions({
createNewCopies: false,
managed: true,
getTypeImpl: (type) => {
if (type === 'foo') {
return {
management: { getTitle: () => 'getTitle-foo', icon: `${type}-icon` },
};
}
return {
management: { icon: `${type}-icon` },
};
},
});

mockCheckConflicts.mockResolvedValue({
errors: [],
filteredObjects: [],
importStateMap: new Map(),
pendingOverwrites: new Set(),
});
mockCreateSavedObjects.mockResolvedValue({
errors: [],
createdObjects: [
{ ...obj1, managed: true },
{ ...obj2, managed: true },
], // make sure the default isn't applied in createSavedObjects
});

const result = await importSavedObjectsFromStream(options);
// successResults only includes the imported object's type, id, managed and destinationId (if a new one was generated)
const successResults = [
{
type: obj1.type,
id: obj1.id,
meta: { title: 'getTitle-foo', icon: `${obj1.type}-icon` },
managed: true,
},
{
type: obj2.type,
id: obj2.id,
meta: { title: 'bar-title', icon: `${obj2.type}-icon` },
managed: true,
},
];

expect(result).toEqual({
success: true,
successCount: 2,
successResults,
warnings: [],
});
}); // assert that the documents being imported retain their prop or have the default applied

test('creates and converts objects from managed to unmanaged', async () => {
const obj1 = createObject({ type: 'foo', managed: true });
const obj2 = createObject({ type: 'bar', title: 'bar-title' });

const options = setupOptions({
createNewCopies: false,
managed: false,
getTypeImpl: (type) => {
if (type === 'foo') {
return {
management: { getTitle: () => 'getTitle-foo', icon: `${type}-icon` },
};
}
return {
management: { icon: `${type}-icon` },
};
},
});

mockCheckConflicts.mockResolvedValue({
errors: [],
filteredObjects: [],
importStateMap: new Map(),
pendingOverwrites: new Set(),
});
mockCreateSavedObjects.mockResolvedValue({
errors: [],
createdObjects: [
{ ...obj1, managed: false },
{ ...obj2, managed: false },
],
});

const result = await importSavedObjectsFromStream(options);
// successResults only includes the imported object's type, id, managed and destinationId (if a new one was generated)
const successResults = [
{
type: obj1.type,
id: obj1.id,
meta: { title: 'getTitle-foo', icon: `${obj1.type}-icon` },
managed: false,
},
{
type: obj2.type,
id: obj2.id,
meta: { title: 'bar-title', icon: `${obj2.type}-icon` },
managed: false,
},
];

expect(result).toEqual({
success: true,
successCount: 2,
successResults,
warnings: [],
});
}); // assert that the documents being imported retain their prop or have the default applied
});
});

Expand Down
Loading