PHPLIB-1129: Improve pipeline checks for replace operations (#1098)

* 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).

* Demonstrate edge case where replacement doc encodes as pipeline array

* 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.

Author: jmikola

psalm-baseline.xml

@@ -36,8 +36,9 @@
     </UnsafeInstantiation>
   </file>
   <file src="src/Exception/InvalidArgumentException.php">
-    <UnsafeInstantiation occurrences="1">
+    <UnsafeInstantiation occurrences="2">
       <code>new static(sprintf('Expected %s to have type "%s" but found "%s"', $name, $expectedType, get_debug_type($value)))</code>
+      <code>new static('Expected update operator(s) or non-empty pipeline for ' . $name)</code>
     </UnsafeInstantiation>
   </file>
   <file src="src/Exception/ResumeTokenException.php">
@@ -308,7 +309,7 @@
     <DocblockTypeContradiction occurrences="1">
       <code>is_array($operation)</code>
     </DocblockTypeContradiction>
-    <MixedArgument occurrences="12">
+    <MixedArgument occurrences="13">
       <code>$args</code>
       <code>$args</code>
       <code>$args</code>
@@ -320,9 +321,10 @@
       <code>$args[1]</code>
       <code>$args[1]</code>
       <code>$args[1]</code>
+      <code>$args[1]</code>
       <code>$args[2]</code>
     </MixedArgument>
-    <MixedArrayAccess occurrences="25">
+    <MixedArrayAccess occurrences="28">
       <code>$args[0]</code>
       <code>$args[0]</code>
       <code>$args[0]</code>
@@ -337,6 +339,9 @@
       <code>$args[1]</code>
       <code>$args[1]</code>
       <code>$args[1]</code>
+      <code>$args[1]</code>
+      <code>$args[1]</code>
+      <code>$args[1]</code>
       <code>$args[2]</code>
       <code>$args[2]</code>
       <code>$args[2]</code>
@@ -349,7 +354,8 @@
       <code>$args[2]['upsert']</code>
       <code>$args[2]['upsert']</code>
     </MixedArrayAccess>
-    <MixedArrayAssignment occurrences="11">
+    <MixedArrayAssignment occurrences="12">
+      <code>$args[1]</code>
       <code>$args[1]</code>
       <code>$args[1]</code>
       <code>$args[2]</code>

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])) {

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

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'])) {

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'])) {

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(

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) {

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(

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(

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;

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