From 142411c866bbc6cdef084256c7e78689ad0c7b3d Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 8 Jun 2023 17:48:55 -0400 Subject: [PATCH 1/3] PHPLIB-1129: Improve pipeline checks for replace operations Consolidates data providers in base TestCase and FunctionalTestCase classes. Change is_pipeline() to conditionally consider empty arrays as pipelines. This was needed for improvements to replace/update parameter validation, and will prove useful for consolidated pipeline validation in PHPLIB-881. Prohibit update documents and pipelines (both empty and non-empty) for replacement operations. Prohibit replacement documents and empty pipeines for update operations. Empty pipelines must be prohibited because libmongoc does not support them for update commands (they're indistinguishable from empty replacement documents). For consistency, we also prohibit them for FindOneAndUpdate even though PHPLIB could work around that. Prohibit update pipelines for replace operations (both empty and non-empty). Previously ReplaceOne only prohibited non-empty pipelines and BulkWrite replaceOne and FindOneAndReplace has no validation. A notable exception is empty arrays, which are treated as replacement documents for BC. Add tests for valid update/replacement arguments for BulkWrite replaceOne, updateMany, and updateOne operations. Also adds more comprehensive tests for valid $update arguments for UpdateMany and UpdateOne operations (update documents and non-empty pipelines). --- psalm-baseline.xml | 14 ++- src/Operation/BulkWrite.php | 11 ++- src/Operation/FindOneAndReplace.php | 12 ++- src/Operation/FindOneAndUpdate.php | 2 +- src/Operation/ReplaceOne.php | 11 ++- src/Operation/Update.php | 2 +- src/Operation/UpdateMany.php | 2 +- src/Operation/UpdateOne.php | 2 +- src/functions.php | 23 ++++- tests/Operation/BulkWriteFunctionalTest.php | 49 ---------- tests/Operation/BulkWriteTest.php | 98 +++++++++++-------- .../CountDocumentsFunctionalTest.php | 14 --- tests/Operation/CountFunctionalTest.php | 14 --- tests/Operation/DeleteFunctionalTest.php | 14 --- tests/Operation/DistinctFunctionalTest.php | 14 --- .../Operation/FindAndModifyFunctionalTest.php | 37 ------- tests/Operation/FindFunctionalTest.php | 12 --- tests/Operation/FindOneAndReplaceTest.php | 41 ++++---- tests/Operation/FindOneAndUpdateTest.php | 26 ++--- tests/Operation/FunctionalTestCase.php | 62 ++++++++++++ tests/Operation/ReplaceOneTest.php | 41 +++----- tests/Operation/TestCase.php | 65 ++++++++++++ tests/Operation/UpdateFunctionalTest.php | 51 ---------- tests/Operation/UpdateManyTest.php | 36 ++----- tests/Operation/UpdateOneTest.php | 36 ++----- tests/Operation/UpdateTest.php | 26 ++--- 26 files changed, 305 insertions(+), 410 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index be0d2150e..ead3a341c 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -36,8 +36,9 @@ - + new static(sprintf('Expected %s to have type "%s" but found "%s"', $name, $expectedType, get_debug_type($value))) + new static('Expected update operator(s) or non-empty pipeline for ' . $name) @@ -308,7 +309,7 @@ is_array($operation) - + $args $args $args @@ -320,9 +321,10 @@ $args[1] $args[1] $args[1] + $args[1] $args[2] - + $args[0] $args[0] $args[0] @@ -337,6 +339,9 @@ $args[1] $args[1] $args[1] + $args[1] + $args[1] + $args[1] $args[2] $args[2] $args[2] @@ -349,7 +354,8 @@ $args[2]['upsert'] $args[2]['upsert'] - + + $args[1] $args[1] $args[1] $args[2] diff --git a/src/Operation/BulkWrite.php b/src/Operation/BulkWrite.php index 2296007dd..b68daed4d 100644 --- a/src/Operation/BulkWrite.php +++ b/src/Operation/BulkWrite.php @@ -191,10 +191,19 @@ public function __construct(string $databaseName, string $collectionName, array throw InvalidArgumentException::invalidType(sprintf('$operations[%d]["%s"][1]', $i, $type), $args[1], 'array or object'); } + // Treat empty arrays as replacement documents for BC + if ($args[1] === []) { + $args[1] = (object) $args[1]; + } + if (is_first_key_operator($args[1])) { throw new InvalidArgumentException(sprintf('First key in $operations[%d]["%s"][1] is an update operator', $i, $type)); } + if (is_pipeline($args[1], true /* allowEmpty */)) { + throw new InvalidArgumentException(sprintf('$operations[%d]["%s"][1] is an update pipeline', $i, $type)); + } + if (! isset($args[2])) { $args[2] = []; } @@ -229,7 +238,7 @@ public function __construct(string $databaseName, string $collectionName, array } if (! is_first_key_operator($args[1]) && ! is_pipeline($args[1])) { - throw new InvalidArgumentException(sprintf('First key in $operations[%d]["%s"][1] is neither an update operator nor a pipeline', $i, $type)); + throw new InvalidArgumentException(sprintf('Expected update operator(s) or non-empty pipeline for $operations[%d]["%s"][1]', $i, $type)); } if (! isset($args[2])) { diff --git a/src/Operation/FindOneAndReplace.php b/src/Operation/FindOneAndReplace.php index 46b0bd5d3..4132985cd 100644 --- a/src/Operation/FindOneAndReplace.php +++ b/src/Operation/FindOneAndReplace.php @@ -27,6 +27,7 @@ use function is_integer; use function is_object; use function MongoDB\is_first_key_operator; +use function MongoDB\is_pipeline; /** * Operation for replacing a document with the findAndModify command. @@ -109,8 +110,17 @@ public function __construct(string $databaseName, string $collectionName, $filte throw InvalidArgumentException::invalidType('$replacement', $replacement, 'array or object'); } + // Treat empty arrays as replacement documents for BC + if ($replacement === []) { + $replacement = (object) $replacement; + } + if (is_first_key_operator($replacement)) { - throw new InvalidArgumentException('First key in $replacement argument is an update operator'); + throw new InvalidArgumentException('First key in $replacement is an update operator'); + } + + if (is_pipeline($replacement, true /* allowEmpty */)) { + throw new InvalidArgumentException('$replacement is an update pipeline'); } if (isset($options['projection']) && ! is_array($options['projection']) && ! is_object($options['projection'])) { diff --git a/src/Operation/FindOneAndUpdate.php b/src/Operation/FindOneAndUpdate.php index adc0ae24c..88bea8bd9 100644 --- a/src/Operation/FindOneAndUpdate.php +++ b/src/Operation/FindOneAndUpdate.php @@ -114,7 +114,7 @@ public function __construct(string $databaseName, string $collectionName, $filte } if (! is_first_key_operator($update) && ! is_pipeline($update)) { - throw new InvalidArgumentException('Expected an update document with operator as first key or a pipeline'); + throw new InvalidArgumentException('Expected update operator(s) or non-empty pipeline for $update'); } if (isset($options['projection']) && ! is_array($options['projection']) && ! is_object($options['projection'])) { diff --git a/src/Operation/ReplaceOne.php b/src/Operation/ReplaceOne.php index ce794f9c7..a2b641aa8 100644 --- a/src/Operation/ReplaceOne.php +++ b/src/Operation/ReplaceOne.php @@ -85,12 +85,17 @@ public function __construct(string $databaseName, string $collectionName, $filte throw InvalidArgumentException::invalidType('$replacement', $replacement, 'array or object'); } + // Treat empty arrays as replacement documents for BC + if ($replacement === []) { + $replacement = (object) $replacement; + } + if (is_first_key_operator($replacement)) { - throw new InvalidArgumentException('First key in $replacement argument is an update operator'); + throw new InvalidArgumentException('First key in $replacement is an update operator'); } - if (is_pipeline($replacement)) { - throw new InvalidArgumentException('$replacement argument is a pipeline'); + if (is_pipeline($replacement, true /* allowEmpty */)) { + throw new InvalidArgumentException('$replacement is an update pipeline'); } $this->update = new Update( diff --git a/src/Operation/Update.php b/src/Operation/Update.php index 611d581d2..a6bd8f441 100644 --- a/src/Operation/Update.php +++ b/src/Operation/Update.php @@ -148,7 +148,7 @@ public function __construct(string $databaseName, string $collectionName, $filte } if ($options['multi'] && ! is_first_key_operator($update) && ! is_pipeline($update)) { - throw new InvalidArgumentException('"multi" option cannot be true if $update is a replacement document'); + throw new InvalidArgumentException('"multi" option cannot be true unless $update has update operator(s) or non-empty pipeline'); } if (isset($options['session']) && ! $options['session'] instanceof Session) { diff --git a/src/Operation/UpdateMany.php b/src/Operation/UpdateMany.php index b751c1c51..a7002baf5 100644 --- a/src/Operation/UpdateMany.php +++ b/src/Operation/UpdateMany.php @@ -89,7 +89,7 @@ public function __construct(string $databaseName, string $collectionName, $filte } if (! is_first_key_operator($update) && ! is_pipeline($update)) { - throw new InvalidArgumentException('Expected an update document with operator as first key or a pipeline'); + throw new InvalidArgumentException('Expected update operator(s) or non-empty pipeline for $update'); } $this->update = new Update( diff --git a/src/Operation/UpdateOne.php b/src/Operation/UpdateOne.php index ab57defbc..63436c0e7 100644 --- a/src/Operation/UpdateOne.php +++ b/src/Operation/UpdateOne.php @@ -89,7 +89,7 @@ public function __construct(string $databaseName, string $collectionName, $filte } if (! is_first_key_operator($update) && ! is_pipeline($update)) { - throw new InvalidArgumentException('Expected an update document with operator as first key or a pipeline'); + throw new InvalidArgumentException('Expected update operator(s) or non-empty pipeline for $update'); } $this->update = new Update( diff --git a/src/functions.php b/src/functions.php index b436d79d1..95f9cc9a1 100644 --- a/src/functions.php +++ b/src/functions.php @@ -213,7 +213,22 @@ function is_first_key_operator($document): bool } /** - * Returns whether an update specification is a valid aggregation pipeline. + * Returns whether the argument is a valid aggregation or update pipeline. + * + * This is primarily used for validating arguments for update and replace + * operations, but can also be used for validating an aggregation pipeline. + * + * The $allowEmpty parameter can be used to control whether an empty array + * should be considered a valid pipeline. Empty arrays are generally valid for + * an aggregation pipeline, but the things are more complicated for update + * pipelines. + * + * Update operations must prohibit empty pipelines, since libmongoc may encode + * an empty pipeline array as an empty replacement document when writing an + * update command (arrays and documents have the same bson_t representation). + * For consistency, findOneAndUpdate should also prohibit empty pipelines. + * Replace operations (e.g. replaceOne, findOneAndReplace) should reject empty + * and non-empty pipelines alike, since neither is a replacement document. * * Note: this method may propagate an InvalidArgumentException from * document_or_array() if a Serializable object within the pipeline array @@ -223,11 +238,11 @@ function is_first_key_operator($document): bool * @param array|object $pipeline * @throws InvalidArgumentException */ -function is_pipeline($pipeline): bool +function is_pipeline($pipeline, bool $allowEmpty = false): bool { if ($pipeline instanceof PackedArray) { /* Nested documents and arrays are intentionally left as BSON. We avoid - * iterator_to_array() since Document iteration returns all values as + * iterator_to_array() since PackedArray iteration returns all values as * MongoDB\BSON\Value instances. */ /** @psalm-var array */ $pipeline = $pipeline->toPHP([ @@ -244,7 +259,7 @@ function is_pipeline($pipeline): bool } if ($pipeline === []) { - return false; + return $allowEmpty; } $expectedKey = 0; diff --git a/tests/Operation/BulkWriteFunctionalTest.php b/tests/Operation/BulkWriteFunctionalTest.php index 9306427dd..0dbb20e67 100644 --- a/tests/Operation/BulkWriteFunctionalTest.php +++ b/tests/Operation/BulkWriteFunctionalTest.php @@ -4,13 +4,11 @@ use MongoDB\BSON\Document; use MongoDB\BSON\ObjectId; -use MongoDB\BSON\PackedArray; use MongoDB\BulkWriteResult; use MongoDB\Collection; use MongoDB\Driver\BulkWrite as Bulk; use MongoDB\Driver\WriteConcern; use MongoDB\Exception\BadMethodCallException; -use MongoDB\Model\BSONArray; use MongoDB\Model\BSONDocument; use MongoDB\Operation\BulkWrite; use MongoDB\Tests\CommandObserver; @@ -177,18 +175,6 @@ function (array $event) use ($expectedFilter): void { ); } - public function provideFilterDocuments(): array - { - $expectedQuery = (object) ['x' => 1]; - - return [ - 'array' => [['x' => 1], $expectedQuery], - 'object' => [(object) ['x' => 1], $expectedQuery], - 'Serializable' => [new BSONDocument(['x' => 1]), $expectedQuery], - 'Document' => [Document::fromPHP(['x' => 1]), $expectedQuery], - ]; - } - /** @dataProvider provideReplacementDocuments */ public function testReplacementDocuments($replacement, stdClass $expectedReplacement): void { @@ -208,18 +194,6 @@ function (array $event) use ($expectedReplacement): void { ); } - public function provideReplacementDocuments(): array - { - $expected = (object) ['x' => 1]; - - return [ - 'replacement:array' => [['x' => 1], $expected], - 'replacement:object' => [(object) ['x' => 1], $expected], - 'replacement:Serializable' => [new BSONDocument(['x' => 1]), $expected], - 'replacement:Document' => [Document::fromPHP(['x' => 1]), $expected], - ]; - } - /** * @dataProvider provideUpdateDocuments * @dataProvider provideUpdatePipelines @@ -250,29 +224,6 @@ function (array $event) use ($expectedUpdate): void { ); } - public function provideUpdateDocuments(): array - { - $expected = (object) ['$set' => (object) ['x' => 1]]; - - return [ - 'update:array' => [['$set' => ['x' => 1]], $expected], - 'update:object' => [(object) ['$set' => ['x' => 1]], $expected], - 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]]), $expected], - 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]]), $expected], - ]; - } - - public function provideUpdatePipelines(): array - { - $expected = [(object) ['$set' => (object) ['x' => 1]]]; - - return [ - 'pipeline:array' => [[['$set' => ['x' => 1]]], $expected], - 'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]]), $expected], - 'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]]), $expected], - ]; - } - public function testDeletes(): void { $this->createFixtures(4); diff --git a/tests/Operation/BulkWriteTest.php b/tests/Operation/BulkWriteTest.php index 31b90d86b..ac21e558e 100644 --- a/tests/Operation/BulkWriteTest.php +++ b/tests/Operation/BulkWriteTest.php @@ -2,11 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -use MongoDB\BSON\PackedArray; use MongoDB\Exception\InvalidArgumentException; -use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\BulkWrite; class BulkWriteTest extends TestCase @@ -149,6 +145,17 @@ public function testReplaceOneFilterArgumentTypeCheck($filter): void ]); } + /** + * @dataProvider provideReplacementDocuments + * @doesNotPerformAssertions + */ + public function testReplaceOneReplacementArgument($replacement): void + { + new BulkWrite($this->getDatabaseName(), $this->getCollectionName(), [ + [BulkWrite::REPLACE_ONE => [['x' => 1], $replacement]], + ]); + } + public function testReplaceOneReplacementArgumentMissing(): void { $this->expectException(InvalidArgumentException::class); @@ -168,8 +175,8 @@ public function testReplaceOneReplacementArgumentTypeCheck($replacement): void ]); } - /** @dataProvider provideInvalidReplacementValues */ - public function testReplaceOneReplacementArgumentRequiresNoOperators($replacement): void + /** @dataProvider provideUpdateDocuments */ + public function testReplaceOneReplacementArgumentProhibitsUpdateDocument($replacement): void { $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage('First key in $operations[0]["replaceOne"][1] is an update operator'); @@ -178,21 +185,17 @@ public function testReplaceOneReplacementArgumentRequiresNoOperators($replacemen ]); } - public function provideInvalidReplacementValues(): array + /** + * @dataProvider provideUpdatePipelines + * @dataProvider provideEmptyUpdatePipelinesExcludingArray + */ + public function testReplaceOneReplacementArgumentProhibitsUpdatePipeline($replacement): void { - return [ - 'update:array' => [['$set' => ['x' => 1]]], - 'update:object' => [(object) ['$set' => ['x' => 1]]], - 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]])], - 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]])], - // TODO: Enable the following tests when implementing PHPLIB-1129 - //'pipeline:array' => [[['$set' => ['x' => 1]]]], - //'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]])], - //'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]])], - //'empty_pipeline:array' => [[]], - //'empty_pipeline:Serializable' => [new BSONArray([])], - //'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('$operations[0]["replaceOne"][1] is an update pipeline'); + new BulkWrite($this->getDatabaseName(), $this->getCollectionName(), [ + [BulkWrite::REPLACE_ONE => [['x' => 1], $replacement]], + ]); } /** @dataProvider provideInvalidDocumentValues */ @@ -239,6 +242,18 @@ public function testUpdateManyFilterArgumentTypeCheck($filter): void ]); } + /** + * @dataProvider provideUpdateDocuments + * @dataProvider provideUpdatePipelines + * @doesNotPerformAssertions + */ + public function testUpdateManyUpdateArgument($update): void + { + new BulkWrite($this->getDatabaseName(), $this->getCollectionName(), [ + [BulkWrite::UPDATE_MANY => [['x' => 1], $update]], + ]); + } + public function testUpdateManyUpdateArgumentMissing(): void { $this->expectException(InvalidArgumentException::class); @@ -258,29 +273,19 @@ public function testUpdateManyUpdateArgumentTypeCheck($update): void ]); } - /** @dataProvider provideInvalidUpdateValues */ - public function testUpdateManyUpdateArgumentRequiresOperatorsOrPipeline($update): void + /** + * @dataProvider provideReplacementDocuments + * @dataProvider provideEmptyUpdatePipelines + */ + public function testUpdateManyUpdateArgumentProhibitsReplacementDocumentOrEmptyPipeline($update): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('First key in $operations[0]["updateMany"][1] is neither an update operator nor a pipeline'); + $this->expectExceptionMessage('Expected update operator(s) or non-empty pipeline for $operations[0]["updateMany"][1]'); new BulkWrite($this->getDatabaseName(), $this->getCollectionName(), [ [BulkWrite::UPDATE_MANY => [['x' => 1], $update]], ]); } - public function provideInvalidUpdateValues(): array - { - return [ - 'replacement:array' => [['x' => 1]], - 'replacement:object' => [(object) ['x' => 1]], - 'replacement:Serializable' => [new BSONDocument(['x' => 1])], - 'replacement:Document' => [Document::fromPHP(['x' => 1])], - 'empty_pipeline:array' => [[]], - 'empty_pipeline:Serializable' => [new BSONArray([])], - 'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; - } - /** @dataProvider provideInvalidArrayValues */ public function testUpdateManyArrayFiltersOptionTypeCheck($arrayFilters): void { @@ -311,6 +316,18 @@ public function testUpdateManyUpsertOptionTypeCheck($upsert): void ]); } + /** + * @dataProvider provideUpdateDocuments + * @dataProvider provideUpdatePipelines + * @doesNotPerformAssertions + */ + public function testUpdateOneUpdateArgument($update): void + { + new BulkWrite($this->getDatabaseName(), $this->getCollectionName(), [ + [BulkWrite::UPDATE_ONE => [['x' => 1], $update]], + ]); + } + public function testUpdateOneFilterArgumentMissing(): void { $this->expectException(InvalidArgumentException::class); @@ -349,11 +366,14 @@ public function testUpdateOneUpdateArgumentTypeCheck($update): void ]); } - /** @dataProvider provideInvalidUpdateValues */ - public function testUpdateOneUpdateArgumentRequiresOperatorsOrPipeline($update): void + /** + * @dataProvider provideReplacementDocuments + * @dataProvider provideEmptyUpdatePipelines + */ + public function testUpdateOneUpdateArgumentProhibitsReplacementDocumentOrEmptyPipeline($update): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('First key in $operations[0]["updateOne"][1] is neither an update operator nor a pipeline'); + $this->expectExceptionMessage('Expected update operator(s) or non-empty pipeline for $operations[0]["updateOne"][1]'); new BulkWrite($this->getDatabaseName(), $this->getCollectionName(), [ [BulkWrite::UPDATE_ONE => [['x' => 1], $update]], ]); diff --git a/tests/Operation/CountDocumentsFunctionalTest.php b/tests/Operation/CountDocumentsFunctionalTest.php index 21415428d..3b3aa7cc1 100644 --- a/tests/Operation/CountDocumentsFunctionalTest.php +++ b/tests/Operation/CountDocumentsFunctionalTest.php @@ -2,8 +2,6 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\CountDocuments; use MongoDB\Operation\InsertMany; use MongoDB\Tests\CommandObserver; @@ -30,18 +28,6 @@ function (array $event) use ($expectedMatchStage): void { ); } - public function provideFilterDocuments(): array - { - $expectedQuery = (object) ['x' => 1]; - - return [ - 'array' => [['x' => 1], $expectedQuery], - 'object' => [(object) ['x' => 1], $expectedQuery], - 'Serializable' => [new BSONDocument(['x' => 1]), $expectedQuery], - 'Document' => [Document::fromPHP(['x' => 1]), $expectedQuery], - ]; - } - public function testEmptyCollection(): void { $operation = new CountDocuments($this->getDatabaseName(), $this->getCollectionName(), []); diff --git a/tests/Operation/CountFunctionalTest.php b/tests/Operation/CountFunctionalTest.php index 3f44db2b7..192dd5434 100644 --- a/tests/Operation/CountFunctionalTest.php +++ b/tests/Operation/CountFunctionalTest.php @@ -2,8 +2,6 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\Count; use MongoDB\Operation\CreateIndexes; use MongoDB\Operation\InsertMany; @@ -31,18 +29,6 @@ function (array $event) use ($expectedQuery): void { ); } - public function provideFilterDocuments(): array - { - $expectedQuery = (object) ['x' => 1]; - - return [ - 'array' => [['x' => 1], $expectedQuery], - 'object' => [(object) ['x' => 1], $expectedQuery], - 'Serializable' => [new BSONDocument(['x' => 1]), $expectedQuery], - 'Document' => [Document::fromPHP(['x' => 1]), $expectedQuery], - ]; - } - public function testDefaultReadConcernIsOmitted(): void { (new CommandObserver())->observe( diff --git a/tests/Operation/DeleteFunctionalTest.php b/tests/Operation/DeleteFunctionalTest.php index 01690b090..692441ac6 100644 --- a/tests/Operation/DeleteFunctionalTest.php +++ b/tests/Operation/DeleteFunctionalTest.php @@ -2,14 +2,12 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; use MongoDB\Collection; use MongoDB\DeleteResult; use MongoDB\Driver\BulkWrite; use MongoDB\Driver\WriteConcern; use MongoDB\Exception\BadMethodCallException; use MongoDB\Exception\UnsupportedException; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\Delete; use MongoDB\Tests\CommandObserver; use stdClass; @@ -48,18 +46,6 @@ function (array $event) use ($expectedQuery): void { ); } - public function provideFilterDocuments(): array - { - $expectedQuery = (object) ['x' => 1]; - - return [ - 'array' => [['x' => 1], $expectedQuery], - 'object' => [(object) ['x' => 1], $expectedQuery], - 'Serializable' => [new BSONDocument(['x' => 1]), $expectedQuery], - 'Document' => [Document::fromPHP(['x' => 1]), $expectedQuery], - ]; - } - public function testDeleteOne(): void { $this->createFixtures(3); diff --git a/tests/Operation/DistinctFunctionalTest.php b/tests/Operation/DistinctFunctionalTest.php index 8e97f48d9..53e411a0f 100644 --- a/tests/Operation/DistinctFunctionalTest.php +++ b/tests/Operation/DistinctFunctionalTest.php @@ -2,9 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; use MongoDB\Driver\BulkWrite; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\Distinct; use MongoDB\Tests\CommandObserver; use stdClass; @@ -35,18 +33,6 @@ function (array $event) use ($expectedQuery): void { ); } - public function provideFilterDocuments(): array - { - $expectedQuery = (object) ['x' => 1]; - - return [ - 'array' => [['x' => 1], $expectedQuery], - 'object' => [(object) ['x' => 1], $expectedQuery], - 'Serializable' => [new BSONDocument(['x' => 1]), $expectedQuery], - 'Document' => [Document::fromPHP(['x' => 1]), $expectedQuery], - ]; - } - public function testDefaultReadConcernIsOmitted(): void { (new CommandObserver())->observe( diff --git a/tests/Operation/FindAndModifyFunctionalTest.php b/tests/Operation/FindAndModifyFunctionalTest.php index 86d2747f9..8b96c6606 100644 --- a/tests/Operation/FindAndModifyFunctionalTest.php +++ b/tests/Operation/FindAndModifyFunctionalTest.php @@ -3,12 +3,10 @@ namespace MongoDB\Tests\Operation; use MongoDB\BSON\Document; -use MongoDB\BSON\PackedArray; use MongoDB\Driver\BulkWrite; use MongoDB\Driver\Exception\CommandException; use MongoDB\Driver\WriteConcern; use MongoDB\Exception\UnsupportedException; -use MongoDB\Model\BSONArray; use MongoDB\Model\BSONDocument; use MongoDB\Operation\FindAndModify; use MongoDB\Tests\CommandObserver; @@ -75,41 +73,6 @@ function (array $event) use ($expectedUpdate): void { ); } - public function provideReplacementDocuments(): array - { - $expected = (object) ['x' => 1]; - - return [ - 'replacement:array' => [['x' => 1], $expected], - 'replacement:object' => [(object) ['x' => 1], $expected], - 'replacement:Serializable' => [new BSONDocument(['x' => 1]), $expected], - 'replacement:Document' => [Document::fromPHP(['x' => 1]), $expected], - ]; - } - - public function provideUpdateDocuments(): array - { - $expected = (object) ['$set' => (object) ['x' => 1]]; - - return [ - 'update:array' => [['$set' => ['x' => 1]], $expected], - 'update:object' => [(object) ['$set' => ['x' => 1]], $expected], - 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]]), $expected], - 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]]), $expected], - ]; - } - - public function provideUpdatePipelines(): array - { - $expected = [(object) ['$set' => (object) ['x' => 1]]]; - - return [ - 'pipeline:array' => [[['$set' => ['x' => 1]]], $expected], - 'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]]), $expected], - 'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]]), $expected], - ]; - } - /** @see https://jira.mongodb.org/browse/PHPLIB-344 */ public function testManagerReadConcernIsOmitted(): void { diff --git a/tests/Operation/FindFunctionalTest.php b/tests/Operation/FindFunctionalTest.php index a4238e0e5..a7975f338 100644 --- a/tests/Operation/FindFunctionalTest.php +++ b/tests/Operation/FindFunctionalTest.php @@ -34,18 +34,6 @@ function (array $event) use ($expectedQuery): void { ); } - public function provideFilterDocuments(): array - { - $expectedQuery = (object) ['x' => 1]; - - return [ - 'array' => [['x' => 1], $expectedQuery], - 'object' => [(object) ['x' => 1], $expectedQuery], - 'Serializable' => [new BSONDocument(['x' => 1]), $expectedQuery], - 'Document' => [Document::fromPHP(['x' => 1]), $expectedQuery], - ]; - } - /** @dataProvider provideModifierDocuments */ public function testModifierDocuments($modifiers, stdClass $expectedSort): void { diff --git a/tests/Operation/FindOneAndReplaceTest.php b/tests/Operation/FindOneAndReplaceTest.php index c50777055..b084036f4 100644 --- a/tests/Operation/FindOneAndReplaceTest.php +++ b/tests/Operation/FindOneAndReplaceTest.php @@ -2,11 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -//use MongoDB\BSON\PackedArray; use MongoDB\Exception\InvalidArgumentException; -//use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\FindOneAndReplace; class FindOneAndReplaceTest extends TestCase @@ -25,29 +21,32 @@ public function testConstructorReplacementArgumentTypeCheck($replacement): void new FindOneAndReplace($this->getDatabaseName(), $this->getCollectionName(), [], $replacement); } - /** @dataProvider provideInvalidReplacementValues */ - public function testConstructorReplacementArgumentRequiresNoOperators($replacement): void + /** + * @dataProvider provideReplacementDocuments + * @doesNotPerformAssertions + */ + public function testConstructorReplacementArgument($replacement): void + { + new FindOneAndReplace($this->getDatabaseName(), $this->getCollectionName(), [], $replacement); + } + + /** @dataProvider provideUpdateDocuments */ + public function testConstructorReplacementArgumentProhibitsUpdateDocument($replacement): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('First key in $replacement argument is an update operator'); + $this->expectExceptionMessage('First key in $replacement is an update operator'); new FindOneAndReplace($this->getDatabaseName(), $this->getCollectionName(), [], $replacement); } - public function provideInvalidReplacementValues(): array + /** + * @dataProvider provideUpdatePipelines + * @dataProvider provideEmptyUpdatePipelinesExcludingArray + */ + public function testConstructorReplacementArgumentProhibitsUpdatePipeline($replacement): void { - return [ - 'update:array' => [['$set' => ['x' => 1]]], - 'update:object' => [(object) ['$set' => ['x' => 1]]], - 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]])], - 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]])], - // TODO: Enable the following tests when implementing PHPLIB-1129 - //'pipeline:array' => [[['$set' => ['x' => 1]]]], - //'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]])], - //'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]])], - //'empty_pipeline:array' => [[]], - //'empty_pipeline:Serializable' => [new BSONArray([])], - //'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('$replacement is an update pipeline'); + new FindOneAndReplace($this->getDatabaseName(), $this->getCollectionName(), [], $replacement); } /** @dataProvider provideInvalidConstructorOptions */ diff --git a/tests/Operation/FindOneAndUpdateTest.php b/tests/Operation/FindOneAndUpdateTest.php index 2b57b7f5b..8514cea86 100644 --- a/tests/Operation/FindOneAndUpdateTest.php +++ b/tests/Operation/FindOneAndUpdateTest.php @@ -2,11 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -use MongoDB\BSON\PackedArray; use MongoDB\Exception\InvalidArgumentException; -use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\FindOneAndUpdate; class FindOneAndUpdateTest extends TestCase @@ -25,27 +21,17 @@ public function testConstructorUpdateArgumentTypeCheck($update): void new FindOneAndUpdate($this->getDatabaseName(), $this->getCollectionName(), [], $update); } - /** @dataProvider provideInvalidUpdateValues */ - public function testConstructorUpdateArgumentRequiresOperatorsOrPipeline($update): void + /** + * @dataProvider provideReplacementDocuments + * @dataProvider provideEmptyUpdatePipelines + */ + public function testConstructorUpdateArgumentProhibitsReplacementDocumentOrEmptyPipeline($update): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Expected an update document with operator as first key or a pipeline'); + $this->expectExceptionMessage('Expected update operator(s) or non-empty pipeline for $update'); new FindOneAndUpdate($this->getDatabaseName(), $this->getCollectionName(), [], $update); } - public function provideInvalidUpdateValues(): array - { - return [ - 'replacement:array' => [['x' => 1]], - 'replacement:object' => [(object) ['x' => 1]], - 'replacement:Serializable' => [new BSONDocument(['x' => 1])], - 'replacement:Document' => [Document::fromPHP(['x' => 1])], - 'empty_pipeline:array' => [[]], - 'empty_pipeline:Serializable' => [new BSONArray([])], - 'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; - } - /** @dataProvider provideInvalidConstructorOptions */ public function testConstructorOptionTypeChecks(array $options): void { diff --git a/tests/Operation/FunctionalTestCase.php b/tests/Operation/FunctionalTestCase.php index 3da36fd71..c07a35712 100644 --- a/tests/Operation/FunctionalTestCase.php +++ b/tests/Operation/FunctionalTestCase.php @@ -2,8 +2,12 @@ namespace MongoDB\Tests\Operation; +use MongoDB\BSON\Document; +use MongoDB\BSON\PackedArray; use MongoDB\Driver\ReadConcern; use MongoDB\Driver\WriteConcern; +use MongoDB\Model\BSONArray; +use MongoDB\Model\BSONDocument; use MongoDB\Tests\FunctionalTestCase as BaseFunctionalTestCase; /** @@ -18,6 +22,64 @@ public function setUp(): void $this->dropCollection($this->getDatabaseName(), $this->getCollectionName()); } + public function provideFilterDocuments(): array + { + $expected = (object) ['x' => 1]; + + return [ + 'filter:array' => [['x' => 1], $expected], + 'filter:object' => [(object) ['x' => 1], $expected], + 'filter:Serializable' => [new BSONDocument(['x' => 1]), $expected], + 'filter:Document' => [Document::fromPHP(['x' => 1]), $expected], + ]; + } + + public function provideReplacementDocuments(): array + { + $expected = (object) ['x' => 1]; + $expectedEmpty = (object) []; + + return [ + 'replacement:array' => [['x' => 1], $expected], + 'replacement:object' => [(object) ['x' => 1], $expected], + 'replacement:Serializable' => [new BSONDocument(['x' => 1]), $expected], + 'replacement:Document' => [Document::fromPHP(['x' => 1]), $expected], + /* Note: empty arrays could also express an empty pipeline, but we + * should interpret them as an empty replacement document for BC. */ + 'empty_replacement:array' => [[], $expectedEmpty], + 'empty_replacement:object' => [(object) [], $expectedEmpty], + 'empty_replacement:Serializable' => [new BSONDocument([]), $expectedEmpty], + 'empty_replacement:Document' => [Document::fromPHP([]), $expectedEmpty], + ]; + } + + public function provideUpdateDocuments(): array + { + $expected = (object) ['$set' => (object) ['x' => 1]]; + + return [ + 'update:array' => [['$set' => ['x' => 1]], $expected], + 'update:object' => [(object) ['$set' => ['x' => 1]], $expected], + 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]]), $expected], + 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]]), $expected], + ]; + } + + public function provideUpdatePipelines(): array + { + $expected = [(object) ['$set' => (object) ['x' => 1]]]; + + return [ + 'pipeline:array' => [[['$set' => ['x' => 1]]], $expected], + 'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]]), $expected], + 'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]]), $expected], + /* Note: although empty pipelines are valid NOPs for update and + * findAndModify commands, they are not supported for updates in + * libmongoc since they are indistinguishable from empty replacement + * documents (both are empty bson_t structs). */ + ]; + } + protected function createDefaultReadConcern() { return new ReadConcern(); diff --git a/tests/Operation/ReplaceOneTest.php b/tests/Operation/ReplaceOneTest.php index 2b4446fc1..fd057921e 100644 --- a/tests/Operation/ReplaceOneTest.php +++ b/tests/Operation/ReplaceOneTest.php @@ -2,11 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -//use MongoDB\BSON\PackedArray; use MongoDB\Exception\InvalidArgumentException; -//use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\ReplaceOne; class ReplaceOneTest extends TestCase @@ -34,37 +30,22 @@ public function testConstructorReplacementArgument($replacement): void new ReplaceOne($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $replacement); } - public function provideReplacementDocuments() - { - return $this->wrapValuesForDataProvider([ - ['y' => 1], - (object) ['y' => 1], - new BSONDocument(['y' => 1]), - ]); - } - - /** @dataProvider provideInvalidReplacementValues */ - public function testConstructorReplacementArgumentRequiresNoOperators($replacement): void + /** @dataProvider provideUpdateDocuments */ + public function testConstructorReplacementArgumentProhibitsUpdateDocument($replacement): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('First key in $replacement argument is an update operator'); + $this->expectExceptionMessage('First key in $replacement is an update operator'); new ReplaceOne($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $replacement); } - public function provideInvalidReplacementValues(): array + /** + * @dataProvider provideUpdatePipelines + * @dataProvider provideEmptyUpdatePipelinesExcludingArray + */ + public function testConstructorReplacementArgumentProhibitsUpdatePipeline($replacement): void { - return [ - 'update:array' => [['$set' => ['x' => 1]]], - 'update:object' => [(object) ['$set' => ['x' => 1]]], - 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]])], - 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]])], - // TODO: Enable the following tests when implementing PHPLIB-1129 - //'pipeline:array' => [[['$set' => ['x' => 1]]]], - //'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]])], - //'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]])], - //'empty_pipeline:array' => [[]], - //'empty_pipeline:Serializable' => [new BSONArray([])], - //'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('$replacement is an update pipeline'); + new ReplaceOne($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $replacement); } } diff --git a/tests/Operation/TestCase.php b/tests/Operation/TestCase.php index 2de8b0e68..684899ec5 100644 --- a/tests/Operation/TestCase.php +++ b/tests/Operation/TestCase.php @@ -2,6 +2,10 @@ namespace MongoDB\Tests\Operation; +use MongoDB\BSON\Document; +use MongoDB\BSON\PackedArray; +use MongoDB\Model\BSONArray; +use MongoDB\Model\BSONDocument; use MongoDB\Tests\TestCase as BaseTestCase; /** @@ -9,4 +13,65 @@ */ abstract class TestCase extends BaseTestCase { + public function provideReplacementDocuments(): array + { + return [ + 'replacement:array' => [['x' => 1]], + 'replacement:object' => [(object) ['x' => 1]], + 'replacement:Serializable' => [new BSONDocument(['x' => 1])], + 'replacement:Document' => [Document::fromPHP(['x' => 1])], + /* Note: empty arrays could also express a pipeline, but PHPLIB + * interprets them as a replacement document for BC. */ + 'empty_replacement:array' => [[]], + 'empty_replacement:object' => [(object) []], + 'empty_replacement:Serializable' => [new BSONDocument([])], + 'empty_replacement:Document' => [Document::fromPHP([])], + ]; + } + + public function provideUpdateDocuments(): array + { + return [ + 'update:array' => [['$set' => ['x' => 1]]], + 'update:object' => [(object) ['$set' => ['x' => 1]]], + 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]])], + 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]])], + ]; + } + + public function provideUpdatePipelines(): array + { + return [ + 'pipeline:array' => [[['$set' => ['x' => 1]]]], + 'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]])], + 'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]])], + ]; + } + + public function provideEmptyUpdatePipelines(): array + { + /* Empty update pipelines are accepted by the update and findAndModify + * commands (as NOPs); however, they are not supported for updates in + * libmongoc because empty arrays and documents have the same bson_t + * representation (libmongoc considers it an empty replacement for BC). + * For consistency, PHPLIB rejects empty pipelines for updateOne, + * updateMany, and findOneAndUpdate operations. Replace operations + * interpret empty arrays as replacement documents for BC, but rejects + * other representations. */ + return [ + 'empty_pipeline:array' => [[]], + 'empty_pipeline:Serializable' => [new BSONArray([])], + 'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], + ]; + } + + public function provideEmptyUpdatePipelinesExcludingArray(): array + { + /* This data provider is used for replace operations, which accept empty + * arrays as replacement documents for BC. */ + return [ + 'empty_pipeline:Serializable' => [new BSONArray([])], + 'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], + ]; + } } diff --git a/tests/Operation/UpdateFunctionalTest.php b/tests/Operation/UpdateFunctionalTest.php index e6a24217e..57a2d827d 100644 --- a/tests/Operation/UpdateFunctionalTest.php +++ b/tests/Operation/UpdateFunctionalTest.php @@ -2,16 +2,12 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; use MongoDB\BSON\ObjectId; -use MongoDB\BSON\PackedArray; use MongoDB\Collection; use MongoDB\Driver\BulkWrite; use MongoDB\Driver\WriteConcern; use MongoDB\Exception\BadMethodCallException; use MongoDB\Exception\UnsupportedException; -use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\Update; use MongoDB\Tests\CommandObserver; use MongoDB\UpdateResult; @@ -52,18 +48,6 @@ function (array $event) use ($expectedFilter): void { ); } - public function provideFilterDocuments(): array - { - $expected = (object) ['x' => 1]; - - return [ - 'array' => [['x' => 1], $expected], - 'object' => [(object) ['x' => 1], $expected], - 'Serializable' => [new BSONDocument(['x' => 1]), $expected], - 'Document' => [Document::fromPHP(['x' => 1]), $expected], - ]; - } - /** * @dataProvider provideReplacementDocuments * @dataProvider provideUpdateDocuments @@ -92,41 +76,6 @@ function (array $event) use ($expectedUpdate): void { ); } - public function provideReplacementDocuments(): array - { - $expected = (object) ['x' => 1]; - - return [ - 'replacement:array' => [['x' => 1], $expected], - 'replacement:object' => [(object) ['x' => 1], $expected], - 'replacement:Serializable' => [new BSONDocument(['x' => 1]), $expected], - 'replacement:Document' => [Document::fromPHP(['x' => 1]), $expected], - ]; - } - - public function provideUpdateDocuments(): array - { - $expected = (object) ['$set' => (object) ['x' => 1]]; - - return [ - 'update:array' => [['$set' => ['x' => 1]], $expected], - 'update:object' => [(object) ['$set' => ['x' => 1]], $expected], - 'update:Serializable' => [new BSONDocument(['$set' => ['x' => 1]]), $expected], - 'update:Document' => [Document::fromPHP(['$set' => ['x' => 1]]), $expected], - ]; - } - - public function provideUpdatePipelines(): array - { - $expected = [(object) ['$set' => (object) ['x' => 1]]]; - - return [ - 'pipeline:array' => [[['$set' => ['x' => 1]]], $expected], - 'pipeline:Serializable' => [new BSONArray([['$set' => ['x' => 1]]]), $expected], - 'pipeline:PackedArray' => [PackedArray::fromPHP([['$set' => ['x' => 1]]]), $expected], - ]; - } - public function testSessionOption(): void { (new CommandObserver())->observe( diff --git a/tests/Operation/UpdateManyTest.php b/tests/Operation/UpdateManyTest.php index 02b2c1ae1..7f12207c8 100644 --- a/tests/Operation/UpdateManyTest.php +++ b/tests/Operation/UpdateManyTest.php @@ -2,11 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -use MongoDB\BSON\PackedArray; use MongoDB\Exception\InvalidArgumentException; -use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\UpdateMany; class UpdateManyTest extends TestCase @@ -27,6 +23,7 @@ public function testConstructorUpdateArgumentTypeCheck($update): void /** * @dataProvider provideUpdateDocuments + * @dataProvider provideUpdatePipelines * @doesNotPerformAssertions */ public function testConstructorUpdateArgument($update): void @@ -34,33 +31,14 @@ public function testConstructorUpdateArgument($update): void new UpdateMany($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $update); } - public function provideUpdateDocuments() - { - return $this->wrapValuesForDataProvider([ - ['$set' => ['y' => 1]], - (object) ['$set' => ['y' => 1]], - new BSONDocument(['$set' => ['y' => 1]]), - ]); - } - - /** @dataProvider provideInvalidUpdateValues */ - public function testConstructorUpdateArgumentRequiresOperators($update): void + /** + * @dataProvider provideReplacementDocuments + * @dataProvider provideEmptyUpdatePipelines + */ + public function testConstructorUpdateArgumentProhibitsReplacementDocumentOrEmptyPipeline($update): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Expected an update document with operator as first key or a pipeline'); + $this->expectExceptionMessage('Expected update operator(s) or non-empty pipeline for $update'); new UpdateMany($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $update); } - - public function provideInvalidUpdateValues(): array - { - return [ - 'replacement:array' => [['x' => 1]], - 'replacement:object' => [(object) ['x' => 1]], - 'replacement:Serializable' => [new BSONDocument(['x' => 1])], - 'replacement:Document' => [Document::fromPHP(['x' => 1])], - 'empty_pipeline:array' => [[]], - 'empty_pipeline:Serializable' => [new BSONArray([])], - 'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; - } } diff --git a/tests/Operation/UpdateOneTest.php b/tests/Operation/UpdateOneTest.php index fa44e7d06..657f3d650 100644 --- a/tests/Operation/UpdateOneTest.php +++ b/tests/Operation/UpdateOneTest.php @@ -2,11 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -use MongoDB\BSON\PackedArray; use MongoDB\Exception\InvalidArgumentException; -use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\UpdateOne; class UpdateOneTest extends TestCase @@ -27,6 +23,7 @@ public function testConstructorUpdateArgumentTypeCheck($update): void /** * @dataProvider provideUpdateDocuments + * @dataProvider provideUpdatePipelines * @doesNotPerformAssertions */ public function testConstructorUpdateArgument($update): void @@ -34,33 +31,14 @@ public function testConstructorUpdateArgument($update): void new UpdateOne($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $update); } - public function provideUpdateDocuments() - { - return $this->wrapValuesForDataProvider([ - ['$set' => ['y' => 1]], - (object) ['$set' => ['y' => 1]], - new BSONDocument(['$set' => ['y' => 1]]), - ]); - } - - /** @dataProvider provideInvalidUpdateValues */ - public function testConstructorUpdateArgumentRequiresOperators($update): void + /** + * @dataProvider provideReplacementDocuments + * @dataProvider provideEmptyUpdatePipelines + */ + public function testConstructorUpdateArgumentProhibitsReplacementDocumentOrEmptyPipeline($update): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Expected an update document with operator as first key or a pipeline'); + $this->expectExceptionMessage('Expected update operator(s) or non-empty pipeline for $update'); new UpdateOne($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $update); } - - public function provideInvalidUpdateValues(): array - { - return [ - 'replacement:array' => [['x' => 1]], - 'replacement:object' => [(object) ['x' => 1]], - 'replacement:Serializable' => [new BSONDocument(['x' => 1])], - 'replacement:Document' => [Document::fromPHP(['x' => 1])], - 'empty_pipeline:array' => [[]], - 'empty_pipeline:Serializable' => [new BSONArray([])], - 'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; - } } diff --git a/tests/Operation/UpdateTest.php b/tests/Operation/UpdateTest.php index 40dce6238..bcc445544 100644 --- a/tests/Operation/UpdateTest.php +++ b/tests/Operation/UpdateTest.php @@ -2,11 +2,7 @@ namespace MongoDB\Tests\Operation; -use MongoDB\BSON\Document; -use MongoDB\BSON\PackedArray; use MongoDB\Exception\InvalidArgumentException; -use MongoDB\Model\BSONArray; -use MongoDB\Model\BSONDocument; use MongoDB\Operation\Update; class UpdateTest extends TestCase @@ -69,24 +65,14 @@ public function provideInvalidConstructorOptions() return $options; } - /** @dataProvider provideInvalidUpdateValues */ - public function testConstructorMultiOptionRequiresOperators($update): void + /** + * @dataProvider provideReplacementDocuments + * @dataProvider provideEmptyUpdatePipelines + */ + public function testConstructorMultiOptionProhibitsReplacementDocumentOrEmptyPipeline($update): void { $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('"multi" option cannot be true if $update is a replacement document'); + $this->expectExceptionMessage('"multi" option cannot be true unless $update has update operator(s) or non-empty pipeline'); new Update($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $update, ['multi' => true]); } - - public function provideInvalidUpdateValues(): array - { - return [ - 'replacement:array' => [['x' => 1]], - 'replacement:object' => [(object) ['x' => 1]], - 'replacement:Serializable' => [new BSONDocument(['x' => 1])], - 'replacement:Document' => [Document::fromPHP(['x' => 1])], - 'empty_pipeline:array' => [[]], - 'empty_pipeline:Serializable' => [new BSONArray([])], - 'empty_pipeline:PackedArray' => [PackedArray::fromPHP([])], - ]; - } } From ae2b853aa96a5dbb1e8eb4e1dc3493305e794243 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 8 Jun 2023 20:42:54 -0400 Subject: [PATCH 2/3] Demonstrate edge case where replacement doc encodes as pipeline array --- tests/Operation/UpdateFunctionalTest.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/Operation/UpdateFunctionalTest.php b/tests/Operation/UpdateFunctionalTest.php index 57a2d827d..3fe2fdb7e 100644 --- a/tests/Operation/UpdateFunctionalTest.php +++ b/tests/Operation/UpdateFunctionalTest.php @@ -52,6 +52,7 @@ function (array $event) use ($expectedFilter): void { * @dataProvider provideReplacementDocuments * @dataProvider provideUpdateDocuments * @dataProvider provideUpdatePipelines + * @dataProvider provideReplacementDocumentLikePipeline */ public function testUpdateDocuments($update, $expectedUpdate): void { @@ -76,6 +77,18 @@ function (array $event) use ($expectedUpdate): void { ); } + public function provideReplacementDocumentLikePipeline(): array + { + /* libmongoc encodes this replacement document as a BSON array because + * it resembles an update pipeline (see: CDRIVER-4658). */ + return [ + 'replacement_like_pipeline' => [ + (object) ['0' => ['$set' => ['x' => 1]]], + [(object) ['$set' => (object) ['x' => 1]]], + ], + ]; + } + public function testSessionOption(): void { (new CommandObserver())->observe( From 45d51d6efe28ce3cfffd658a1ffdfc519474c8e7 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Fri, 9 Jun 2023 14:12:22 -0400 Subject: [PATCH 3/3] Test FindAndModify with replacement doc resembling a pipeline Note that FindAndModify's behavior differs from Update since it is unaffected by libmongoc's pipeline detection for update commands. FindAndModify behaves correctly. Add a comment explaining how the conditional object cast in FindAndModify::createCommandDocument() affects BSON encoding for the update option. --- src/Operation/FindAndModify.php | 8 ++++++++ tests/Operation/FindAndModifyFunctionalTest.php | 14 ++++++++++++++ tests/Operation/UpdateFunctionalTest.php | 4 ++-- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/Operation/FindAndModify.php b/src/Operation/FindAndModify.php index cb97637f7..88f411362 100644 --- a/src/Operation/FindAndModify.php +++ b/src/Operation/FindAndModify.php @@ -295,6 +295,14 @@ private function createCommandDocument(): array if (isset($this->options['update'])) { /** @psalm-var array|object */ $update = $this->options['update']; + /* A non-empty pipeline will encode as a BSON array, so leave it + * as-is. Cast anything else to an object since a BSON document is + * likely expected. This includes empty arrays, which historically + * can be used to represent empty replacement documents. + * + * This also allows an empty pipeline expressed as a PackedArray or + * Serializable to still encode as a BSON array, since the object + * cast will have no effect. */ $cmd['update'] = is_pipeline($update) ? $update : (object) $update; } diff --git a/tests/Operation/FindAndModifyFunctionalTest.php b/tests/Operation/FindAndModifyFunctionalTest.php index 8b96c6606..22b2c9e56 100644 --- a/tests/Operation/FindAndModifyFunctionalTest.php +++ b/tests/Operation/FindAndModifyFunctionalTest.php @@ -51,6 +51,7 @@ public function provideQueryDocuments(): array * @dataProvider provideReplacementDocuments * @dataProvider provideUpdateDocuments * @dataProvider provideUpdatePipelines + * @dataProvider provideReplacementDocumentLikePipeline */ public function testUpdateDocuments($update, $expectedUpdate): void { @@ -73,6 +74,19 @@ function (array $event) use ($expectedUpdate): void { ); } + public function provideReplacementDocumentLikePipeline(): array + { + /* Note: this expected value differs from UpdateFunctionalTest because + * FindAndModify is not affected by libmongoc's pipeline detection for + * update commands (see: CDRIVER-4658). */ + return [ + 'replacement_like_pipeline' => [ + (object) ['0' => ['$set' => ['x' => 1]]], + (object) ['0' => (object) ['$set' => (object) ['x' => 1]]], + ], + ]; + } + /** @see https://jira.mongodb.org/browse/PHPLIB-344 */ public function testManagerReadConcernIsOmitted(): void { diff --git a/tests/Operation/UpdateFunctionalTest.php b/tests/Operation/UpdateFunctionalTest.php index 3fe2fdb7e..7af2fc674 100644 --- a/tests/Operation/UpdateFunctionalTest.php +++ b/tests/Operation/UpdateFunctionalTest.php @@ -79,8 +79,8 @@ function (array $event) use ($expectedUpdate): void { public function provideReplacementDocumentLikePipeline(): array { - /* libmongoc encodes this replacement document as a BSON array because - * it resembles an update pipeline (see: CDRIVER-4658). */ + /* Note: libmongoc encodes this replacement document as a BSON array + * because it resembles an update pipeline (see: CDRIVER-4658). */ return [ 'replacement_like_pipeline' => [ (object) ['0' => ['$set' => ['x' => 1]]],