Skip to content

Commit e7c4b9e

Browse files
tdonohuetdonohue
committed
Add pagination to /groups/[uuid]/subgroups endpoint, along with tests
1 parent 457dd9a commit e7c4b9e

9 files changed

Lines changed: 207 additions & 7 deletions

File tree

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,11 @@ void addMember(EPerson e) {
9898
}
9999

100100
/**
101-
* Return EPerson members of a Group
101+
* Return EPerson members of a Group.
102+
* <P>
103+
* WARNING: This method may have bad performance for Groups with large numbers of EPerson members.
104+
* Therefore, only use this when you need to access every EPerson member. Instead, consider using
105+
* EPersonService.findByGroups() for a paginated list of EPersons.
102106
*
103107
* @return list of EPersons
104108
*/
@@ -143,9 +147,13 @@ List<Group> getParentGroups() {
143147
}
144148

145149
/**
146-
* Return Group members of a Group.
150+
* Return Group members (i.e. direct subgroups) of a Group.
151+
* <P>
152+
* WARNING: This method may have bad performance for Groups with large numbers of Subgroups.
153+
* Therefore, only use this when you need to access every Subgroup. Instead, consider using
154+
* GroupService.findByParent() for a paginated list of Subgroups.
147155
*
148-
* @return list of groups
156+
* @return list of subgroups
149157
*/
150158
public List<Group> getMemberGroups() {
151159
return groups;

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -829,4 +829,20 @@ public List<Group> findByMetadataField(final Context context, final String searc
829829
public String getName(Group dso) {
830830
return dso.getName();
831831
}
832+
833+
@Override
834+
public List<Group> findByParent(Context context, Group parent, int pageSize, int offset) throws SQLException {
835+
if (parent == null) {
836+
return null;
837+
}
838+
return groupDAO.findByParent(context, parent, pageSize, offset);
839+
}
840+
841+
@Override
842+
public int countByParent(Context context, Group parent) throws SQLException {
843+
if (parent == null) {
844+
return 0;
845+
}
846+
return groupDAO.countByParent(context, parent);
847+
}
832848
}

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,4 +146,28 @@ List<Group> findAll(Context context, List<MetadataField> metadataSortFields, int
146146
*/
147147
Group findByIdAndMembership(Context context, UUID id, EPerson ePerson) throws SQLException;
148148

149+
/**
150+
* Find all groups which are members of a given parent group.
151+
* This provides the same behavior as group.getMemberGroups(), but in a paginated fashion.
152+
*
153+
* @param context The DSpace context
154+
* @param parent Parent Group to search within
155+
* @param pageSize how many results return
156+
* @param offset the position of the first result to return
157+
* @return Groups matching the query
158+
* @throws SQLException if database error
159+
*/
160+
List<Group> findByParent(Context context, Group parent, int pageSize, int offset) throws SQLException;
161+
162+
/**
163+
* Returns the number of groups which are members of a given parent group.
164+
* This provides the same behavior as group.getMemberGroups().size(), but with better performance for large groups.
165+
* This method may be used with findByParent() to perform pagination.
166+
*
167+
* @param context The DSpace context
168+
* @param parent Parent Group to search within
169+
* @return Number of Groups matching the query
170+
* @throws SQLException if database error
171+
*/
172+
int countByParent(Context context, Group parent) throws SQLException;
149173
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,4 +196,27 @@ public int countRows(Context context) throws SQLException {
196196
return count(createQuery(context, "SELECT count(*) FROM Group"));
197197
}
198198

199+
@Override
200+
public List<Group> findByParent(Context context, Group parent, int pageSize, int offset) throws SQLException {
201+
Query query = createQuery(context,
202+
"from Group where (from Group g where g.id = :parent_id) in elements (parentGroups)");
203+
query.setParameter("parent_id", parent.getID());
204+
if (pageSize > 0) {
205+
query.setMaxResults(pageSize);
206+
}
207+
if (offset > 0) {
208+
query.setFirstResult(offset);
209+
}
210+
query.setHint("org.hibernate.cacheable", Boolean.TRUE);
211+
212+
return list(query);
213+
}
214+
215+
public int countByParent(Context context, Group parent) throws SQLException {
216+
Query query = createQuery(context, "SELECT count(*) from Group " +
217+
"where (from Group g where g.id = :parent_id) in elements (parentGroups)");
218+
query.setParameter("parent_id", parent.getID());
219+
220+
return count(query);
221+
}
199222
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,8 @@ public EPerson create(Context context) throws SQLException,
254254
/**
255255
* Retrieve all EPerson accounts which belong to at least one of the specified groups.
256256
* <P>
257-
* WARNING: This method should be used sparingly, as it could have performance issues for Groups with very large
258-
* lists of members. In that situation, a very large number of EPerson objects will be loaded into memory.
259-
* See https://github.com/DSpace/DSpace/issues/9052
257+
* WARNING: This method may have bad performance issues for Groups with a very large number of members,
258+
* as it will load all member EPerson objects into memory.
260259
* <P>
261260
* For better performance, use the paginated version of this method.
262261
*

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,4 +327,29 @@ public List<Group> findAll(Context context, List<MetadataField> metadataSortFiel
327327
*/
328328
List<Group> findByMetadataField(Context context, String searchValue, MetadataField metadataField)
329329
throws SQLException;
330+
331+
/**
332+
* Find all groups which are a member of the given Parent group
333+
*
334+
* @param context The relevant DSpace Context.
335+
* @param parent The parent Group to search on
336+
* @param pageSize how many results return
337+
* @param offset the position of the first result to return
338+
* @return List of all groups which are members of the parent group
339+
* @throws SQLException database exception if error
340+
*/
341+
List<Group> findByParent(Context context, Group parent, int pageSize, int offset)
342+
throws SQLException;
343+
344+
/**
345+
* Return number of groups which are a member of the given Parent group.
346+
* Can be used with findByParent() for pagination of all groups within a given Parent group.
347+
*
348+
* @param context The relevant DSpace Context.
349+
* @param parent The parent Group to search on
350+
* @return number of groups which are members of the parent group
351+
* @throws SQLException database exception if error
352+
*/
353+
int countByParent(Context context, Group parent)
354+
throws SQLException;
330355
}

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import static org.hamcrest.CoreMatchers.equalTo;
1111
import static org.hamcrest.CoreMatchers.notNullValue;
1212
import static org.hamcrest.MatcherAssert.assertThat;
13+
import static org.junit.Assert.assertEquals;
1314
import static org.junit.Assert.assertFalse;
1415
import static org.junit.Assert.assertTrue;
1516
import static org.junit.Assert.fail;
@@ -24,6 +25,7 @@
2425
import org.apache.logging.log4j.Logger;
2526
import org.dspace.AbstractUnitTest;
2627
import org.dspace.authorize.AuthorizeException;
28+
import org.dspace.builder.GroupBuilder;
2729
import org.dspace.eperson.factory.EPersonServiceFactory;
2830
import org.dspace.eperson.service.EPersonService;
2931
import org.dspace.eperson.service.GroupService;
@@ -620,6 +622,31 @@ public void isEmpty() throws SQLException, AuthorizeException, EPersonDeletionEx
620622
assertTrue(groupService.isEmpty(level2Group));
621623
}
622624

625+
@Test
626+
public void findAndCountByParent() throws SQLException, AuthorizeException, IOException {
627+
// Create a parent group with 3 child groups
628+
Group parentGroup = createGroup("parentGroup");
629+
Group childGroup = createGroup("childGroup");
630+
Group child2Group = createGroup("child2Group");
631+
Group child3Group = createGroup("child3Group");
632+
groupService.addMember(context, parentGroup, childGroup);
633+
groupService.addMember(context, parentGroup, child2Group);
634+
groupService.addMember(context, parentGroup, child3Group);
635+
groupService.update(context, parentGroup);
636+
637+
// Assert that findByParent is the same list of groups as getMemberGroups() when pagination is ignored
638+
// (NOTE: Pagination is tested in GroupRestRepositoryIT)
639+
assertEquals(parentGroup.getMemberGroups(), groupService.findByParent(context, parentGroup, -1, -1));
640+
// Assert countBy parent is the same as the size of group members
641+
assertEquals(parentGroup.getMemberGroups().size(), groupService.countByParent(context, parentGroup));
642+
643+
// Clean up our data
644+
groupService.delete(context, parentGroup);
645+
groupService.delete(context, childGroup);
646+
groupService.delete(context, child2Group);
647+
groupService.delete(context, child3Group);
648+
}
649+
623650

624651
protected Group createGroup(String name) throws SQLException, AuthorizeException {
625652
context.turnOffAuthorisationSystem();

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
package org.dspace.app.rest.repository;
99

1010
import java.sql.SQLException;
11+
import java.util.List;
1112
import java.util.UUID;
1213
import javax.annotation.Nullable;
1314
import javax.servlet.http.HttpServletRequest;
@@ -45,7 +46,11 @@ public Page<GroupRest> getGroups(@Nullable HttpServletRequest request,
4546
if (group == null) {
4647
throw new ResourceNotFoundException("No such group: " + groupId);
4748
}
48-
return converter.toRestPage(group.getMemberGroups(), optionalPageable, projection);
49+
int total = groupService.countByParent(context, group);
50+
Pageable pageable = utils.getPageable(optionalPageable);
51+
List<Group> memberGroups = groupService.findByParent(context, group, pageable.getPageSize(),
52+
Math.toIntExact(pageable.getOffset()));
53+
return converter.toRestPage(memberGroups, pageable, total, projection);
4954
} catch (SQLException e) {
5055
throw new RuntimeException(e);
5156
}

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3169,6 +3169,79 @@ public void epersonMemberPaginationTest() throws Exception {
31693169
.andExpect(jsonPath("$.page.totalElements", is(5)));
31703170
}
31713171

3172+
// Test of /groups/[uuid]/subgroups pagination
3173+
@Test
3174+
public void subgroupPaginationTest() throws Exception {
3175+
context.turnOffAuthorisationSystem();
3176+
3177+
Group group = GroupBuilder.createGroup(context)
3178+
.withName("Test group")
3179+
.build();
3180+
3181+
GroupBuilder.createGroup(context)
3182+
.withParent(group)
3183+
.withName("Test subgroup 1")
3184+
.build();
3185+
GroupBuilder.createGroup(context)
3186+
.withParent(group)
3187+
.withName("Test subgroup 2")
3188+
.build();
3189+
GroupBuilder.createGroup(context)
3190+
.withParent(group)
3191+
.withName("Test subgroup 3")
3192+
.build();
3193+
GroupBuilder.createGroup(context)
3194+
.withParent(group)
3195+
.withName("Test subgroup 4")
3196+
.build();
3197+
GroupBuilder.createGroup(context)
3198+
.withParent(group)
3199+
.withName("Test subgroup 5")
3200+
.build();
3201+
3202+
context.restoreAuthSystemState();
3203+
3204+
String authTokenAdmin = getAuthToken(admin.getEmail(), password);
3205+
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/subgroups")
3206+
.param("page", "0")
3207+
.param("size", "2"))
3208+
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
3209+
.andExpect(jsonPath("$._embedded.subgroups", Matchers.everyItem(
3210+
hasJsonPath("$.type", is("group")))
3211+
))
3212+
.andExpect(jsonPath("$._embedded.subgroups").value(Matchers.hasSize(2)))
3213+
.andExpect(jsonPath("$.page.size", is(2)))
3214+
.andExpect(jsonPath("$.page.number", is(0)))
3215+
.andExpect(jsonPath("$.page.totalPages", is(3)))
3216+
.andExpect(jsonPath("$.page.totalElements", is(5)));
3217+
3218+
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/subgroups")
3219+
.param("page", "1")
3220+
.param("size", "2"))
3221+
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
3222+
.andExpect(jsonPath("$._embedded.subgroups", Matchers.everyItem(
3223+
hasJsonPath("$.type", is("group")))
3224+
))
3225+
.andExpect(jsonPath("$._embedded.subgroups").value(Matchers.hasSize(2)))
3226+
.andExpect(jsonPath("$.page.size", is(2)))
3227+
.andExpect(jsonPath("$.page.number", is(1)))
3228+
.andExpect(jsonPath("$.page.totalPages", is(3)))
3229+
.andExpect(jsonPath("$.page.totalElements", is(5)));
3230+
3231+
getClient(authTokenAdmin).perform(get("/api/eperson/groups/" + group.getID() + "/subgroups")
3232+
.param("page", "2")
3233+
.param("size", "2"))
3234+
.andExpect(status().isOk()).andExpect(content().contentType(contentType))
3235+
.andExpect(jsonPath("$._embedded.subgroups", Matchers.everyItem(
3236+
hasJsonPath("$.type", is("group")))
3237+
))
3238+
.andExpect(jsonPath("$._embedded.subgroups").value(Matchers.hasSize(1)))
3239+
.andExpect(jsonPath("$.page.size", is(2)))
3240+
.andExpect(jsonPath("$.page.number", is(2)))
3241+
.andExpect(jsonPath("$.page.totalPages", is(3)))
3242+
.andExpect(jsonPath("$.page.totalElements", is(5)));
3243+
}
3244+
31723245
@Test
31733246
public void commAdminAndColAdminCannotExploitItemReadGroupTest() throws Exception {
31743247

0 commit comments

Comments
 (0)