[Cases] Split attachment types for versioned http apis #155440
Conversation
jonathan-buttner
added
release_note:skip
| index?: string | string[]; | ||
| rule?: Record<string, unknown>; | ||
| comment?: string; | ||
| actions?: Record<string, unknown>; |
There was a problem hiding this comment.
Should the child fields of this field be represented as well? It doesn't seem to affect type errors.
There was a problem hiding this comment.
I think we should be more explicit on this and define them. Same for the rule.
| result = result.concat( | ||
| // We need a cast here because to limited attachment type conflicts with the expected result even though they | ||
| // should be the same | ||
| userActionSavedObject.saved_objects as unknown as Array<SavedObject<AttributesTypeAlerts>> |
There was a problem hiding this comment.
Just calling this out. Do you see a better way around this? I believe we skip transforming this because alerts don't have fields that need to be migrated (the rule.id should have been but we had that bugfix).
There was a problem hiding this comment.
Yes, I cannot think of another way of doing it.
jonathan-buttner
changed the base branch from
main
to
cases-split-types-feature
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
Pinging @elastic/response-ops-cases (Feature:Cases) |
| type: string; | ||
| soType?: string; | ||
| }; | ||
| persistableStateAttachmentState?: Record<string, unknown>; |
There was a problem hiding this comment.
I think the persistableStateAttachmentTypeId is missing.
There was a problem hiding this comment.
Oops, yeah I was getting confused between that and the externalId 👍 I'll add it back.
| @@ -258,7 +259,7 @@ export const isCommentRequestTypeAlert = ( | |||
| * A type narrowing function for external reference so attachments. | |||
| */ | |||
| export const isCommentRequestTypeExternalReferenceSO = ( | |||
| context: Partial<CommentRequest> | |||
| context: Partial<AttachmentRequestAttributes> | |||
There was a problem hiding this comment.
Shouldn't we do the same for the other utility functions? Even if we don't get TS errors now it will protects us from mistakes in the future.
There was a problem hiding this comment.
I was a little unsure about these functions because most of them are used in the clients and not the service layer. Maybe we should eventually move the isCommentRequestTypeExternalReferenceSO to the service layer since that's where it is used.
There was a problem hiding this comment.
Oh ok. If they are used in the client's layer then it is fine. Moving isCommentRequestTypeExternalReferenceSO sounds good to me.
| @@ -207,7 +173,7 @@ export class AttachmentService { | |||
| ); | |||
|
|
|||
| const attachment = | |||
| await this.context.unsecuredSavedObjectsClient.create<AttachmentAttributesWithoutRefs>( | |||
| await this.context.unsecuredSavedObjectsClient.create<AttachmentPersistedAttributes>( | |||
There was a problem hiding this comment.
What about bulkDelete, update, and bulkUpdate?
There was a problem hiding this comment.
🤦♂️ don't know why I missed all that.
There was a problem hiding this comment.
Looks like bulkDelete doesn't take a type. I'll update the others though.
| @@ -59,7 +50,7 @@ export class AttachmentGetter { | |||
| ); | |||
|
|
|||
| const response = | |||
| await this.context.unsecuredSavedObjectsClient.bulkGet<AttachmentAttributesWithoutRefs>( | |||
| await this.context.unsecuredSavedObjectsClient.bulkGet<AttachmentPersistedAttributes>( | |||
There was a problem hiding this comment.
Same here, I think we should add the new type to all SO methods of the AttachmentGetter class.
There was a problem hiding this comment.
I think the ones I didn't add it to here are intentionally wanting unknown. Let me know if I missed any though.
There was a problem hiding this comment.
Can we define the return type of all inject*/extract* functions like the transform functions in the Case service? I think it will help us avoid mistakes in the future. Also, the injectAttachmentSOAttributesFromRefsForPatch accepts a CommentPatchAttributes which I think it should be AttachmentRequestAttributes.
There was a problem hiding this comment.
Can we define the return type of all inject*/extract* functions like the transform functions in the Case service? I think it will help us avoid mistakes in the future.
Yep will do!
Also, the injectAttachmentSOAttributesFromRefsForPatch accepts a CommentPatchAttributes which I think it should be AttachmentRequestAttributes
I think that's the field that represents the original patch request not the results from the saved object update. We use that to determine if we are deleting a reference I think. I think it should stay as CommentPatchAttributes.
I did update the saved object response though
… into cases-split-comments
💔 Build FailedFailed CI StepsTest Failures
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
This PR separates the persisted SO attributes from the fields received in the HTTP API requests. This is to address step 2 in preparing our HTTP routes as versioned. Issue: #153726 This PR encompasses a few PRs here which have been reviewed individually by the cases team members: #155325 - User actions #155440 - Attachments #155444 - Configure, Connector Mappings #155277 - Cases The large number of files is because of renaming some types throughout the frontend code. There shouldn't be many functionality changes, mostly just type changes. --------- Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co> Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
This PR separates the http API io-ts types from the types that are used in the cases service layer to interact with the saved object client. This PR is specifically for the attachments it only affects the types used when interacting with the saved object client and doesn't touch the transformation logic yet.
Issue: #153726