[Cases] Remove case id from alerts when deleting a case #154829
Conversation
cnasikas
added
release_note:skip
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Pinging @elastic/response-ops-cases (Feature:Cases) |
| ctx._source['${ALERT_CASE_IDS}'].remove(i); | ||
| } | ||
| if (ctx._source['${ALERT_CASE_IDS}'].contains('${caseId}')) { | ||
| int index = ctx._source['${ALERT_CASE_IDS}'].indexOf('${caseId}'); |
There was a problem hiding this comment.
remove may mess up with the index and accessing the array like array[i] can produce an out-of-bounds error.
There was a problem hiding this comment.
Great catch, I totally forgot about that in Java: https://stackoverflow.com/questions/10431981/remove-elements-from-collection-while-iterating
| const SCRIPT_PARAMS_ID = 'caseIds'; | ||
|
|
||
| const painlessScript = `if (ctx._source['${ALERT_CASE_IDS}'] != null && ctx._source['${ALERT_CASE_IDS}'].length > 0 && params['${SCRIPT_PARAMS_ID}'] != null && params['${SCRIPT_PARAMS_ID}'].length > 0) { | ||
| for (int i=0; i < params['${SCRIPT_PARAMS_ID}'].length; i++) { |
There was a problem hiding this comment.
nit: it might be a little easier to read if we create a temporary variable for params['${SCRIPT_PARAMS_ID}'] and ctx._source['${ALERT_CASE_IDS}']
Something like:
if (...) {
List storedCaseIds = ctx._source['${ALERT_CASE_IDS}']; <- I think List is the right type here
List caseIdsToRemove = params['${SCRIPT_PARAMS_ID}'];
for (...) { }
}
| @@ -75,6 +75,7 @@ export async function deleteCases(ids: string[], clientArgs: CasesClientArgs): P | |||
| entities: bulkDeleteEntities, | |||
| options: { refresh: 'wait_for' }, | |||
| }), | |||
| alertsService.removeCaseIdsFromAllAlerts({ caseIds: ids }), | |||
There was a problem hiding this comment.
In the future it might be interesting to investigate if we can skip some cases that we're pretty confident do not have any alerts attached to them. I think we could leverage our total_alerts field and ignore cases that have it set to 0. Not something we should do in this PR and might not even be worth doing in general 🤷♂️
There was a problem hiding this comment.
Nice idea! We can do it when we bound our delete API so the number of cases to retrieve is also bounded.
|
|
||
| const alertAfterDeletion = await getAlerts(alerts); | ||
|
|
||
| const caseIdsWithoutRemovedCase = |
There was a problem hiding this comment.
nit: I think this logic is the same as in createCaseAttachAlertAndDeleteAlert, might be worth moving it to a function
💔 Build FailedFailed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
This PR removes the case id from all alerts attached to a case when deleting a case. It also removes any alert authorization when removing alerts from a case.
Checklist
Delete any items that are not applicable to this PR.
For maintainers