(NEEDS DISCUSSION) Contract for isMemberOf and isSubGroupOf searches off /api/eperson/groups

[tdonohue]

Related to DSpace/DSpace#9052

While investigating performance problems described in DSpace/DSpace#9052, I've realized that our Angular UI includes code which requests every EPerson in a Group or every Subgroup in a Group in order to determine whether a given EPerson or Group is already included in a given parent Group.

For example:

• isMemberOfGroup in members-list.component.ts: https://github.com/DSpace/dspace-angular/blob/main/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts#L216C1-L233C3
• isSubgroupOfGroup in subgroups-list.component.ts: https://github.com/DSpace/dspace-angular/blob/main/src/app/access-control/group-registry/group-form/subgroup-list/subgroups-list.component.ts#L138-L159

This UI behavior will cause performance issues when a Group has a large number of members (either EPerson or Subgroup).
These queries will have much better performance if moved to the backend via isMemberOf and isSubGroupOf endpoints.

Therefore, this PR suggestions two new REST API endpoints (to replace the above UI methods):

1. GET /api/eperson/groups/search/isMemberOf?group=<:name>
   • Implementation would likely use a slightly updated version of the GroupService.isMember() method: https://github.com/DSpace/DSpace/blob/main/dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java#L311-L313
   • Strangely, we already have (unused) code in dspace-angular which attempts to use this /search/isMemberOf endpoint. https://github.com/DSpace/dspace-angular/blob/main/src/app/core/eperson/group-data.service.ts#L114-L124
2. GET /api/eperson/groups/search/isSubGroupOf?group=<:name>&subgroup=<:name>
   • Implementation would likely use a new GroupService.isMemberGroup() method, modeled similar to isMember() above.

Because this is a performance fix & is backwards compatible, I'd recommend we consider this change for 7.6.1. The code to implement these endpoints would be rather simple (see implementation notes above)

[tdonohue]

@abollini and/or @benbosman , I'd appreciate your thoughts on this. This adds two new /api/eperson/groups/search endpoints, but is arguably "backwards compatible", so I'd like to recommend it for inclusion in a bug-fix release (7.6.1).

Without these changes (or some sort of similar REST API endpoints), it's not possible to fully fix major performance issues described in DSpace/DSpace#9052 (see description above for more details).

The implementation here is rather simple as the necessary methods mostly exist in our Java API. So, the question is really whether we can allow this sort of change in a bug-fix release (7.6.1) or if there are major concerns with that direction.

(Feedback on the suggested endpoints is also welcome)

[mwoodiupui]

My first reaction is that the unpatched design requires the front end to know too much about the back-end implementation, so these new abstractions are going in the right direction. I would suggest a little further thought on "what does the front-end want to know and what does it have to know only to figure that out?"

[tdonohue]

After some more thought, I feel this REST API design is flawed because the UI design of the "Edit Group" page itself is problematic.

The current UI design (for "Edit Group") allows for the ability to search across all EPerson objects, and then one-by-one determines whether that EPerson is in the Group already. If so, a "delete" icon is added next to it. If not, an "add" icon is added next to it. (See circled section in screenshot) The one-by-one determination of group membership is problematic as it doesn't scale. If you list 20 results on that page, then it requires 20 additional requests to an "isMemberOf" endpoint to determine which of those 20 are already members of the group.

I think this design needs to possibly be changed to a tab-based approach similar to how the "Item Mapper" works. In the design of the "Item Mapper" the existing "members" (existing "mapped items") are on a separate tab from the option to add "new mapped items". See this screenshot:

This tabbed design means we no longer would need to do a one-by-one determination of which objects are already "members". The members list is on a separate tab from the list of non-members, which means each can be found separately in a single query which provides better performance.

If we went with this new design, we'd no longer need an isMemberOf or isSubGroupOf search. Instead, though, we may need new endpoints like this:

• Search across EPerson objects not already in this Group: /api/eperson/epersons/search/isNotMemberOf?group=[:uuid]&query=[search terms]
• Search across Group objects not already in this Group: /api/eperson/groups/search/isNotMemberOf?group=[:uuid]&query=[search terms]

This design would be much more scalable, but would require redesign of the "Edit Group" UI (/access-control/groups/[:uuid]) to provide tabs for "Current Members" vs "Add New Members" (for both EPerson and SubGroup sections).

[tdonohue]

Closing as "won't fix". This PR includes an old design which isn't correct. I'm going to replace this with the design described in my prior comment: #237 (comment) (which others agreed with in today's DevMtg)