silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 820 forks source link

Eagerloading chains skip adding to some relation lists #11138

Closed GuySartorelli closed 4 months ago

GuySartorelli commented 4 months ago

Module version(s) affected

5.1.x

Description

When eagerloading a chain that has a many_many (possibly also with has_many) relation before some other relation, the eager loaded data is only added to one of potentially many many_many relation lists.

For example:

$list = Member::get()->eagerLoad('Groups.Permissions');

When looping through this list and the nested relations, we get (excluding all eager-loading queries): 1 query to get all members 0 queries to get all groups (correctly eager-loaded) ~n-1 queries to get all permissions

This happens because each group can be related to multiple members, and each permission is only added to the eager loaded data of a single list. So if MemberA and MemberB are both in GroupA, the permissions for MemberA->GroupA are eager loaded, but the permissions for MemberB->GroupA are not eager loaded, which means there's another query triggered.

You might expect the permissions to be added to GroupA directly, but the eager loaded data isn't hydrated into a DataObject object until you start looping over the EagerLoadedList, so each EagerLoadedList ends up with its own separate copy of the group, and needs its own separate copy of the eager loaded relation data as well.

How to reproduce

as above

Possible Solution

We should just not break early in DataList::addEagerLoadedDataToParent() when looping through arrays of EagerLoadedList.

Note that it would probably be better to instead hydrate the DataObject records early, and share a single object for each record across lists, which would also save more memory... but that would arguably be a breaking behavioural change at this stage.

Validations

PRs