Mapping collection

[tdonohue]

Replacement for #35 (@benbosman and @KevinVdV) and #44 (@abollini).

This PR includes alterations to the items.md contract to add for mapping of items to collections.
JIRA: https://jira.duraspace.org/browse/DS-4097

I've attempted to bring in feedback from both prior PRs. Here's what I've done:

• Started from Mapping collection #35
• Changed all "internal" atmire.com URLs to same base URL we use elsewhere. Yes, these links won't resolve, but that's fine. Likely we should replace all URLs in the future, and we just want consistency for now
• Added PUT definition from Provide contract for item to collections mapping #44 (was missing from Mapping collection #35).

BEFORE WE CAN MERGE THIS
One outstanding question remains...we have a discrepancy on the DELETE method format:

• Mapping collection #35 expects DELETE to use "text/uri-list" (like PUT and POST)
• Provide contract for item to collections mapping #44 expects DELETE to use items/[:uuid]/mappedCollections/[:uuid_of_collection_to_remove]

In the Spring Data REST community, they seem to recommend using PUT for deletion (by calling PUT with an empty body). See: https://stackoverflow.com/a/28713232/3750035 Though, there's also an answer more like #44 here: https://stackoverflow.com/a/40785090/3750035

For ease of parsing all these endpoints the same way, I wonder if we should simply use PUT for deletion.

[benbosman]

Thanks @tdonohue for merging the input from #35 and #44 into one PR.

I only see one minor thing (also from your question), to remove the PUT since this is not planned at the moment. If it's ok to have potential future contracts in here which are not planned, this can remain here, but I would still at least mention it's not planned

The use cases we eventually want to support are:

• Mapping items to an additional collection
• Removing items from a specific collection
  If PUT would be used for this, the UI would need to GET all mapped collections, remove the one which is not desired, and PUT the updated list, which adds burden to the UI

Support for POST and DELETE with multiple collections is also not necessary in the current planned functionality, but there's little additional work in the API to support multiple collections instead of just one. So I would leave that as is

[benbosman]

At the moment there's no use case for a PUT, so that's not planned to be implemented
To avoid any confusion, I assume it's best to leave it out here

[abollini]

I will like to have an uniform behavior across all our endpoints as much as possible. In the general section
https://github.com/DSpace/Rest7Contract#on-sub-path-of-a-single-resource-endpoint-associations
we state that PUT is supported, and I still convinced that this is the way to go. If we don't have plan/resources to do that for DSpace7 for this specific endpoint I will prefer to have here mentioned the "exception" (i.e. PUT ... (not supported yet))

[abollini]

I feel more natural to issue a delete to empty a collection instead than replace it with an empty list (PUT). This is also inline with the use of the DELETE on a single association endpoint (i.e. to delete a logo of a collection). Unfortunately the spring data rest community has not yet set a definitive answer as their documentation still talk only about delete (that is what we have copied in our "front page REST documentation" but the implementation in the case of * to many association rely on PUT as noted in the links that you provide).

Also the classic newbie website for REST propose the use of DELETE to empty a collection or remove a single association from a collection see
https://www.restapitutorial.com/lessons/httpmethods.html
https://restfulapi.net/http-methods/#delete

[abollini]

I have checked the documentation of other association endpoint and we don't include an example most of time as it is very verbose. The only one that I found is
https://github.com/DSpace/Rest7Contract/blob/master/collections.md#default-access-conditions

that said, if we want to keep the example we should move the mappedCollections inside a _embedded attribute

[abollini]

#39 has been merged so we should use the new syntax here as well

[abollini]

I will like to have an uniform behavior across all our endpoints as much as possible. In the general section
https://github.com/DSpace/Rest7Contract#on-sub-path-of-a-single-resource-endpoint-associations
we state that PUT is supported, and I still convinced that this is the way to go. If we don't have plan/resources to do that for DSpace7 for this specific endpoint I will prefer to have here mentioned the "exception" (i.e. PUT ... (not supported yet))

[abollini]

there are lot of discussion in stack overflow about the possibility or less to include a request body in the DELETE method
https://stackoverflow.com/questions/299628/is-an-entity-body-allowed-for-an-http-delete-request
The result of such discussions generally agree that it is formally possible (not denied by the spec) but it looks confusing to many people and at the best it is not standardised the semantic/purpose of the body and in some case there are suggestions to just ignore it

Personally I see a major issue with this use of the DELETE method here. Looking to the http spec

The DELETE method requests that the origin server delete the resource identified by the Request-URI.

in our case we are saying that what need to be deleted is not identified by the Request-URI

[tdonohue]

@benbosman and @abollini : I've updated this REST Contract PR based on your feedback:

• Per feedback from @benbosman: I've noted that PUT is not supported at this time, but left in the section (to remain consistent with other areas of the Contract)
• Per feedback from @abollini : I've removed the detailed example GET response. Attempting to keep these detailed responses up-to-date will be very difficult until the Contract is closer to being finalized. If we decide we want detailed response examples for all GET requests, we can add them in later.
• Per feedback from @abollini : I've updated the DELETE request format to pass the UUID of the Mapped Collection in the request URI. So, the format is now DELETE /api/core/items/<item:uuid>/mappedCollections/<collection:uuid>

Please re-review and let me know what you think!

[abollini]

thanks @tdonohue, it looks good to me now

[benbosman]

Thanks for the updates @tdonohue

I think this would work, but I'm a bit worried about how clear this DELETE syntax is.
We currently already have two solutions to identify the related collection, using the uri-list or a parameter. Having a third solution using DELETE /api/core/items/<item:uuid>/mappedCollections/<collection:uuid> will complicate the REST contracts again. Also this is a URI which doesn't support a GET request, which may also make it less clear what the expected behavior is.

I think that if this third syntax is required, it should be well documented.

[tdonohue]

@benbosman : While I understand your concerns about the DELETE syntax, I think we're somewhat forced into this syntax, not only by Spring Data REST best practices, but also via general REST best practices. If you look at the StackOverflow article that @abollini referenced in his prior comments, there are a few key lines I'd like to stress in the answer:

If a DELETE request includes an entity body, the body is ignored [...]

And, in a comment below that:

Please note that a lot of clients are also unable to send a DELETE with a body. This just burned me on Android.

So, it seems that this is not a question of "what does Spring do", but really a question of "what do REST Clients tend to support". So, in my opinion, we're going to be forced into supporting DELETE requests that do not include information in the request body...but, I agree we'll need to clearly document this in the REST Contract.

[benbosman]

@tdonohue I would prefer to mention in this REST Contract still that GET /api/core/items/<item:uuid>/mappedCollections/<collection:uuid> is not supported, only DELETE /api/core/items/<item:uuid>/mappedCollections/<collection:uuid> and GET /api/core/collections/<collection:uuid>

If that's mentioned in here, I think this REST Contract is clear enough, and only the Readme file should be updated to explain these use cases.

[tdonohue]

@benbosman : If you could give this another review, I'd appreciate it. I've updated this REST Contract with the following:

• A note that GET /api/core/items/<:uuid>/mappedCollections/<:collection_uuid> is UNSUPPORTED
• A note that DELETE /api/core/items/<:uuid>/mappedCollections is also UNSUPPORTED
• Modified README.md to note that DELETE cannot include a request body, and added examples for how to implement DELETE for associations (like mappedCollections)
• Minor formatting cleanup

[benbosman]

Thanks @tdonohue
For me, this is clearly documented now, and we can use these examples and guidelines in other rest contracts where relations should be deleted as well.

[tdonohue]

Thanks @benbosman and @abollini! Merging this as it's now at +2