Add pagination to /groups/[uuid]/subgroups endpoint, along with tests

dspace-api/src/main/java/org/dspace/eperson/Group.java

@@ -98,7 +98,11 @@ void addMember(EPerson e) {
     }
 
     /**
-     * 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
      */
@@ -143,9 +147,13 @@ List<Group> getParentGroups() {
     }
 
     /**
-     * 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() {
         return groups;

dspace-api/src/main/java/org/dspace/eperson/GroupServiceImpl.java

@@ -829,4 +829,20 @@ public List<Group> findByMetadataField(final Context context, final String searc
     public String getName(Group dso) {
         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);
+    }
 }

dspace-api/src/main/java/org/dspace/eperson/dao/GroupDAO.java

@@ -146,4 +146,28 @@ List<Group> findAll(Context context, List<MetadataField> metadataSortFields, int
      */
     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;
 }

dspace-api/src/main/java/org/dspace/eperson/dao/impl/GroupDAOImpl.java

@@ -196,4 +196,27 @@ public int countRows(Context context) throws SQLException {
         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,
+                                  "from Group where (from Group g where g.id = :parent_id) in elements (parentGroups)");
+        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(*) from Group " +
+                                      "where (from Group g where g.id = :parent_id) in elements (parentGroups)");
+        query.setParameter("parent_id", parent.getID());
+
+        return count(query);
+    }
 }

dspace-api/src/main/java/org/dspace/eperson/service/EPersonService.java

@@ -254,9 +254,8 @@ public EPerson create(Context context) throws SQLException,
     /**
      * Retrieve all EPerson accounts which belong to at least one of the specified groups.
      * <P>
-     * WARNING: This method should be used sparingly, as it could have performance issues for Groups with very large
-     * lists of members. In that situation, a very large number of EPerson objects will be loaded into memory.
-     * See https://github.com/DSpace/DSpace/issues/9052
+     * 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.
      *

dspace-api/src/main/java/org/dspace/eperson/service/GroupService.java

@@ -327,4 +327,29 @@ public List<Group> findAll(Context context, List<MetadataField> metadataSortFiel
      */
     List<Group> findByMetadataField(Context context, String searchValue, MetadataField metadataField)
         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;
 }

dspace-api/src/test/java/org/dspace/eperson/GroupTest.java

@@ -10,6 +10,7 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -24,6 +25,7 @@
 import org.apache.logging.log4j.Logger;
 import org.dspace.AbstractUnitTest;
 import org.dspace.authorize.AuthorizeException;
+import org.dspace.builder.GroupBuilder;
 import org.dspace.eperson.factory.EPersonServiceFactory;
 import org.dspace.eperson.service.EPersonService;
 import org.dspace.eperson.service.GroupService;
@@ -620,6 +622,31 @@ public void isEmpty() throws SQLException, AuthorizeException, EPersonDeletionEx
         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);
+
+        // Assert that findByParent is the same list of groups as getMemberGroups() when pagination is ignored
+        // (NOTE: Pagination is tested in GroupRestRepositoryIT)
+        assertEquals(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));
+
+        // Clean up our data
+        groupService.delete(context, parentGroup);
+        groupService.delete(context, childGroup);
+        groupService.delete(context, child2Group);
+        groupService.delete(context, child3Group);
+    }
+
 
     protected Group createGroup(String name) throws SQLException, AuthorizeException {
         context.turnOffAuthorisationSystem();

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/GroupGroupLinkRepository.java

@@ -8,6 +8,7 @@
 package org.dspace.app.rest.repository;
 
 import java.sql.SQLException;
+import java.util.List;
 import java.util.UUID;
 import javax.annotation.Nullable;
 import javax.servlet.http.HttpServletRequest;
@@ -45,7 +46,11 @@ public Page<GroupRest> getGroups(@Nullable HttpServletRequest request,
             if (group == null) {
                 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) {
             throw new RuntimeException(e);
         }

dspace-server-webapp/src/test/java/org/dspace/app/rest/GroupRestRepositoryIT.java

@@ -3169,6 +3169,79 @@ public void epersonMemberPaginationTest() throws Exception {
                                  .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
     public void commAdminAndColAdminCannotExploitItemReadGroupTest() throws Exception {