feat(Firestore): Upgrade Commit API logic and tests (#7330)

Core/src/Testing/FirestoreTestHelperTrait.php

@@ -31,6 +31,9 @@ trait FirestoreTestHelperTrait
 
     private static $_serializer;
 
+    /**
+     * @return Serializer
+     */
     private function getSerializer()
     {
         if (!self::$_serializer) {

Firestore/src/BulkWriter.php

@@ -19,6 +19,7 @@
 
 use Google\ApiCore\Serializer;
 use Google\Cloud\Core\ArrayTrait;
+use Google\Cloud\Core\ApiHelperTrait;
 use Google\Cloud\Core\DebugInfoTrait;
 use Google\Cloud\Core\RequestHandler;
 use Google\Cloud\Core\Timestamp;
@@ -28,6 +29,9 @@
 use Google\Cloud\Firestore\FieldValue\DeleteFieldValue;
 use Google\Cloud\Firestore\FieldValue\DocumentTransformInterface;
 use Google\Cloud\Firestore\FieldValue\FieldValueInterface;
+use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
+use Google\Cloud\Firestore\V1\CommitRequest;
+use Google\Protobuf\Timestamp as ProtobufTimestamp;
 use Google\Rpc\Code;
 
 /**
@@ -49,7 +53,7 @@
  */
 class BulkWriter
 {
-    use ArrayTrait;
+    use ApiHelperTrait;
     use DebugInfoTrait;
     use TimeTrait;
     use ValidateTrait;
@@ -722,23 +726,32 @@ public function flush($waitForRetryableFailures = false)
      * ```
      *
      * @codingStandardsIgnoreStart
-     * @see https://firebase.google.com/docs/firestore/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Firestore.Commit Commit
+     * @see https://firebase.google.com/docs/firestore/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.Commit Commit
      *
      * @internal Only supposed to be used internally in Transaction class.
      * @access private
      * @param array $options Configuration Options
-     * @return array [https://firebase.google.com/docs/firestore/reference/rpc/google.firestore.v1beta1#commitresponse](CommitResponse)
+     * @return array [https://firebase.google.com/docs/firestore/reference/rpc/google.firestore.v1#commitresponse](CommitResponse)
      * @codingStandardsIgnoreEnd
      */
     public function commit(array $options = [])
     {
         unset($options['merge'], $options['precondition']);
 
-        $response = $this->connection->commit(array_filter([
+        list($data, $optionalArgs) = $this->splitOptionalArgs($options);
+        $data += array_filter([
             'database' => $this->database,
             'writes' => $this->writes,
             'transaction' => $this->transaction,
-        ]) + $options);
+        ]);
+        $request = $this->serializer->decodeMessage(new CommitRequest(), $data);
+
+        $response = $this->requestHandler->sendRequest(
+            V1FirestoreClient::class,
+            'commit',
+            $request,
+            $optionalArgs
+        );
 
         if (isset($response['commitTime'])) {
             $time = $this->parseTimeString($response['commitTime']);
@@ -986,10 +999,14 @@ private function sendBatch(array $writes, array $options = [])
 
         if (isset($response['writeResults'])) {
             foreach ($response['writeResults'] as &$result) {
-                if (isset($result['updateTime'])) {
-                    $time = $this->parseTimeString($result['updateTime']);
-                    $result['updateTime'] = new Timestamp($time[0], $time[1]);
-                }
+                // Commenting this out right now because we need to decide if we want to parse the returned
+                // RFC3339 string which is not very user friendly to interpret. The tradeoff is we need
+                // to parse the returned value. Maybe we can add this logic in serializer or something.
+
+                // if (isset($result['updateTime'])) {
+                //     $time = $this->parseTimeString($result['updateTime']);
+                //     $result['updateTime'] = new Timestamp($time[0], $time[1]);
+                // }
             }
         }
 
@@ -1095,15 +1112,16 @@ private function validatePrecondition(array &$options)
         }
 
         if (isset($precondition['updateTime'])) {
-            if (!($precondition['updateTime'] instanceof Timestamp)) {
+            if (!($precondition['updateTime'] instanceof ProtobufTimestamp)
+                && !is_array($precondition['updateTime'])
+            ) {
                 throw new \InvalidArgumentException(
-                    'Precondition Update Time must be an instance of `Google\\Cloud\\Core\\Timestamp`'
+                    'Precondition Update Time must be an instance of `Google\\Protobuf\\Timestamp` ' .
+                    'or array representation of the same.'
                 );
             }
 
-            return [
-                'updateTime' => $precondition['updateTime']->formatForApi(),
-            ];
+            return $precondition;
         }
 
         throw new \InvalidArgumentException('Preconditions must provide either `exists` or `updateTime`.');

Firestore/src/DocumentReference.php

@@ -188,14 +188,14 @@ public function id()
      * ```
      *
      * @codingStandardsIgnoreStart
-     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Firestore.Commit Commit
+     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.Commit Commit
      *
      * @param array $fields An array containing fields, where keys are the field
      *        names, and values are field values. Nested arrays are allowed.
      *        Note that unlike {@see \Google\Cloud\Firestore\DocumentReference::update()},
      *        field paths are NOT supported by this method.
      * @param array $options Configuration Options.
-     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.WriteResult)
+     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.WriteResult)
      * @codingStandardsIgnoreEnd
      */
     public function create(array $fields = [], array $options = [])
@@ -223,7 +223,7 @@ public function create(array $fields = [], array $options = [])
      * ```
      *
      * @codingStandardsIgnoreStart
-     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Firestore.Commit Commit
+     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.Commit Commit
      * @codingStandardsIgnoreEnd
      *
      * @param array $fields An array containing fields, where keys are the field
@@ -238,7 +238,7 @@ public function create(array $fields = [], array $options = [])
      *           `false`.
      * }
      * @codingStandardsIgnoreStart
-     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.WriteResult)
+     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.WriteResult)
      * @codingStandardsIgnoreEnd
      */
     public function set(array $fields, array $options = [])
@@ -304,12 +304,12 @@ public function set(array $fields, array $options = [])
      * ```
      *
      * @codingStandardsIgnoreStart
-     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Firestore.Commit Commit
+     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.Commit Commit
      *
      * @param array[] $data A list of arrays of form
      *        `[FieldPath|string $path, mixed $value]`.
      * @param array $options Configuration options
-     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.WriteResult)
+     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.WriteResult)
      * @throws \InvalidArgumentException If data is given in an invalid format
      *         or is empty.
      * @throws \InvalidArgumentException If any field paths are empty.
