-
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] Registering file attachment type #152399
[Cases] Registering file attachment type #152399
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
@@ -104,6 +105,8 @@ export class CasePlugin { | |||
)}] and plugins [${Object.keys(plugins)}]` | |||
); | |||
|
|||
registerInternalAttachments(this.externalReferenceAttachmentTypeRegistry); |
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.
If this is an "Internal attachment type" that should always exist maybe it would make more sense to register it when externalReferenceAttachmentTypeRegistry
is initialized and not when the cases plugin is set up?
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.
The externalReferenceAttachmentTypeRegistry
should not be considered as a singleton. It can be instantiated in various places where a clean state is needed and can lead to bugs if not. It will also make the testing much harder if the state of the class contains our internal attachments.
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 I'm open to either keeping the registration in the plugin or making it a part of the ExternalReferenceAttachmentTypeRegistry
. If it makes testing easier to do registration in the plugin that's fine with me.
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 will also make the testing much harder if the state of the class contains our internal attachments.
But if I understand it correctly the class is "ours" so wherever it is used it will need those internal attachments.
My reasoning was that it is why more error-prone to have to remember to register internal types every time we instantiate it. External ones though are fine because those are the responsibility of the "instantiator".
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.
My reasoning was that it is why more error-prone to have to remember to register internal types every time we instantiate it
I see what you mean. This should never happen. This class is instantiated only once in the setup
lifecycle method of the cases plugin. The same instance is passed down to the cases code and lives throughout the whole lifecycle of the cases plugin (This can be a counterargument to my main point 🙂). If you need a new instance of the class you probably need it for testing or to do something else unrelated to the internal attachments.
I see the cases plugin as an external consumer of the externalReferenceAttachmentTypeRegistry
and not as an internal consumer. By registering data in the constructor of the externalReferenceAttachmentTypeRegistry
you tight couple the data with the logic of the class (representation). The class is now aware of the data which are implementation detail. The class has more concerns than it should have.
x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts
Outdated
Show resolved
Hide resolved
...t/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts
Outdated
Show resolved
Hide resolved
...t/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts
Outdated
Show resolved
Hide resolved
x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts
Outdated
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.
lgtm, just some minor comments 👍
…ibana into cases-file-attachment
💚 Build Succeeded
Metrics [docs]Module Count
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.
Code LGMT! Tested without issues 🚀
This PR registers the file attachment type `.files` within the cases attachment framework. Notable changes: - Attachment framework accepts an optional validation function for use when a request is received for that attachment type, this allows validating that the file metadata is correct - Refactored the logic for enforcing that only 1000 alerts can be attached to a case and 100 files Issue: elastic#151933 <details><summary>cUrl request to create file attachments</summary> ``` curl --location --request POST 'http://elastic:changeme@localhost:5601/internal/cases/<case id>/attachments/_bulk_create' \ --header 'kbn-xsrf: hello' \ --header 'Content-Type: application/json' \ --data-raw '[ { "type": "externalReference", "externalReferenceStorage": { "type": "savedObject", "soType": "file" }, "externalReferenceId": "my-id", "externalReferenceAttachmentTypeId": ".files", "externalReferenceMetadata": { "file": { "name": "test_file", "extension": "png", "mimeType": "image/png", "createdAt": "2023-02-27T20:26:54.345Z" } }, "owner": "cases" }, { "type": "externalReference", "externalReferenceStorage": { "type": "savedObject", "soType": "file" }, "externalReferenceId": "my-id", "externalReferenceAttachmentTypeId": ".files", "externalReferenceMetadata": { "file": { "name": "test_file", "extension": "png", "mimeType": "image/png", "createdAt": "2023-02-27T20:26:54.345Z" } }, "owner": "cases" }, { "type": "externalReference", "externalReferenceStorage": { "type": "savedObject", "soType": "file" }, "externalReferenceId": "my-id", "externalReferenceAttachmentTypeId": ".files", "externalReferenceMetadata": { "file": { "name": "test_file", "extension": "jpeg", "mimeType": "image/jpeg", "createdAt": "2023-02-27T20:26:54.345Z" } }, "owner": "cases" } ] ' ``` </details>
This PR registers the file attachment type
.files
within the cases attachment framework.Notable changes:
Issue: #151933
cUrl request to create file attachments