Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correctly determine auto-paginated field #156

Merged
merged 4 commits into from
Nov 22, 2019
Merged

fix: correctly determine auto-paginated field #156

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
987c2d5
fix: correctly determine auto-paginated field
alexander-fenster Nov 21, 2019
709a8b2
fix: gts fix
alexander-fenster Nov 21, 2019
e51fca8
Merge branch 'master' into pagination
alexander-fenster Nov 22, 2019
28a7a5d
fix: fail on error
alexander-fenster Nov 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 30 additions & 12 deletions typescript/src/schema/proto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,30 +294,48 @@ function pagingField(messages: MessagesMap, method: MethodDescriptorProto) {
field.label ===
plugin.google.protobuf.FieldDescriptorProto.Label.LABEL_REPEATED
);
if (repeatedFields.length !== 1) {
if (repeatedFields.length === 0) {
return undefined;
}
if (repeatedFields.length === 1) {
return repeatedFields[0];
}
// According to https://aip.dev/client-libraries/4233, if there are two
// or more repeated fields in the output, we must take the the first one
// (in order of appearance in the file AND field number).
// We believe that all proto fields have numbers, hence !.
const minFieldNumber = repeatedFields.reduce(
(min: number, field: plugin.google.protobuf.IFieldDescriptorProto) => {
if (field.number! < min) {
min = field.number!;
}
return min;
},
repeatedFields[0].number!
);
if (minFieldNumber !== repeatedFields[0].number) {
console.warn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So warnings like this always give me a moment's pause. Chances are this will be run by a robot most of the time, and a human will often never see the output. Realistically - do we think this merits an error, and human attention? Or are we ok with the warning getting ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this for a while. If the API is broken in this specific way, we cannot fix it (AIPs and annotations have no way of saying "forget it, it's not auto-paginated"). AIP actually suggests to fail in this case, but then we just cannot generate this API at all, end of story.

How about this: we'll fail by default and will proceed with a command line option?

$ gapic-generator-typescript ... foo.proto bar.proto
Error 42: blablabla
(non-zero exit code)

$ gapic-generator-typescript --ignore-errors=42 ... foo.proto bar.proto
Warning 42: blablabla
(library generated on a best effort basis, zero exit code)

If you're OK with this approach, I'll first implement it (in a separate PR) and then will apply here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the surface, this sounds correct. If we know an API will be generated poorly, I'd rather us not generate at all, and harass the partner team to fix the proto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! Failing now, also #158 to make it possible to ignore the error.

`Warning: method ${method.name} has several repeated fields in the output type and violates https://aip.dev/client-libraries/4233 for auto-pagination. Disabling auto-pagination for this method.`
);
console.warn('Fields considered for pagination:');
console.warn(
repeatedFields.map(field => `${field.name} = ${field.number}`).join('\n')
);
}
return repeatedFields[0];
}

function pagingFieldName(messages: MessagesMap, method: MethodDescriptorProto) {
const repeatedFields = pagingField(messages, method);
if (repeatedFields && repeatedFields.name) {
return repeatedFields.name;
} else {
return undefined;
}
const field = pagingField(messages, method);
return field?.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see you living your best life here

}

function pagingResponseType(
messages: MessagesMap,
method: MethodDescriptorProto
) {
const repeatedFields = pagingField(messages, method);
if (repeatedFields && repeatedFields.typeName) {
return repeatedFields.typeName; //.google.showcase.v1beta1.EchoResponse
}
return undefined;
const field = pagingField(messages, method);
return field?.typeName; //.google.showcase.v1beta1.EchoResponse
}

export function getHeaderParams(rule: plugin.google.api.IHttpRule): string[] {
Expand Down
227 changes: 147 additions & 80 deletions typescript/test/testdata/redis/src/v1beta1/cloud_redis_client.ts.baseline
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
// ** All changes to this file may be overwritten. **

import * as gax from 'google-gax';
import {APICallback, Callback, CallOptions, Descriptors, ClientOptions, LROperation} from 'google-gax';
import {APICallback, Callback, CallOptions, Descriptors, ClientOptions, LROperation, PaginationCallback, PaginationResponse} from 'google-gax';
import * as path from 'path';

import { Transform } from 'stream';
import * as protosTypes from '../../protos/protos';
import * as gapicConfig from './cloud_redis_client_config.json';

Expand Down Expand Up @@ -152,6 +153,14 @@ export class CloudRedisClient {
),
};

