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.

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

feat: add helpful warnings on paged calls #1668

Merged
merged 5 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions gax/src/paginationCalls/pageDescriptor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
import {Descriptor} from '../descriptor';
import {CallSettings} from '../gax';
import {NormalApiCaller} from '../normalCalls/normalApiCaller';
import {warn} from '.././warnings';

import {PagedApiCaller} from './pagedApiCaller';

Expand Down Expand Up @@ -63,6 +64,13 @@ export class PageDescriptor implements Descriptor {
request: {},
options: CallSettings
): Transform {
if (options?.autoPaginate) {
warn(
'autoPaginate true',
'Autopaginate will always be set to false in stream paging methods. See more info at https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md#auto-pagination for more information on how to configure paging calls',
'AutopaginateTrueWarning'
);
}
const stream = new PassThrough({objectMode: true});
options = Object.assign({}, options, {autoPaginate: false});
const maxResults = 'maxResults' in options ? options.maxResults : -1;
Expand Down Expand Up @@ -137,6 +145,13 @@ export class PageDescriptor implements Descriptor {
request: RequestType,
options?: CallSettings
): AsyncIterable<{} | undefined> {
if (options?.autoPaginate) {
warn(
'autoPaginate true',
'Autopaginate will always be set to false in Async paging methods. See more info at https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md#auto-pagination for more information on how to configure paging calls',
'AutopaginateTrueWarning'
);
}
options = Object.assign({}, options, {autoPaginate: false});
const iterable = this.createIterator(apiCall, request, options);
return iterable;
Expand Down
10 changes: 10 additions & 0 deletions gax/src/paginationCalls/pagedApiCaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {CallOptions} from '../gax';
import {GoogleError} from '../googleError';
import {PageDescriptor} from './pageDescriptor';
import {ResourceCollector} from './resourceCollector';
import {warn} from '.././warnings';

export class PagedApiCaller implements APICaller {
pageDescriptor: PageDescriptor;
Expand Down Expand Up @@ -152,6 +153,15 @@ export class PagedApiCaller implements APICaller {
ongoingCall.call(apiCall, request);
return;
}
console.log('155', request, settings);
if (request.pageSize && settings.autoPaginate) {
warn(
'autoPaginate true',
'Providing a pageSize without setting autoPaginate to false will still return all results. See https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md#auto-pagination for more information on how to configure manual paging',
'AutopaginateTrueWarning'
);
}
console.log(request.pageSize && settings.autoPaginate);

const maxResults = settings.maxResults || -1;

Expand Down
29 changes: 29 additions & 0 deletions gax/test/test-application/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ async function testShowcase() {
await testExpand(grpcClient);
await testPagedExpand(grpcClient);
await testPagedExpandAsync(grpcClient);
await testPagedExpandAutopaginateOff(grpcClient);
await testCollect(grpcClient);
await testChat(grpcClient);
await testWait(grpcClient);
Expand All @@ -101,6 +102,7 @@ async function testShowcase() {
await testExpand(restClient); // REGAPIC supports server streaming
await testPagedExpand(restClient);
await testPagedExpandAsync(restClient);
await testPagedExpandAutopaginateOff(restClient);
await testCollectThrows(restClient); // REGAPIC does not support client streaming
await testChatThrows(restClient); // REGAPIC does not support bidi streaming
await testWait(restClient);
Expand All @@ -109,6 +111,7 @@ async function testShowcase() {
await testExpand(restClientCompat); // REGAPIC supports server streaming
await testPagedExpand(restClientCompat);
await testPagedExpandAsync(restClientCompat);
await testPagedExpandAutopaginateOff(restClientCompat);
await testCollectThrows(restClientCompat); // REGAPIC does not support client streaming
await testChatThrows(restClientCompat); // REGAPIC does not support bidi streaming
await testWait(restClientCompat);
Expand Down Expand Up @@ -167,6 +170,7 @@ async function testShowcase() {
await testExpand(grpcClientWithServerStreamingRetries);
await testPagedExpand(grpcClientWithServerStreamingRetries);
await testPagedExpandAsync(grpcClientWithServerStreamingRetries);
await testPagedExpandAutopaginateOff(grpcClientWithServerStreamingRetries);
await testCollect(grpcClientWithServerStreamingRetries);
await testChat(grpcClientWithServerStreamingRetries);
await testWait(grpcClientWithServerStreamingRetries);
Expand Down Expand Up @@ -526,6 +530,31 @@ async function testPagedExpandAsync(client: EchoClient) {
assert.deepStrictEqual(words, response);
}

async function testPagedExpandAutopaginateOff(client: EchoClient) {
const words = ['nobody', 'ever', 'reads', 'test', 'input'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I did! :)

const request = {
content: words.join(' '),
pageSize: 2,
};
const timer = setTimeout(() => {
throw new Error('End-to-end testPagedExpand method fails with timeout');
}, 12000);
const [resultArray, nextPageRequest] = await client.pagedExpand(request, {
autoPaginate: false,
});
clearTimeout(timer);
const result = resultArray.map(r => r.content);
assert.deepStrictEqual(words.slice(0, 2), result);
// manually paginate
const [response2] = await client.pagedExpand(nextPageRequest!, {
autoPaginate: false,
});

clearTimeout(timer);
const result2 = response2.map(r => r.content);
assert.deepStrictEqual(words.slice(2, 4), result2);
}

async function testCollect(client: EchoClient) {
const words = ['nobody', 'ever', 'reads', 'test', 'input'];
const promise = new Promise<string>((resolve, reject) => {
Expand Down
88 changes: 87 additions & 1 deletion gax/test/unit/pagedIteration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {describe, it, beforeEach} from 'mocha';
import * as util from './utils';
import {Stream} from 'stream';
import * as gax from '../../src/gax';
import * as warnings from '../../src/warnings';

describe('paged iteration', () => {
const pageSize = 3;
Expand Down Expand Up @@ -58,6 +59,31 @@ describe('paged iteration', () => {
}
}

it('warns when pageSize is configured without configuring autoPaginate', done => {
const warnStub = sinon.stub(warnings, 'warn');

const apiCall = util.createApiCall(func, createOptions);
const expected: Array<{}> = [];
for (let i = 0; i < pageSize * pagesToStream; ++i) {
expected.push(i);
}
apiCall({pageSize: pageSize}, undefined)
.then(results => {
assert.ok(Array.isArray(results));
assert.deepStrictEqual(results[0], expected);
assert.strictEqual(warnStub.callCount, 1);
assert(
warnStub.calledWith(
'autoPaginate true',
'Providing a pageSize without setting autoPaginate to false will still return all results. See https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md#auto-pagination for more information on how to configure manual paging',
'AutopaginateTrueWarning'
)
);
warnStub.restore();
done();
})
.catch(done);
});
it('returns an Array of results', done => {
const apiCall = util.createApiCall(func, createOptions);
const expected: Array<{}> = [];
Expand Down Expand Up @@ -225,7 +251,43 @@ describe('paged iteration', () => {
);
assert.strictEqual(resources.length, 10);
});
it('returns an iterable, count to 10, warns if autopaginate is specified', async () => {
const warnStub = sinon.stub(warnings, 'warn');

const spy = sinon.spy(func);
const apiCall = util.createApiCall(spy, createOptions);

async function iterableChecker(iterable: AsyncIterable<{} | undefined>) {
let counter = 0;
const resources = [];
for await (const resource of iterable) {
counter++;
resources.push(resource);
if (counter === 10) {
break;
}
}
return resources;
}

const settings = new gax.CallSettings(
(createOptions && createOptions.settings) || {}
);
settings.autoPaginate = true;
const resources = await iterableChecker(
descriptor.asyncIterate(apiCall, {}, settings)
);
assert.strictEqual(resources.length, 10);
assert.strictEqual(warnStub.callCount, 1);
assert(
warnStub.calledWith(
'autoPaginate true',
'Autopaginate will always be set to false in Async paging methods. See more info at https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md#auto-pagination for more information on how to configure paging calls',
'AutopaginateTrueWarning'
)
);
warnStub.restore();
});
it('does not stop on empty resources list', async () => {
function func(
request: {pageToken?: number},
Expand Down Expand Up @@ -337,8 +399,32 @@ describe('paged iteration', () => {
0
);
});
it('ignores autoPaginate options and warns, but respects others', done => {
const warnStub = sinon.stub(warnings, 'warn');

it('ignores autoPaginate options, but respects others', done => {
// Specifies autoPaginate: true, which will be ignored, and maxResults:
// pageSize which will be used.
const options = {maxResults: pageSize, autoPaginate: true};
streamChecker(
// @ts-ignore incomplete options
descriptor.createStream(apiCall, {}, options),
() => {
assert.strictEqual(spy.callCount, 1);
assert.strictEqual(warnStub.callCount, 1);
assert(
warnStub.calledWith(
'autoPaginate true',
'Autopaginate will always be set to false in stream paging methods. See more info at https://github.com/googleapis/gax-nodejs/blob/main/client-libraries.md#auto-pagination for more information on how to configure paging calls',
'AutopaginateTrueWarning'
)
);
warnStub.restore();
},
done,
0
);
});
it('ignores autoPaginate options but respects others', done => {
// Specifies autoPaginate: false, which will be ignored, and maxResults:
// pageSize which will be used.
const options = {maxResults: pageSize, autoPaginate: false};
Expand Down