BUG Eager loading with multiple objects pointing to the same has_one trigger extra db fetch

[beerbohmdo]

Module version(s) affected

5.1.18

Description

The current implementation of eagerLoad does not allow that multiple objects have the same has_one relation.

If I have a relation like in the yaml file below, the eager query will load all required EagerLoadBars but will only pass the object into the last EagerLoadFoo object, all objects before will trigger an additional query to the database.

Is this wanted?

How to reproduce

<?php

namespace tests;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;
use SilverStripe\ORM\FieldType\DBVarchar;
use SilverStripe\ORM\HasManyList;

/**
 * @property string|null $Dummy
 * @method HasManyList<EagerLoadFoo> Foos()
 */
class EagerLoadBar extends DataObject implements TestOnly
{
    /**
     * @var array
     * @config
     */
    private static array $db = [
        'Dummy' => DBVarchar::class . '(255)',
    ];

    /**
     * @var array
     * @config
     */
    private static array $has_many = [
        'Foos'  => EagerLoadFoo::class,
    ];
}

<?php

namespace tests;

use SilverStripe\Dev\TestOnly;
use SilverStripe\ORM\DataObject;
use SilverStripe\ORM\FieldType\DBVarchar;

/**
 * @property string|null $Dummy
 * @property int $BarID
 * @method EagerLoadBar Bar()
 */
class EagerLoadFoo extends DataObject implements TestOnly
{
    /**
     * @var array
     * @config
     */
    private static array $db = [
        'Dummy' => DBVarchar::class . '(255)',
    ];

    /**
     * @var array
     * @config
     */
    private static array $has_one = [
        'Bar'   => EagerLoadBar::class,
    ];
}

tests\EagerLoadBar:
  bar1:
    Dummy: 1
  bar2:
    Dummy: 2
  bar3:
    Dummy: 3
  bar4:
    Dummy: 4

tests\EagerLoadFoo:
  foo1:
    Bar: =>tests\EagerLoadBar.bar1
  foo2:
    Bar: =>tests\EagerLoadBar.bar2
  foo3:
    Bar: =>tests\EagerLoadBar.bar1
  foo4:
    Bar: =>tests\EagerLoadBar.bar3
  foo5:
    Bar: =>tests\EagerLoadBar.bar2
  foo6:
    Bar: =>tests\EagerLoadBar.bar4

<?php

namespace tests;

use ReflectionProperty;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\ORM\DataObject;

class EagerLoadTest extends SapphireTest
{
    protected static $fixture_file = [
        'eager.yml',
    ];

    protected static $extra_dataobjects = [
        EagerLoadBar::class,
        EagerLoadFoo::class,
    ];

    public function testEagerLoading(): void
    {
        $fooNames = array_flip($this->allFixtureIDs(EagerLoadFoo::class));

        $list = EagerLoadFoo::get()->eagerLoad('Bar');

        $property = new ReflectionProperty(DataObject::class, 'eagerLoadedData');

        /** @var EagerLoadFoo $foo */
        foreach ($list as $foo) {
            $eagerLoadedData = $property->getValue($foo);

            $this->assertArrayHasKey('Bar', $eagerLoadedData, 'asserting that ' . ($fooNames[$foo->ID] ?? $foo->ID) . ' has loaded Bar');
            $this->assertEquals($foo->BarID, $eagerLoadedData['Bar']?->ID, 'asserting that ' . ($fooNames[$foo->ID] ?? $foo->ID) . ' has loaded Bar');
        }
    }
}

Possible Solution

Modify the DataList::fetchEagerLoadHasOne to store multiple IDs per Relation.

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/11185

[GuySartorelli]

PR merged. This will be automatically tagged by GitHub Actions.