Merge pull request #9166 from DSpace/backport-9078-to-dspace-7_x

[Port dspace-7_x] Improve performance for Groups with many EPerson members. Fix pagination on endpoints
This commit is contained in:
Tim Donohue
2023-11-02 14:47:13 -05:00
committed by GitHub
14 changed files with 567 additions and 23 deletions

View File

@@ -305,10 +305,13 @@ public class EPersonServiceImpl extends DSpaceObjectServiceImpl<EPerson> impleme
throw new AuthorizeException( throw new AuthorizeException(
"You must be an admin to delete an EPerson"); "You must be an admin to delete an EPerson");
} }
// Get all workflow-related groups that the current EPerson belongs to
Set<Group> workFlowGroups = getAllWorkFlowGroups(context, ePerson); Set<Group> workFlowGroups = getAllWorkFlowGroups(context, ePerson);
for (Group group: workFlowGroups) { for (Group group: workFlowGroups) {
List<EPerson> ePeople = groupService.allMembers(context, group); // Get total number of unique EPerson objs who are a member of this group (or subgroup)
if (ePeople.size() == 1 && ePeople.contains(ePerson)) { 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()); throw new EmptyWorkflowGroupException(ePerson.getID(), group.getID());
} }
} }
@@ -567,14 +570,29 @@ public class EPersonServiceImpl extends DSpaceObjectServiceImpl<EPerson> impleme
@Override @Override
public List<EPerson> findByGroups(Context c, Set<Group> groups) throws SQLException { public List<EPerson> findByGroups(Context c, Set<Group> groups) throws SQLException {
return findByGroups(c, groups, -1, -1);
}
@Override
public List<EPerson> findByGroups(Context c, Set<Group> groups, int pageSize, int offset) throws SQLException {
//Make sure we at least have one group, if not don't even bother searching. //Make sure we at least have one group, if not don't even bother searching.
if (CollectionUtils.isNotEmpty(groups)) { if (CollectionUtils.isNotEmpty(groups)) {
return ePersonDAO.findByGroups(c, groups); return ePersonDAO.findByGroups(c, groups, pageSize, offset);
} else { } else {
return new ArrayList<>(); return new ArrayList<>();
} }
} }
@Override
public int countByGroups(Context c, Set<Group> groups) throws SQLException {
//Make sure we at least have one group, if not don't even bother counting.
if (CollectionUtils.isNotEmpty(groups)) {
return ePersonDAO.countByGroups(c, groups);
} else {
return 0;
}
}
@Override @Override
public List<EPerson> findEPeopleWithSubscription(Context context) throws SQLException { public List<EPerson> findEPeopleWithSubscription(Context context) throws SQLException {
return ePersonDAO.findAllSubscribers(context); return ePersonDAO.findAllSubscribers(context);

View File

@@ -98,7 +98,11 @@ public class Group extends DSpaceObject implements DSpaceObjectLegacySupport {
} }
/** /**
* Return EPerson members of a Group * Return EPerson members of a Group.
* <P>
* WARNING: This method may have bad performance for Groups with large numbers of EPerson members.
* Therefore, only use this when you need to access every EPerson member. Instead, consider using
* EPersonService.findByGroups() for a paginated list of EPersons.
* *
* @return list of EPersons * @return list of EPersons
*/ */
@@ -143,9 +147,13 @@ public class Group extends DSpaceObject implements DSpaceObjectLegacySupport {
} }
/** /**
* Return Group members of a Group. * Return Group members (i.e. direct subgroups) of a Group.
* <P>
* WARNING: This method may have bad performance for Groups with large numbers of Subgroups.
* Therefore, only use this when you need to access every Subgroup. Instead, consider using
* GroupService.findByParent() for a paginated list of Subgroups.
* *
* @return list of groups * @return list of subgroups
*/ */
public List<Group> getMemberGroups() { public List<Group> getMemberGroups() {
return groups; return groups;

View File

@@ -179,8 +179,13 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
for (CollectionRole collectionRole : collectionRoles) { for (CollectionRole collectionRole : collectionRoles) {
if (StringUtils.equals(collectionRole.getRoleId(), role.getId()) if (StringUtils.equals(collectionRole.getRoleId(), role.getId())
&& claimedTask.getWorkflowItem().getCollection() == collectionRole.getCollection()) { && claimedTask.getWorkflowItem().getCollection() == collectionRole.getCollection()) {
List<EPerson> ePeople = allMembers(context, group); // Count number of EPersons who are *direct* members of this group
if (ePeople.size() == 1 && ePeople.contains(ePerson)) { int totalDirectEPersons = ePersonService.countByGroups(context, Set.of(group));
// Count number of Groups which have this groupParent as a direct parent
int totalChildGroups = countByParent(context, group);
// If this group has only one direct EPerson and *zero* child groups, then we cannot delete the
// EPerson or we will leave this group empty.
if (totalDirectEPersons == 1 && totalChildGroups == 0) {
throw new IllegalStateException( throw new IllegalStateException(
"Refused to remove user " + ePerson "Refused to remove user " + ePerson
.getID() + " from workflow group because the group " + group .getID() + " from workflow group because the group " + group
@@ -191,8 +196,13 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
} }
} }
if (!poolTasks.isEmpty()) { if (!poolTasks.isEmpty()) {
List<EPerson> ePeople = allMembers(context, group); // Count number of EPersons who are *direct* members of this group
if (ePeople.size() == 1 && ePeople.contains(ePerson)) { int totalDirectEPersons = ePersonService.countByGroups(context, Set.of(group));
// Count number of Groups which have this groupParent as a direct parent
int totalChildGroups = countByParent(context, group);
// If this group has only one direct EPerson and *zero* child groups, then we cannot delete the
// EPerson or we will leave this group empty.
if (totalDirectEPersons == 1 && totalChildGroups == 0) {
throw new IllegalStateException( throw new IllegalStateException(
"Refused to remove user " + ePerson "Refused to remove user " + ePerson
.getID() + " from workflow group because the group " + group .getID() + " from workflow group because the group " + group
@@ -212,9 +222,13 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
if (!collectionRoles.isEmpty()) { if (!collectionRoles.isEmpty()) {
List<PoolTask> poolTasks = poolTaskService.findByGroup(context, groupParent); List<PoolTask> poolTasks = poolTaskService.findByGroup(context, groupParent);
if (!poolTasks.isEmpty()) { if (!poolTasks.isEmpty()) {
List<EPerson> parentPeople = allMembers(context, groupParent); // Count number of Groups which have this groupParent as a direct parent
List<EPerson> childPeople = allMembers(context, childGroup); int totalChildGroups = countByParent(context, groupParent);
if (childPeople.containsAll(parentPeople)) { // Count number of EPersons who are *direct* members of this group
int totalDirectEPersons = ePersonService.countByGroups(context, Set.of(groupParent));
// If this group has only one childGroup and *zero* direct EPersons, then we cannot delete the
// childGroup or we will leave this group empty.
if (totalChildGroups == 1 && totalDirectEPersons == 0) {
throw new IllegalStateException( throw new IllegalStateException(
"Refused to remove sub group " + childGroup "Refused to remove sub group " + childGroup
.getID() + " from workflow group because the group " + groupParent .getID() + " from workflow group because the group " + groupParent
@@ -368,7 +382,8 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
// Get all groups which are a member of this group // Get all groups which are a member of this group
List<Group2GroupCache> group2GroupCaches = group2GroupCacheDAO.findByParent(c, g); List<Group2GroupCache> group2GroupCaches = group2GroupCacheDAO.findByParent(c, g);
Set<Group> groups = new HashSet<>(); // Initialize HashSet based on List size to avoid Set resizing. See https://stackoverflow.com/a/21822273
Set<Group> groups = new HashSet<>((int) (group2GroupCaches.size() / 0.75 + 1));
for (Group2GroupCache group2GroupCache : group2GroupCaches) { for (Group2GroupCache group2GroupCache : group2GroupCaches) {
groups.add(group2GroupCache.getChild()); groups.add(group2GroupCache.getChild());
} }
@@ -381,6 +396,23 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
return new ArrayList<>(childGroupChildren); return new ArrayList<>(childGroupChildren);
} }
@Override
public int countAllMembers(Context context, Group group) throws SQLException {
// Get all groups which are a member of this group
List<Group2GroupCache> group2GroupCaches = group2GroupCacheDAO.findByParent(context, group);
// Initialize HashSet based on List size + current 'group' to avoid Set resizing.
// See https://stackoverflow.com/a/21822273
Set<Group> groups = new HashSet<>((int) ((group2GroupCaches.size() + 1) / 0.75 + 1));
for (Group2GroupCache group2GroupCache : group2GroupCaches) {
groups.add(group2GroupCache.getChild());
}
// Append current group as well
groups.add(group);
// Return total number of unique EPerson objects in any of these groups
return ePersonService.countByGroups(context, groups);
}
@Override @Override
public Group find(Context context, UUID id) throws SQLException { public Group find(Context context, UUID id) throws SQLException {
if (id == null) { if (id == null) {
@@ -829,4 +861,20 @@ public class GroupServiceImpl extends DSpaceObjectServiceImpl<Group> implements
public String getName(Group dso) { public String getName(Group dso) {
return dso.getName(); return dso.getName();
} }
@Override
public List<Group> findByParent(Context context, Group parent, int pageSize, int offset) throws SQLException {
if (parent == null) {
return null;
}
return groupDAO.findByParent(context, parent, pageSize, offset);
}
@Override
public int countByParent(Context context, Group parent) throws SQLException {
if (parent == null) {
return 0;
}
return groupDAO.countByParent(context, parent);
}
} }

View File

@@ -38,7 +38,29 @@ public interface EPersonDAO extends DSpaceObjectDAO<EPerson>, DSpaceObjectLegacy
public int searchResultCount(Context context, String query, List<MetadataField> queryFields) throws SQLException; public int searchResultCount(Context context, String query, List<MetadataField> queryFields) throws SQLException;
public List<EPerson> findByGroups(Context context, Set<Group> groups) throws SQLException; /**
* Find all EPersons who are a member of one or more of the listed groups in a paginated fashion. This returns
* EPersons ordered by UUID.
*
* @param context current Context
* @param groups Set of group(s) to check membership in
* @param pageSize number of EPerson objects to load at one time. Set to <=0 to disable pagination
* @param offset number of page to load (starting with 1). Set to <=0 to disable pagination
* @return List of all EPersons who are a member of one or more groups.
* @throws SQLException
*/
List<EPerson> findByGroups(Context context, Set<Group> groups, int pageSize, int offset) throws SQLException;
/**
* Count total number of EPersons who are a member of one or more of the listed groups. This provides the total
* number of results to expect from corresponding findByGroups() for pagination purposes.
*
* @param context current Context
* @param groups Set of group(s) to check membership in
* @return total number of (unique) EPersons who are a member of one or more groups.
* @throws SQLException
*/
int countByGroups(Context context, Set<Group> groups) throws SQLException;
public List<EPerson> findWithPasswordWithoutDigestAlgorithm(Context context) throws SQLException; public List<EPerson> findWithPasswordWithoutDigestAlgorithm(Context context) throws SQLException;

View File

@@ -146,4 +146,28 @@ public interface GroupDAO extends DSpaceObjectDAO<Group>, DSpaceObjectLegacySupp
*/ */
Group findByIdAndMembership(Context context, UUID id, EPerson ePerson) throws SQLException; Group findByIdAndMembership(Context context, UUID id, EPerson ePerson) throws SQLException;
/**
* Find all groups which are members of a given parent group.
* This provides the same behavior as group.getMemberGroups(), but in a paginated fashion.
*
* @param context The DSpace context
* @param parent Parent Group to search within
* @param pageSize how many results return
* @param offset the position of the first result to return
* @return Groups matching the query
* @throws SQLException if database error
*/
List<Group> findByParent(Context context, Group parent, int pageSize, int offset) throws SQLException;
/**
* Returns the number of groups which are members of a given parent group.
* This provides the same behavior as group.getMemberGroups().size(), but with better performance for large groups.
* This method may be used with findByParent() to perform pagination.
*
* @param context The DSpace context
* @param parent Parent Group to search within
* @return Number of Groups matching the query
* @throws SQLException if database error
*/
int countByParent(Context context, Group parent) throws SQLException;
} }

View File

@@ -112,12 +112,36 @@ public class EPersonDAOImpl extends AbstractHibernateDSODAO<EPerson> implements
} }
@Override @Override
public List<EPerson> findByGroups(Context context, Set<Group> groups) throws SQLException { public List<EPerson> findByGroups(Context context, Set<Group> groups, int pageSize, int offset)
throws SQLException {
Query query = createQuery(context, Query query = createQuery(context,
"SELECT DISTINCT e FROM EPerson e " + "SELECT DISTINCT e FROM EPerson e " +
"JOIN e.groups g " + "JOIN e.groups g " +
"WHERE g.id IN (:idList) "); "WHERE g.id IN (:idList) ");
List<UUID> idList = new ArrayList<>(groups.size());
for (Group group : groups) {
idList.add(group.getID());
}
query.setParameter("idList", idList);
if (pageSize > 0) {
query.setMaxResults(pageSize);
}
if (offset > 0) {
query.setFirstResult(offset);
}
return list(query);
}
@Override
public int countByGroups(Context context, Set<Group> groups) throws SQLException {
Query query = createQuery(context,
"SELECT count(DISTINCT e) FROM EPerson e " +
"JOIN e.groups g " +
"WHERE g.id IN (:idList) ");
List<UUID> idList = new ArrayList<>(groups.size()); List<UUID> idList = new ArrayList<>(groups.size());
for (Group group : groups) { for (Group group : groups) {
idList.add(group.getID()); idList.add(group.getID());
@@ -125,7 +149,7 @@ public class EPersonDAOImpl extends AbstractHibernateDSODAO<EPerson> implements
query.setParameter("idList", idList); query.setParameter("idList", idList);
return list(query); return count(query);
} }
@Override @Override

View File

@@ -196,4 +196,28 @@ public class GroupDAOImpl extends AbstractHibernateDSODAO<Group> implements Grou
return count(createQuery(context, "SELECT count(*) FROM Group")); return count(createQuery(context, "SELECT count(*) FROM Group"));
} }
@Override
public List<Group> findByParent(Context context, Group parent, int pageSize, int offset) throws SQLException {
Query query = createQuery(context,
"SELECT g FROM Group g JOIN g.parentGroups pg " +
"WHERE pg.id = :parent_id");
query.setParameter("parent_id", parent.getID());
if (pageSize > 0) {
query.setMaxResults(pageSize);
}
if (offset > 0) {
query.setFirstResult(offset);
}
query.setHint("org.hibernate.cacheable", Boolean.TRUE);
return list(query);
}
public int countByParent(Context context, Group parent) throws SQLException {
Query query = createQuery(context, "SELECT count(g) FROM Group g JOIN g.parentGroups pg " +
"WHERE pg.id = :parent_id");
query.setParameter("parent_id", parent.getID());
return count(query);
}
} }

View File

@@ -252,14 +252,42 @@ public interface EPersonService extends DSpaceObjectService<EPerson>, DSpaceObje
public List<String> getDeleteConstraints(Context context, EPerson ePerson) throws SQLException; public List<String> getDeleteConstraints(Context context, EPerson ePerson) throws SQLException;
/** /**
* Retrieve all accounts which belong to at least one of the specified groups. * Retrieve all EPerson accounts which belong to at least one of the specified groups.
* <P>
* WARNING: This method may have bad performance issues for Groups with a very large number of members,
* as it will load all member EPerson objects into memory.
* <P>
* For better performance, use the paginated version of this method.
* *
* @param c The relevant DSpace Context. * @param c The relevant DSpace Context.
* @param groups set of eperson groups * @param groups set of eperson groups
* @return a list of epeople * @return a list of epeople
* @throws SQLException An exception that provides information on a database access error or other errors. * @throws SQLException An exception that provides information on a database access error or other errors.
*/ */
public List<EPerson> findByGroups(Context c, Set<Group> groups) throws SQLException; List<EPerson> findByGroups(Context c, Set<Group> groups) throws SQLException;
/**
* Retrieve all EPerson accounts which belong to at least one of the specified groups, in a paginated fashion.
*
* @param c The relevant DSpace Context.
* @param groups Set of group(s) to check membership in
* @param pageSize number of EPerson objects to load at one time. Set to <=0 to disable pagination
* @param offset number of page to load (starting with 1). Set to <=0 to disable pagination
* @return a list of epeople
* @throws SQLException An exception that provides information on a database access error or other errors.
*/
List<EPerson> findByGroups(Context c, Set<Group> groups, int pageSize, int offset) throws SQLException;
/**
* Count all EPerson accounts which belong to at least one of the specified groups. This provides the total
* number of results to expect from corresponding findByGroups() for pagination purposes.
*
* @param c The relevant DSpace Context.
* @param groups Set of group(s) to check membership in
* @return total number of (unique) EPersons who are a member of one or more groups.
* @throws SQLException An exception that provides information on a database access error or other errors.
*/
int countByGroups(Context c, Set<Group> groups) throws SQLException;
/** /**
* Retrieve all accounts which are subscribed to receive information about new items. * Retrieve all accounts which are subscribed to receive information about new items.

View File

@@ -189,9 +189,11 @@ public interface GroupService extends DSpaceObjectService<Group>, DSpaceObjectLe
Set<Group> allMemberGroupsSet(Context context, EPerson ePerson) throws SQLException; Set<Group> allMemberGroupsSet(Context context, EPerson ePerson) throws SQLException;
/** /**
* Get all of the epeople who are a member of the * Get all of the EPerson objects who are a member of the specified group, or a member of a subgroup of the
* specified group, or a member of a sub-group of the
* specified group, etc. * specified group, etc.
* <P>
* WARNING: This method may have bad performance for Groups with a very large number of members, as it will load
* all member EPerson objects into memory. Only use if you need access to *every* EPerson object at once.
* *
* @param context The relevant DSpace Context. * @param context The relevant DSpace Context.
* @param group Group object * @param group Group object
@@ -200,6 +202,18 @@ public interface GroupService extends DSpaceObjectService<Group>, DSpaceObjectLe
*/ */
public List<EPerson> allMembers(Context context, Group group) throws SQLException; public List<EPerson> allMembers(Context context, Group group) throws SQLException;
/**
* Count all of the EPerson objects who are a member of the specified group, or a member of a subgroup of the
* specified group, etc.
* In other words, this will return the size of "allMembers()" without having to load all EPerson objects into
* memory.
* @param context current DSpace context
* @param group Group object
* @return count of EPerson object members
* @throws SQLException if error
*/
int countAllMembers(Context context, Group group) throws SQLException;
/** /**
* Find the group by its name - assumes name is unique * Find the group by its name - assumes name is unique
* *
@@ -327,4 +341,29 @@ public interface GroupService extends DSpaceObjectService<Group>, DSpaceObjectLe
*/ */
List<Group> findByMetadataField(Context context, String searchValue, MetadataField metadataField) List<Group> findByMetadataField(Context context, String searchValue, MetadataField metadataField)
throws SQLException; throws SQLException;
/**
* Find all groups which are a member of the given Parent group
*
* @param context The relevant DSpace Context.
* @param parent The parent Group to search on
* @param pageSize how many results return
* @param offset the position of the first result to return
* @return List of all groups which are members of the parent group
* @throws SQLException database exception if error
*/
List<Group> findByParent(Context context, Group parent, int pageSize, int offset)
throws SQLException;
/**
* Return number of groups which are a member of the given Parent group.
* Can be used with findByParent() for pagination of all groups within a given Parent group.
*
* @param context The relevant DSpace Context.
* @param parent The parent Group to search on
* @return number of groups which are members of the parent group
* @throws SQLException database exception if error
*/
int countByParent(Context context, Group parent)
throws SQLException;
} }

View File

@@ -10,15 +10,18 @@ package org.dspace.eperson;
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull; import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail; import static org.junit.Assert.fail;
import java.io.IOException; import java.io.IOException;
import java.sql.SQLException; import java.sql.SQLException;
import java.util.Iterator; import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.Set;
import javax.mail.MessagingException; import javax.mail.MessagingException;
import org.apache.commons.codec.DecoderException; import org.apache.commons.codec.DecoderException;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.Logger;
import org.dspace.AbstractUnitTest; import org.dspace.AbstractUnitTest;
@@ -1029,6 +1032,57 @@ public class EPersonTest extends AbstractUnitTest {
wfi.getSubmitter()); wfi.getSubmitter());
} }
@Test
public void findAndCountByGroups() throws SQLException, AuthorizeException, IOException {
// Create a group with 3 EPerson members
Group group = createGroup("parentGroup");
EPerson eperson1 = createEPersonAndAddToGroup("test1@example.com", group);
EPerson eperson2 = createEPersonAndAddToGroup("test2@example.com", group);
EPerson eperson3 = createEPersonAndAddToGroup("test3@example.com", group);
groupService.update(context, group);
Group group2 = null;
EPerson eperson4 = null;
try {
// Assert that findByGroup is the same list of EPersons as getMembers() when pagination is ignored
// (NOTE: Pagination is tested in GroupRestRepositoryIT)
// NOTE: isEqualCollection() must be used for comparison because Hibernate's "PersistentBag" cannot be
// compared directly to a List. See https://stackoverflow.com/a/57399383/3750035
assertTrue(
CollectionUtils.isEqualCollection(group.getMembers(),
ePersonService.findByGroups(context, Set.of(group), -1, -1)));
// Assert countByGroups is the same as the size of members
assertEquals(group.getMembers().size(), ePersonService.countByGroups(context, Set.of(group)));
// Add another group with duplicate EPerson
group2 = createGroup("anotherGroup");
groupService.addMember(context, group2, eperson1);
groupService.update(context, group2);
// Verify countByGroups is still 3 (existing person should not be counted twice)
assertEquals(3, ePersonService.countByGroups(context, Set.of(group, group2)));
// Add a new EPerson to new group, verify count goes up by one
eperson4 = createEPersonAndAddToGroup("test4@example.com", group2);
assertEquals(4, ePersonService.countByGroups(context, Set.of(group, group2)));
} finally {
// Clean up our data
context.turnOffAuthorisationSystem();
groupService.delete(context, group);
if (group2 != null) {
groupService.delete(context, group2);
}
ePersonService.delete(context, eperson1);
ePersonService.delete(context, eperson2);
ePersonService.delete(context, eperson3);
if (eperson4 != null) {
ePersonService.delete(context, eperson4);
}
context.restoreAuthSystemState();
}
}
/** /**
* Creates an item, sets the specified submitter. * Creates an item, sets the specified submitter.
* *
@@ -1075,4 +1129,32 @@ public class EPersonTest extends AbstractUnitTest {
context.restoreAuthSystemState(); context.restoreAuthSystemState();
return wsi; return wsi;
} }
protected Group createGroup(String name) throws SQLException, AuthorizeException {
context.turnOffAuthorisationSystem();
Group group = groupService.create(context);
group.setName(name);
groupService.update(context, group);
context.restoreAuthSystemState();
return group;
}
protected EPerson createEPersonAndAddToGroup(String email, Group group) throws SQLException, AuthorizeException {
context.turnOffAuthorisationSystem();
EPerson ePerson = createEPerson(email);
groupService.addMember(context, group, ePerson);
groupService.update(context, group);
ePersonService.update(context, ePerson);
context.restoreAuthSystemState();
return ePerson;
}
protected EPerson createEPerson(String email) throws SQLException, AuthorizeException {
context.turnOffAuthorisationSystem();
EPerson ePerson = ePersonService.create(context);
ePerson.setEmail(email);
ePersonService.update(context, ePerson);
context.restoreAuthSystemState();
return ePerson;
}
} }

View File

@@ -10,6 +10,7 @@ package org.dspace.eperson;
import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail; import static org.junit.Assert.fail;
@@ -21,6 +22,7 @@ import java.util.Arrays;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.Logger;
import org.dspace.AbstractUnitTest; import org.dspace.AbstractUnitTest;
import org.dspace.authorize.AuthorizeException; import org.dspace.authorize.AuthorizeException;
@@ -604,6 +606,30 @@ public class GroupTest extends AbstractUnitTest {
} }
} }
@Test
public void countAllMembers() throws SQLException, AuthorizeException, EPersonDeletionException, IOException {
List<EPerson> allEPeopleAdded = new ArrayList<>();
try {
context.turnOffAuthorisationSystem();
allEPeopleAdded.add(createEPersonAndAddToGroup("allMemberGroups1@dspace.org", topGroup));
allEPeopleAdded.add(createEPersonAndAddToGroup("allMemberGroups2@dspace.org", level1Group));
allEPeopleAdded.add(createEPersonAndAddToGroup("allMemberGroups3@dspace.org", level2Group));
context.restoreAuthSystemState();
assertEquals(3, groupService.countAllMembers(context, topGroup));
assertEquals(2, groupService.countAllMembers(context, level1Group));
assertEquals(1, groupService.countAllMembers(context, level2Group));
} finally {
// Remove all the people added (in order to not impact other tests)
context.turnOffAuthorisationSystem();
for (EPerson ePerson : allEPeopleAdded) {
ePersonService.delete(context, ePerson);
}
context.restoreAuthSystemState();
}
}
@Test @Test
public void isEmpty() throws SQLException, AuthorizeException, EPersonDeletionException, IOException { public void isEmpty() throws SQLException, AuthorizeException, EPersonDeletionException, IOException {
assertTrue(groupService.isEmpty(topGroup)); assertTrue(groupService.isEmpty(topGroup));
@@ -620,6 +646,40 @@ public class GroupTest extends AbstractUnitTest {
assertTrue(groupService.isEmpty(level2Group)); assertTrue(groupService.isEmpty(level2Group));
} }
@Test
public void findAndCountByParent() throws SQLException, AuthorizeException, IOException {
// Create a parent group with 3 child groups
Group parentGroup = createGroup("parentGroup");
Group childGroup = createGroup("childGroup");
Group child2Group = createGroup("child2Group");
Group child3Group = createGroup("child3Group");
groupService.addMember(context, parentGroup, childGroup);
groupService.addMember(context, parentGroup, child2Group);
groupService.addMember(context, parentGroup, child3Group);
groupService.update(context, parentGroup);
try {
// Assert that findByParent is the same list of groups as getMemberGroups() when pagination is ignored
// (NOTE: Pagination is tested in GroupRestRepositoryIT)
// NOTE: isEqualCollection() must be used for comparison because Hibernate's "PersistentBag" cannot be
// compared directly to a List. See https://stackoverflow.com/a/57399383/3750035
assertTrue(
CollectionUtils.isEqualCollection(parentGroup.getMemberGroups(),
groupService.findByParent(context, parentGroup, -1, -1)));
// Assert countBy parent is the same as the size of group members
assertEquals(parentGroup.getMemberGroups().size(), groupService.countByParent(context, parentGroup));
} finally {
// Clean up our data
context.turnOffAuthorisationSystem();
groupService.delete(context, parentGroup);
groupService.delete(context, childGroup);
groupService.delete(context, child2Group);
groupService.delete(context, child3Group);
context.restoreAuthSystemState();
}
}
protected Group createGroup(String name) throws SQLException, AuthorizeException { protected Group createGroup(String name) throws SQLException, AuthorizeException {
context.turnOffAuthorisationSystem(); context.turnOffAuthorisationSystem();

View File

@@ -8,6 +8,8 @@
package org.dspace.app.rest.repository; package org.dspace.app.rest.repository;
import java.sql.SQLException; import java.sql.SQLException;
import java.util.List;
import java.util.Set;
import java.util.UUID; import java.util.UUID;
import javax.annotation.Nullable; import javax.annotation.Nullable;
import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest;
@@ -15,7 +17,9 @@ import javax.servlet.http.HttpServletRequest;
import org.dspace.app.rest.model.GroupRest; import org.dspace.app.rest.model.GroupRest;
import org.dspace.app.rest.projection.Projection; import org.dspace.app.rest.projection.Projection;
import org.dspace.core.Context; import org.dspace.core.Context;
import org.dspace.eperson.EPerson;
import org.dspace.eperson.Group; import org.dspace.eperson.Group;
import org.dspace.eperson.service.EPersonService;
import org.dspace.eperson.service.GroupService; import org.dspace.eperson.service.GroupService;
import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.data.domain.Page; import org.springframework.data.domain.Page;
@@ -31,6 +35,9 @@ import org.springframework.stereotype.Component;
public class GroupEPersonLinkRepository extends AbstractDSpaceRestRepository public class GroupEPersonLinkRepository extends AbstractDSpaceRestRepository
implements LinkRestRepository { implements LinkRestRepository {
@Autowired
EPersonService epersonService;
@Autowired @Autowired
GroupService groupService; GroupService groupService;
@@ -45,7 +52,11 @@ public class GroupEPersonLinkRepository extends AbstractDSpaceRestRepository
if (group == null) { if (group == null) {
throw new ResourceNotFoundException("No such group: " + groupId); throw new ResourceNotFoundException("No such group: " + groupId);
} }
return converter.toRestPage(group.getMembers(), optionalPageable, projection); int total = epersonService.countByGroups(context, Set.of(group));
Pageable pageable = utils.getPageable(optionalPageable);
List<EPerson> members = epersonService.findByGroups(context, Set.of(group), pageable.getPageSize(),
Math.toIntExact(pageable.getOffset()));
return converter.toRestPage(members, pageable, total, projection);
} catch (SQLException e) { } catch (SQLException e) {
throw new RuntimeException(e); throw new RuntimeException(e);
} }

View File

@@ -8,6 +8,7 @@
package org.dspace.app.rest.repository; package org.dspace.app.rest.repository;
import java.sql.SQLException; import java.sql.SQLException;
import java.util.List;
import java.util.UUID; import java.util.UUID;
import javax.annotation.Nullable; import javax.annotation.Nullable;
import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest;
@@ -45,7 +46,11 @@ public class GroupGroupLinkRepository extends AbstractDSpaceRestRepository
if (group == null) { if (group == null) {
throw new ResourceNotFoundException("No such group: " + groupId); throw new ResourceNotFoundException("No such group: " + groupId);
} }
return converter.toRestPage(group.getMemberGroups(), optionalPageable, projection); int total = groupService.countByParent(context, group);
Pageable pageable = utils.getPageable(optionalPageable);
List<Group> memberGroups = groupService.findByParent(context, group, pageable.getPageSize(),
Math.toIntExact(pageable.getOffset()));
return converter.toRestPage(memberGroups, pageable, total, projection);
} catch (SQLException e) { } catch (SQLException e) {
throw new RuntimeException(e); throw new RuntimeException(e);
} }

View File

@@ -3091,6 +3091,157 @@ public class GroupRestRepositoryIT extends AbstractControllerIntegrationTest {
} }
// Test of /groups/[uuid]/epersons pagination
@Test
public void epersonMemberPaginationTest() throws Exception {
context.turnOffAuthorisationSystem();
EPerson eperson1 = EPersonBuilder.createEPerson(context)
.withEmail("test1@example.com")
.withNameInMetadata("Test1", "User")
.build();
EPerson eperson2 = EPersonBuilder.createEPerson(context)
.withEmail("test2@example.com")
.withNameInMetadata("Test2", "User")
.build();
EPerson eperson3 = EPersonBuilder.createEPerson(context)
.withEmail("test3@example.com")
.withNameInMetadata("Test3", "User")
.build();
EPerson eperson4 = EPersonBuilder.createEPerson(context)
.withEmail("test4@example.com")
.withNameInMetadata("Test4", "User")
.build();
EPerson eperson5 = EPersonBuilder.createEPerson(context)
.withEmail("test5@example.com")
.withNameInMetadata("Test5", "User")
.build();
Group group = GroupBuilder.createGroup(context)
.withName("Test group")
.addMember(eperson1)
.addMember(eperson2)
.addMember(eperson3)
.addMember(eperson4)
.addMember(eperson5)
.build();
context.restoreAuthSystemState();
String authTokenAdmin = getAuthToken(admin.getEmail(), password);
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/epersons")
.param("page", "0")
.param("size", "2"))
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
.andExpect(jsonPath("$._embedded.epersons", Matchers.everyItem(
hasJsonPath("$.type", is("eperson")))
))
.andExpect(jsonPath("$._embedded.epersons").value(Matchers.hasSize(2)))
.andExpect(jsonPath("$.page.size", is(2)))
.andExpect(jsonPath("$.page.number", is(0)))
.andExpect(jsonPath("$.page.totalPages", is(3)))
.andExpect(jsonPath("$.page.totalElements", is(5)));
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/epersons")
.param("page", "1")
.param("size", "2"))
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
.andExpect(jsonPath("$._embedded.epersons", Matchers.everyItem(
hasJsonPath("$.type", is("eperson")))
))
.andExpect(jsonPath("$._embedded.epersons").value(Matchers.hasSize(2)))
.andExpect(jsonPath("$.page.size", is(2)))
.andExpect(jsonPath("$.page.number", is(1)))
.andExpect(jsonPath("$.page.totalPages", is(3)))
.andExpect(jsonPath("$.page.totalElements", is(5)));
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/epersons")
.param("page", "2")
.param("size", "2"))
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
.andExpect(jsonPath("$._embedded.epersons", Matchers.everyItem(
hasJsonPath("$.type", is("eperson")))
))
.andExpect(jsonPath("$._embedded.epersons").value(Matchers.hasSize(1)))
.andExpect(jsonPath("$.page.size", is(2)))
.andExpect(jsonPath("$.page.number", is(2)))
.andExpect(jsonPath("$.page.totalPages", is(3)))
.andExpect(jsonPath("$.page.totalElements", is(5)));
}
// Test of /groups/[uuid]/subgroups pagination
@Test
public void subgroupPaginationTest() throws Exception {
context.turnOffAuthorisationSystem();
Group group = GroupBuilder.createGroup(context)
.withName("Test group")
.build();
GroupBuilder.createGroup(context)
.withParent(group)
.withName("Test subgroup 1")
.build();
GroupBuilder.createGroup(context)
.withParent(group)
.withName("Test subgroup 2")
.build();
GroupBuilder.createGroup(context)
.withParent(group)
.withName("Test subgroup 3")
.build();
GroupBuilder.createGroup(context)
.withParent(group)
.withName("Test subgroup 4")
.build();
GroupBuilder.createGroup(context)
.withParent(group)
.withName("Test subgroup 5")
.build();
context.restoreAuthSystemState();
String authTokenAdmin = getAuthToken(admin.getEmail(), password);
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/subgroups")
.param("page", "0")
.param("size", "2"))
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
.andExpect(jsonPath("$._embedded.subgroups", Matchers.everyItem(
hasJsonPath("$.type", is("group")))
))
.andExpect(jsonPath("$._embedded.subgroups").value(Matchers.hasSize(2)))
.andExpect(jsonPath("$.page.size", is(2)))
.andExpect(jsonPath("$.page.number", is(0)))
.andExpect(jsonPath("$.page.totalPages", is(3)))
.andExpect(jsonPath("$.page.totalElements", is(5)));
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/subgroups")
.param("page", "1")
.param("size", "2"))
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
.andExpect(jsonPath("$._embedded.subgroups", Matchers.everyItem(
hasJsonPath("$.type", is("group")))
))
.andExpect(jsonPath("$._embedded.subgroups").value(Matchers.hasSize(2)))
.andExpect(jsonPath("$.page.size", is(2)))
.andExpect(jsonPath("$.page.number", is(1)))
.andExpect(jsonPath("$.page.totalPages", is(3)))
.andExpect(jsonPath("$.page.totalElements", is(5)));
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/subgroups")
.param("page", "2")
.param("size", "2"))
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
.andExpect(jsonPath("$._embedded.subgroups", Matchers.everyItem(
hasJsonPath("$.type", is("group")))
))
.andExpect(jsonPath("$._embedded.subgroups").value(Matchers.hasSize(1)))
.andExpect(jsonPath("$.page.size", is(2)))
.andExpect(jsonPath("$.page.number", is(2)))
.andExpect(jsonPath("$.page.totalPages", is(3)))
.andExpect(jsonPath("$.page.totalElements", is(5)));
}
@Test @Test
public void commAdminAndColAdminCannotExploitItemReadGroupTest() throws Exception { public void commAdminAndColAdminCannotExploitItemReadGroupTest() throws Exception {