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.

Sign up for GitHub

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

[Security Sollution][Alerts] fixes rule preview issue for new terms field #145707

Merged
merged 8 commits into from
Nov 28, 2022
Merged

[Security Sollution][Alerts] fixes rule preview issue for new terms field #145707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5b2a6d4
[Security Sollution][Alerts] fixes rule preview issue for new terms f…
vitaliidm Nov 18, 2022
79269dd
Merge branch 'main' into alerts/preview-new-terms-issue
vitaliidm Nov 18, 2022
712635b
use validate only
vitaliidm Nov 18, 2022
d900adf
Merge branch 'alerts/preview-new-terms-issue' of https://github.com/v…
vitaliidm Nov 18, 2022
c26ce90
wrapping in setTimeout
vitaliidm Nov 21, 2022
790bf9e
Merge branch 'main' into alerts/preview-new-terms-issue
vitaliidm Nov 21, 2022
be05935
Merge branch 'main' into alerts/preview-new-terms-issue
vitaliidm Nov 24, 2022
c7c2a59
Merge branch 'main' into alerts/preview-new-terms-issue
vitaliidm Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type { ChartSeriesConfigs } from '../../../../common/components/charts/co
import type { FieldValueQueryBar } from '../query_bar';
import type { TimeframePreviewOptions } from '../../../pages/detection_engine/rules/types';
import { DataSourceType } from '../../../pages/detection_engine/rules/types';
import { MAX_NUMBER_OF_NEW_TERMS_FIELDS } from '../../../../../common/constants';

/**
* Determines whether or not to display noise warning.
Expand Down Expand Up @@ -108,6 +109,10 @@ export const getHistogramConfig = (
};
};

const isNewTermsPreviewDisabled = (newTermsFields: string[]): boolean => {
return newTermsFields.length === 0 || newTermsFields.length > MAX_NUMBER_OF_NEW_TERMS_FIELDS;
};

export const getIsRulePreviewDisabled = ({
ruleType,
isQueryBarValid,
Expand Down Expand Up @@ -157,7 +162,7 @@ export const getIsRulePreviewDisabled = ({
return isEmpty(queryBar.query.query) && isEmpty(queryBar.filters);
}
if (ruleType === 'new_terms') {
return newTermsFields.length === 0;
return isNewTermsPreviewDisabled(newTermsFields);
}
return false;
};
32 changes: 17 additions & 15 deletions ...k/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
schema,
});

const { getFields, getFormData, reset, submit } = form;
const { getFields, getFormData, reset, validate } = form;
const [formData] = useFormData<DefineStepRule>({
form,
watch: [
Expand Down Expand Up @@ -392,21 +392,23 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
}, [onSubmit]);

const getData = useCallback(async () => {
const result = await submit();
result.data = {
...result.data,
eqlOptions: optionsSelected,
// validate doesn't return actual state of form
// more details here: https://github.com/elastic/kibana/issues/144322#issuecomment-1321838136
// wrapping in setTimeout is a workaround until solution within forms-lib can be found
const isValid = await new Promise<boolean>((resolve) => {
setTimeout(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The fix looks dirty but fine if it's temporary.

I just wanted to share my thoughts. Usually a necessity of setTimeout() usage says about design flaws and over-complication. In this case it looks possible to get rid of the problem by rethinking the approach to the data handling. For example I can see formHooks is used as a global variable and the value is reused the level up.
The cause of the problem is double validation which actually shouldn't happen.

I'm curious if an approach to redesign the components to handle validation in another way was considered as well.

Copy link
Contributor Author

@vitaliidm vitaliidm Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The fix looks dirty but fine if it's temporary.

I would recommend to read this section before writing comments like this.

I just wanted to share my thoughts. Usually a necessity of setTimeout() usage says about design flaws and over-complication. In this case it looks possible to get rid of the problem by rethinking the approach to the data handling. For example I can see formHooks is used as a global variable and the value is reused the level up.
#144322 (comment) is double validation which actually shouldn't happen.

What do you mean by 'double validation'?
I would expect once data in form is changed and available in onChange callback, it can be checked whether it's valid or not. But if you look into standalone example, it's not a case: data is changed, but its validity can not be established.
I would like to see forms-lib assessment of library behaviour before committing to any refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation and pointing to the guidelines.

I didn't mean to offend but rather say that setTimeout usually is a red flag which signals there is something wrong.

You are right the problem in the the forms-lib. Though it's strange the popular scenario of accessing the validity state in the onChange callback wasn't covered already.

Copy link
Contributor Author

@vitaliidm vitaliidm Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

rather say that setTimeout usually is a red flag which signals there is something wrong.

yes, I also commented that it's a workaround and put together ticket and links to it

// wrapping in setTimeout is a workaround until solution within forms-lib can be found

You are right, the problem is in the forms-lib. Though it's strange the popular scenario of accessing the validity state in the onChange callback wasn't covered already.

In this standalone example , it can be seen that in onChange, accessed validity is not correct. Very simple case, but the result is rather unexpected. Let's see what forms-lib will say on that.
I would expect if I get any data from callback: to either access validation status straight away or through running validate method. We did the latter in form(though through calling a submit before this change) and relied on its results. Which, as it turns out, are not correct in some cases.

const valid = await validate();
resolve(valid);
}, 0);
});
return {
isValid,
data: {
...getFormData(),
eqlOptions: optionsSelected,
},
};
return result.isValid
? result
: {
isValid: false,
data: {
...getFormData(),
eqlOptions: optionsSelected,
},
};
}, [getFormData, optionsSelected, submit]);
}, [getFormData, optionsSelected, validate]);

useEffect(() => {
let didCancel = false;
Expand Down