Skip to content

Recursive check for entity identifiers and hashes #232

Closed
wants to merge 4 commits into from

6 participants

@goetas
goetas commented Dec 20, 2011

Hi all!
This PR will add a better support for entities with association keys.

getType will check recursively to find a type for the identifier.
getIndividualValue will search recursively to find the identifier value

trygetById improved, using a recursive function to find an id value instead of implode functions (that cause exceptions if the identifier is an object and do not implements __toString method).

@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -1448,37 +1448,42 @@ private function expandParameters($criteria)
*/
private function getType($field, $value)
{
+ $type = $this->getRecursiveType($field, $this->_class);
+ if (is_array($value)) {
+ $type = Type::getType( $type )->getBindingType();
+ $type += Connection::ARRAY_PARAM_OFFSET;
+ }
+ return $type;
+ }
+ /**
@stof
Doctrine member
stof added a note Dec 20, 2011

you should add an empty line before the next function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -1448,37 +1448,42 @@ private function expandParameters($criteria)
*/
private function getType($field, $value)
{
+ $type = $this->getRecursiveType($field, $this->_class);
+ if (is_array($value)) {
+ $type = Type::getType( $type )->getBindingType();
+ $type += Connection::ARRAY_PARAM_OFFSET;
+ }
+ return $type;
+ }
+ /**
+ * Infer (recursivley) field type to be used by parameter type casting.
@stof
Doctrine member
stof added a note Dec 20, 2011

typo here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -1448,37 +1448,42 @@ private function expandParameters($criteria)
*/
private function getType($field, $value)
{
+ $type = $this->getRecursiveType($field, $this->_class);
+ if (is_array($value)) {
+ $type = Type::getType( $type )->getBindingType();
+ $type += Connection::ARRAY_PARAM_OFFSET;
+ }
+ return $type;
+ }
+ /**
+ * Infer (recursivley) field type to be used by parameter type casting.
+ * Used by getType() method.
@stof
Doctrine member
stof added a note Dec 20, 2011

this line is useless IMO. And if you really want to link to getType, it should be done using the @see phpdoc tag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
switch (true) {
- case (isset($this->_class->fieldMappings[$field])):
- $type = $this->_class->fieldMappings[$field]['type'];
+ case (isset($class->fieldMappings[$field])):
+ return $class->fieldMappings[$field]['type'];
break;
@stof
Doctrine member
stof added a note Dec 20, 2011

break is useless as you return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
if (count($assoc['sourceToTargetKeyColumns']) > 1) {
throw Query\QueryException::associationPathCompositeKeyNotSupported();
}
$targetClass = $this->_em->getClassMetadata($assoc['targetEntity']);
- $targetColumn = $assoc['joinColumns'][0]['referencedColumnName'];
- $type = null;
-
- if (isset($targetClass->fieldNames[$targetColumn])) {
- $type = $targetClass->fieldMappings[$targetClass->fieldNames[$targetColumn]]['type'];
- }
-
+ return $this->getRecursiveType($targetClass->identifier[0], $targetClass);
break;
@stof
Doctrine member
stof added a note Dec 20, 2011

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@beberlei
Doctrine member

What do you mean by "better" support? It works for me, i don't understand why this is necessary.

@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/UnitOfWork.php
@@ -1305,6 +1305,11 @@ public function getEntityState($entity, $assume = null)
if ( ! $id) {
return self::STATE_NEW;
+ } elseif (count($id) && is_object(reset($id))) {
@stof
Doctrine member
stof added a note Dec 20, 2011

you should simply use if instead of elseif as the previous if returns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/UnitOfWork.php
@@ -1305,6 +1305,11 @@ public function getEntityState($entity, $assume = null)
if ( ! $id) {
return self::STATE_NEW;
+ } elseif (count($id) && is_object(reset($id))) {
+ $state = $this->getEntityState(reset($id));
+ if ($state===self::STATE_NEW) {
@stof
Doctrine member
stof added a note Dec 20, 2011

missing spaces around ===

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/UnitOfWork.php
+ foreach ($ids as $id){
+ if (is_object($id) && $this->em->getMetadataFactory()->hasMetadataFor(get_class($id))) {
+ $oid = spl_object_hash($id);
+ if (isset($this->entityIdentifiers[$oid])) {
+ $strings[] = $this->getHashForEntityIdentifier($this->entityIdentifiers[$oid]);
+ } else {
+ $value = $this->em->getClassMetadata(get_class($id))->getIdentifierValues($id);
+ $strings[] = $this->getHashForEntityIdentifier($value);
+ }
+ } else {
+ $strings[] = $id;
+ }
+ }
+ return implode(' ', $strings);
+ }
+ /**
@stof
Doctrine member
stof added a note Dec 20, 2011

missing empty line here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/UnitOfWork.php
@@ -2607,6 +2612,30 @@ public function getEntityIdentifier($entity)
}
/**
+ * Compute a hash for the given id. Acept also objects ( for association keys)
+ *
+ * @param mixed $id The entity identifier to look for.
+ * @return strine Returns the computed hash.
@stof
Doctrine member
stof added a note Dec 20, 2011

typo here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
lib/Doctrine/ORM/UnitOfWork.php
@@ -2607,6 +2612,30 @@ public function getEntityIdentifier($entity)
}
/**
+ * Compute a hash for the given id. Acept also objects ( for association keys)
@stof
Doctrine member
stof added a note Dec 20, 2011

typo here, and extra space after the brace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
...ine/Tests/ORM/Functional/AssociationKeysChainTest.php
@@ -0,0 +1,106 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+require_once __DIR__ . '/../../TestInit.php';
+/**
+ * Functional tests for the Single Table Inheritance mapping strategy.
+ *
+ * @author robo
+ */
+class AssociationKeysChainTest extends \Doctrine\Tests\OrmFunctionalTestCase{
+ public function setUp(){
@stof
Doctrine member
stof added a note Dec 20, 2011

the curly brace sould be on its own line for the class and the method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Dec 20, 2011
...ine/Tests/ORM/Functional/AssociationKeysChainTest.php
+/**
+ * Functional tests for the Single Table Inheritance mapping strategy.
+ *
+ * @author robo
+ */
+class AssociationKeysChainTest extends \Doctrine\Tests\OrmFunctionalTestCase{
+ public function setUp(){
+ parent::setUp();
+ $this->_schemaTool->createSchema(array(
+ $this->_em->getClassMetadata(__NAMESPACE__ . '\\AssociationKeysChain0'),
+ $this->_em->getClassMetadata(__NAMESPACE__ . '\\AssociationKeysChainA'),
+ $this->_em->getClassMetadata(__NAMESPACE__ . '\\AssociationKeysChainB'),
+ $this->_em->getClassMetadata(__NAMESPACE__ . '\\AssociationKeysChainC'),
+ ));
+ }
+ public function testIssue()
@stof
Doctrine member
stof added a note Dec 20, 2011

missing empty line between the methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@goetas
goetas commented Dec 20, 2011

added fixes

@guilhermeblanco
Doctrine member

@goetas Can you also update the ObjectHydrator to consume the getHashEntityIdentifier?

@beberlei I do see a value on this patch because the current hash generation is broken (if you use a @Column(type="string") @Id) and it does contain spaces in its value, you can break the hash generation.

However, I want to see a more complete solution like I mentioned. There're many places where the hash generation is used (look at UoW::creatEntity or Hydrators::registerManaged).

@goetas
goetas commented Dec 20, 2011

added

@guilhermeblanco
Doctrine member

@goetas you missed the UoW::createEntity too. =)

@guilhermeblanco
Doctrine member

@goetas Also, please consider the performance impact of this change.
I'm concerned that it may slow down the suite by 50% with this change. Think about how many times this will be called. I can't thought too much about it (YET), but I want to give a try on your patch as soon as possible.

@beberlei
Doctrine member
@goetas
goetas commented Dec 21, 2011

@guilhermeblanco
is really required to add getHashForEntityIdentifier() inside UoW::createEntity?
$isHash is composed using $data array that contains only scalar values, so getHashForEntityIdentifier may be too expensive. on the other hand, using getHashForEntityIdentifier we obtain a single point where hashes are generated...

i take a look to performances... the suite runs in equal times, as before, because the performance overhead is appreciable only if we have a large set of objects that use entities as primary key...

@guilhermeblanco
Doctrine member

@goetas It's the exact point.
We current have hash generation on multiple places, decentralized. We need to wrap it into a single location.

Regarding performance, there are performance tests in Doctrine that you can run. I think the suite of Performance is commented by default, but you just uncomment it and run.

@beberlei
Doctrine member

There is a unit of work performance test that specifically tests UnitOfWork::createEntity.

Then there are a bunch of hydration performance tests.

You can run them with "phpunit -c config tests/Doctrine/Tests/ORM/Performance", make sure that your config does not contain an exclude-group for the performance.

@asm89
Doctrine member
asm89 commented Mar 11, 2012

@goetas Any news on running the performance benchmark?

@beberlei Maybe you can check it out? You mentioned earlier that the performance impact seemed to high.

@goetas
goetas commented Mar 12, 2012

Current doctrine master:
project@sviluppo:~/doctrine2$ phpunit -c sqlite.phpunit.xml -d memory_limit=256M tests/Doctrine/Tests/ORM/Performance
PHPUnit 3.6.4 by Sebastian Bergmann.

Configuration read from /home/project/doctrine2/sqlite.phpunit.xml

.testSimpleQueryScalarHydrationPerformance10000Rows - 0.45193099975586 seconds
.testSimpleQueryArrayHydrationPerformance10000Rows - 0.68628287315369 seconds
.testMixedQueryFetchJoinArrayHydrationPerformance10000Rows - 1.5172929763794 seconds
.testSimpleQueryPartialObjectHydrationPerformance10000Rows - 1.3703680038452 seconds
FtestSimpleQueryFullObjectHydrationPerformance10000Rows - 5.5112481117249 seconds
.testMixedQueryFetchJoinPartialObjectHydrationPerformance2000Rows - 0.63160705566406 seconds
FtestMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows - 1.3217060565948 seconds
.99 CompanyContract: 0.039680
99 CompanyContract: 0.037567
. Inserted 10000 objects in 8.2778890132904 seconds
.100 CmsArticle findAll(): 0.050949
100 CmsArticle findAll(): 0.025102
100 CmsArticle find(): 0.085930
100 CmsArticle find(): 0.085909
.100 CmsGroup: 0.016937
100 CmsGroup: 0.016911
.100 CmsUser: 0.041710
100 CmsUser: 0.039994
. Compute ChangeSet 100 objects in 0.066016912460327 seconds

Time: 24 seconds, Memory: 154.25Mb

There were 2 failures:

1) Doctrine\Tests\ORM\Performance\HydrationPerformanceTest::testSimpleQueryFullObjectHydrationPerformance10000Rows
expected running time: <= 5 but was: 5.9293828010559

/home/project/doctrine2/tests/Doctrine/Tests/OrmPerformanceTestCase.php:33
/usr/local/zend/bin/phpunit:44

2) Doctrine\Tests\ORM\Performance\HydrationPerformanceTest::testMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows
expected running time: <= 1 but was: 1.3423340320587

