Skip to content
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

Mapping collection #35

Merged
merged 5 commits into from Feb 5, 2019
Merged

Mapping collection #35

merged 5 commits into from Feb 5, 2019

Conversation

KevinVdV
Copy link
Member

@KevinVdV KevinVdV commented Dec 6, 2018

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

Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be managed using the spring data rest approach for association endpoint
https://docs.spring.io/spring-data/rest/docs/current/reference/html/#repository-resources.association-

it will be useful to state something about the "inverse endpoint". Which is our plan? do we want to implement the item mapper functionality also on the collection side (i.e. /api/core/collections/:uuid/items)?

items.md Outdated
@@ -137,6 +137,172 @@ Example: <https://dspace7.4science.it/dspace-spring-rest/#https://dspace7.4scien

It returns the collection where the item belong to

### Mapping Collections
**GET /api/core/items/<:uuid>/mappingCollections**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an association. This should be included in the list of links returned by the single item endpoint.
It is important to clarify if the returned list include or not the owningCollection. I have no strong preference about it: probably excluding the owningCollection from the returned list could simplify some use cases on the client side but I guess it will add extra effort on the rest implementation or at least some "hack" on the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @abollini.

Also, I wonder if this should simply be called collections in the links on an Item...especially if we have it also include the owningCollection. I don't see a reason to actually use the term "mapping" or "mapped" in the REST API unless we really need to distinguish it from owningCollection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the "included in the list of links returned by the single item endpoint", are you implying the HAL link to mappingCollections as displayed in
https://dspace7-internal.atmire.com/rest/#/rest/api/core/items/a649a58a-6b5a-4c15-9151-093cd6841491
Would you just want us to also update the item Rest Contract to contain this HAL links in the sample JSON?

The mapping collections don't include the owning collection, we will mention that explicitly in the Rest Contract

The reason to be explicit about this being mapping collections is to also easily allow a UI to add/remove mapping collections for an item. The difference with the owning collection is important here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

items.md Outdated Show resolved Hide resolved
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Overall, this looks good now. I will however admit that I don't really like the term mappingCollections as "mapping" sounds like an action, not an object/resource. I'd prefer this endpoint be renamed to mappedCollections. But, if others are OK with this terminology, I'll let it go (I don't feel that strongly here).

Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created an alternative PR #44 based on the Spring Data Rest practice and the naming suggestion (mappedCollection) from @tdonohue

items.md Outdated
@@ -137,6 +137,172 @@ Example: <https://dspace7.4science.it/dspace-spring-rest/#https://dspace7.4scien

It returns the collection where the item belong to

### Mapping Collections
**GET /api/core/items/<:uuid>/mappingCollections**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

items.md Outdated Show resolved Hide resolved
items.md Show resolved Hide resolved
items.md Outdated Show resolved Hide resolved
@benbosman
Copy link
Member

Thanks @abollini for the feedback.
I've updated the contract apart from the change which is also discussed in
#34 (comment) (for the consistency reasons detailed there)
I would recommend to keep that discussion limited to PR34

items.md Outdated Show resolved Hide resolved
@abollini
Copy link
Member

abollini commented Feb 5, 2019

conflict on #44 has been solved. I suggest to merge it as by my understanding it reflects what we agree and don't require further work in fixing links

@tdonohue
Copy link
Member

tdonohue commented Feb 5, 2019

Merging, as this has been updated as requested and has approval from myself & @abollini .

@tdonohue tdonohue merged commit 8c4f513 into DSpace:master Feb 5, 2019
@tdonohue
Copy link
Member

tdonohue commented Feb 5, 2019

I've reverted the merger of this PR (see #51). My apologies, I merged the wrong PR. Will merge #44 instead.

@tdonohue tdonohue mentioned this pull request Feb 6, 2019
3 tasks
@tdonohue
Copy link
Member

tdonohue commented Feb 6, 2019

Disregard my previous comment. This (and #44) were replaced by #52. Please refer to #52 now.

@benbosman benbosman deleted the mapping-collection branch March 6, 2020 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants