fix(types)!: properly format listing methods with gaxOptions #925
Conversation
AVaksman
commented
Apr 24, 2020
AVaksman
commented
- Ensure the tests and linter pass
- Code coverage does not decrease (if any source code was changed)
- Appropriate docs were updated (if necessary)
Codecov Report
@@ Coverage Diff @@
## master #925 +/- ##
==========================================
+ Coverage 98.20% 98.25% +0.04%
==========================================
Files 21 21
Lines 19959 20321 +362
Branches 1069 1076 +7
==========================================
+ Hits 19601 19966 +365
+ Misses 354 352 -2
+ Partials 4 3 -1
Continue to review full report at Codecov.
|
googlebot
added
the
cla: yes
| @@ -365,22 +365,31 @@ class Instance extends common.GrpcServiceObject { | |||
| * @property {number} [pageSize] Maximum number of results per page. | |||
| * @property {string} [pageToken] A previously-returned page token | |||
| * representing part of the larger set of results to view. | |||
| * @property {object} [gaxOptions] Request configuration options, outlined | |||
There was a problem hiding this comment.
How does it work that we have top-level "pageSize" and "pageToken", but they're also in gaxOptions (e.g. "gaxOptions.pageSize")?
There was a problem hiding this comment.
Thank you!
The existing behavior is as follows:
Current options for listing methods typically look something like this
{
parent?: string;
pageSize?: number;
pageToken?: string;
filter?: string;
autoPaginate?: boolean;
maxApiCalls?: number;
gaxOptions?: CallOptions;
}Where gaxOptions (among others) contain the following options as well
{
autoPaginate?: boolean;
pageToken?: string;
pageSize?: number;
}Which is utilized as
instance.getBackups({
pageSize: 3,
pageToken,
gaxOptions: {autoPaginate: false},
});in system tests as well as samples.
In terms of reqOpts and gaxOpts of the request() method the following options are respected
reqOpts = {
parent: string;
pageSize?: number;
pageToken?: string;
filter?: string;
}
gaxOpts = {
autoPaginate?: boolean;
}having pageSize and pageToken in two places is confusing and misleading. Should we just remove them from the top-level and pass to the reqOts from the the gaxOptions object, or accept and respect pageSize and pageToken values from both top-level and gaxOptios, or another option?
WDYT @stephenplusplus
There was a problem hiding this comment.
This is indeed tricky! I don't think we have the option to remove it from the top-level, since that would conflict with the raw API documentation, e.g. https://cloud.google.com/spanner/docs/reference/rpc/google.spanner.admin.database.v1#google.spanner.admin.database.v1.ListBackupsRequest which accepts "pageSize" and "pageToken".
Do you know where & what gax is doing with "pageSize" and "pageToken". It seems like the wrong place to send a native request parameter, as opposed to the other options in "gaxOptions" which are all custom helpers like "autoPaginate", "bundleOptions", etc.
There was a problem hiding this comment.
Do you know where & what gax is doing with "pageSize" and "pageToken". It seems like the wrong place to send a native request parameter, as opposed to the other options in "gaxOptions" which are all custom helpers like "autoPaginate", "bundleOptions", etc.
Perhaps these values where used by paginator.
Speaking of which, I believe it (paginator.streamify) could be dropped as both paginated and streaming calls can be handled now.
There was a problem hiding this comment.
Should gax stop listing those under CallOptions?
There was a problem hiding this comment.
refactored to pass pageSize and pageToken as reqOpts.
PTAL
There was a problem hiding this comment.
Thanks for trying to clean up this mess @AVaksman. I'm not a fan of having to support pageSize and pageToken from CallOptions but I feel like we have no choice :( Given that it's in the documentation, it's very likely someone will try to set it in gaxOptions and wonder why it's not being respected.
| delete reqOpts.gaxOptions; | ||
|
|
||
| // Copy over pageSize and pageToken values from gaxOptions. |
There was a problem hiding this comment.
Since we're in a breaking change, let's not even worry about supporting this improper usage. Most people have probably been using it in reqOpts already anyway.
There was a problem hiding this comment.
I completely agree with this but I think we have no choice but to do this while CallOptions still has pageSize and pageToken parameters (see my reply here). Someone is bound to set it here instead of in reqOpts.
There was a problem hiding this comment.
That's a good point! I sent a PR to gax to remove those options. After that, we can re-generate the docs, and remove them. I would be surprised if we had any users putting those options in there. Already, turning off auto-pagination and setting pageSize and pageToken manually is rare. Setting options through gax (a very unfamiliar concept for a user) would be even more unlikely.
I would be okay with still dropping our backup support while we wait for the changes from the linked PR to take effect. Otherwise, I think waiting is worth it, to avoid the complexity of support. Of course, we can always come back and remove the code later, so whatever everyone thinks is best :)
There was a problem hiding this comment.
@stephenplusplus do you know how long it would take to get the gax PR merged and into a release?
The main concern I have is that this PR needs to go into v5.0.0. We need v5.0.0 to go out asap since it's got the backup & restore support which is already a GA'ed feature but is currently not supported by a Node.js release.
So if we think the gax change will take more than a week to get through, then we should merge this as is then remove the code later.
There was a problem hiding this comment.
I'm not sure how long that would take. Hopefully @alexander-fenster can take a look at that PR (googleapis/gax-nodejs#821) when he's available and give us a better idea.
There was a problem hiding this comment.
We just made a semver major for gax a few weeks ago. From one hand, major releases are cheap, but from another hand, having too many majors makes it harder to hot fix security issues so I would better wait for some time before making gax v3.0.0. So I would probably suggest you go to v5.0.0 here as is. What do you think @skuruppu @stephenplusplus?
There was a problem hiding this comment.
Thanks @alexander-fenster. It's okay with me to wait. The temporary code won't be too bad. Alternatively, I still think we're safe to remove support for the options anyway, and if anybody in the world notices, we can release a hotfix. But that's completely your call @skuruppu.
There was a problem hiding this comment.
I agree that it's so very unlikely that anyone would be trying to set these options through CallOptions. But just in case, we'll leave the temporary code. It won't be too hard to remove it when gax v3.0.0 is ready. I'm open to doing another major release whenever we need to.
| @@ -365,22 +365,31 @@ class Instance extends common.GrpcServiceObject { | |||
| * @property {number} [pageSize] Maximum number of results per page. | |||
| * @property {string} [pageToken] A previously-returned page token | |||
| * representing part of the larger set of results to view. | |||
| * @property {object} [gaxOptions] Request configuration options, outlined | |||
There was a problem hiding this comment.
Thanks for trying to clean up this mess @AVaksman. I'm not a fan of having to support pageSize and pageToken from CallOptions but I feel like we have no choice :( Given that it's in the documentation, it's very likely someone will try to set it in gaxOptions and wonder why it's not being respected.
| delete reqOpts.gaxOptions; | ||
|
|
||
| // Copy over pageSize and pageToken values from gaxOptions. |
There was a problem hiding this comment.
I completely agree with this but I think we have no choice but to do this while CallOptions still has pageSize and pageToken parameters (see my reply here). Someone is bound to set it here instead of in reqOpts.
| delete reqOpts.gaxOptions; | ||
|
|
||
| // Copy over pageSize and pageToken values from gaxOptions. |
There was a problem hiding this comment.
That's a good point! I sent a PR to gax to remove those options. After that, we can re-generate the docs, and remove them. I would be surprised if we had any users putting those options in there. Already, turning off auto-pagination and setting pageSize and pageToken manually is rare. Setting options through gax (a very unfamiliar concept for a user) would be even more unlikely.
I would be okay with still dropping our backup support while we wait for the changes from the linked PR to take effect. Otherwise, I think waiting is worth it, to avoid the complexity of support. Of course, we can always come back and remove the code later, so whatever everyone thinks is best :)
|
@stephenplusplus, let us know if there's anything else we should change in this PR. Your requested changes status is blocking merging this PR :) |
|
@skuruppu sorry about that! |
