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

fix(types)!: properly format listing methods with gaxOptions #925

Merged
merged 22 commits into from
May 14, 2020
Merged
Show file tree
Hide file tree
Changes from 8 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
4 changes: 2 additions & 2 deletions src/backup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import {
ResourceCallback,
} from './common';
import {EnumKey, Spanner, RequestConfig, TranslateEnumKeys} from '.';
import {Metadata, Operation as GaxOperation} from 'google-gax';
import {Metadata, Operation as GaxOperation, CallOptions} from 'google-gax';
import {DateStruct, PreciseDate} from '@google-cloud/precise-date';
import {CallOptions, status} from 'grpc';
import {status} from 'grpc';
import {google as databaseAdmin} from '../protos/protos';
import {common as p} from 'protobufjs';

Expand Down
16 changes: 7 additions & 9 deletions src/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
* limitations under the License.
*/

import {ServiceError, CallOptions} from 'grpc';
import {Operation as GaxOperation} from 'google-gax';
import {ServiceError} from 'grpc';
import {Operation as GaxOperation, CallOptions} from 'google-gax';
import {google as instanceAdmin} from '../protos/protos';
import {google as databaseAdmin} from '../protos/protos';

Expand All @@ -33,9 +33,7 @@ export interface ResourceCallback<Resource, Response> {
response?: Response
): void;
}
export type PagedResponse<Item, Response> =
| [Item[]]
| [Item[], {} | null, Response];
export type PagedResponse<Item, Response> = [Item[], {} | null, Response];

export type RequestCallback<T, R = void> = R extends void
? NormalCallback<T>
Expand Down Expand Up @@ -63,8 +61,8 @@ export interface LongRunningCallback<Resource> {
): void;
}

export type PagedRequest<P> = P & {
autoPaginate?: boolean;
maxApiCalls?: number;
export interface PagedOptions {
pageSize?: number;
pageToken?: string;
skuruppu marked this conversation as resolved.
Show resolved Hide resolved
gaxOptions?: CallOptions;
};
}
78 changes: 51 additions & 27 deletions src/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import * as extend from 'extend';
import * as r from 'teeny-request';
import * as streamEvents from 'stream-events';
import * as through from 'through2';
import {Operation as GaxOperation} from 'google-gax';
import {Operation as GaxOperation, CallOptions} from 'google-gax';
import {Backup} from './backup';
import {BatchTransaction, TransactionIdentifier} from './batch-transaction';
import {google as databaseAdmin} from '../protos/protos';
Expand Down Expand Up @@ -71,13 +71,13 @@ import {
IOperation,
Schema,
RequestCallback,
PagedRequest,
PagedOptions,
ResourceCallback,
PagedResponse,
NormalCallback,
LongRunningCallback,
} from './common';
import {ServiceError, CallOptions} from 'grpc';
import {ServiceError} from 'grpc';
import {Readable, Transform, Duplex} from 'stream';
import {PreciseDate} from '@google-cloud/precise-date';
import {google as spannerClient} from '../protos/protos';
Expand Down Expand Up @@ -120,7 +120,9 @@ type PoolRequestCallback = RequestCallback<Session>;

type ResultSetStats = spannerClient.spanner.v1.ResultSetStats;

type GetSessionsOptions = PagedRequest<google.spanner.v1.IListSessionsRequest>;
interface GetSessionsOptions extends PagedOptions {
filter?: string;
}

/**
* IDatabase structure with database state enum translated to string form.
Expand Down Expand Up @@ -175,8 +177,8 @@ export type CreateSessionResponse = [
];

export interface CreateSessionOptions {
name?: string | null;
labels?: {[k: string]: string} | null;
gaxOptions?: CallOptions;
}

export type CreateSessionCallback = ResourceCallback<
Expand Down Expand Up @@ -576,6 +578,18 @@ class Database extends GrpcServiceObject {
options: CreateSessionOptions,
callback: CreateSessionCallback
): void;
/**
* Create a new session.
*
* @typedef {object} CreateSessionOptions
* @property {Object.<string, string>} [labels] The labels for the session.
*
* * Label keys must be between 1 and 63 characters long and must conform to
* the following regular expression: `[a-z]([-a-z0-9]*[a-z0-9])?`.
* * Label values must be between 0 and 63 characters long and must conform
* to the regular expression `([a-z]([-a-z0-9]*[a-z0-9])?)?`.
* * No more than 64 labels can be associated with a given session.
*/
skuruppu marked this conversation as resolved.
Show resolved Hide resolved
/**
* @typedef {array} CreateSessionResponse
* @property {Session} 0 The newly created session.
Expand Down Expand Up @@ -637,10 +651,8 @@ class Database extends GrpcServiceObject {
cb?: CreateSessionCallback
): void | Promise<CreateSessionResponse> {
const callback =
typeof optionsOrCallback === 'function'
? (optionsOrCallback as CreateSessionCallback)
: cb!;
const gaxOpts =
typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!;
const options =
typeof optionsOrCallback === 'object' && optionsOrCallback
? extend({}, optionsOrCallback)
: ({} as CreateSessionOptions);
Expand All @@ -649,17 +661,16 @@ class Database extends GrpcServiceObject {
database: this.formattedName_,
};

if (gaxOpts.labels) {
reqOpts.session = {labels: gaxOpts.labels};
delete gaxOpts.labels;
if (options.labels) {
reqOpts.session = {labels: options.labels};
}

this.request<google.spanner.v1.ISession>(
{
client: 'SpannerClient',
method: 'createSession',
reqOpts,
gaxOpts,
gaxOpts: options.gaxOptions,
},
(err, resp) => {
if (err) {
Expand Down Expand Up @@ -1141,9 +1152,7 @@ class Database extends GrpcServiceObject {
/**
* Options object for listing sessions.
*
* @typedef {object} GetSessionsRequest
* @property {boolean} [autoPaginate=true] Have pagination handled
* automatically.
* @typedef {object} GetSessionsOptions
* @property {string} [filter] An expression for filtering the results of the
* request. Filter rules are case insensitive. The fields eligible for
* filtering are:
Expand All @@ -1158,21 +1167,23 @@ class Database extends GrpcServiceObject {
* - **`labels.env:dev`** The instance's label env has the value dev.
* - **`name:howl labels.env:dev`** The instance's name is howl and it has
* the label env with value dev.
* @property {number} [maxApiCalls] Maximum number of API calls to make.
* @property {number} [maxResults] Maximum number of items to return.
* @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
* here: https://googleapis.github.io/gax-nodejs/global.html#CallOptions.
*/
/**
* @typedef {array} GetSessionsResponse
* @property {Session[]} 0 Array of {@link Session} instances.
stephenplusplus marked this conversation as resolved.
Show resolved Hide resolved
* @param {object} nextQuery A query object to receive more results.
AVaksman marked this conversation as resolved.
Show resolved Hide resolved
* @property {object} 1 The full API response.
*/
/**
* @callback GetSessionsCallback
* @param {?Error} err Request error, if any.
* @param {Session[]} instances Array of {@link Session} instances.
* @param {object} nextQuery A query object to to receive more results.
* @param {object} apiResponse The full API response.
*/
/**
Expand Down Expand Up @@ -1210,7 +1221,7 @@ class Database extends GrpcServiceObject {
* }
*
* database.getInstances({
* autoPaginate: false
* gaxOptions: {autoPaginate: false}
* }, callback);
*
* //-
Expand All @@ -1227,19 +1238,32 @@ class Database extends GrpcServiceObject {
// eslint-disable-next-line @typescript-eslint/no-this-alias
const self = this;
const callback =
typeof optionsOrCallback === 'function'
? (optionsOrCallback as GetSessionsCallback)
: cb;
typeof optionsOrCallback === 'function' ? optionsOrCallback : cb;
const options =
typeof optionsOrCallback === 'object'
? (optionsOrCallback as GetSessionsOptions)
: {gaxOptions: {}};
const gaxOpts: CallOptions = options.gaxOptions as CallOptions;
const reqOpts = extend({}, options, {
? optionsOrCallback
: ({} as GetSessionsOptions);
const gaxOpts = extend(true, {}, options.gaxOptions);
let reqOpts = extend({}, options, {
database: this.formattedName_,
});

delete reqOpts.gaxOptions;

// Copy over pageSize and pageToken values from gaxOptions.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// However values set on options take precedence.
if (gaxOpts) {
reqOpts = extend(
{},
{
pageSize: gaxOpts.pageSize,
pageToken: gaxOpts.pageToken,
},
reqOpts
);
delete gaxOpts.pageSize;
delete gaxOpts.pageToken;
}

this.request<
google.spanner.v1.ISession,
google.spanner.v1.IListSessionsResponse
Expand Down
Loading