EagerLoad error when eager loaded relationship is empty

sig-peggy commented 5 months ago

Module version(s) affected

5.2.2

Description

I've been trying to follow the instructions on https://docs.silverstripe.org/en/5/developer_guides/model/relations/#eager-loading about using the 5.2 updates for eager loading and I'm hitting errors if the eager loaded list is empty.

When I downgrade back to silverstripe/framework 5.1.23 this error goes away. Let me know if I've just missed something in how these newer eager loading functions are meant to be used.

How to reproduce

When eager loading a many_many relationship on a has_many relationship (as per the Team/Players example in the documentation)

$items = $this->HasManyRelationship()->eagerLoad('ManyManyRelationship');
foreach ($items as $item) {
    //does not matter what is in here, the error is on the foreach line
}

This works correctly if there are any many_many results to be loaded, but if the many_many relationship is empty for all of the has_many items in question then I get the following error: [Emergency] Uncaught SilverStripe\ORM\Connect\DatabaseException: Couldn't run query: SELECT * FROM "HasOneRelationship_ManyManyRelationship" WHERE "HasOneRelationshipID" IN (3,4) AND ManyManyRelationshipID IN () ORDER BY FIELD(ManyManyRelationshipID, ) You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ') ORDER BY FIELD(ManyManyRelationshipID, )' at line 1

The only solution I've been able to find to work around this is to use a $items->filter('ManyManyRelationship.ID:not', null)->count() to make sure there is anything in the eager loaded list before trying to do the foreach, but this becomes problematic when there is more than one relationship that I want to eager load.

Full error stack trace

