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

[Cases] Fix _find API total bug #95235

Merged

Conversation

jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Mar 23, 2021

Summary

This PR is waiting on this one: #95122 to be merged because they share some of the same files for restructuring the integration tests.

This PR address the bug described here: #94929 where the _find cases API does not return the correct value for the total field. This makes the UI think there are no additional cases and therefore breaks the pagination functionality.

The backend was incorrectly setting the total field to be the number of cases that were returned by the saved objects find call. Instead it should be setting the total field to the returned total attribute in the saved objects client response.

This solution will work until the sub cases feature is enabled by default. Once that occurs it will be possible to have empty case collections which in most cases should probably be ignored in the case list view. If we end up going that route we'll need to filter out the empty collections and adjust the total response accordingly.

A possible solution would be to add a new field to the case saved object that indicates if a collection is empty or not. We'd need to check and set the field any time a sub case is added to the collection or deleted from a collection which could run into race conditions.

@jonathan-buttner jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Feature:Cases Cases feature v7.13.0 labels Mar 23, 2021
@@ -6,6 +6,8 @@
*/

import expect from '@kbn/expect';
import supertestAsPromised from 'supertest-as-promised';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look worse than they are. I wrapped the original tests in a describe block so I could create a new describe for the pagination tests. These tests should be equivalent to the ones added here: #94971

count_closed_cases: 0,
count_in_progress_cases: 0,
describe('basic find tests', () => {
afterEach(async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the deletion of the sub cases to only the basic tests so they don't delete the sub cases for the pagination describe block.

caseID: caseInfo.id,
actionID,
describe('pagination', () => {
const numCases = 4;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only doing 4 sub cases for the pagination because it takes a long time create the sub cases.

.send(postCollectionReq)
.expect(200));

await createSubCases(numCases, collection.id);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't use a Promise.all here because we run into saved object conflicts when the alerts get generated.

supertest,
caseID: caseInfo.id,
actionID,
describe('pagination', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested these locally but they're skipped anyway because the case connector feature is disabled.

@jonathan-buttner jonathan-buttner marked this pull request as ready for review March 25, 2021 21:32
@jonathan-buttner jonathan-buttner requested a review from a team as a code owner March 25, 2021 21:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@jonathan-buttner jonathan-buttner merged commit f4526ad into elastic:master Mar 26, 2021
@jonathan-buttner jonathan-buttner deleted the case-find-total-bug-master branch March 26, 2021 12:46
jonathan-buttner added a commit to jonathan-buttner/kibana that referenced this pull request Mar 26, 2021
* Cherry-picking 7.12 find total fix

* Starting fix for total bug in master with new field

* Adding feature flag for sub cases

* Disabling case as a connector in security solution

* Adding additional tests for pagination

* Removing other api integration tests

* Fixing up problems from merge

* Fixing sub case tests and type errors

* Renaming comment tag for case connector
jonathan-buttner added a commit that referenced this pull request Mar 26, 2021
* Cherry-picking 7.12 find total fix

* Starting fix for total bug in master with new field

* Adding feature flag for sub cases

* Disabling case as a connector in security solution

* Adding additional tests for pagination

* Removing other api integration tests

* Fixing up problems from merge

* Fixing sub case tests and type errors

* Renaming comment tag for case connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants