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]Fix error toasts in sample data page #8842

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8842.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- [Workspace]Fix error toasts in sample data page ([#8842](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8842))
12 changes: 11 additions & 1 deletion src/plugins/home/public/application/sample_data_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,16 @@ export async function listSampleDataSets(dataSourceId) {
return await getServices().http.get(sampleDataUrl, { query });
}

const isWorkspaceEnabled = () => {
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking if it is workspaceEnabled or permissionControlEnabled.

const workspaces = getServices().application.capabilities.workspaces;
return !!(workspaces && workspaces.enabled);
};

export async function installSampleDataSet(id, sampleDataDefaultIndex, dataSourceId) {
const query = buildQuery(dataSourceId);
await getServices().http.post(`${sampleDataUrl}/${id}`, { query });

if (getServices().uiSettings.isDefault('defaultIndex')) {
if (!isWorkspaceEnabled() && getServices().uiSettings.isDefault('defaultIndex')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add a unit test to cover this condition check?

getServices().uiSettings.set('defaultIndex', sampleDataDefaultIndex);
}

Expand All @@ -59,6 +64,11 @@ export async function uninstallSampleDataSet(id, sampleDataDefaultIndex, dataSou
const uiSettings = getServices().uiSettings;

if (
/**
* Skip this logic when workspace is enabled, as the default index pattern UI setting
* will be removed in the workspace UI setting wrapper.
*/
!isWorkspaceEnabled() &&
!uiSettings.isDefault('defaultIndex') &&
uiSettings.get('defaultIndex') === sampleDataDefaultIndex
) {
Expand Down
152 changes: 152 additions & 0 deletions src/plugins/home/public/application/sample_data_client.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { setServices } from '../application/opensearch_dashboards_services';
import { installSampleDataSet, uninstallSampleDataSet } from './sample_data_client';

const mockHttp = {
post: jest.fn(),
delete: jest.fn(),
};

const mockUiSettings = {
isDefault: jest.fn(),
set: jest.fn(),
get: jest.fn(),
};

const mockApplication = {
capabilities: {
workspaces: {
enabled: false,
},
},
};

const mockIndexPatternService = {
clearCache: jest.fn(),
};

const mockServices = {
http: mockHttp,
uiSettings: mockUiSettings,
application: mockApplication,
indexPatternService: mockIndexPatternService,
};

setServices(mockServices);

describe('installSampleDataSet', () => {
beforeEach(() => {
jest.clearAllMocks();
mockUiSettings.isDefault.mockReturnValue(true);
setServices(mockServices);
});

it('should install the sample data set and set the default index', async () => {
const id = 'sample-data-id';
const sampleDataDefaultIndex = 'sample-data-index';
const dataSourceId = 'data-source-id';

await installSampleDataSet(id, sampleDataDefaultIndex, dataSourceId);

expect(mockHttp.post).toHaveBeenCalledWith(`/api/sample_data/${id}`, {
query: expect.anything(),
});
expect(mockUiSettings.set).toHaveBeenCalledWith('defaultIndex', sampleDataDefaultIndex);
expect(mockIndexPatternService.clearCache).toHaveBeenCalled();
});

it('should install the sample data set and not set the default index when workspace is enabled', async () => {
const id = 'sample-data-id';
const sampleDataDefaultIndex = 'sample-data-index';
const dataSourceId = 'data-source-id';

setServices({
...mockServices,
application: {
capabilities: {
workspaces: {
enabled: true,
},
},
},
});

await installSampleDataSet(id, sampleDataDefaultIndex, dataSourceId);

expect(mockHttp.post).toHaveBeenCalledWith(`/api/sample_data/${id}`, {
query: expect.anything(),
});
expect(mockUiSettings.set).not.toHaveBeenCalled();
expect(mockIndexPatternService.clearCache).toHaveBeenCalled();
});
});

describe('uninstallSampleDataSet', () => {
beforeEach(() => {
jest.clearAllMocks();
mockUiSettings.isDefault.mockReturnValue(false);
setServices(mockServices);
});

it('should uninstall the sample data set and clear the default index', async () => {
const id = 'sample-data-id';
const sampleDataDefaultIndex = 'sample-data-index';
const dataSourceId = 'data-source-id';

mockUiSettings.get.mockReturnValue(sampleDataDefaultIndex);

await uninstallSampleDataSet(id, sampleDataDefaultIndex, dataSourceId);

expect(mockHttp.delete).toHaveBeenCalledWith(`/api/sample_data/${id}`, {
query: expect.anything(),
});
expect(mockUiSettings.set).toHaveBeenCalledWith('defaultIndex', null);
expect(mockIndexPatternService.clearCache).toHaveBeenCalled();
});

it('should uninstall the sample data set and not clear the default index when workspace is enabled', async () => {
const id = 'sample-data-id';
const sampleDataDefaultIndex = 'sample-data-index';
const dataSourceId = 'data-source-id';

setServices({
...mockServices,
application: {
capabilities: {
workspaces: {
enabled: true,
},
},
},
});

await uninstallSampleDataSet(id, sampleDataDefaultIndex, dataSourceId);

expect(mockHttp.delete).toHaveBeenCalledWith(`/api/sample_data/${id}`, {
query: expect.anything(),
});
expect(mockUiSettings.set).not.toHaveBeenCalled();
expect(mockIndexPatternService.clearCache).toHaveBeenCalled();
});

it('should uninstall the sample data set and not clear the default index when it is not the sample data index', async () => {
const id = 'sample-data-id';
const sampleDataDefaultIndex = 'sample-data-index';
const dataSourceId = 'data-source-id';

mockUiSettings.isDefault.mockReturnValue(false);
mockUiSettings.get.mockReturnValue('other-index');

await uninstallSampleDataSet(id, sampleDataDefaultIndex, dataSourceId);

expect(mockHttp.delete).toHaveBeenCalledWith(`/api/sample_data/${id}`, {
query: expect.anything(),
});
expect(mockUiSettings.set).not.toHaveBeenCalled();
expect(mockIndexPatternService.clearCache).toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { CoreSetup, RequestHandlerContext } from 'src/core/server';
import { coreMock, httpServerMock } from '../../../../../../core/server/mocks';
import { updateWorkspaceState } from '../../../../../../core/server/utils';
import { SavedObjectsErrorHelpers } from '../../../../../../core/server';
import { flightsSpecProvider } from '../data_sets';
import { SampleDatasetSchema } from '../lib/sample_dataset_registry_types';
import { createUninstallRoute } from './uninstall';
Expand Down Expand Up @@ -131,4 +132,50 @@ describe('sample data uninstall route', () => {
expect(mockClient).toBeCalled();
expect(mockSOClient.delete).toBeCalled();
});

it('should response custom error when failed to delete index pattern inside workspace', async () => {
const mockWorkspaceId = 'workspace';
const mockContext = {
core: {
opensearch: {
legacy: {
client: { callAsCurrentUser: mockClient },
},
},
savedObjects: { client: mockSOClient },
},
};
const mockBody = { id: 'flights' };
const mockQuery = {};
const mockRequest = httpServerMock.createOpenSearchDashboardsRequest({
params: mockBody,
query: mockQuery,
});
updateWorkspaceState(mockRequest, { requestWorkspaceId: mockWorkspaceId });
const mockResponse = httpServerMock.createResponseFactory();

createUninstallRoute(mockCoreSetup.http.createRouter(), sampleDatasets, mockUsageTracker);

const mockRouter = mockCoreSetup.http.createRouter.mock.results[0].value;
const handler = mockRouter.delete.mock.calls[0][1];
mockContext.core.savedObjects.client.delete.mockImplementation(async () => {
throw SavedObjectsErrorHelpers.createBadRequestError(
'Failed to delete default index pattern setting'
);
});
const customErrorSpy = jest.spyOn(mockResponse, 'customError');

await handler((mockContext as unknown) as RequestHandlerContext, mockRequest, mockResponse);

expect(mockContext.core.savedObjects.client.delete).toHaveBeenCalledWith(
'index-pattern',
expect.any(String)
);
expect(customErrorSpy).toHaveBeenCalledWith({
statusCode: 400,
body: {
message: expect.stringContaining('Unable to delete sample dataset saved objects'),
},
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,40 @@ export function createUninstallRoute(
? context.dataSource.opensearch.legacy.getClient(dataSourceId).callAPI
: context.core.opensearch.legacy.client.callAsCurrentUser;

let savedObjectsList = getFinalSavedObjects({
dataset: sampleDataset,
workspaceId,
dataSourceId,
});

/**
* Try to delete index patterns first to avoid partial delete issues.
* This is necessary because default index patterns cannot be deleted
* when a workspace is enabled and the user lacks the required permissions.
Comment on lines +77 to +78
Copy link
Member

Choose a reason for hiding this comment

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

This is necessary because default index patterns cannot be deleted
when a workspace is enabled and the user lacks the required permissions.

I might miss something here, do we have special check on index pattern when deleting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, we will delete index patterns of sample data first when user want to uninstall sample data. So there are different cases when user try to uninstall sample data.

  • Case 1: Index pattern is default index pattern and user is dashboards admin or workspace admin, Index pattern can be removed
  • Case 2: Index pattern is default index pattern and user is workspace read and write and workspace read only, index pattern can't be removed
  • Case 3: Index pattern is not default index pattern and user is dashboards admin, workspace admin or workspace read & write, index pattern can be removed
  • Case 4: Index pattern is not default index pattern and user is workspace read only, index pattern can't be removed
    This PR will enhance case 1 and case 2, will try to remove ui settings first when deleted index patterns are default index pattern.

*/
if (workspaceId) {
const indexPatternDeletePromises = savedObjectsList
.filter(({ type }) => type === 'index-pattern')
.map(({ type, id }) => context.core.savedObjects.client.delete(type, id));

try {
await Promise.all(indexPatternDeletePromises);
} catch (err) {
// ignore 404s since users could have deleted some of the saved objects via the UI
if (_.get(err, 'output.statusCode') !== 404) {
return response.customError({
statusCode: err.status || _.get(err, 'output.statusCode'),
body: {
message: `Unable to delete sample dataset saved objects, error: ${err.message}`,
},
});
}
}

// Remove index patterns from the list of saved objects to be deleted
savedObjectsList = savedObjectsList.filter(({ type }) => type !== 'index-pattern');
}

for (let i = 0; i < sampleDataset.dataIndices.length; i++) {
const dataIndexConfig = sampleDataset.dataIndices[i];
const index =
Expand All @@ -83,12 +117,6 @@ export function createUninstallRoute(
}
}

const savedObjectsList = getFinalSavedObjects({
dataset: sampleDataset,
workspaceId,
dataSourceId,
});

const deletePromises = savedObjectsList.map(({ type, id }) =>
context.core.savedObjects.client.delete(type, id)
);
Expand All @@ -99,7 +127,7 @@ export function createUninstallRoute(
// ignore 404s since users could have deleted some of the saved objects via the UI
if (_.get(err, 'output.statusCode') !== 404) {
return response.customError({
statusCode: err.status,
statusCode: err.status || _.get(err, 'output.statusCode'),
body: {
message: `Unable to delete sample dataset saved objects, error: ${err.message}`,
},
Expand Down
Loading
Loading