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

Eagerly loaded indexed associations use hydrated values instead of database values #11185

Closed
wants to merge 5 commits into from

Conversation

wmouwen
Copy link

@wmouwen wmouwen commented Jan 25, 2024

Description

Update: bug appears to exists in 3.0.x as well

There is a breaking change between 2.16.x and 2.17.x where for eagerly loaded collections indexBy started looking at the hydrated set of values instead of the raw values returned from the database. This broke setups for all eager associations using either non-scalar fields (Uuid, other entities) or associations where the inverse field's database name didn't match the property name.

Affected versions

  • 2.17
  • 2.18
  • 2.19
  • 3.0
  • 3.1

Caused by

Issues

Related

@wmouwen wmouwen changed the title [2.17.x] Eager load indexed associations with non-scalar value [2.17.x] Eager load indexed associations uses hydrated values instead of database values Jan 25, 2024
@wmouwen wmouwen changed the title [2.17.x] Eager load indexed associations uses hydrated values instead of database values [2.17.x] Eagerly loaded indexed associations uses hydrated values instead of database values Jan 25, 2024
@wmouwen wmouwen changed the title [2.17.x] Eagerly loaded indexed associations uses hydrated values instead of database values [2.17.x] Eagerly loaded indexed associations use hydrated values instead of database values Jan 25, 2024
@wmouwen
Copy link
Author

wmouwen commented Jan 25, 2024

In order to load collections in a same way as other spots, I've chosen to reuse the existing method \Doctrine\ORM\UnitOfWork::loadCollection. This way the functionality of eager loaded collections is in line with both older versions (<= 2.16.x) and non-eager loads.

@greg0ire
Copy link
Member

In case you want to run stuff locally, you can take a look at this guide.

@wmouwen
Copy link
Author

wmouwen commented Jan 25, 2024

In case you want to run stuff locally, you can take a look at this guide.

That's the fun part; it works on my machine, and the reason why the test EagerFetchCollectionTest has been altered. Apparently the values between my machine (PHP 8.3) and the failing GitHub actions (PHP 7.2) differ, which isn't weird considering that the eager fetch logic has been altered. Guess the next step is to have a good look at whether EagerFetchCollectionTest is still valid.

@wmouwen
Copy link
Author

wmouwen commented Jan 26, 2024

Forcing a run on PHP 7.2.34 with the same settings as the failing GitHub action (link), still passes the tests.

> docker run -it -v "$PWD":/app -w /app --rm php:7.2-cli vendor/bin/phpunit -c ci/github/phpunit/pdo_sqlite.xml --exclude-group performance,non-cacheable,locking_functional tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php
PHPUnit 8.5.35 by Sebastian Bergmann and contributors.


Using DB driver Doctrine\DBAL\Driver\PDO\SQLite\Driver
....                                                                4 / 4 (100%)

Time: 54 ms, Memory: 12.00 MB

OK (4 tests, 12 assertions)

I'm a bit clueless as to what is causing the difference in behavior. I have chosen to disable parts of the broken tests, that way we can see if it is the only class to fail and narrow down the search.

@wmouwen wmouwen requested a review from greg0ire January 29, 2024 15:48
@greg0ire
Copy link
Member

greg0ire commented Jan 29, 2024

@wmouwen I just had a hunch that your issue was caused by the second level cache. If you look more closely at the failing action in your link above, you will notice that there's a successful "Run PHPUnit" step, followed by a failing "Run PHPUnit with Second Level Cache". You should be able to reproduce the issue with ENABLE_SECOND_LEVEL_CACHE=1 vendor/bin/phpunit.

@wmouwen wmouwen marked this pull request as draft February 1, 2024 08:20
@wmouwen
Copy link
Author

wmouwen commented Feb 1, 2024

@greg0ire I'm failing to find a way to get the raw database values of the correct indexBy column in \Doctrine\ORM\UnitOfWork::eagerLoadCollections.

Secondly, #11163 has been created for a different problem caused by the same PR #8391. As #8391 was an optimization, I would suggest reverting #8391 for now until a new optimization can be written without breaking changes.

cc @beberlei

@wmouwen wmouwen changed the title [2.17.x] Eagerly loaded indexed associations use hydrated values instead of database values Eagerly loaded indexed associations use hydrated values instead of database values Feb 16, 2024
@wmouwen wmouwen changed the base branch from 2.17.x to 3.0.x February 16, 2024 22:22
@wmouwen wmouwen marked this pull request as ready for review February 17, 2024 08:43
@wmouwen
Copy link
Author

