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

[Discover-next] Add query editor extensions #7034

Merged
merged 10 commits into from
Jun 20, 2024
2 changes: 2 additions & 0 deletions changelogs/fragments/7034.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Add search bar extensions ([#7034](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7034))
1 change: 1 addition & 0 deletions src/plugins/data/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ export {
TimeHistoryContract,
QueryStateChange,
QueryStart,
PersistedLog,
} from './query';

export { AggsStart } from './search/aggs';
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/data/public/ui/query_editor/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,5 @@ export const QueryEditor = (props: QueryEditorProps) => (
</React.Suspense>
);
export type { QueryEditorProps };

export { QueryEditorExtensions, QueryEditorExtensionConfig } from './query_editor_extensions';
8 changes: 8 additions & 0 deletions src/plugins/data/public/ui/query_editor/query_editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ export interface QueryEditorProps {
isInvalid?: boolean;
queryEditorHeaderRef: React.RefObject<HTMLDivElement>;
queryEditorHeaderClassName?: string;
joshuali925 marked this conversation as resolved.
Show resolved Hide resolved
queryEditorBannerRef: React.RefObject<HTMLDivElement>;
queryEditorBannerClassName?: string;
}

interface Props extends QueryEditorProps {
Expand Down Expand Up @@ -253,8 +255,14 @@ export default class QueryEditorUI extends Component<Props, State> {
this.props.queryEditorHeaderClassName
);

const queryEditorBannerClassName = classNames(
'osdQueryEditorBanner',
this.props.queryEditorBannerClassName
);

return (
<div className={className}>
<div ref={this.props.queryEditorBannerRef} className={queryEditorBannerClassName} />
<EuiFlexGroup gutterSize="xs" direction="column">
<EuiFlexItem grow={false}>
<EuiFlexGroup gutterSize="xs">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import React, { ComponentProps } from 'react';

const Fallback = () => <div />;

const LazyQueryEditorExtensions = React.lazy(() => import('./query_editor_extensions'));
export const QueryEditorExtensions = (props: ComponentProps<typeof LazyQueryEditorExtensions>) => (
<React.Suspense fallback={<Fallback />}>

Check warning on line 12 in src/plugins/data/public/ui/query_editor/query_editor_extensions/index.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data/public/ui/query_editor/query_editor_extensions/index.tsx#L12

Added line #L12 was not covered by tests
<LazyQueryEditorExtensions {...props} />
</React.Suspense>
);

export { QueryEditorExtensionConfig } from './query_editor_extension';
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { render, waitFor } from '@testing-library/react';
import React, { ComponentProps } from 'react';
import { IIndexPattern } from '../../../../common';
import { QueryEditorExtension } from './query_editor_extension';

jest.mock('react-dom', () => ({
...jest.requireActual('react-dom'),
createPortal: jest.fn((element) => element),
}));

type QueryEditorExtensionProps = ComponentProps<typeof QueryEditorExtension>;

const mockIndexPattern = {
id: '1234',
title: 'logstash-*',
fields: [
{
name: 'response',
type: 'number',
esTypes: ['integer'],
aggregatable: true,
filterable: true,
searchable: true,
},
],
} as IIndexPattern;

describe('QueryEditorExtension', () => {
const getComponentMock = jest.fn();
const getBannerMock = jest.fn();
const isEnabledMock = jest.fn();

const defaultProps: QueryEditorExtensionProps = {
config: {
id: 'test-extension',
order: 1,
isEnabled: isEnabledMock,
getComponent: getComponentMock,
getBanner: getBannerMock,
},
dependencies: {
indexPatterns: [mockIndexPattern],
language: 'Test',
},
componentContainer: document.createElement('div'),
bannerContainer: document.createElement('div'),
};

beforeEach(() => {
jest.clearAllMocks();
});

it('renders correctly when isEnabled is true', async () => {
isEnabledMock.mockResolvedValue(true);
getComponentMock.mockReturnValue(<div>Test Component</div>);
getBannerMock.mockReturnValue(<div>Test Banner</div>);

const { getByText } = render(<QueryEditorExtension {...defaultProps} />);

await waitFor(() => {
expect(getByText('Test Component')).toBeInTheDocument();
expect(getByText('Test Banner')).toBeInTheDocument();
});

expect(isEnabledMock).toHaveBeenCalled();
expect(getComponentMock).toHaveBeenCalledWith(defaultProps.dependencies);
});

it('does not render when isEnabled is false', async () => {
isEnabledMock.mockResolvedValue(false);
getComponentMock.mockReturnValue(<div>Test Component</div>);

const { queryByText } = render(<QueryEditorExtension {...defaultProps} />);

await waitFor(() => {
expect(queryByText('Test Component')).toBeNull();
});

expect(isEnabledMock).toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { EuiErrorBoundary } from '@elastic/eui';
import React, { useEffect, useMemo, useRef, useState } from 'react';
import ReactDOM from 'react-dom';
import { IIndexPattern } from '../../../../common';
import { DataSource } from '../../../data_sources/datasource';

interface QueryEditorExtensionProps {
config: QueryEditorExtensionConfig;
dependencies: QueryEditorExtensionDependencies;
componentContainer: Element;
bannerContainer: Element;
}

export interface QueryEditorExtensionDependencies {
/**
* Currently selected index patterns.
*/
indexPatterns?: Array<IIndexPattern | string>;
/**
* Currently selected data source.
*/
dataSource?: DataSource;
/**
* Currently selected query language.
*/
language: string;
}

export interface QueryEditorExtensionConfig {
/**
* The id for the search bar extension.
*/
id: string;
/**
* Lower order indicates higher position on UI.
*/
order: number;
/**
* A function that determines if the search bar extension is enabled and should be rendered on UI.
* @returns whether the extension is enabled.
*/
isEnabled: (dependencies: QueryEditorExtensionDependencies) => Promise<boolean>;
/**
* A function that returns the search bar extension component. The component
* will be displayed on top of the query editor in the search bar.
* @param dependencies - The dependencies required for the extension.
* @returns The component the search bar extension.
*/
getComponent?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null;
/**
* A function that returns the search bar extension banner. The banner is a
* component that will be displayed on top of the search bar.
* @param dependencies - The dependencies required for the extension.
* @returns The component the search bar extension.
*/
getBanner?: (dependencies: QueryEditorExtensionDependencies) => React.ReactElement | null;
}

const QueryEditorExtensionPortal: React.FC<{ container: Element }> = (props) => {
if (!props.children) return null;

return ReactDOM.createPortal(
<EuiErrorBoundary>{props.children}</EuiErrorBoundary>,
props.container
);
};

export const QueryEditorExtension: React.FC<QueryEditorExtensionProps> = (props) => {
const [isEnabled, setIsEnabled] = useState(false);
const isMounted = useRef(false);

const banner = useMemo(() => props.config.getBanner?.(props.dependencies), [
props.config,
props.dependencies,
]);

const component = useMemo(() => props.config.getComponent?.(props.dependencies), [
props.config,
props.dependencies,
]);

useEffect(() => {
isMounted.current = true;
return () => {
isMounted.current = false;
};
}, []);

useEffect(() => {
props.config.isEnabled(props.dependencies).then((enabled) => {
if (isMounted.current) setIsEnabled(enabled);
});
}, [props.dependencies, props.config]);

if (!isEnabled) return null;
Comment on lines +96 to +100
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: use braces even for single line if statements

Copy link
Member Author

Choose a reason for hiding this comment

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

i think this would be a good linter rule. I personally prefer one line if-returns, but i agree using braces is a better practice

i'll create an issue to track this and enable eslint curly, without linter there might be missed ones or mistakes


return (
<>
<QueryEditorExtensionPortal container={props.bannerContainer}>
{banner}
</QueryEditorExtensionPortal>
<QueryEditorExtensionPortal container={props.componentContainer}>
{component}
</QueryEditorExtensionPortal>
</>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { render, waitFor } from '@testing-library/react';
import React, { ComponentProps } from 'react';
import { QueryEditorExtension } from './query_editor_extension';
import QueryEditorExtensions from './query_editor_extensions';

type QueryEditorExtensionProps = ComponentProps<typeof QueryEditorExtension>;
type QueryEditorExtensionsProps = ComponentProps<typeof QueryEditorExtensions>;

jest.mock('./query_editor_extension', () => ({
QueryEditorExtension: jest.fn(({ config, dependencies }: QueryEditorExtensionProps) => (
<div>
Mocked QueryEditorExtension {config.id} with{' '}
{dependencies.indexPatterns?.map((i) => (typeof i === 'string' ? i : i.title)).join(', ')}
</div>
)),
}));

describe('QueryEditorExtensions', () => {
const defaultProps: QueryEditorExtensionsProps = {
indexPatterns: [
{
id: '1234',
title: 'logstash-*',
fields: [
{
name: 'response',
type: 'number',
esTypes: ['integer'],
aggregatable: true,
filterable: true,
searchable: true,
},
],
},
],
componentContainer: document.createElement('div'),
bannerContainer: document.createElement('div'),
language: 'Test',
};

beforeEach(() => {
jest.clearAllMocks();
});

it('renders without any configurations', () => {
const { container } = render(<QueryEditorExtensions {...defaultProps} />);
expect(container).toBeEmptyDOMElement();
});

it('renders without any items in map', () => {
const { container } = render(<QueryEditorExtensions {...defaultProps} configMap={{}} />);
expect(container).toBeEmptyDOMElement();
});

it('correctly orders configurations based on order property', () => {
const configMap = {
'1': { id: '1', order: 2, isEnabled: jest.fn(), getComponent: jest.fn() },
'2': { id: '2', order: 1, isEnabled: jest.fn(), getComponent: jest.fn() },
};

const { getAllByText } = render(
<QueryEditorExtensions {...defaultProps} configMap={configMap} />
);
const renderedExtensions = getAllByText(/Mocked QueryEditorExtension/);

expect(renderedExtensions).toHaveLength(2);
expect(renderedExtensions[0]).toHaveTextContent('2');
expect(renderedExtensions[1]).toHaveTextContent('1');
});

it('passes dependencies correctly to QueryEditorExtension', async () => {
const configMap = {
'1': { id: '1', order: 1, isEnabled: jest.fn(), getComponent: jest.fn() },
};

const { getByText } = render(<QueryEditorExtensions {...defaultProps} configMap={configMap} />);

await waitFor(() => {
expect(getByText(/logstash-\*/)).toBeInTheDocument();
});

expect(QueryEditorExtension).toHaveBeenCalledWith(
expect.objectContaining({
dependencies: { indexPatterns: defaultProps.indexPatterns, language: 'Test' },
}),
expect.anything()
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import React, { useMemo } from 'react';
import {
QueryEditorExtension,
QueryEditorExtensionConfig,
QueryEditorExtensionDependencies,
} from './query_editor_extension';

interface QueryEditorExtensionsProps extends QueryEditorExtensionDependencies {
configMap?: Record<string, QueryEditorExtensionConfig>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

q: is there a reason this has to be a map vs an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

consistency with existing code, see #7034 (comment)

componentContainer: Element;
bannerContainer: Element;
}

const QueryEditorExtensions: React.FC<QueryEditorExtensionsProps> = React.memo((props) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: is there a better name for this given its both a collection of extensions as well as specifying their destinations (and its easy to confuse components that are one character apart). perhaps its a manager as its responsibilities could include managing what gets rendered if we want more rules than rendering everything

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe QueryEditorExtensionManager and QueryEditorExtension? but I'm not very sure if a manager is something that should be rendered on the page, current impl needs QueryEditor to render QueryEditorExtensions. The one character naming existed in other places, like suggestion_component and suggestions_component, although i do find it to be confusing

const { configMap, componentContainer, bannerContainer, ...dependencies } = props;

const sortedConfigs = useMemo(() => {
if (!configMap || !Object.keys(configMap)) return [];
Copy link
Collaborator

@AMoo-Miki AMoo-Miki Jun 19, 2024

Choose a reason for hiding this comment

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

Object.keys will either return an array or throw. ![] is false always. If this was meant to prevent wasting time on an empty object, it fails to do that.

Suggested change
if (!configMap || !Object.keys(configMap)) return [];
if ('[object Object]' !== Object.prototype.toString.call(configMap) || Object.keys(configMap).length === 0) return [];

This will need linting though.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks good point on .length === 0, i missed that

is the object check necessary or conventional in OSD?

return Object.values(configMap).sort((a, b) => a.order - b.order);
}, [configMap]);

return (
<>
{sortedConfigs.map((config) => (
<QueryEditorExtension
key={config.id}
config={config}
dependencies={dependencies}
componentContainer={componentContainer}
bannerContainer={bannerContainer}
/>
))}
</>
);
});

// Needed for React.lazy
// eslint-disable-next-line import/no-default-export
export default QueryEditorExtensions;
Loading
Loading