[Cases] Validate page and perPage parameters in find APIs #161111

adcoelho · 2023-07-03T15:08:11Z

Connected to #146945

Summary

Description	Limit	Done?	Documented?
Total number of cases/user actions/comments per page	100	✅	No
Maximum number of cases/user actions/comments returned from the API	10.000	✅	No

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Release Notes

Max value for perPage parameter in find Cases API is now 100.
Max value for perPage parameter in find User Actions API is now 100.
Max value for perPage parameter in find Comments API is now 100.

elasticmachine · 2023-07-03T18:46:55Z

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

elasticmachine · 2023-07-03T18:46:57Z

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

cnasikas

Great job. I left some comments.

x-pack/plugins/cases/server/common/validators.ts

x-pack/plugins/cases/common/api/cases/case.ts

x-pack/plugins/cases/common/api/cases/user_actions/operations/find.ts

Update pagination validation for find user actions. Update pagination validation for find comments. Added unit and e2e tests. Updated the documentation.

Remove code validation for paginations. Tests.

…-fix'

lcawl

OAS LGTM, thanks!

cnasikas

Nice 🚀!

cnasikas · 2023-07-04T14:38:52Z

x-pack/plugins/cases/common/schema/index.test.ts

+      `);
+    });
+
+    it(`fails when page * perPage > ${MAX_DOCS_PER_PAGE}`, () => {


I think this test is the same as the previous one. Probably you wanted to set the perPage to MAX_DOCS_PER_PAGE.

x-pack/plugins/cases/common/schema/index.test.ts

cnasikas · 2023-07-05T08:46:43Z

x-pack/plugins/cases/common/schema/index.test.ts

+      `);
+    });
+
+    it('fails when page > maxPerPage', () => {


nit: fails when page -> fails when perPage

This was actually right, the schema doesMath.max(pageAsNumber, pageAsNumber * perPageAsNumber) > MAX_DOCS_PER_PAGE so here I am testing the first half of the Math.max.

You pass decode({ perPage: 4 }))) so I think it tests the perPage and not the page.

ah, that is the mistake though :D

I was looking at the similar named test fails when page > ${MAX_DOCS_PER_PAGE}

ah, that is the mistake though :D

Depends on the angle you are looking at it hahahaha

cnasikas · 2023-07-05T08:49:33Z

x-pack/plugins/cases/common/schema/index.test.ts

+      `);
+    });
+
+    it.skip('fails when page number is negative', () => {


We should unskip the tests, right?

I wrote the tests before the feature so I skipped them. But I didn't implement this validation yet 😅

Maybe I'll delete and do it in a different PR.

Oh, I thought it was part of this PR. We can leave them as it is and unskip them when you add the new validation.

cnasikas · 2023-07-05T08:56:46Z

x-pack/plugins/cases/common/schema/types.ts

+const PageTypeRt = rt.union([rt.number, NumberFromString]);
+type PageNumberType = rt.TypeOf<typeof PageTypeRt>;
+
+export interface PaginationType {


nit: I found the Type at the end a bit redundant.

If I don't export this one we get a typescript error everywhere we use the paginationSchema.

Exported variable '<whatever>' has or is using name 'PaginationType' from external module "/Users/adcoelho/Development/kibana/x-pack/plugins/cases/common/schema/types" but cannot be named.

Exporting is fine. I think that the wording (naming of the type) is a bit confusing.

cnasikas · 2023-07-05T08:57:22Z

x-pack/plugins/cases/common/schema/types.ts

+}
+
+export const PaginationSchemaRt = rt.exact(rt.partial({ page: PageTypeRt, perPage: PageTypeRt }));
+export type PartialPaginationType = Partial<PaginationType>;


nit: What about Pagination?

js-jankisalvi

looks good 👍

x-pack/plugins/cases/common/schema/index.test.ts

kibana-ci · 2023-07-05T11:26:00Z

💚 Build Succeeded

Buildkite Build
Commit: a297f3f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cases`	657	658	+1

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`cases`	26	27	+1

Page load bundle

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

id	before	after	diff
`cases`	144.6KB	145.5KB	+860.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	410	414	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	15	17	+2
`securitySolution`	489	493	+4
total			+6

History