wmouwen commented Feb 17, 2024

Updated the test for 3.0.x and rebased. Still couldn't find a solution except for reverting the original changes and incurring a performance loss, help is welcome.

@greg0ire
Copy link
Member

@mpdude , maybe you can help with this?

@wmouwen
Copy link
Author

wmouwen commented Feb 27, 2024

The below code fetches entities for associations of multiple entities at once.

$found = $this->getEntityPersister($targetEntity)->loadAll([$mappedBy => $entities]);

We can't inject an 'indexBy' property here. Even if we could, the result would be ambiguous, as what is unique for one entity does not have to be unique across multiple entities.

The ->loadAll() call hydrates the entities. This causes the variable $indexByProperty a few lines below to try and find a non-existent column.

orm/src/UnitOfWork.php

Lines 2672 to 2674 in b187bc8

$indexByProperty = $targetClass->getReflectionProperty($mapping->indexBy);
assert($indexByProperty !== null);
$collectionBatch[$idHash]->hydrateSet($indexByProperty->getValue($targetValue), $targetValue);

The column locale_code in the testcases doesn't exist on the hydrated object.

There is no clear way to combine $targetClass (Doctrine\ORM\Mapping\ClassMetadata) and $mapping (Doctrine\ORM\Mapping\OneToManyAssociationMapping) to get the value for indexing from the hydrated object ($targetValue) given the database column name.

@beberlei @mpdude Does either of you know a way?

@beberlei
Copy link
Member

Superseded by #11380

@beberlei beberlei closed this Mar 19, 2024
@wmouwen wmouwen deleted the gh11149-eager-index-by branch March 19, 2024 07:21
derrabus pushed a commit to beberlei/doctrine2 that referenced this pull request Mar 20, 2024
derrabus pushed a commit that referenced this pull request Mar 20, 2024
derrabus added a commit that referenced this pull request Mar 21, 2024
* 2.19.x:
  [Documentation] Removing "Doctrine Mapping Types" ... (#11384)
  [GH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (#11376)
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 2.19.x:
  Remove unused variable (doctrine#11391)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 2.19.x:
  Add missing import
  Remove unused variable (doctrine#11391)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 2.19.x:
  Add missing import
  Remove unused variable (doctrine#11391)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 2.19.x:
  Add missing import
  Remove unused variable (doctrine#11391)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 2.19.x:
  Add missing import
  Remove unused variable (doctrine#11391)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 2.19.x:
  Set column length explicitly (doctrine#11393)
  Add missing import
  Remove unused variable (doctrine#11391)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 3.1.x:
  Adjust PHPBench mocks
  Set column length explicitly (doctrine#11393)
  Add missing import
  Remove unused variable (doctrine#11391)
  Fixed proxy initialization for EnumReflectionProperty
  Remove older versions from the docs (doctrine#11383)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
  Remove outdated git metadata files (doctrine#11362)
  Switch join columns around, otherwise index doesnt match
  Key on fk
  Fix entities and mapping.
  Minor code style fix in AbstractRemoteControl
  Do not schedule batch loading for target classes with composite identifier.
  Cleanup tests not to use model sets.
  provides a test case for github issue 11154
derrabus added a commit to derrabus/orm that referenced this pull request Mar 21, 2024
* 3.2.x:
  Adjust PHPBench mocks
  Set column length explicitly (doctrine#11393)
  Add missing import
  Remove unused variable (doctrine#11391)
  Fixed proxy initialization for EnumReflectionProperty
  Remove older versions from the docs (doctrine#11383)
  [Documentation] Removing "Doctrine Mapping Types" ... (doctrine#11384)
  [doctrineGH-11185] Bugfix: do not use collection batch loading for indexBy assocations. (doctrine#11380)
  Improve lazy ghost performance by avoiding self-referencing closure. (doctrine#11376)
  Remove outdated git metadata files (doctrine#11362)
  Switch join columns around, otherwise index doesnt match
  Key on fk
  Fix entities and mapping.
  Minor code style fix in AbstractRemoteControl
  Do not schedule batch loading for target classes with composite identifier.
  Cleanup tests not to use model sets.
  provides a test case for github issue 11154
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants