From a2a88e35510a65769b97e9a53f619b010965eee4 Mon Sep 17 00:00:00 2001 From: Alexander Fenster Date: Thu, 21 Nov 2019 19:42:24 -0800 Subject: [PATCH] fix: correctly determine auto-paginated field (#156) * fix: correctly determine auto-paginated field * fix: gts fix * fix: fail on error --- typescript/src/schema/proto.ts | 29 ++- .../v1beta1/cloud_redis_client.ts.baseline | 227 ++++++++++++------ .../gapic-cloud_redis-v1beta1.ts.baseline | 92 +++---- 3 files changed, 221 insertions(+), 127 deletions(-) diff --git a/typescript/src/schema/proto.ts b/typescript/src/schema/proto.ts index f1fd71946..486977097 100644 --- a/typescript/src/schema/proto.ts +++ b/typescript/src/schema/proto.ts @@ -287,9 +287,36 @@ 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( + `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') + ); + // TODO: an option to ignore errors + throw new Error(`Bad pagination settings for ${method.name}`); + } return repeatedFields[0]; } diff --git a/typescript/test/testdata/redis/src/v1beta1/cloud_redis_client.ts.baseline b/typescript/test/testdata/redis/src/v1beta1/cloud_redis_client.ts.baseline index 73b2470a3..6a73cfa0d 100644 --- a/typescript/test/testdata/redis/src/v1beta1/cloud_redis_client.ts.baseline +++ b/typescript/test/testdata/redis/src/v1beta1/cloud_redis_client.ts.baseline @@ -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'; @@ -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. @@ -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): @@ -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 -- // -------------------- diff --git a/typescript/test/testdata/redis/test/gapic-cloud_redis-v1beta1.ts.baseline b/typescript/test/testdata/redis/test/gapic-cloud_redis-v1beta1.ts.baseline index 187022e39..47e1932a8 100644 --- a/typescript/test/testdata/redis/test/gapic-cloud_redis-v1beta1.ts.baseline +++ b/typescript/test/testdata/redis/test/gapic-cloud_redis-v1beta1.ts.baseline @@ -95,52 +95,6 @@ describe('v1beta1.CloudRedisClient', () => { }); assert(client); }); - describe('listInstances', () => { - it('invokes listInstances without error', done => { - const client = new cloudredisModule.v1beta1.CloudRedisClient({ - credentials: {client_email: 'bogus', private_key: 'bogus'}, - projectId: 'bogus', - }); - // Mock request - const request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest = {}; - // Mock response - const expectedResponse = {}; - // Mock gRPC layer - client._innerApiCalls.listInstances = mockSimpleGrpcMethod( - request, - expectedResponse, - null - ); - client.listInstances(request, (err: {}, response: {}) => { - assert.ifError(err); - assert.deepStrictEqual(response, expectedResponse); - done(); - }) - }); - - it('invokes listInstances with error', done => { - const client = new cloudredisModule.v1beta1.CloudRedisClient({ - credentials: {client_email: 'bogus', private_key: 'bogus'}, - projectId: 'bogus', - }); - // Mock request - const request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest = {}; - // Mock response - const expectedResponse = {}; - // Mock gRPC layer - client._innerApiCalls.listInstances = mockSimpleGrpcMethod( - request, - null, - error - ); - client.listInstances(request, (err: FakeError, response: {}) => { - assert(err instanceof FakeError); - assert.strictEqual(err.code, FAKE_STATUS_CODE); - assert(typeof response === 'undefined'); - done(); - }) - }); - }); describe('getInstance', () => { it('invokes getInstance without error', done => { const client = new cloudredisModule.v1beta1.CloudRedisClient({ @@ -509,4 +463,50 @@ describe('v1beta1.CloudRedisClient', () => { }); }); }); + describe('listInstances', () => { + it('invokes listInstances without error', done => { + const client = new cloudredisModule.v1beta1.CloudRedisClient({ + credentials: {client_email: 'bogus', private_key: 'bogus'}, + projectId: 'bogus', + }); + // Mock request + const request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest = {}; + // Mock response + const expectedResponse = {}; + // Mock Grpc layer + client._innerApiCalls.listInstances = (actualRequest: {}, options: {}, callback: Callback) => { + assert.deepStrictEqual(actualRequest, request); + callback(null, expectedResponse); + }; + client.listInstances(request, (err: FakeError, response: {}) => { + assert.ifError(err); + assert.deepStrictEqual(response, expectedResponse); + done(); + }); + }); + }); + describe('listInstancesStream', () => { + it('invokes listInstancesStream without error', done => { + const client = new cloudredisModule.v1beta1.CloudRedisClient({ + credentials: {client_email: 'bogus', private_key: 'bogus'}, + projectId: 'bogus', + }); + // Mock request + const request: protosTypes.google.cloud.redis.v1beta1.IListInstancesRequest = {}; + // Mock response + const expectedResponse = {}; + // Mock Grpc layer + client._innerApiCalls.listInstances = (actualRequest: {}, options: {}, callback: Callback) => { + assert.deepStrictEqual(actualRequest, request); + callback(null, expectedResponse); + }; + const stream = client.listInstancesStream(request, {}).on('data', (response: {}) =>{ + assert.deepStrictEqual(response, expectedResponse); + done(); + }).on('error', (err: FakeError) => { + done(err); + }); + stream.write(request); + }); + }); });