PHPLIB-1144: Add unit tests on `Explainable::getCommandDocument()` #1105

GromNaN · 2023-06-14T13:12:01Z

Fix PHPLIB-1144

Add let and comment options to explain delete.
~~Remove explain option for Aggregate, it is incompatible with the explain command.~~

Adding a unit test will full options for each implementation of Explainable::getCommandDocument:

Count ✅
Delete ✅
DeleteMany covered by Delete
DeleteOne covered by Delete
Distinct ✅
EstimatedDocumentCount covered by Count
Find ✅
FindAndModify ✅
FindOne covered by Find
FindOneAndDelete ✅ + covered by FindAndModify
FindOneAndReplace ✅ + covered by FindAndModify
FindOneAndUpdate ✅ + covered by FindAndModify
Update ✅
UpdateMany covered by Update
UpdateOne covered by Update

These tests allow us to review exactly which options are sent to the explain command.

The comparison of ReadConcern values will become stronger with PHPC-2234.

tests/Operation/FindAndModifyTest.php

GromNaN · 2023-06-14T13:31:52Z

tests/Operation/FindTest.php

+            'noCursorTimeout' => true,
+            'oplogReplay' => true,
+            'projection' => ['_id' => 0],
+            'readConcern' => new ReadConcern(ReadConcern::LOCAL),


I know that this is not an effective comparison of the readConcern value, at least it asserts the readConcern option is sent. The assertion will be stronger once PHPC-2234 is implemented.

I suppose it's not worth adding a @todo comment here since the code won't need to be changed in PHPLIB. This is worth calling out in the commit message, though (with a reference to PHPC-2234).

tests/Operation/DeleteTest.php

tests/Operation/DistinctTest.php

tests/Operation/FindAndModifyTest.php

jmikola · 2023-06-15T15:34:16Z

tests/Operation/FindOneAndDeleteTest.php

+            'projection' => ['_id' => 0],
+            'sort' => ['x' => 1],
+            'let' => ['a' => 3],
+            'typeMap' => ['root' => 'array'],


I suggest adding a comment before any ignored options.

jmikola · 2023-06-15T15:35:26Z

tests/Operation/FindTest.php

+            'noCursorTimeout' => true,
+            'oplogReplay' => true,
+            'projection' => ['_id' => 0],
+            'readConcern' => new ReadConcern(ReadConcern::LOCAL),


I suppose it's not worth adding a @todo comment here since the code won't need to be changed in PHPLIB. This is worth calling out in the commit message, though (with a reference to PHPC-2234).

GromNaN · 2023-06-16T13:12:10Z

src/Operation/Aggregate.php

@@ -300,6 +300,10 @@ public function getCommandDocument()
    {
        $cmd = $this->createCommandDocument();

+        // The 'explain' option is incompatible with the explain command
+        unset($cmd['explain']);


I get the following error if the option explain is set, even if it's false.

MongoDB\Driver\Exception\CommandException: The 'explain' option is illegal when a explain verbosity is also provided

I'm OK relying on the server to raise an error here if you'd like to remove this.

I'd argue this is fundamentally different from intentionally omitting writeConcern (PHPLIB-802) because that option is easily inherited from a parent context.

If a user encounters the error you highlighted above, it's because they've explicitly crafted an Aggregate operation with an explain option, and then decided to pass that to Collection::explain().

GromNaN · 2023-06-16T13:30:24Z

tests/Operation/FindTest.php

+            'filter' => (object) ['x' => 1],
+            'allowDiskUse' => true,
+            'allowPartialResults' => true,
+            'batchSize' => 123,


The batchSize seems to be useless for an explain. It is not supported by the aggregate command with explain option:

mongo-php-library/src/Operation/Aggregate.php

Lines 225 to 229 in 78cc925

// Explain does not use a cursor

if ($this->isExplain) {

$options['useCursor'] = false;

unset($options['batchSize']);

}

The batchSize seems to be useless for an explain

I imagine so, but I don't think there's any reason for PHPLIB to care about that. We can rely on the server to ignore it or raise an error. I wouldn't expect an error (not going to bother testing myself), but if so the user could always revise their operation options.

The case for unsetting batchSize when using the explain option with aggregate is different. That logic has nothing to do with it being relevant/supported when explaining a pipeline, but rather the fact that aggregate with explain: true returns a single document. The batchSize option is only relevant if aggregate yields a cursor.

jmikola

LGTM with at least the Psalm error resolved.

I'll defer to you on the other suggested changes.

jmikola · 2023-06-17T00:35:14Z

tests/Operation/AggregateTest.php

+            'readConcern' => new ReadConcern(ReadConcern::LOCAL),
+            'useCursor' => true,
+            // Intentionally omitted options
+            'explain' => true,


As mentioned in #1105 (comment), I think it makes sense to include this and allow the server to raise an error.

If you agree, then I'd suggesting moving this above // Intentionally omitted options and under a comment noting that the explain that is rejected but we don't intentionally exclude it.

If you don't agree, then OK to keep this as-is.

jmikola · 2023-06-17T00:36:40Z

src/Operation/Delete.php

+        $cmd = ['delete' => $this->collectionName, 'deletes' => [['q' => $this->filter] + $this->createDeleteOptions()]];
+
+        if (isset($this->options['comment'])) {
+            $cmd['comment'] = $this->options['comment'];


Note Psalm's MixedAssignment error. You may need an annotation to note that comment is actually mixed (any BSON type is supported).

Alternatively, you could just add this to the baseline file. We already have a number suppressions for MixedAssignment on $cmd['comment'].

jmikola · 2023-06-17T01:00:32Z

src/Operation/Find.php

@@ -353,6 +338,11 @@ private function createCommandDocument(): array
            return $cmd;
        }

+        // Read concern can change the query plan
+        if (isset($this->options['readConcern'])) {


I think this conditional can be removed.

createQueryOptions() actually includes the readConcern option, so it will already be added by the final $cmd + $options union in the return statement. This is because the readConcern option is passed to the Query constructor instead of Server::executeQuery() (as it might be for executeReadCommand() in other operations).

Hypothetically, if that wasn't the case, the position of this conditional would still be problematic due to the early return above if createQueryOptions() returns an empty array. The edge case would be constructing Find with only a readConcern option.

While writing this, I realized the docs regarding read concern inheritance for queries is not very clear. Opened PHPC-2249 to improve that.

Remove private function createCommandDocument introduced by c7b2b03 and used only for the explain command.

Separate Intentionally omitted option

The 'explain' option is illegal when a explain verbosity is also provided

GromNaN · 2023-06-19T08:11:22Z

CS and Psalm checks OK. The failing tests are unrelated, due to mongodb/mongo-php-driver@358fd2f

alcaeus · 2023-06-19T08:11:56Z

Test failures are unrelated.

GromNaN changed the title ~~PHPLIB-1144: Add unit tests on Explainable::getCommandDocument~~ PHPLIB-1144: Add unit tests on Explainable::getCommandDocument() Jun 14, 2023

GromNaN requested a review from jmikola June 14, 2023 13:26

GromNaN marked this pull request as ready for review June 14, 2023 13:26

GromNaN force-pushed the PHPLIB-1144 branch from bde18c2 to 3d7d2eb Compare June 14, 2023 13:27

GromNaN commented Jun 14, 2023

View reviewed changes

tests/Operation/FindAndModifyTest.php Show resolved Hide resolved

GromNaN commented Jun 14, 2023

View reviewed changes

GromNaN force-pushed the PHPLIB-1144 branch 2 times, most recently from 7d9ef90 to 1714264 Compare June 15, 2023 12:58

jmikola reviewed Jun 15, 2023

View reviewed changes

GromNaN force-pushed the PHPLIB-1144 branch 2 times, most recently from 9960219 to e370122 Compare June 16, 2023 13:28

GromNaN commented Jun 16, 2023

View reviewed changes

jmikola approved these changes Jun 17, 2023

View reviewed changes

GromNaN added 6 commits June 19, 2023 09:43

PHPLIB-1144: Add unit tests on Explainable::getCommandDocument

d92dd0b

Add tests on FindOneAnd*::getCommandDocument

668becb

Refactor Find::getCommandDocument

cf740db

Remove private function createCommandDocument introduced by c7b2b03 and used only for the explain command.

Add let and comment to explain delete

aedff6c

Separate Intentionally omitted option

Aggregate option explain is incompatible with the explain command

fac470b

The 'explain' option is illegal when a explain verbosity is also provided

Let the server throw errors

8513129

GromNaN force-pushed the PHPLIB-1144 branch from e370122 to 8513129 Compare June 19, 2023 07:43

alcaeus merged commit 53052b6 into mongodb:master Jun 19, 2023

GromNaN deleted the PHPLIB-1144 branch June 19, 2023 08:14

	// Explain does not use a cursor
	if ($this->isExplain) {
	$options['useCursor'] = false;
	unset($options['batchSize']);
	}

PHPLIB-1144: Add unit tests on Explainable::getCommandDocument() #1105

PHPLIB-1144: Add unit tests on Explainable::getCommandDocument() #1105

Uh oh!

Conversation

GromNaN commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

jmikola Jun 16, 2023 • 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!

jmikola left a comment

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!

GromNaN commented Jun 19, 2023

Uh oh!

alcaeus commented Jun 19, 2023

Uh oh!

Uh oh!

PHPLIB-1144: Add unit tests on `Explainable::getCommandDocument()` #1105

PHPLIB-1144: Add unit tests on `Explainable::getCommandDocument()` #1105

GromNaN commented Jun 14, 2023 •

edited

Loading

jmikola Jun 16, 2023 •

edited

Loading