Skip to content

Commit

Permalink
Fix object empty check and minor perf issue in query editor extensions (
Browse files Browse the repository at this point in the history 
#7077)

Follow up for #7034, this PR
- addresses #7034 (comment)
- addresses #7034 (comment)
- fixes render order
   - QueryEditorExtensions requires the container divs to be mounted first,
but in the previous implementation, extensions will be mounted first and
it relied on rerendering of queryEditorTopRow. Moving them into query
editor fixes the render order and ensures refs are set.

@AMoo-Miki I didn't use the object check `'[object Object]' !== Object.prototype.toString.call(configMap)`. I don't know what access user has, but if an attacker can arbitrarily alter `configMap`, the code will still break with something like
```tsx
        configMap={
          new Proxy(
            {},
            {
              ownKeys(target) {
                throw new Error('Accessing keys is not allowed.');
              },
            }
          )
        }
```

Given that our code creates the config map here, I think relying on static type check is enough, but feel free to comment if otherwise.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/7f0e39eb9809c95b98069cc971611edc2cbbc62b/src/plugins/data/public/ui/ui_service.ts#L31

Signed-off-by: Joshua Li <joshuali925@gmail.com>
  • Loading branch information
@joshuali925
joshuali925 authored Jun 28, 2024
1 parent 5ee6f58 commit b85e177
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 67 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/7077.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Fix object empty check and minor perf issue in query editor extensions ([#7077](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7077))
73 changes: 38 additions & 35 deletions src/plugins/data/public/ui/query_editor/query_editor.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import classNames from 'classnames';
import { isEqual } from 'lodash';
import React, { Component, createRef, RefObject } from 'react';
import { Settings } from '..';
import { IDataPluginServices, IIndexPattern, Query, TimeRange } from '../..';
import { DataSource, IDataPluginServices, IIndexPattern, Query, TimeRange } from '../..';
import {
CodeEditor,
OpenSearchDashboardsReactContextValue,
Expand All @@ -19,9 +19,11 @@ import { SuggestionsListSize } from '../typeahead/suggestions_component';
import { DataSettings } from '../types';
import { fetchIndexPatterns } from './fetch_index_patterns';
import { QueryLanguageSelector } from './language_selector';
import { QueryEditorExtensions } from './query_editor_extensions';

export interface QueryEditorProps {
indexPatterns: Array<IIndexPattern | string>;
dataSource?: DataSource;
query: Query;
containerRef?: React.RefCallback<HTMLDivElement>;
settings: Settings;
Expand All @@ -41,10 +43,9 @@ export interface QueryEditorProps {
size?: SuggestionsListSize;
className?: string;
isInvalid?: boolean;
queryEditorHeaderRef: React.RefObject<HTMLDivElement>;
queryEditorHeaderClassName?: string;
queryEditorBannerRef: React.RefObject<HTMLDivElement>;
queryEditorBannerClassName?: string;
queryLanguage?: string;
headerClassName?: string;
bannerClassName?: string;
}

interface Props extends QueryEditorProps {
Expand All @@ -57,7 +58,6 @@ interface State {
index: number | null;
suggestions: QuerySuggestion[];
indexPatterns: IIndexPattern[];
queryEditorRect: DOMRect | undefined;
}

const KEY_CODES = {
Expand All @@ -82,7 +82,6 @@ export default class QueryEditorUI extends Component<Props, State> {
index: null,
suggestions: [],
indexPatterns: [],
queryEditorRect: undefined,
};

public inputRef: HTMLElement | null = null;
Expand All @@ -91,7 +90,9 @@ export default class QueryEditorUI extends Component<Props, State> {
private abortController?: AbortController;
private services = this.props.opensearchDashboards.services;
private componentIsUnmounting = false;
private queryEditorDivRefInstance: RefObject<HTMLDivElement> = createRef();
private headerRef: RefObject<HTMLDivElement> = createRef();
private bannerRef: RefObject<HTMLDivElement> = createRef();
private extensionMap = this.props.settings?.getQueryEditorExtensionMap();

private getQueryString = () => {
if (!this.props.query.query) {
Expand Down Expand Up @@ -120,6 +121,30 @@ export default class QueryEditorUI extends Component<Props, State> {
});
};

private renderQueryEditorExtensions() {
if (
!(
this.headerRef.current &&
this.bannerRef.current &&
this.props.queryLanguage &&
this.extensionMap &&
Object.keys(this.extensionMap).length > 0
)
) {
return null;
}
return (
<QueryEditorExtensions
language={this.props.queryLanguage}
configMap={this.extensionMap}
componentContainer={this.headerRef.current}
bannerContainer={this.bannerRef.current}
indexPatterns={this.props.indexPatterns}
dataSource={this.props.dataSource}
/>
);
}

private onSubmit = (query: Query, dateRange?: TimeRange) => {
if (this.props.onSubmit) {
if (this.persistedLog) {
Expand Down Expand Up @@ -221,12 +246,6 @@ export default class QueryEditorUI extends Component<Props, State> {
this.initPersistedLog();
// this.fetchIndexPatterns().then(this.updateSuggestions);
this.initDataSourcesVisibility();
this.handleListUpdate();

window.addEventListener('scroll', this.handleListUpdate, {
passive: true, // for better performance as we won't call preventDefault
capture: true, // scroll events don't bubble, they must be captured instead
});
}

public componentDidUpdate(prevProps: Props) {
Expand All @@ -241,17 +260,8 @@ export default class QueryEditorUI extends Component<Props, State> {
public componentWillUnmount() {
if (this.abortController) this.abortController.abort();
this.componentIsUnmounting = true;
window.removeEventListener('scroll', this.handleListUpdate, { capture: true });
}

handleListUpdate = () => {
if (this.componentIsUnmounting) return;

return this.setState({
queryEditorRect: this.queryEditorDivRefInstance.current?.getBoundingClientRect(),
});
};

handleOnFocus = () => {
if (this.props.onChangeQueryEditorFocus) {
this.props.onChangeQueryEditorFocus(true);
Expand All @@ -260,20 +270,12 @@ export default class QueryEditorUI extends Component<Props, State> {

public render() {
const className = classNames(this.props.className);

const queryEditorHeaderClassName = classNames(
'osdQueryEditorHeader',
this.props.queryEditorHeaderClassName
);

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

return (
<div className={className}>
<div ref={this.props.queryEditorBannerRef} className={queryEditorBannerClassName} />
<div ref={this.bannerRef} className={bannerClassName} />
<EuiFlexGroup gutterSize="xs" direction="column">
<EuiFlexItem grow={false}>
<EuiFlexGroup gutterSize="xs" alignItems="center" className={`${className}__wrapper`}>
Expand All @@ -294,7 +296,7 @@ export default class QueryEditorUI extends Component<Props, State> {
</EuiFlexGroup>
</EuiFlexItem>
<EuiFlexItem onClick={this.onClickInput} grow={true}>
<div ref={this.props.queryEditorHeaderRef} className={queryEditorHeaderClassName} />
<div ref={this.headerRef} className={headerClassName} />
<CodeEditor
height={70}
languageId="opensearchql"
Expand All @@ -315,6 +317,7 @@ export default class QueryEditorUI extends Component<Props, State> {
/>
</EuiFlexItem>
</EuiFlexGroup>
{this.renderQueryEditorExtensions()}
</div>
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extensions.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const QueryEditorExtensions: React.FC<QueryEditorExtensionsProps> = React.memo((
const { configMap, componentContainer, bannerContainer, ...dependencies } = props;

const sortedConfigs = useMemo(() => {
if (!configMap || !Object.keys(configMap)) return [];
if (!configMap || Object.keys(configMap).length === 0) return [];
return Object.values(configMap).sort((a, b) => a.order - b.order);
}, [configMap]);

Expand Down
33 changes: 2 additions & 31 deletions src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
} from '../../../../opensearch_dashboards_react/public';
import { UI_SETTINGS } from '../../../common';
import { fromUser, getQueryLog, PersistedLog } from '../../query';
import { QueryEditorExtensions } from './query_editor_extensions';
import { Settings } from '../types';
import { NoDataPopover } from './no_data_popover';
import QueryEditorUI from './query_editor';
Expand Down Expand Up @@ -71,8 +70,6 @@ export interface QueryEditorTopRowProps {
export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
const [isDateRangeInvalid, setIsDateRangeInvalid] = useState(false);
const [isQueryEditorFocused, setIsQueryEditorFocused] = useState(false);
const queryEditorHeaderRef = useRef<HTMLDivElement | null>(null);
const queryEditorBannerRef = useRef<HTMLDivElement | null>(null);

const opensearchDashboards = useOpenSearchDashboards<IDataPluginServices>();
const { uiSettings, storage, appName } = opensearchDashboards.services;
Expand All @@ -85,7 +82,6 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
props.settings &&
props.settings.getQueryEnhancements(queryLanguage)?.searchBar) ||
null;
const queryEditorExtensionMap = props.settings?.getQueryEditorExtensionMap();
const parsedQuery =
!queryUiEnhancement || isValidQuery(props.query)
? props.query!
Expand Down Expand Up @@ -235,6 +231,7 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
<QueryEditor
disableAutoFocus={props.disableAutoFocus}
indexPatterns={props.indexPatterns!}
dataSource={props.dataSource}
prepend={props.prepend}
query={parsedQuery}
containerRef={props.containerRef}
Expand All @@ -247,33 +244,12 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
persistedLog={persistedLog}
className="osdQueryEditor"
dataTestSubj={props.dataTestSubj}
queryEditorHeaderRef={queryEditorHeaderRef}
queryEditorBannerRef={queryEditorBannerRef}
queryLanguage={queryLanguage}
/>
</EuiFlexItem>
);
}

function renderQueryEditorExtensions() {
if (
!shouldRenderQueryEditorExtensions() ||
!queryEditorHeaderRef.current ||
!queryEditorBannerRef.current ||
!queryLanguage
)
return;
return (
<QueryEditorExtensions
language={queryLanguage}
configMap={queryEditorExtensionMap}
componentContainer={queryEditorHeaderRef.current}
bannerContainer={queryEditorBannerRef.current}
indexPatterns={props.indexPatterns}
dataSource={props.dataSource}
/>
);
}

function renderSharingMetaFields() {
const { from, to } = getDateRange();
const dateRangePretty = prettyDuration(
Expand Down Expand Up @@ -305,10 +281,6 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
);
}

function shouldRenderQueryEditorExtensions(): boolean {
return Boolean(queryEditorExtensionMap && Object.keys(queryEditorExtensionMap).length);
}

function renderUpdateButton() {
const button = props.customSubmitButton ? (
React.cloneElement(props.customSubmitButton, { onClick: onClickSubmitButton })
Expand Down Expand Up @@ -401,7 +373,6 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) {
direction="column"
justifyContent="flexEnd"
>
{renderQueryEditorExtensions()}
{renderQueryEditor()}
<EuiFlexItem>
<EuiFlexGroup responsive={false} gutterSize="none">
Expand Down

0 comments on commit b85e177

Please sign in to comment.