Source ``` 54 if (!empty($sql)) { 55 $formatter = new SQLFormatter(); 56 $formattedSQL = $formatter->formatPlain($sql); 57 $msg = "Couldn't run query:\n\n{$formattedSQL}\n\n{$msg}"; 58 } 59 60 if ($errorLevel === E_USER_ERROR) { 61 // Treating errors as exceptions better allows for responding to errors 62 // in code, such as credential checking during installation 63 throw new DatabaseException($msg, 0, null, $sql, $parameters); 64 } else { 65 user_error($msg ?? '', $errorLevel ?? 0); 66 } 67 } 68 69 /** ``` Trace SilverStripe\ORM\Connect\DBConnector->databaseError MySQLiConnector.php:194 SilverStripe\ORM\Connect\MySQLiConnector->query Database.php:159 SilverStripe\ORM\Connect\Database->SilverStripe\ORM\Connect\{closure} Database.php:258 SilverStripe\ORM\Connect\Database->benchmarkQuery Database.php:156 SilverStripe\ORM\Connect\Database->query MySQLDatabase.php:415 SilverStripe\ORM\Connect\MySQLDatabase->query DB.php:339 SilverStripe\ORM\DB::query DataList.php:1326 SilverStripe\ORM\DataList->fetchEagerLoadManyMany DataList.php:1091 SilverStripe\ORM\DataList->fetchEagerLoadRelations DataList.php:1029 SilverStripe\ORM\DataList->executeQuery DataList.php:967 SilverStripe\ORM\DataList->getFinalisedQuery DataList.php:947 SilverStripe\ORM\DataList->getIterator Page.php:34 Page->getEagerRelationship CustomMethods.php:85 SilverStripe\View\ViewableData->__call ViewableData.php:142 SilverStripe\View\ViewableData->__get ViewableData.php:558 SilverStripe\View\ViewableData->obj ViewableData.php:618 SilverStripe\View\ViewableData->XML_val SSViewer_Scope.php:342 SilverStripe\View\SSViewer_Scope->__call SSViewer_DataPresenter.php:327 SilverStripe\View\SSViewer_DataPresenter->__call .cachethemes.simple.templates.Page.ss:109 include(/tmp/silverstripe-cache-php8.2.17-home-webspace-scratch-peggy-site/www-data/.cachethemes.simple.templates.Page.ss) SSViewer.php:576 SilverStripe\View\SSViewer->includeGeneratedTemplate SSViewer.php:648 SilverStripe\View\SSViewer->process Controller.php:290 SilverStripe\Control\Controller->handleAction RequestHandler.php:200 SilverStripe\Control\RequestHandler->handleRequest Controller.php:200 SilverStripe\Control\Controller->handleRequest ContentController.php:219 SilverStripe\CMS\Controllers\ContentController->handleRequest ModelAsController.php:91 SilverStripe\CMS\Controllers\ModelAsController->handleRequest Director.php:348 SilverStripe\Control\Director->SilverStripe\Control\{closure} VersionedHTTPMiddleware.php:41 SilverStripe\Versioned\VersionedHTTPMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} LoginSessionMiddleware.php:56 SilverStripe\SessionManager\Middleware\LoginSessionMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} ConfirmationMiddleware.php:254 SilverStripe\Control\Middleware\ConfirmationMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} ConfirmationMiddleware.php:254 SilverStripe\Control\Middleware\ConfirmationMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} ExecMetricMiddleware.php:20 SilverStripe\Control\Middleware\ExecMetricMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} PasswordExpirationMiddleware.php:84 SilverStripe\Security\PasswordExpirationMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} BasicAuthMiddleware.php:68 SilverStripe\Security\BasicAuthMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} AuthenticationMiddleware.php:61 SilverStripe\Security\AuthenticationMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} CanonicalURLMiddleware.php:245 SilverStripe\Control\Middleware\CanonicalURLMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} HTTPCacheControlMiddleware.php:39 SilverStripe\Control\Middleware\HTTPCacheControlMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} ChangeDetectionMiddleware.php:28 SilverStripe\Control\Middleware\ChangeDetectionMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} FlushMiddleware.php:30 SilverStripe\Control\Middleware\FlushMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} SessionMiddleware.php:20 SilverStripe\Control\Middleware\SessionMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} AllowedHostsMiddleware.php:60 SilverStripe\Control\Middleware\AllowedHostsMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} TrustedProxyMiddleware.php:176 SilverStripe\Control\Middleware\TrustedProxyMiddleware->process HTTPMiddlewareAware.php:62 SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure} HTTPMiddlewareAware.php:65 SilverStripe\Control\Director->callMiddleware Director.php:357 SilverStripe\Control\Director->handleRequest HTTPApplication.php:114 SilverStripe\Control\HTTPApplication::SilverStripe\Control\{closure} call_user_func HTTPApplication.php:137 SilverStripe\Control\HTTPApplication->SilverStripe\Control\{closure} HTTPMiddlewareAware.php:65 SilverStripe\Control\HTTPApplication->callMiddleware HTTPApplication.php:130 SilverStripe\Control\HTTPApplication->execute HTTPApplication.php:113 SilverStripe\Control\HTTPApplication->handle index.php:24

Possible Solution

No response

Additional Context

No response

Validations

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

PRs

https://github.com/silverstripe/silverstripe-framework/pull/11220

beerbohmdo commented 5 months ago

A bit offtopic, but if somebody refactor this: FIELD() is a mysql method and does not work with other databases. (And please quote the field name)

GuySartorelli commented 4 months ago

@beerbohmdo If what you're talking about is a separate bug or feature request, please open a separate issue about it. It doesn't seem like it's related to the bug described in this issue.

GuySartorelli commented 4 months ago

git bisect shows that the first commit that surfaces this bug is 528344d1b0c9d02787aed0acca42b022e8c14b98 which was in https://github.com/silverstripe/silverstripe-framework/pull/11140

GuySartorelli commented 4 months ago

This was a result of making the query for join records more efficient - but not considering the case where there are no child records to join to. PR incoming.

emteknetnz commented 4 months ago

Linked PR has been merged, it will be automatically tagged shortly

silverstripe / silverstripe-framework