💚 Build #140116 succeeded 76f739d
💔 Build #140029 failed 41cdde5
💔 Build #140012 failed 173a910
💚 Build #139857 succeeded ca981fda1144e068c507be4e69ff1a130a6a8a1c

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

cc @adcoelho

adcoelho added release_note:breaking Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.10.0 labels Jul 3, 2023

adcoelho self-assigned this Jul 3, 2023

adcoelho changed the title ~~[Cases] Validate page and perPage in find APIs~~ [Cases] Validate page and perPage parameters in find APIs Jul 3, 2023

adcoelho marked this pull request as ready for review July 3, 2023 18:46

adcoelho requested review from a team as code owners July 3, 2023 18:46

cnasikas mentioned this pull request Jul 4, 2023

[Cases] Guardrails in Cases APIs #146945

Closed

cnasikas reviewed Jul 4, 2023

View reviewed changes

x-pack/plugins/cases/server/common/validators.ts Outdated Show resolved Hide resolved

x-pack/plugins/cases/common/api/cases/case.ts Outdated Show resolved Hide resolved

x-pack/plugins/cases/common/api/cases/user_actions/operations/find.ts Outdated Show resolved Hide resolved

adcoelho added 3 commits July 4, 2023 14:36

Update pagination validation for find cases.

8e1f127

Update pagination validation for find user actions. Update pagination validation for find comments. Added unit and e2e tests. Updated the documentation.

Updated documentation.

1d1684d

Create PaginationSchema.

173a910

Remove code validation for paginations. Tests.

adcoelho force-pushed the per-page-limit branch from 9e1d3c3 to 173a910 Compare July 4, 2023 12:39

kibanamachine and others added 3 commits July 4, 2023 13:19

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

41cdde5

…-fix'

Fixed tests by making paginationSchema exact.

e549867

[DOCS] Generate API preview

76f739d

lcawl approved these changes Jul 4, 2023

View reviewed changes

cnasikas approved these changes Jul 5, 2023

View reviewed changes

js-jankisalvi approved these changes Jul 5, 2023

View reviewed changes

x-pack/plugins/cases/common/schema/index.test.ts Outdated Show resolved Hide resolved

adcoelho added 2 commits July 5, 2023 11:52

Address PR.

7c1ebf9

Addressing PR comments 2

a297f3f

adcoelho merged commit f43601d into elastic:main Jul 5, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Validate page and perPage parameters in find APIs #161111

[Cases] Validate page and perPage parameters in find APIs #161111

adcoelho commented Jul 3, 2023

elasticmachine commented Jul 3, 2023

elasticmachine commented Jul 3, 2023

cnasikas left a comment

lcawl left a comment

cnasikas left a comment •

edited

Loading

cnasikas Jul 4, 2023

cnasikas Jul 5, 2023

adcoelho Jul 5, 2023 •

edited

Loading

cnasikas Jul 5, 2023

adcoelho Jul 5, 2023

adcoelho Jul 5, 2023

cnasikas Jul 5, 2023 •

edited

Loading

cnasikas Jul 5, 2023

adcoelho Jul 5, 2023

cnasikas Jul 5, 2023

cnasikas Jul 5, 2023

adcoelho Jul 5, 2023

cnasikas Jul 5, 2023

cnasikas Jul 5, 2023

js-jankisalvi left a comment

kibana-ci commented Jul 5, 2023

ESLint disabled line counts

Total ESLint disabled count

[Cases] Validate page and perPage parameters in find APIs #161111

[Cases] Validate page and perPage parameters in find APIs #161111

Conversation

adcoelho commented Jul 3, 2023

Summary

Checklist

Release Notes

elasticmachine commented Jul 3, 2023

elasticmachine commented Jul 3, 2023

cnasikas left a comment

Choose a reason for hiding this comment

lcawl left a comment

Choose a reason for hiding this comment

cnasikas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adcoelho Jul 5, 2023 • edited Loading

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 Jul 5, 2023 • edited Loading

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

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

kibana-ci commented Jul 5, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing exports

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

cnasikas left a comment •

edited

Loading

adcoelho Jul 5, 2023 •

edited

Loading

cnasikas Jul 5, 2023 •

edited

Loading