Improve performance for Groups with many EPerson members. Fix pagination on endpoints

[tdonohue]

References

• Fixes Performance Issues with Groups having many EPerson members #9052 by providing major speed improvements on backend & proper pagination of group members.
• NOTE: This is not a complete fix, but will improve performance significantly. Additional endpoints will be necessary to completely fix performance issues in Angular UI. See "Edit Group" page always requests all member subgroups and EPersons. This has bad performance for large groups. dspace-angular#2512

Description

This PR improves the performance of Groups which have many EPerson (or Group) members by adding proper pagination to these endpoints. (Previously all objects were always loaded into memory before pagination was applied.)

• GET /api/eperson/groups/<:uuid>/epersons
• GET /api/eperson/groups/<:uuid>/subgroups

This was implemented via new paginated methods in EPersonService/DAO and GroupService/DAO:

• EPersonService/DAO.findByGroups() -> Locates all EPersons who are a member of one or more groups in a paginated manner.
• EPersonService/DAO.countByGroups() -> Provides total count of those EPerson members (necessary for pagination)
• GroupService/DAO.findByParent() -> Locates all (Sub)Groups who are a member group of the given Parent group in a paginated manner.
• GroupService/DAO.countByParent() -> Provides total count of those (Sub)Groups members (necessary for pagination)
• Unit & Integration Tests were added for these new methods

Warnings were added to JavaDoc for the following methods. All of which will have very BAD performance for large groups:

• Group.getMembers()
• Group.getMemberGroups()
• EPersonService.findByGroups() (unpaginated version)
• GroupService.allMembers()
• NOTE: I did NOT remove all usages of these methods. Attempting to remove all usages of these methods would require more discussion/thought. I just replaced the simple ones (i.e. some calls to allMembers()) as described below.

Finally, I discovered several usages of GroupService.allMembers() where the code only required the size() of that list. These usages were replaced with new countAllMembers() or countByParent() methods. All these usages have existing detailed tests were still pass with the newly refactored code:

• EPersonService.delete(Context, EPerson, cascade)
• GroupService.removeMember(Context, Group, EPerson)
• GroupService.removeMember(Context, Group, Group)

Instructions for Reviewers

• See Performance Issues with Groups having many EPerson members #9052.
• In UI, verify "Access Control" -> Groups is working properly for search/editing.
• In REST API, verify pagination still works properly for BOTH the /epersons and /subgroups endpoints (size = how many to return, page = which page of results to return). With this PR in place, small pages (small values of size) will return a response quickly. Without this PR (e.g. in 7.6), the size of page doesn't matter as every object is loaded into memory regardless of size before a response is returned.
  • e.g. GET /api/eperson/groups/<:uuid>/epersons?page=0&size=1 (Only return one result at a time per page)
  • e.g. GET /api/eperson/groups/<:uuid>/subgroups?page=0&size=5 (Return first page of 5 results per page)
• Review code and new tests
• Verify all automated tests pass.

[alanorth]

I tested this patch on DSpace 7.6. I was able to search and edit groups as expected, with no errors in the browser console or dspace.log. I notice the dspace.log shows the new pagination:

2023-09-19 21:58:38,419 INFO  aaf64908-f03a-4b93-a975-1445a69c26fc c1902ac1-d0e2-4148-bc19-93bf3ff2c572 org.dspace.app.rest.utils.DSpaceAPIRequestLoggingFilter @ Before request [GET /server/api/eperson/groups/8b556d2d-a5d0-46c1-b648-128c10792e9b/epersons] originated from /access-control/groups?gl.page=7

I was not able to test the REST API directly because I don't know how to do it via simple HTTP requests.

[tdonohue]

@alanorth : Thanks for the tests! I added more details under "Instructions for Reviewers" on how to test pagination. If you are interested, It can be done either via commandline (wget or curl) or from the HAL Browser. (Pagination also has some automated tests in my updates to GroupRestRepositoryIT in this PR, so I feel confident it works. But that's how you'd manually test it)

[alanorth]

Thanks @tdonohue. The HAL browser is easier than curl because we don't have to mess with signing tokens.

I checked the two endpoints in the HAL browser and don't see a difference. Paging already seems to work on my unpatched DSpace 7.6 instance. For reference, I tried:

• /server/api/eperson/groups/<:uuid>/epersons?page=2&size=1

And I get this response in the HAL browser:

{
  "page": {
    "number": 2,
    "size": 1,
    "totalPages": 521,
    "totalElements": 521
  }
}

Perhaps I'm misunderstanding...

[tdonohue]

@alanorth : Apologies, I've misworded the description slightly. Yes, paging already works in 7.6... but this PR changes the paging to provide better performance. In 7.6, paging will first load every object into memory (in your test, 512 objects will be loaded into memory, as totalElements=512) before returning just the few necessary for the page. With this PR in place, only the objects on the page (i.e. size value) will be loaded into memory.

So, if you test with 7.6, you'll see slower behavior to the paging, as every page will load totalElements number of objects before returning the response (even when size=1). But, if you test with this PR, you'll see much quicker responses, as you'll only load size number of objects. Based on your response above, if you use size=1, that means 7.6 will still load 512 objects into memory while this PR would only load 1.

It can be difficult to see the speed difference on smaller repositories, but it should still be noticeable.

[alanorth]

@tdonohue ok thanks for taking the time to explain. I was contemplating not commenting, assuming I was doing something stupid. So I am +1 on this by testing because it doesn't change functionality at all and, while I didn't notice the performance increase on localhost in my case, it didn't have any negative affect either.

I will let someone else review the code since I don't know it well.

[vins01-4science]

Hello @tdonohue , thank you very much for this improvement 🚀 !

However, I am a little bit concerned about the paginated queries that are not using any ordering. I think that in both cases we should offer an order, otherwise the pagination cannot be used in a real scenario.

On the other hand I found out some other little improvements that can be made, and that I suggest that can be done.

[tdonohue]

@vins01-4science : Thanks for your review. I've addressed all feedback in the latest commit. That said, I did NOT fix the issues you pointed out with nested for loops in GroupServiceImpl (and similar) as this PR didn't create that code. While I agree that code could be cleaned up better, I won't have time to refactor it for the 7.6.1 release... so I'd prefer moving forward with this PR as-is and only reviewing the code this PR adds/removes. Thanks!

[vins01-4science]

Looks good to me, thank you @tdonohue 🚀

[tdonohue]

Merging as this is at +2.

[dspace-bot]

Successfully created backport PR for dspace-7_x:

• [Port dspace-7_x] Improve performance for Groups with many EPerson members. Fix pagination on endpoints #9166