[Security Solution] Simplify bulk action execution #144091

maximpn · 2022-10-27T11:21:29Z

Relates to: #142748

Summary

Improves typing and simplifies usage of bulk editing hooks.

Details

This patch includes the following changes

improved typing allows to strictly control the input
processing rules loading state for the rules table handling logic is moved inside the hooks
hooks are covered by unit tests

Checklist

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

xcrzx · 2022-10-27T13:40:06Z

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts

-  edit?: BulkActionEditPayload[];
-  isDryRun?: boolean;
-}
+export type QueryOrIds = { query: string } | { ids: string[] };


What do you think about using a discriminated union instead?

export type QueryOrIds = { query: string; ids?: undefined } | { query?: undefined; ids: string[] };

A discriminated union is discriminated by some field. In this case there is no way to discriminate types so { query: string; ids?: undefined } | { query?: undefined; ids: string[] } is the same as { query: string } | { ids: string[] } which is the same as { query?: string; ids?: string[]; } if compare which object type can be assigned. So that it's possible to assign { query: 'some query', ids: ['id1', 'id2'] } to all of the above types but the under hood behaviour is undefined if both query and ids are specified.

string | string[] looks as a better option here

A discriminated union is discriminated by some field. In this case there is no way to discriminate types so { query: string; ids?: undefined } | { query?: undefined; ids: string[] } is the same as { query: string } | { ids: string[] } which is the same as { query?: string; ids?: string[]; }

No, this is not correct. Both query and ids properties become a common discriminant in this case as they have mutually exclusive types in the union:

Hm, I see another picture. I tried it inside Kibana and TS playground. TS does not complain on passing { query: 'myQuery', ids: ['id'] }. It complains on an empty object and passing readonly array when a mutable one expected (which also can create some issues).

Sorry, your QueryOrIds type is not correct 🙂 You should use this one:

type QueryOrIds = { query: string; ids?: undefined } | { query?: undefined; ids: string[] };

Yeah, sorry, it's my bad. I didn't pay enough attention to undefined. Basically the key is still assignable but this way it doesn't not create issues.

elasticmachine · 2022-10-30T21:49:48Z

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

elasticmachine · 2022-10-30T21:49:49Z

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

maximpn · 2022-10-31T06:20:04Z

@elasticmachine merge upstream

maximpn · 2022-10-31T08:37:06Z

@elasticmachine merge upstream

maximpn · 2022-10-31T10:21:26Z

@elasticmachine merge upstream

xcrzx

Thanks for the refactoring, @maximpn. I tested the PR locally and looked through the changes. The code looks much cleaner now 👍
I've left a couple of suggestions, primarily minor improvements, nothing critical.

xcrzx · 2022-10-31T11:19:18Z

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts

-      ...(isDryRun ? { dry_run: isDryRun } : {}),
-    },
+    body: JSON.stringify(params),
+    query: dryRun ? { dry_run: true } : {},


Can it be further simplified?

Suggested change

query: dryRun ? { dry_run: true } : {},

query: { dry_run: dryRun },

xcrzx · 2022-10-31T11:31:14Z

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts

+  type: BulkAction.edit;
+  editPayload: BulkActionEditPayload[];
+} & QueryOrIds;
+export type BulkActionDescriptor = PlainBulkActionDescriptor | EditBulkActionDescriptor;


Nit: maybe it's just my previous experience, but the descriptor suffix looks a bit confusing. In JavaScript, it usually refers to property descriptors, and, for example, in Python to property getters and setters. But I've never seen method arguments being named as descriptors.

Can we use a more consistent naming for these API method arguments, like BulkActionProps? The Props suffix is used for almost every other method in this file.

I agree that descriptor isn't the best match here though Prop looks like a universal stub. I finally decided to rename BulkAction to BulkActionType which is consistent with BulkActionEditType and rename BulkActionDescriptor to BulkAction which makes it concise and simple.

xcrzx · 2022-10-31T11:56:54Z

...urity_solution/public/detection_engine/rule_management/logic/bulk_actions/use_bulk_export.ts

+        setLoadingRules?.({
+          ids: queryOrIds.ids ?? [],
+          action: BulkAction.export,
+        });
+        return await mutateAsync(queryOrIds);


Not sure what changed, but previously we showed a loading indicator when exporting rules.

Before

Screen.Recording.2022-10-31.at.12.53.47.mov

After

Screen.Recording.2022-10-31.at.12.54.37.mov

