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

[Workspace] Delete saved objects by workspace #6013

Merged
merged 10 commits into from
Mar 11, 2024
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multiple Datasource] Add Vega support to MDS by specifying a data source name in the Vega spec ([#5975](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5975))
- [Workspace] Consume workspace id in saved object client ([#6014](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6014))

Copy link
Member

Choose a reason for hiding this comment

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

Nit: useless empty line

- [Workspace] Add delete saved objects by workspace functionality([#6013](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6013))

### 🐛 Bug Fixes

- [BUG][Discover] Allow saved sort from search embeddable to load in Dashboard ([#5934](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5934))
Expand Down
1 change: 1 addition & 0 deletions src/core/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ export {
exportSavedObjectsToStream,
importSavedObjectsFromStream,
resolveSavedObjectsImportErrors,
SavedObjectsDeleteByWorkspaceOptions,
} from './saved_objects';

export {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const create = (): jest.Mocked<ISavedObjectsRepository> => ({
deleteFromNamespaces: jest.fn(),
deleteByNamespace: jest.fn(),
incrementCounter: jest.fn(),
deleteByWorkspace: jest.fn(),
});

export const savedObjectsRepositoryMock = { create };
79 changes: 79 additions & 0 deletions src/core/server/saved_objects/service/lib/repository.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2585,6 +2585,85 @@ describe('SavedObjectsRepository', () => {
});
});

describe('#deleteByWorkspace', () => {
const workspace = 'bar-workspace';
const mockUpdateResults = {
took: 15,
timed_out: false,
total: 3,
updated: 2,
deleted: 1,
batches: 1,
version_conflicts: 0,
noops: 0,
retries: { bulk: 0, search: 0 },
throttled_millis: 0,
requests_per_second: -1.0,
throttled_until_millis: 0,
failures: [],
};

const deleteByWorkspaceSuccess = async (workspace, options) => {
client.updateByQuery.mockResolvedValueOnce(
opensearchClientMock.createSuccessTransportRequestPromise(mockUpdateResults)
);
const result = await savedObjectsRepository.deleteByWorkspace(workspace, options);
expect(getSearchDslNS.getSearchDsl).toHaveBeenCalledTimes(1);
expect(client.updateByQuery).toHaveBeenCalledTimes(1);
return result;
};

describe('client calls', () => {
it(`should use the OpenSearch updateByQuery action`, async () => {
await deleteByWorkspaceSuccess(workspace);
expect(client.updateByQuery).toHaveBeenCalledTimes(1);
});

it(`should use all indices for all types`, async () => {
await deleteByWorkspaceSuccess(workspace);
expect(client.updateByQuery).toHaveBeenCalledWith(
expect.objectContaining({ index: ['.opensearch_dashboards_test', 'custom'] }),
expect.anything()
);
});
});

describe('errors', () => {
it(`throws when workspace is not a string or is '*'`, async () => {
const test = async (workspace) => {
await expect(savedObjectsRepository.deleteByWorkspace(workspace)).rejects.toThrowError(
`workspace is required, and must be a string that is not equal to '*'`
);
expect(client.updateByQuery).not.toHaveBeenCalled();
};
await test(undefined);
await test(null);
await test(['foo-workspace']);
await test(123);
await test(true);
await test(ALL_NAMESPACES_STRING);
});
});

describe('returns', () => {
it(`returns the query results on success`, async () => {
const result = await deleteByWorkspaceSuccess(workspace);
expect(result).toEqual(mockUpdateResults);
});
});

describe('search dsl', () => {
it(`constructs a query that have workspace as search critieria`, async () => {
await deleteByWorkspaceSuccess(workspace);
const allTypes = registry.getAllTypes().map((type) => type.name);
expect(getSearchDslNS.getSearchDsl).toHaveBeenCalledWith(mappings, registry, {
workspaces: [workspace],
type: allTypes,
});
});
});
});

describe('#find', () => {
const generateSearchResults = (namespace) => {
return {
Expand Down
50 changes: 50 additions & 0 deletions src/core/server/saved_objects/service/lib/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import {
SavedObjectsAddToNamespacesResponse,
SavedObjectsDeleteFromNamespacesOptions,
SavedObjectsDeleteFromNamespacesResponse,
SavedObjectsDeleteByWorkspaceOptions,
} from '../saved_objects_client';
import {
SavedObject,
Expand Down Expand Up @@ -714,6 +715,55 @@ export class SavedObjectsRepository {
return body;
}

/**
* Deletes all objects from the provided workspace.
*
* @param {string} workspace - workspace id
* @param options SavedObjectsDeleteByWorkspaceOptions
* @returns {promise} - { took, timed_out, total, deleted, batches, version_conflicts, noops, retries, failures }
*/
async deleteByWorkspace(
workspace: string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it taking the workspace name or id as the param ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if it's actually taking the id maybe let's try to name like workspaceId or add the detail in the comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make sense. As we are using workspace everywhere to reference as its id, to make the name consistent, let me add this to parameter comment.

options: SavedObjectsDeleteByWorkspaceOptions = {}
): Promise<any> {
if (!workspace || typeof workspace !== 'string' || workspace === '*') {
throw new TypeError(`workspace is required, and must be a string that is not equal to '*'`);
}

const allTypes = Object.keys(getRootPropertiesObjects(this._mappings));

const { body } = await this.client.updateByQuery(
{
index: this.getIndicesForTypes(allTypes),
refresh: options.refresh,
body: {
script: {
source: `
if (!ctx._source.containsKey('workspaces')) {
ctx.op = "delete";
} else {
ctx._source['workspaces'].removeAll(Collections.singleton(params['workspace']));
if (ctx._source['workspaces'].empty) {
ctx.op = "delete";
}
}
`,
lang: 'painless',
params: { workspace },
},
conflicts: 'proceed',
...getSearchDsl(this._mappings, this._registry, {
workspaces: [workspace],
type: allTypes,
}),
},
},
{ ignore: [404] }
);

return body;
}

/**
* @param {object} [options={}]
* @property {(string|Array<string>)} [options.type]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,18 @@ test(`#deleteFromNamespaces`, async () => {
expect(mockRepository.deleteFromNamespaces).toHaveBeenCalledWith(type, id, namespaces, options);
expect(result).toBe(returnValue);
});

test(`#deleteByWorkspace`, async () => {
const returnValue = Symbol();
const mockRepository = {
deleteByWorkspace: jest.fn().mockResolvedValue(returnValue),
};
const client = new SavedObjectsClient(mockRepository);

const workspace = Symbol();
const options = Symbol();
const result = await client.deleteByWorkspace(workspace, options);

expect(mockRepository.deleteByWorkspace).toHaveBeenCalledWith(workspace, options);
expect(result).toBe(returnValue);
});
21 changes: 21 additions & 0 deletions src/core/server/saved_objects/service/saved_objects_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,15 @@ export interface SavedObjectsUpdateResponse<T = unknown>
references: SavedObjectReference[] | undefined;
}

/**
*
* @public
*/
export interface SavedObjectsDeleteByWorkspaceOptions extends SavedObjectsBaseOptions {
/** The OpenSearch supports only boolean flag for this operation */
refresh?: boolean;
}

/**
*
* @public
Expand Down Expand Up @@ -446,6 +455,18 @@ export class SavedObjectsClient {
return await this._repository.deleteFromNamespaces(type, id, namespaces, options);
}

/**
* delete saved objects by workspace id
* @param workspace
* @param options
*/
deleteByWorkspace = async (
workspace: string,
options: SavedObjectsDeleteByWorkspaceOptions = {}
): Promise<any> => {
return await this._repository.deleteByWorkspace(workspace, options);
};

/**
* Bulk Updates multiple SavedObject at once
*
Expand Down
46 changes: 43 additions & 3 deletions src/plugins/workspace/server/integration_tests/routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { WorkspaceAttribute } from 'src/core/types';
import * as osdTestServer from '../../../../core/test_helpers/osd_server';
import { WORKSPACE_TYPE } from '../../../../core/server';

const omitId = <T extends { id?: string }>(object: T): Omit<T, 'id'> => {
const { id, ...others } = object;
Expand All @@ -20,6 +21,7 @@ const testWorkspace: WorkspaceAttribute = {
describe('workspace service', () => {
let root: ReturnType<typeof osdTestServer.createRoot>;
let opensearchServer: osdTestServer.TestOpenSearchUtils;
let osd: osdTestServer.TestOpenSearchDashboardsUtils;
beforeAll(async () => {
const { startOpenSearch, startOpenSearchDashboards } = osdTestServer.createTestServers({
adjustTimeout: (t: number) => jest.setTimeout(t),
Expand All @@ -28,12 +30,13 @@ describe('workspace service', () => {
workspace: {
enabled: true,
},
migrations: { skip: false },
},
},
});
opensearchServer = await startOpenSearch();
const startOSDResp = await startOpenSearchDashboards();
root = startOSDResp.root;
osd = await startOpenSearchDashboards();
root = osd.root;
});
afterAll(async () => {
await root.shutdown();
Expand All @@ -47,9 +50,13 @@ describe('workspace service', () => {
page: 1,
})
.expect(200);
const savedObjectsRepository = osd.coreStart.savedObjects.createInternalRepository([
WORKSPACE_TYPE,
]);
await Promise.all(
listResult.body.result.workspaces.map((item: WorkspaceAttribute) =>
osdTestServer.request.delete(root, `/api/workspaces/${item.id}`).expect(200)
// this will delete reserved workspace
savedObjectsRepository.delete(WORKSPACE_TYPE, item.id)
)
);
});
Expand Down Expand Up @@ -119,6 +126,16 @@ describe('workspace service', () => {
})
.expect(200);

await osdTestServer.request
.post(root, `/api/saved_objects/index-pattern/logstash-*`)
.send({
attributes: {
title: 'logstash-*',
},
workspaces: [result.body.result.id],
})
.expect(200);

await osdTestServer.request
.delete(root, `/api/workspaces/${result.body.result.id}`)
.expect(200);
Expand All @@ -129,6 +146,29 @@ describe('workspace service', () => {
);

expect(getResult.body.success).toEqual(false);

// saved objects been deleted
await osdTestServer.request
.get(root, `/api/saved_objects/index-pattern/logstash-*`)
.expect(404);
});
it('delete reserved workspace', async () => {
const reservedWorkspace: WorkspaceAttribute = { ...testWorkspace, reserved: true };
const result: any = await osdTestServer.request
.post(root, `/api/workspaces`)
.send({
attributes: omitId(reservedWorkspace),
})
.expect(200);

const deleteResult = await osdTestServer.request
.delete(root, `/api/workspaces/${result.body.result.id}`)
.expect(200);

expect(deleteResult.body.success).toEqual(false);
expect(deleteResult.body.error).toEqual(
`Reserved workspace ${result.body.result.id} is not allowed to delete.`
);
});
it('list', async () => {
await osdTestServer.request
Expand Down
1 change: 1 addition & 0 deletions src/plugins/workspace/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const workspaceAttributesSchema = schema.object({
color: schema.maybe(schema.string()),
icon: schema.maybe(schema.string()),
defaultVISTheme: schema.maybe(schema.string()),
reserved: schema.maybe(schema.boolean()),
});

export function registerRoutes({
Expand Down
18 changes: 17 additions & 1 deletion src/plugins/workspace/server/workspace_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,23 @@ export class WorkspaceClient implements IWorkspaceClientImpl {
}
public async delete(requestDetail: IRequestDetail, id: string): Promise<IResponse<boolean>> {
try {
await this.getSavedObjectClientsFromRequestDetail(requestDetail).delete(WORKSPACE_TYPE, id);
const savedObjectClient = this.getSavedObjectClientsFromRequestDetail(requestDetail);
const workspaceInDB: SavedObject<WorkspaceAttribute> = await savedObjectClient.get(
WORKSPACE_TYPE,
id
);
if (workspaceInDB.attributes.reserved) {
return {
success: false,
error: i18n.translate('workspace.deleteReservedWorkspace.errorMessage', {
defaultMessage: 'Reserved workspace {id} is not allowed to delete.',
values: { id: workspaceInDB.id },
}),
};
}
await savedObjectClient.deleteByWorkspace(id);
// delete workspace itself at last, deleteByWorkspace depends on the workspace to do permission check
await savedObjectClient.delete(WORKSPACE_TYPE, id);
return {
success: true,
result: true,
Expand Down
Loading