OneToOne relationship with owning side having key of inverse side broken

[asgrim]

gist created by @Ocramius shows a reproducible case for this issue: https://gist.github.com/Ocramius/7adc2079884991122ca95a74d2eb4927

When hydrating an entity constructed as per the gist above using findOneBy, the ORM raises a notice starting Notice: Undefined index: targetToSourceKeyColumns. I believe the issue is not reproducible when using find.

Unsure as to the root cause of the issue currently. Apologies for the vague description here, but @Ocramius will hopefully be able to provide more detail about this issue.

[lcobucci]

I executed that gist (just changing the names to GH6124* to relate to this issue) and I had a different error on master - so probably we already fixed that notice.

Basically the ORM complains that it wasn't able to call the method BasicEntityPersister#getCacheRegion() because the mapping side is not cacheable. However if I enable L2C on the OwingSide class, and also on the association, the test passes with find() and findOneBy(): https://gist.github.com/lcobucci/665d1f03d72af1c03c5e76af6d64b1d1

PHPUnit 5.7-gb0c4a96 by Sebastian Bergmann and contributors.

E                                                                   1 / 1 (100%)

Time: 415 ms, Memory: 10.00MB

There was 1 error:

1) Doctrine\Tests\ORM\Functional\Ticket\GH6124Test::testIssue
Exception: [Error] Call to undefined method Doctrine\ORM\Persisters\Entity\BasicEntityPersister::getCacheRegion()

With queries:
9. SQL: 'SELECT t0.id AS id_1, t0.name AS name_2, t3.gh6124_inverse_side_id AS gh6124_inverse_side_id_4 FROM GH6124InverseSide t0 LEFT JOIN GH6124OwningSide t3 ON t3.gh6124_inverse_side_id = t0.id WHERE t0.name = ? LIMIT 1' Params: 'name58233cc3e27ba1.69337219'
8. SQL: '"COMMIT"' Params: 
7. SQL: 'INSERT INTO GH6124OwningSide (gh6124_inverse_side_id) VALUES (?)' Params: 1
6. SQL: '"START TRANSACTION"' Params: 
5. SQL: '"COMMIT"' Params: 
4. SQL: 'INSERT INTO GH6124InverseSide (name) VALUES (?)' Params: 'name58233cc3e27ba1.69337219'
3. SQL: '"START TRANSACTION"' Params: 
2. SQL: 'CREATE TABLE GH6124OwningSide (gh6124_inverse_side_id INTEGER NOT NULL, PRIMARY KEY(gh6124_inverse_side_id), CONSTRAINT FK_4833BEE3DCA6C60D FOREIGN KEY (gh6124_inverse_side_id) REFERENCES GH6124InverseSide (id) NOT DEFERRABLE INITIALLY IMMEDIATE)' Params: 
1. SQL: 'CREATE TABLE GH6124InverseSide (id INTEGER NOT NULL, name VARCHAR(255) NOT NULL, PRIMARY KEY(id))' Params: 

Trace:
/vhosts/doctrine2/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:278
/vhosts/doctrine2/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php:389
/vhosts/doctrine2/lib/Doctrine/ORM/EntityRepository.php:196
/vhosts/doctrine2/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6124Test.php:54
/vhosts/doctrine2/vendor/phpunit/phpunit/src/Framework/TestCase.php:1103
/vhosts/doctrine2/vendor/phpunit/phpunit/src/Framework/TestCase.php:954
/vhosts/doctrine2/vendor/phpunit/phpunit/src/Framework/TestResult.php:701
/vhosts/doctrine2/vendor/phpunit/phpunit/src/Framework/TestCase.php:909
/vhosts/doctrine2/vendor/phpunit/phpunit/src/Framework/TestSuite.php:753
/vhosts/doctrine2/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:465
/vhosts/doctrine2/vendor/phpunit/phpunit/src/TextUI/Command.php:185
/vhosts/doctrine2/vendor/phpunit/phpunit/src/TextUI/Command.php:115
/vhosts/doctrine2/vendor/phpunit/phpunit/phpunit:47


/vhosts/doctrine2/tests/Doctrine/Tests/OrmFunctionalTestCase.php:785

Caused by
Error: Call to undefined method Doctrine\ORM\Persisters\Entity\BasicEntityPersister::getCacheRegion()

/vhosts/doctrine2/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:321
/vhosts/doctrine2/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:278
/vhosts/doctrine2/lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php:389
/vhosts/doctrine2/lib/Doctrine/ORM/EntityRepository.php:196
/vhosts/doctrine2/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6124Test.php:54

ERRORS!
Tests: 1, Assertions: 3, Errors: 1.

[asgrim]

I think that may be related then; in this circumstance the owning side GH6124OwningSide can't be in the L2C as the contents are written to outside the scope of the application (not our choice, I hasten to add). Our workaround for now is to make it a OneToMany relationship which works (although isn't ideal), as we can't rely on the contents being L2-cachable.

[lcobucci]

We need to think about what should be the behaviour of the L2C query regarding this... I mean, we could easily hack something on DefaultQueryCache#put or DefaultQueryCache#storeAssociationCache to bypass associations that are not L2-cacheable something like:

private function storeAssociationCache(QueryCacheKey $key, array $assoc, $assocValue)
{
    $assocPersister = $this->uow->getEntityPersister($assoc['targetEntity']);
    $assocMetadata  = $assocPersister->getClassMetadata();

    if (!$assocPersister instanceof Cache\Persister\Entity\CachedEntityPersister) {
        return []; // returning null would invalidate the whole query cache entry
    }
    // ...
}

But is this right?

[lcobucci]

Maybe @guilhermeblanco can help us on this one

[lcobucci]

Also mentioned on #5808 (but here we have more detail).

[lcobucci]

@asgrim @Ocramius I was investigating this issue last evening and unfortunately my conclusion is that is not really worth to have a way to workaround the cache on this situation.

My suggestion is to simply make the schema validator add a warning. The reason for this is the way the ORM handles one-to-one associations.

What are your thoughts?

[Ocramius]

A limitation warning referencing an issue is sufficient for me. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Wed, Feb 22, 2017 at 10:10 AM, Luís Cobucci ***@***.***> wrote: @asgrim <https://github.com/asgrim> @Ocramius <https://github.com/Ocramius> I was investigating this issue last evening and unfortunately my conclusion is that is not really worth to have a way to workaround the cache on this situation. My suggestion is to simply make the schema validator add a warning. The reason for this is the way the ORM handles one-to-one associations. What are your thoughts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6124 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakOL5R08jRUZXSQY36Lr3G2VzmOSLks5re_ubgaJpZM4KtlTJ> .

[guilhermeblanco]

@lcobucci Did you re-run the test? It seems we already check for that in https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Cache/DefaultQueryCache.php#L249 and this shouldn't happen anymore.

However, it might be worth looking into switching the association to owning side and caching the owning side (which must be cacheable, or you would have an exception).

I'm long away of master (in develop there's no more targetToSourceKeyColumns)... field is greener there! =)

[lcobucci]

@guilhermeblanco I did.

On master the problem is not the targetToSourceKeyColumns anymore but actually a call to BasicEntityPersister#getCacheRegion().

I agree that we shouldn't allow that on storeAssociationCache() and throw an exception but we should also alert the users that an uncached owning side on an one-to-one association is an invalid mapping.

I'm long away of master (in develop there's no more targetToSourceKeyColumns)... field is greener there! =)

That's excellent!

[guilhermeblanco]

@lcobucci Makes sense now... however I don't think it's a matter of implying to have a @Cache mapped. *-to-one should be implicit per entity definition and added to the entity cache (not as a separate record). Let me try to explain:

Whenever you have a class like this:

/**
 * @Entity
 * @Cache
 */
class User {
    /**
     * @Id @GeneratedValue @Column(type="integer")
     */
    public $id;

    /**
     * @ManyToOne(targetEntity="Group")
     */
    public $group;

    /**
     * @OneToMany(targetEntity="Phone")
     * @Cache
     */
    public $phones = [];
}

In this scenario, it is expected to have 2 cache entries:

$entries = [
   'region_name:user_1_hash' => ['id'=> 1, 'group'=>['id'=>1]],
   'region_name:user_1_phones_hash' => ['ownerId'=> 1, 'list' => [1, 2, 3]],
];

As you may see, we don't necessarily need to add @Cache in the *-to-one association since it must be implicit to exist in the owning side as part of the cache entry.
If it doesn't, then you'd always be forced to query the DB even to build the proxy, defeating the purpose of having SLC at all. At the same time, if a *-to-many doesn't have @Cache configured, this means the second entry wouldn't exist, forcing the DB query to happen when loading the collection.

There's also the limitation we introduced that other side must be cached (which means, both Group and Phone require to be @Cache).
So here is where Doctrine becomes deficient: As you may see, this is not entirely required, but we enforce this because collection loading is fetched based on the owner id, which would completely trash the need of having the collection marked as @Cache.
We cannot safely rewrite the portions inside PersistentCollection to achieve that, so this requirement needs to stay until 3.0.

However, it's an optional action to cache the collection association (*-to-many), and implicit for *-to-one (owning side) associations.

Things get tricky when you have bi-directionals. I think it's worth discussing on this thread and properly implement. I don't think we should add a warning anywhere tbh.

[lcobucci]

@guilhermeblanco it's not about *-to-one but rather one-to-one. It happens because the ORM always tries to fetch the inversed side of one-to-one eagerly and that breaks stuff (if the entity is not cacheable on the L2C).

So my suggestion to only force the @Cache on that situation.

[lcobucci]

@Ocramius @guilhermeblanco I was thinking about move this to v3.0 since we're changing a lot of things regarding mapping, do you agree?

[Ocramius]

Full ack - this can be fixed easily now that we have a stricter metadata API thanks to @guilhermeblanco's efforts.

[Webonaute]

Is there a solutions for ppl who are waiting for this to be fixed? in the mean time of 3.0?

[Ocramius]

@Webonaute if you didn't see one in the thread: no.

[asgrim]

@Webonaute did you try:

Our workaround for now is to make it a OneToMany relationship which works (although isn't ideal), as we can't rely on the contents being L2-cachable.

[pkly]

Hey, I just hit this issue when attempting to enable L2 cache for our application. Is there truly nothing else other than making it a OneToMany relationship? It breaks some assumptions in the db which is... not preferable, obviously.

I was hoping it'd be as simple as enabling cache on a couple of our entities but since the relationships are complex (about 190 entities) lack of support for this is a deal breaker, and L2 is essentially unusable.

Is there a reason it was moved away from a 3.0 release milestone? I'd love to see this resolved in the near future. Perhaps for now it'd be okay to simply mention that 1:1 relationships cannot be cached and instead either load them from database on request as usual or eagerly if set? It seems to me one could simply store some metadata instead which would tell it which cache key to read or if to pull it from db instead.