Although, in the current main, I don't see the same behavior (loading overlay) for other bulk actions. Rules remain interactable during a preflight request.

Should we return the previous behavior for the export action and update other actions accordingly for consistency, or is it OK not to show the overlay?

thanks, there was a problem which was fixed

Once the user clicked any of the bulk actions, we should "lock" the table: the user shouldn't be able to change the filters and the rule selection until the bulk action is sent or canceled. If we don't do this like in the 2nd video, there's a chance that the user could make some changes in the table which would lead to different arguments of the dry run request and the final request. Which would be a bug.

I think we should fix this. @maximpn could we fix this for all the bulk actions in a follow-up PR after merging this one? @xcrzx could you please write up a ticket for this issue?

Added a separate ticket: #144264

xcrzx · 2022-10-31T12:11:41Z

...lution/public/detection_engine/rule_management/logic/bulk_actions/use_execute_bulk_action.ts


  const executeBulkAction = useCallback(
-    async ({


This method looks much cleaner now 👍

xcrzx · 2022-10-31T12:16:37Z

...n/public/detection_engine/rule_management/logic/bulk_actions/use_execute_bulk_action.test.ts

+  await executeBulkActionFn(bulkActionDescriptor);
+}
+
+describe('useExecuteBulkAction', () => {


Thanks for adding the tests 🙌

xcrzx · 2022-10-31T12:22:46Z

...lution/public/detection_engine/rule_management/logic/bulk_actions/show_bulk_success_toast.ts

+import { explainBulkSuccess, summarizeBulkSuccess } from './translations';
+
+export function showBulkSuccessToast(
+  toasts: UseAppToasts,


It would probably make sense to transform this function into a hook, so toasts will become a detail of internal implementation rather than passed from outside. It could be used then as follows:

// No knowledge about the `toasts` service on the call side const showBulkSuccessToast = useShowBulkSuccessToast() showBulkSuccessToast(action, summary)

It's impossible to use hooks inside usual functions. downloadExportedRules uses showBulkSuccessToast as well.

Okay, I think downloadExportedRules should not know anything about toasts and provide generic onError and onSuccess callbacks instead. But let's leave this improvement for another time.

xcrzx · 2022-10-31T12:25:21Z

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts

+  const params = {
+    action: BulkAction.export,
+    ...(queryOrIds.query ? { query: queryOrIds.query } : {}),
+    ...(queryOrIds.ids ? { ids: queryOrIds.ids } : {}),
+  };


Would that work?

Suggested change

const params = {

action: BulkAction.export,

...(queryOrIds.query ? { query: queryOrIds.query } : {}),

...(queryOrIds.ids ? { ids: queryOrIds.ids } : {}),

};

const params = {

action: BulkAction.export,

query: queryOrIds.query,

ids: queryOrIds.ids,

};

xcrzx · 2022-10-31T12:27:26Z

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts

+  const params = {
+    action: bulkActionDescriptor.type,
+    ...(bulkActionDescriptor.query ? { query: bulkActionDescriptor.query } : {}),
+    ...(bulkActionDescriptor.ids ? { ids: bulkActionDescriptor.ids } : {}),
+    ...(bulkActionDescriptor.type === BulkAction.edit
+      ? { edit: bulkActionDescriptor.editPayload }
+      : {}),
+  };


Would that work?

Suggested change

const params = {

action: bulkActionDescriptor.type,

...(bulkActionDescriptor.query ? { query: bulkActionDescriptor.query } : {}),

...(bulkActionDescriptor.ids ? { ids: bulkActionDescriptor.ids } : {}),

...(bulkActionDescriptor.type === BulkAction.edit

? { edit: bulkActionDescriptor.editPayload }

: {}),

};

const params = {

action: bulkActionDescriptor.type,

query: bulkActionDescriptor.query,

ids: bulkActionDescriptor.ids,

edit:

bulkActionDescriptor.type === BulkAction.edit ? bulkActionDescriptor.editPayload : undefined,

};

xcrzx · 2022-10-31T12:40:38Z

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts

+  bulkActionDescriptor: BulkActionDescriptor,
+  dryRun?: boolean


Let's use an object for method arguments for consistency with other methods in this file. It also makes it much more understandable for other developers to see what is being passed to the function.

Compare:

// What does this "true" mean? It is not possible to say without looking into the function's implementation performBulkAction(bulkActionDescriptor, true) // It's now clear that "true" enables the dry run mode performBulkAction({ bulkActionDescriptor, dryRun: true })

...urity_solution/public/detection_engine/rule_management/api/hooks/use_bulk_action_mutation.ts

banderror · 2022-10-31T17:31:27Z

...urity_solution/public/detection_engine/rule_management/api/hooks/use_bulk_action_mutation.ts

+          case BulkActionType.enable:
+          case BulkActionType.disable: {


FWIW I'm fine with this specific renaming, BulkActionType is a more precise name.

banderror · 2022-10-31T17:56:45Z

...urity_solution/public/detection_engine/rule_management/logic/bulk_actions/use_bulk_export.ts

+        setLoadingRules?.({
+          ids: queryOrIds.ids ?? [],
+          action: BulkAction.export,
+        });
+        return await mutateAsync(queryOrIds);


Once the user clicked any of the bulk actions, we should "lock" the table: the user shouldn't be able to change the filters and the rule selection until the bulk action is sent or canceled. If we don't do this like in the 2nd video, there's a chance that the user could make some changes in the table which would lead to different arguments of the dry run request and the final request. Which would be a bug.

I think we should fix this. @maximpn could we fix this for all the bulk actions in a follow-up PR after merging this one? @xcrzx could you please write up a ticket for this issue?

banderror

@maximpn Thank you for the clean-up and the added tests 👍 I just left a comment on some of the renamings that could have been avoided. Let's revert them if it won't take too much time.

I think an important thing to address would be to preserve the "locked" state of the table during the bulk action workflow: #144091 (comment)

xcrzx · 2022-11-01T17:16:38Z

...lution/public/detection_engine/rule_management/logic/bulk_actions/show_bulk_success_toast.ts

+import { explainBulkSuccess, summarizeBulkSuccess } from './translations';
+
+export function showBulkSuccessToast(
+  toasts: UseAppToasts,


Okay, I think downloadExportedRules should not know anything about toasts and provide generic onError and onSuccess callbacks instead. But let's leave this improvement for another time.

xcrzx · 2022-11-01T17:21:56Z

...blic/detection_engine/rule_management/logic/bulk_actions/use_all_rule_ids_for_bulk_action.ts

+
+  return useCallback(
+    (bulkActionType: BulkActionType) => {
+      const allRules = rulesTableContext?.state.isAllSelected ? rulesTableContext.state.rules : [];


Not sure I understand this logic. If isAllSelected is true, we get all rules from the table's state, but why an empty array otherwise? How it works if the user selects only a couple of rules, for example?

performBulkAction accepts a query or ids. It's clear what rules are processing if ids is passed but it's impossible to say anything about processing rules in the easy way when a query is passed. To handle a specific case when all rules are selected and a query is passed isAllSelected is used to detect that.

I've changed the hood name to reflect its purpose better.

xcrzx · 2022-11-01T17:38:05Z

...urity_solution/public/detection_engine/rule_management/api/hooks/use_bulk_action_mutation.ts

-  return useMutation<BulkActionResponse, Error, BulkActionProps>(
-    (action: BulkActionProps) => performBulkAction(action),
+  return useMutation<BulkActionResponse, Error, BulkAction>(
+    (bulkAction: BulkAction) => performBulkAction({ bulkAction }),


Let's add back all the arguments accepted by the performBulkAction method.

Hooks located in api/hooks have to be generic. They are designed as an abstraction layer above the API client methods matching them one to one. And these hooks' only responsibility is to define query keys and invalidate other related queries. So hook's interface should match its underlying API method exactly.

We will prohibit direct API usage in the future so that these hooks will become the only API "client" facing users. See #143923

BulkAction -> BulkActionType (which is consistent with BulkActionEditType) BulkActionDescriptor -> BulkAction

kibana-ci · 2022-11-01T21:00:27Z

💚 Build Succeeded

Buildkite Build
Commit: 15d440c

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3197	3200	+3

Async chunks

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

id	before	after	diff
`securitySolution`	9.5MB	9.5MB	+1.8KB

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	57	63	+6
`osquery`	103	108	+5
`securitySolution`	439	443	+4
total			+17

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	65	71	+6
`osquery`	104	110	+6
`securitySolution`	516	520	+4
total			+18

History

💔 Build #84447 failed c7cfdb432e27a0e8ee5289f76954ddb9bae8cf32
💚 Build #84199 succeeded aaf692a74662b4a1211812c6a6158204281cfc6b
💔 Build #84146 failed e54e9ff8f3810c5ca8be36abc3eececf0a53916f
💔 Build #84116 failed fdf8c0855fc60ea3a82a9a7b199319fdc7af5873
💔 Build #84002 failed 353b482a677a137190664fcbda7edd6d12eb6088

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

cc @maximpn

xcrzx

Haven't tested locally for the second time, but the code changes LGTM 👍 Thank you, @maximpn

maximpn self-assigned this Oct 27, 2022

maximpn linked an issue Oct 27, 2022 that may be closed by this pull request

[Security Solution] Improve bulk action executor #142748

Closed

xcrzx reviewed Oct 27, 2022

View reviewed changes

maximpn force-pushed the simplify-bulk-action-execution branch 3 times, most recently from 40d5ffb to 8a74864 Compare October 30, 2022 17:50

maximpn marked this pull request as ready for review October 30, 2022 21:49

maximpn requested a review from a team as a code owner October 30, 2022 21:49

maximpn requested a review from xcrzx October 30, 2022 21:49

xcrzx reviewed Oct 31, 2022

View reviewed changes

maximpn force-pushed the simplify-bulk-action-execution branch from 6788b84 to 353b482 Compare October 31, 2022 14:50

banderror reviewed Oct 31, 2022

View reviewed changes

maximpn requested a review from a team as a code owner October 31, 2022 18:08

xcrzx mentioned this pull request Oct 31, 2022

[Security Solution] The rules management table should be inactive during bulk action preflight requests #144264

Closed

xcrzx reviewed Nov 1, 2022

View reviewed changes

maximpn added 2 commits November 1, 2022 20:24

improve typing of performBulkAction

bcc31a8

simplify performBulkAction

c5ee823

maximpn added 23 commits November 1, 2022 20:24

cover useExecuteBulkAction by unit tests

52fa196

cover useBulkExport by unit tests

1376f5c

implement stricter type division

634d14b

fix lint error

4d40222

fix check types errors

116346c

get rid of unnecessary hook dependency

96b0d7f

fix unit tests

a0cb881

update QueryOrIds type

42247b2

fix unit tests

14e5043

fix unit tests

3f508ba

set rules loading state when all rules are selected

78c205c

fix loading state for all selected rules

b0fccdd

display enable/disable loader only for the relevant rules

12fb9f8

fix unit tests

2c008bd

simplify bulk parameters preparation

f9279f6

rename BulkAction to be consistent

19ab8f1

BulkAction -> BulkActionType (which is consistent with BulkActionEditType) BulkActionDescriptor -> BulkAction

fix bulk export loading state

0d2cbfa

rename bulkActionDescriptor to bulkAction

5839f61

use object notation as an input type

a64847e

update imports

f62db8e

rename to snake case

64d47c2

fix e2e tests

dda048b

mirror performBulkAction API at the mutation hook

bc3202e

maximpn force-pushed the simplify-bulk-action-execution branch from 01da93e to bc3202e Compare November 1, 2022 18:24

use a verbose name for the bulk rule ids hook

15d440c

maximpn force-pushed the simplify-bulk-action-execution branch from c7cfdb4 to 15d440c Compare November 1, 2022 19:34

xcrzx approved these changes Nov 2, 2022

View reviewed changes

maximpn merged commit e391276 into elastic:main Nov 2, 2022

maximpn deleted the simplify-bulk-action-execution branch November 2, 2022 13:46

	query: dryRun ? { dry_run: true } : {},
	query: { dry_run: dryRun },

[Security Solution] Simplify bulk action execution #144091

[Security Solution] Simplify bulk action execution #144091

Conversation

maximpn commented Oct 27, 2022 • edited Loading

Summary

Details

Checklist

Choose a reason for hiding this comment

maximpn Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

xcrzx Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

maximpn Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 30, 2022

elasticmachine commented Oct 30, 2022

maximpn commented Oct 31, 2022

maximpn commented Oct 31, 2022

maximpn commented Oct 31, 2022

xcrzx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpn Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Nov 1, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

xcrzx left a comment

Choose a reason for hiding this comment

maximpn commented Oct 27, 2022 •

edited

Loading

maximpn Oct 27, 2022 •

edited

Loading

xcrzx Oct 28, 2022 •

edited

Loading

maximpn Oct 28, 2022 •

edited

Loading

maximpn Nov 1, 2022 •

edited

Loading