[Security Solution] [Platform] Fix bug where rule form would remove index field when switching between index pattern + data view #134783

dhurley14 · 2022-06-20T20:09:45Z

Summary

Fixes: #135513

reset index field on rule form to empty array when it presents as null / undefined in the defaultValues prop.

Cypress tests were added as "skip" for now. Will work on them at a later date.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

…l / undefined in the defaultValues prop

dhurley14 · 2022-06-28T13:35:21Z

@elasticmachine merge upstream

rylnd

A few things to clean up, but the attention to detail and the bugfix are mucho appreciated.

rylnd · 2022-06-29T23:14:52Z

x-pack/plugins/security_solution/cypress/integration/detection_rules/custom_query_rule.spec.ts

@@ -200,6 +207,101 @@ describe('Custom query rules', () => {
      cy.get(ALERT_GRID_CELL).contains(this.rule.name);
    });
  });
+  describe.skip('Custom detection rules creation with data view', () => {


Can you add a note about why these were skipped, and delete the commented code?

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

…form-bug

…re adding an exception from the rule details table alerts table would not utilize the data view defined on that rule when rendering available fields to define an exception on.

…form-bug

…-fix'

…form-bug

…bana into fix-dataview-index-form-bug

dhurley14 · 2022-07-07T18:12:56Z

@elasticmachine merge upstream

dhurley14 · 2022-07-08T13:25:15Z

@elasticmachine merge upstream

nkhristinin · 2022-07-08T15:31:32Z

...plugins/security_solution/public/common/components/exceptions/add_exception_flyout/index.tsx

@@ -176,14 +176,29 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({

  useEffect(() => {
    const fetchSingleDataView = async () => {
-      if (dataViewId != null && dataViewId !== '') {
-        const dv = await data.dataViews.get(dataViewId);
+      const notNullDataViewId =


Hey, I have a couple of questions here.

If I look at the type AddExceptionFlyoutProps, I seedataViewId?: string;
meaning that it can be string or undefined, but in the code, you are explicitly checking for null, does there some specific reason for that?

nit: Personally I often found it difficult to read nested ternary expressions, I maybe use for this case something like:
const notNullDataViewId = dataViewId || maybeRule?.data_view_id || null

I agree the ternary isn't the clearest. I'll try to re-work it. Where exactly am I doing and explicit null check? Having trouble finding that. Looks like all the checks are nullish != not !== or were you referencing somewhere else?

Oh, I get it! It was != not !== :)

nkhristinin · 2022-07-08T15:49:35Z

...plugins/security_solution/public/common/components/exceptions/add_exception_flyout/index.tsx

-  }, [data.dataViews, dataViewId, setIndexPattern]);
+  }, [data.dataViews, dataViewId, maybeRule?.data_view_id, setIndexPattern]);
+
+  const getIndexPattern = useCallback(


Also here, do we really need to use getIndexPattern as a function and useCallback here?
I saw that we invoke this function once, and exactly in this component render

Probably is simplier way to write this is -

const selectedIndexPattern= (dataViewId || maybeRule?.data_view_id) ? indexPattern : indexIndexPatterns

yctercero

Pulled down and tested - just wanted to confirm that the following is all desired behavior:

in rule creation:
- switching between data views and index patterns resets the index patterns to the default
in rule edit:
- (rule using data views) switching from data views to index patterns and back, index patterns show up as empty and data views remains as original value
- (rule using index patterns) switching from index pattern to data views, my index pattern is back to the default index pattern

I know the original bug was that the form would blow up switching from the edit tabs and such, so definitely fixed here! Thanks for fighting through the forms code on this one.

x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

nkhristinin

LGTM!

…form-bug

dhurley14 · 2022-07-11T18:22:45Z

@elasticmachine merge upstream

dhurley14 · 2022-07-11T18:24:45Z

Test Failures
[job] [logs] Security Solution Tests #2 / EQL rules Detection rules, EQL Creates and enables a new EQL rule
[job] [logs] Security Solution Tests #2 / Exceptions flyout flyout errors When updating an item for a list that has since been deleted Displays missing exception list error
[job] [logs] Security Solution Tests #2 / Exceptions flyout flyout errors When updating an item for a list that has since been deleted Displays missing exception list error
[job] [logs] Security Solution Tests #2 / Exceptions flyout flyout errors When updating an item with version conflict Displays version conflict error
[job] [logs] Security Solution Tests #2 / Exceptions flyout flyout errors When updating an item with version conflict Displays version conflict error

Looks like some kind of flake with cypress. These tests are passing locally..

kibana-ci · 2022-07-11T19:42:33Z

💛 Build succeeded, but was flaky

Failed CI Steps

Security Solution Tests #2

Test Failures

[job] [logs] Security Solution Tests #2 / Related integrations integrations not installed "before all" hook for "should display a badge with the installed integrations on the rule management page"

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	5.2MB	5.2MB	+149.0B

History

💔 Build #56686 failed 6e9935c
💔 Build #56406 failed e71ce9e
💔 Build #56377 failed 234e9e4
💔 Build #56361 failed 89f48bb
💔 Build #56295 failed a2f498b

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

cc @dhurley14

spong

Code review LGTM! Didn't check out and test as looked like other reviewers already took care of that. Thanks for this fix @dhurley14! 🚀

reset index field on rule form to empty array when it presents as nul…

0b43b21

…l / undefined in the defaultValues prop

dhurley14 self-assigned this Jun 20, 2022

dhurley14 added the ci:deploy-cloud label Jun 20, 2022

dhurley14 added 3 commits June 23, 2022 14:19

adds cypress test

d045c0d

working cypress test

1323ea8

skip new cypress tests for now.

6faf18e

Merge branch 'main' into fix-dataview-index-form-bug

0d86b99

dhurley14 changed the title ~~[WIP] [Security Solution] [Platform] reset index field on rule form to empty array when it presents as nul…~~ [Security Solution] [Platform] Fix bug where rule form would remove index field when switching between index pattern + data view Jun 29, 2022

dhurley14 added v8.4.0 Feature:Detection Rules Security Solution rules and Detection Engine Team:Security Solution Platform Security Solution Platform Team review release_note:skip Skip the PR/issue when compiling release notes labels Jun 29, 2022

dhurley14 marked this pull request as ready for review June 29, 2022 15:05

dhurley14 requested review from a team as code owners June 29, 2022 15:05

rylnd reviewed Jun 29, 2022

View reviewed changes

dhurley14 mentioned this pull request Jun 30, 2022

[Security Solution]Red Banner on switching from data view to index pattern #135513

Closed

dhurley14 added 2 commits June 30, 2022 11:19

Merge remote-tracking branch 'upstream/main' into fix-dataview-index-…

5b5ccfa

…form-bug

sets defaultValue on define rule schema to empty array, fixes bug whe…

7455626

…re adding an exception from the rule details table alerts table would not utilize the data view defined on that rule when rendering available fields to define an exception on.

dhurley14 requested a review from a team as a code owner June 30, 2022 19:13

dhurley14 and others added 6 commits July 5, 2022 10:42

cleanup cypress test

24f0072

Merge remote-tracking branch 'upstream/main' into fix-dataview-index-…

3eb3516

…form-bug

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

5f20ee4

…-fix'

remove unused import, to be used later

66d3692

Merge remote-tracking branch 'upstream/main' into fix-dataview-index-…

156b49e

…form-bug

Merge branch 'fix-dataview-index-form-bug' of github.com:dhurley14/ki…

898dee6

…bana into fix-dataview-index-form-bug

Merge branch 'main' into fix-dataview-index-form-bug

13d27d1

Merge branch 'main' into fix-dataview-index-form-bug

a2f498b

nkhristinin reviewed Jul 8, 2022

View reviewed changes

dhurley14 added 2 commits July 8, 2022 12:31

uncomment

89f48bb

replace ternary and remove useCallback

234e9e4

yctercero approved these changes Jul 8, 2022

View reviewed changes

yctercero reviewed Jul 8, 2022

View reviewed changes

x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts Outdated Show resolved Hide resolved

work on cypress stuff later

e71ce9e

nkhristinin self-requested a review July 8, 2022 18:36

nkhristinin approved these changes Jul 8, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix-dataview-index-…

6e9935c

…form-bug

dhurley14 removed the ci:deploy-cloud label Jul 11, 2022

Merge branch 'main' into fix-dataview-index-form-bug

e99e6d3

dhurley14 enabled auto-merge (squash) July 11, 2022 18:53

spong approved these changes Jul 11, 2022

View reviewed changes

dhurley14 merged commit 807cac0 into elastic:main Jul 11, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jul 11, 2022

dhurley14 mentioned this pull request Jul 14, 2022

[Security Solution] [Platform] Adds state to remember what was in data view or index pattern selection when switching between the two #136448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] [Platform] Fix bug where rule form would remove index field when switching between index pattern + data view #134783

[Security Solution] [Platform] Fix bug where rule form would remove index field when switching between index pattern + data view #134783

dhurley14 commented Jun 20, 2022 •

edited

Loading

dhurley14 commented Jun 28, 2022

rylnd left a comment

rylnd Jun 29, 2022

dhurley14 commented Jul 7, 2022

dhurley14 commented Jul 8, 2022

nkhristinin Jul 8, 2022

dhurley14 Jul 8, 2022

nkhristinin Jul 8, 2022

nkhristinin Jul 8, 2022

yctercero left a comment

nkhristinin left a comment

dhurley14 commented Jul 11, 2022

dhurley14 commented Jul 11, 2022

kibana-ci commented Jul 11, 2022

spong left a comment

[Security Solution] [Platform] Fix bug where rule form would remove index field when switching between index pattern + data view #134783

[Security Solution] [Platform] Fix bug where rule form would remove index field when switching between index pattern + data view #134783

Conversation

dhurley14 commented Jun 20, 2022 • edited Loading

Summary

Checklist

dhurley14 commented Jun 28, 2022

rylnd left a comment

Choose a reason for hiding this comment

rylnd Jun 29, 2022

Choose a reason for hiding this comment

dhurley14 commented Jul 7, 2022

dhurley14 commented Jul 8, 2022

nkhristinin Jul 8, 2022

Choose a reason for hiding this comment

dhurley14 Jul 8, 2022

Choose a reason for hiding this comment

nkhristinin Jul 8, 2022

Choose a reason for hiding this comment

nkhristinin Jul 8, 2022

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

nkhristinin left a comment

Choose a reason for hiding this comment

dhurley14 commented Jul 11, 2022

dhurley14 commented Jul 11, 2022

kibana-ci commented Jul 11, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

spong left a comment

Choose a reason for hiding this comment

dhurley14 commented Jun 20, 2022 •

edited

Loading