[Saved Objects Management] Encapsulate saved objects deletion behind an API endpoint#148602
[Saved Objects Management] Encapsulate saved objects deletion behind an API endpoint#148602dokmic merged 8 commits intoelastic:mainfrom
Conversation
9ab4332 to
22899e3
Compare
|
Pinging @elastic/kibana-core (Team:Core) |
TinaHeiligers
left a comment
There was a problem hiding this comment.
Overall, great work!
We need to make sure we don't accidentally call bulk_delete for hidden saved objects. The API won't recognize those and we should give the impression that one can delete hidden objects from SOM.
src/plugins/saved_objects_management/server/routes/bulk_delete.ts
Outdated
Show resolved
Hide resolved
...ins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Adding a toast for every failed delete is going to get very noisy for large payloads. It's not a problem for single-item deletes but might cause toast floods for multiple delete failures.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Alright, I've updated the code to show a single toast only. But I have some concerns that it's better. It seems like we are hiding from the user all the errors we've got during the deletion.
IMO, it's okay to show a toast per each failed operation as we perform it only for the manually selected objects which should not be more than 50 in the worst case.
There was a problem hiding this comment.
which should not be more than 50 in the worst case
With anything related to saved objects, we cannot assume that. If there aren't any objections from other members of the team, go with what you 'feel' is right.
There was a problem hiding this comment.
The repository won't recognize hidden saved object types as those aren't recognized as registered unless the hidden types are included. We're not instantiating the client with any hidden types and will need to filter these out in a similar manner as:
https://github.com/elastic/kibana/pull/148602/files#diff-47d724c5a7bb6e8c1286f67e3ed1b117ac27e05387052760388bd477bd76337bL525
const deletes = selectedSavedObjects
.filter((object) => !object.meta.hiddenType)
.map((object) => savedObjectsClient.delete(object.type, object.id, { force: true }));
await Promise.all(deletes);
There was a problem hiding this comment.
Now that we have a dedicated endpoint, we could use a custom client with all hidden types. Not sure if we should, though.
There was a problem hiding this comment.
I implemented that at first but then removed it to keep the existing behavior exactly the same. Just wanted to confirm whether should we ever provide such an option as hidden objects deletion.
There was a problem hiding this comment.
Just wanted to confirm whether should we ever provide such an option as hidden objects deletion.
Right now, IMHO, no. There are far too many issues that could cause. Type owners specifically register their saved objects as hidden to implement their own APIs and handle dependencies themselves. I don't think we could or even should bypass that and blindly allow users to shoot themselves in the foot. Preventing issues from editing saved objects in SOM is why we changed the edit to view-only in the first place.
Maybe, if we had object-level security we could implement it or even some other "advanced, super scary uber auth" where we have a registry of hidden types' delete APIs to use as the underlying implementation?
There was a problem hiding this comment.
+1 to keep the behaviour as is. In general / long term, I would lean towards removing the delete ability for most types in management and moving that to their listing UIs which I think is also part of content management's vision.
...plugins/saved_objects_management/public/management_section/object_view/saved_object_view.tsx
Outdated
Show resolved
Hide resolved
src/plugins/saved_objects_management/public/lib/bulk_delete_objects.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Now that we have a dedicated endpoint, we could use a custom client with all hidden types. Not sure if we should, though.
src/plugins/saved_objects_management/public/lib/bulk_delete_objects.ts
Outdated
Show resolved
Hide resolved
...plugins/saved_objects_management/public/management_section/object_view/saved_object_view.tsx
Outdated
Show resolved
Hide resolved
...plugins/saved_objects_management/public/management_section/object_view/saved_object_view.tsx
Outdated
Show resolved
Hide resolved
src/plugins/saved_objects_management/server/routes/bulk_delete.ts
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7b579b5 to
607c3b6
Compare
|
@TinaHeiligers @pgayvallet I've updated the PR with some fixes based on your feedback. Would you mind taking another look? |
TinaHeiligers
left a comment
There was a problem hiding this comment.
I still need to pull the branch and test
8e2dd86 to
e8eb772
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
## Summary
Part of preparing HTTP APIs and associated interfaces for versioning:
* Add domain-specific interfaces to the saved object management plugin
* Add a V1 interface of domain types to `common`
* Remove use of deprecated `SavedObject` type from public
* Follows on from #148602
Related #149098
Fixes #149495
---------
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Resolves #148615.
The API endpoint delegates the delete operation to the
bulkDelete(#139680) method of the SO client on the server.Checklist