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

Paginator won't take into account entity identifiers that use custom DBAL types #7820

Closed
Ocramius opened this issue Sep 17, 2019 · 2 comments
Assignees
Labels
Milestone

Comments

@Ocramius
Copy link
Member

Bug Report

Q A
BC Break no
Version 2.6.3

Summary

When using a Doctrine\ORM\Tools\Pagination\Paginator to iterate over a query that has entities with a custom DBAL type used in the identifier, then $id->__toString() is used implicitly by PDO, instead of being converted by the Doctrine\DBAL\Types system.

In order to reproduce this, you must have identifiers implementing #__toString() (to allow the UnitOfWork to hash them) and other accessors that are used by the custom DBAL type during DB/PHP conversions. If #__toString() and the DBAL type conversions are asymmetric, then the paginator will fail to find records.

Tricky situation, but this very much affects ramsey/uuid-doctrine and anyone relying on the uuid_binary.

Current behavior

class MyEntity {
    public TheId $id;
}

class TheId {
    public function toId() : string
    {
        return 'yep';
    }
    public function __toString() : string
    {
        return 'nope';
    }
}

class TheIdType extends StringType
{
    public function convertToDatabaseValue($value, $platform)
    {
        return $value->toId();
    }
}

$entity = new MyEntity();
$entity->id = new TheId();

$em->persist($entity);
$em->flush();

