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.

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

EagerLoadedList can't filter or sort by fields on its relations #10874

Closed
GuySartorelli opened this issue Jul 19, 2023 · 5 comments
Closed

EagerLoadedList can't filter or sort by fields on its relations #10874

GuySartorelli opened this issue Jul 19, 2023 · 5 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Jul 19, 2023

#10864 introduces a new EagerLoadedList which mimics DataList funtionality, but with a dataset that has already been fetched from the database.

Currently that list doesn't support sorting or filtering by relation fields, e.g.

$item->SomeEagerLoadedRelation()->filter('AnotherRelation.MyField', 'SomeValue')->sort('AnotherRelation.MyField');

The sort() implementation currently does check if relations are valid, but then throws an exception because it can't actually sort by it.
filter() currently ignores them entirely.

Acceptance Criteria

See #10874 (comment)

  • Can sort by relations, the same as with the DataList relation lists
  • Can filter/exclude by relations, the same as with the DataList relation lists
  • If feasible, filtering by collations is also implemented (e.g. $list->filter('Relation.Count()', 5))
    • If that's not feasible, it must be documented as a limitation for this list.
  • Documentation is updated to remove any references to this not being possible.

Notes

  • Possibly filtering will be implemented in this issue instead (which is in progress) - Static lists like ArrayList or EagerLoadedDataList should support SearchFilter syntax for filter/exclude #5911
  • We already might have some of the data stored in $this->eagerLoadedData - we should avoid making any new db queries to fetch relations we've already loaded.
  • For relations that we haven't loaded yet, make a judgement call as to how to best fetch that data in a way that results in a minimal number of database queries.
  • There are some unit tests for this in DataList which should be copied through to the EagerLoadedListTest class. this commit highlights most if not all of those.
    • Note that unit tests don't have to be limited to that coverage -but they should include at least that much.

PRs

@kinglozzer
Copy link
Member

I’ve been using https://github.com/gurucomkz/silverstripe-eagerloading/ in a project recently where I needed to eager load relations with a filter applied, e.g. product has_many attributes, eager load attributes where Featured = 1.

If we’re chaining like $item->EagerLoadedRelationList()->filter() then I think we’ll always end up having to filter in-memory, as the queries have all been issued by the time we reach that point. Ideally we’d have the filters ready ahead of time so that the eager loaded data is filtered/sorted by the database query.

Do you think an API like this would be possible?

$products = $products->eagerLoad('Attributes', function (EagerLoadedDataList $attributes) {
    return $attributes->filter('Featured', true)->sort('Title', 'ASC');
});

I’m not sure how it would work for chained relations, I think it’d probably have to be something like:

$groups = $groups->eagerLoad('Members.FavouriteProducts', function (EagerLoadedDataList $products) {
    // $products would be favourited products for *all* members in $groups
});

$groups = $groups->eagerLoad('Members', function (EagerLoadedDataList $members) {
    return $members->filter('FirstName', 'Bob');
})->eagerLoad('Members.FavouriteProducts', function (EagerLoadedDataList $products) {
    // $products would be favourited products for only members named Bob in $groups
});

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jul 19, 2023

That sounds really complex, not just to implement but also to use... That sort of thing could be possible down the line, but I don't think it's in scope for the current iteration.

That said, if you take a look at what's been implemented so far in both the linked pr and the eager loading that's already on the 5 branch and you see a clear way forward, then it could definitely be worth exploring.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jul 19, 2023

Just to be a little bit more specific about why the recommended additional API would be out of scope for this iteration: Regardless of whether we have some mechanism for pre-sorting and pre-filtering, the new EagerLoadedDataList should ideally be plug-and-play (for the most part) with the existing relation lists - which means it will still need some mechanism for filtering and sorting. So the idea is to get that list implementation done, and then look for additional improvements afterward (or asynchronously, but that requires more people to be working on it :p)

@GuySartorelli GuySartorelli changed the title EagerLoadedRelationList can't filter or sort by fields on its relations EagerLoadedDataList can't filter or sort by fields on its relations Jul 20, 2023
@kinglozzer
Copy link
Member

That all makes sense, I appreciate that you’re actually doing the work here and I’m just spouting ideas 😛

@GuySartorelli GuySartorelli added this to the Silverstripe CMS 5.1 milestone Jul 21, 2023
@GuySartorelli GuySartorelli changed the title EagerLoadedDataList can't filter or sort by fields on its relations EagerLoadedList can't filter or sort by fields on its relations Aug 29, 2023
@GuySartorelli GuySartorelli self-assigned this Oct 12, 2023
@GuySartorelli
Copy link
Member Author

I've come to the conclusion that trying to filter by relations post-eagerloading is not feasible in a way that isn't likely to blow away any optimisation the eager loading provides.
I'll add explicit tests to show we throw exceptions when it is attempted, and people can use #10929 instead once that's been implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants