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

Group and eperson management #41

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@benbosman
Copy link
Member

commented Dec 19, 2018

  • Created a Rest Contract for EPerson Groups (some of the API behavior already exists in the API)
  • Updated the Rest Contract for EPersons (some of the API behavior already exists in the API)
  • Created a Rest Contract for adding child Groups to parent groups (and deleting, querying)
  • Created a Rest Contract for adding EPersons to parent groups (and deleting, querying)

benbosman added some commits Dec 18, 2018

@abollini
Copy link
Member

left a comment

Thanks to provide the contract also for existing implementation this will help to reduce our documentation gap. Please review the contract for the mapping according to the practice of Spring Data Rest using the text-uri/list format and the association links (some need to be introduced as stated in the comment)

"href": "https://dspace7-internal.atmire.com/rest/api/eperson/groups/617cf46b-535c-42d5-9d22-327ce2eff6dc/groups"
},
"self": {
"href": "https://dspace7-internal.atmire.com/rest/api/eperson/groups/617cf46b-535c-42d5-9d22-327ce2eff6dc"

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

Please use the official REST & UI demo web sites for examples. This will allow us to easily replace the reference to dspace.org if needed


```json
{
"name": "Administrator",

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

I suggest to remove the name attribute from the example as it is a "read-only" attribute derived from the dc.title

"metadata": [
{
"key": "dc.title",
"value": "Administrator"

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

I suggest to use a "My Group" or other generic name instead one close to the special "Administrators"

epersons.md Outdated
"type": "eperson",
"_links": {
"self": {
"href": "https://dspace7-internal.atmire.com/rest/api/eperson/epersons/028dcbb8-0da2-4122-a0ea-254be49ca107"

This comment has been minimized.

@@ -0,0 +1,98 @@
# Group 2 EPerson mapping Endpoints

This comment has been minimized.

Copy link
@abollini

abollini Dec 31, 2018

Member

we don't need a new endpoint for that. We should use an association endpoint inside the groups and epersons endpoints, we can eventually decide to implement the mapping only from one side (in such case my preference goes to the groups endpoint as here we will have in any case the "subgroups").

For example to add a subgroup to a group a text-uri/list POST request should be sent to /api/eperson/groups/<:uuid>/groups

to add an eperson we should introduce a new link epersons in the groups endpoint and sent a POST request to /api/eperson/groups/<:uuid>/epersons
with the request body to be the URI of the subgroup or eperson (i.e. https://dspace7.4science.it/dspace-spring-rest/api/eperson/groups/<:subgroup-uuid> or https://dspace7.4science.it/dspace-spring-rest/api/eperson/epersons/<:eperson-uuid>

A PUT request can be used to fully override the mapping if needed. See #44 for an example

"totalPages": 1,
"totalElements": 0
}
}

This comment has been minimized.

Copy link
@AlexanderS

AlexanderS Jan 29, 2019

I would expect something like epersons or members here.

epersons.md Outdated
"email": "user@institution.edu",
"requireCertificate": false,
"selfRegistered": true,
"groups": null,

This comment has been minimized.

Copy link
@AlexanderS

AlexanderS Jan 29, 2019

Is this a list of group names or a href-list?

@tdonohue
Copy link
Member

left a comment

@benbosman : I gave this a review today. Sorry for the questions, but there are a few things I don't understand. If you and @abollini have previously discussed any of these points & come to a different decision, feel free to let me know. I'm not trying to slow down this PR, but I did come away from the code review with many questions.

Show resolved Hide resolved epersongroups.md
**GET /api/eperson/groups/<:uuid>/groups**

Filter using parameter:
* all : retrieve indirect groups as well

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 8, 2019

Member

This filter is not well described. What is meant by an "indirect group"? Are you talking about crawling the entire group hierarchy of groups within groups within groups? What is the use case here?

To clarify, I'm not trying to be picky. I just don't understand what ?all=true is doing, and where it might be used in the Angular UI. I also wonder if this should instead be a type of /search method. For example, on the Communities endpoint, we don't have a "filter" for top-level communities or sub-communities. Instead, those are Search methods (e.g. /api/core/communities/search/top and /api/core/communities/search/subCommunities?parent=<:uuid>): https://github.com/DSpace/Rest7Contract/blob/master/communities.md

This comment has been minimized.

Copy link
@abollini

abollini Feb 10, 2019

Member

I share the same doubt than @tdonohue
The "all" filter should be replaced with a search method or a "projection" once implemented to declare with depth of embedding you want in the response

This comment has been minimized.

Copy link
@benbosman

benbosman Feb 12, 2019

Author Member

When comparing it with /api/core/communities/search/subCommunities?parent=<:uuid>, doesn't that imply /api/eperson/groups/<:uuid>/groups without the parameter would also become a search?

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 12, 2019

Member

@benbosman : I'm less concerned with the /api/eperson/groups/<:uuid>/groups as there's a clear "membership" between the parent group and the sub group. However, if you wanted to refactor that into a /search endpoint, I'm OK with that. It'd likely end up being /api/eperson/groups/search/subGroups?parent=<:uuid> then.

My question about the purpose of ?all=true still exists though. I don't understand what that param is doing and how it is expected to be used in the Angular UI. So, it's difficult for me to recommend exactly how we might rethink this endpoint.

This comment has been minimized.

Copy link
@benbosman

benbosman Feb 13, 2019

Author Member

@tdonohue It seems I lost the primary use case for the ?all=true in the refactoring at c614a94
The previous REST Contract contained an option to get all the groups of an EPerson, e.g. similar to what is included on the profile page. Having the ability to see all groups an EPerson belongs to is frequently useful. I'll need to recreate the Contract to get the groups of an EPerson.
I agree the ?all=true parameter would indeed make more sense as a search.

I would also like to know whether you see a substantial difference between a community and sub communities (implemented as a search) vs a group and sub groups (currently in the contract without a search). I just want to know whether there's a particular difference between both cases requiring them to be treated differently, or whether your opinion is that in both cases, both approaches would be good solutions and there's no need for them to be consistent.

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 13, 2019

Member

Hi @benbosman : since we are already talking about the need to have a /search method for the ?all=true use case, it seems reasonable to also implement the subgroup endpoint as /api/eperson/groups/search/subGroups?parent=<:uuid>.

As you noted, it also keeps this new endpoint consistent with the subcommunities endpoint.

To be honest, I had not noticed this inconsistency between subgroups and subcommunities until you pointed it out. But, I agree with you that it's probably best to align them.

This comment has been minimized.

Copy link
@abollini

abollini Feb 14, 2019

Member

we should have both. The main purpose of the /api/eperson/groups/:uuid/groups endpoint is to manage the association. We need to support DELETE, POST and optionally PUT to this endpoint to give the ability to add groups as subgroups.

Using the LinkRest annotation with a linkMethod we should be able to avoid code duplication and just use the same method for the search and for the link, see https://github.com/DSpace/DSpace/blob/master/dspace-spring-rest/src/main/java/org/dspace/app/rest/RestResourceController.java#L717

This comment has been minimized.

Copy link
@benbosman

benbosman Feb 21, 2019

Author Member

Implementing both a search and a direct get to retrieve subgroups doesn't make any sense either. The data should be retrievable using a single endpoint.

We should either:

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 27, 2019

Member

Looking back on this discussion, I'm having a hard time providing a direction/path forward.

I'd still like to understand the ?all=true filter better...If that is needed, then I think it's definitely a search (and not a filter -- a filter should limit results, and not expand the number of results).

So, at a minimum, I think we need a /api/eperson/groups/search/subGroups endpoint to support the concept of all=true. However, that does not require that we remove all GET support on /api/eperson/groups/:uuid/groups, instead we'd just need to document that this endpoint only returns subgroups that are directly members of the given group. We can then point folks at the /api/eperson/groups/search/subGroups endpoint if they want to perform a different type of search within groups.

While alignment with Communities/SubCommunities is "nice", I don't think it's 100% required here. Relationships between Communities & SubCommunities are quite different from relationships between Groups. Keep in mind, the Group <-> Group association is many-to-many, while Community <-> Community is one-to-many (a subcommunity never belongs to multiple parents, but a Community can have many subcommunities). So, we shouldn't be treating these concepts as 100% identical... general alignment is nice, but not required.


### Remove a Group from a parent Group

**DELETE /api/eperson/groups/<:parentgroupuuid>/groups**

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 8, 2019

Member

Similar to discussions in PR #52 , I think this DELETE request should be updated to be DELETE /api/core/groups/<:parent_group_uuid>/groups/<:sub_group_uuid>

This comment has been minimized.

Copy link
@benbosman

benbosman Feb 21, 2019

Author Member

This has been adjusted to be consistent with mapping collections

**GET /api/eperson/groups/<:uuid>/epersons**

Filter using parameter:
* all : retrieve indirect epeople as well

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 8, 2019

Member

Same with the all filter on /groups/<:uuid>/groups, I don't really understand the use case here, and I immediately wonder if this should be a /search endpoint.

@@ -2,10 +2,53 @@
[Back to the list of all defined endpoints](endpoints.md)

## Main Endpoint
**/api/eperson/epersons**
**GET /api/eperson/epersons**

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 8, 2019

Member

This is a really oddly named endpoint. Why isn't this called /api/core/epersons? @abollini is this really what we've chosen for the EPersons endpoint? It seems inconsistent with what we've documented here: https://github.com/DSpace/Rest7Contract/blob/master/endpoints.md

This comment has been minimized.

Copy link
@abollini

abollini Feb 10, 2019

Member

yes, the current code use eperson as category as we have reserved "core" for the entities more specific of the DSpace domain
https://github.com/DSpace/DSpace/blob/master/dspace-spring-rest/src/main/java/org/dspace/app/rest/model/EPersonRest.java#L25

No a strong opinion here, it was an early stage decision if we found a better way to structure such endpoints I'm absolutely open to discussion

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 12, 2019

Member

As EPerson is also a type of DSpaceObject, I don't understand the purpose to creating a separate category for EPerson objects. It results in very odd request URIs, in my opinion.

So, if we don't have a clear reason for having an "eperson" category, I feel we should refactor the code to drop it (in favor of "core"). That said, this decision isn't a fault of this PR, so we can do that refactor in a separate PR as needed.

**GET /api/eperson/groups/<:uuid>/groups**

Filter using parameter:
* all : retrieve indirect groups as well

This comment has been minimized.

Copy link
@abollini

abollini Feb 10, 2019

Member

I share the same doubt than @tdonohue
The "all" filter should be replaced with a search method or a "projection" once implemented to declare with depth of embedding you want in the response

* 204: if the update succeeded
* 401 Forbidden - if you are not authenticated
* 403 Unauthorized - if you are not logged in with sufficient permissions
* 422: if the specified group is not found

This comment has been minimized.

Copy link
@abollini

abollini Feb 10, 2019

Member

it could be useful to state 404 explicitly to discriminate between the case that the "parent" group is not found and the 422 child group is not found

What if the child cannot be added to the parent due to a cyclic reference (ie. A -> B -> C, -> mean contains we cannot add A to C)? 409 Conflict could be appropriate here. It will be ideal if we report to the client that A should be removed of B to be added to C

This comment has been minimized.

Copy link
@benbosman

benbosman Feb 12, 2019

Author Member

404 and 409 can indeed be useful
@abollini can you elaborate on how it should be further reported to the client that A should be removed of B to be added to C? Do you have an example for this

This comment has been minimized.

Copy link
@abollini

abollini Feb 14, 2019

Member

We can start with just reporting that as plain text, it is similar to what we do when an eperson cannot be deleted due to db constraints.
Something like: The group A cannot be added as it is a parent of group C via group B will be ideal but we can also just start with "The group cannot be added due to a cyclic reference"

BTW, looking for an example I found that EPerson delete should be treated in the same way, see for instance
https://github.com/DSpace/DSpace/blob/master/dspace-spring-rest/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java#L249

If we like to go with 409 I suggest to open a JIRA ticket to fix the above contract/implementation

the Item deletions are different
https://github.com/DSpace/DSpace/blob/master/dspace-spring-rest/src/main/java/org/dspace/app/rest/repository/ItemRestRepository.java#L196
https://github.com/DSpace/DSpace/blob/master/dspace-spring-rest/src/main/java/org/dspace/app/rest/repository/ItemRestRepository.java#L200

as the client cannot "solve" the conflicts (if it is a templateItem or an inprogress submission it needs to use a different endpoint, there will be no a second call to the delete after "solving").

This comment has been minimized.

Copy link
@benbosman

benbosman Feb 21, 2019

Author Member

I've included it in this contract.
For the others a JIRA ticket will indeed be required

* GET /rest/api/eperson/groups/3b1de75d-5bf9-4ca3-bf4d-42bc8abd0d42/epersons?all=true


```json

This comment has been minimized.

Copy link
@abollini

abollini Feb 10, 2019

Member

better avoid examples here as it will be difficult to keep all of them up-to-dated

Show resolved Hide resolved epersongroups.md
* 204: if the update succeeded
* 401 Forbidden - if you are not authenticated
* 403 Unauthorized - if you are not logged in with sufficient permissions
* 422: if the specified eperson is not found

This comment has been minimized.

Copy link
@abollini

abollini Feb 10, 2019

Member

this is ambiguous. I guess that we mean that the specified eperson is not found in the system, it doesn't exist at all. It can be misunderstood as the specified eperson is not found in the group. In this later case, I guess that 204 should be returned with a no-op

This comment has been minimized.

Copy link
@benbosman

benbosman Feb 21, 2019

Author Member

This has been been updated

@@ -2,10 +2,53 @@
[Back to the list of all defined endpoints](endpoints.md)

## Main Endpoint
**/api/eperson/epersons**
**GET /api/eperson/epersons**

This comment has been minimized.

Copy link
@abollini

abollini Feb 10, 2019

Member

yes, the current code use eperson as category as we have reserved "core" for the entities more specific of the DSpace domain
https://github.com/DSpace/DSpace/blob/master/dspace-spring-rest/src/main/java/org/dspace/app/rest/model/EPersonRest.java#L25

No a strong opinion here, it was an early stage decision if we found a better way to structure such endpoints I'm absolutely open to discussion

@cwilper

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Note that metadata-as-map has since been merged, so examples given in this PR should be updated to use that new structure, e.g. what I did here: #53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.