Skip to content

Commit

Permalink
[GH-8410] Fix memory leak in new toIterable and state bug. (#8467)
Browse files Browse the repository at this point in the history
* [GH-8410] Fix memory leak in new toIterable and state bug.

The new AbstractQuery::toIterable() had a memory leak that
AbstractQuery::iterable() did not have. This leak is now fixed.

After fixing the leak, one test failed where the identity map in
ObjectHydrator triggered and lead to a notice. Introduced a new
AbstractHydrator::cleanupAfterRowIteration() that the ObjectHydrator
uses to cleanup the state.

* [GH-8413] Bugfix: Iterating with multiple, mixed results

When multiple entity results are part of a row, the result handling
must be different. In addition mixed results with scalars are broken
and now throw an exception as illegal operation.

* Housekeeping: phpcs

* [GH-8413] Add assertions for entity alias iteration.

* [GH-8387] Missing @deprecated on Query::iterate
  • Loading branch information
beberlei authored Feb 16, 2021
1 parent 3a9b8fd commit 30a7c2a
Show file tree
Hide file tree
Showing 9 changed files with 96 additions and 6 deletions.
8 changes: 7 additions & 1 deletion lib/Doctrine/ORM/AbstractQuery.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use Doctrine\ORM\Internal\Hydration\IterableResult;
use Doctrine\ORM\Mapping\MappingException as ORMMappingException;
use Doctrine\ORM\Query\Parameter;
use Doctrine\ORM\Query\QueryException;
use Doctrine\ORM\Query\ResultSetMapping;
use Doctrine\Persistence\Mapping\MappingException;
use Traversable;
Expand Down Expand Up @@ -998,7 +999,12 @@ public function toIterable(iterable $parameters = [], $hydrationMode = null): it
$this->setParameters($parameters);
}

$rsm = $this->getResultSetMapping();
$rsm = $this->getResultSetMapping();

if ($rsm->isMixed && count($rsm->scalarMappings) > 0) {
throw QueryException::iterateWithMixedResultNotAllowed();
}

$stmt = $this->_doExecute();

return $this->_em->newHydrator($this->_hydrationMode)->toIterable($stmt, $rsm, $this->_hints);
Expand Down
17 changes: 14 additions & 3 deletions lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

use function array_map;
use function array_merge;
use function count;
use function end;
use function in_array;
use function trigger_error;
Expand Down Expand Up @@ -165,8 +166,6 @@ public function toIterable(Statement $stmt, ResultSetMapping $resultSetMapping,

$this->prepare();

$result = [];

while (true) {
$row = $this->_stmt->fetch(FetchMode::ASSOCIATIVE);

Expand All @@ -176,9 +175,17 @@ public function toIterable(Statement $stmt, ResultSetMapping $resultSetMapping,
break;
}

$result = [];

$this->hydrateRowData($row, $result);

yield end($result);
$this->cleanupAfterRowIteration();

if (count($result) === 1) {
yield end($result);
} else {
yield $result;
}
}
}

Expand Down Expand Up @@ -274,6 +281,10 @@ protected function cleanup()
->removeEventListener([Events::onClear], $this);
}

protected function cleanupAfterRowIteration(): void
{
}

/**
* Hydrates a single row from the current statement instance.
*
Expand Down
8 changes: 8 additions & 0 deletions lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,14 @@ protected function cleanup()
$this->_uow->hydrationComplete();
}

protected function cleanupAfterRowIteration(): void
{
$this->identifierMap =
$this->initializedCollections =
$this->existingCollections =
$this->resultPointers = [];
}

/**
* {@inheritdoc}
*/
Expand Down
2 changes: 2 additions & 0 deletions lib/Doctrine/ORM/Query.php
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,8 @@ public function getMaxResults()
* Executes the query and returns an IterableResult that can be used to incrementally
* iterated over the result.
*
* @deprecated
*
* @param ArrayCollection|mixed[]|null $parameters The query parameters.
* @param string|int $hydrationMode The hydration mode to use.
*
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/ORM/Query/QueryException.php
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,11 @@ public static function iterateWithFetchJoinNotAllowed($assoc)
);
}

public static function iterateWithMixedResultNotAllowed(): QueryException
{
return new self('Iterating a query with mixed results (using scalars) is not supported.');
}

/**
* @return QueryException
*/
Expand Down
2 changes: 0 additions & 2 deletions tests/Doctrine/Tests/ORM/Functional/AdvancedDqlQueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,6 @@ public function testSelectSubselect(): void
$this->assertEquals('Caramba', $result[0]['brandName']);

$this->_em->clear();

IterableTester::assertResultsAreTheSame($query);
}

public function testInSubselect(): void
Expand Down
4 changes: 4 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/QueryIterableTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public function testAlias(): void
$users = $query->getResult();
self::assertCount(1, $users);

$this->assertEquals('gblanco', $users[0]['user']->username);

$this->_em->clear();

IterableTester::assertResultsAreTheSame($query);
Expand Down Expand Up @@ -60,6 +62,8 @@ public function testAliasInnerJoin(): void
$users = $query->getResult();
self::assertCount(1, $users);

$this->assertEquals('gblanco', $users[0]['user']->username);

$this->_em->clear();

IterableTester::assertResultsAreTheSame($query);
Expand Down
45 changes: 45 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/QueryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,51 @@ public function testIterateResultIterativelyBuildUpUnitOfWork(): void
self::assertSame(2, $iteratedCount);
}

public function testToIterableWithMultipleSelectElements(): void
{
$author = new CmsUser();
$author->name = 'Ben';
$author->username = 'beberlei';

$article1 = new CmsArticle();
$article1->topic = 'Doctrine 2';
$article1->text = 'This is an introduction to Doctrine 2.';
$article1->setAuthor($author);

$article2 = new CmsArticle();
$article2->topic = 'Symfony 2';
$article2->text = 'This is an introduction to Symfony 2.';
$article2->setAuthor($author);

$this->_em->persist($article1);
$this->_em->persist($article2);
$this->_em->persist($author);

$this->_em->flush();
$this->_em->clear();

$query = $this->_em->createQuery('select a, u from ' . CmsArticle::class . ' a JOIN ' . CmsUser::class . ' u WITH a.user = u');

$result = iterator_to_array($query->toIterable());

$this->assertCount(2, $result);

foreach ($result as $row) {
$this->assertCount(2, $row);
$this->assertInstanceOf(CmsArticle::class, $row[0]);
$this->assertInstanceOf(CmsUser::class, $row[1]);
}
}

public function testToIterableWithMixedResultIsNotAllowed(): void
{
$this->expectException(QueryException::class);
$this->expectExceptionMessage('Iterating a query with mixed results (using scalars) is not supported.');

$query = $this->_em->createQuery('select a, a.topic from ' . CmsArticle::class . ' a');
$query->toIterable();
}

public function testIterateResultClearEveryCycle(): void
{
$article1 = new CmsArticle();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,20 @@ public function testNonUniqueObjectHydrationDuringIteration(): void
$bs = IterableTester::iterableToArray(
$q->toIterable([], AbstractQuery::HYDRATE_OBJECT)
);

$this->assertCount(2, $bs);
$this->assertInstanceOf(GH7496EntityB::class, $bs[0]);
$this->assertInstanceOf(GH7496EntityB::class, $bs[1]);
$this->assertEquals(1, $bs[0]->id);
$this->assertEquals(1, $bs[1]->id);

$bs = IterableTester::iterableToArray(
$q->toIterable([], AbstractQuery::HYDRATE_ARRAY)
);

$this->assertCount(2, $bs);
$this->assertEquals(1, $bs[0]['id']);
$this->assertEquals(1, $bs[1]['id']);
}
}

Expand Down

0 comments on commit 30a7c2a

Please sign in to comment.