Skip to content

Commit

Permalink
[Detections][Alerts] fixes saved_query rule validation issue (#142602)
Browse files Browse the repository at this point in the history
## Summary

- addresses #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
<img width="1995" alt="Screenshot 2022-10-05 at 09 39 09" src="https://user-images.githubusercontent.com/92328789/194018652-5612a032-e908-4e01-9858-2049c8e453e2.png">

#### After
Only one request sent


### Checklist
<img width="1996" alt="Screenshot 2022-10-05 at 09 37 12" src="https://user-images.githubusercontent.com/92328789/194018637-770a08df-832b-4838-a060-05058c436ed6.png">


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
  • Loading branch information
vitaliidm authored Oct 10, 2022
1 parent 411c261 commit 4feb397
Show file tree
Hide file tree
Showing 10 changed files with 122 additions and 74 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface QueryBarDefineRuleProps {
* called when fetching of saved query fails
*/
onSavedQueryError?: () => void;
defaultSavedQuery?: SavedQuery | undefined;
}

const actionTimelineToHide: ActionTimelineToShow[] = ['duplicate', 'createFrom'];
Expand All @@ -74,6 +75,7 @@ const savedQueryToFieldValue = (savedQuery: SavedQuery): FieldValueQueryBar => (
});

export const QueryBarDefineRule = ({
defaultSavedQuery,
browserFields,
dataTestSubj,
field,
Expand All @@ -91,7 +93,7 @@ export const QueryBarDefineRule = ({
const { value: fieldValue, setValue: setFieldValue } = field as FieldHook<FieldValueQueryBar>;
const [originalHeight, setOriginalHeight] = useState(-1);
const [loadingTimeline, setLoadingTimeline] = useState(false);
const [savedQuery, setSavedQuery] = useState<SavedQuery | undefined>(undefined);
const [savedQuery, setSavedQuery] = useState<SavedQuery | undefined>(defaultSavedQuery);
const [isSavedQueryFailedToLoad, setIsSavedQueryFailedToLoad] = useState(false);
const { isInvalid, errorMessage } = getFieldValidityAndErrorMessage(field);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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)`
Expand Down Expand Up @@ -124,6 +126,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
threatIndicesConfig,
onRuleDataChange,
onPreviewDisabledStateChange,
defaultSavedQuery,
}) => {
const mlCapabilities = useMlCapabilities();
const [openTimelineSearch, setOpenTimelineSearch] = useState(false);
Expand Down Expand Up @@ -615,6 +618,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
onValidityChange: setIsQueryBarValid,
onCloseTimelineSearch: handleCloseTimelineSearch,
onSavedQueryError: handleSavedQueryError,
defaultSavedQuery,
} as QueryBarDefineRuleProps
}
/>
Expand All @@ -629,6 +633,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
openTimelineSearch,
formShouldLoadQueryDynamically,
handleSavedQueryError,
defaultSavedQuery,
]
);
const onOptionsChange = useCallback((field: FieldsEqlOptions, value: string | undefined) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -306,11 +306,9 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
const [filterGroup, setFilterGroup] = useState<Status>(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$<string[]>(DEFAULT_INDEX_KEY);
const [threatIndicesConfig] = useUiSetting$<string[]>(DEFAULT_THREAT_INDEX_KEY);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
);

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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<ReturnType<typeof useAppToastsMock.create>>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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> => undefined;

Expand All @@ -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<RuleStepsFormHooks>({
[RuleStep.defineRule]: formHookNoop,
[RuleStep.aboutRule]: formHookNoop,
Expand Down Expand Up @@ -195,6 +202,17 @@ const EditRulePageComponent: FC = () => {
const [indicesConfig] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);
const [threatIndicesConfig] = useUiSetting$<string[]>(DEFAULT_THREAT_INDEX_KEY);

const defineStepDataWithSavedQuery = useMemo(
() =>
defineStep.data
? {
...defineStep.data,
queryBar: savedQueryBar ?? defineStep.data.queryBar,
}
: defineStep.data,
[defineStep.data, savedQueryBar]
);

const tabs = useMemo(
() => [
{
Expand All @@ -205,19 +223,20 @@ const EditRulePageComponent: FC = () => {
content: (
<>
<EuiSpacer />
<StepPanel loading={loading} title={ruleI18n.DEFINITION}>
{defineStep.data != null && (
<StepPanel loading={loading || isSavedQueryLoading} title={ruleI18n.DEFINITION}>
{defineStepDataWithSavedQuery != null && !isSavedQueryLoading && (
<StepDefineRule
isReadOnlyView={false}
isLoading={isLoading}
isLoading={isLoading || isSavedQueryLoading}
isUpdateView
defaultValues={defineStep.data}
defaultValues={defineStepDataWithSavedQuery}
setForm={setFormHook}
kibanaDataViews={dataViewOptions}
indicesConfig={indicesConfig}
threatIndicesConfig={threatIndicesConfig}
onRuleDataChange={onDataChange}
onPreviewDisabledStateChange={setIsPreviewDisabled}
defaultSavedQuery={savedQuery}
/>
)}
<EuiSpacer />
Expand Down Expand Up @@ -305,6 +324,8 @@ const EditRulePageComponent: FC = () => {
loading,
defineStep.data,
isLoading,
isSavedQueryLoading,
defineStepDataWithSavedQuery,
setFormHook,
dataViewOptions,
indicesConfig,
Expand All @@ -314,6 +335,7 @@ const EditRulePageComponent: FC = () => {
scheduleStep.data,
actionsStep.data,
actionMessageParams,
savedQuery,
]
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
);
Original file line number Diff line number Diff line change
@@ -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<DefineStepRule['queryBar'] | null>(
() =>
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,
};
};

0 comments on commit 4feb397

Please sign in to comment.