Restore document proxy state to uninitialized on load exception #2521
Conversation
notrix
force-pushed
the
fix-proxy-state-on-init-exception
branch
2 times, most recently
from
a7b30d5
to
bb532ef
Compare
|
@malarzm I would like some comments in here: do you agree that it is a bug to leave ghost object initialized if there was exception loading data? |
I'm not sure yet :) Sounds plausible but I haven't given it a deeper thought yet
Given your description it's better to classify as a bug, in which case
Please rebase atop 2.5.x - there checks are green :) |
notrix
force-pushed
the
fix-proxy-state-on-init-exception
branch
from
bb532ef
to
cdd5c90
Compare
notrix
force-pushed
the
fix-proxy-state-on-init-exception
branch
3 times, most recently
from
cc182e9
to
dda9e4f
Compare
|
|
||
| $collection->expects($this->once()) | ||
| ->method('findOne') | ||
| ->willThrowException(LockException::lockFailed(null)); |
There was a problem hiding this comment.
I'm a bit torn so here goes nothing: how about instead of mocking 3 levels deep we mock DocumentPersister, have it thrown an exception on load? This would require manipulating UnitOfWork::$documentPersisters with reflection though, but the test should be easier to maintain with real database connection. For your consideration :)
There was a problem hiding this comment.
At first I also wanted to mock DocumentPersister::load, but DocumentPersister is a final class. So my best shot was to mock Collection::findOne
| $metadata = $this->dm->getClassMetadata(PaymentProvider::class); | ||
|
|
||
| $proxy = $this->proxyFactory->getProxy($metadata, '123'); | ||
| $uow->registerManaged($proxy, '123', []); |
There was a problem hiding this comment.
It's better to call DocumentManager::getReference
tests/Doctrine/ODM/MongoDB/Tests/Proxy/Factory/StaticProxyFactoryTest.php
Show resolved
Hide resolved
| try { | ||
| $proxy->initializeProxy(); | ||
| } catch (LockException $exception) { | ||
| $this->assertInstanceOf(LockException::class, $exception); |
There was a problem hiding this comment.
Please use self::assert* wherever possible, most of code was migrated to that
| $uow->computeChangeSets(); | ||
|
|
||
| $this->assertFalse($uow->isScheduledForUpdate($proxy), 'Proxy should not be scheduled for update'); |
There was a problem hiding this comment.
In my opinion this is not needed. The test should care whether proxy is still uninitialized after the exception, not how change set computation works with uninitialized proxies (there are other tests for that).
| $uow->computeChangeSets(); | ||
|
|
||
| $this->assertFalse($uow->isScheduledForUpdate($proxy), 'Proxy should not be scheduled for update'); | ||
| $this->assertFalse($proxy->isProxyInitialized(), 'Proxy should not be initialized'); |
There was a problem hiding this comment.
If possible, the test should also add an event listener for documentNotFound and assert the listener is never called.
tests/Doctrine/ODM/MongoDB/Tests/Proxy/Factory/StaticProxyFactoryTest.php
Show resolved
Hide resolved
tests/Documents/PaymentProvider.php
Outdated
| use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM; | ||
|
|
||
| /** @ODM\Document */ | ||
| class PaymentProvider |
There was a problem hiding this comment.
Could you please use any existing document instead of adding new one?
There was a problem hiding this comment.
The main issue for me was that my document has default values and those default value were flushed to database. But since we do not assert change set - I have removed new document
| { | ||
| } | ||
|
|
||
| protected function createMockedDocumentManager(): DocumentManager |
There was a problem hiding this comment.
| protected function createMockedDocumentManager(): DocumentManager | |
| private function createMockedDocumentManager(): DocumentManager |
No need for protected visibility unless this was supposed to be createTestDocumentManager originally?
notrix
force-pushed
the
fix-proxy-state-on-init-exception
branch
from
dda9e4f
to
6b95e60
Compare
notrix
force-pushed
the
fix-proxy-state-on-init-exception
branch
from
6b95e60
to
9bbe84a
Compare
|
And enjoy 2.5.2 ;) |
Summary
This PR should fix issue when exception is thrown during proxy object load. If application catches exceptions and flushes at the end of request, unit of work finds changes for fields that have default values. I have seen network glitches of Amazon KMS service on load and successful flush at the end. This broke production environment badly.
Proxy is marked as not initialized if id is not found in database. So I think same should be if exception happens on load event.