/home/project/doctrine2/tests/Doctrine/Tests/OrmPerformanceTestCase.php:33
/usr/local/zend/bin/phpunit:44

Modified doctrine version:

project@sviluppo:~/goetas-doctrine$ phpunit -c sqlite.phpunit.xml -d memory_limit=256M tests/Doctrine/Tests/ORM/Performance
PHPUnit 3.6.4 by Sebastian Bergmann.

Configuration read from /home/project/goetas-doctrine/sqlite.phpunit.xml

.testSimpleQueryScalarHydrationPerformance10000Rows - 0.45950889587402 seconds
.testSimpleQueryArrayHydrationPerformance10000Rows - 0.75007891654968 seconds
.testMixedQueryFetchJoinArrayHydrationPerformance10000Rows - 1.7030961513519 seconds
.testSimpleQueryPartialObjectHydrationPerformance10000Rows - 1.757111787796 seconds
FtestSimpleQueryFullObjectHydrationPerformance10000Rows - 6.6794309616089 seconds
.testMixedQueryFetchJoinPartialObjectHydrationPerformance2000Rows - 0.81370711326599 seconds
FtestMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows - 1.8190610408783 seconds
.99 CompanyContract: 0.076754
99 CompanyContract: 0.044255
. Inserted 10000 objects in 8.7414720058441 seconds
.100 CmsArticle findAll(): 0.133854
100 CmsArticle findAll(): 0.027600
100 CmsArticle find(): 0.098926
100 CmsArticle find(): 0.100574
.100 CmsGroup: 0.018479
100 CmsGroup: 0.020694
.100 CmsUser: 0.045250
100 CmsUser: 0.041374
. Compute ChangeSet 100 objects in 0.072705984115601 seconds

