[Cases] Add decode to attachment functionality #158424
Conversation
jonathan-buttner
added
release_note:skip
| @@ -172,7 +180,7 @@ const CommentAttributesWithoutRefsRt = rt.union([ | |||
| AttributesTypeUserRt, | |||
| AttributesTypeAlertsRt, | |||
| AttributesTypeActionsRt, | |||
| ExternalReferenceWithoutRefsRt, | |||
| AttributesTypeExternalReferenceWithoutRefsRt, | |||
There was a problem hiding this comment.
I think this was a bug before. ExternalReferenceWithoutRefsRt. Doesn't include the common attributes like created_by etc.
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Pinging @elastic/response-ops-cases (Feature:Cases) |
| if (isSOError(attachment)) { | ||
| // Forcing the type here even though it is an error. The client is responsible for | ||
| // determining what to do with the errors | ||
| // TODO: we should fix the return type of this function so that it can return errors |
There was a problem hiding this comment.
Unfortunately the SavedObjectsBulkUpdateResponse is wrong so we need to cast it at some point somewhere. Similar to patchCases in x-pack/plugins/cases/server/services/cases/index.ts we had to cast.
There was a problem hiding this comment.
Yeah, I mean we can create our own type where we force it to be what we want.
| ): SavedObjectsBulkResponse<AttachmentTransformedAttributes> { | ||
| const validatedAttachments: AttachmentSavedObjectTransformed[] = []; | ||
|
|
||
| for (const so of res.saved_objects) { |
There was a problem hiding this comment.
Shouldn't we check if it is a SO error?
There was a problem hiding this comment.
Yep, good idea
| ): Array<SavedObject<AttributesTypeAlerts>> { | ||
| return response.saved_objects.map((so) => { | ||
| // We need a cast here because the limited attachment type conflicts with the expected result even though they | ||
| // should be the same |
There was a problem hiding this comment.
Lol, oops, I'll remove it.
| id: '1', | ||
| type: CASE_COMMENT_SAVED_OBJECT, | ||
| attributes: { | ||
| comment: 'Wow, good luck catching that bad meanie!', |
There was a problem hiding this comment.
Is funny how this follows us after 3 years 😆.
There was a problem hiding this comment.
Haha, good ol' copy paste!
| }); | ||
|
|
||
| await expect(attachmentGetter.bulkGet(['1'])).rejects.toThrowErrorMatchingInlineSnapshot( | ||
| `"Invalid value \\"undefined\\" supplied to \\"comment\\",Invalid value \\"user\\" supplied to \\"type\\",Invalid value \\"undefined\\" supplied to \\"alertId\\",Invalid value \\"undefined\\" supplied to \\"index\\",Invalid value \\"undefined\\" supplied to \\"rule\\",Invalid value \\"undefined\\" supplied to \\"actions\\",Invalid value \\"undefined\\" supplied to \\"externalReferenceAttachmentTypeId\\",Invalid value \\"undefined\\" supplied to \\"externalReferenceMetadata\\",Invalid value \\"undefined\\" supplied to \\"externalReferenceId\\",Invalid value \\"undefined\\" supplied to \\"externalReferenceStorage\\",Invalid value \\"undefined\\" supplied to \\"persistableStateAttachmentTypeId\\",Invalid value \\"undefined\\" supplied to \\"persistableStateAttachmentState\\""` |
There was a problem hiding this comment.
I would expect a message only about the comment. Is it because io-ts tries to find a matching definion on the other attachment types?
There was a problem hiding this comment.
Yeah that's what I thought originally too. Yeah I think the issue is that io-ts doesn't know what type to use from the union because it doesn't match any of them.
💚 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: |
This PR adds io-ts decode calls to the service layer of the attachment functionality. This ensures that the results from elasticsearch do not violate the schema.
For bulk operations, error saved objects are not decoded.