[Security Solution] Remove extra data structures in create/edit rule forms #157749

marshallmain · 2023-05-15T15:16:12Z

Summary

This PR refactors the forms used by the Detection Engine create and edit rule pages. The existing forms have a lot of complexity in how data is managed: each of the 4 steps (Define, About, Schedule, and Actions) map to a separate component, and these individual components own the form for that step. However, some steps have dependencies on data in other steps. In addition, Rule Preview and submitting the rule to the API require the data from all steps. To resolve this, I moved the form ownership from the individual step components to the Create/Edit Rule page components and passed the appropriate form into each individual step component.

With the forms owned at the Page level, many of the additional data structures we previously needed to pass form data around are no longer necessary.

Removed

formHooks, setFormHooks - These hooks were used to let step components pass the form's validate function back up to the Page component. With the forms created in the Page component, the validate functions are available without additional structures.
stepsData, setStepsData - This data structure held the form data from each step and the validation status. We now use the live form data through useFormData without additional sidecar data structures.
defineRuleData/aboutRuleData/scheduleRuleData - Sidecar data structures for rule preview, also held the form data from each step. We now use the live form data through useFormData without additional sidecar data structures.

Simplified

editStep() no longer relies on formHooks
submitStep() no longer relies on formHooks
Step forms no longer need watch and onChange configurations

Key Implementation Notes

In order for useFormData to be reliable, the forms for all steps must stay in the DOM at all times while on the Create/Edit pages. Thus the "read only" view has been separated from the editable view, and the visibility of each view is toggled by hiding elements with CSS rather than removing them from the DOM.

If the form were to be removed from the DOM when switching to a read only view, the values from useFormData would reset to the default values so we would need a sidecar object to hold the form values when the form is not rendered. It would also require re-loading the values from the sidecar into the form when the form gets re-added to the DOM. Keeping the form in the DOM at all times allows us to rely on useFormData consistently, which simplifies the data model.

Bug Fixes

When moving back to step 1 from step 2 by clicking "Edit", data is no longer lost in step 2

…factor-success

…-ref HEAD~1..HEAD --fix'

elasticmachine · 2023-05-23T14:23:54Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2023-05-23T14:23:55Z

Pinging @elastic/security-solution (Team: SecuritySolution)

maximpn

@marshallmain Thank you for the long awaited refactoring 🙏 It made the rule's form much cleaner and it's a great step towards simplicity.

Tested locally and it seems working fine though I haven't performed complex manual testing scenarios.

It's clear that some code is just coming through but there is no hard to improve it a bit. As it doesn't look critical I approve the PR and it's up to you to react on the nit comments.

x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/form.tsx

...ins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/index.tsx

maximpn · 2023-05-30T10:11:43Z

...ins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/index.tsx

-                          />
+                          <div
+                            style={{
+                              display: activeStep === RuleStep.defineRule ? undefined : 'none',


I'm curious if there is already an utility component or something exposed by EUI (I wasn't able to find it in the docs) to improve readability. Something like

<DisplayOnCondition visible={activeStep === RuleStep.defineRule}> ... </DisplayOnCondition>

or it could be a component specific for this functionality only accepting a step name.

...ins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/index.tsx

maximpn · 2023-05-30T10:25:21Z

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/index.tsx

+    </StepContentWrapper>
+  );
+};
+export const StepAboutRuleReadOnly = memo(StepAboutRuleReadOnlyComponent);


I see memo() utility is used for the other steps as well though it's unclear if it does a real performance improvement it's here just "in case".

++ I think only a shallow comparison is being made, so it the props may nullify the memo each time.

Yeah I haven't checked to see if we're getting a benefit here or not - the forms lib doesn't seem great about providing the same object on each render so defaultValues might be changing a lot and taking away the memo benefits. But, the "Read only" view was memoized before when it was combined with the edit component, so I just memoized it here to keep the behavior as similar as possible.

I have another branch where I was working on improving the memoization around the create rules page, but I wasn't seeing a noticeable difference in user experience so I shelved it to work on this refactor instead. But, it could be worth picking it up again next.

…-ref HEAD~1..HEAD --fix'

…-fix'

yctercero · 2023-06-01T18:53:30Z

x-pack/plugins/security_solution/cypress/e2e/detection_rules/custom_saved_query_rule.cy.ts

-      it('Shows validation error on rule edit when saved query can not be loaded', function () {
+      // TODO: this error depended on the schema validation running. Can we show the error
+      // based on the saved query failing to load instead of relying on the schema validation?
+      it.skip('Shows validation error on rule edit when saved query can not be loaded', function () {


Can you create an issue to record this as being skipped if we're unable to skip it before merging?

Issue: #159060

yctercero · 2023-06-01T19:08:00Z

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/index.tsx

+    </StepContentWrapper>
+  );
+};
+export const StepAboutRuleReadOnly = memo(StepAboutRuleReadOnlyComponent);


++ I think only a shallow comparison is being made, so it the props may nullify the memo each time.

yctercero

This is a much awaited refactor! LGTM! I pulled down to test creating and editing rules. I tried to fill out pretty much every field to test it out. All seemed good. There's some existing validation bugs with the actions section but unrelated to any of these changes.

kibana-ci · 2023-06-05T07:55:15Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 84c8f85

Failed CI Steps

FTR Configs #31

Test Failures

[job] [logs] FTR Configs #31 / ObservabilityApp Observability alerts / Add to case > When user has read permissions for cases "after all" hook for "does not render case options in the overflow menu"
[job] [logs] FTR Configs #31 / ObservabilityApp Observability alerts / Add to case > When user has read permissions for cases "before all" hook for "does not render case options in the overflow menu"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4103	4104	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	10.7MB	10.7MB	-306.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	414	417	+3
total			+5

References to deprecated APIs

id	before	after	diff
`securitySolution`	606	603	-3

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	498	501	+3
total			+5

History

💔 Build #132263 failed 9271a79
💚 Build #131032 succeeded 8f0e7cb
💛 Build #129441 was flaky 3aabd4f
💔 Build #129430 failed ddf1e99
💔 Build #129112 failed ff438b0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

**Addresses:** #159349 ## Summary This PR fixes and re-enables rule snoozing Cypress test `Rule editing page / actions tab - adds an action to a snoozed rule`. ## Details Basically the test wasn't flaky it just failed on the `main` branch and was skipped. The cause lays in interlacing [Rule snooze tests PR](#158195) with [Rule Editing page refactoring PR](#157749). Two PRs were merged independently to the `main` branch (while the tests PR was merged the last) so combining them lead to a test failure while each one separately didn't cause any problems. ### The root cause The root cause is in a combination of factors. [useForm](https://github.com/elastic/kibana/blob/main/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts#L33) hook has a flaw it can't update its state when new `defaultValue` comes in. The same issue also in [useField](https://github.com/elastic/kibana/blob/main/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts#L77) hook. Rule Editing page fetched a fresh rule's data every time it's rendered via [useRule](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_editing/index.tsx#L581). As `useRule` hook is based on `React Query` it returns stale data during the first render while firing an HTTP request for the fresh data. Obviously the problem happens if the stale data is passed to `useForm` hook as `defaultValue` and when fresh data is fetched and passed again to `useForm` hook the latter just ignores it. Staying longer on the Rule Details page helps to avoid the problem as fetched rule's data is cached and returned as stale data on the Rule Editing page after transition. As stale and fresh data are the same the test would pass. Anyway this behaviour can be reproduced in prod with a slow internet connection. ### What was done to fix the problem? Functionality has been added to update the cached rule's data (React Query cache) upon mutation successful update rule mutation. The mutation if fired by pressing "Save changes" button on the Rule Editing page. It is possible as update rule endpoint returns an updated rule in its response. Along the way it turned out update rule endpoint's and read rule endpoint's responses weren't properly typed so it lead to types mismatch. To fix it `updateRule`'s and `UseMutationOptions`'s return types were changed to `Rule` instead of `RuleResponse`. ### Misc Along the way it turned out `cy.get(RULE_INDICES).should('be.visible');` isn't required anymore to access rule actions form.

…163050) ## Summary Original ticket: #159060 This PR un-skips test which was disabled after the Rule Editing page [refactoring](#157749). There we stopped fields validation on page loading. To be able to show the "failed to load saved query" error on page loading we force the field validation when we failed to load a saved query.

marshallmain and others added 14 commits May 15, 2023 07:34

Remove extra data structures in create/edit rule forms

5d9925c

Fix unit tests, include EqlOptions in defineStepData

3caa468

Fix actions step test

010b44b

Merge branch 'main' of github.com:elastic/kibana into create-rules-re…

f90621f

…factor-success

Fix cypress tests, hoist index patterns into hook

0528df2

Fix stepDefineRule unit tests

69a8108

Fix IM cypress tests and saved query checkbox label

e1cb0c5

Fix new terms and threshold cypress tests

ff438b0

Fix rule actions cypress test

ddf1e99

Merge branch 'main' into create-rules-refactor-success

c938da3

Hopefully fix response actions cypress test

672142e

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

1e8db8a

…-ref HEAD~1..HEAD --fix'

Fix response actions again maybe

459fa08

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

3aabd4f

…-ref HEAD~1..HEAD --fix'

marshallmain marked this pull request as ready for review May 23, 2023 14:23

marshallmain requested review from a team as code owners May 23, 2023 14:23

marshallmain requested a review from banderror May 23, 2023 14:23

marshallmain added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels May 23, 2023

marshallmain added release_note:skip Skip the PR/issue when compiling release notes v8.9.0 labels May 23, 2023

banderror requested review from maximpn and removed request for banderror May 29, 2023 12:11

maximpn approved these changes May 30, 2023

View reviewed changes

marshallmain and others added 6 commits May 30, 2023 07:31

PR feedback: loading var naming, combine if statements

3820ed8

Merge branch 'main' into create-rules-refactor-success

9306c98

Remove extra spacing

40ff7e7

PR feedback: aboutRuleSchema for threat match

491f88f

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

57b4d40

…-ref HEAD~1..HEAD --fix'

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

8f0e7cb

…-fix'

yctercero reviewed Jun 1, 2023

View reviewed changes

yctercero approved these changes Jun 1, 2023

View reviewed changes

marshallmain and others added 3 commits June 2, 2023 11:09

Remove unused helpers and types

b9a3b17

Merge branch 'main' into create-rules-refactor-success

9271a79

Merge branch 'main' into create-rules-refactor-success

84c8f85

marshallmain merged commit 9abed02 into elastic:main Jun 5, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 5, 2023

marshallmain mentioned this pull request Jun 5, 2023

[Security Solution][Skipped Test] Saved query validation error test #159060

Closed

e40pud mentioned this pull request Jun 14, 2023

[Security Solution] Rule is created when the conditional logic "If alert matches a query" is left blank #159690

Merged

e40pud mentioned this pull request Aug 3, 2023

[Security Solution][Skipped Test] Saved query validation error test #163050

Merged

maximpn mentioned this pull request Aug 3, 2023

[Security Solution] Re-enable fixed rule snoozing Cypress test #160037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Remove extra data structures in create/edit rule forms #157749

[Security Solution] Remove extra data structures in create/edit rule forms #157749

marshallmain commented May 15, 2023 •

edited

Loading

elasticmachine commented May 23, 2023

elasticmachine commented May 23, 2023

maximpn left a comment

maximpn May 30, 2023

maximpn May 30, 2023

yctercero Jun 1, 2023

marshallmain Jun 2, 2023

yctercero Jun 1, 2023

marshallmain Jun 5, 2023

yctercero Jun 1, 2023

yctercero left a comment

kibana-ci commented Jun 5, 2023

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

[Security Solution] Remove extra data structures in create/edit rule forms #157749

[Security Solution] Remove extra data structures in create/edit rule forms #157749

Conversation

marshallmain commented May 15, 2023 • edited Loading

Summary

Removed

Simplified

Key Implementation Notes

Bug Fixes

elasticmachine commented May 23, 2023

elasticmachine commented May 23, 2023

maximpn left a comment

Choose a reason for hiding this comment

maximpn May 30, 2023

Choose a reason for hiding this comment

maximpn May 30, 2023

Choose a reason for hiding this comment

yctercero Jun 1, 2023

Choose a reason for hiding this comment

marshallmain Jun 2, 2023

Choose a reason for hiding this comment

yctercero Jun 1, 2023

Choose a reason for hiding this comment

marshallmain Jun 5, 2023

Choose a reason for hiding this comment

yctercero Jun 1, 2023

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

kibana-ci commented Jun 5, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

marshallmain commented May 15, 2023 •

edited

Loading