PHPLIB-1129: Improve pipeline checks for replace operations

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