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.

Sign up for GitHub

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

Jump to bottom

[Multiple Datasource] Expose filterfn in datasource menu component to allow filter data sources before rendering in navigation bar #6113

Merged
merged 4 commits into from
Mar 12, 2024
Merged

[Multiple Datasource] Expose filterfn in datasource menu component to allow filter data sources before rendering in navigation bar #6113

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6088b41
expose filterfn in datasource menu component
BionIT Mar 11, 2024
2846c0a
add changelog
BionIT Mar 11, 2024
a534d7f
Merge branch 'main' into addfilterFn
BionIT Mar 12, 2024
1ce11a3
Merge branch 'main' into addfilterFn
BionIT Mar 12, 2024
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: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multiple Datasource] Test connection schema validation for registered auth types ([#6109](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6109))
- [Workspace] Consume workspace id in saved object client ([#6014](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6014))
- [Multiple Datasource] Export DataSourcePluginRequestContext at top level for plugins to use ([#6108](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6108))

- [Multiple Datasource] Expose filterfn in datasource menu component to allow filter data sources before rendering in navigation bar ([#6113](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6113))
- [Workspace] Add delete saved objects by workspace functionality([#6013](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6013))

### 🐛 Bug Fixes
Expand Down
76 changes: 76 additions & 0 deletions ...ent/public/components/data_source_menu/__snapshots__/data_source_selectable.test.tsx.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion ...ata_source_management/public/components/data_source_menu/create_data_source_menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('create data source menu', () => {
const component = render(<TestComponent {...props} />);
expect(component).toMatchSnapshot();
expect(client.find).toBeCalledWith({
fields: ['id', 'description', 'title'],
fields: ['id', 'title', 'auth.type'],
perPage: 10000,
type: 'data-source',
});
Expand Down
3 changes: 3 additions & 0 deletions src/plugins/data_source_management/public/components/data_source_menu/data_source_menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export interface DataSourceMenuProps {
className?: string;
selectedOption?: DataSourceOption[];
setMenuMountPoint?: (menuMount: MountPoint | undefined) => void;
filterFn?: (dataSource: any) => boolean;
}

export function DataSourceMenu(props: DataSourceMenuProps): ReactElement | null {
Expand All @@ -40,6 +41,7 @@ export function DataSourceMenu(props: DataSourceMenuProps): ReactElement | null
fullWidth,
hideLocalCluster,
selectedOption,
filterFn,
} = props;

if (!showDataSourceSelectable) {
Expand All @@ -66,6 +68,7 @@ export function DataSourceMenu(props: DataSourceMenuProps): ReactElement | null
onSelectedDataSource={dataSourceCallBackFunc}
disabled={disableDataSourceSelectable || false}
selectedOption={selectedOption && selectedOption.length > 0 ? selectedOption : undefined}
filterFn={filterFn}
/>
);
}
Expand Down
26 changes: 24 additions & 2 deletions ...data_source_management/public/components/data_source_menu/data_source_selectable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,21 @@ import { SavedObjectsClientContract } from '../../../../../core/public';
import { notificationServiceMock } from '../../../../../core/public/mocks';
import React from 'react';
import { DataSourceSelectable } from './data_source_selectable';
import { AuthType } from '../../types';
import { getDataSourcesWithFieldsResponse, mockResponseForSavedObjectsCalls } from '../../mocks';

describe('DataSourceSelectable', () => {
let component: ShallowWrapper<any, Readonly<{}>, React.Component<{}, {}, any>>;

let client: SavedObjectsClientContract;
const { toasts } = notificationServiceMock.createStartContract();
const nextTick = () => new Promise((res) => process.nextTick(res));

beforeEach(() => {
client = {
find: jest.fn().mockResolvedValue([]),
} as any;
mockResponseForSavedObjectsCalls(client, 'find', getDataSourcesWithFieldsResponse);
});

it('should render normally with local cluster not hidden', () => {
Expand All @@ -34,7 +38,7 @@ describe('DataSourceSelectable', () => {
);
expect(component).toMatchSnapshot();
expect(client.find).toBeCalledWith({
fields: ['id', 'description', 'title'],
fields: ['id', 'title', 'auth.type'],
perPage: 10000,
type: 'data-source',
});
Expand All @@ -54,10 +58,28 @@ describe('DataSourceSelectable', () => {
);
expect(component).toMatchSnapshot();
expect(client.find).toBeCalledWith({
fields: ['id', 'description', 'title'],
fields: ['id', 'title', 'auth.type'],
perPage: 10000,
type: 'data-source',
});
expect(toasts.addWarning).toBeCalledTimes(0);
});

it('should filter options if configured', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Where we configure this filterfn, is configurable in the yml file or somewhere or must change the code to configure the filter? thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This filterFn is passed by plugins when they consume the data source menu component, it is used then in the component when it is mounted to remove unwanted data sources before rendering

component = shallow(
<DataSourceSelectable
savedObjectsClient={client}
notifications={toasts}
onSelectedDataSource={jest.fn()}
disabled={false}
hideLocalCluster={false}
fullWidth={false}
filterFn={(ds) => ds.attributes.auth.type !== AuthType.NoAuth}
/>
);
component.instance().componentDidMount!();
await nextTick();
expect(component).toMatchSnapshot();
expect(toasts.addWarning).toBeCalledTimes(0);
});
});
25 changes: 16 additions & 9 deletions ...gins/data_source_management/public/components/data_source_menu/data_source_selectable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
EuiSpacer,
} from '@elastic/eui';
import { SavedObjectsClientContract, ToastsStart } from 'opensearch-dashboards/public';
import { getDataSources } from '../utils';
import { getDataSourcesWithFields } from '../utils';
import { DataSourceOption, LocalCluster } from '../data_source_selector/data_source_selector';

interface DataSourceSelectableProps {
Expand All @@ -26,6 +26,7 @@ interface DataSourceSelectableProps {
hideLocalCluster: boolean;
fullWidth: boolean;
selectedOption?: DataSourceOption[];
filterFn?: (dataSource: any) => boolean;
}

interface DataSourceSelectableState {
Expand Down Expand Up @@ -69,18 +70,24 @@ export class DataSourceSelectable extends React.Component<

async componentDidMount() {
this._isMounted = true;
getDataSources(this.props.savedObjectsClient)
getDataSourcesWithFields(this.props.savedObjectsClient, ['id', 'title', 'auth.type'])
Copy link
Member

Choose a reason for hiding this comment

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

how we extend this, which not only can filter auth.type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can extend by adding more fields, the fields entered here are to specify which fields should be returned from the API for data source object, and before, it restricts to id, title, description, auth type is required for security plugin to remove no auth data sources since they would indicate non-FGAC domains

.then((fetchedDataSources) => {
if (fetchedDataSources?.length) {
let dataSourceOptions = fetchedDataSources.map((dataSource) => ({
id: dataSource.id,
label: dataSource.title,
}));
let filteredDataSources = [];
if (this.props.filterFn) {
filteredDataSources = fetchedDataSources.filter((ds) => this.props.filterFn!(ds));
}

dataSourceOptions = dataSourceOptions.sort((a, b) =>
a.label.toLowerCase().localeCompare(b.label.toLowerCase())
);
if (filteredDataSources.length === 0) {
filteredDataSources = fetchedDataSources;
Copy link
Member

@xinruiba xinruiba Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If no filteredDataSources exist after apply filter, we will show all dataSource instead. And those datasource won't meet the filter condition.

Will it introduce in some confusions? What if we just show nothing.

Thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This filter is for plugins to decide how they want to render the options, there is possibility the filter would remove all options then there will be no options displayed. If plugins decide not to add filter options, then it should show all data sources, why it is confusing?

Copy link
Member

@xinruiba xinruiba Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This comment is not a blocker of this PR~

Suppose we have following dataSources:
[{datasource1, NoAuth}, {datasource2, NoAuth}, {datasource3, NoAuth}]

And filter out all "NoAuth" dataSources.

Looks like we will return [{datasource1, NoAuth}, {datasource2, NoAuth}, {datasource3, NoAuth}] here,
instead of an empty array. Is that expected?

Thanks~

}

const dataSourceOptions = filteredDataSources
.map((dataSource) => ({
id: dataSource.id,
label: dataSource.attributes?.title || '',
}))
.sort((a, b) => a.label.toLowerCase().localeCompare(b.label.toLowerCase()));
if (!this.props.hideLocalCluster) {
dataSourceOptions.unshift(LocalCluster);
}
Expand Down
Loading