eagerLoad crashes with unhelpful errors when provided a 'hasOne.hasMany' relation chain

[MasonD]

Module version(s) affected

5

Description

When provided with a relation chain that has a has one followed by a has many, eagerLoad will return successfully. However, when read, the DataList cause some (ignorable depending on php config) warnings for array to string conversion and undefined array key accesses before crashing with Cannot bind parameter "" as it is an unsupported type ()

I've tracked down the cause of this to be the fact that DataList::fetchEagerLoadHasOne's return value is in a different form from the other fetchEagerLoad relation methods. The second value in the return tuple, $fetchedIDs is a map of [$className => $idList] while all other relation methods return a flat ID array. fetchEagerLoadRelations and the following relations in the relation chain all assume that fetchEagerLoadHasOne's return id list is a flat list and pass this value directly as a value in a DataList::filter like DataList->filter([$idField => $parentIDs]).

How to reproduce

Using only framework DataObjects, it's possible to trigger this bug like so:

$list = MemberPassword::get()->eagerLoad(['Member.RememberLoginHashes']);
Debug::dump('the above causes no error');
$list->toArray();
Debug::dump('never reach here');

Possible Solution

The complication here is that fetchEagerLoadHasOne is written the way it is in order to handle polymorphic hasOne relations. I'm happy to write a PR, but first I'd like confirmation on the behaviour I should aim for. Should eagerLoad throw an exception if a relation chain includes a nonterminal polymorphic hasOne, and fetchEagerLoadHasOne return a flattened list of IDs like the rest of the eager load methods if the relation isn't polymorphic?

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

• FIX eagerLoad crash with nonterminal hasOne relation #11483

[GuySartorelli]

What do you mean by a "nonterminal" polymorphic hasOne?

[GuySartorelli]

Should eagerLoad throw an exception if a relation chain includes a nonterminal polymorphic hasOne

Probably not, but it depends what "nonterminal" means in this context

[should] fetchEagerLoadHasOne return a flattened list of IDs like the rest of the eager load methods if the relation isn't polymorphic?

I think that's missing the trick here, which is that it probably shouldn't matter if the has_one is polymorphic or not. We know what classes the records in those has_ones belong to, so if $parentIDs is associative (ArrayLib::is_associative($parentIDs)), we should then be eagerly fetching the has_many (or many_many if it was a many_many) records for the classes that do have a relation with that name, so long as at least one of those classes does have such a relation.

Let me know if I'm missing something here.

[MasonD]

@GuySartorelli So, what I mean by nonterminal polymorphic hasOne is if the relation chain continues on after the hasOne relation (e.g. 'polymorphicHasOne.someFurtherRelation'. My biggest hesitancy towards supporting such is that since a polymorphic relation is to any dataobject, the behaviour of such an eagerLoad would be fraught with potential errors.

Either we would have to support a relation in the chain just not existing (not all your dataobjects will have a someFurtherRelation relation) and silently not eager load anything (bad DX imo) or using eagerLoad with a polymorphic hasOne followed by another relation would throw conditionally if any of the related objects' classes don't have relations named the same thing, which sounds like a very good way for someone to accidentally shoot themself in the foot and not notice during testing.

[GuySartorelli]

Either we would have to support a relation in the chain just not existing (not all your dataobjects will have a someFurtherRelation relation) and silently not eager load anything (bad DX imo)

This is what I would recommend we do - I'm interested in why you think this is bad DX, as opposed to just throwing an exception even though the relation we want to eagerly load does exist on at least one of the has_one classes?

[MasonD]

Because it's inconsistent with how it currently works when there isn't a polymorphic relation, and it's more opaque to the developer whether the method is doing something or not. Did it work or is it just silently failing? I guess I'll go add ?showqueries to the page and hunt through them to see whether or not it's working. And if you misspell a relation, eagerLoad would silently pass it by without catching the mistake.

I wouldn't argue that it's worse behaviour than throwing: you're right there, I wasn't thinking it through properly when I considered throwing.

[GuySartorelli]

Because it's inconsistent with how it currently works when there isn't a polymorphic relation

If I remember correctly, right now without a polymorphic relation, if the has_one class doesn't have the someFurtherRelation relation, an exception is thrown - but if it does have that relation, the records are loaded. Or that would happen if the bug this issue points out didn't happen.

I'm recommending that still be true, even for polymorphic relations. If the relation exists (on at least one of the classes in the has_one) then it's clear the developer was intending to load the records in that relation. If none of those classes have the has_one, the behaviour should be the same as when that happens without a polymorphic relation.

That does depend on records being in the database though, so I guess the bad DX is that if there are no records in the relation then we'd get an exception, even though there's a possible scenario for it to not throw an exception if there are records in the database. So that's not ideal.

Maybe throwing an exception is better, given that? So eagerLoad('polymorphicHasOne.someFurtherRelation') would throw an exception that says something like "Cannot chain eager loaded relations after a polymorphic has_one"?

Alternatively, what would you recommend?

[MasonD]

Yeah, the behaviour of throwing/not throwing depending on what's in the database was what I was alluding to with "shoot themself in the foot and not notice during testing", I just didn't communicate it well enough. Any throwing would be too inconsistent to be very useful to the developer.

Maybe throwing an exception is better, given that? So eagerLoad('polymorphicHasOne.someFurtherRelation') would throw an exception that says something like "Cannot chain eager loaded relations after a polymorphic has_one"?

This is the implementation I settled on for our internal patch while opening this issue, partially because it's also the easiest to implement and we didn't need polymorphic functionality. I don't really have any other idea of how to solve this except for not throwing at all on unknown relations after a polymorphic hasOne has been encountered, which has the issues I mentioned above.

I think disallowing further relations after a polymorhic has_one with a more helpful exception than the current crash is the best solution, but I'm willing to have a go at implementing extra support for polymorphic has_ones if you think the extra complexity is worth it. Maybe with an added warning in the docblock about how polymorphic has_ones have the new gotcha.

[GuySartorelli]

I think disallowing further relations after a polymorhic has_one with a more helpful exception than the current crash is the best solution

Sounds like the way to go, now that I've had some time to think about it more thoroughly and bounce my thoughts off you. Thanks for your patience.

I'd be happy for you to make a PR with that solution. We can always implement additional polymorphic support in future minor releases if someone has a good idea for how to go about it.