@@ -334,10 +334,10 @@ public function update(array $data, array $options = [])
      * ```
      *
      * @codingStandardsIgnoreStart
-     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Firestore.Commit Commit
+     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.Commit Commit
      *
      * @param array $options Configuration Options
-     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.WriteResult)
+     * @return array [WriteResult](https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.WriteResult)
      * @codingStandardsIgnoreEnd
      */
     public function delete(array $options = [])

Firestore/tests/Snippet/BulkWriterTest.php

@@ -25,6 +25,7 @@
 use Google\Cloud\Core\Testing\TestHelpers;
 use Google\Cloud\Firestore\BulkWriter;
 use Google\Cloud\Firestore\Connection\ConnectionInterface;
+use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
 use Google\Cloud\Firestore\V1\DocumentTransform\FieldTransform\ServerValue;
 use Google\Cloud\Firestore\ValueMapper;
 use Google\Rpc\Code;

Firestore/tests/Snippet/CollectionReferenceTest.php

@@ -29,6 +29,8 @@
 use Google\Cloud\Firestore\CollectionReference;
 use Google\Cloud\Core\Testing\Snippet\SnippetTestCase;
 use Google\Cloud\Firestore\Connection\ConnectionInterface;
+use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
+use Google\Cloud\Firestore\V1\CommitRequest;
 
 /**
  * @group firestore
@@ -65,7 +67,7 @@ public function setUp(): void
                 false
             ),
             self::NAME
-        ]);
+        ], ['requestHandler', 'connection']);
     }
 
     public function testClass()
@@ -152,10 +154,16 @@ public function testNewDocument()
 
     public function testAdd()
     {
-        $this->connection->commit(Argument::any())
+        $this->requestHandler->sendRequest(
+            V1FirestoreClient::class,
+            'commit',
+            Argument::type(CommitRequest::class),
+            Argument::cetera()
+        )
             ->shouldBeCalled()
             ->willReturn([[]]);
-        $this->collection->___setProperty('connection', $this->connection->reveal());
+
+        $this->collection->___setProperty('requestHandler', $this->requestHandler->reveal());
 
         $snippet = $this->snippetFromMethod(CollectionReference::class, 'add');
         $snippet->addLocal('collection', $this->collection);