fix: return extra args when autoPaginate is on #1365

alvarowolfx · 2024-05-22T21:09:01Z

alvarowolfx · 2024-05-23T15:08:33Z

needs fix from googleapis/nodejs-paginator#365

alvarowolfx · 2024-05-23T18:14:29Z

src/bigquery.ts

+export type QueryResultsResponse = bigquery.IGetQueryResultsResponse &
+  bigquery.IQueryResponse;
+
 export type QueryRowsResponse = PagedResponse<
  RowMetadata,
  Query,
-  bigquery.IGetQueryResultsResponse
+  QueryResultsResponse
 >;
 export type QueryRowsCallback = PagedCallback<
  RowMetadata,
  Query,
-  bigquery.IGetQueryResultsResponse
+  QueryResultsResponse


@leahecole do you think this is a breaking change ? I feel like is fine because it still compatible with the previous type, I'm just enhancing to cover other response types when getting query results, which can come as IGetQueryResultsResponse (normal path with jobs.getQueryResults) or IQueryResponse (jobs.query "fast query path).

ooo this is such a good question. I thought about it for a bit and I think you're right - it is backwards compatible - I think it does merit a minor release and not just a patch release though because you are adding functionality - you're just doing so in a way that is backwards compatible.

leahecole

I think this is okay from a TS perspective - left one comment, but would love a BQ review too

leahecole · 2024-05-23T18:28:39Z

src/bigquery.ts

+export type QueryResultsResponse = bigquery.IGetQueryResultsResponse &
+  bigquery.IQueryResponse;
+
 export type QueryRowsResponse = PagedResponse<
  RowMetadata,
  Query,
-  bigquery.IGetQueryResultsResponse
+  QueryResultsResponse
 >;
 export type QueryRowsCallback = PagedCallback<
  RowMetadata,
  Query,
-  bigquery.IGetQueryResultsResponse
+  QueryResultsResponse


ooo this is such a good question. I thought about it for a bit and I think you're right - it is backwards compatible - I think it does merit a minor release and not just a patch release though because you are adding functionality - you're just doing so in a way that is backwards compatible.

leahecole · 2024-05-23T18:30:19Z

src/bigquery.ts

+  query(
+    query: Query,
+    options?: QueryOptions
+  ): Promise<SimpleQueryRowsResponse> | Promise<QueryRowsResponse>;


Do you need to also update the docstring to have an example of this? Or is it going to be the same?

I removed this signature change as was not needed. So I think we don't need updates to the docstring, this is just to fix a behavior that users were already expecting from that method.

shollyman · 2024-05-23T20:49:59Z

src/bigquery.ts

@@ -80,15 +80,18 @@ export type PagedRequest<P> = P & {
  maxApiCalls?: number;
 };

+export type QueryResultsResponse = bigquery.IGetQueryResultsResponse &


this syntax feels absolutely wild to me, a concrete type made from ANDing two interfaces? Should it be IQueryResultsResponse?
Does this blow up if you have fields with equal names but varying types?

I changed it to be an or, so it's more accurate. When using the .query method, the return can be either a IQueryResultsResponse or IQueryResponse, this way all common fields are gonna be in that type by default and if you need an specific type, you can create a type coercion like I show in the system-tests:

nodejs-bigquery/system-test/bigquery.ts

Line 346 in bef5262

it('should query with jobs.query and return all PagedResponse as positional parameters', async () => {

nodejs-bigquery/system-test/bigquery.ts

Line 357 in bef5262

it('should query without jobs.query and return all PagedResponse as positional parameters', async () => {

"this syntax feels absolutely wild to me" that's because it is - you can do some wild and wacky things with typescript 😆

Re Alvaro, I think the OR is probably a better choice

alvarowolfx added 2 commits May 22, 2024 17:07

fix: return extra args when autoPaginate is on

f0db3ec

fix: remove tpc test

d4f6723

alvarowolfx requested review from a team as code owners May 22, 2024 21:09

alvarowolfx requested a review from obada-ab May 22, 2024 21:09

product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the googleapis/nodejs-bigquery API. labels May 22, 2024

fix: use latest version of paginator

add2823

alvarowolfx added the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 23, 2024

leahecole self-assigned this May 23, 2024

shollyman requested review from shollyman and leahecole and removed request for obada-ab May 23, 2024 18:10

alvarowolfx requested a review from obada-ab May 23, 2024 18:10

alvarowolfx commented May 23, 2024

View reviewed changes

alvarowolfx removed the request for review from obada-ab May 23, 2024 18:15

leahecole reviewed May 23, 2024

View reviewed changes

shollyman approved these changes May 23, 2024

View reviewed changes

fix: adjust type changes to be more accurate

bef5262

alvarowolfx added owlbot:run Add this label to trigger the Owlbot post processor. labels May 24, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 24, 2024

Merge branch 'main' into fix-pagination-extra-args

efc3636

leahecole approved these changes May 30, 2024

View reviewed changes

alvarowolfx added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels May 31, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 31, 2024

gcf-merge-on-green bot merged commit 9d77cd8 into googleapis:main May 31, 2024
15 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 31, 2024

release-please bot mentioned this pull request May 31, 2024

chore(main): release 7.7.1 #1367

Merged

This was referenced Jun 5, 2024

June 03, 2024 kitta65/bq-extension-vscode#415

Closed

June 03, 2024 kitta65/prettier-plugin-bq#399

Closed

June 03, 2024 kitta65/bq2cst#375

Closed

WontonSam mentioned this pull request Jul 17, 2024

[Snyk] Upgrade @google-cloud/bigquery from 6.0.3 to 7.8.0 WontonSam/repo-automation-bots#191

Open

Pyloris mentioned this pull request Jul 31, 2024

[Snyk] Upgrade @google-cloud/bigquery from 7.5.1 to 7.8.0 Pyloris/nango#4

Open

alvarowolfx mentioned this pull request Aug 30, 2024

Several issues with stateless queries #1399

Closed

WontonSam mentioned this pull request Sep 20, 2024

[Snyk] Upgrade: , , , gaxios, , , , , , ajv, c8, jsonwebtoken, lru-cache, yargs, gcf-utils, gts, mocha, nock, sinon, typescript WontonSam/repo-automation-bots#511

Open

WontonSam mentioned this pull request Oct 3, 2024

[Snyk] Upgrade @google-cloud/bigquery from 6.0.3 to 7.9.0 WontonSam/repo-automation-bots#588

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: return extra args when autoPaginate is on #1365

fix: return extra args when autoPaginate is on #1365

alvarowolfx commented May 22, 2024 •

edited

Loading

alvarowolfx commented May 23, 2024

alvarowolfx May 23, 2024

leahecole May 23, 2024

leahecole left a comment

leahecole May 23, 2024

leahecole May 23, 2024

alvarowolfx May 24, 2024

shollyman May 23, 2024

alvarowolfx May 24, 2024

leahecole May 30, 2024

leahecole May 30, 2024

fix: return extra args when autoPaginate is on #1365

fix: return extra args when autoPaginate is on #1365

Conversation

alvarowolfx commented May 22, 2024 • edited Loading

alvarowolfx commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leahecole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarowolfx commented May 22, 2024 •

edited

Loading