Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[BC break!] Fix DocumentRepository to fit ObjectRepository interface #752

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

lemoinem commented Jan 9, 2014

Hello,

https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/ObjectRepository.php#L60 explicitly document the findBy() method as returning an array of objects.

This interface spec is broken in spec and behaviour by https://github.com/doctrine/mongodb-odm/blob/master/lib/Doctrine/ODM/MongoDB/DocumentRepository.php#L164

This PR aims to fix this. It is quite a major BC break for the MongoDB ODM (as the various updated tests show), but I believe the interface shouldn't have been broken in the first place. We were able to break the interface only because PHP doesn't provide a way to check the type of the return value of a method and because Doctrine Common does not provide implementation tests for its interfaces (I don't intend this remark as a critic, but to describe a fact here 😉). It prevents code base engineered to interact with the Doctrine\Common\Persistence interfaces to crash miserably without clean workarounds.

In order to minimize BC break. I provided the protected method getDocumentPersister() in the DocumentRepository so custom repositories could easily return a Cursor (For example, to load associations through custom repository methods).

If you have any ideas that could fix this without BC Break, I would be happy to ear about it and try as hard as possible to reduce BC breaks and update my PR accordingly.

Owner

jmikola commented Jan 9, 2014

We've definitely known about this for a while, but it's been brushed aside as a dirty little secret.

ObjectRepository should technically allow array|Traversable to be returned. Converting MongoDB cursors to arrays up front can drastically affect memory and performance.

@Ocramius, @jwage: Thoughts?

Owner

Ocramius commented Jan 9, 2014

@jmikola we can't change that interface in 2.x - I'd rather fix the behavior here in a minor bump. Users should not have relied on the cursor anyway given the interface...

Owner

jmikola commented Jan 15, 2014

@jwage: Thoughts?

Owner

jwage commented Jan 16, 2014

👍 for this necessary BC break.

@jmikola jmikola commented on an outdated diff Jan 16, 2014

lib/Doctrine/ODM/MongoDB/DocumentRepository.php
*/
public function findBy(array $criteria, array $sort = null, $limit = null, $skip = null)
{
- return $this->uow->getDocumentPersister($this->documentName)->loadAll($criteria, $sort, $limit, $skip);
+ return iterator_to_array($this->getDocumentPersister()->loadAll($criteria, $sort, $limit, $skip));
@jmikola

jmikola Jan 16, 2014

Owner

You should add false as the second iterator_to_array() argument here, in case the document has a non-scalar _id value. This will also ensure the user gets back a numerically indexed (i.e. real) array, instead of an associative one.

@jmikola jmikola commented on an outdated diff Jan 16, 2014

...octrine/ODM/MongoDB/Tests/Functional/CustomIdTest.php
@@ -75,7 +75,7 @@ public function testBatchInsertCustomId()
unset($user1, $user2, $user3);
- $users = $this->dm->getRepository("Documents\User")->findAll();
+ $users = $this->uow->getDocumentPersister("Documents\User")->loadAll();
@jmikola

jmikola Jan 16, 2014

Owner

This test only counts $users and iterates over it with foreach(), so why not use the findAll() array return value?

@jmikola jmikola commented on an outdated diff Jan 16, 2014

...octrine/ODM/MongoDB/Tests/Functional/CustomIdTest.php
@@ -75,7 +75,7 @@ public function testBatchInsertCustomId()
unset($user1, $user2, $user3);
- $users = $this->dm->getRepository("Documents\User")->findAll();
+ $users = $this->uow->getDocumentPersister("Documents\User")->loadAll();
$this->assertEquals(2, $users->count());
@jmikola

jmikola Jan 16, 2014

Owner

The above change would entail changing this to count($users). Alternatively, you can just use assertCount().

@jmikola jmikola commented on an outdated diff Jan 16, 2014

...octrine/ODM/MongoDB/Tests/Functional/CustomIdTest.php
@@ -90,7 +90,7 @@ public function testBatchInsertCustomId()
$users = $this->dm->getRepository("Documents\CustomUser")->findAll();
- $this->assertEquals(1, $users->count());
+ $this->assertEquals(1, count($users));
@jmikola

jmikola Jan 16, 2014

Owner

Likewise re: assertCount().

@jmikola jmikola commented on an outdated diff Jan 16, 2014

...ine/ODM/MongoDB/Tests/Functional/RepositoriesTest.php
@@ -33,7 +33,7 @@ public function testFindAll()
{
$users = $this->repository->findAll();
- $this->assertInstanceOf('Doctrine\ODM\MongoDB\Cursor', $users);
+ $this->assertInternalType('array', $users);
$this->assertEquals(1, count($users));
@jmikola

jmikola Jan 16, 2014

Owner

assertCount()?

@jmikola jmikola commented on an outdated diff Jan 16, 2014

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH580Test.php
@@ -15,7 +15,7 @@ public function testDocumentPersisterShouldClearQueuedInsertsOnMongoException()
$repository = $this->dm->getRepository($class);
- $this->assertEquals(0, $repository->findAll()->count());
+ $this->assertEquals(0, count($repository->findAll()));
@jmikola

jmikola Jan 16, 2014

Owner

assertCount()?

@jmikola jmikola commented on an outdated diff Jan 16, 2014

...ine/ODM/MongoDB/Tests/Functional/Ticket/GH580Test.php
@@ -54,7 +54,7 @@ public function testDocumentPersisterShouldClearQueuedInsertsOnMongoException()
/* Repository should contain one object, but may contain two if the
* DocumentPersister was not cleaned up.
*/
- $this->assertEquals(1, $repository->findAll()->count());
+ $this->assertEquals(1, count($repository->findAll()));
@jmikola

jmikola Jan 16, 2014

Owner

assertCount()?

Owner

jmikola commented Jan 16, 2014

@lemoinem: Please make the above changes (feel free to squash commits), and I'll create the changelog entry for this before merging. Thanks!

Contributor

lemoinem commented Jan 16, 2014

@jmikola Thanks for the feedback. I took your comments into account and double checked my commit, I don't think I forget anything. Here is the new one!

Contributor

lemoinem commented Jan 16, 2014

Here, I rebased on current master and update the phpDoc (to change @return array to @return array The entities., to be consistent with findAll). I think I'm done with playing around with the branch.

Owner

jmikola commented Jan 16, 2014

Manually merged in 64ed807.

I left @return array as-is, as "The entities" seemed redundant.

@jmikola jmikola closed this Jan 16, 2014

Contributor

tgabi333 commented Jan 17, 2014

I have two questions in my mind:
1, there is a Cursor::toArray method, wouldn't be better to use that against iterator_to_array, keeping the consistency of result sets?
2, we used to call Cursor::snapshot method in context of findAll like query-s, it could help keeping up eventual consistency

Contributor

lemoinem commented Jan 17, 2014

  1. Cursor::toArray() actually uses iterator_to_array internally. So it shouldn't change anything.
  2. Cursor::snapshot(), while highly relevant, seems both impossible to use in the general case (php.net => Currently, snapshot mode may not be used with sorting or explicit hints) and not the right place to use it (I think it would be better off in the Cursor::toArray() method, in which case my 1. comment would be made invalid).
Contributor

tgabi333 commented Jan 17, 2014

@lemoinem , thank for your answer. As i see, calling toArray() would be a good example of DRY principle.

lemoinem added a commit to lemoinem/mongodb-odm that referenced this pull request Jan 17, 2014

[DocumentRepository] Use Cursor::toArray() instead of iterator_to_arr…
…ay()

As per doctrine#752 (comment)
by @tgabi333 and following. This replace a direct call to iterator_to_array by
a call to Cursor::toArray() (DRY principal and better respect of encapsulation).
Contributor

lemoinem commented Jan 17, 2014

@tgabi333 Indeed. To be frank, I didn't even consider using toArray() in my original PR. New PR (#769) addressing this issue.

There is still whether calling Cursor::snapshot() would be a good idea too. The documentation is not explicit as to what the behavior would be when it may not be called... It looks like it's undefined behavior, so I would prefer not to call it when the doc says we shouldn't.

Perhaps including a test to ensure we are under the right conditions? Consistency is actually rather important IMHO.

@jmikola and others, Any thoughts?

lemoinem added a commit to lemoinem/mongodb-odm that referenced this pull request Jan 17, 2014

[DocumentRepository] Use Cursor::toArray() instead of iterator_to_arr…
…ay()

As per doctrine#752 (comment)
by @tgabi333 and following. This replaces a direct call to iterator_to_array by
a call to Cursor::toArray() (DRY principal and better respect of encapsulation).
Owner

jmikola commented Jan 17, 2014

@tgabi333: Thanks for bringing this up.

@lemoinem: snapshot() should be left to the query builder and something that users opt in to use. I think simply switching over to toArray() is sufficient. I'll follow up in #769.

Contributor

lemoinem commented Jan 17, 2014

@jmikola Yes, I agree, but in the case of findAll or findBy, the user doesn't have access to the QueryBuilder or the Cursor, although they will (and should) expect consistency...

@lemoinem lemoinem deleted the lemoinem:fix/repository-find-by-returns-cursor branch Jan 17, 2014

Owner

jmikola commented Jan 17, 2014

snapshot() is mainly useful for ensuring you don't iterate over the same result document multiple times. A common example would be if you're iterating through a result set and updating documents as you go through. If one of the updates requires the document to be moved on disk, its index entry might be updated and we could see it a second time later in the iteration.

If we exhaust the result set immediately by converting it to an array, it's less likely a snapshot would be necessary. Granted, it's still possible if another connection changes and moves data around while the driver fetches all of the batches.

Contributor

lemoinem commented Jan 17, 2014

Fair point! Thanks for the explanation.
@tgabi333 I hope the discussion cleared it up for you too. Thanks for the feedback!

Contributor

tgabi333 commented Jan 17, 2014

Thank you guys.

jmikola added a commit that referenced this pull request Feb 24, 2014

Construct query builder in QueryCommand (fixes #801)
Since #752, the repository find methods return arrays instead of cursors in order to be consistent with the common repository interface. With this change, a query builder is used, and the skip/limit/hydrate options can be applied to the builder directly.

@jmikola jmikola referenced this pull request in doctrine/DoctrineMongoODMModule Oct 2, 2014

Closed

Paginator does not work with Mongodb ODM > 1.0.0-beta10@dev #107

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