Composite primary key & association entities as keys improvements #1113

goetas · 2014-08-18T14:58:07Z

Hi!
I tried to implement the matching of entities that uses composite primary keys...

Any suggestion?

~~BC changes here https://github.com/doctrine/doctrine2/pull/1113/files#diff-c18001a2ca9928743b1e99854ffad33bL1611 (protected method makred as private and changed return type..)~~

doctrinebot · 2014-08-18T14:58:21Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3258

We use Jira to track the state of pull requests and the versions they got
included in.

Ocramius · 2014-08-18T15:03:40Z

@goetas can you add a functional test that shows how this API is supposed to work at repository or collection level?

stof · 2014-08-18T16:43:02Z

lib/Doctrine/ORM/ORMException.php

+     *
+     * @return ORMException
+     */
+    public static function cantUseInOperatorOnComposteKeys()


typo. Missing i

goetas · 2014-08-19T07:41:49Z

@stof Thanks for reviewing my code

goetas · 2014-08-19T07:49:25Z

It would be nice to have:

//Admin1 is an entity with a composite key (country+code)
//Admin1AlternativeNames is an entity with a simple key (id) + Admin1 as association

$compositeEntity = $em->getRepository('Admin1')
   ->findOneBy(array('country' => 'IT', 'id' => 1));

$dql = $em->getRepository('Admin1AlternativeNames')
   ->createQueryBuilder('n')
   ->andWhere("n.admin1 = :admin1")
   ->setParameter("admin1", $compositeEntity);

