-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] Restrict the Find Comment API query params #156863
[Cases] Restrict the Find Comment API query params #156863
Conversation
e21af7b
to
475c731
Compare
…-ref HEAD~1..HEAD --fix'
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job. I left some comments.
x-pack/plugins/cases/docs/openapi/paths/s@{spaceid}@api@cases@{caseid}@comments@_find.yaml
Show resolved
Hide resolved
x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/find_comments.ts
Outdated
Show resolved
Hide resolved
Avoid spread notation in find comments.
New tests for validation.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, left a few comments. How about we switch the integration tests to use the helper functions that we have?
if ( | ||
queryParams?.page && | ||
queryParams?.perPage && | ||
queryParams?.page * queryParams?.perPage > MAX_DOCS_PER_PAGE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queryParams.page
can be 0 right? If it is 0 and perPage
is 1 million we'll still want to throw. Also if queryParams.page
was 0 it is falsy right?
await supertest | ||
.post(`${CASES_URL}/${postedCase.id}/comments`) | ||
.post(INTERNAL_BULK_CREATE_ATTACHMENTS_URL.replace('{case_id}', postedCase.id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the helper function here: https://github.com/elastic/kibana/blob/main/x-pack/test/cases_api_integration/common/lib/api/attachments.ts#L82
.expect(200); | ||
|
||
const { body: patchedCase } = await supertest | ||
const { body: caseComments } = await supertest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}); | ||
|
||
it('unhappy path - 400s when query is bad', async () => { | ||
it('unhappy path - 400s when total items invalid', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test when page
is 0?
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @adcoelho |
Fixes #155983 ## Summary This PR changes the accepted params for the Find Comments API to be only `perPage`, `page` and `sort_order`. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: lcawl <lcawley@elastic.co>
## Summary [This PR was merged too soon](#156863). (my bad, I pressed merged automatically and some comments showed up afterwards). I am addressing them now. Basically: 1. Check for possible 0 as `page` query param in the `findComments` API. 2. Use the test utils in the `findComments` tests. I also deleted a test that was duplicated and used the same utils in every test not just the one connected to the PR comment.
Fixes #155983
Summary
This PR changes the accepted params for the Find Comments API to be only
perPage
,page
andsort_order
.