$paginator = (new Paginator($em->createQuery('SELECT f FROM ' . MyEntity::class . ' f'));

foreach ($paginator as $entry) {
    die('this won't be reached with current ORM state');
}

Expected behavior

The code above should loop over one found record.

@Ocramius Ocramius added the Bug label Sep 17, 2019
@Ocramius Ocramius added this to the 2.6.4 milestone Sep 17, 2019
@Ocramius Ocramius self-assigned this Sep 17, 2019
@lcobucci
Copy link
Member

Possibly related to #7025 and #4632

Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 17, 2019
… types

When using a `Doctrine\ORM\Tools\Pagination\Paginator` to iterate over a query that has entities with a custom DBAL type used in the identifier, then `$id->__toString()` is used implicitly by PDO, instead of being converted by the `Doctrine\DBAL\Types` system.

In order to reproduce this, you must have identifiers implementing `#__toString()` (to allow the `UnitOfWork` to hash them) and other accessors that are used by the custom DBAL type during DB/PHP conversions. If `#__toString()` and the DBAL type conversions are asymmetric, then the paginator will fail to find records.

Tricky situation, but this very much affects `ramsey/uuid-doctrine` and anyone relying on the `uuid_binary`.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 17, 2019
…efore binding parameters

This patch introduces new internal API on the `ResultSetMapping` class, which is responsible
for finding the type of the single column identifier of a DQL query selection root.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 17, 2019
…efore binding parameters

This patch introduces new internal API on the `ResultSetMapping` class, which is responsible
for finding the type of the single column identifier of a DQL query selection root.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 17, 2019
…efore binding parameters

This patch introduces new internal API on the `ResultSetMapping` class, which is responsible
for finding the type of the single column identifier of a DQL query selection root.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 17, 2019
…efore binding parameters

This patch introduces new internal API on the `ResultSetMapping` class, which is responsible
for finding the type of the single column identifier of a DQL query selection root.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 19, 2019
…in `WhereInWalker`

As per discussion with @lcobucci, it is better to keep dragons where
there be dragons, and this change does indeed rewrite the previous
approach by moving the responsibility of type conversion on a query
object from the `Paginator` to the `WhereInWalker`, which already
has access to class metadata for the root of the selection (and can
reliably detect the root of the selection too)
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 20, 2019
…tadataInfo#getTypeOfColumn()`

This method will always return `string|null`, so we can safely
remove DBAL types from its possible return types.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 20, 2019
…urn type

Array values are `string`, and the array is a packed array.
Ocramius added a commit to Ocramius/doctrine2 that referenced this issue Sep 20, 2019
This logic was pre-existing, but I forgot about it while writing
doctrine#7820, therefore it was re-implemented inside this unit of
code. Now we just use the `PersisterHelper`, which does all
the nice and shiny identifier type discovery operations we need.
lcobucci added a commit that referenced this issue Sep 20, 2019
…al-type-conversions-in-identifiers

Bug: #7820 paginator ignores dbal type conversions in identifiers
@lcobucci
Copy link
Member

Fixed by #7821

Voziv added a commit to ratehub/doctrine2 that referenced this issue Oct 22, 2019
v2.6.4

[![Build Status](https://travis-ci.org/doctrine/orm.svg?branch=v2.6.4)](https://travis-ci.org/doctrine/orm)

In this release we've fixes many bugs (including a performance regression) and
made the v2.x series compatible with PHP 7.4.

--------------------------------------------

- Total issues resolved: **11**
- Total pull requests resolved: **32**
- Total contributors: **30**

Improvement
-----------

 - [7785: Fix "access array offset on value of type null" PHP 7.4 notices](doctrine#7785) thanks to @mlocati
 - [7142: Rename this repository to doctrine/orm](doctrine#7142) thanks to @greg0ire

Bug
------------------

 - [7821: Bug: doctrine#7820 paginator ignores dbal type conversions in identifiers](doctrine#7821) thanks to @Ocramius
 - [7778: Guard L2C regions against corrupted data](doctrine#7778) thanks to @umpirsky
 - [7767: PersistentCollection::matching() does not respect the collections native sorting](doctrine#7767) thanks to @stephanschuler
 - [7766: Respect collection orderBy meta when matching()](doctrine#7766) thanks to @stephanschuler
 - [7761: Do not modify UOW on PersistentCollection::clear() when owner has DEFFERED&doctrine#95;EXPLICIT change tracking policy](doctrine#7761) thanks to @paxal
 - [7750: Fix incorrect return of null values in L2C](doctrine#7750) thanks to @AlexSmerw
 - [7737: Fix MEMBER&doctrine#95;OF comparison when using criteria in query builder](doctrine#7737) thanks to @Smartel1
 - [7735: Null in fields value in Cached Entity several times on day on high-load project.](doctrine#7735) thanks to @AlexSmerw
 - [7630: Fix doctrine#7629 - `scheduledForSynchronization` leaks memory when using `@ORM\ChangeTrackingPolicy("DEFERRED&doctrine#95;EXPLICIT")`](doctrine#7630) thanks to @yethee
 - [7528: Prevent `UnitOfWork` lookup for DBAL types specified in `Doctrine\ORM\Query#setParameter()`](doctrine#7528) thanks to @Ocramius
 - [7322: JoinedSubclassPersister pass identifier types on delete](doctrine#7322) thanks to @dennisenderink and @fred-jan
 - [7266: Call to a member function resolveAssociationEntries() on boolean {"detail":"&doctrine#91;object&doctrine#93; (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Call to a member function resolveAssociationEntries() on boolean at /www/vendor/doctrine/orm/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:140)"}](doctrine#7266) thanks to @mingmingxianseng
 - [4632: DDC-3789: Paginator does not convert entity ids if they are value objects](doctrine#4632) thanks to @doctrinebot

Documentation
-------------

 - [7818: Add note into docs about not using SimpleAnnotationReader](doctrine#7818) thanks to @SenseException
 - [7791: Fix preFlush event documentation stating incorrectly that flush can be called safely](doctrine#7791) thanks to @Steveb-p
 - [7753: Add ORM annotations in getting-started docs](doctrine#7753) thanks to @SenseException and @wajdijurry
 - [7744: Fixed a typo-error](doctrine#7744) thanks to @noobshow
 - [7732: &doctrine#91;Documentation&doctrine#93; Missing comma fix](doctrine#7732) thanks to @lchrusciel
 - [7729: Update DATE&doctrine#95;ADD and DATE&doctrine#95;SUB docs](doctrine#7729) thanks to @JoppeDC
 - [7672: Added cross-links to relevant documentation](doctrine#7672) thanks to @jschaedl
 - [7612: Update ordered-associations.rst](doctrine#7612) thanks to @spirlici
 - [7610: Change APC to OPcache in improving-performance.rst ](doctrine#7610) thanks to @smtchahal
 - [7596: Correct method names and broken link in docs](doctrine#7596) thanks to @mbessolov
 - [7577: Fix of single link to dbal docs in advanced-configuration.rst](doctrine#7577) thanks to @SenseException
 - [7572: Remove codeigniter Framework example](doctrine#7572) thanks to @SenseException
 - [7571: Fix typo in inheritance mappings docs](doctrine#7571) thanks to @batwolf
 - [7557: Change Stackoverflow tag to doctrine-orm](doctrine#7557) thanks to @malarzm
 - [7551: &doctrine#91;2.6&doctrine#93; Migrate repository name doctrine/doctrine2 -> doctrine/orm](doctrine#7551) thanks to @Majkl578
 - [7530: Documentation error typo fix: s/Used-defined/User-Defined](doctrine#7530) thanks to @vladyslavstartsev
 - [7519: doctrine#7518 Fixed type mismatch between `EntityRepository#&doctrine#95;&doctrine#95;construct()` and its documented constructor arguments](doctrine#7519) thanks to @koftikes
 - [7518: `EntityRepository::&doctrine#95;&doctrine#95;construct()` expects `Doctrine\ORM\EntityManager` instead of actual required `EntityManagerInterface`](doctrine#7518) thanks to @koftikes
 - [7490: Fix broken link](doctrine#7490) thanks to @vladyslavstartsev
 - [7483: Fixed a minor syntax issue](doctrine#7483) thanks to @javiereguiluz

CI
-----------------

 - [7794: Fix test compatibility with DBAL 2.10.x-dev](doctrine#7794) thanks to @lcobucci
 - [7731: Replace custom install script with add-on](doctrine#7731) thanks to @greg0ire
 - [7473: Incremental CS checks in 2.x branches](doctrine#7473) thanks to @Majkl578
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 22, 2023
This PR prevents the Paginator from causing OpCache "wasted memory" to increase _on every request_ when used with Symfony's `PhpFilesAdapter` as the cache implementation for the query cache.

Depending on configured thresholds, wasted memory this will either cause periodic opcache restarts or running out of memory and not being able to cache additional scripts ([Details](https://tideways.com/profiler/blog/fine-tune-your-opcache-configuration-to-avoid-caching-suprises)).

Fixes doctrine#9917, closes doctrine#10095.

There is a long story (doctrine#7820, doctrine#7821, doctrine#7837, doctrine#7865) behind how the Paginator can take care of DBAL type conversions when creating the pagination query. This conversion has to transform identifier values before they will be used as a query parameter, so it has to happen every time the Paginator is used.

For reasons, this conversion happens inside `WhereInWalker`. Tree walkers like this are used only during the DQL parsing/AST processing steps. Having a DQL query in the query cache short-cuts this step by fetching the parsing/processing result from the cache.

So, to make sure the conversion happens also with the query cache being enabled, this line

https://github.com/doctrine/orm/blob/1753d035005c1125c9fb4855c3fa629341e5734d/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L165

was added in doctrine#7837. It causes `\Doctrine\ORM\Query::parse()` to re-parse the query every time, but will also put the result into the query cache afterwards.

At this point, the setup described in doctrine#9917 – which, to my knowledge, is the default in Symfony + DoctrineBundle projects – will ultimately bring us to this code:

https://github.com/symfony/symfony/blob/4b3391725f2fc4a072e776974f00a992cbc70515/src/Symfony/Component/Cache/Adapter/PhpFilesAdapter.php#L248-L249

When writing a cache item with an already existing key, the driver has to make sure the opcache will honor the changed PHP file. This is what causes _wasted memory_ to increase.

Instead of using `\Doctrine\ORM\Query::expireQueryCache()`, which will force `\Doctrine\ORM\Query::parse()` to parse the query again before putting it into the cache, use `\Doctrine\ORM\Query::useQueryCache(false)`. The subtle difference is the latter will not place the processed query in the cache in the first place.

A test case is added to check that repeated use of the paginator does not call the cache to update existing keys. That should suffice to make sure we're not running into the issue, while at the same time not complicating tests by using the `PhpFilesAdapter` directly.

Note that in order to observe the described issue in tests, you will need to use the `PhpFilesDriver` and also make sure that OpCache is enabled and also activated for `php-cli` (which is running the unit tests).

This particular subquery generated/used by the Paginator is not put into the query cache. The DQL parsing/to-SQL conversion has to happen _every time_ the Paginator is used.

This, however, was already the case before this PR. In other words, this PR only changes that we do not store/update the cached result every time, but instead completely omit caching the query.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2, id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (doctrine#7820, fixed in doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type class for the root entity's identifier, and we have to derive it from a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so doctrine#7821 placed the conversion code in the `WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the paginator is run, but the query that results from running `WhereInWalker` would be kept in the query cache. This was reported in doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL re-parsed and transformed in every request. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

This PR moves the code that determines the DBAL type out of `WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is that `RootTypeWalker` results can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of the necessary conversions by itself. This happens every time the Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since it no longer has side effects.
derrabus pushed a commit that referenced this issue Jan 23, 2023
This PR prevents the Paginator from causing OpCache "wasted memory" to increase _on every request_ when used with Symfony's `PhpFilesAdapter` as the cache implementation for the query cache.

Depending on configured thresholds, wasted memory this will either cause periodic opcache restarts or running out of memory and not being able to cache additional scripts ([Details](https://tideways.com/profiler/blog/fine-tune-your-opcache-configuration-to-avoid-caching-suprises)).

Fixes #9917, closes #10095.

There is a long story (#7820, #7821, #7837, #7865) behind how the Paginator can take care of DBAL type conversions when creating the pagination query. This conversion has to transform identifier values before they will be used as a query parameter, so it has to happen every time the Paginator is used.

For reasons, this conversion happens inside `WhereInWalker`. Tree walkers like this are used only during the DQL parsing/AST processing steps. Having a DQL query in the query cache short-cuts this step by fetching the parsing/processing result from the cache.

So, to make sure the conversion happens also with the query cache being enabled, this line

https://github.com/doctrine/orm/blob/1753d035005c1125c9fb4855c3fa629341e5734d/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L165

was added in #7837. It causes `\Doctrine\ORM\Query::parse()` to re-parse the query every time, but will also put the result into the query cache afterwards.

At this point, the setup described in #9917 – which, to my knowledge, is the default in Symfony + DoctrineBundle projects – will ultimately bring us to this code:

https://github.com/symfony/symfony/blob/4b3391725f2fc4a072e776974f00a992cbc70515/src/Symfony/Component/Cache/Adapter/PhpFilesAdapter.php#L248-L249

When writing a cache item with an already existing key, the driver has to make sure the opcache will honor the changed PHP file. This is what causes _wasted memory_ to increase.

Instead of using `\Doctrine\ORM\Query::expireQueryCache()`, which will force `\Doctrine\ORM\Query::parse()` to parse the query again before putting it into the cache, use `\Doctrine\ORM\Query::useQueryCache(false)`. The subtle difference is the latter will not place the processed query in the cache in the first place.

A test case is added to check that repeated use of the paginator does not call the cache to update existing keys. That should suffice to make sure we're not running into the issue, while at the same time not complicating tests by using the `PhpFilesAdapter` directly.

Note that in order to observe the described issue in tests, you will need to use the `PhpFilesDriver` and also make sure that OpCache is enabled and also activated for `php-cli` (which is running the unit tests).

This particular subquery generated/used by the Paginator is not put into the query cache. The DQL parsing/to-SQL conversion has to happen _every time_ the Paginator is used.

This, however, was already the case before this PR. In other words, this PR only changes that we do not store/update the cached result every time, but instead completely omit caching the query.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2, id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (doctrine#7820, fixed in doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type class for the root entity's identifier, and we have to derive it from a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so doctrine#7821 placed the conversion code in the `WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the paginator is run, but the query that results from running `WhereInWalker` would be kept in the query cache. This was reported in doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL re-parsed and transformed in every request. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

This PR moves the code that determines the DBAL type out of `WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is that `RootTypeWalker` results can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of the necessary conversions by itself. This happens every time the Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since it no longer has side effects.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2, id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (doctrine#7820, fixed in doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type class for the root entity's identifier, and we have to derive it from a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so doctrine#7821 placed the conversion code in the `WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the paginator is run, but the query that results from running `WhereInWalker` would be kept in the query cache. This was reported in doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL re-parsed and transformed in every request. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

This PR moves the code that determines the DBAL type out of `WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is that `RootTypeWalker` results can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of the necessary conversions by itself. This happens every time the Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since it no longer has side effects.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2, id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (doctrine#7820, fixed in doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type class for the root entity's identifier, and we have to derive it from a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so doctrine#7821 placed the conversion code in the `WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the paginator is run, but the query that results from running `WhereInWalker` would be kept in the query cache. This was reported in doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL re-parsed and transformed in every request. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

This PR moves the code that determines the DBAL type out of `WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is that `RootTypeWalker` results can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of the necessary conversions by itself. This happens every time the Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since it no longer has side effects.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2, id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (doctrine#7820, fixed in doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type class for the root entity's identifier, and we have to derive it from a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so doctrine#7821 placed the conversion code in the `WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the paginator is run, but the query that results from running `WhereInWalker` would be kept in the query cache. This was reported in doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL re-parsed and transformed in every request. In addition to the processing overhead, this may also waste opcache memory (doctrine#9917).

This PR moves the code that determines the DBAL type out of `WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is that `RootTypeWalker` results can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of the necessary conversions by itself. This happens every time the Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since it no longer has side effects.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jan 23, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2, id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual result limiting, it has to take DBAL type conversions for the identifier column of the paginated root entity into account (doctrine#7820, fixed in doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type class for the root entity's `#[Id]`, and we have to figure it out based on a given (arbitrary) DQL query. This requires DQL parsing and inspecting the AST, so doctrine#7821 placed the conversion code in the `WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the paginator is run, but the query that results from running `WhereInWalker` would be kept in the query cache. This was reported in doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every time. The query must not be cached, since the necessary ID type conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL re-parsed and transformed in every request.

This PR moves the code that determines the DBAL type out of `WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It sets the type as the resulting "SQL" string. The benefit is that `RootTypeWalker` results can be cached in the query cache themselves. Only the first time a given DQL query has to be paginated, we need to run this walker to find out the root entity's ID type. After that, the type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of the necessary conversions by itself. This happens every time the Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since it no longer has side effects.
greg0ire pushed a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2,
id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual
result limiting, it has to take DBAL type conversions for the identifier
column of the paginated root entity into account (doctrine#7820, fixed in
 doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type
class for the root entity's `#[Id]`, and we have to figure it out based
on a given (arbitrary) DQL query. This requires DQL parsing and
inspecting the AST, so doctrine#7821 placed the conversion code in the
`WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the
paginator is run, but the query that results from running
`WhereInWalker` would be kept in the query cache. This was reported in
 doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every
time. The query must not be cached, since the necessary ID type
conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL
re-parsed and transformed in every request.

This PR moves the code that determines the DBAL type out of
`WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It
sets the type as the resulting "SQL" string. The benefit is that
`RootTypeWalker` results can be cached in the query cache themselves.
Only the first time a given DQL query has to be paginated, we need to
run this walker to find out the root entity's ID type. After that, the
type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of
the necessary conversions by itself. This happens every time the
Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since
it no longer has side effects.
Gwemox pushed a commit to Gwemox/orm that referenced this issue Feb 10, 2023
Make the `Paginator`-internal query (`... WHERE ... IN (id, id2,
id3...)`) cacheable in the query cache again.

When the Paginator creates the internal subquery that does the actual
result limiting, it has to take DBAL type conversions for the identifier
column of the paginated root entity into account (doctrine#7820, fixed in
 doctrine#7821).

In order to perform this type conversion, we need to know the DBAL type
class for the root entity's `#[Id]`, and we have to figure it out based
on a given (arbitrary) DQL query. This requires DQL parsing and
inspecting the AST, so doctrine#7821 placed the conversion code in the
`WhereInWalker` where all the necessary information is available.

The problem is that type conversion has to happen every time the
paginator is run, but the query that results from running
`WhereInWalker` would be kept in the query cache. This was reported in
 doctrine#7837 and fixed by doctrine#7865, by making this particular query expire every
time. The query must not be cached, since the necessary ID type
conversion happens as a side-effect of running the `WhereInWalker`.

The Paginator internal query that uses `WhereInWalker` has its DQL
re-parsed and transformed in every request.

This PR moves the code that determines the DBAL type out of
`WhereInWalker` into a dedicated SQL walker class, `RootTypeWalker`.

`RootTypeWalker` uses a ~hack~  clever trick to report the type back: It
sets the type as the resulting "SQL" string. The benefit is that
`RootTypeWalker` results can be cached in the query cache themselves.
Only the first time a given DQL query has to be paginated, we need to
run this walker to find out the root entity's ID type. After that, the
type will be returned from the query cache.

With the type information being provided, `Paginator` can take care of
the necessary conversions by itself. This happens every time the
Paginator is used.

The internal query that uses `WhereInWalker` can be cached again since
it no longer has side effects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants