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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Use REST #1698

Merged
merged 8 commits into from Sep 12, 2022
Merged

feat: Use REST #1698

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4767a64
feat: Use Rest
schmidt-sebastian Apr 8, 2022
58add47
Merge remote-tracking branch 'origin/main' into mrschmidt/restclient
MarkDuckworth Jul 25, 2022
6bc4b18
Merge branch 'main' into mrschmidt/restclient
alexander-fenster Aug 23, 2022
5d42fb4
Merge branch 'main' into mrschmidt/restclient
alexander-fenster Aug 30, 2022
33b57fe
fix: make it work without loading the full google-gax
alexander-fenster Aug 31, 2022
61231c0
fix: use newer prerelease version of google-gax
alexander-fenster Sep 1, 2022
34de66d
fix: use the latest google-gax
alexander-fenster Sep 2, 2022
fcbe062
Merge branch 'main' into mrschmidt/restclient
alexander-fenster Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions dev/src/bulk-writer.ts
View file
Open in desktop
Expand Up @@ -16,7 +16,7 @@
import * as firestore from '@google-cloud/firestore';

import * as assert from 'assert';
import {GoogleError} from 'google-gax';
import type {GoogleError} from 'google-gax';

import {google} from '../protos/firestore_v1_proto_api';
import {FieldPath, Firestore} from '.';
Expand Down Expand Up @@ -285,9 +285,10 @@ class BulkCommitBatch extends WriteBatch {
);
this.pendingOps[i].onSuccess(new WriteResult(updateTime));
} else {
const error = new (require('google-gax').GoogleError)(
status.message || undefined
);
const error =
new (require('google-gax/build/src/fallback').GoogleError)(
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 wondering if instead of using this path to fallback, we can use a subpath export to the fallback? https://nodejs.org/api/packages.html#subpath-exports

Maybe it's already set up that way, I didn't look at gax source yet. But the path indicates maybe it is not.

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 great idea (and "today I learned" as well).

We will be able to safely start using it when we drop Node 12 support. The documentation says subpath exports were added in v12.7.0 but we have "engines.node" set to ">=12" now, so we cannot possibly start using it without making a semver major. But for the next major I definitely want to do it. I filed googleapis/gax-nodejs#1337 so that we don't forget.

status.message || undefined
);
error.code = status.code as number;
this.pendingOps[i].onError(wrapError(error, stack));
}
Expand Down
62 changes: 48 additions & 14 deletions dev/src/index.ts
View file
Open in desktop
Expand Up @@ -16,7 +16,9 @@

import * as firestore from '@google-cloud/firestore';

import {CallOptions} from 'google-gax';
import type {CallOptions} from 'google-gax';
import type * as googleGax from 'google-gax';
import type * as googleGaxFallback from 'google-gax/build/src/fallback';
import {Duplex, PassThrough, Transform} from 'stream';

import {URL} from 'url';
Expand Down Expand Up @@ -393,6 +395,16 @@ export class Firestore implements firestore.Firestore {
*/
private _clientPool: ClientPool<GapicClient>;

/**
* Preloaded instance of google-gax (full module, with gRPC support).
*/
private _gax?: typeof googleGax;

/**
* Preloaded instance of google-gax HTTP fallback implementation (no gRPC).
*/
private _gaxFallback?: typeof googleGaxFallback;

/**
* The configuration options for the GAPIC client.
* @private
Expand Down Expand Up @@ -541,20 +553,41 @@ export class Firestore implements firestore.Firestore {
const useFallback =
!this._settings.preferRest || requiresGrpc ? false : 'rest';

let gax: typeof googleGax | typeof googleGaxFallback;
if (useFallback) {
if (!this._gaxFallback) {
gax = this._gaxFallback = require('google-gax/build/src/fallback');
} else {
gax = this._gaxFallback;
}
} else {
if (!this._gax) {
gax = this._gax = require('google-gax');
} else {
gax = this._gax;
}
}

if (this._settings.ssl === false) {
const grpcModule = this._settings.grpc ?? require('google-gax').grpc;
const sslCreds = grpcModule.credentials.createInsecure();

client = new module.exports.v1({
sslCreds,
...this._settings,
fallback: useFallback,
});
client = new module.exports.v1(
{
sslCreds,
...this._settings,
fallback: useFallback,
},
gax
);
} else {
client = new module.exports.v1({
...this._settings,
fallback: useFallback,
});
client = new module.exports.v1(
{
...this._settings,
fallback: useFallback,
},
gax
);
}

logger('Firestore', null, 'Initialized Firestore GAPIC Client');
Expand Down Expand Up @@ -1431,10 +1464,11 @@ export class Firestore implements firestore.Firestore {

if (retryCodes) {
const retryParams = getRetryParams(methodName);
callOptions.retry = new (require('google-gax').RetryOptions)(
retryCodes,
retryParams
);
callOptions.retry =
new (require('google-gax/build/src/fallback').RetryOptions)(
retryCodes,
retryParams
);
}

return callOptions;
Expand Down
4 changes: 2 additions & 2 deletions dev/src/recursive-delete.ts
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ import Firestore, {
QueryDocumentSnapshot,
} from '.';
import {Deferred, wrapError} from './util';
import {GoogleError} from 'google-gax';
import type {GoogleError} from 'google-gax';
import {BulkWriterError} from './bulk-writer';
import {QueryOptions} from './reference';
import {StatusCode} from './status-code';
Expand Down Expand Up @@ -291,7 +291,7 @@ export class RecursiveDelete {
if (this.lastError === undefined) {
this.completionDeferred.resolve();
} else {
let error = new (require('google-gax').GoogleError)(
let error = new (require('google-gax/build/src/fallback').GoogleError)(
`${this.errorCount} ` +
`${this.errorCount !== 1 ? 'deletes' : 'delete'} ` +
'failed. The last delete failed with: '
Expand Down
10 changes: 5 additions & 5 deletions dev/src/util.ts
View file
Open in desktop
Expand Up @@ -17,8 +17,8 @@
import {DocumentData} from '@google-cloud/firestore';

import {randomBytes} from 'crypto';
import {CallSettings, ClientConfig, GoogleError} from 'google-gax';
import {BackoffSettings} from 'google-gax/build/src/gax';
import type {CallSettings, ClientConfig, GoogleError} from 'google-gax';
import type {BackoffSettings} from 'google-gax/build/src/gax';
import * as gapicConfig from './v1/firestore_client_config.json';

/**
Expand Down Expand Up @@ -157,11 +157,11 @@ let serviceConfig: Record<string, CallSettings> | undefined;
/** Lazy-loads the service config when first accessed. */
function getServiceConfig(methodName: string): CallSettings | undefined {
if (!serviceConfig) {
serviceConfig = require('google-gax').constructSettings(
serviceConfig = require('google-gax/build/src/fallback').constructSettings(
'google.firestore.v1.Firestore',
gapicConfig as ClientConfig,
{},
require('google-gax').Status
require('google-gax/build/src/status').Status
) as {[k: string]: CallSettings};
}
return serviceConfig[methodName];
Expand All @@ -185,7 +185,7 @@ export function getRetryCodes(methodName: string): number[] {
export function getRetryParams(methodName: string): BackoffSettings {
return (
getServiceConfig(methodName)?.retry?.backoffSettings ??
require('google-gax').createDefaultBackoffSettings()
require('google-gax/build/src/fallback').createDefaultBackoffSettings()
);
}

Expand Down
2 changes: 1 addition & 1 deletion dev/system-test/firestore.ts
View file
Open in desktop
Expand Up @@ -51,7 +51,7 @@ import {
verifyInstance,
} from '../test/util/helpers';
import {BulkWriter} from '../src/bulk-writer';
import {Status} from 'google-gax';
import {Status} from 'google-gax/build/src/status';
import {QueryPartition} from '../src/query-partition';
import {CollectionGroup} from '../src/collection-group';

Expand Down
11 changes: 7 additions & 4 deletions dev/test/util/helpers.ts
View file
Open in desktop
Expand Up @@ -21,12 +21,11 @@ import {

import {expect} from 'chai';
import * as extend from 'extend';
import {grpc} from 'google-gax';
import {JSONStreamIterator} from 'length-prefixed-json-stream';
import {Duplex, PassThrough} from 'stream';
import * as through2 from 'through2';
import {firestore, google} from '../../protos/firestore_v1_proto_api';

import type {grpc} from 'google-gax';
import * as proto from '../../protos/firestore_v1_proto_api';
import * as v1 from '../../src/v1';
import {Firestore, QueryDocumentSnapshot} from '../../src';
Expand All @@ -35,7 +34,11 @@ import {GapicClient} from '../../src/types';

import api = proto.google.firestore.v1;

const SSL_CREDENTIALS = grpc.credentials.createInsecure();
let SSL_CREDENTIALS: grpc.ChannelCredentials | null = null;
if (!process.env.USE_REST_FALLBACK) {
const grpc = require('google-gax').grpc;
SSL_CREDENTIALS = grpc.credentials.createInsecure();
}

export const PROJECT_ID = 'test-project';
export const DATABASE_ROOT = `projects/${PROJECT_ID}/databases/(default)`;
Expand All @@ -62,7 +65,7 @@ export function createInstance(
firestoreSettings?: Settings
): Promise<Firestore> {
const initializationOptions = {
...{projectId: PROJECT_ID, sslCreds: SSL_CREDENTIALS},
...{projectId: PROJECT_ID, sslCreds: SSL_CREDENTIALS!},
...firestoreSettings,
};

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@
"dependencies": {
"fast-deep-equal": "^3.1.1",
"functional-red-black-tree": "^1.0.1",
"google-gax": "^3.2.1",
"google-gax": "^3.3.2-pre",
"protobufjs": "^7.0.0"
},
"devDependencies": {
Expand Down