Upgrade BeginTransaction API (#7313)

* Enable credential config settings * Upgrade BatchGetDocuments API and their tests * Update migration guide for BatchGetDocuments * Update documentation for BeginTransaction API * Upgrade BeginTransaction API + testing * Self Review
googleapis · May 15, 2024 · d19f564 · d19f564
1 parent db81702
commit d19f564
Show file tree

Hide file tree

Showing 9 changed files with 273 additions and 105 deletions.
diff --git a/Firestore/src/FirestoreClient.php b/Firestore/src/FirestoreClient.php
@@ -33,7 +33,8 @@
 use Google\Cloud\Core\Retry;
 use Google\Cloud\Core\ValidateTrait;
 use Google\Cloud\Firestore\Connection\Grpc;
-use Google\Cloud\Firestore\V1\Client\FirestoreClient as ClientFirestoreClient;
+use Google\Cloud\Firestore\V1\BeginTransactionRequest;
+use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
 use Psr\Cache\CacheItemPoolInterface;
 use Psr\Http\Message\StreamInterface;
 
@@ -93,14 +94,6 @@ class FirestoreClient
 
     const MAX_RETRIES = 5;
 
-    /**
-     * Keeping this consistent with veneer libraries where
-     * multiple clients are present.
-     */
-    private const GAPIC_KEYS = [
-        ClientFirestoreClient::class
-    ];
-
     /**
      * @var Connection\ConnectionInterface
      * @internal
@@ -215,7 +208,7 @@ public function __construct(array $config = [])
 
         $this->requestHandler = new RequestHandler(
             $this->serializer,
-            self::GAPIC_KEYS,
+            [V1FirestoreClient::class],
             $config
         );
 
@@ -573,9 +566,9 @@ public function collectionGroup($id)
      * ```
      *
      * @codingStandardsIgnoreStart
-     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Firestore.BeginTransaction BeginTransaction
-     * @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.v1beta1#google.firestore.v1beta1.Firestore.Rollback Rollback
+     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.BeginTransaction BeginTransaction
+     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.Commit Commit
+     * @see https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1#google.firestore.v1.Firestore.Rollback Rollback
      * @codingStandardsIgnoreEnd
      *
      * @param callable $callable A callable function, allowing atomic operations
@@ -631,10 +624,19 @@ public function runTransaction(callable $callable, array $options = [])
         ) use (&$transactionId) {
             $database = $this->databaseName($this->projectId, $this->database);
 
-            $beginTransaction = $this->connection->beginTransaction(array_filter([
-                'database' => $database,
-                'retryTransaction' => $transactionId,
-            ]) + $options['begin']);
+            list($data, $optionalArgs) = $this->splitOptionalArgs($options['begin']);
+            if ($transactionId) {
+                $data['options']['readWrite']['retryTransaction'] = $transactionId;
+            }
+            $data['database'] = $database;
+
+            $request = $this->serializer->decodeMessage(new BeginTransactionRequest(), $data);
+            $beginTransaction = $this->requestHandler->sendRequest(
+                V1FirestoreClient::class,
+                'beginTransaction',
+                $request,
+                $optionalArgs
+            );
 
             $transactionId = $beginTransaction['transaction'];
 

diff --git a/Firestore/src/FirestoreSessionHandler.php b/Firestore/src/FirestoreSessionHandler.php
@@ -21,6 +21,8 @@
 use Google\Cloud\Core\RequestHandler;
 use SessionHandlerInterface;
 use Google\Cloud\Firestore\Connection\ConnectionInterface;
+use Google\Cloud\Firestore\V1\BeginTransactionRequest;
+use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
 
 /**
  * Custom session handler backed by Cloud Firestore.
@@ -242,9 +244,16 @@ public function open($savePath, $sessionName)
         $database = $this->databaseName($this->projectId, $this->database);
 
         try {
-            $beginTransaction = $this->connection->beginTransaction([
-                'database' => $database
-            ] + $this->options['begin']);
+            list($data, $optionalArgs) = $this->splitOptionalArgs($this->options['begin']);
+            $data['database'] = $database;
+
+            $request = $this->serializer->decodeMessage(new BeginTransactionRequest(), $data);
+            $beginTransaction = $this->requestHandler->sendRequest(
+                V1FirestoreClient::class,
+                'beginTransaction',
+                $request,
+                $optionalArgs
+            );
         } catch (ServiceException $e) {
             trigger_error(
                 sprintf('Firestore beginTransaction failed: %s', $e->getMessage()),
@@ -385,9 +394,16 @@ public function gc($maxlifetime)
         $deleteCount = 0;
         try {
             $database = $this->databaseName($this->projectId, $this->database);
-            $beginTransaction = $this->connection->beginTransaction([
-                'database' => $database
-            ] + $this->options['begin']);
+            list($data, $optionalArgs) = $this->splitOptionalArgs($this->options['begin']);
+            $data['database'] = $database;
+
+            $request = $this->serializer->decodeMessage(new BeginTransactionRequest(), $data);
+            $beginTransaction = $this->requestHandler->sendRequest(
+                V1FirestoreClient::class,
+                'beginTransaction',
+                $request,
+                $optionalArgs
+            );
 
             $transaction = new Transaction(
                 $this->connection,

diff --git a/Firestore/src/test.php b/Firestore/src/test.php
diff --git a/Firestore/tests/Snippet/FirestoreClientTest.php b/Firestore/tests/Snippet/FirestoreClientTest.php
@@ -33,6 +33,7 @@
 use Google\Cloud\Firestore\FieldPath;
 use Google\Cloud\Firestore\FirestoreClient;
 use Google\Cloud\Firestore\V1\BatchGetDocumentsRequest;
+use Google\Cloud\Firestore\V1\BeginTransactionRequest;
 use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
 use Google\Cloud\Firestore\WriteBatch;
 use Prophecy\Argument;
@@ -218,9 +219,12 @@ public function testRunTransaction()
         $from = sprintf('projects/%s/databases/%s/documents/users/john', self::PROJECT, self::DATABASE);
         $to = sprintf('projects/%s/databases/%s/documents/users/dave', self::PROJECT, self::DATABASE);
 
-        $this->connection->beginTransaction(Argument::any())
-            ->shouldBeCalled()
-            ->willReturn(['transaction' => 'foo']);
+        $this->requestHandler->sendRequest(
+            V1FirestoreClient::class,
+            'beginTransaction',
+            Argument::type(BeginTransactionRequest::class),
+            Argument::cetera()
+        )->shouldBeCalled()->willReturn(['transaction' => 'foo']);
 
         $this->requestHandler->sendRequest(
             V1FirestoreClient::class,

diff --git a/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php b/Firestore/tests/Snippet/FirestoreSessionHandlerTest.php
@@ -26,6 +26,7 @@
 use Google\Cloud\Firestore\FirestoreClient;
 use Google\Cloud\Firestore\FirestoreSessionHandler;
 use Google\Cloud\Firestore\V1\BatchGetDocumentsRequest;
+use Google\Cloud\Firestore\V1\BeginTransactionRequest;
 use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
 use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
@@ -93,11 +94,14 @@ public function testClass()
             ]
         ]));
 
-        $this->connection->beginTransaction(Argument::any())
-            ->shouldBeCalled()
-            ->willReturn([
-                'transaction' => self::TRANSACTION
-            ]);
+        $this->requestHandler->sendRequest(
+            V1FirestoreClient::class,
+            'beginTransaction',
+            Argument::type(BeginTransactionRequest::class),
+            Argument::cetera()
+        )->shouldBeCalled()->willReturn([
+            'transaction' => self::TRANSACTION
+        ]);
 
         $value = 'name|' . serialize('Bob');
         $this->connection->commit(Argument::allOf(
@@ -139,11 +143,14 @@ public function testSessionHandlerMethod()
             ]
         ]));
 
-        $this->connection->beginTransaction(Argument::any())
-            ->shouldBeCalled()
-            ->willReturn([
-                'transaction' => self::TRANSACTION
-            ]);
+        $this->requestHandler->sendRequest(
+            V1FirestoreClient::class,
+            'beginTransaction',
+            Argument::type(BeginTransactionRequest::class),
+            Argument::cetera()
+        )->shouldBeCalled()->willReturn([
+            'transaction' => self::TRANSACTION
+        ]);
 
         $value = 'name|' . serialize('Bob');
         $this->connection->commit(Argument::allOf(
@@ -188,11 +195,14 @@ public function testClassErrorHandler()
             ]
         ]));
 
-        $this->connection->beginTransaction(Argument::any())
-            ->shouldBeCalled()
-            ->willReturn([
-                'transaction' => self::TRANSACTION
-            ]);
+        $this->requestHandler->sendRequest(
+            V1FirestoreClient::class,
+            'beginTransaction',
+            Argument::type(BeginTransactionRequest::class),
+            Argument::cetera()
+        )->shouldBeCalled()->willReturn([
+            'transaction' => self::TRANSACTION
+        ]);
 
         $this->connection->commit(Argument::any())
             ->shouldBeCalled()

diff --git a/Firestore/tests/Snippet/TransactionTest.php b/Firestore/tests/Snippet/TransactionTest.php
@@ -36,6 +36,7 @@
 use Google\Cloud\Firestore\QuerySnapshot;
 use Google\Cloud\Firestore\Transaction;
 use Google\Cloud\Firestore\V1\BatchGetDocumentsRequest;
+use Google\Cloud\Firestore\V1\BeginTransactionRequest;
 use Google\Cloud\Firestore\V1\Client\FirestoreClient as V1FirestoreClient;
 use Google\Cloud\Firestore\ValueMapper;
 use Google\Cloud\Firestore\WriteBatch;
@@ -95,14 +96,21 @@ public function testClass()
     {
         $this->checkAndSkipGrpcTests();
 
-        $this->connection->beginTransaction(Argument::any())
-            ->shouldBeCalled()
-            ->willReturn(['transaction' => self::TRANSACTION]);
+        $this->requestHandler->sendRequest(
+            V1FirestoreClient::class,
+            'beginTransaction',
+            Argument::type(BeginTransactionRequest::class),
+            Argument::cetera()
+        )->shouldBeCalled()->willReturn(['transaction' => self::TRANSACTION]);
 
         $this->connection->rollback(Argument::any())
             ->shouldBeCalled();
 
-        $client = TestHelpers::stub(FirestoreClient::class);
+        $client = TestHelpers::stub(FirestoreClient::class, [], [
+            'connection',
+            'requestHandler'
+        ]);
+        $client->___setProperty('requestHandler', $this->requestHandler->reveal());
         $client->___setProperty('connection', $this->connection->reveal());
 
         $snippet = $this->snippetFromClass(Transaction::class);

diff --git a/Firestore/tests/Unit/ConformanceTest.php b/Firestore/tests/Unit/ConformanceTest.php
@@ -91,7 +91,7 @@ public function setUp(): void
             [
                 'projectId' => 'projectID'
             ]
-        ], ['requestHandler']);
+        ], ['requestHandler', 'connection']);
 
         $this->connection = $this->prophesize(ConnectionInterface::class);
         $this->requestHandler = $this->prophesize(RequestHandler::class);