-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Create and update case API guardrails for title, description, category, tags #160844
[Cases] Create and update case API guardrails for title, description, category, tags #160844
Conversation
…eate and update case
…vi/kibana into guardrails-cases-api
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
}); | ||
}); | ||
|
||
it(`does not throw error when tags array is empty`, 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.
I wonder if we should throw for scenarios like this.
Calling patch
without any params is allowed but we might as well notify the caller that something is probably wrong. @cnasikas ?
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.
We should let users put an empty array of tags as is the only way to remove all tags from a case. Same when creating a case.
x-pack/test/cases_api_integration/security_and_spaces/tests/common/cases/patch_cases.ts
Show resolved
Hide resolved
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 work, left a few questions.
rt.string.is, | ||
(input, context) => | ||
either.chain(rt.string.validate(input, context), (s) => { | ||
if (s.trim().length < min) { |
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.
Hmm do we want spaces to count as valid characters towards the min and max?
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.
I am assuming here that start and end of string spaces are unnecessary and should be removed before checking the length. But I am open if we want to keep it.
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.
I think it all depends on what we persist in ES. If we persist the spaces (which I think we are), then we should not trim. Or the other way around, if we think that we should trim (probably yes because we should not allow a title of only spaces), then we should trim before persisting it to ES. I lean towards trimming it to not allow titles with only empty spaces.
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 makes sense to me to trim for validation as well as before persisting to ES.
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.
Ok, let's leave the trimming but let's be consistent (not on this PR). In the UI for example no error is being shown in the case view page when you add an empty string to the title. In the backend, we do not trim before saving.
x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/docs/openapi/components/schemas/create_case_request.yaml
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml
Outdated
Show resolved
Hide resolved
x-pack/plugins/cases/docs/openapi/components/schemas/update_case_request.yaml
Outdated
Show resolved
Hide resolved
rt.string.is, | ||
(input, context) => | ||
either.chain(rt.string.validate(input, context), (s) => { | ||
if (s.trim().length === 0 && s.trim().length < min) { |
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.
As we trim a lot in this function I think it is better if we put it on a variable. For example, const trimedString = s.trim()
💚 Build Succeeded
Metrics [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: |
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.
OAS changes LGTM, thanks!
Connected to #146945
Summary
Note: In this PR,
maximum length of title (160 characters)
andmaximum length of category field (50 characters)
validations are also moved to schema validation.Checklist
Release notes
The Create Case and Update Case APIs put the following limits: