From 4feb397bae32e2d045339d31dcd6a6cabf6fd69a Mon Sep 17 00:00:00 2001 From: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com> Date: Mon, 10 Oct 2022 18:42:24 +0100 Subject: [PATCH] [Detections][Alerts] fixes saved_query rule validation issue (#142602) ## Summary - addresses https://github.com/elastic/kibana/issues/141758 - loads saved query on rules edit page before form rendering, thus prevents validation run on uncompleted data(saved query wasn't fetched before QueryBar component renders). Then fetched saved query passed to QueryBarDefineRule component ### Before https://user-images.githubusercontent.com/92328789/193884969-3647e06b-085a-4923-82e8-546bedabeb3e.mp4 ### After https://user-images.githubusercontent.com/92328789/193884323-629fb879-3a51-4412-8b63-2f36afc618cb.mov As side effect, it reduced number of requests for saved query: #### Before 2 requests were sent Screenshot 2022-10-05 at 09 39 09 #### After Only one request sent ### Checklist Screenshot 2022-10-05 at 09 37 12 Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --- .../common/hooks/use_fetch/request_names.ts | 1 - .../components/rules/query_bar/index.tsx | 4 +- .../rules/step_define_rule/index.tsx | 5 ++ .../detection_engine/rules/details/index.tsx | 10 +-- .../rules/details/translations.ts | 7 -- .../rules/details/use_get_saved_query.ts | 55 -------------- .../rules/edit/index.test.tsx | 4 + .../detection_engine/rules/edit/index.tsx | 30 +++++++- .../detection_engine/rules/translations.ts | 7 ++ .../rules/use_get_saved_query.ts | 73 +++++++++++++++++++ 10 files changed, 122 insertions(+), 74 deletions(-) delete mode 100644 x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/use_get_saved_query.ts create mode 100644 x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/use_get_saved_query.ts diff --git a/x-pack/plugins/security_solution/public/common/hooks/use_fetch/request_names.ts b/x-pack/plugins/security_solution/public/common/hooks/use_fetch/request_names.ts index 2d6c414090367..326b1ce69ceaa 100644 --- a/x-pack/plugins/security_solution/public/common/hooks/use_fetch/request_names.ts +++ b/x-pack/plugins/security_solution/public/common/hooks/use_fetch/request_names.ts @@ -10,7 +10,6 @@ export const REQUEST_NAMES = { SECURITY_DASHBOARDS: `${APP_UI_ID} fetch security dashboards`, ANOMALIES_TABLE: `${APP_UI_ID} fetch anomalies table data`, GET_RISK_SCORE_DEPRECATED: `${APP_UI_ID} fetch is risk score deprecated`, - GET_SAVED_QUERY: `${APP_UI_ID} fetch saved query`, ENABLE_RISK_SCORE: `${APP_UI_ID} fetch enable risk score`, REFRESH_RISK_SCORE: `${APP_UI_ID} fetch refresh risk score`, UPGRADE_RISK_SCORE: `${APP_UI_ID} fetch upgrade risk score`, diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx index 7d7c9534c1688..ec198514f7e82 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx @@ -53,6 +53,7 @@ export interface QueryBarDefineRuleProps { * called when fetching of saved query fails */ onSavedQueryError?: () => void; + defaultSavedQuery?: SavedQuery | undefined; } const actionTimelineToHide: ActionTimelineToShow[] = ['duplicate', 'createFrom']; @@ -74,6 +75,7 @@ const savedQueryToFieldValue = (savedQuery: SavedQuery): FieldValueQueryBar => ( }); export const QueryBarDefineRule = ({ + defaultSavedQuery, browserFields, dataTestSubj, field, @@ -91,7 +93,7 @@ export const QueryBarDefineRule = ({ const { value: fieldValue, setValue: setFieldValue } = field as FieldHook; const [originalHeight, setOriginalHeight] = useState(-1); const [loadingTimeline, setLoadingTimeline] = useState(false); - const [savedQuery, setSavedQuery] = useState(undefined); + const [savedQuery, setSavedQuery] = useState(defaultSavedQuery); const [isSavedQueryFailedToLoad, setIsSavedQueryFailedToLoad] = useState(false); const { isInvalid, errorMessage } = getFieldValidityAndErrorMessage(field); diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx index 21bf8125540b4..1dd2a2d1de798 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx @@ -24,6 +24,7 @@ import { isEqual, isEmpty, omit } from 'lodash'; import type { FieldSpec } from '@kbn/data-views-plugin/common'; import usePrevious from 'react-use/lib/usePrevious'; +import type { SavedQuery } from '@kbn/data-plugin/public'; import type { DataViewBase } from '@kbn/es-query'; import { FormattedMessage } from '@kbn/i18n-react'; import { isMlRule } from '../../../../../common/machine_learning/helpers'; @@ -88,6 +89,7 @@ interface StepDefineRuleProps extends RuleStepProps { defaultValues: DefineStepRule; onRuleDataChange?: (data: DefineStepRule) => void; onPreviewDisabledStateChange?: (isDisabled: boolean) => void; + defaultSavedQuery?: SavedQuery; } export const MyLabelButton = styled(EuiButtonEmpty)` @@ -124,6 +126,7 @@ const StepDefineRuleComponent: FC = ({ threatIndicesConfig, onRuleDataChange, onPreviewDisabledStateChange, + defaultSavedQuery, }) => { const mlCapabilities = useMlCapabilities(); const [openTimelineSearch, setOpenTimelineSearch] = useState(false); @@ -615,6 +618,7 @@ const StepDefineRuleComponent: FC = ({ onValidityChange: setIsQueryBarValid, onCloseTimelineSearch: handleCloseTimelineSearch, onSavedQueryError: handleSavedQueryError, + defaultSavedQuery, } as QueryBarDefineRuleProps } /> @@ -629,6 +633,7 @@ const StepDefineRuleComponent: FC = ({ openTimelineSearch, formShouldLoadQueryDynamically, handleSavedQueryError, + defaultSavedQuery, ] ); const onOptionsChange = useCallback((field: FieldsEqlOptions, value: string | undefined) => { diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx index 53d65e15a468e..f962cd806cc07 100644 --- a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx +++ b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx @@ -116,7 +116,7 @@ import { ExecutionLogTable } from './execution_log_table/execution_log_table'; import * as detectionI18n from '../../translations'; import * as ruleI18n from '../translations'; import { RuleDetailsContextProvider } from './rule_details_context'; -import { useGetSavedQuery } from './use_get_saved_query'; +import { useGetSavedQuery } from '../use_get_saved_query'; import * as i18n from './translations'; import { NeedAdminForUpdateRulesCallOut } from '../../../../components/callouts/need_admin_for_update_callout'; import { MissingPrivilegesCallOut } from '../../../../components/callouts/missing_privileges_callout'; @@ -306,11 +306,9 @@ const RuleDetailsPageComponent: React.FC = ({ const [filterGroup, setFilterGroup] = useState(FILTER_OPEN); const [dataViewOptions, setDataViewOptions] = useState<{ [x: string]: DataViewListItem }>({}); - // load saved query only if rule type === 'saved_query', as other rule types still can have saved_id property that is not used - // Rule schema allows to save any rule with saved_id property, but it only used for saved_query rule type - // In future we might look in possibility to restrict rule schema (breaking change!) and remove saved_id from the rest of rules through migration - const savedQueryId = rule?.type === 'saved_query' ? rule?.saved_id : undefined; - const { isSavedQueryLoading, savedQueryBar } = useGetSavedQuery(savedQueryId); + const { isSavedQueryLoading, savedQueryBar } = useGetSavedQuery(rule?.saved_id, { + ruleType: rule?.type, + }); const [indicesConfig] = useUiSetting$(DEFAULT_INDEX_KEY); const [threatIndicesConfig] = useUiSetting$(DEFAULT_THREAT_INDEX_KEY); diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/translations.ts b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/translations.ts index b1c7e0033f5f4..1a50d570ea179 100644 --- a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/translations.ts +++ b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/translations.ts @@ -69,10 +69,3 @@ export const DELETED_RULE = i18n.translate( defaultMessage: 'Deleted rule', } ); - -export const SAVED_QUERY_LOAD_ERROR_TOAST = i18n.translate( - 'xpack.securitySolution.hooks.useGetSavedQuery.errorToastMessage', - { - defaultMessage: 'Failed to load the saved query', - } -); diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/use_get_saved_query.ts b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/use_get_saved_query.ts deleted file mode 100644 index 4c3c4c6c42fe0..0000000000000 --- a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/use_get_saved_query.ts +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -import { useEffect, useMemo } from 'react'; - -import { useSavedQueryServices } from '../../../../../common/utils/saved_query_services'; -import type { DefineStepRule } from '../types'; - -import { useFetch, REQUEST_NAMES } from '../../../../../common/hooks/use_fetch'; -import { useAppToasts } from '../../../../../common/hooks/use_app_toasts'; - -import { SAVED_QUERY_LOAD_ERROR_TOAST } from './translations'; - -export const useGetSavedQuery = (savedQueryId: string | undefined) => { - const savedQueryServices = useSavedQueryServices(); - const { addError } = useAppToasts(); - const { fetch, data, isLoading, error } = useFetch( - REQUEST_NAMES.GET_SAVED_QUERY, - savedQueryServices.getSavedQuery - ); - - useEffect(() => { - if (savedQueryId) { - fetch(savedQueryId); - } - }, [savedQueryId, fetch]); - - useEffect(() => { - if (error) { - addError(error, { title: SAVED_QUERY_LOAD_ERROR_TOAST }); - } - }, [error, addError]); - - const savedQueryBar = useMemo( - () => - data - ? { - saved_id: data.id, - filters: data.attributes.filters ?? [], - query: data.attributes.query, - title: data.attributes.title, - } - : null, - [data] - ); - - return { - isSavedQueryLoading: isLoading, - savedQueryBar, - }; -}; diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.test.tsx b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.test.tsx index c990621acd47b..d283879045165 100644 --- a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.test.tsx +++ b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.test.tsx @@ -31,6 +31,10 @@ jest.mock('react-router-dom', () => { }; }); jest.mock('../../../../../common/hooks/use_app_toasts'); +jest.mock('../use_get_saved_query', () => ({ + __esModule: true, + useGetSavedQuery: jest.fn().mockReturnValue({}), +})); describe('EditRulePage', () => { let appToastsMock: jest.Mocked>; diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.tsx b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.tsx index 7b133d1cb15a3..23d96c93aea2a 100644 --- a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.tsx +++ b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/edit/index.tsx @@ -18,6 +18,7 @@ import { FormattedMessage } from '@kbn/i18n-react'; import type { FC } from 'react'; import React, { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useParams } from 'react-router-dom'; +import { noop } from 'lodash'; import type { DataViewListItem } from '@kbn/data-views-plugin/common'; import type { UpdateRulesSchema } from '../../../../../../common/detection_engine/schemas/request'; @@ -75,6 +76,7 @@ import { HeaderPage } from '../../../../../common/components/header_page'; import { useStartTransaction } from '../../../../../common/lib/apm/use_start_transaction'; import { SINGLE_RULE_ACTIONS } from '../../../../../common/lib/apm/user_actions'; import { PreviewFlyout } from '../preview'; +import { useGetSavedQuery } from '../use_get_saved_query'; const formHookNoop = async (): Promise => undefined; @@ -98,6 +100,11 @@ const EditRulePageComponent: FC = () => { const [ruleLoading, rule] = useRule(ruleId); const loading = ruleLoading || userInfoLoading || listsConfigLoading; + const { isSavedQueryLoading, savedQueryBar, savedQuery } = useGetSavedQuery(rule?.saved_id, { + ruleType: rule?.type, + onError: noop, + }); + const formHooks = useRef({ [RuleStep.defineRule]: formHookNoop, [RuleStep.aboutRule]: formHookNoop, @@ -195,6 +202,17 @@ const EditRulePageComponent: FC = () => { const [indicesConfig] = useUiSetting$(DEFAULT_INDEX_KEY); const [threatIndicesConfig] = useUiSetting$(DEFAULT_THREAT_INDEX_KEY); + const defineStepDataWithSavedQuery = useMemo( + () => + defineStep.data + ? { + ...defineStep.data, + queryBar: savedQueryBar ?? defineStep.data.queryBar, + } + : defineStep.data, + [defineStep.data, savedQueryBar] + ); + const tabs = useMemo( () => [ { @@ -205,19 +223,20 @@ const EditRulePageComponent: FC = () => { content: ( <> - - {defineStep.data != null && ( + + {defineStepDataWithSavedQuery != null && !isSavedQueryLoading && ( )} @@ -305,6 +324,8 @@ const EditRulePageComponent: FC = () => { loading, defineStep.data, isLoading, + isSavedQueryLoading, + defineStepDataWithSavedQuery, setFormHook, dataViewOptions, indicesConfig, @@ -314,6 +335,7 @@ const EditRulePageComponent: FC = () => { scheduleStep.data, actionsStep.data, actionMessageParams, + savedQuery, ] ); diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts index f49e4695731bc..8b32821bc71f1 100644 --- a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts +++ b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts @@ -1105,3 +1105,10 @@ export const CANCEL_BUTTON_LABEL = i18n.translate( defaultMessage: 'Cancel', } ); + +export const SAVED_QUERY_LOAD_ERROR_TOAST = i18n.translate( + 'xpack.securitySolution.hooks.useGetSavedQuery.errorToastMessage', + { + defaultMessage: 'Failed to load the saved query', + } +); diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/use_get_saved_query.ts b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/use_get_saved_query.ts new file mode 100644 index 0000000000000..5cd530754d8ce --- /dev/null +++ b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/use_get_saved_query.ts @@ -0,0 +1,73 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { useMemo } from 'react'; +import { useQuery } from '@tanstack/react-query'; + +import type { Type } from '@kbn/securitysolution-io-ts-alerting-types'; + +import { useSavedQueryServices } from '../../../../common/utils/saved_query_services'; +import type { DefineStepRule } from './types'; + +import { useAppToasts } from '../../../../common/hooks/use_app_toasts'; + +import { SAVED_QUERY_LOAD_ERROR_TOAST } from './translations'; + +interface UseGetSavedQuerySettings { + onError?: (e: unknown) => void; + ruleType: Type | undefined; +} + +export const useGetSavedQuery = ( + savedQueryId: string | undefined, + { ruleType, onError }: UseGetSavedQuerySettings +) => { + const savedQueryServices = useSavedQueryServices(); + const { addError } = useAppToasts(); + + const defaultErrorHandler = (e: unknown) => { + addError(e, { title: SAVED_QUERY_LOAD_ERROR_TOAST }); + }; + + const query = useQuery( + ['detectionEngine', 'rule', 'savedQuery', savedQueryId], + async () => { + if (!savedQueryId) { + return null; + } + + return savedQueryServices.getSavedQuery(savedQueryId); + }, + { + onError: onError ?? defaultErrorHandler, + // load saved query only if rule type === 'saved_query', as other rule types still can have saved_id property that is not used + // Rule schema allows to save any rule with saved_id property, but it only used for saved_query rule type + // In future we might look in possibility to restrict rule schema (breaking change!) and remove saved_id from the rest of rules through migration + enabled: Boolean(savedQueryId) && ruleType === 'saved_query', + retry: false, + } + ); + + const savedQueryBar = useMemo( + () => + query.data + ? { + saved_id: query.data.id, + filters: query.data.attributes.filters ?? [], + query: query.data.attributes.query, + title: query.data.attributes.title, + } + : null, + [query.data] + ); + + return { + isSavedQueryLoading: savedQueryId ? query.isLoading : false, + savedQueryBar, + savedQuery: query.data ?? undefined, + }; +};