DSpace / RestContract

REST Contract for DSpace 7-8
https://wiki.lyrasis.org/display/DSDOC8x/
37 stars 48 forks source link

REST Contract for the delete multiple bitstreams endpoint #203

Closed Raf-atmire closed 1 year ago

Raf-atmire commented 1 year ago

Related to https://github.com/DSpace/DSpace/pull/8485

This is the REST Contract that'll allow the REST api to delete multiple bitstreams with a single request

abollini commented 1 year ago

I agree with @tdonohue we should avoid to use a request body in a DELETE request. My preference goes to the use of a POST request that indeed will create a bulk operation that can be run sync or async. In the sync scenario the response will give details about the result of the operation (it doesn't need to be atomic if we wish so).

benbosman commented 1 year ago

I agree that the contract was indeed not RESTful I've updated it now, I was considering either a PATCH on /bundles/<:uuid>/bitstreams, or on the main /bitstreams endpoint. The former has the advantage that the endpoint can't be abused to batch delete too many bitstreams (but that can also be checked by defining a configurable limit in REST). But it has the disadvantage that it would complicate the UI when the user wants to delete bitstreams across multiple bundles in the item So I did consider the latter to be best, but I'd recommend limiting the amount of bitstreams to be deleted at once

benbosman commented 1 year ago

@tdonohue This is a client contribution We have approval from the client for the simpler implementation based on the /bitstreams location, but not for the implementation based on the /bundles/{uuid} endpoint. The latter would also have a larger UI impact

I need to schedule this for next week to ensure we can get this completed by the deadline. We're planning to schedule this using the /bitstreams location so the developers can complete this work (it also needs Angular work, but that will be limited using this implementation)

tdonohue commented 1 year ago

@benbosman : I'll admit, my assumption was that the /bundles/{uuid} endpoint approach would be easier because the UI already uses that endpoint with PATCH to reorder bitstreams.

But, if that's not the case, I'm OK with using /bitstreams for this PATCH providing bulk delete. I can also see the argument here that having bulk delete on /bitstreams is more appropriate, since single deletions also occur off that endpoint (DELETE /bitstreams/[uuid]).

So, overall, I think I've come to the conclusion that either approach is OK. We should just choose one. So, I'm OK with your proceeding with PATCH /bitstreams using "remove" actions.

tdonohue commented 1 year ago

@Raf-atmire and @benbosman : I agree with @abollini that it'd be good to document this decision in the main README.md in the "On Collection of Resources Endpoints" section. It could say something like this:

* `PATCH` - Allows for batch updates (e.g. batch deletion via Patch 'remove' operation) to several resources in the collection at once.  The request body must adhere to the the [JSON Patch specification RFC6902](https://tools.ietf.org/html/rfc6902).
See [General rules for the Patch operation](patch.md) for more details.

I'm also +1 this RestContract as soon as we remove the "atmire.com" URL (see my note above).