Time: 27 seconds, Memory: 154.00Mb

There were 2 failures:

1) Doctrine\Tests\ORM\Performance\HydrationPerformanceTest::testSimpleQueryFullObjectHydrationPerformance10000Rows
expected running time: <= 5 but was: 7.1469988822937

/home/project/goetas-doctrine/tests/Doctrine/Tests/OrmPerformanceTestCase.php:33
/usr/local/zend/bin/phpunit:44

2) Doctrine\Tests\ORM\Performance\HydrationPerformanceTest::testMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows
expected running time: <= 1 but was: 1.8401811122894

/home/project/goetas-doctrine/tests/Doctrine/Tests/OrmPerformanceTestCase.php:33
/usr/local/zend/bin/phpunit:44

Summary
Doctrine\Tests\ORM\Performance\HydrationPerformanceTest::testSimpleQueryFullObjectHydrationPerformance10000Rows
expected running time: <= 5 but was: 7.1469988822937 (my)
expected running time: <= 5 but was: 5.9293828010559 (doctrine)
+1.24s = +21% performance lost

Doctrine\Tests\ORM\Performance\HydrationPerformanceTest::testMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows
expected running time: <= 1 but was: 1.8401811122894 (my)
expected running time: <= 1 but was: 1.3423340320587 (doctrine)
+0.5s = +34% performance lost

