[Cases] Fixing issue with find api total response

[jonathan-buttner]

Summary

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.

GIF of it working

Checklist

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

[jonathan-buttner]

These changes are because I created a new describe block for the already existing tests and for the new pagination tests. These changes are really just bringing the older tests under the new describe block and indentation.

[jonathan-buttner]

This is where the new tests begin.

[elasticmachine]

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

[jonathan-buttner]

@XavierM question about the release_note:breaking change label, is this a breaking change for 7.12? Or does that label mean we need to add some docs about the presence of the issue? I don't think this PR is a breaking change on the API right?

[cnasikas]

nit: As of 2.0.0 supertest supports native promised and supertest-as-promised is not needed. Do you need this only for the type?

[jonathan-buttner]

Yeah it's just needed for the type because I'm putting all the promises in an array like this:

const responsePromises: supertestAsPromised.Test[] = [];

That was the location that vscode thought the type was defined 🤷‍♂️

[cnasikas]

I read your comment on top of this function but still I don't get why we need this to validate the cases?

[jonathan-buttner]

Sorry for the confusion, I'll see if I can describe issue here. Essentially, for the tests that walk the pages I wanted some way to verify that the cases returned by the _find API were in the correct order (and actually on that page). To do that we need a reliable way to retrieve all the cases and then we can compare the chunk that the _find API returns for a specific page.

For example if the test requests page 2 with a per_page of 2 when there are 10 total cases (say case, 0,1,2,3,4,5 etc created in that order), then the find API retrieve cases 2 and 3 (0 and 1 would be on the first page). So we need a source of truth ideally separate from the find API to gather all the cases and then compare the find results against that ground truth.

So then the question is how can we ensure that specific cases will be on a specific page for pagination. We could make 10 create API requests one at a time that each create a case. This would guarantee that the created_at field of the 10 cases would be in ascending order and we could get away with just comparing the find response's created_at to the response from the create API in the test setup. Unfortunately, creating cases one at a time was pretty slow.

Another option would be not have the sorting be on the created_at field. For example if we could set each case title to a deterministic value as we create the tests then we'd know what the order would be when the find API is paginating through them. Unfortunately, the case backend implementation only allows us to sort on a few fields and title isn't allowed.

So the only fast solution I could think of was to create the cases using a Promise.all and use an outside method (using Elasticsearch directly to retrieve the cases in sorted order) to retrieve all the cases. Promise.all doesn't guarantee that for a specific i in the loop that the case's created_at date would be in order because they're being done asynchronously. The loop goes from 0-9 but case 9 could be created before 0 for example.

[cnasikas]

I see! Now all make sense. Thanks for the detailed explanation!

[cnasikas]

I left some nit comments. Ignore them as we are in hurry and this fix must get into 7.12 the soonest possible. This changes need to be merge on master and 7.x, right?

@XavierM question about the release_note:breaking change label, is this a breaking change for 7.12? Or does that label mean we need to add some docs about the presence of the issue? I don't think this PR is a breaking change on the API right?

You are right. There is no need for the release_note:breaking change label as it should be used when there is a configuration change, for example in kibana.yml, or an API breaking change. If you add this label you need to document the breaking changes which in this PR I don't see any. @XavierM Is that a blocker?

[jonathan-buttner]

I left some nit comments. Ignore them as we are in hurry and this fix must get into 7.12 the soonest possible. This changes need to be merge on master and 7.x, right?

Yeah it does need to be in 7.x and master. We ran into some problems with master though because with the introduction of sub cases and empty collections it's more difficult to accurately calculate the correct total for the _find API (specifically, we don't want to include empty collections if. the request is filtering on status and the only way to know that currently is to request the sub cases for every case). Xavier came up with a more performant solution though 👍 . So the implementation on master is going to be fairly different from the fix here.

[stephmilovic]

code review ✅
manual testing ✅
thanks for the fix and the tests to confirm. LGTM 🏁

[jonathan-buttner]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 1b73d56
• Storybooks Preview

Metrics [docs]

✅ unchanged

History

• 💚 Build #114138 succeeded 8dbfffe
• 💚 Build #114091 succeeded 8dbfffe

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