// Some of the methods on this service return "paged" results,
// (e.g. 50 results at a time, with tokens to get subsequent
// pages). Denote the keys used for pagination and results.
this._descriptors.page = {
listInstances:
new gaxModule.PageDescriptor('pageToken', 'nextPageToken', 'instances')
};

// This API contains "long-running operations", which return a
// an Operation object that allows for tracking of the operation,
// rather than holding a request open.
Expand Down Expand Up @@ -320,85 +329,6 @@ export class CloudRedisClient {
// -------------------
// -- Service calls --
// -------------------
listInstances(
request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest,
options?: gax.CallOptions):
Promise<[
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse,
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|undefined, {}|undefined
]>;
listInstances(
request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest,
options: gax.CallOptions,
callback: Callback<
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse,
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|undefined,
{}|undefined>): void;
/**
* Lists all Redis instances owned by a project in either the specified
* location (region) or all locations.
*
* The location should have the following format:
* * `projects/{project_id}/locations/{location_id}`
*
* If `location_id` is specified as `-` (wildcard), then all regions
* available to the project are queried, and the results are aggregated.
*
* @param {Object} request
* The request object that will be sent.
* @param {string} request.parent
* Required. The resource name of the instance location using the form:
* `projects/{project_id}/locations/{location_id}`
* where `location_id` refers to a GCP region.
* @param {number} request.pageSize
* The maximum number of items to return.
*
* If not specified, a default value of 1000 will be used by the service.
* Regardless of the page_size value, the response may include a partial list
* and a caller should only rely on response's
* [next_page_token][CloudRedis.ListInstancesResponse.next_page_token]
* to determine if there are more instances left to be queried.
* @param {string} request.pageToken
* The next_page_token value returned from a previous List request,
* if any.
* @param {object} [options]
* Call options. See {@link https://googleapis.dev/nodejs/google-gax/latest/interfaces/CallOptions.html|CallOptions} for more details.
* @returns {Promise} - The promise which resolves to an array.
* The first element of the array is an object representing [ListInstancesResponse]{@link google.cloud.redis.v1beta1.ListInstancesResponse}.
* The promise has a method named "cancel" which cancels the ongoing API call.
*/
listInstances(
request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest,
optionsOrCallback?: gax.CallOptions|Callback<
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse,
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|undefined, {}|undefined>,
callback?: Callback<
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse,
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|undefined,
{}|undefined>):
Promise<[
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse,
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|undefined, {}|undefined
]>|void {
request = request || {};
let options: gax.CallOptions;
if (typeof optionsOrCallback === 'function' && callback === undefined) {
callback = optionsOrCallback;
options = {};
}
else {
options = optionsOrCallback as gax.CallOptions;
}
options = options || {};
options.otherArgs = options.otherArgs || {};
options.otherArgs.headers = options.otherArgs.headers || {};
options.otherArgs.headers[
'x-goog-request-params'
] = gax.routingHeader.fromParams({
'parent': request.parent || '',
});
return this._innerApiCalls.listInstances(request, options, callback);
}
getInstance(
request: protosTypes.google.cloud.redis.v1beta1.IGetInstanceRequest,
options?: gax.CallOptions):
Expand Down Expand Up @@ -882,6 +812,143 @@ export class CloudRedisClient {
});
return this._innerApiCalls.deleteInstance(request, options, callback);
}
listInstances(
request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest,
options?: gax.CallOptions):
Promise<[
protosTypes.google.cloud.redis.v1beta1.IInstance[],
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|null,
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse
]>;
listInstances(
request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest,
options: gax.CallOptions,
callback: Callback<
protosTypes.google.cloud.redis.v1beta1.IInstance[],
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|null,
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse>): void;
/**
* Lists all Redis instances owned by a project in either the specified
* location (region) or all locations.
*
* The location should have the following format:
* * `projects/{project_id}/locations/{location_id}`
*
* If `location_id` is specified as `-` (wildcard), then all regions
* available to the project are queried, and the results are aggregated.
*
* @param {Object} request
* The request object that will be sent.
* @param {string} request.parent
* Required. The resource name of the instance location using the form:
* `projects/{project_id}/locations/{location_id}`
* where `location_id` refers to a GCP region.
* @param {number} request.pageSize
* The maximum number of items to return.
*
* If not specified, a default value of 1000 will be used by the service.
* Regardless of the page_size value, the response may include a partial list
* and a caller should only rely on response's
* [next_page_token][CloudRedis.ListInstancesResponse.next_page_token]
* to determine if there are more instances left to be queried.
* @param {string} request.pageToken
* The next_page_token value returned from a previous List request,
* if any.
* @param {object} [options]
* Call options. See {@link https://googleapis.dev/nodejs/google-gax/latest/interfaces/CallOptions.html|CallOptions} for more details.
* @returns {Promise} - The promise which resolves to an array.
* The first element of the array is an object representing [ListInstancesResponse]{@link google.cloud.redis.v1beta1.ListInstancesResponse}.
*
* When autoPaginate: false is specified through options, the array has three elements.
* The first element is Array of [ListInstancesResponse]{@link google.cloud.redis.v1beta1.ListInstancesResponse} in a single response.
* The second element is the next request object if the response
* indicates the next page exists, or null. The third element is
* an object representing [ListInstancesResponse]{@link google.cloud.redis.v1beta1.ListInstancesResponse}.
*
* The promise has a method named "cancel" which cancels the ongoing API call.
*/
listInstances(
request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest,
optionsOrCallback?: gax.CallOptions|Callback<
protosTypes.google.cloud.redis.v1beta1.IInstance[],
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|null,
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse>,
callback?: Callback<
protosTypes.google.cloud.redis.v1beta1.IInstance[],
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|null,
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse>):
Promise<[
protosTypes.google.cloud.redis.v1beta1.IInstance[],
protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest|null,
protosTypes.google.cloud.redis.v1beta1.IListInstancesResponse
]>|void {
request = request || {};
let options: gax.CallOptions;
if (typeof optionsOrCallback === 'function' && callback === undefined) {
callback = optionsOrCallback;
options = {};
}
else {
options = optionsOrCallback as gax.CallOptions;
}
options = options || {};
options.otherArgs = options.otherArgs || {};
options.otherArgs.headers = options.otherArgs.headers || {};
options.otherArgs.headers[
'x-goog-request-params'
] = gax.routingHeader.fromParams({
'parent': request.parent || '',
});
return this._innerApiCalls.listInstances(request, options, callback);
}

