Skip to content

Commit 51b22ec

Browse files
HectorhammettHectorhammett
authored
feat: update pagination rules for compute clients (#765)
1 parent 86f901a commit 51b22ec

24 files changed

+2033
-53
lines changed

‎composer.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@
2929
"Testing\\DisableSnippets\\": "tests/Unit/ProtoTests/DisableSnippets/out/src",
3030
"Testing\\GrpcServiceConfig\\": "tests/Unit/ProtoTests/GrpcServiceConfig/out/src",
3131
"Testing\\ResourceNames\\": "tests/Unit/ProtoTests/ResourceNames/out/src",
32-
"Testing\\RoutingHeaders\\": "tests/Unit/ProtoTests/RoutingHeaders/out/src"
32+
"Testing\\RoutingHeaders\\": "tests/Unit/ProtoTests/RoutingHeaders/out/src",
33+
"Testing\\DiregapicPaginated\\": "tests/Unit/ProtoTests/DiregapicPaginated/out/src"
3334
}
3435
},
3536
"require": {

‎src/Generation/MethodDetails.php‎

Lines changed: 67 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,9 @@ private static function maybeCreatePaginated(ServiceDetails $svc, MethodDescript
127127
$catalog = $svc->catalog;
128128
$inputMsg = $catalog->msgsByFullname[$desc->getInputType()];
129129
$outputMsg = $catalog->msgsByFullname[$desc->getOutputType()];
130-
$isRestOnly = $svc->transportType === Transport::REST;
130+
$isDireGapic = $svc->transportType === Transport::REST;
131131
$pageSize = $inputMsg->desc->getFieldByName('page_size');
132-
if ($isRestOnly && is_null($pageSize)) {
132+
if ($isDireGapic && is_null($pageSize)) {
133133
$pageSize = $inputMsg->desc->getFieldByName('max_results');
134134
}
135135

@@ -141,7 +141,7 @@ private static function maybeCreatePaginated(ServiceDetails $svc, MethodDescript
141141

142142
// If we have the type inside the ExplicitPagination class,
143143
// use the field stored in the paginations array
144-
if (ExplicitPagination::exists($outputMsg->desc->getFullName())) {
144+
if ($isDireGapic && ExplicitPagination::exists($outputMsg->desc->getFullName())) {
145145
$resources = $outputMsg->desc->getFieldByName(
146146
ExplicitPagination::getPagination($outputMsg->desc->getFullName())
147147
);
@@ -152,20 +152,8 @@ private static function maybeCreatePaginated(ServiceDetails $svc, MethodDescript
152152
->filter(fn ($x) => $x[1]->isRepeated());
153153
$resourceByNumber = $resourceCandidates->orderBy(fn ($x) => $x[0])->firstOrNull();
154154

155-
if ($isRestOnly) {
156-
$resourceListCandidates = $resourceCandidates->filter(fn ($x) => !ProtoHelpers::isMap($catalog, $x[1]));
157-
$resourceMapCandidates = $resourceCandidates->filter(fn ($x) => ProtoHelpers::isMap($catalog, $x[1]));
158-
159-
// If there are more than one of either, do not generate a paginated method.
160-
if (count($resourceListCandidates) > 1 || count($resourceMapCandidates) > 1) {
161-
return null;
162-
}
163-
164-
// A map field takes precedence over a repeated (i.e. list) field.
165-
$resourceByNumber = $resourceMapCandidates->orderBy(fn ($x) => $x[0])->firstOrNull();
166-
if (is_null($resourceByNumber)) {
167-
$resourceByNumber = $resourceListCandidates->orderBy(fn ($x) => $x[0])->firstOrNull();
168-
}
155+
if ($isDireGapic) {
156+
$resourceByNumber = self::getCandidate($resourceCandidates, $catalog);
169157
}
170158

171159
$resourceByPosition = $resourceCandidates->firstOrNull();
@@ -176,7 +164,7 @@ private static function maybeCreatePaginated(ServiceDetails $svc, MethodDescript
176164
$resourceFieldValid = !is_null($resources);
177165

178166
// Leverage short-circuting.
179-
if ($resourceFieldValid && !$isRestOnly) {
167+
if ($resourceFieldValid && !$isDireGapic) {
180168
$resourceFieldValid &= !ProtoHelpers::isMap($catalog, $resources)
181169
&& $resourceByNumber[0] === $resourceByPosition[0];
182170
}
@@ -186,13 +174,13 @@ private static function maybeCreatePaginated(ServiceDetails $svc, MethodDescript
186174
}
187175

188176
$isValidPageSize = !$pageSize->isRepeated();
189-
if ($isRestOnly) {
177+
if ($isDireGapic) {
190178
$isValidPageSize = $pageSize->getType() === GPBType::UINT32 || $pageSize->getType() === GPBType::INT32;
191179
} else {
192180
$isValidPageSize = $pageSize->getType() === GPBType::INT32;
193181
}
194182
if (!$isValidPageSize) {
195-
throw new \Exception("page_size field must be of type " . ($isRestOnly ? "uint32 or int32" : "int32") . ".");
183+
throw new \Exception("page_size field must be of type " . ($isDireGapic ? "uint32 or int32" : "int32") . ".");
196184
}
197185
if ($pageToken->isRepeated() || $pageToken->getType() !== GPBType::STRING) {
198186
throw new \Exception("page_token field must be of type string.");
@@ -201,7 +189,7 @@ private static function maybeCreatePaginated(ServiceDetails $svc, MethodDescript
201189
throw new \Exception("next_page_token field must be of type string.");
202190
}
203191
if (!$resourceFieldValid) {
204-
if ($isRestOnly) {
192+
if ($isDireGapic) {
205193
throw new \Exception("Item resources field must a map or repeated field.");
206194
}
207195
throw new \Exception("Item resources field must be the first repeated field by number and position.");
@@ -448,6 +436,64 @@ public function __construct($svc, $desc)
448436
};
449437
}
450438

439+
/**
440+
* Determine if the method should paginated for DIREGAPICs, which do not have annotations for pagination
441+
* yet. The following heuristic is used for selecting the field for pagination:
442+
* 1. If exactly one map field exists, select that field
443+
* 2. If exactly one list field exists, select that field
444+
* 3. If more than one list field exists, but exactly one of those fields is a message, select that field
445+
* 4. Otherwise, do not paginate
446+
*/
447+
private static function getCandidate(Vector $resourceCandidates, ProtoCatalog $catalog): null|array
448+
{
449+
$resourceListCandidates = $resourceCandidates->filter(fn ($x) => !ProtoHelpers::isMap($catalog, $x[1]));
450+
$resourceMapCandidates = $resourceCandidates->filter(fn ($x) => ProtoHelpers::isMap($catalog, $x[1]));
451+
452+
// If only one map exists, return it.
453+
if (count($resourceMapCandidates) === 1) {
454+
return $resourceMapCandidates->firstOrNull();
455+
}
456+
457+
// If only one list exists, return it.
458+
if (count($resourceListCandidates) === 1) {
459+
return $resourceListCandidates->firstOrNull();
460+
}
461+
462+
// If there are more than one repeated field,
463+
// search for a non primitive one.
464+
if (count($resourceListCandidates) > 1) {
465+
$resourceCandidateIndex = null;
466+
467+
foreach ($resourceListCandidates as $index => $candidate) {
468+
$descriptor = $candidate[1];
469+
470+
// If is a primitive type, we cannot use it for pagination.
471+
// Search for another one.
472+
if ($descriptor->getType() !== GPBType::MESSAGE) {
473+
continue;
474+
}
475+
476+
// If we already find a non primitive type
477+
// then we have more than one.
478+
// unable to paginate.
479+
if (!is_null($resourceCandidateIndex)) {
480+
return null;
481+
}
482+
483+
$resourceCandidateIndex = $index;
484+
}
485+
486+
if (is_null($resourceCandidateIndex)) {
487+
return null;
488+
}
489+
490+
return $resourceListCandidates[$resourceCandidateIndex];
491+
}
492+
493+
// We cannot determine what is the pagination.
494+
return null;
495+
}
496+
451497
/** @var ServiceDetails The service that contains this method. */
452498
public ServiceDetails $serviceDetails;
453499

‎tests/Unit/ProtoTests/BasicExplicitPaginated/out/src/resources/basic_explicit_paginated_descriptor_config.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
'requestPageSizeGetMethod' => 'getPageSize',
3131
'requestPageSizeSetMethod' => 'setPageSize',
3232
'responsePageTokenGetMethod' => 'getNextPageToken',
33-
'resourcesGetMethod' => 'getTheRealResults',
33+
'resourcesGetMethod' => 'getTheResults',
3434
],
3535
'callType' => \Google\ApiCore\Call::PAGINATED_CALL,
3636
'responseType' => 'Testing\BasicExplicitPaginated\ExplicitResponse',

‎tests/Unit/ProtoTests/BasicExplicitPaginated/out/tests/Unit/BasicExplicitPaginatedClientTest.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -73,17 +73,17 @@ public function methodExplicitPaginatedTest()
7373
$pageToken2 = 649316932;
7474
$aField2 = false;
7575
$anotherField = 'anotherField1551924414';
76-
$theRealResultsElement = 'theRealResultsElement-1510860256';
77-
$theRealResults = [
78-
$theRealResultsElement,
76+
$theResultsElement = 'theResultsElement-1546403867';
77+
$theResults = [
78+
$theResultsElement,
7979
];
8080
$expectedResponse = new ExplicitResponse();
8181
$expectedResponse->setPageSize($pageSize2);
8282
$expectedResponse->setNextPageToken($nextPageToken);
8383
$expectedResponse->setPageToken($pageToken2);
8484
$expectedResponse->setAField($aField2);
8585
$expectedResponse->setAnotherField($anotherField);
86-
$expectedResponse->setTheRealResults($theRealResults);
86+
$expectedResponse->setTheResults($theResults);
8787
$transport->addResponse($expectedResponse);
8888
// Mock request
8989
$aField = 'aField-1289259108';
@@ -93,7 +93,7 @@ public function methodExplicitPaginatedTest()
9393
$this->assertEquals($expectedResponse, $response->getPage()->getResponseObject());
9494
$resources = iterator_to_array($response->iterateAllElements());
9595
$this->assertSame(1, count($resources));
96-
$this->assertEquals($expectedResponse->getTheRealResults()[0], $resources[0]);
96+
$this->assertEquals($expectedResponse->getTheResults()[0], $resources[0]);
9797
$actualRequests = $transport->popReceivedCalls();
9898
$this->assertSame(1, count($actualRequests));
9999
$actualFuncCall = $actualRequests[0]->getFuncCall();

‎tests/Unit/ProtoTests/BasicExplicitPaginated/out/tests/Unit/Client/BasicExplicitPaginatedClientTest.php‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -74,17 +74,17 @@ public function methodExplicitPaginatedTest()
7474
$pageToken2 = 649316932;
7575
$aField2 = false;
7676
$anotherField = 'anotherField1551924414';
77-
$theRealResultsElement = 'theRealResultsElement-1510860256';
78-
$theRealResults = [
79-
$theRealResultsElement,
77+
$theResultsElement = 'theResultsElement-1546403867';
78+
$theResults = [
79+
$theResultsElement,
8080
];
8181
$expectedResponse = new ExplicitResponse();
8282
$expectedResponse->setPageSize($pageSize2);
8383
$expectedResponse->setNextPageToken($nextPageToken);
8484
$expectedResponse->setPageToken($pageToken2);
8585
$expectedResponse->setAField($aField2);
8686
$expectedResponse->setAnotherField($anotherField);
87-
$expectedResponse->setTheRealResults($theRealResults);
87+
$expectedResponse->setTheResults($theResults);
8888
$transport->addResponse($expectedResponse);
8989
// Mock request
9090
$aField = 'aField-1289259108';
@@ -98,7 +98,7 @@ public function methodExplicitPaginatedTest()
9898
$this->assertEquals($expectedResponse, $response->getPage()->getResponseObject());
9999
$resources = iterator_to_array($response->iterateAllElements());
100100
$this->assertSame(1, count($resources));
101-
$this->assertEquals($expectedResponse->getTheRealResults()[0], $resources[0]);
101+
$this->assertEquals($expectedResponse->getTheResults()[0], $resources[0]);
102102
$actualRequests = $transport->popReceivedCalls();
103103
$this->assertSame(1, count($actualRequests));
104104
$actualFuncCall = $actualRequests[0]->getFuncCall();
@@ -166,17 +166,17 @@ public function methodExplicitPaginatedAsyncTest()
166166
$pageToken2 = 649316932;
167167
$aField2 = false;
168168
$anotherField = 'anotherField1551924414';
169-
$theRealResultsElement = 'theRealResultsElement-1510860256';
170-
$theRealResults = [
171-
$theRealResultsElement,
169+
$theResultsElement = 'theResultsElement-1546403867';
170+
$theResults = [
171+
$theResultsElement,
172172
];
173173
$expectedResponse = new ExplicitResponse();
174174
$expectedResponse->setPageSize($pageSize2);
175175
$expectedResponse->setNextPageToken($nextPageToken);
176176
$expectedResponse->setPageToken($pageToken2);
177177
$expectedResponse->setAField($aField2);
178178
$expectedResponse->setAnotherField($anotherField);
179-
$expectedResponse->setTheRealResults($theRealResults);
179+
$expectedResponse->setTheResults($theResults);
180180
$transport->addResponse($expectedResponse);
181181
// Mock request
182182
$aField = 'aField-1289259108';
@@ -190,7 +190,7 @@ public function methodExplicitPaginatedAsyncTest()
190190
$this->assertEquals($expectedResponse, $response->getPage()->getResponseObject());
191191
$resources = iterator_to_array($response->iterateAllElements());
192192
$this->assertSame(1, count($resources));
193-
$this->assertEquals($expectedResponse->getTheRealResults()[0], $resources[0]);
193+
$this->assertEquals($expectedResponse->getTheResults()[0], $resources[0]);
194194
$actualRequests = $transport->popReceivedCalls();
195195
$this->assertSame(1, count($actualRequests));
196196
$actualFuncCall = $actualRequests[0]->getFuncCall();

‎tests/Unit/ProtoTests/DiregapicPaginated/diregapic-paginated.proto‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
syntax = "proto3";
2+
3+
package testing.diregapicpaginated;
4+
5+
option php_namespace = "Testing\\DiregapicPaginated";
6+
7+
import "google/api/annotations.proto";
8+
import "google/api/client.proto";
9+
import "google/api/field_behavior.proto";
10+
11+
/**
12+
* This test proto is built to test the following heuristics for pagination:
13+
* 1. If exactly one map field exists, select that field
14+
* 2. If exactly one list field exists, select that field
15+
* 3. If more than one list field exists, but exactly one of those fields is a message, select that field
16+
* 4. Otherwise, do not paginate
17+
*/
18+
service HeuristicPaginationClient {
19+
option (google.api.default_host) = "diregapic.example.com";
20+
option (google.api.oauth_scopes) = "scope1,scope2";
21+
22+
// Tests heuristic #1
23+
rpc MapMethod(Request) returns(MapResponse) {
24+
option (google.api.http) = {
25+
post: "/path:methodPaginated"
26+
body: "*"
27+
};
28+
}
29+
30+
// Tests heuristic 2
31+
rpc ListMethod(Request) returns(listResponse) {
32+
option (google.api.http) = {
33+
post: "/path:methodPaginated"
34+
body: "*"
35+
};
36+
}
37+
38+
// Tests heuristic 3
39+
rpc MultipleListMethod(Request) returns(MultipleListResponse) {
40+
option (google.api.http) = {
41+
post: "/path:methodPaginated"
42+
body: "*"
43+
};
44+
}
45+
46+
// Tests Heuristic 4
47+
rpc NonPaginatedMethod(Request) returns(NonPaginatedResponse) {
48+
option (google.api.http) = {
49+
post: "/path:methodPaginated"
50+
body: "*"
51+
};
52+
}
53+
}
54+
55+
message Request {
56+
int32 max_results = 1;
57+
string page_token = 2;
58+
}
59+
60+
message MapResponse {
61+
map<int32, string> results = 1;
62+
string next_page_token = 2;
63+
}
64+
65+
message listResponse {
66+
repeated string results = 1;
67+
string next_page_token = 2;
68+
}
69+
70+
message MultipleListResponse {
71+
repeated string not_paginated = 1;
72+
repeated string also_not_paginated = 2;
73+
repeated string also_also_not_paginated = 3;
74+
repeated RepeatedResource results = 4;
75+
string next_page_token = 5;
76+
}
77+
78+
message NonPaginatedResponse {
79+
repeated string not_paginated = 1;
80+
repeated string also_not_paginated = 2;
81+
repeated string also_also_not_paginated = 3;
82+
string next_page_token = 4;
83+
}
84+
85+
message RepeatedResource {
86+
string name = 1;
87+
}

‎tests/Unit/ProtoTests/DiregapicPaginated/diregapic-paginated_gapic.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
type: com.google.api.codegen.ConfigProto
2+
config_schema_version: 2.0.0
3+
language_settings:
4+
php:
5+
package_name: Testing\DiregapicPaginated

‎tests/Unit/ProtoTests/DiregapicPaginated/diregapic-paginated_service.yaml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
type: google.api.Service
2+
config_version: 3
3+
4+
authentication:
5+
rules:
6+
- selector: 'testing.diregapic.BasicPaginated.*'
7+
oauth:
8+
canonical_scopes: |-
9+
scope1,
10+
scope2

0 commit comments

Comments
 (0)