Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Use ORM functions to perform eager loading instead of raw query #11509

Open
wants to merge 1 commit into
base: 5.3
Choose a base branch
from
Open

FIX Use ORM functions to perform eager loading instead of raw query #11509

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/ORM/DataList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\Debug;
use SilverStripe\ORM\Queries\SQLConditionGroup;
use SilverStripe\ORM\Queries\SQLSelect;
use SilverStripe\View\ViewableData;
use Exception;
use InvalidArgumentException;
Expand Down Expand Up @@ -1349,6 +1350,10 @@ private function fetchEagerLoadManyMany(
$fetchList = $relationList->forForeignID($parentIDs);
$fetchList = $this->manipulateEagerLoadingQuery($fetchList, $relationChain, $relationType);
$fetchedRows = $fetchList->getFinalisedQuery();
$query = $fetchList->dataQuery()->query();
$fetchedOrderBy = $query->getOrderBy();
$childTables = $query->queriedTables();
$childTable = reset($childTables);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we arbitrarily take the first table?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I could tell this is the only way to derive the base table (the FROM) from a DataList query. An alternative is to grab the table name from the baseClass data object.

I'll add comments to my next commit to make clear what's happening exactly.


foreach ($fetchedRows as $row) {
$fetchedRowsArray[$row['ID']] = $row;
Expand All @@ -1361,15 +1366,18 @@ private function fetchEagerLoadManyMany(
$joinRows = [];
if (!empty($parentIDs) && !empty($fetchedIDs)) {
$fetchedIDsAsString = implode(',', $fetchedIDs);
$joinRows = DB::query(
'SELECT * FROM "' . $joinTable
$joinRows = SQLSelect::create()
// Only select join columns to avoid ambiguity with joined child table columns
->setSelect('"' . $joinTable . '".' . "*")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->setSelect('"' . $joinTable . '".' . "*")

It will select everything by default, I believe.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the table-specific select may lead to ambiguity with the columns from the joined table. This ensures that we're always working with the correct values.

->setFrom('"' . $joinTable . '"')
// Only get joins relevant for the parent list
. '" WHERE "' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')'
// Exclude any children that got filtered out
. ' AND ' . $childIDField . ' IN (' . $fetchedIDsAsString . ')'
// Respect sort order of fetched items
. ' ORDER BY FIELD(' . $childIDField . ', ' . $fetchedIDsAsString . ')'
);
->addWhere('"' . $joinTable . '"."' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')')
// Exclude children that got filtered out
->addWhere('"' . $joinTable . '"."' . $childIDField . '" IN (' . $fetchedIDsAsString . ')')
// Respect sort order of fetched children by joining child table and using its order by clause
->addLeftJoin($childTable, "$childTable.ID = $joinTable.$childIDField")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this join?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say we have a joinTable AB linking A's many_many to B. The original query preserved the order of the items that were fetched from table B by putting their IDs in the FIELD list. This changes that by joining table B so that the original order by used to fetch B items can also be used in this query.

The perfect solution would be to right join the join table to the relation query (line 1350), which removes the need for this query altogether, though I'm not sure what the implications will be. I'll have to look into that.

->setOrderBy($fetchedOrderBy)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this orderby? How do we know it's what we actually want?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This order by is the replacement for the FIELD order by in the original query as explained in the other comment.

->execute();
}

// Store the children in an EagerLoadedList against the correct parent
Expand Down