NEW Allow manipulating eagerloading queries #11140
Conversation
| $list->eagerLoadAllRelations[$item] = $item; | ||
| $list->eagerLoadAllRelations[$item] ??= null; |
There was a problem hiding this comment.
This change does two things:
- Default the value to
null, so we can do a simply nullable check to see if there was a callback set for that chain or not - Doesn't override existing values - if a callback was explicitly set, we don't want to remove it here.
| // Set the callback for this chain | ||
| $list->eagerLoadAllRelations[$relationChain] = $callback; |
There was a problem hiding this comment.
Note that this will override a callback with null if someone adds the same chain twice, the second time without a callback. For example:
$list = $list->eagerLoad(['MyRelation.AnotherRelation' => $someCallback]);
$list = $list->eagerLoad('MyRelation.AnotherRelation');This allows complex branching logic, where some branches can remove the filters/sorting/etc that other branches might apply.
src/ORM/DataList.php
Outdated
| // Get the join records so we can correctly identify which children belong to which parents | ||
| // This also holds extra fields data | ||
| $joinRows = DB::query('SELECT * FROM "' . $joinTable . '" WHERE "' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ') AND ' . $childIDField . ' IN (' . implode(',', $fetchedIDs) . ')'); |
There was a problem hiding this comment.
Note we're now only getting the join records that are applicable to the potentially-filtered-down set of relations.
GuySartorelli
force-pushed
the
pulls/5/eagerloading-prefilter
branch
from
77851f8 to
befd2ad
Compare
GuySartorelli
changed the title
GuySartorelli
force-pushed
the
pulls/5/eagerloading-prefilter
branch
from
befd2ad to
342f3bf
Compare
GuySartorelli
force-pushed
the
pulls/5/eagerloading-prefilter
branch
from
342f3bf to
3df0768
Compare
| // Throw exception if developers try to manipulate a has_one relation as a list | ||
| if ($this->eagerLoadAllRelations[$relationChain] !== null) { | ||
| throw new LogicException("Cannot manipulate eagerloading query for $relationType relation $relationName"); | ||
| } |
There was a problem hiding this comment.
You can't go $myRecord->MyHasOne()->filter(), for example. This would be the eagerloading equivalent of that if we allowed it.
| // Get the join records so we can correctly identify which children belong to which parents | ||
| // This also holds extra fields data | ||
| $joinRows = DB::query('SELECT * FROM "' . $joinTable . '" WHERE "' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')'); |
There was a problem hiding this comment.
Moved to be after fetching the records we care about, and is now filtered and sorted based on that.
| // Respect sort order of fetched items | ||
| . ' ORDER BY FIELD(' . $childIDField . ', ' . $fetchedIDsAsString . ')' |
There was a problem hiding this comment.
The sorting is important here because the join rows are what we're looping through - so the order of these join rows determines the order of the records in the resultant EagerLoadedList
| /** | ||
| * NOTE: Do not change `DataList` to `static` in this method signature. | ||
| * Subclasses of DataList must still accept DataList arguments and return DataList! | ||
| */ | ||
| private function manipulateEagerLoadingQuery( | ||
| DataList $fetchList, | ||
| string $relationChain, | ||
| string $relationType | ||
| ): DataList { |
There was a problem hiding this comment.
Discussed with Steve - he prefers DataList here over self.
I'm agnostic - I think the comment needs to be there for our future selves either way, and both IDEs and static analysis will ultimately treat them the same.
| * <code> | ||
| * $myDataList->eagerLoad('MyRelation.NestedRelation.EvenMoreNestedRelation', 'DifferentRelation') | ||
| * </code> |
There was a problem hiding this comment.
Adding <code> tags here is an unrelated improvement for API docs since I'm adding another example here.
| // If an array is passed in directly, treat it as though $relationChains wasn't spread. | ||
| if (count($relationChains) === 1 && is_array($relationChains[array_key_first($relationChains)])) { | ||
| $relationChains = $relationChains[array_key_first($relationChains)]; | ||
| } |
There was a problem hiding this comment.
Handles the difference between passing strings (original API, useful for templates) and passing an array (enhanced API, necessary if applying manipulations)
| // Reject non-callable in associative array | ||
| if ($callback !== null && !is_callable($callback)) { | ||
| throw new LogicException( | ||
| 'Value of associative array must be a callable.' | ||
| . 'If you don\'t want to pre-filter the list, use an indexed array.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bit of added type safety - it would cause an error later on, but throwing out a clear message about what's wrong here will improve the debugging experience
src/ORM/DataQuery.php
Outdated
| /** | ||
| * Get the current limit of this query. | ||
| */ | ||
| public function getLimit(): array | ||
| { | ||
| return $this->query->getLimit(); | ||
| } |
There was a problem hiding this comment.
This new API is necessary for us to explicitly disallow calling limit() in an eagerloading manipulation, since at best allowing that would provide unexpected results.
| $dataList = EagerLoadObject::get()->filter($filter)->eagerLoad(...$eagerLoad); | ||
| $dataList = EagerLoadObject::get()->filter($filter)->eagerLoad($eagerLoad); |
There was a problem hiding this comment.
I've swapped a few existing tests to pass the array directly, so we have coverage for non-associative arrays. I've left others alone to retain coverage for passing strings.
| $this->assertGreaterThan(1, $loop1Count); | ||
| $this->assertGreaterThan(1, $loop2Count); |
There was a problem hiding this comment.
Unrelated change - the test was meant to have these originally which is why those variables were there.
| EagerLoadObject::get()->eagerLoad(['HasManyEagerLoadObjects' => 'HasManyEagerLoadObjects']); | ||
| } | ||
|
|
||
| public function provideNoLimitEagerloadingQuery(): array |
There was a problem hiding this comment.
Do we need to test/implement belongs_many_many?
many_many_through?
There was a problem hiding this comment.
No harm in it, will do.
There was a problem hiding this comment.
Done - all of the many_many relation types are built the same so was pretty easy to refactor for the extra types.
| EagerLoadObject::get()->eagerLoad(['HasManyEagerLoadObjects' => 'HasManyEagerLoadObjects']); | ||
| } | ||
|
|
||
| public function provideNoLimitEagerloadingQuery(): array |
There was a problem hiding this comment.
| public function provideNoLimitEagerloadingQuery(): array | |
| public function provideNoLimitEagerLoadingQuery(): array |
| * @dataProvider provideNoLimitEagerloadingQuery | ||
| */ | ||
| public function testNoLimitEagerloadingQuery(string $relation, string $relationType, callable $callback): void |
There was a problem hiding this comment.
| * @dataProvider provideNoLimitEagerloadingQuery | |
| */ | |
| public function testNoLimitEagerloadingQuery(string $relation, string $relationType, callable $callback): void | |
| * @dataProvider provideNoLimitEagerLoadingQuery | |
| */ | |
| public function testNoLimitEagerLoadingQuery(string $relation, string $relationType, callable $callback): void |
tests/php/ORM/DataQueryTest.php
Outdated
| @@ -879,4 +879,38 @@ public function testWithUsingDataQueryAppliesRelations() | |||
| // This will throw an exception if it fails - it passes if there's no exception. | |||
| $dataQuery->execute(); | |||
| } | |||
|
|
|||
| public function provideGetLimit(): array | |||
There was a problem hiding this comment.
Ideally we'd get rid of this test - see my comment on DataQuery
There was a problem hiding this comment.
I got the following error when testing this:
// WITH EAGERLOADING, FILTER/SORT IN DB
// 0.1115, 0.1077, 0.1019, 0.1110 -- 0.1080s (avg) (~29% faster than without eagerloading and ~85% faster than filtering in PHP)
foreach (MyDataObject::get()->eagerLoad(['MySubDataObjects' => fn (DataList $list) => $list->filter('Title:PartialMatch', 1)->Sort('Title', 'DESC')]) as $do) {
$do->MySubDataObjects()->toArray();
}
ERROR [Emergency]: Uncaught TypeError: {closure}(): Argument #1 ($list) must be of type DataList, SilverStripe\ORM\DataList given, called in /var/www/vendor/silverstripe/framework/src/ORM/DataList.php on line 1435
IN GET dev/build
The code I copied into the issue is missing a |
GuySartorelli
force-pushed
the
pulls/5/eagerloading-prefilter
branch
from
3df0768 to
95c9f33
Compare
GuySartorelli
force-pushed
the
pulls/5/eagerloading-prefilter
branch
from
95c9f33 to
6ed0d21
Compare
Description
Adds a backwards-compatible way to pre-filter, pre-sort, and apply basically any other type of transformation you want to eagerloaded data.
Note that I have opted to keep the existing public API, since the alternative suggested in the issue would not be usable in templates. I tried consulting with other devs on slack per the acceptance criteria but there's really not anyone who's familiar with eagerloading yet.
Applying a limit in the callback is explicitly disallowed because it wouldn't be applied per relation list, it'd be applied to the whole eager query for reach relation.
Manual testing steps
There's benchmarking code in the linked issue.
Issues
Pull request checklist