[Detection Engine][Exceptions] - Fix exception item update route #159223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yctercero
added
v8.9.0
v8.8.2
bug
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
rylnd
reviewed
Jun 7, 2023
| @@ -72,7 +72,7 @@ export const updateOverwriteExceptionListItem = async ({ | |||
| version: exceptionListItem._version ? parseInt(exceptionListItem._version, 10) : undefined, | |||
| }, | |||
| { | |||
| id, | |||
| id: exceptionListItem.id, | |||
There was a problem hiding this comment.
Should we be respecting the id argument at all here?
Suggested change
| id: exceptionListItem.id, | |
| id: id ?? exceptionListItem.id, |
There was a problem hiding this comment.
Good call! Yes, I think we should.
rylnd
reviewed
Jun 7, 2023
Comment on lines
34
to
36
| // SDH came up where this route was creating a new item when updating | ||
| // by item_id, ensuring that's no longer the case | ||
| it('should not create a new item on update', async () => { |
There was a problem hiding this comment.
I think we could do away with the comment and put this context into the test description. I deleted the mention of an SDH because IMO the fact that it's a regression test isn't particularly important; deleting a test is an exception for which one had better have a good reason!
Suggested change
| // SDH came up where this route was creating a new item when updating | |
| // by item_id, ensuring that's no longer the case | |
| it('should not create a new item on update', async () => { | |
| it('should not create a new item when updating by item_id', async () => { |
rylnd
approved these changes
Jun 7, 2023
Similar to how specifying item_id when updating causes a new id to be generated, specifying an id causes the item_id to be deleted if unspecified. This commit adds a test to cover this case, along with updating the existing regression to have more assertions and have more accurate variable names.
This test attempts to see exactly what's going on with this endpoint,
currently:
1. Create a fully custom, fully specified exception item
* WIP; see TODOs in code
2. Update it with only the required fields
3. See what changed after the update
… into exceptions_update_route
* More descriptive test/comments * Remove server-generated noise from our assertions * Add comments to the item This test is currently failing due to the fact that we drop tags if they aren't specified. Will see if this behavior is consistent with the old implementation.
If we specify custom tags here, the test will fail because tags are overwritten if not specified on update. However, this has always been the behavior of this endpoint (it at least preceded the changes causing the id-related bugs), so we're leaving it for now.
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @yctercero |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jun 9, 2023
…stic#159223) ## Summary Addresses issue 159230 (cherry picked from commit f895c5c)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jun 9, 2023
…stic#159223) ## Summary Addresses issue 159230 (cherry picked from commit f895c5c)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
saarikabhasi
pushed a commit
to saarikabhasi/kibana
that referenced
this pull request
Jun 14, 2023
…stic#159223) ## Summary Addresses issue 159230
mistic
pushed a commit
that referenced
this pull request
Jun 21, 2023
#159223) (#159420) # Backport This will backport the following commits from `main` to `8.8`: - [[Detection Engine][Exceptions] - Fix exception item update route (#159223)](#159223) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Yara Tercero","email":"yctercero@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-06-09T19:31:03Z","message":"[Detection Engine][Exceptions] - Fix exception item update route (#159223)\n\n## Summary\r\n\r\nAddresses issue 159230","sha":"f895c5c2058f3f36c828fe9d8f1dbc0c19a2b381","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team: SecuritySolution","Feature:Rule Exceptions","v8.7.2","v8.9.0","Team:Detection Engine","v8.8.2"],"number":159223,"url":"#159223 Engine][Exceptions] - Fix exception item update route (#159223)\n\n## Summary\r\n\r\nAddresses issue 159230","sha":"f895c5c2058f3f36c828fe9d8f1dbc0c19a2b381"}},"sourceBranch":"main","suggestedTargetBranches":["8.7","8.8"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"#159223 Engine][Exceptions] - Fix exception item update route (#159223)\n\n## Summary\r\n\r\nAddresses issue 159230","sha":"f895c5c2058f3f36c828fe9d8f1dbc0c19a2b381"}},{"branch":"8.8","label":"v8.8.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Yara Tercero <yctercero@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses #159230. Fixes a bug in exception item update route. When updating an exception item and not passing in
id,idhere is undefined and because it does not see an SO to overwrite, it creates it.Wrote test to confirm that this behavior is occurring:
Checklist