From 63655f5babf8b25738f0b34817e2e3b354139b94 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 9 Nov 2022 11:54:12 +0530 Subject: [PATCH 01/12] fix(datastore): fix query limit above 300 modified: tests/System/QueryResultPaginationTest.php --- Datastore/src/Connection/Grpc.php | 2 +- Datastore/src/Operation.php | 5 ++++ .../System/QueryResultPaginationTest.php | 28 +++++++++++++++++-- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/Datastore/src/Connection/Grpc.php b/Datastore/src/Connection/Grpc.php index fb6e5fc80fd..f9add2fa8a1 100644 --- a/Datastore/src/Connection/Grpc.php +++ b/Datastore/src/Connection/Grpc.php @@ -242,7 +242,7 @@ public function runQuery(array $args) $query['filter'] = $this->convertFilterProps($query['filter']); } - if (isset($query['limit'])) { + if (isset($query['limit']) && is_int($query['limit'])) { $query['limit'] = [ 'value' => $query['limit'] ]; diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index b8dd58ef0da..5844f232395 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -451,6 +451,11 @@ public function runQuery(QueryInterface $query, array $options = []) } ]; + if (isset($query->queryObject()['limit'])) { + // Setting resultLimit ensures we loop through all pages. + // Datastore doesn't process more than 1000 skipped results in a query. + $iteratorConfig['resultLimit'] = $query->queryObject()['limit']; + } $runQueryObj = clone $query; $runQueryFn = function (array $args = []) use (&$runQueryObj, $options) { $args += [ diff --git a/Datastore/tests/System/QueryResultPaginationTest.php b/Datastore/tests/System/QueryResultPaginationTest.php index ad08e22c44d..28026d20993 100644 --- a/Datastore/tests/System/QueryResultPaginationTest.php +++ b/Datastore/tests/System/QueryResultPaginationTest.php @@ -51,7 +51,7 @@ public static function set_up_before_class() $key->ancestorKey(self::$parentKey); $set[] = $client->entity($key, [ - 'a' => rand(1, 10) + 'a' => rand(1, 10), ]); if (count($set) === 100) { @@ -128,12 +128,25 @@ public function testGqlQueryPaginationByPage(DatastoreClient $client) $parentId ); $q = $client->gqlQuery($queryString, [ - 'allowLiterals' => true + 'allowLiterals' => true, ]); $this->assertQueryPageCount(self::$expectedTotal, $client, $q); } + /** + * @dataProvider clientProvider + */ + public function testGqlQueryPaginationWithLimit(DatastoreClient $client) + { + $testLimit = 310; + $q = $client->gqlQuery( + sprintf('SELECT * FROM `%s` LIMIT %s', self::$testKind, $testLimit), + ['allowLiterals' => true] + ); + $this->assertQueryCount($testLimit, $client, $q); + } + /** * @dataProvider clientProvider */ @@ -144,6 +157,17 @@ public function testQueryPaginationByPage(DatastoreClient $client) $this->assertQueryPageCount(self::$expectedTotal, $client, $q); } + /** + * @dataProvider clientProvider + */ + public function testQueryPaginationWithLimit(DatastoreClient $client) + { + $testLimit = 310; + $q = $client->query()->kind(self::$testKind)->limit($testLimit); + + $this->assertQueryPageCount($testLimit, $client, $q); + } + private function assertQueryCount($expected, DatastoreClient $client, QueryInterface $query) { $res = $client->runQuery($query); From 0fc89a15615875614ca9a5ee0bee13c40245945c Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 9 Nov 2022 15:28:20 +0530 Subject: [PATCH 02/12] chore: move GQL query test --- Datastore/src/Operation.php | 1 - .../tests/System/QueryResultPaginationTest.php | 13 ------------- 2 files changed, 14 deletions(-) diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index 5844f232395..3ccd2499838 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -453,7 +453,6 @@ public function runQuery(QueryInterface $query, array $options = []) if (isset($query->queryObject()['limit'])) { // Setting resultLimit ensures we loop through all pages. - // Datastore doesn't process more than 1000 skipped results in a query. $iteratorConfig['resultLimit'] = $query->queryObject()['limit']; } $runQueryObj = clone $query; diff --git a/Datastore/tests/System/QueryResultPaginationTest.php b/Datastore/tests/System/QueryResultPaginationTest.php index 28026d20993..9e411c876d9 100644 --- a/Datastore/tests/System/QueryResultPaginationTest.php +++ b/Datastore/tests/System/QueryResultPaginationTest.php @@ -134,19 +134,6 @@ public function testGqlQueryPaginationByPage(DatastoreClient $client) $this->assertQueryPageCount(self::$expectedTotal, $client, $q); } - /** - * @dataProvider clientProvider - */ - public function testGqlQueryPaginationWithLimit(DatastoreClient $client) - { - $testLimit = 310; - $q = $client->gqlQuery( - sprintf('SELECT * FROM `%s` LIMIT %s', self::$testKind, $testLimit), - ['allowLiterals' => true] - ); - $this->assertQueryCount($testLimit, $client, $q); - } - /** * @dataProvider clientProvider */ From f902098f1d0a63b9d72841f62db08d84d79a749d Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 9 Nov 2022 15:29:12 +0530 Subject: [PATCH 03/12] fix: test for gql query 300 limit --- Datastore/src/EntityPageIterator.php | 13 ++++ .../System/QueryResultPaginationTest.php | 13 ++++ .../tests/Unit/EntityPageIteratorTest.php | 66 +++++++++++++++++-- 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/Datastore/src/EntityPageIterator.php b/Datastore/src/EntityPageIterator.php index 0af78d977f9..3d865f2e3e5 100644 --- a/Datastore/src/EntityPageIterator.php +++ b/Datastore/src/EntityPageIterator.php @@ -61,6 +61,19 @@ public function current() $this->moreResultsType = isset($this->page['batch']['moreResults']) ? $this->page['batch']['moreResults'] : null; + if ($this->config['resultLimit'] === 0 && + isset($this->page['query']['limit']) + ) { + // limit is not set properly, so set the limit from parsed query + // which is returned by the server together with batch response + $limit = $this->page['query']['limit']; + if (is_array($limit) && + isset($this->page['query']['limit']['value']) + ) { + $limit = $this->page['query']['limit']['value']; + } + $this->config['resultLimit'] = $limit; + } return $this->get($this->itemsPath, $this->page); } diff --git a/Datastore/tests/System/QueryResultPaginationTest.php b/Datastore/tests/System/QueryResultPaginationTest.php index 9e411c876d9..28026d20993 100644 --- a/Datastore/tests/System/QueryResultPaginationTest.php +++ b/Datastore/tests/System/QueryResultPaginationTest.php @@ -134,6 +134,19 @@ public function testGqlQueryPaginationByPage(DatastoreClient $client) $this->assertQueryPageCount(self::$expectedTotal, $client, $q); } + /** + * @dataProvider clientProvider + */ + public function testGqlQueryPaginationWithLimit(DatastoreClient $client) + { + $testLimit = 310; + $q = $client->gqlQuery( + sprintf('SELECT * FROM `%s` LIMIT %s', self::$testKind, $testLimit), + ['allowLiterals' => true] + ); + $this->assertQueryCount($testLimit, $client, $q); + } + /** * @dataProvider clientProvider */ diff --git a/Datastore/tests/Unit/EntityPageIteratorTest.php b/Datastore/tests/Unit/EntityPageIteratorTest.php index 750d4e361d3..ef0e42b42be 100644 --- a/Datastore/tests/Unit/EntityPageIteratorTest.php +++ b/Datastore/tests/Unit/EntityPageIteratorTest.php @@ -18,13 +18,16 @@ namespace Google\Cloud\Datastore\Tests\Unit; use Google\Cloud\Datastore\EntityPageIterator; -use PHPUnit\Framework\TestCase; +use Yoast\PHPUnitPolyfills\Polyfills\AssertIsType; +use Yoast\PHPUnitPolyfills\TestCases\TestCase; /** * @group datastore */ class EntityPageIteratorTest extends TestCase { + use AssertIsType; + private static $moreResultsType = 'NOT_FINISHED'; private static $items = ['a', 'b', 'c']; @@ -40,6 +43,61 @@ function ($item) { $pagesArray = iterator_to_array($pages); + $this->assertOverPages($pages); + } + + public function testCurrentSetsQueryIntegerLimits() + { + $pages = new EntityPageIterator( + function ($item) { + return $item; + }, + [$this, 'theCall'], + [ + 'test' => 'call', + 'query' => ['limit' => 1000], + ] + ); + + $pagesArray = iterator_to_array($pages); + + $this->assertOverPages($pages, 1000); + } + + public function testCurrentSetsQueryArrayLimits() + { + + $pages = new EntityPageIterator( + function ($item) { + return $item; + }, + [$this, 'theCall'], + [ + 'test' => 'call', + 'query' => ['limit' => ['value' => 1100]], + ] + ); + + $this->assertOverPages($pages, 1100); + } + + private function assertOverPages($pages, $expectedLimit = 0) + { + $pagesArray = iterator_to_array($pages); + + $reflection = new \ReflectionClass($pages); + $configProp = $reflection->getProperty('config'); + $configProp->setAccessible(true); + $configs = $configProp->getValue($pages); + + $this->assertIsArray($configs); + $this->assertArrayHasKey('resultLimit', $configs); + $this->assertEquals( + $expectedLimit, + $configs['resultLimit'], + "resultLimit assertion failed." + ); + $this->assertEquals(self::$moreResultsType, $pages->moreResultsType()); $this->assertEquals(self::$items, $pagesArray[0]); } @@ -48,9 +106,9 @@ public function theCall(array $options) { return [ 'batch' => [ - 'moreResults' => self::$moreResultsType + 'moreResults' => self::$moreResultsType, ], - 'items' => self::$items - ]; + 'items' => self::$items, + ] + $options; } } From 8ba0ac3ce87e32e4e248fd1985225856c2c9ce63 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Tue, 22 Nov 2022 01:42:10 +0530 Subject: [PATCH 04/12] fix: limit last page size --- Datastore/src/Operation.php | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index 3ccd2499838..05988c1b29c 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -431,12 +431,12 @@ public function runQuery(QueryInterface $query, array $options = []) 'className' => Entity::class, 'namespaceId' => $this->namespaceId ]; - + $runQueryObj = clone $query; $iteratorConfig = [ 'itemsKey' => 'batch.entityResults', 'resultTokenKey' => 'query.startCursor', 'nextResultTokenKey' => 'batch.endCursor', - 'setNextResultTokenCondition' => function ($res) use ($query) { + 'setNextResultTokenCondition' => function ($res) use (&$runQueryObj, &$remainingLimit) { if (isset($res['batch']['moreResults'])) { $moreResultsType = $res['batch']['moreResults']; // Transform gRPC enum to string @@ -444,25 +444,38 @@ public function runQuery(QueryInterface $query, array $options = []) $moreResultsType = MoreResultsType::name($moreResultsType); } - return $query->canPaginate() && $moreResultsType === 'NOT_FINISHED'; + $isNotFinished = $runQueryObj->canPaginate() && $moreResultsType === 'NOT_FINISHED'; + if($isNotFinished && array_key_exists('limit', $runQueryObj->queryObject())) { + $remainingLimit = $runQueryObj->queryObject()['limit']; + if(is_array($remainingLimit) && array_key_exists('value', $remainingLimit)){ + $remainingLimit = $remainingLimit['value']; + } + + $remainingLimit -= count($res['batch']['entityResults']); + } + + return $isNotFinished; } return false; } ]; - if (isset($query->queryObject()['limit'])) { + if (array_key_exists('limit', $query->queryObject())) { // Setting resultLimit ensures we loop through all pages. $iteratorConfig['resultLimit'] = $query->queryObject()['limit']; + $remainingLimit = $query->queryObject()['limit']; } - $runQueryObj = clone $query; - $runQueryFn = function (array $args = []) use (&$runQueryObj, $options) { + $runQueryFn = function (array $args = []) use (&$runQueryObj, $options, &$remainingLimit) { $args += [ 'query' => [] ]; // The iterator provides the startCursor for subsequent pages as an argument. $requestQueryArr = $args['query'] + $runQueryObj->queryObject(); + if(isset($remainingLimit)) { + $requestQueryArr['limit'] = $remainingLimit; + } $request = [ 'projectId' => $this->projectId, 'partitionId' => $this->partitionId($this->projectId, $options['namespaceId']), From 5086fa003fcee3dd54036c97df014d7ef9b6e8da Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Tue, 22 Nov 2022 02:12:28 +0530 Subject: [PATCH 05/12] fix: style --- Datastore/src/Operation.php | 57 +++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index 05988c1b29c..f2801becb08 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -101,7 +101,7 @@ public function __construct( public function key($kind, $identifier = null, array $options = []) { $options += [ - 'namespaceId' => $this->namespaceId + 'namespaceId' => $this->namespaceId, ]; $key = new Key($this->projectId, $options); @@ -142,7 +142,7 @@ public function keys($kind, array $options = []) 'number' => 1, 'ancestors' => [], 'id' => null, - 'name' => null + 'name' => null, ]; if ($options['number'] < 1) { @@ -157,12 +157,12 @@ public function keys($kind, array $options = []) $path[] = array_filter([ 'kind' => $kind, 'id' => $options['id'], - 'name' => $options['name'] + 'name' => $options['name'], ]); $key = new Key($this->projectId, [ 'path' => $path, - 'namespaceId' => $this->namespaceId + 'namespaceId' => $this->namespaceId, ]); $keys = [$key]; @@ -215,7 +215,7 @@ public function keys($kind, array $options = []) public function entity($key = null, array $entity = [], array $options = []) { $options += [ - 'className' => null + 'className' => null, ]; if ($key && !is_string($key) && !($key instanceof Key)) { @@ -255,7 +255,7 @@ public function beginTransaction($transactionOptions, array $options = []) { $res = $this->connection->beginTransaction([ 'projectId' => $this->projectId, - 'transactionOptions' => $transactionOptions + 'transactionOptions' => $transactionOptions, ] + $options); return $res['transaction']; @@ -299,7 +299,7 @@ public function allocateIds(array $keys, array $options = []) $res = $this->connection->allocateIds([ 'projectId' => $this->projectId, - 'keys' => $serviceKeys + 'keys' => $serviceKeys, ] + $options); if (isset($res['keys'])) { @@ -348,7 +348,7 @@ public function lookup(array $keys, array $options = []) { $options += [ 'className' => Entity::class, - 'sort' => false + 'sort' => false, ]; $serviceKeys = []; @@ -366,7 +366,7 @@ public function lookup(array $keys, array $options = []) $res = $this->connection->lookup($options + $this->readOptions($options) + [ 'projectId' => $this->projectId, - 'keys' => $serviceKeys + 'keys' => $serviceKeys, ]); $result = []; @@ -429,7 +429,7 @@ public function runQuery(QueryInterface $query, array $options = []) { $options += [ 'className' => Entity::class, - 'namespaceId' => $this->namespaceId + 'namespaceId' => $this->namespaceId, ]; $runQueryObj = clone $query; $iteratorConfig = [ @@ -445,9 +445,10 @@ public function runQuery(QueryInterface $query, array $options = []) } $isNotFinished = $runQueryObj->canPaginate() && $moreResultsType === 'NOT_FINISHED'; - if($isNotFinished && array_key_exists('limit', $runQueryObj->queryObject())) { + if ($isNotFinished && array_key_exists('limit', $runQueryObj->queryObject())) { $remainingLimit = $runQueryObj->queryObject()['limit']; - if(is_array($remainingLimit) && array_key_exists('value', $remainingLimit)){ + if (is_array($remainingLimit) && + array_key_exists('value', $remainingLimit)) { $remainingLimit = $remainingLimit['value']; } @@ -458,7 +459,7 @@ public function runQuery(QueryInterface $query, array $options = []) } return false; - } + }, ]; if (array_key_exists('limit', $query->queryObject())) { @@ -468,18 +469,18 @@ public function runQuery(QueryInterface $query, array $options = []) } $runQueryFn = function (array $args = []) use (&$runQueryObj, $options, &$remainingLimit) { $args += [ - 'query' => [] + 'query' => [], ]; // The iterator provides the startCursor for subsequent pages as an argument. $requestQueryArr = $args['query'] + $runQueryObj->queryObject(); - if(isset($remainingLimit)) { + if (isset($remainingLimit)) { $requestQueryArr['limit'] = $remainingLimit; } $request = [ 'projectId' => $this->projectId, 'partitionId' => $this->partitionId($this->projectId, $options['namespaceId']), - $runQueryObj->queryKey() => $requestQueryArr + $runQueryObj->queryKey() => $requestQueryArr, ] + $this->readOptions($options) + $options; $res = $this->connection->runQuery($request); @@ -529,13 +530,13 @@ function (array $entityResult) use ($options) { public function commit(array $mutations, array $options = []) { $options += [ - 'transaction' => null + 'transaction' => null, ]; $res = $this->connection->commit($options + [ 'mode' => ($options['transaction']) ? 'TRANSACTIONAL' : 'NON_TRANSACTIONAL', 'mutations' => $mutations, - 'projectId' => $this->projectId + 'projectId' => $this->projectId, ]); return $res; @@ -616,7 +617,7 @@ public function mutation( return array_filter([ $operation => $data, - 'baseVersion' => $baseVersion + 'baseVersion' => $baseVersion, ]); } @@ -630,7 +631,7 @@ public function rollback($transactionId) { $this->connection->rollback([ 'projectId' => $this->projectId, - 'transaction' => $transactionId + 'transaction' => $transactionId, ]); } @@ -650,9 +651,9 @@ public function checkOverwrite(array $entities, $allowOverwrite = false) foreach ($entities as $entity) { if (!$entity->populatedByService() && !$allowOverwrite) { throw new \InvalidArgumentException(sprintf( - 'Given entity cannot be saved because it may overwrite an '. - 'existing record. When updating manually created entities, '. - 'please set the options `$allowOverwrite` flag to `true`. '. + 'Given entity cannot be saved because it may overwrite an ' . + 'existing record. When updating manually created entities, ' . + 'please set the options `$allowOverwrite` flag to `true`. ' . 'Invalid entity key was %s', (string) $entity->key() )); @@ -684,7 +685,7 @@ private function mapEntityResult(array $result, $class) $key = new Key($this->projectId, [ 'path' => $entity['key']['path'], - 'namespaceId' => $namespaceId + 'namespaceId' => $namespaceId, ]); if (is_array($class)) { @@ -723,7 +724,7 @@ private function mapEntityResult(array $result, $class) 'className' => $className, 'populatedByService' => true, 'excludeFromIndexes' => $excludes, - 'meanings' => $meanings + 'meanings' => $meanings, ]); } @@ -743,16 +744,16 @@ private function readOptions(array $options = []) { $options += [ 'readConsistency' => null, - 'transaction' => null + 'transaction' => null, ]; $readOptions = array_filter([ 'readConsistency' => $options['readConsistency'], - 'transaction' => $options['transaction'] + 'transaction' => $options['transaction'], ]); return array_filter([ - 'readOptions' => $readOptions + 'readOptions' => $readOptions, ]); } From e60e12e813e87087233efe6bceeadb0affb1692e Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Thu, 8 Dec 2022 15:29:45 +0530 Subject: [PATCH 06/12] fix: merge conflicts --- Datastore/src/Operation.php | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index 09e465f76c1..62c285f85fe 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -387,7 +387,10 @@ public function lookup(array $keys, array $options = []) $result = []; if (isset($res['found'])) { foreach ($res['found'] as $found) { + $result['found'][] = $this->mapEntityResult($found, $options['className']); + } + if ($options['sort']) { $result['found'] = $this->sortEntities($result['found'], $keys); } } @@ -701,11 +704,11 @@ private function mapEntityResult(array $result, $class) $entity = $result['entity']; $namespaceId = (isset($entity['key']['partitionId']['namespaceId'])) - ? $entity['key']['partitionId']['namespaceId'] - : null; + ? $entity['key']['partitionId']['namespaceId'] + : null; $databaseId = (isset($entity['key']['partitionId']['databaseId'])) - ? $entity['key']['partitionId']['databaseId'] - : ''; + ? $entity['key']['partitionId']['databaseId'] + : ''; $key = new Key($this->projectId, [ 'path' => $entity['key']['path'], @@ -741,11 +744,11 @@ private function mapEntityResult(array $result, $class) return $this->entity($key, $properties, [ 'cursor' => (isset($result['cursor'])) - ? $result['cursor'] - : null, + ? $result['cursor'] + : null, 'baseVersion' => (isset($result['version'])) - ? $result['version'] - : null, + ? $result['version'] + : null, 'className' => $className, 'populatedByService' => true, 'excludeFromIndexes' => $excludes, From 5940fdf2dd7aee011e1e39dec881ab8104e7361c Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Thu, 8 Dec 2022 15:33:52 +0530 Subject: [PATCH 07/12] fix: test provider should be default db only --- Datastore/src/Operation.php | 2 +- Datastore/tests/System/QueryResultPaginationTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index 62c285f85fe..97f2c17d858 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -387,7 +387,7 @@ public function lookup(array $keys, array $options = []) $result = []; if (isset($res['found'])) { foreach ($res['found'] as $found) { - $result['found'][] = $this->mapEntityResult($found, $options['className']); + $result['found'][] = $this->mapEntityResult($found, $options['className']); } if ($options['sort']) { diff --git a/Datastore/tests/System/QueryResultPaginationTest.php b/Datastore/tests/System/QueryResultPaginationTest.php index ee5001d1037..a48acaae8ee 100644 --- a/Datastore/tests/System/QueryResultPaginationTest.php +++ b/Datastore/tests/System/QueryResultPaginationTest.php @@ -148,7 +148,7 @@ public function testGqlQueryPaginationWithLimit(DatastoreClient $client) } /** - * @dataProvider clientProvider + * @dataProvider defaultDbClientProvider */ public function testQueryPaginationByPage(DatastoreClient $client) { @@ -158,7 +158,7 @@ public function testQueryPaginationByPage(DatastoreClient $client) } /** - * @dataProvider clientProvider + * @dataProvider defaultDbClientProvider */ public function testQueryPaginationWithLimit(DatastoreClient $client) { From 691531e4f96c724f98c4af91cde96bc715125fda Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 14 Dec 2022 00:40:03 +0530 Subject: [PATCH 08/12] fix: contained remainingLimit logic in runQueryFb --- Datastore/src/EntityPageIterator.php | 6 ++---- Datastore/src/Operation.php | 21 +++++++-------------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/Datastore/src/EntityPageIterator.php b/Datastore/src/EntityPageIterator.php index 3d865f2e3e5..c80a048a0c6 100644 --- a/Datastore/src/EntityPageIterator.php +++ b/Datastore/src/EntityPageIterator.php @@ -67,10 +67,8 @@ public function current() // limit is not set properly, so set the limit from parsed query // which is returned by the server together with batch response $limit = $this->page['query']['limit']; - if (is_array($limit) && - isset($this->page['query']['limit']['value']) - ) { - $limit = $this->page['query']['limit']['value']; + if (isset($limit['value'])) { + $limit = $limit['value']; } $this->config['resultLimit'] = $limit; } diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index 97f2c17d858..a6ccf481c16 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -448,12 +448,12 @@ public function runQuery(QueryInterface $query, array $options = []) 'namespaceId' => $this->namespaceId, 'databaseId' => $this->databaseId, ]; - $runQueryObj = clone $query; + $iteratorConfig = [ 'itemsKey' => 'batch.entityResults', 'resultTokenKey' => 'query.startCursor', 'nextResultTokenKey' => 'batch.endCursor', - 'setNextResultTokenCondition' => function ($res) use (&$runQueryObj, &$remainingLimit) { + 'setNextResultTokenCondition' => function ($res) use ($query) { if (isset($res['batch']['moreResults'])) { $moreResultsType = $res['batch']['moreResults']; // Transform gRPC enum to string @@ -461,18 +461,7 @@ public function runQuery(QueryInterface $query, array $options = []) $moreResultsType = MoreResultsType::name($moreResultsType); } - $isNotFinished = $runQueryObj->canPaginate() && $moreResultsType === 'NOT_FINISHED'; - if ($isNotFinished && array_key_exists('limit', $runQueryObj->queryObject())) { - $remainingLimit = $runQueryObj->queryObject()['limit']; - if (is_array($remainingLimit) && - array_key_exists('value', $remainingLimit)) { - $remainingLimit = $remainingLimit['value']; - } - - $remainingLimit -= count($res['batch']['entityResults']); - } - - return $isNotFinished; + return $query->canPaginate() && $moreResultsType === 'NOT_FINISHED'; } return false; @@ -484,6 +473,7 @@ public function runQuery(QueryInterface $query, array $options = []) $iteratorConfig['resultLimit'] = $query->queryObject()['limit']; $remainingLimit = $query->queryObject()['limit']; } + $runQueryObj = clone $query; $runQueryFn = function (array $args = []) use (&$runQueryObj, $options, &$remainingLimit) { $args += [ 'query' => [], @@ -515,6 +505,9 @@ public function runQuery(QueryInterface $query, array $options = []) if (isset($res['query'])) { $runQueryObj = new Query($this->entityMapper, $res['query']); } + if (isset($remainingLimit)) { + $remainingLimit -= count($res['batch']['entityResults']); + } return $res; }; From 950c2af9a33d0b56eef629f8717d751b12a2826f Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 14 Dec 2022 00:50:43 +0530 Subject: [PATCH 09/12] fix: ci build jobs cancel out --- .github/workflows/datastore-emulator-system-tests.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/datastore-emulator-system-tests.yaml b/.github/workflows/datastore-emulator-system-tests.yaml index daa43957d9a..0c7ea957c94 100644 --- a/.github/workflows/datastore-emulator-system-tests.yaml +++ b/.github/workflows/datastore-emulator-system-tests.yaml @@ -24,7 +24,7 @@ jobs: with: php-version: '7.4' tools: pecl - extensions: grpc + extensions: grpc-1.49.0 - name: Install dependencies run: | From a37b3c4c0b190e681e02e30ae1823aa1136a6c0d Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 14 Dec 2022 14:15:08 +0530 Subject: [PATCH 10/12] revert: undo EntityPageIterator --- Datastore/src/EntityPageIterator.php | 11 ---- .../tests/Unit/EntityPageIteratorTest.php | 66 ++----------------- 2 files changed, 4 insertions(+), 73 deletions(-) diff --git a/Datastore/src/EntityPageIterator.php b/Datastore/src/EntityPageIterator.php index c80a048a0c6..0af78d977f9 100644 --- a/Datastore/src/EntityPageIterator.php +++ b/Datastore/src/EntityPageIterator.php @@ -61,17 +61,6 @@ public function current() $this->moreResultsType = isset($this->page['batch']['moreResults']) ? $this->page['batch']['moreResults'] : null; - if ($this->config['resultLimit'] === 0 && - isset($this->page['query']['limit']) - ) { - // limit is not set properly, so set the limit from parsed query - // which is returned by the server together with batch response - $limit = $this->page['query']['limit']; - if (isset($limit['value'])) { - $limit = $limit['value']; - } - $this->config['resultLimit'] = $limit; - } return $this->get($this->itemsPath, $this->page); } diff --git a/Datastore/tests/Unit/EntityPageIteratorTest.php b/Datastore/tests/Unit/EntityPageIteratorTest.php index ef0e42b42be..750d4e361d3 100644 --- a/Datastore/tests/Unit/EntityPageIteratorTest.php +++ b/Datastore/tests/Unit/EntityPageIteratorTest.php @@ -18,16 +18,13 @@ namespace Google\Cloud\Datastore\Tests\Unit; use Google\Cloud\Datastore\EntityPageIterator; -use Yoast\PHPUnitPolyfills\Polyfills\AssertIsType; -use Yoast\PHPUnitPolyfills\TestCases\TestCase; +use PHPUnit\Framework\TestCase; /** * @group datastore */ class EntityPageIteratorTest extends TestCase { - use AssertIsType; - private static $moreResultsType = 'NOT_FINISHED'; private static $items = ['a', 'b', 'c']; @@ -43,61 +40,6 @@ function ($item) { $pagesArray = iterator_to_array($pages); - $this->assertOverPages($pages); - } - - public function testCurrentSetsQueryIntegerLimits() - { - $pages = new EntityPageIterator( - function ($item) { - return $item; - }, - [$this, 'theCall'], - [ - 'test' => 'call', - 'query' => ['limit' => 1000], - ] - ); - - $pagesArray = iterator_to_array($pages); - - $this->assertOverPages($pages, 1000); - } - - public function testCurrentSetsQueryArrayLimits() - { - - $pages = new EntityPageIterator( - function ($item) { - return $item; - }, - [$this, 'theCall'], - [ - 'test' => 'call', - 'query' => ['limit' => ['value' => 1100]], - ] - ); - - $this->assertOverPages($pages, 1100); - } - - private function assertOverPages($pages, $expectedLimit = 0) - { - $pagesArray = iterator_to_array($pages); - - $reflection = new \ReflectionClass($pages); - $configProp = $reflection->getProperty('config'); - $configProp->setAccessible(true); - $configs = $configProp->getValue($pages); - - $this->assertIsArray($configs); - $this->assertArrayHasKey('resultLimit', $configs); - $this->assertEquals( - $expectedLimit, - $configs['resultLimit'], - "resultLimit assertion failed." - ); - $this->assertEquals(self::$moreResultsType, $pages->moreResultsType()); $this->assertEquals(self::$items, $pagesArray[0]); } @@ -106,9 +48,9 @@ public function theCall(array $options) { return [ 'batch' => [ - 'moreResults' => self::$moreResultsType, + 'moreResults' => self::$moreResultsType ], - 'items' => self::$items, - ] + $options; + 'items' => self::$items + ]; } } From 71ac41c2a0f036a143e4467984955d1265c56958 Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Wed, 14 Dec 2022 14:45:21 +0530 Subject: [PATCH 11/12] fix: removing resultLimit from Operation --- Datastore/src/Operation.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index a6ccf481c16..357286bb3ee 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -469,8 +469,6 @@ public function runQuery(QueryInterface $query, array $options = []) ]; if (array_key_exists('limit', $query->queryObject())) { - // Setting resultLimit ensures we loop through all pages. - $iteratorConfig['resultLimit'] = $query->queryObject()['limit']; $remainingLimit = $query->queryObject()['limit']; } $runQueryObj = clone $query; @@ -505,6 +503,12 @@ public function runQuery(QueryInterface $query, array $options = []) if (isset($res['query'])) { $runQueryObj = new Query($this->entityMapper, $res['query']); } + if (isset($res['query']['limit'])) { + $remainingLimit = $res['query']['limit']; + } + if (isset($remainingLimit['value'])) { + $remainingLimit = $remainingLimit['value']; + } if (isset($remainingLimit)) { $remainingLimit -= count($res['batch']['entityResults']); } From 31448e405ac2bffdd607e2609d4826e8edd6a81a Mon Sep 17 00:00:00 2001 From: Vishwaraj Anand Date: Thu, 15 Dec 2022 09:57:46 +0530 Subject: [PATCH 12/12] nit: migrated to is_null for var check --- Datastore/src/Operation.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Datastore/src/Operation.php b/Datastore/src/Operation.php index 357286bb3ee..ecaa344636c 100644 --- a/Datastore/src/Operation.php +++ b/Datastore/src/Operation.php @@ -504,12 +504,14 @@ public function runQuery(QueryInterface $query, array $options = []) $runQueryObj = new Query($this->entityMapper, $res['query']); } if (isset($res['query']['limit'])) { + // limit for GqlQuery in REST mode $remainingLimit = $res['query']['limit']; } if (isset($remainingLimit['value'])) { + // limit for GqlQuery in GRPC mode $remainingLimit = $remainingLimit['value']; } - if (isset($remainingLimit)) { + if (!is_null($remainingLimit)) { $remainingLimit -= count($res['batch']['entityResults']); }