Skip to content

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

Merged
merged 3 commits into from
Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 72 additions & 11 deletions src/Operation/EstimatedDocumentCount.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
*
Expand All @@ -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'])) {
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

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]);
}

/**
Expand All @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we expose the command document using getCommandDocument, I thought it was best to create the command and attach special handling to the aggregate command, as opposed to checking for the correct wire version here again. Let me know if you'd prefer an explicit wire version check here instead of the hidden check in createCommand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize getCommandDocument complicates things a bit. I think this is fine.

I'd probably just suggest a blank line between assigning $command and the if for readability.

try {
$cursor = $command->execute($server);
} catch (CommandException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that ListIndexes uses MongoDB\Driver\Exception\RuntimeException for this logic. Reported that in PHPLIB-636.

if ($e->getCode() == self::$errorCodeCollectionNotFound) {
return 0;
}

throw $e;
}

$cursor->rewind();

return $cursor->current()->n;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this worth an (integer) cast just to be sure? I wouldn't expect anything else to be returned, but it is technically a wild value from the server.

}

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);
}
Expand Down
3 changes: 2 additions & 1 deletion tests/FunctionalTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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']);
Copy link
Member Author

Choose a reason for hiding this comment

The 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 4.9.0-alpha4-... as version number, which version_compare interprets as being lower than 4.9.0 (due to the alpha flag). The Version String in the unified test format makes no mention of stabilities, but instead requires the version to conform to the <major>.<minor>.<patch> schema.

Copy link
Member

Choose a reason for hiding this comment

The 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 versionArray and build the string ourselves. Presumably, we'll need to do this for the test runners as well?

Copy link
Member Author

Choose a reason for hiding this comment

The 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');
Expand Down
8 changes: 4 additions & 4 deletions tests/SpecTests/ClientSideEncryptionSpecTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,8 @@ function ($command) use (&$commands) {
}
);

$test->assertCount(2, $commands);
},
$test->assertCount(2, $commands);
},
];

yield 'Test 4' => [
Expand Down Expand Up @@ -424,8 +424,8 @@ function ($command) use (&$commands) {
}
);

$test->assertCount(2, $commands);
},
$test->assertCount(2, $commands);
},
];

yield 'Test 5' => [
Expand Down
Loading