/**
* Equivalent to {@link listInstances}, but returns a NodeJS Stream object.
*
* This fetches the paged responses for {@link listInstances} continuously
* and invokes the callback registered for 'data' event for each element in the
* responses.
*
* The returned object has 'end' method when no more elements are required.
*
* autoPaginate option will be ignored.
*
* @see {@link https://nodejs.org/api/stream.html}
*
* @param {Object} request
* The request object that will be sent.
* @param {string} request.parent
* Required. The resource name of the instance location using the form:
* `projects/{project_id}/locations/{location_id}`
* where `location_id` refers to a GCP region.
* @param {number} request.pageSize
* The maximum number of items to return.
*
* If not specified, a default value of 1000 will be used by the service.
* Regardless of the page_size value, the response may include a partial list
* and a caller should only rely on response's
* [next_page_token][CloudRedis.ListInstancesResponse.next_page_token]
* to determine if there are more instances left to be queried.
* @param {string} request.pageToken
* The next_page_token value returned from a previous List request,
* if any.
* @param {object} [options]
* Call options. See {@link https://googleapis.dev/nodejs/google-gax/latest/interfaces/CallOptions.html|CallOptions} for more details.
* @returns {Stream}
* An object stream which emits an object representing [Instance]{@link google.cloud.redis.v1beta1.Instance} on 'data' event.
*/
listInstancesStream(
request?: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest,
options?: gax.CallOptions | {}):
Transform{
request = request || {};
const callSettings = new gax.CallSettings(options);
return this._descriptors.page.listInstances.createStream(
this._innerApiCalls.listInstances as gax.GaxCall,
request,
callSettings
);
}
// --------------------
// -- Path templates --
// --------------------
Expand Down
Loading