-
Notifications
You must be signed in to change notification settings - Fork 266
PHPLIB-618: Change estimatedDocumentCount to use $collStats #811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,12 +17,18 @@ | |
|
||
namespace MongoDB\Operation; | ||
|
||
use MongoDB\Driver\Exception\CommandException; | ||
use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException; | ||
use MongoDB\Driver\ReadConcern; | ||
use MongoDB\Driver\ReadPreference; | ||
use MongoDB\Driver\Server; | ||
use MongoDB\Driver\Session; | ||
use MongoDB\Exception\InvalidArgumentException; | ||
use MongoDB\Exception\UnexpectedValueException; | ||
use MongoDB\Exception\UnsupportedException; | ||
use function array_intersect_key; | ||
use function is_integer; | ||
use function MongoDB\server_supports_feature; | ||
|
||
/** | ||
* Operation for obtaining an estimated count of documents in a collection | ||
|
@@ -42,11 +48,15 @@ class EstimatedDocumentCount implements Executable, Explainable | |
/** @var array */ | ||
private $options; | ||
|
||
/** @var Count */ | ||
private $count; | ||
/** @var int */ | ||
private static $errorCodeCollectionNotFound = 26; | ||
|
||
/** @var int */ | ||
private static $wireVersionForCollStats = 12; | ||
|
||
/** | ||
* Constructs a count command. | ||
* Constructs a command to get the estimated number of documents in a | ||
* collection. | ||
* | ||
* Supported options: | ||
* | ||
|
@@ -73,9 +83,24 @@ public function __construct($databaseName, $collectionName, array $options = []) | |
{ | ||
$this->databaseName = (string) $databaseName; | ||
$this->collectionName = (string) $collectionName; | ||
$this->options = array_intersect_key($options, ['maxTimeMS' => 1, 'readConcern' => 1, 'readPreference' => 1, 'session' => 1]); | ||
|
||
$this->count = $this->createCount(); | ||
if (isset($options['maxTimeMS']) && ! is_integer($options['maxTimeMS'])) { | ||
throw InvalidArgumentException::invalidType('"maxTimeMS" option', $options['maxTimeMS'], 'integer'); | ||
} | ||
|
||
if (isset($options['readConcern']) && ! $options['readConcern'] instanceof ReadConcern) { | ||
throw InvalidArgumentException::invalidType('"readConcern" option', $options['readConcern'], ReadConcern::class); | ||
} | ||
|
||
if (isset($options['readPreference']) && ! $options['readPreference'] instanceof ReadPreference) { | ||
throw InvalidArgumentException::invalidType('"readPreference" option', $options['readPreference'], ReadPreference::class); | ||
} | ||
|
||
if (isset($options['session']) && ! $options['session'] instanceof Session) { | ||
throw InvalidArgumentException::invalidType('"session" option', $options['session'], Session::class); | ||
} | ||
|
||
$this->options = array_intersect_key($options, ['maxTimeMS' => 1, 'readConcern' => 1, 'readPreference' => 1, 'session' => 1]); | ||
} | ||
|
||
/** | ||
|
@@ -90,18 +115,54 @@ public function __construct($databaseName, $collectionName, array $options = []) | |
*/ | ||
public function execute(Server $server) | ||
{ | ||
return $this->count->execute($server); | ||
$command = $this->createCommand($server); | ||
|
||
if ($command instanceof Aggregate) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we expose the command document using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize I'd probably just suggest a blank line between assigning |
||
try { | ||
$cursor = $command->execute($server); | ||
} catch (CommandException $e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed that ListIndexes uses |
||
if ($e->getCode() == self::$errorCodeCollectionNotFound) { | ||
return 0; | ||
} | ||
|
||
throw $e; | ||
} | ||
jmikola marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
$cursor->rewind(); | ||
|
||
return $cursor->current()->n; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this worth an |
||
} | ||
|
||
return $command->execute($server); | ||
} | ||
|
||
public function getCommandDocument(Server $server) | ||
{ | ||
return $this->count->getCommandDocument($server); | ||
return $this->createCommand($server)->getCommandDocument($server); | ||
} | ||
|
||
/** | ||
* @return Count | ||
*/ | ||
private function createCount() | ||
private function createAggregate() : Aggregate | ||
{ | ||
return new Aggregate( | ||
$this->databaseName, | ||
$this->collectionName, | ||
[ | ||
['$collStats' => ['count' => (object) []]], | ||
['$group' => ['_id' => 1, 'n' => ['$sum' => '$count']]], | ||
], | ||
$this->options | ||
); | ||
} | ||
|
||
/** @return Aggregate|Count */ | ||
private function createCommand(Server $server) | ||
{ | ||
return server_supports_feature($server, self::$wireVersionForCollStats) | ||
? $this->createAggregate() | ||
: $this->createCount(); | ||
} | ||
|
||
private function createCount() : Count | ||
{ | ||
return new Count($this->databaseName, $this->collectionName, [], $this->options); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ | |
use function phpinfo; | ||
use function preg_match; | ||
use function preg_quote; | ||
use function preg_replace; | ||
use function sprintf; | ||
use function version_compare; | ||
use const INFO_MODULES; | ||
|
@@ -278,7 +279,7 @@ protected function getServerVersion(ReadPreference $readPreference = null) | |
$document = current($cursor->toArray()); | ||
|
||
if (isset($document['version']) && is_string($document['version'])) { | ||
return $document['version']; | ||
return preg_replace('#^(\d+\.\d+\.\d+).*$#', '\1', $document['version']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change was necessary since my tests were being skipped. My local MongoDB server reported There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems related to mongodb/specifications#931. Works for me. The other alternative would be to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that issue was raised afterwards, and we have PHPLIB-634 as a downstream ticket to take care of this. If you don't mind I'd leave this in for now (as it ensures that the tests run as expected) and then the logic for all test runners as part of PHPLIB-634. |
||
} | ||
|
||
throw new UnexpectedValueException('Could not determine server version'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided that checking options for
estimatedDocumentCount
here specifically makes sense now that we no longer know which command (count vs. aggregate) is being executed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both Count and Aggregate do the same checks for these options, so does this change anything?
Perhaps this is needed to ensure we raise these errors in the constructor, since construction of the other operation is now deferred to
execute()
? I suppose the only alternative would be to create both a Count and Aggregate operation up front and decide which is used later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the main difference. I considered instantiating both operation objects up front, but avoided this as one object may throw an exception during initialisation even though it wouldn't be used later on.