@goetas
goetas commented Mar 12, 2012

any suggestion?

@asm89
Doctrine member
asm89 commented May 4, 2012

ping @beberlei @guilhermeblanco

@goutas Ran the performance test suite. Any thoughts? :)

@goetas
goetas commented May 7, 2012

there is a lot of recursion and some checks... the question is:
the performance lost equals to the value of this patch? there is a real need? (i say 'yes')

@lstrojny lstrojny commented on the diff Nov 26, 2012
lib/Doctrine/ORM/UnitOfWork.php
@@ -2607,6 +2614,31 @@ public function getEntityIdentifier($entity)
}
/**
+ * Compute a hash for the given id. Accept also objects (for association keys)
+ *
+ * @param mixed $id The entity identifier to look for.
+ * @return string Returns the computed hash.
+ */
+ private function getHashForEntityIdentifier(array $ids)
+ {
+ $strings = array();
+ foreach ($ids as $id){
@lstrojny
lstrojny added a note Nov 26, 2012

Try using a breadth first search instead of recursion here to reduce the number of required method calls.

<?php
while($ids) {
     foreach ($ids as $id) {
          ...
     }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lstrojny lstrojny commented on the diff Nov 26, 2012
lib/Doctrine/ORM/UnitOfWork.php
+ {
+ $strings = array();
+ foreach ($ids as $id){
+ if (is_object($id) && $this->em->getMetadataFactory()->hasMetadataFor(get_class($id))) {
+ $oid = spl_object_hash($id);
+ if (isset($this->entityIdentifiers[$oid])) {
+ $strings[] = $this->getHashForEntityIdentifier($this->entityIdentifiers[$oid]);
+ } else {
+ $value = $this->em->getClassMetadata(get_class($id))->getIdentifierValues($id);
+ $strings[] = $this->getHashForEntityIdentifier($value);
+ }
+ } else {
+ $strings[] = $id;
+ }
+ }
+ return implode(' ', $strings);
@lstrojny
lstrojny added a note Nov 26, 2012

Instead of creating an array, append to a string.

@goetas
goetas added a note Apr 26, 2013

Appending to array is faster than concatenating strings...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lstrojny lstrojny commented on the diff Nov 26, 2012
lib/Doctrine/ORM/UnitOfWork.php
@@ -2607,6 +2614,31 @@ public function getEntityIdentifier($entity)
}
/**
+ * Compute a hash for the given id. Accept also objects (for association keys)
+ *
+ * @param mixed $id The entity identifier to look for.
+ * @return string Returns the computed hash.
+ */
+ private function getHashForEntityIdentifier(array $ids)
+ {
+ $strings = array();
+ foreach ($ids as $id){
+ if (is_object($id) && $this->em->getMetadataFactory()->hasMetadataFor(get_class($id))) {
+ $oid = spl_object_hash($id);
+ if (isset($this->entityIdentifiers[$oid])) {
@lstrojny
lstrojny added a note Nov 26, 2012

It might be a good idea to create local references of all your lookup arrays, as it is faster to access the index of an array and not a field additionally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lstrojny lstrojny commented on the diff Nov 26, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- case (isset($this->_class->associationMappings[$field])):
- $assoc = $this->_class->associationMappings[$field];
+ /**
+ * Infer (recursively) field type to be used by parameter type casting.
+ * @see getType($field, $value)
+ *
+ * @param string $field
+ * @param mixed $value
+ * @return integer
+ */
+ private function getRecursiveType($field, ClassMetadata $class)
@lstrojny
lstrojny added a note Nov 26, 2012

Same here. Try breadth first search instead of recursion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@beberlei
Doctrine member

closing in favor of GH-522

@beberlei beberlei closed this Mar 12, 2013
@goetas goetas deleted the goetas:recur branch Sep 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.