[Cases] Remove `fields` param from the bulk get cases internal API #156366

cnasikas · 2023-05-02T12:04:26Z

Summary

This PR removes the fields parameter from the bulk get cases internal API. It also returns a subset of the cases attributes.

Fixes: #156364

Checklist

Delete any items that are not applicable to this PR.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-05-02T12:07:42Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2023-05-02T12:07:44Z

Pinging @elastic/response-ops-cases (Feature:Cases)

adcoelho · 2023-05-02T12:42:02Z

x-pack/plugins/cases/public/api/decoders.ts

-  fields?: string[]
-) => {
+export const decodeCasesBulkGetResponse = (res: CasesBulkGetResponse) => {
+  const fields = Object.keys(CasesBulkGetResponseFieldsRt.props);
  const typeToDecode = getTypeForCertainFieldsFromArray(CasesRt, fields);


This feels a bit overly complicated. If we always return the same fields and we already have their types we don't need to generate fields and then create typeToDecode we might as well store that in a const and do away with the extra logic.

I think something like rt.array(CasesBulkGetResponseFieldsRt).decode(res.cases) might work here.

Good point!

adcoelho · 2023-05-02T12:45:47Z

x-pack/plugins/cases/public/api/index.test.ts

@@ -68,7 +61,7 @@ describe('api', () => {
    });

    it('should have been called with the correct path', async () => {
-      await bulkGetCases({ http, params: { ids: ['test'], fields: ['title'] } });
+      await bulkGetCases({ http, params: { ids: ['test'] } });
      expect(http.post).toHaveBeenCalledWith('/internal/cases/_bulk_get', {
        body: '{"ids":["test"],"fields":["title"]}',


,"fields":["title"] This test still passes?

They shouldn't 🙂. I missed that.

x-pack/plugins/cases/server/client/cases/bulk_get.ts

adcoelho · 2023-05-02T12:57:26Z

x-pack/plugins/cases/server/client/cases/bulk_get.ts

        alerts: 0,
        userComments: 0,
      };

      const flattenedCase = flattenCaseSavedObject({
        savedObject: theCase,
        totalComment: userComments,
-        totalAlerts: alerts,
      });

      if (!fields?.length) {


Do we still need this? fields is always defined.

No, you are right.

x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_get_cases.ts

js-jankisalvi

looks good 👍 one minor question.

js-jankisalvi · 2023-05-03T07:59:43Z

x-pack/plugins/cases/public/api/decoders.ts

-  pipe(typeToDecode.decode(res.cases), fold(throwErrors(createToasterPlainError), identity));
+export const decodeCasesBulkGetResponse = (res: CasesBulkGetResponse) => {
+  pipe(
+    CasesBulkGetResponseFieldsRt.decode(res.cases),


Doesn't CasesBulkGetResponseFieldsRt attribute types need to be verified with excess?

This is a good question. We only care about excess parameters coming from external requests to our APIs. For responses, it is ok to not check about them. In the serverless world, we should use strict and remove any fields we do not recognize.

js-jankisalvi

Nice work 👍

jonathan-buttner · 2023-05-03T16:09:59Z

x-pack/plugins/cases/public/api/decoders.ts

-  pipe(typeToDecode.decode(res.cases), fold(throwErrors(createToasterPlainError), identity));
+export const decodeCasesBulkGetResponse = (res: CasesBulkGetResponse) => {
+  pipe(
+    CasesBulkGetResponseRt.props.cases.decode(res.cases),


Do we want to use the entire CasesBulkGetResponseRt for this? We probably want to know if the errors are formatted correctly right?

Correct! Before it was not possible because I was constructing the type dynamically.

x-pack/plugins/cases/server/client/cases/bulk_get.ts

jonathan-buttner · 2023-05-03T16:15:29Z

x-pack/plugins/cases/server/client/cases/bulk_get.ts

-    const typeToEncode = getTypeForCertainFieldsFromArray(CasesRt, fields);
-    const casesToReturn = typeToEncode.encode(flattenedCases);
-
+    const casesToReturn = CasesBulkGetResponseRt.props.cases.encode(flattenedCases);
    const errors = constructErrors(soBulkGetErrors, unauthorizedCases);

    return { cases: casesToReturn, errors };


Should we encode the entire response?

Again correct!

kibana-ci · 2023-05-05T08:49:29Z

💚 Build Succeeded

Buildkite Build
Commit: 1044e0d

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	1.4MB	1.4MB	-100.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`cases`	166.4KB	165.9KB	-560.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	398	401	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	478	481	+3
total			+5

History

💚 Build #125147 succeeded d077668
💔 Build #124859 failed 7ddecb8
💔 Build #124757 failed 7357c93

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

cc @cnasikas

cnasikas added 3 commits May 2, 2023 14:15

Remove the fields parameter

c1b6868

Fix tests

50709ef

Fix error in alerts table

7357c93

cnasikas mentioned this pull request May 2, 2023

[Cases][Meta] Road to versioned HTTP APIs #153726

Open

30 tasks

cnasikas self-assigned this May 2, 2023

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.9.0 labels May 2, 2023

cnasikas changed the title ~~[Cases] Narrow bulk get~~ [Cases] Remove fields param from the bulk get cases internal API May 2, 2023

cnasikas marked this pull request as ready for review May 2, 2023 12:07

cnasikas requested a review from a team as a code owner May 2, 2023 12:07

adcoelho reviewed May 2, 2023

View reviewed changes

x-pack/plugins/cases/server/client/cases/bulk_get.ts Show resolved Hide resolved

adcoelho reviewed May 2, 2023

View reviewed changes

x-pack/plugins/cases/server/client/cases/bulk_get.ts Show resolved Hide resolved

adcoelho reviewed May 2, 2023

View reviewed changes

PR feedback

7ddecb8

adcoelho reviewed May 2, 2023

View reviewed changes

x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_get_cases.ts Show resolved Hide resolved

js-jankisalvi reviewed May 3, 2023

View reviewed changes

cnasikas added 2 commits May 3, 2023 16:01

Fix bug

6c1a6c0

Merge branch 'main' into narrow_bulk_get

d077668

cnasikas requested review from adcoelho and js-jankisalvi May 3, 2023 13:22

js-jankisalvi approved these changes May 3, 2023

View reviewed changes

jonathan-buttner reviewed May 3, 2023

View reviewed changes

PR feedback

28162e1

cnasikas requested a review from jonathan-buttner May 5, 2023 07:39

Merge branch 'main' into narrow_bulk_get

1044e0d

jonathan-buttner approved these changes May 8, 2023

View reviewed changes

cnasikas merged commit 6493331 into elastic:main May 8, 2023

cnasikas deleted the narrow_bulk_get branch May 8, 2023 14:40

kibanamachine added the backport:skip This commit does not require backporting label May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Remove `fields` param from the bulk get cases internal API #156366

[Cases] Remove `fields` param from the bulk get cases internal API #156366

cnasikas commented May 2, 2023 •

edited

Loading

elasticmachine commented May 2, 2023

elasticmachine commented May 2, 2023

adcoelho May 2, 2023

adcoelho May 2, 2023

cnasikas May 2, 2023

adcoelho May 2, 2023

cnasikas May 2, 2023 •

edited

Loading

adcoelho May 2, 2023

cnasikas May 2, 2023

js-jankisalvi left a comment

js-jankisalvi May 3, 2023

cnasikas May 3, 2023

js-jankisalvi left a comment

jonathan-buttner May 3, 2023

cnasikas May 5, 2023

jonathan-buttner May 3, 2023

cnasikas May 5, 2023

kibana-ci commented May 5, 2023

ESLint disabled line counts

Total ESLint disabled count

[Cases] Remove fields param from the bulk get cases internal API #156366

[Cases] Remove fields param from the bulk get cases internal API #156366

Conversation

cnasikas commented May 2, 2023 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented May 2, 2023

elasticmachine commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js-jankisalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js-jankisalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented May 5, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

[Cases] Remove `fields` param from the bulk get cases internal API #156366

[Cases] Remove `fields` param from the bulk get cases internal API #156366

cnasikas commented May 2, 2023 •

edited

Loading

cnasikas May 2, 2023 •

edited

Loading