Skip to content

Commit

Permalink
#7527 failing test case: UnitOfWork#getSingleIdentifierValue() shou…
Browse files Browse the repository at this point in the history
…ld not be called for a well specified parameter type

As previously reported by @flaushi in #7471 (comment), we discovered
that binding a parameter causes a `ClassMetadataFactory#getClassMetadata()` call, which in turn leads to large performance
regression when using any `object` type as parameter.

Following two snippets lead to an internal `ClassMetadataFactory#getClassMetadata()` call, which in turn leads to an
exception being thrown and garbage collected, plus multiple associated performance implications:

```php
$query->setParameter('foo', new DateTime());
$query->getResult();
```

```php
$query->setParameter('foo', new DateTime(), DateTimeType::NAME);
$query->getResult();
```

This is due to following portion of code:

https://github.com/doctrine/doctrine2/blob/434820973cadf2da2d66e7184be370084cc32ca8/lib/Doctrine/ORM/Query.php#L406-L409

Notice how `$value = $this->processParameterValue($value);` happens before attempting to infer the type for the parameter value.

That call leads to this segment being reached, which leads to the regression:

https://github.com/doctrine/doctrine2/blob/434820973cadf2da2d66e7184be370084cc32ca8/lib/Doctrine/ORM/AbstractQuery.php#L423-L433

Assuming the bound parameter type is provided, we can completely skip attempting to introspect the given object:

```php
$query->setParameter('foo', new DateTime(), DateTimeType::NAME);
$query->getResult();
```

Processing the parameter value is not needed in this case, so we can safely skip that logic for all known parameters.
In order to not introduce a BC break or change the `AbstractQuery#processParameterValue()` implementation, we could filter
out all parameters for which the type is given upfront, and later on merge them back in instead.

The test expectation to be set is for `UnitOfWork#getSingleIdentifierValue()` to never be called.
  • Loading branch information
Ocramius committed Dec 16, 2018
1 parent 237bebe commit 960a437
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 9 deletions.
30 changes: 25 additions & 5 deletions tests/Doctrine/Tests/ORM/Query/QueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,26 @@

namespace Doctrine\Tests\ORM\Query;

use DateTime;
use Doctrine\Common\Cache\ArrayCache;
use Doctrine\Common\Collections\ArrayCollection;

use Doctrine\DBAL\Cache\QueryCacheProfile;
use Doctrine\ORM\EntityManager;
use Doctrine\DBAL\Types\Type;
use Doctrine\ORM\Internal\Hydration\IterableResult;
use Doctrine\ORM\Query\Parameter;
use Doctrine\ORM\Query\QueryException;
use Doctrine\ORM\UnitOfWork;
use Doctrine\Tests\Mocks\DriverConnectionMock;
use Doctrine\Tests\Mocks\EntityManagerMock;
use Doctrine\Tests\Mocks\StatementArrayMock;
use Doctrine\Tests\Models\CMS\CmsAddress;
use Doctrine\Tests\Models\CMS\CmsUser;
use Doctrine\Tests\Models\Generic\DateTimeModel;
use Doctrine\Tests\OrmTestCase;

class QueryTest extends OrmTestCase
{
/** @var EntityManager */
protected $_em = null;
/** @var EntityManagerMock */
protected $_em;

protected function setUp()
{
Expand Down Expand Up @@ -400,4 +402,22 @@ public function testResultCacheProfileCanBeRemovedViaSetter() : void

self::assertAttributeSame(null, '_queryCacheProfile', $query);
}

/** @group 7527 */
public function testValuesAreNotBeingResolvedForSpecifiedParameterTypes() : void
{
$unitOfWork = $this->createMock(UnitOfWork::class);

$this->_em->setUnitOfWork($unitOfWork);

$unitOfWork
->expects(self::never())
->method('getSingleIdentifierValue');

$query = $this->_em->createQuery('SELECT d FROM ' . DateTimeModel::class . ' d WHERE d.datetime = :value');

$query->setParameter('value', new DateTime(), Type::DATETIME);

self::assertEmpty($query->getResult());
}
}
7 changes: 3 additions & 4 deletions tests/Doctrine/Tests/OrmTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Doctrine\ORM\Configuration;
use Doctrine\ORM\Mapping\Driver\AnnotationDriver;
use Doctrine\Tests\Mocks;
use Doctrine\Tests\Mocks\EntityManagerMock;

/**
* Base testcase class for all ORM testcases.
Expand Down Expand Up @@ -113,10 +114,8 @@ protected function createAnnotationDriver($paths = [], $alias = null)
* @param mixed $conf
* @param \Doctrine\Common\EventManager|null $eventManager
* @param bool $withSharedMetadata
*
* @return \Doctrine\ORM\EntityManager
*/
protected function _getTestEntityManager($conn = null, $conf = null, $eventManager = null, $withSharedMetadata = true)
protected function _getTestEntityManager($conn = null, $conf = null, $eventManager = null, $withSharedMetadata = true) : EntityManagerMock
{
$metadataCache = $withSharedMetadata
? self::getSharedMetadataCacheImpl()
Expand Down Expand Up @@ -160,7 +159,7 @@ protected function _getTestEntityManager($conn = null, $conf = null, $eventManag
$conn = DriverManager::getConnection($conn, $config, $eventManager);
}

return Mocks\EntityManagerMock::create($conn, $config, $eventManager);
return EntityManagerMock::create($conn, $config, $eventManager);
}

protected function enableSecondLevelCache($log = true)
Expand Down

0 comments on commit 960a437

Please sign in to comment.