From e52a5feb96bbee10ebad11573b60760f818dbc24 Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Mon, 1 Jul 2024 12:35:59 -0700 Subject: [PATCH] Fix object empty check and minor perf issue in query editor extensions (#7077) (#7140) Follow up for #7034, this PR - addresses https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7034#discussion_r1646749538 - addresses https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7034#discussion_r1646750783 - 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 (cherry picked from commit b85e1775c4c1b46f3a04b4e63ab60943fad318a5) Signed-off-by: Joshua Li Signed-off-by: github-actions[bot] Co-authored-by: github-actions[bot] --- changelogs/fragments/7077.yml | 2 + .../public/ui/query_editor/query_editor.tsx | 73 ++++++++++--------- .../query_editor_extensions.tsx | 2 +- .../ui/query_editor/query_editor_top_row.tsx | 33 +-------- 4 files changed, 43 insertions(+), 67 deletions(-) create mode 100644 changelogs/fragments/7077.yml diff --git a/changelogs/fragments/7077.yml b/changelogs/fragments/7077.yml new file mode 100644 index 000000000000..591c9f307d02 --- /dev/null +++ b/changelogs/fragments/7077.yml @@ -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)) \ No newline at end of file diff --git a/src/plugins/data/public/ui/query_editor/query_editor.tsx b/src/plugins/data/public/ui/query_editor/query_editor.tsx index 2433794645ba..0fe6aad3f8b4 100644 --- a/src/plugins/data/public/ui/query_editor/query_editor.tsx +++ b/src/plugins/data/public/ui/query_editor/query_editor.tsx @@ -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, @@ -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; + dataSource?: DataSource; query: Query; containerRef?: React.RefCallback; settings: Settings; @@ -41,10 +43,9 @@ export interface QueryEditorProps { size?: SuggestionsListSize; className?: string; isInvalid?: boolean; - queryEditorHeaderRef: React.RefObject; - queryEditorHeaderClassName?: string; - queryEditorBannerRef: React.RefObject; - queryEditorBannerClassName?: string; + queryLanguage?: string; + headerClassName?: string; + bannerClassName?: string; } interface Props extends QueryEditorProps { @@ -57,7 +58,6 @@ interface State { index: number | null; suggestions: QuerySuggestion[]; indexPatterns: IIndexPattern[]; - queryEditorRect: DOMRect | undefined; } const KEY_CODES = { @@ -82,7 +82,6 @@ export default class QueryEditorUI extends Component { index: null, suggestions: [], indexPatterns: [], - queryEditorRect: undefined, }; public inputRef: HTMLElement | null = null; @@ -91,7 +90,9 @@ export default class QueryEditorUI extends Component { private abortController?: AbortController; private services = this.props.opensearchDashboards.services; private componentIsUnmounting = false; - private queryEditorDivRefInstance: RefObject = createRef(); + private headerRef: RefObject = createRef(); + private bannerRef: RefObject = createRef(); + private extensionMap = this.props.settings?.getQueryEditorExtensionMap(); private getQueryString = () => { if (!this.props.query.query) { @@ -120,6 +121,30 @@ export default class QueryEditorUI extends Component { }); }; + private renderQueryEditorExtensions() { + if ( + !( + this.headerRef.current && + this.bannerRef.current && + this.props.queryLanguage && + this.extensionMap && + Object.keys(this.extensionMap).length > 0 + ) + ) { + return null; + } + return ( + + ); + } + private onSubmit = (query: Query, dateRange?: TimeRange) => { if (this.props.onSubmit) { if (this.persistedLog) { @@ -221,12 +246,6 @@ export default class QueryEditorUI extends Component { 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) { @@ -241,17 +260,8 @@ export default class QueryEditorUI extends Component { 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); @@ -260,20 +270,12 @@ export default class QueryEditorUI extends Component { 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 (
-
+
@@ -294,7 +296,7 @@ export default class QueryEditorUI extends Component { -
+
{ /> + {this.renderQueryEditorExtensions()}
); } diff --git a/src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extensions.tsx b/src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extensions.tsx index 6b2d5011216c..45b0d076877e 100644 --- a/src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extensions.tsx +++ b/src/plugins/data/public/ui/query_editor/query_editor_extensions/query_editor_extensions.tsx @@ -20,7 +20,7 @@ const QueryEditorExtensions: React.FC = 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]); diff --git a/src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx b/src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx index a41e57eaa93d..be75c44ece60 100644 --- a/src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx +++ b/src/plugins/data/public/ui/query_editor/query_editor_top_row.tsx @@ -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'; @@ -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(null); - const queryEditorBannerRef = useRef(null); const opensearchDashboards = useOpenSearchDashboards(); const { uiSettings, storage, appName } = opensearchDashboards.services; @@ -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! @@ -235,6 +231,7 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) { ); } - function renderQueryEditorExtensions() { - if ( - !shouldRenderQueryEditorExtensions() || - !queryEditorHeaderRef.current || - !queryEditorBannerRef.current || - !queryLanguage - ) - return; - return ( - - ); - } - function renderSharingMetaFields() { const { from, to } = getDateRange(); const dateRangePretty = prettyDuration( @@ -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 }) @@ -401,7 +373,6 @@ export default function QueryEditorTopRow(props: QueryEditorTopRowProps) { direction="column" justifyContent="flexEnd" > - {renderQueryEditorExtensions()} {renderQueryEditor()}