Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed typo in hints. Caused slow loading of eager entities. #611

Merged
merged 3 commits into from

4 participants

@stefankleff

This small typo prevents entities from being fetched with the deferred- eager-loading path.

@doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2346

@Ocramius
Owner

Hey @stefankleff! Good catch, but do you have a test case for this fix? (to avoid this in future)

@stefankleff

This already took me almost two days. It would be nice if someone else can add some tests 0:-)

@Ocramius
Owner

@stefankleff do you have a query to start from? I can do it if I know what kind of issue spawned this.

@stefankleff

I ran into that while dealing with a rather complex use case. I think it should be sufficient to do the following: A has a bidirectional eager 1:n relation r to B. fetchAll(A) should hydrate the objects with 2 queries not with 1 + n

@stefankleff

Important: You have to use the "OOP-way" (with repository) not the Query or QueryBuilder.

@Ocramius
Owner

I see. Will check if a test is possible :)

@beberlei
Owner

Can you make a constant out of the "magic string"?

@Ocramius Ocramius referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@Ocramius Ocramius referenced this pull request from a commit in Ocramius/doctrine2
@Ocramius Ocramius Adding failing test for DDC-2346 doctrine/doctrine2#611 e468ced
@Ocramius
Owner

@stefankleff please check stefankleff/doctrine2#1 (quick and dirty one, but I couldn't reproduce the fix)

@stefankleff

First of all thanks for your test! The use case I described yesterday was the wrong way: fetch all Bs where every B has a eager to-one relation to A. Doing this will result in a regular join. But if C extends B and you fetch all Cs the JoinedSubclassPersister is used, which is not able to generate the SQL and the resultset for the join. Therefore the "join" is done in the UoW with the help of deferred loading. Without the fix this fails and every related A is fetched while creating the C entitity - not afterwards.

@Ocramius
Owner

@stefankleff aha, didn't know there was an inheritance :) Thank you for incorporating the test!

@stefankleff

And a constant would be much better - but I'm not sure in which class it should be defined. I would prefer the UoW. Any suggestions?

@beberlei
Owner

Adding the constant in the UnitOfWork is good.

@beberlei
Owner

@stefankleff any news?

@beberlei beberlei merged commit 52b2e06 into doctrine:master

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 12, 2013
  1. @stefankleff
Commits on Mar 13, 2013
  1. @stefankleff
Commits on Apr 8, 2013
  1. @stefankleff

    Added constant

    stefankleff authored
This page is out of date. Refresh to see the latest.
View
7 lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php
@@ -19,6 +19,7 @@
namespace Doctrine\ORM\Internal\Hydration;
+use Doctrine\ORM\UnitOfWork;
use PDO;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\PersistentCollection;
@@ -94,8 +95,8 @@ protected function prepare()
$this->resultCounter = 0;
- if ( ! isset($this->_hints['deferEagerLoad'])) {
- $this->_hints['deferEagerLoad'] = true;
+ if ( ! isset($this->_hints[UnitOfWork::HINT_DEFEREAGERLOAD])) {
+ $this->_hints[UnitOfWork::HINT_DEFEREAGERLOAD] = true;
}
foreach ($this->_rsm->aliasMap as $dqlAlias => $className) {
@@ -152,7 +153,7 @@ protected function prepare()
*/
protected function cleanup()
{
- $eagerLoad = (isset($this->_hints['deferEagerLoad'])) && $this->_hints['deferEagerLoad'] == true;
+ $eagerLoad = (isset($this->_hints[UnitOfWork::HINT_DEFEREAGERLOAD])) && $this->_hints[UnitOfWork::HINT_DEFEREAGERLOAD] == true;
parent::cleanup();
View
10 lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -851,7 +851,7 @@ public function loadCriteria(Criteria $criteria)
$stmt = $this->conn->executeQuery($query, $params, $types);
$hydrator = $this->em->newHydrator(($this->selectJoinSql) ? Query::HYDRATE_OBJECT : Query::HYDRATE_SIMPLEOBJECT);
- return $hydrator->hydrateAll($stmt, $this->rsm, array('deferEagerLoads' => true));
+ return $hydrator->hydrateAll($stmt, $this->rsm, array(UnitOfWork::HINT_DEFEREAGERLOAD => true));
}
/**
@@ -908,7 +908,7 @@ public function loadAll(array $criteria = array(), array $orderBy = null, $limit
$hydrator = $this->em->newHydrator(($this->selectJoinSql) ? Query::HYDRATE_OBJECT : Query::HYDRATE_SIMPLEOBJECT);
- return $hydrator->hydrateAll($stmt, $this->rsm, array('deferEagerLoads' => true));
+ return $hydrator->hydrateAll($stmt, $this->rsm, array(UnitOfWork::HINT_DEFEREAGERLOAD => true));
}
/**
@@ -939,7 +939,7 @@ public function getManyToManyCollection(array $assoc, $sourceEntity, $offset = n
private function loadArrayFromStatement($assoc, $stmt)
{
$rsm = $this->rsm;
- $hints = array('deferEagerLoads' => true);
+ $hints = array(UnitOfWork::HINT_DEFEREAGERLOAD => true);
if (isset($assoc['indexBy'])) {
$rsm = clone ($this->rsm); // this is necessary because the "default rsm" should be changed.
@@ -962,8 +962,8 @@ private function loadCollectionFromStatement($assoc, $stmt, $coll)
{
$rsm = $this->rsm;
$hints = array(
- 'deferEagerLoads' => true,
- 'collection' => $coll
+ UnitOfWork::HINT_DEFEREAGERLOAD => true,
+ 'collection' => $coll
);
if (isset($assoc['indexBy'])) {
View
11 lib/Doctrine/ORM/UnitOfWork.php
@@ -77,6 +77,13 @@ class UnitOfWork implements PropertyChangedListener
const STATE_REMOVED = 4;
/**
+ * Hint used to collect all primary keys of associated entities during hydration
+ * and execute it in a dedicated query afterwards
+ * @see https://doctrine-orm.readthedocs.org/en/latest/reference/dql-doctrine-query-language.html?highlight=eager#temporarily-change-fetch-mode-in-dql
+ */
+ const HINT_DEFEREAGERLOAD = 'deferEagerLoad';
+
+ /**
* The identity map that holds references to all managed entities that have
* an identity. The entities are grouped by their class name.
* Since all classes in a hierarchy must share the same identifier set,
@@ -2616,7 +2623,7 @@ public function createEntity($className, array $data, &$hints = array())
// this association is marked as eager fetch, and its an uninitialized proxy (wtf!)
// then we can append this entity for eager loading!
if ($hints['fetchMode'][$class->name][$field] == ClassMetadata::FETCH_EAGER &&
- isset($hints['deferEagerLoad']) &&
+ isset($hints[self::HINT_DEFEREAGERLOAD]) &&
!$targetClass->isIdentifierComposite &&
$newValue instanceof Proxy &&
$newValue->__isInitialized__ === false) {
@@ -2641,7 +2648,7 @@ public function createEntity($className, array $data, &$hints = array())
break;
// Deferred eager load only works for single identifier classes
- case (isset($hints['deferEagerLoad']) && ! $targetClass->isIdentifierComposite):
+ case (isset($hints[self::HINT_DEFEREAGERLOAD]) && ! $targetClass->isIdentifierComposite):
// TODO: Is there a faster approach?
$this->eagerLoadingEntities[$targetClass->rootEntityName][$relatedIdHash] = current($associatedId);
View
109 tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2346Test.php
@@ -0,0 +1,109 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\Common\Collections\ArrayCollection;
+use Doctrine\DBAL\Logging\DebugStack;
+
+/**
+ * @group DDC-2346
+ */
+class DDC2346Test extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+ /**
+ * @var \Doctrine\DBAL\Logging\DebugStack
+ */
+ protected $logger;
+
+ /**
+ * {@inheritDoc}
+ */
+ protected function setup()
+ {
+ parent::setup();
+
+ $this->_schemaTool->createSchema(array(
+ $this->_em->getClassMetadata(__NAMESPACE__ . '\DDC2346Foo'),
+ $this->_em->getClassMetadata(__NAMESPACE__ . '\DDC2346Bar'),
+ $this->_em->getClassMetadata(__NAMESPACE__ . '\DDC2346Baz'),
+ ));
+
+ $this->logger = new DebugStack();
+ }
+
+ /**
+ * Verifies that fetching a OneToMany association with fetch="EAGER" does not cause N+1 queries
+ */
+ public function testIssue()
+ {
+ $foo1 = new DDC2346Foo();
+ $foo2 = new DDC2346Foo();
+
+ $baz1 = new DDC2346Baz();
+ $baz2 = new DDC2346Baz();
+
+ $baz1->foo = $foo1;
+ $baz2->foo = $foo2;
+
+ $foo1->bars[] = $baz1;
+ $foo1->bars[] = $baz2;
+
+ $this->_em->persist($foo1);
+ $this->_em->persist($foo2);
+ $this->_em->persist($baz1);
+ $this->_em->persist($baz2);
+
+ $this->_em->flush();
+ $this->_em->clear();
+
+ $this->_em->getConnection()->getConfiguration()->setSQLLogger($this->logger);
+
+ $fetchedBazs = $this->_em->getRepository(__NAMESPACE__ . '\\DDC2346Baz')->findAll();
+
+ $this->assertCount(2, $fetchedBazs);
+ $this->assertCount(2, $this->logger->queries, 'The total number of executed queries is 2, and not n+1');
+ }
+}
+
+/** @Entity */
+class DDC2346Foo
+{
+ /** @Id @Column(type="integer") @GeneratedValue */
+ public $id;
+
+ /**
+ * @var DDC2346Bar[]|\Doctrine\Common\Collections\Collection
+ *
+ * @OneToMany(targetEntity="DDC2346Bar", mappedBy="foo")
+ */
+ public $bars;
+
+ /** Constructor */
+ public function __construct() {
+ $this->bars = new ArrayCollection();
+ }
+}
+
+/**
+ * @Entity
+ * @InheritanceType("JOINED")
+ * @DiscriminatorColumn(name="discr", type="string")
+ * @DiscriminatorMap({"baz" = "DDC2346Baz"})
+ */
+class DDC2346Bar
+{
+ /** @Id @Column(type="integer") @GeneratedValue */
+ public $id;
+
+ /** @ManyToOne(targetEntity="DDC2346Foo", inversedBy="bars", fetch="EAGER") */
+ public $foo;
+}
+
+
+/**
+ * @Entity
+ */
+class DDC2346Baz extends DDC2346Bar
+{
+
+}
Something went wrong with that request. Please try again.