PHPLIB-618: Change estimatedDocumentCount to use $collStats #811

alcaeus · 2021-02-12T12:57:30Z

alcaeus · 2021-02-12T13:14:49Z

src/Operation/EstimatedDocumentCount.php


-        $this->count = $this->createCount();
+        if (isset($options['maxTimeMS']) && ! is_integer($options['maxTimeMS'])) {


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.

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.

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.

src/Operation/EstimatedDocumentCount.php

alcaeus · 2021-02-12T13:17:22Z

src/Operation/EstimatedDocumentCount.php

@@ -90,18 +115,53 @@ 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) {


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.

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.

alcaeus · 2021-02-12T13:19:29Z

tests/FunctionalTestCase.php

@@ -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']);


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.

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?

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.

alcaeus · 2021-02-12T13:20:23Z

tests/UnifiedSpecTests/UnifiedSpecTest.php

+    /**
+     * @dataProvider provideCrudTests
+     */
+    public function testCrud(...$args)


Having separate test methods for individual specs allows for easier filtering of what tests to execute (without having to mess with --filter and dataset names.

Is this change related to anything else in the PR? I don't see any tests added to the crud/ directory.

Given that UnifiedSpecTest is primarily testing its own tests (e.g. passing/failing), I wonder if other specs using the runner should just have their own test classes.

This would probably be best deferred until after I merge #806, as that PR extracts the runner itself from the test class. If this isn't urgent, I'd suggest pulling this out to avoid merge conflicts and just proposing this in a PHPLIB ticket dependent on PHPLIB-613.

Given that the tests for each spec would only contain a single test<Spec>(...$args) method, I'm not sure what there is to be gained by having each in a separate class. If we want to separate them somewhat, I'd suggest the following:

Refactor UnifiedSpecTest to an abstract UnifiedSpecTestCase class

Add a UnifiedRunnerTest class that only tests the runner itself (e.g. passing/failing)

From there, add either a common SpecTest class or a single <Spec>Test class for each spec that has tests in the unified format.

In the meantime, I can extract this change to a separate file until we're done with #806, especially considering that the PR for PHPLIB-622 will add versioned API tests where we'll see the problem. That should avoid merge conflicts in the refactoring PR.

To follow up on the tests themselves, there's a single new test file added to tests/UnifiedSpecTests/crud/, which is what is tested here. The other tests have not been ported to the unified format yet.

Thinking of it, I remember discussing where these tests would live, but don't remember coming to a conclusion. I've put them in the UnifiedSpecTests folder for the time being, but we may want to not use that directory as a catch-all for unified spec tests and rather use different directories for it.

Oh, I must have missed tests/UnifiedSpecTests/crud/estimatedDocumentCount.json in the previous review. I see it now. Fine to keep as-is. I'll sort the conflict out later in #806.

jmikola · 2021-03-22T18:34:01Z

src/Operation/EstimatedDocumentCount.php


-        $this->count = $this->createCount();
+        if (isset($options['maxTimeMS']) && ! is_integer($options['maxTimeMS'])) {


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.

jmikola · 2021-03-22T18:44:50Z

src/Operation/EstimatedDocumentCount.php

+        if ($command instanceof Aggregate) {
+            try {
+                $cursor = $command->execute($server);
+            } catch (CommandException $e) {


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

src/Operation/EstimatedDocumentCount.php

jmikola · 2021-03-22T18:47:03Z

src/Operation/EstimatedDocumentCount.php

+
+            $cursor->rewind();
+
+            return $cursor->current()->n;


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.

jmikola · 2021-03-22T18:49:04Z

src/Operation/EstimatedDocumentCount.php

@@ -90,18 +115,53 @@ 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) {


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.

jmikola · 2021-03-22T18:55:58Z

tests/FunctionalTestCase.php

@@ -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']);


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?

jmikola · 2021-03-22T18:59:58Z

tests/UnifiedSpecTests/UnifiedSpecTest.php

+    /**
+     * @dataProvider provideCrudTests
+     */
+    public function testCrud(...$args)


Is this change related to anything else in the PR? I don't see any tests added to the crud/ directory.

Given that UnifiedSpecTest is primarily testing its own tests (e.g. passing/failing), I wonder if other specs using the runner should just have their own test classes.

This would probably be best deferred until after I merge #806, as that PR extracts the runner itself from the test class. If this isn't urgent, I'd suggest pulling this out to avoid merge conflicts and just proposing this in a PHPLIB ticket dependent on PHPLIB-613.

jmikola

The coding standards check failed. Once that is addressed, LGTM.

alcaeus requested a review from jmikola February 12, 2021 12:57

alcaeus self-assigned this Feb 12, 2021

alcaeus force-pushed the phplib-618 branch 2 times, most recently from 1163bc5 to 03d91f1 Compare February 12, 2021 13:14

alcaeus commented Feb 12, 2021

View reviewed changes

alcaeus force-pushed the phplib-618 branch 2 times, most recently from 24c8342 to 0b8362b Compare February 15, 2021 12:37

jmikola reviewed Mar 22, 2021

View reviewed changes

jmikola mentioned this pull request Mar 22, 2021

PHPLIB-624 Sync change stream spec tests for delta oplog entries #814

Merged

alcaeus requested a review from jmikola March 24, 2021 08:59

jmikola approved these changes Mar 24, 2021

View reviewed changes

alcaeus added 3 commits March 25, 2021 08:29

PHPLIB-618: Change estimatedDocumentCount to use $collStats

d9d9216

Address CR

847b6a3

Fix checkstyle errors

f65bb9f

alcaeus force-pushed the phplib-618 branch from 2d9890b to f65bb9f Compare March 25, 2021 07:32

alcaeus merged commit 2c80b35 into mongodb:master Mar 25, 2021

alcaeus deleted the phplib-618 branch March 25, 2021 08:05


		$this->count = $this->createCount();
		if (isset($options['maxTimeMS']) && ! is_integer($options['maxTimeMS'])) {

PHPLIB-618: Change estimatedDocumentCount to use $collStats #811

PHPLIB-618: Change estimatedDocumentCount to use $collStats #811

Uh oh!

Conversation

alcaeus commented Feb 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola Mar 24, 2021 •

edited

Loading