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.

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 Solution] Improve bulk action executor #142748

Closed
maximpn opened this issue Oct 5, 2022 · 2 comments · Fixed by #144091
Closed

[Security Solution] Improve bulk action executor #142748

maximpn opened this issue Oct 5, 2022 · 2 comments · Fixed by #144091
Assignees
Labels
8.6 candidate Feature:Rule Management Security Solution Detection Rule Management area refactoring Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture v8.6.0

Comments

@maximpn
Copy link
Contributor

maximpn commented Oct 5, 2022

Relates to: #139897

Summary

Current bulk action executor violates single responsibility principle so that it blocks concise feature development.

Details

While working on #139897 I've discovered that bulk action executor function violates single responsibility principle which makes it harder to extend its functionality by composition. The major part of the input parameters are optional and related to some of the actions. It makes it harder to use TypeScript guarding against passing unrelated parameters and requires good understanding of the function internals before its usage.

Obviously there is a room for improvement here. It can be achieved by splitting the logic by responsibility and improve typings. Decisions about the next steps like on success or on error should be done by the driver code. Taking that into account there are two ways

  • split the executor into action executors
type BulkSearchParams = { query: string } | { ids: string[] };

async function executeRulesBulkAction(searchParams: BulkSearchParams): Promise<BulkActionResponse> {...}

async function executeRulesEditBulkAction(searchParams: BulkSearchParams, payload: BulkActionEditPayload[]): Promise<BulkActionResponse> {...}

async function executeRulesExportBulkAction(searchParams: BulkSearchParams): Promise<Blob> {...}
  • split actions by type
type BulkSearchParams = { query: string } | { ids: string[] };
type BulkAction = {
  action: BulkAction.enable | BulkAction.disable | BulkAction.delete | BulkAction.duplicate | BulkAction.export;
  search: BulkSearchParams;
} | {
  action: BulkAction.edit;
  search: BulkSearchParams;
  payload: BulkActionEditPayload[];
}

async function executeRulesBulkAction<T extends BulkAction>(action: BulkAction): Promise<T extends 
 BulkAction.export ? Blob : BulkActionResponse> {...}

To handle errors and perform any other actions it's enough to wrap a function from above to try/catch/finally or create a reusable wrapper function. Unspecified arguments should be used by the driver code.

defaultSuccessHandler and defaultErrorHandler should be split into pure message generator functions to be exported separately and used by the driver code.

@maximpn maximpn added technical debt Improvement of the software architecture and operational architecture impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team labels Oct 5, 2022
@maximpn maximpn self-assigned this Oct 5, 2022
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@banderror banderror added refactoring Feature:Rule Management Security Solution Detection Rule Management area 8.6 candidate and removed impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Oct 6, 2022
@maximpn maximpn linked a pull request Oct 11, 2022 that will close this issue
3 tasks
@maximpn maximpn linked a pull request Oct 27, 2022 that will close this issue
1 task
maximpn added a commit that referenced this issue Nov 2, 2022
**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

- [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.6 candidate Feature:Rule Management Security Solution Detection Rule Management area refactoring Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture v8.6.0
Projects
None yet
3 participants