From 55f15b8708f0d1ddaaba475d0226dd84b493d519 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Fri, 21 Jul 2023 18:12:35 +0000 Subject: [PATCH 1/7] fix(Firestore): Custom Multiple Db routing headers --- Firestore/src/Connection/Grpc.php | 12 +- .../tests/System/FirestoreMultipleDbTest.php | 128 ++++++++++++++++++ Firestore/tests/System/FirestoreTestCase.php | 10 +- Firestore/tests/Unit/Connection/GrpcTest.php | 3 +- 4 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 Firestore/tests/System/FirestoreMultipleDbTest.php diff --git a/Firestore/src/Connection/Grpc.php b/Firestore/src/Connection/Grpc.php index e9a1218c2163..fe2acbc3bcfc 100644 --- a/Firestore/src/Connection/Grpc.php +++ b/Firestore/src/Connection/Grpc.php @@ -113,10 +113,17 @@ public function __construct(array $config = []) //@codeCoverageIgnoreEnd $this->firestore = $this->constructGapic(FirestoreClient::class, $grpcConfig); + $projectId = $this->pluck('projectId', $config); + $databaseId = $this->pluck('database', $config); $this->resourcePrefixHeader = FirestoreClient::databaseRootName( - $this->pluck('projectId', $config), - $this->pluck('database', $config) + $projectId, + $databaseId + ); + $this->databaseRoutingHeader = sprintf( + 'project_id=%s&database_id=%s', + $projectId, + $databaseId ); } @@ -301,6 +308,7 @@ private function addRequestHeaders(array $args) ]; $args['headers']['google-cloud-resource-prefix'] = [$this->resourcePrefixHeader]; + $args['headers']['x-goog-request-params'] = [$this->databaseRoutingHeader]; // Provide authentication header for requests when emulator is enabled. if ($this->isUsingEmulator) { diff --git a/Firestore/tests/System/FirestoreMultipleDbTest.php b/Firestore/tests/System/FirestoreMultipleDbTest.php new file mode 100644 index 000000000000..b1b0104aa4a0 --- /dev/null +++ b/Firestore/tests/System/FirestoreMultipleDbTest.php @@ -0,0 +1,128 @@ +newDocument(); + $this->document = $doc; + } + + public function testInsert() + { + $this->assertFalse($this->document->snapshot()->exists()); + + self::$multiDbClient->runTransaction(function ($t) { + $t->create($this->document, [ + 'foo' => 'bar' + ]); + }); + + $this->assertTrue($this->document->snapshot()->exists()); + } + + public function testUpdate() + { + $this->document->create([ + 'foo' => 'bar' + ]); + + self::$multiDbClient->runTransaction(function ($t) { + $t->update($this->document, [ + ['path' => 'bat', 'value' => 'baz'] + ]); + }); + + $this->assertEquals([ + 'foo' => 'bar', + 'bat' => 'baz' + ], $this->document->snapshot()->data()); + } + + + public function testCollectionGroup() + { + list ($query) = $this->createDocuments([ + 'abc/123/%s/cg-doc1', + 'abc/123/%s/cg-doc2', + '%s/cg-doc3', + '%s/cg-doc4', + 'def/456/%s/cg-doc5', + '%s/virtual-doc/nested-coll/not-cg-doc', + 'x%s/not-cg-doc', + '%sx/not-cg-doc', + 'abc/123/%sx/not-cg-doc', + 'abc/123/x%s/not-cg-doc', + 'abc/%s', + ]); + + $this->assertEquals( + ['cg-doc1', 'cg-doc2', 'cg-doc3', 'cg-doc4', 'cg-doc5'], + $this->getIds($query) + ); + } + + private function createDocuments(array $paths) + { + // Create a random collection name, but make sure + // it starts with 'b' for predictable ordering. + $collectionGroup = 'b' . uniqid(self::COLLECTION_NAME); + $query = self::$multiDbClient->collectionGroup($collectionGroup); + + foreach ($paths as &$path) { + $path = sprintf($path, $collectionGroup); + } + $batch = self::$multiDbClient->bulkWriter(); + + foreach ($paths as $docpath) { + $doc = self::$multiDbClient->document($docpath); + self::$localDeletionQueue->add($doc); + $batch->set($doc, [ + 'x' => 1 + ]); + } + + $batch->flush(); + self::$localDeletionQueue->add($collectionGroup); + + return [$query, $paths, $collectionGroup]; + } + + private function getIds(Query $query) + { + $documents = $query->documents()->rows(); + $ids = []; + foreach ($documents as $document) { + $ids[] = $document->id(); + } + sort($ids); + + return $ids; + } +} diff --git a/Firestore/tests/System/FirestoreTestCase.php b/Firestore/tests/System/FirestoreTestCase.php index 949c7c91e5ea..770ea2e8bbb5 100644 --- a/Firestore/tests/System/FirestoreTestCase.php +++ b/Firestore/tests/System/FirestoreTestCase.php @@ -25,9 +25,12 @@ class FirestoreTestCase extends SystemTestCase { const COLLECTION_NAME = 'system-test'; + const TEST_DB_NAME = 'system-tests-named-db'; protected static $client; + protected static $multiDbClient; protected static $collection; + protected static $multiDbCollection; protected static $localDeletionQueue; private static $hasSetUp = false; @@ -43,9 +46,14 @@ public static function setUpBeforeClass(): void self::$client = new FirestoreClient([ 'keyFilePath' => $keyFilePath ]); + self::$multiDbClient = new FirestoreClient([ + 'keyFilePath' => $keyFilePath, + 'database' => self::TEST_DB_NAME + ]); self::$collection = self::$client->collection(uniqid(self::COLLECTION_NAME)); + self::$multiDbCollection = self::$multiDbClient->collection(uniqid(self::COLLECTION_NAME)); self::$localDeletionQueue->add(self::$collection); - + self::$localDeletionQueue->add(self::$multiDbCollection); self::$hasSetUp = true; } diff --git a/Firestore/tests/Unit/Connection/GrpcTest.php b/Firestore/tests/Unit/Connection/GrpcTest.php index 9f982162d908..a05499f7981b 100644 --- a/Firestore/tests/Unit/Connection/GrpcTest.php +++ b/Firestore/tests/Unit/Connection/GrpcTest.php @@ -404,7 +404,8 @@ private function header() { return [ "headers" => [ - "google-cloud-resource-prefix" => ["projects/test/databases/(default)"] + "google-cloud-resource-prefix" => ["projects/test/databases/(default)"], + "x-goog-request-params" => ["project_id=test&database_id=(default)"] ] ]; } From ef5b2ca6781e9311654f5c0aa3a2becbec5c535a Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Fri, 21 Jul 2023 18:35:36 +0000 Subject: [PATCH 2/7] fix: databaseRoutingHeader is undefined --- Firestore/src/Connection/Grpc.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Firestore/src/Connection/Grpc.php b/Firestore/src/Connection/Grpc.php index fe2acbc3bcfc..037ee6a7b96f 100644 --- a/Firestore/src/Connection/Grpc.php +++ b/Firestore/src/Connection/Grpc.php @@ -56,6 +56,11 @@ class Grpc implements ConnectionInterface */ private $resourcePrefixHeader; + /** + * @var string + */ + private $databaseRoutingHeader; + /** * @var bool */ @@ -347,6 +352,7 @@ public function __debugInfo() 'serializer' => get_class($this->serializer), 'firestore' => get_class($this->firestore), 'resourcePrefixHeader' => $this->resourcePrefixHeader, + 'databaseRoutingHeader' => $this->databaseRoutingHeader, 'isUsingEmulator' => $this->isUsingEmulator ]; } From 5379bf639bcc97cd7b688d1363738caa44050821 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Tue, 25 Jul 2023 23:11:29 +0530 Subject: [PATCH 3/7] fix: incorporate pr comments Co-authored-by: Brent Shaffer --- Firestore/tests/System/FirestoreMultipleDbTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Firestore/tests/System/FirestoreMultipleDbTest.php b/Firestore/tests/System/FirestoreMultipleDbTest.php index b1b0104aa4a0..cb7f18313468 100644 --- a/Firestore/tests/System/FirestoreMultipleDbTest.php +++ b/Firestore/tests/System/FirestoreMultipleDbTest.php @@ -30,8 +30,7 @@ class FirestoreMultipleDbTest extends FirestoreTestCase public function setUp(): void { - $doc = self::$multiDbCollection->newDocument(); - $this->document = $doc; + $this->document = self::$multiDbCollection->newDocument(); } public function testInsert() From ce9a105a6d8e89bae295357693a5983a87640cda Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 26 Jul 2023 11:43:43 +0530 Subject: [PATCH 4/7] fix: incorporate PR comments --- Firestore/tests/System/FirestoreMultipleDbTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/Firestore/tests/System/FirestoreMultipleDbTest.php b/Firestore/tests/System/FirestoreMultipleDbTest.php index cb7f18313468..270c83957284 100644 --- a/Firestore/tests/System/FirestoreMultipleDbTest.php +++ b/Firestore/tests/System/FirestoreMultipleDbTest.php @@ -81,6 +81,7 @@ public function testCollectionGroup() 'abc/%s', ]); + // Returns docs with parent path having the collection id $this->assertEquals( ['cg-doc1', 'cg-doc2', 'cg-doc3', 'cg-doc4', 'cg-doc5'], $this->getIds($query) From 4f8a15dd9c612b501bf28ec9b06a62b8775dfcd1 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Thu, 27 Jul 2023 18:33:31 +0000 Subject: [PATCH 5/7] fix: address comments --- Firestore/tests/System/FirestoreMultipleDbTest.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Firestore/tests/System/FirestoreMultipleDbTest.php b/Firestore/tests/System/FirestoreMultipleDbTest.php index 270c83957284..0d3ba725ca8c 100644 --- a/Firestore/tests/System/FirestoreMultipleDbTest.php +++ b/Firestore/tests/System/FirestoreMultipleDbTest.php @@ -67,7 +67,7 @@ public function testUpdate() public function testCollectionGroup() { - list ($query) = $this->createDocuments([ + $query = $this->createDocumentsQuery([ 'abc/123/%s/cg-doc1', 'abc/123/%s/cg-doc2', '%s/cg-doc3', @@ -81,14 +81,14 @@ public function testCollectionGroup() 'abc/%s', ]); - // Returns docs with parent path having the collection id + // Returns docs only with matching exact collection group id $this->assertEquals( ['cg-doc1', 'cg-doc2', 'cg-doc3', 'cg-doc4', 'cg-doc5'], $this->getIds($query) ); } - private function createDocuments(array $paths) + private function createDocumentsQuery(array $paths) { // Create a random collection name, but make sure // it starts with 'b' for predictable ordering. @@ -111,7 +111,7 @@ private function createDocuments(array $paths) $batch->flush(); self::$localDeletionQueue->add($collectionGroup); - return [$query, $paths, $collectionGroup]; + return $query; } private function getIds(Query $query) From ff197c3c81015bf6ac53687729a2a78cd34d0344 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Thu, 27 Jul 2023 18:43:40 +0000 Subject: [PATCH 6/7] fix: better comments --- Firestore/tests/System/FirestoreMultipleDbTest.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Firestore/tests/System/FirestoreMultipleDbTest.php b/Firestore/tests/System/FirestoreMultipleDbTest.php index 0d3ba725ca8c..6b1362b9b165 100644 --- a/Firestore/tests/System/FirestoreMultipleDbTest.php +++ b/Firestore/tests/System/FirestoreMultipleDbTest.php @@ -68,17 +68,19 @@ public function testUpdate() public function testCollectionGroup() { $query = $this->createDocumentsQuery([ + // following doc paths will match based on the collection group id 'abc/123/%s/cg-doc1', 'abc/123/%s/cg-doc2', '%s/cg-doc3', '%s/cg-doc4', 'def/456/%s/cg-doc5', - '%s/virtual-doc/nested-coll/not-cg-doc', - 'x%s/not-cg-doc', - '%sx/not-cg-doc', - 'abc/123/%sx/not-cg-doc', - 'abc/123/x%s/not-cg-doc', - 'abc/%s', + // following doc paths will NOT match with collection group id + '%s/virtual-doc/nested-coll/not-cg-doc', // nested-coll + 'x%s/not-cg-doc', // x-prefix + '%sx/not-cg-doc', // x-suffix + 'abc/123/%sx/not-cg-doc', // x-prefix + 'abc/123/x%s/not-cg-doc', // x-suffix + 'abc/%s', // abc ]); // Returns docs only with matching exact collection group id From e444aeddaee35580dbdc54b0c0bee542c81d04b3 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Thu, 27 Jul 2023 19:01:42 +0000 Subject: [PATCH 7/7] fix: code nits --- .../tests/System/FirestoreMultipleDbTest.php | 43 +++++++------------ 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/Firestore/tests/System/FirestoreMultipleDbTest.php b/Firestore/tests/System/FirestoreMultipleDbTest.php index 6b1362b9b165..56f6d6aaac33 100644 --- a/Firestore/tests/System/FirestoreMultipleDbTest.php +++ b/Firestore/tests/System/FirestoreMultipleDbTest.php @@ -67,7 +67,10 @@ public function testUpdate() public function testCollectionGroup() { - $query = $this->createDocumentsQuery([ + // Create a random collection name, but make sure + // it starts with 'b' for predictable ordering. + $collectionGroup = 'b' . uniqid(self::COLLECTION_NAME); + $query = $this->createDocuments([ // following doc paths will match based on the collection group id 'abc/123/%s/cg-doc1', 'abc/123/%s/cg-doc2', @@ -81,24 +84,24 @@ public function testCollectionGroup() 'abc/123/%sx/not-cg-doc', // x-prefix 'abc/123/x%s/not-cg-doc', // x-suffix 'abc/%s', // abc - ]); + ], $collectionGroup); - // Returns docs only with matching exact collection group id - $this->assertEquals( + $query = self::$multiDbClient->collectionGroup($collectionGroup); + $documentIds = array_map( + fn($doc) => $doc->id(), + // Returns docs only with matching exact collection group id + $query->documents()->rows() + ); + $this->assertEqualsCanonicalizing( ['cg-doc1', 'cg-doc2', 'cg-doc3', 'cg-doc4', 'cg-doc5'], - $this->getIds($query) + $documentIds ); } - private function createDocumentsQuery(array $paths) + private function createDocuments(array $paths, $collectionGroupId) { - // Create a random collection name, but make sure - // it starts with 'b' for predictable ordering. - $collectionGroup = 'b' . uniqid(self::COLLECTION_NAME); - $query = self::$multiDbClient->collectionGroup($collectionGroup); - foreach ($paths as &$path) { - $path = sprintf($path, $collectionGroup); + $path = sprintf($path, $collectionGroupId); } $batch = self::$multiDbClient->bulkWriter(); @@ -111,20 +114,6 @@ private function createDocumentsQuery(array $paths) } $batch->flush(); - self::$localDeletionQueue->add($collectionGroup); - - return $query; - } - - private function getIds(Query $query) - { - $documents = $query->documents()->rows(); - $ids = []; - foreach ($documents as $document) { - $ids[] = $document->id(); - } - sort($ids); - - return $ids; + self::$localDeletionQueue->add($collectionGroupId); } }