but at the moment seems to be complicated for me... :-(

goetas · 2014-08-19T08:13:16Z

I think that postgres failure is not related to my changes...

deeky666 · 2014-08-19T08:16:19Z

@goetas no I think this is related to the recent merge of doctrine/dbal#444

guilhermeblanco · 2014-08-19T15:17:52Z

@goetas there's no BC break, since we do not allow custom persisters (yet)

goetas · 2014-08-19T15:18:47Z

@guilhermeblanco good to know!

goetas · 2014-08-22T08:02:06Z

lib/Doctrine/ORM/Cache/Persister/AbstractEntityPersister.php

@@ -413,6 +422,7 @@ public function loadAll(array $criteria = array(), array $orderBy = null, $limit
        $rsm        = $this->getResultSetMapping();
        $querykey   = new QueryCacheKey($hash, 0, Cache::MODE_NORMAL);
        $queryCache = $this->cache->getQueryCache($this->regionName);
+


white line.. i will fix it :-(

goetas · 2014-08-22T09:11:57Z

Hi everybody! Now it should be complete!

deeky666 · 2014-08-22T09:29:43Z

lib/Doctrine/ORM/Cache/Persister/AbstractEntityPersister.php

@@ -232,6 +233,14 @@ public function storeEntityCache($entity, EntityCacheKey $key)
            $class = $this->metadataFactory->getMetadataFor($className);
        }

+        if ($class->containsForeignIdentifier) {
+            foreach ($class->associationMappings as $name => $assoc) {
+                if (isset($assoc['id']) && $assoc['id'] && !isset($assoc['cache'])) {


Why not use ! empty($assoc['id']) instead of isset($assoc['id']) && $assoc['id']?

Nice to know! I will do that!

stof · 2014-08-22T11:57:46Z

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php

     *
     * @param string $field
     * @param mixed  $value
     *
-     * @return integer
+     * @return array


I would document it as integer[] which explains what type is inside the array too

getTypes will return strings and integers... integers will be only when $value is an array

goetas · 2014-09-01T13:57:15Z

News on this?

ltouro · 2014-10-02T14:52:13Z

lib/Doctrine/ORM/ORMException.php

+     */
+    public static function cantUseInOperatorOnCompositeKeys()
+    {
+        return new self("Can't use IN operator on entities that have composte keys.");


typo on "composite" word

goetas · 2014-10-22T07:17:48Z

@Ocramius I can try to adapt it, but the current implementation of IdentifierFlattener has some lacks...

It does not work with composite entities (see here)
To make it work well, IdentifierFlattener::flattenIdentifier() has to become recursive, while my implementation does not need recursion since it has access to UoW::$entityIdentifiers hash.
The order of $id is very important to get the right flat identifier, and the current implementation relies on it. My implementation relies on ClassMetadata::$identifier property which is more secure to be always correct.
This is the most important piece of code where IdentifierFlattener should be used, but at the moment is not used.
This is correct only if data comes from second level cache.

Ocramius · 2014-10-22T18:31:51Z

It does not work with composite entities (see here)

This is something that I discussed in the PR that introduced it ( #1143 ): needs improvement

To make it work well, IdentifierFlattener::flattenIdentifier() has to become recursive, while my implementation does not need recursion since it has access to UoW::$entityIdentifiers hash.

Making it recursive seems acceptable to me here: what's the problem with that? We could also make the entity identifiers accessible to it.

The order of $id is very important to get the right flat identifier, and the current implementation relies on it. My implementation relies on ClassMetadata::$identifier property which is more secure to be always correct.

Should be improved also in the IdentifierFlattener implementation then, and we get rid of duplication of this hidden rule, plus we can test specifically the order here.

This is the most important piece of code where IdentifierFlattener should be used, but at the moment is not used.

It was just introduced, so it is obviously not yet used across the system :-)

This is correct only if data comes from second level cache.

Can you clarify on this? What would be a failure case?

goetas · 2015-01-29T22:04:11Z

lib/Doctrine/ORM/UnitOfWork.php

@@ -2639,10 +2625,18 @@ public function createEntity($className, array $data, &$hints = array())

                        if ($joinColumnValue !== null) {
                            if ($targetClass->containsForeignIdentifier) {
+                                if ($joinColumnValue instanceof AssociationCacheEntry) {


I'm really not proud of this. Ideas?

@FabioBatSilva maybe got ideas?

@Ocramius It was my mistake... fixed with goetas@268426a
/cc @FabioBatSilva

goetas · 2015-01-29T23:39:23Z

Ok, now looks better

Ocramius · 2015-02-01T18:11:57Z

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

@@ -335,7 +335,11 @@ protected function fetchVersionValue($versionedClass, $id)
             . ' FROM '  . $tableName
             . ' WHERE ' . implode(' = ? AND ', $identifier) . ' = ?';

-        $flatId = $this->identifierFlattener->flattenIdentifier($versionedClass, (array) $id);
+        if (!is_array($id)) {
+            $id = array($this->class->identifier[0] => $id);


@goetas this is very naive, and assumes a single column identifier.

What if I try to fetch the version of an entity with a composite identity?

We may also avoid calling this method with a mixed $id, and change its signature to enforce array

Changing the assignDefaultVersionValue signature, simplified a lot! Good advice! :-D

goetas · 2015-02-02T23:38:53Z

Now I'm more satisfied with the pull request status!

Ocramius · 2015-02-16T01:18:25Z

Manually merged after a rebase at f908974

Ocramius · 2015-02-16T01:18:35Z

Thanks @goetas, as usual nice work :-)

stof reviewed Aug 18, 2014
View reviewed changes

goetas mentioned this pull request Aug 19, 2014

Composite pk break test #1114

Closed

goetas changed the title ~~Added support for composite primary key on findBy methods and Criteria~~ [WIP] Composite primary key & association entities as keys improvements Aug 22, 2014

goetas reviewed Aug 22, 2014
View reviewed changes

goetas changed the title ~~[WIP] Composite primary key & association entities as keys improvements~~ Composite primary key & association entities as keys improvements Aug 22, 2014

deeky666 reviewed Aug 22, 2014
View reviewed changes

goetas mentioned this pull request Aug 22, 2014

[DDC-2166] Refactor identity hash generation #522

Closed

stof reviewed Aug 22, 2014
View reviewed changes

goetas mentioned this pull request Sep 23, 2014

Always allow proxies on ToOne associations #1141

Closed

ltouro reviewed Oct 2, 2014
View reviewed changes

Ocramius self-assigned this Oct 22, 2014

goetas reviewed Jan 29, 2015
View reviewed changes

goetas changed the title ~~[WIP] Composite primary key & association entities as keys improvements~~ Composite primary key & association entities as keys improvements Jan 29, 2015

Ocramius added Requires merge/review and removed WIP Delayed labels Feb 1, 2015

Ocramius reviewed Feb 1, 2015
View reviewed changes

goetas added 8 commits February 3, 2015 00:28

Fixed type checking on to-many relations

25ce87f

Identity map check

169d169

Removed unused methods

bf5fb6e

Right type detection on to-many relations

3f18a36

Fixed owing-inverse side search by criteria

4100bd5

Changed getSelectConditionStatementColumnSQL return docblock

e5bdd5d

Default Version Value identifier is always an array

8435e1f

Resolve association entries on multi get cache

268426a

goetas force-pushed the composite-pk branch from 9ad5d88 to 268426a Compare February 2, 2015 23:28

This was referenced Feb 11, 2015

Add failing test with ToOne SL2 association #1301

Closed

Store column values of not cache-able associations #1302

Merged

Ocramius added a commit that referenced this pull request Feb 16, 2015

#1113 - minor CS fixes (spacing/alignment)

2a99d5a

Ocramius closed this in f908974 Feb 16, 2015

goetas mentioned this pull request Feb 20, 2015

Test for DDC-2988 #1307

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composite primary key & association entities as keys improvements #1113

Composite primary key & association entities as keys improvements #1113

goetas commented Aug 18, 2014

doctrinebot commented Aug 18, 2014

Ocramius commented Aug 18, 2014

stof Aug 18, 2014

goetas commented Aug 19, 2014

goetas commented Aug 19, 2014

goetas commented Aug 19, 2014

deeky666 commented Aug 19, 2014

guilhermeblanco commented Aug 19, 2014

goetas commented Aug 19, 2014

goetas Aug 22, 2014

goetas commented Aug 22, 2014

deeky666 Aug 22, 2014

goetas Aug 22, 2014

stof Aug 22, 2014

goetas Aug 22, 2014

goetas commented Sep 1, 2014

ltouro Oct 2, 2014

goetas commented Oct 22, 2014

Ocramius commented Oct 22, 2014

goetas Jan 29, 2015

Ocramius Feb 1, 2015

goetas Feb 2, 2015

goetas commented Jan 29, 2015

Ocramius Feb 1, 2015

Ocramius Feb 1, 2015

goetas Feb 2, 2015

goetas commented Feb 2, 2015

Ocramius commented Feb 16, 2015

Ocramius commented Feb 16, 2015

Composite primary key & association entities as keys improvements #1113

Composite primary key & association entities as keys improvements #1113

Conversation

goetas commented Aug 18, 2014

doctrinebot commented Aug 18, 2014

Ocramius commented Aug 18, 2014

Choose a reason for hiding this comment

goetas commented Aug 19, 2014

goetas commented Aug 19, 2014

goetas commented Aug 19, 2014

deeky666 commented Aug 19, 2014

guilhermeblanco commented Aug 19, 2014

goetas commented Aug 19, 2014

Choose a reason for hiding this comment

goetas commented Aug 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Sep 1, 2014

Choose a reason for hiding this comment

goetas commented Oct 22, 2014

Ocramius commented Oct 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Jan 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented Feb 2, 2015

Ocramius commented Feb 16, 2015

Ocramius commented Feb 16, 2015