Skip to content

Commit 79744fb

Browse files
yubonluoopensearch-changeset-bot[bot]
yubonluo
and
opensearch-changeset-bot[bot]
authored
[Fix] Revert workspaces=* and add warning log (opensearch-project#9848)
* revert workspace * and add warning log Signed-off-by: yubonluo <yubonluo@amazon.com> * add integration test Signed-off-by: yubonluo <yubonluo@amazon.com> * Changeset file for PR opensearch-project#9848 created/updated --------- Signed-off-by: yubonluo <yubonluo@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
1 parent 2355f15 commit 79744fb

File tree

5 files changed

+106
-13
lines changed

5 files changed

+106
-13
lines changed

‎changelogs/fragments/9848.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
fix:
2+
- Revert workspaces=* and add warning log ([#9848](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9848))

‎src/plugins/workspace/server/plugin.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
256256
core.savedObjects.addClientWrapper(
257257
PRIORITY_FOR_WORKSPACE_ID_CONSUMER_WRAPPER,
258258
WORKSPACE_ID_CONSUMER_WRAPPER_ID,
259-
new WorkspaceIdConsumerWrapper(this.client).wrapperFactory
259+
new WorkspaceIdConsumerWrapper(this.client, this.logger).wrapperFactory
260260
);
261261

262262
const maxImportExportSize = core.savedObjects.getImportExportObjectLimit();

‎src/plugins/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,33 @@ describe('workspace_id_consumer integration test', () => {
339339
);
340340
});
341341

342+
it('should not return 400 error when find with *', async () => {
343+
await osdTestServer.request
344+
.post(root, `/w/${createdFooWorkspace.id}/api/saved_objects/_bulk_create`)
345+
.send([
346+
{
347+
...dashboard,
348+
id: 'foo',
349+
},
350+
])
351+
.expect(200);
352+
353+
const findResult = await osdTestServer.request
354+
.get(
355+
root,
356+
`/w/${createdFooWorkspace.id}/api/saved_objects/_find?type=${dashboard.type}&workspaces=*`
357+
)
358+
.expect(200);
359+
360+
expect(findResult.body.total).toEqual(1);
361+
expect(findResult.body.saved_objects[0].workspaces).toEqual([createdFooWorkspace.id]);
362+
363+
await deleteItem({
364+
type: dashboard.type,
365+
id: 'foo',
366+
});
367+
});
368+
342369
it('should return error when find with a not existing workspace', async () => {
343370
const findResult = await osdTestServer.request
344371
.get(root, `/w/not_exist_workspace_id/api/saved_objects/_find?type=${dashboard.type}`)

‎src/plugins/workspace/server/saved_objects/workspace_id_consumer_wrapper.test.ts‎

Lines changed: 63 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,22 @@
55

66
import { updateWorkspaceState } from '../../../../core/server/utils';
77
import { SavedObject } from '../../../../core/public';
8-
import { httpServerMock, savedObjectsClientMock, coreMock } from '../../../../core/server/mocks';
8+
import {
9+
httpServerMock,
10+
savedObjectsClientMock,
11+
coreMock,
12+
loggingSystemMock,
13+
} from '../../../../core/server/mocks';
914
import { WorkspaceIdConsumerWrapper } from './workspace_id_consumer_wrapper';
1015
import { workspaceClientMock } from '../workspace_client.mock';
1116
import { SavedObjectsErrorHelpers } from '../../../../core/server';
1217

1318
describe('WorkspaceIdConsumerWrapper', () => {
1419
const requestHandlerContext = coreMock.createRequestHandlerContext();
20+
const loggingService = loggingSystemMock.create();
21+
const logger = loggingService.get();
1522
const mockedWorkspaceClient = workspaceClientMock.create();
16-
const wrapperInstance = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
23+
const wrapperInstance = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient, logger);
1724
const mockedClient = savedObjectsClientMock.create();
1825
const workspaceEnabledMockRequest = httpServerMock.createOpenSearchDashboardsRequest();
1926
updateWorkspaceState(workspaceEnabledMockRequest, {
@@ -77,7 +84,10 @@ describe('WorkspaceIdConsumerWrapper', () => {
7784
});
7885

7986
it(`Should throw error when passing in invalid workspaces`, async () => {
80-
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
87+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
88+
mockedWorkspaceClient,
89+
logger
90+
);
8191
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
8292
updateWorkspaceState(mockRequest, {});
8393
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
@@ -209,7 +219,10 @@ describe('WorkspaceIdConsumerWrapper', () => {
209219
});
210220

211221
it(`Should pass a empty workspace array`, async () => {
212-
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
222+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
223+
mockedWorkspaceClient,
224+
logger
225+
);
213226
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
214227
updateWorkspaceState(mockRequest, {});
215228
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
@@ -226,7 +239,10 @@ describe('WorkspaceIdConsumerWrapper', () => {
226239
});
227240

228241
it(`Should throw error when passing in invalid workspaces`, async () => {
229-
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
242+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
243+
mockedWorkspaceClient,
244+
logger
245+
);
230246
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
231247
updateWorkspaceState(mockRequest, {});
232248
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
@@ -243,6 +259,32 @@ describe('WorkspaceIdConsumerWrapper', () => {
243259
expect(mockedWorkspaceClient.get).toBeCalledTimes(0);
244260
expect(mockedWorkspaceClient.list).toBeCalledTimes(1);
245261
});
262+
263+
it(`Should not throw error when passing in '*'`, async () => {
264+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
265+
mockedWorkspaceClient,
266+
logger
267+
);
268+
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
269+
updateWorkspaceState(mockRequest, {});
270+
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
271+
client: mockedClient,
272+
typeRegistry: requestHandlerContext.savedObjects.typeRegistry,
273+
request: mockRequest,
274+
});
275+
await mockedWrapperClient.find({
276+
type: ['dashboard', 'visualization'],
277+
workspaces: ['*'],
278+
});
279+
expect(mockedClient.find).toBeCalledWith({
280+
type: ['dashboard', 'visualization'],
281+
});
282+
expect(logger.warn).toHaveBeenCalledWith(
283+
expect.stringContaining(
284+
'DEPRECATED: Using workspace="*" is deprecated and will be removed in future release.'
285+
)
286+
);
287+
});
246288
});
247289

248290
describe('get', () => {
@@ -309,7 +351,10 @@ describe('WorkspaceIdConsumerWrapper', () => {
309351
});
310352

311353
it(`Should get object when there is no workspace in options/request`, async () => {
312-
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
354+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
355+
mockedWorkspaceClient,
356+
logger
357+
);
313358
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
314359
updateWorkspaceState(mockRequest, {});
315360
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
@@ -366,7 +411,10 @@ describe('WorkspaceIdConsumerWrapper', () => {
366411
});
367412

368413
it(`Should get data source when user is data source admin`, async () => {
369-
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
414+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
415+
mockedWorkspaceClient,
416+
logger
417+
);
370418
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
371419
updateWorkspaceState(mockRequest, { isDataSourceAdmin: true, requestWorkspaceId: 'foo' });
372420
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
@@ -626,7 +674,10 @@ describe('WorkspaceIdConsumerWrapper', () => {
626674
});
627675

628676
it(`Should bulkGet objects when there is no workspace in options/request`, async () => {
629-
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
677+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
678+
mockedWorkspaceClient,
679+
logger
680+
);
630681
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
631682
updateWorkspaceState(mockRequest, {});
632683
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
@@ -656,7 +707,10 @@ describe('WorkspaceIdConsumerWrapper', () => {
656707
});
657708

658709
it(`Should bulkGet data source when user is data source admin`, async () => {
659-
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
710+
const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(
711+
mockedWorkspaceClient,
712+
logger
713+
);
660714
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
661715
updateWorkspaceState(mockRequest, { isDataSourceAdmin: true, requestWorkspaceId: 'foo' });
662716
const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({

‎src/plugins/workspace/server/saved_objects/workspace_id_consumer_wrapper.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
SavedObject,
1919
SavedObjectsBulkGetObject,
2020
SavedObjectsBulkResponse,
21+
Logger,
2122
} from '../../../../core/server';
2223
import { IWorkspaceClientImpl } from '../types';
2324
import { validateIsWorkspaceDataSourceAndConnectionObjectType } from '../../common/utils';
@@ -36,6 +37,7 @@ const generateSavedObjectsForbiddenError = () =>
3637
);
3738

3839
export class WorkspaceIdConsumerWrapper {
40+
private readonly logger: Logger;
3941
private formatWorkspaceIdParams<T extends WorkspaceOptions>(
4042
request: OpenSearchDashboardsRequest,
4143
options?: T
@@ -46,8 +48,14 @@ export class WorkspaceIdConsumerWrapper {
4648
const workspaceIdsInUserOptions = options?.workspaces;
4749
let finalWorkspaces: string[] = [];
4850
if (options?.hasOwnProperty('workspaces')) {
49-
// In order to get all data sources in workspace
50-
finalWorkspaces = workspaceIdsInUserOptions || [];
51+
if (workspaceIdsInUserOptions?.includes('*')) {
52+
this.logger.warn(
53+
'DEPRECATED: Using workspace="*" is deprecated and will be removed in future release.'
54+
);
55+
}
56+
// In order to get all data sources in workspace, use * to skip appending workspace id automatically
57+
// TODO: Remove workspace="*" in future release.
58+
finalWorkspaces = (workspaceIdsInUserOptions || []).filter((id) => id !== '*');
5159
} else if (workspaceIdParsedFromRequest) {
5260
finalWorkspaces = [workspaceIdParsedFromRequest];
5361
}
@@ -239,5 +247,7 @@ export class WorkspaceIdConsumerWrapper {
239247
};
240248
};
241249

242-
constructor(private readonly workspaceClient: IWorkspaceClientImpl) {}
250+
constructor(private readonly workspaceClient: IWorkspaceClientImpl, logger: Logger) {
251+
this.logger = logger;
252+
}
243253
}

0 commit comments

Comments
 (0)
⚡