From 2c9165afb08126189ee3367347e7011f89227b7c Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Mon, 18 Sep 2023 10:58:18 -0500 Subject: [PATCH] Replace several usages of allMembers() with count methods to avoid performance issues --- .../dspace/eperson/EPersonServiceImpl.java | 7 +++++-- .../org/dspace/eperson/GroupServiceImpl.java | 19 ++++++++++++------- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java b/dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java index 5f17051dbb..ce117282de 100644 --- a/dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/eperson/EPersonServiceImpl.java @@ -305,10 +305,13 @@ public class EPersonServiceImpl extends DSpaceObjectServiceImpl impleme throw new AuthorizeException( "You must be an admin to delete an EPerson"); } + // Get all workflow-related groups that the current EPerson belongs to Set workFlowGroups = getAllWorkFlowGroups(context, ePerson); for (Group group: workFlowGroups) { - List ePeople = groupService.allMembers(context, group); - if (ePeople.size() == 1 && ePeople.contains(ePerson)) { + // Get total number of unique EPerson objs who are a member of this group (or subgroup) + int totalMembers = groupService.countAllMembers(context, group); + // If only one EPerson is a member, then we cannot delete the last member of this group. + if (totalMembers == 1) { throw new EmptyWorkflowGroupException(ePerson.getID(), group.getID()); } } diff --git a/dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java b/dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java index faf7b2b52a..d5d7ebcec1 100644 --- a/dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java @@ -179,8 +179,10 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl implements for (CollectionRole collectionRole : collectionRoles) { if (StringUtils.equals(collectionRole.getRoleId(), role.getId()) && claimedTask.getWorkflowItem().getCollection() == collectionRole.getCollection()) { - List ePeople = allMembers(context, group); - if (ePeople.size() == 1 && ePeople.contains(ePerson)) { + // Get total number of unique EPerson objs who are a member of this group (or subgroup) + int totalMembers = countAllMembers(context, group); + // If only one EPerson is a member, then we cannot delete the last member of this group. + if (totalMembers == 1) { throw new IllegalStateException( "Refused to remove user " + ePerson .getID() + " from workflow group because the group " + group @@ -191,8 +193,10 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl implements } } if (!poolTasks.isEmpty()) { - List ePeople = allMembers(context, group); - if (ePeople.size() == 1 && ePeople.contains(ePerson)) { + // Get total number of unique EPerson objs who are a member of this group (or subgroup) + int totalMembers = countAllMembers(context, group); + // If only one EPerson is a member, then we cannot delete the last member of this group. + if (totalMembers == 1) { throw new IllegalStateException( "Refused to remove user " + ePerson .getID() + " from workflow group because the group " + group @@ -212,9 +216,10 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl implements if (!collectionRoles.isEmpty()) { List poolTasks = poolTaskService.findByGroup(context, groupParent); if (!poolTasks.isEmpty()) { - List parentPeople = allMembers(context, groupParent); - List childPeople = allMembers(context, childGroup); - if (childPeople.containsAll(parentPeople)) { + // Count number of Groups which have this groupParent as a direct parent + int totalChildren = countByParent(context, groupParent); + // If only one group has this as a parent, we cannot delete the last child group + if (totalChildren == 1) { throw new IllegalStateException( "Refused to remove sub group " + childGroup .getID() + " from workflow group because the group " + groupParent