From 33c7dd3be80d31b7adee9cee6b5213cc1329632a Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Wed, 15 Nov 2017 17:47:40 -0800 Subject: [PATCH 1/5] Revive Node.JS Support for Cloud Firestore (fixes #221). * Fix miscellaneous node / GRPC bit rot. * Re-add grpc / protobufjs dependencies. * Remove crazy grpc stream error handling nonsense that is no longer necessary after https://github.com/grpc/grpc/pull/9101 * Clean up grpc_connection logging (and consistently use util.inspect()). * Fix WebChannel / GRPC Connection compatibility issues. * Add an explicit mapping from "RPC name" (e.g. "BatchGetDocuments") to the REST url path (e.g. "batchGet") for WebChannel, and for GRPC just assume the first letter should be lowercased (e.g. "batchGetDocuments"). * Split Connection.invoke() into invokeRPC() and invokeStreamingRPC(), with the latter accepting a stream of results and aggregating them into an array (needed to support BatchGetDocuments RPC). * Fix serializer issues * Query limits are an 'Int32Value' but we were serializing them as a normal int which GRPC / protobufjs didn't like. * Several proto "oneof tags" were outdated. * Add build steps to copy protos into npm package. * Run integration tests for Node.JS * Added to 'test:node' script in package.json and in .vscode/launch.json * Include index.ts for browser and index.node.ts for node so the appropriate PlatformSupport gets registered. * Misc cleanup * Remove unused MockPlatform since we use the normal NodePlatform now. * Remove 'google-auth-library' CredentialsProvider that we used to use for node.js (before we were integrated with FirebaseAuth). * Fixed several tests that were hitting node.js warnings about unhandled promise failures. * mocha commmand-line args: * "--compilers ts:ts-node/register" was deprecated in favor of "--require ts-node/register" * Remove "--retries 5" when running mocha tests from VS Code. * Consistently use "--require" instead of "-r" * Add "--exit" when running from VS Code. --- .vscode/launch.json | 28 ++- packages/firestore/gulpfile.js | 12 +- packages/firestore/package.json | 7 +- packages/firestore/src/api/credentials.ts | 48 ------ .../firestore/src/local/garbage_source.ts | 2 +- packages/firestore/src/platform/platform.ts | 4 + .../src/platform_browser/browser_platform.ts | 5 + .../platform_browser/webchannel_connection.ts | 50 +++--- .../src/platform_mock_node/mock_init.ts | 27 --- .../src/platform_mock_node/mock_platform.ts | 54 ------ .../src/platform_node/grpc_connection.ts | 160 ++++++++++-------- .../src/platform_node/node_platform.ts | 8 + packages/firestore/src/remote/connection.ts | 19 ++- packages/firestore/src/remote/datastore.ts | 18 +- packages/firestore/src/remote/rpc_error.ts | 10 +- packages/firestore/src/remote/serializer.ts | 52 ++++-- packages/firestore/src/typings/grpc.d.ts | 2 +- packages/firestore/src/util/log.ts | 4 +- .../test/integration/api/batch_writes.test.ts | 5 + .../firestore/test/integration/bootstrap.ts | 4 +- .../integration/browser/indexeddb.test.ts | 13 +- .../integration/browser/webchannel.test.ts | 7 +- .../test/integration/util/firebase_export.ts | 1 - .../test/integration/util/helpers.ts | 8 +- .../test/unit/remote/node/serializer.test.ts | 4 +- .../test/unit/specs/spec_test_runner.ts | 6 +- 26 files changed, 289 insertions(+), 269 deletions(-) delete mode 100644 packages/firestore/src/platform_mock_node/mock_init.ts delete mode 100644 packages/firestore/src/platform_mock_node/mock_platform.ts diff --git a/.vscode/launch.json b/.vscode/launch.json index 843a6f63b95..41fc6edcaa5 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -11,11 +11,27 @@ "program": "${workspaceRoot}/packages/firestore/node_modules/.bin/_mocha", "cwd": "${workspaceRoot}/packages/firestore", "args": [ - "--compilers", "ts:ts-node/register", - "-r", "src/platform_node/node_init.ts", - "--retries", "5", + "--require", "ts-node/register", + "--require", "index.node.ts", "--timeout", "5000", - "test/{,!(integration|browser)/**/}*.test.ts" + "test/{,!(browser|integration)/**/}*.test.ts", + "--exit" + ], + "sourceMaps": true, + "protocol": "inspector" + }, + { + "type": "node", + "request": "launch", + "name": "Firestore Integration Tests (Node)", + "program": "${workspaceRoot}/packages/firestore/node_modules/.bin/_mocha", + "cwd": "${workspaceRoot}/packages/firestore", + "args": [ + "--require", "ts-node/register", + "--require", "index.node.ts", + "--timeout", "5000", + "test/{,!(browser|unit)/**/}*.test.ts", + "--exit" ], "sourceMaps": true, "protocol": "inspector" @@ -25,7 +41,7 @@ "request": "launch", "name": "Firestore Unit Tests (Browser)", "program": "${workspaceRoot}/packages/firestore/node_modules/.bin/karma", - "cwd": "${workspaceRoot}/packages/firestore", + "cwd": "${workspaceRoot}/packages/firestore", "args": [ "start", "--auto-watch", @@ -38,7 +54,7 @@ "request": "launch", "name": "Firestore Integration Tests (Browser)", "program": "${workspaceRoot}/packages/firestore/node_modules/.bin/karma", - "cwd": "${workspaceRoot}/packages/firestore", + "cwd": "${workspaceRoot}/packages/firestore", "args": [ "start", "--auto-watch", diff --git a/packages/firestore/gulpfile.js b/packages/firestore/gulpfile.js index 4373efc0d75..183210190a8 100644 --- a/packages/firestore/gulpfile.js +++ b/packages/firestore/gulpfile.js @@ -17,9 +17,19 @@ const gulp = require('gulp'); const tools = require('../../tools/build'); +function copyProtos(dest) { + return function copyProtos() { + return gulp + .src([__dirname + '/src/protos/**/*.proto']) + .pipe(gulp.dest(dest)); + }; +} + const buildModule = gulp.parallel([ tools.buildCjs(__dirname), - tools.buildEsm(__dirname) + copyProtos('dist/cjs/src/protos'), + tools.buildEsm(__dirname), + copyProtos('dist/esm/src/protos') ]); const setupWatcher = () => { diff --git a/packages/firestore/package.json b/packages/firestore/package.json index f111b95729f..515958a4780 100644 --- a/packages/firestore/package.json +++ b/packages/firestore/package.json @@ -7,7 +7,7 @@ "test": "run-p test:browser test:node", "test:browser": "karma start --single-run", "test:browser:debug": "karma start --browsers=Chrome --auto-watch", - "test:node": "mocha 'test/{,!(integration|browser)/**/}*.test.ts' --compilers ts:ts-node/register -r src/platform_node/node_init.ts --retries 5 --timeout 5000 --exit", + "test:node": "mocha 'test/{,!(browser)/**/}*.test.ts' --require ts-node/register --require index.node.ts --retries 5 --timeout 5000 --exit", "prepare": "gulp build" }, "main": "dist/cjs/index.node.js", @@ -18,7 +18,9 @@ "module": "dist/esm/index.js", "license": "Apache-2.0", "dependencies": { - "@firebase/webchannel-wrapper": "^0.2.4" + "@firebase/webchannel-wrapper": "^0.2.4", + "grpc": "^1.6.6", + "protobufjs": "^5.0.0" }, "peerDependencies": { "@firebase/app": "^0.1.0" @@ -28,7 +30,6 @@ "@types/mocha": "^2.2.44", "@types/sinon": "^2.3.7", "chai": "^4.1.1", - "grpc": "^1.6.6", "gulp": "gulpjs/gulp#4.0", "karma": "^1.7.0", "karma-chrome-launcher": "^2.2.0", diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts index c91229f427e..932e0b6f1e8 100644 --- a/packages/firestore/src/api/credentials.ts +++ b/packages/firestore/src/api/credentials.ts @@ -29,11 +29,6 @@ export interface FirstPartyCredentialsSettings { sessionIndex: string; } -export interface GoogleAuthCredentialsSettings { - type: 'google-auth'; - client: GoogleAuthClient; -} - export interface ProviderCredentialsSettings { type: 'provider'; client: CredentialsProvider; @@ -42,7 +37,6 @@ export interface ProviderCredentialsSettings { /** Settings for private credentials */ export type CredentialsSettings = | FirstPartyCredentialsSettings - | GoogleAuthCredentialsSettings | ProviderCredentialsSettings; export type TokenType = 'OAuth' | 'FirstParty'; @@ -238,45 +232,6 @@ export class FirebaseCredentialsProvider implements CredentialsProvider { } } -// Wrap a google-auth-library client as a CredentialsProvider. -// NOTE: grpc-connection can natively accept a google-auth-library -// client via createFromGoogleCredential(), but we opt to plumb the tokens -// through our CredentialsProvider interface, at least for now. -export class GoogleCredentialsProvider implements CredentialsProvider { - constructor(private authClient: GoogleAuthClient) {} - - getToken(forceRefresh: boolean): Promise { - return new Promise((resolve, reject) => { - // TODO(b/32935141): ideally this would be declared as an extern - this.authClient[ - 'getAccessToken' - ]((error: AnyJs, tokenLiteral: string) => { - if (error) { - reject(error); - } else { - resolve(new OAuthToken(tokenLiteral, User.GOOGLE_CREDENTIALS)); - } - }); - }); - } - - // NOTE: A google-auth-library client represents an immutable "user", so - // once we fire the initial event, it'll never change. - setUserChangeListener(listener: UserListener): void { - // Fire with initial uid. - listener(User.GOOGLE_CREDENTIALS); - } - - removeUserChangeListener(): void {} -} - -/** - * Very incomplete typing for an auth client from - * https://github.com/google/google-auth-library-nodejs/ - */ -export interface GoogleAuthClient { - getAccessToken(callback: (error?: Error, token?: string) => void): void; -} // TODO(b/32935141): Ideally gapi type would be declared as an extern // tslint:disable-next-line:no-any export type Gapi = any; @@ -348,9 +303,6 @@ export function makeCredentialsProvider(credentials?: CredentialsSettings) { } switch (credentials.type) { - case 'google-auth': - return new GoogleCredentialsProvider(credentials.client); - case 'gapi': return new FirstPartyCredentialsProvider( credentials.client, diff --git a/packages/firestore/src/local/garbage_source.ts b/packages/firestore/src/local/garbage_source.ts index 940593a0165..fb9ebbceaac 100644 --- a/packages/firestore/src/local/garbage_source.ts +++ b/packages/firestore/src/local/garbage_source.ts @@ -37,7 +37,7 @@ export interface GarbageSource { * This can be used by garbage collectors to double-check if a key exists in * this collection when it was released elsewhere. * - * PORTING NODE: This is used in contexts where PersistenceTransaction is + * PORTING NOTE: This is used in contexts where PersistenceTransaction is * known not to be needed, in this case we just pass in null. Therefore * any implementations must gaurd against null values. */ diff --git a/packages/firestore/src/platform/platform.ts b/packages/firestore/src/platform/platform.ts index 0a8ebf48f59..cb2ff6dc5b7 100644 --- a/packages/firestore/src/platform/platform.ts +++ b/packages/firestore/src/platform/platform.ts @@ -19,6 +19,7 @@ import { ProtoByteString } from '../core/types'; import { Connection } from '../remote/connection'; import { JsonProtoSerializer } from '../remote/serializer'; import { fail } from '../util/assert'; +import { AnyJs } from '../util/misc'; /** * Provides a common interface to load anything platform dependent, e.g. @@ -30,6 +31,9 @@ export interface Platform { loadConnection(databaseInfo: DatabaseInfo): Promise; newSerializer(databaseId: DatabaseId): JsonProtoSerializer; + /** Formats an object as a JSON string, suitable for logging. */ + formatJSON(value: AnyJs): string; + /** Converts a Base64 encoded string to a binary string. */ atob(encoded: string): string; diff --git a/packages/firestore/src/platform_browser/browser_platform.ts b/packages/firestore/src/platform_browser/browser_platform.ts index 2eac816b3f2..157af2d7ee0 100644 --- a/packages/firestore/src/platform_browser/browser_platform.ts +++ b/packages/firestore/src/platform_browser/browser_platform.ts @@ -20,6 +20,7 @@ import { Connection } from '../remote/connection'; import { JsonProtoSerializer } from '../remote/serializer'; import { WebChannelConnection } from './webchannel_connection'; +import { AnyJs } from '../util/misc'; export class BrowserPlatform implements Platform { readonly base64Available: boolean; @@ -38,6 +39,10 @@ export class BrowserPlatform implements Platform { return new JsonProtoSerializer(databaseId, { useProto3Json: true }); } + formatJSON(value: AnyJs): string { + return JSON.stringify(value); + } + atob(encoded: string): string { return atob(encoded); } diff --git a/packages/firestore/src/platform_browser/webchannel_connection.ts b/packages/firestore/src/platform_browser/webchannel_connection.ts index 869cbdfe938..f63bed378b5 100644 --- a/packages/firestore/src/platform_browser/webchannel_connection.ts +++ b/packages/firestore/src/platform_browser/webchannel_connection.ts @@ -38,8 +38,15 @@ import { Rejecter, Resolver } from '../util/promise'; const LOG_TAG = 'Connection'; +const RPC_STREAM_SERVICE = 'google.firestore.v1beta1.Firestore'; const RPC_URL_VERSION = 'v1beta1'; +/** Maps RPC names to the corresponding REST endpoint name. */ +const RPC_NAME_REST_MAPPING = { + BatchGetDocuments: 'batchGet', + Commit: 'commit' +}; + // TODO(b/38203344): The SDK_VERSION is set independently from Firebase because // we are doing out-of-band releases. Once we release as part of Firebase, we // should use the Firebase version instead. @@ -52,24 +59,6 @@ export class WebChannelConnection implements Connection { private readonly baseUrl: string; private readonly pool: XhrIoPool; - /** - * Mapping from RPC name to service providing the RPC. - * For streaming RPCs only. - */ - private static RPC_STREAM_SERVICE_MAPPING: { [rpc: string]: string } = { - Write: 'google.firestore.v1beta1.Firestore', - Listen: 'google.firestore.v1beta1.Firestore' - }; - - /** - * Mapping from RPC name to actual RPC name in URLs. - * For streaming RPCs only. - */ - private static RPC_STREAM_NAME_MAPPING: { [rpc: string]: string } = { - Write: 'Write', - Listen: 'Listen' - }; - constructor(info: DatabaseInfo) { this.databaseId = info.databaseId; this.pool = new XhrIoPool(); @@ -100,7 +89,7 @@ export class WebChannelConnection implements Connection { `databases/${this.databaseId.database}`; } - invoke(rpcName: string, request: any, token: Token | null): Promise { + invokeRPC(rpcName: string, request: any, token: Token | null): Promise { const url = this.makeUrl(rpcName); return new Promise((resolve: Resolver, reject: Rejecter) => { @@ -178,18 +167,23 @@ export class WebChannelConnection implements Connection { }); } + invokeStreamingRPC( + rpcName: string, + request: any, + token: Token | null + ): Promise { + // The REST API automatically aggregates all of the streamed results, so we + // can just use the normal invoke() method. + return this.invokeRPC(rpcName, request, token); + } + openStream(rpcName: string, token: Token | null): Stream { - const rpcService = WebChannelConnection.RPC_STREAM_SERVICE_MAPPING[rpcName]; - const rpcUrlName = WebChannelConnection.RPC_STREAM_NAME_MAPPING[rpcName]; - if (!rpcService || !rpcUrlName) { - fail('Unknown RPC name: ' + rpcName); - } const urlParts = [ this.baseUrl, '/', - rpcService, + RPC_STREAM_SERVICE, '/', - rpcUrlName, + rpcName, '/channel' ]; const webchannelTransport = createWebChannelTransport(); @@ -339,6 +333,8 @@ export class WebChannelConnection implements Connection { // visible for testing makeUrl(rpcName: string): string { + const urlRpcName = RPC_NAME_REST_MAPPING[rpcName]; + assert(urlRpcName != null, 'Unknown REST mapping for: ' + rpcName); const url = [this.baseUrl, '/', RPC_URL_VERSION]; url.push('/projects/'); url.push(this.databaseId.projectId); @@ -348,7 +344,7 @@ export class WebChannelConnection implements Connection { url.push('/documents'); url.push(':'); - url.push(rpcName); + url.push(urlRpcName); return url.join(''); } } diff --git a/packages/firestore/src/platform_mock_node/mock_init.ts b/packages/firestore/src/platform_mock_node/mock_init.ts deleted file mode 100644 index 8834348b2d0..00000000000 --- a/packages/firestore/src/platform_mock_node/mock_init.ts +++ /dev/null @@ -1,27 +0,0 @@ -/** - * Copyright 2017 Google Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { PlatformSupport } from '../platform/platform'; -import { MockPlatform } from './mock_platform'; - -/** - * This code needs to run before Firestore is used. This can be achieved in - * several ways: - * 1) Through the JSCompiler compiling this code and then (automatically) - * executing it before exporting the Firestore symbols. - * 2) Through importing this module first in a Firestore main module - */ -PlatformSupport.setPlatform(new MockPlatform()); diff --git a/packages/firestore/src/platform_mock_node/mock_platform.ts b/packages/firestore/src/platform_mock_node/mock_platform.ts deleted file mode 100644 index 52145a95a2a..00000000000 --- a/packages/firestore/src/platform_mock_node/mock_platform.ts +++ /dev/null @@ -1,54 +0,0 @@ -/** - * Copyright 2017 Google Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { DatabaseId, DatabaseInfo } from '../core/database_info'; -import { Platform } from '../platform/platform'; -import { Connection } from '../remote/connection'; -import { JsonProtoSerializer } from '../remote/serializer'; -import { fail } from '../util/assert'; -import { Code, FirestoreError } from '../util/error'; - -export class MockPlatform implements Platform { - /** A "mock" platform for use in NodeJS unit tests since we can't depend on - * gRPC. */ - - readonly base64Available = true; - - readonly emptyByteString = new Uint8Array(0); - - loadConnection(databaseInfo: DatabaseInfo): Promise { - return fail('loadConnection() not supported in unit tests.'); - } - newSerializer(databaseId: DatabaseId): JsonProtoSerializer { - return new JsonProtoSerializer(databaseId, { useProto3Json: false }); - } - - atob(encoded: string): string { - // Node actually doesn't validate base64 strings. - // A quick sanity check that is not a fool-proof validation - if (/[^-A-Za-z0-9+/=]/.test(encoded)) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - 'Not a valid Base64 string: ' + encoded - ); - } - return new Buffer(encoded, 'base64').toString('binary'); - } - - btoa(raw: string): string { - return new Buffer(raw, 'binary').toString('base64'); - } -} diff --git a/packages/firestore/src/platform_node/grpc_connection.ts b/packages/firestore/src/platform_node/grpc_connection.ts index 902e8827235..a8724cdb452 100644 --- a/packages/firestore/src/platform_node/grpc_connection.ts +++ b/packages/firestore/src/platform_node/grpc_connection.ts @@ -14,24 +14,13 @@ * limitations under the License. */ -// TODO(dimond): The following imports have been replaced with require -// statements to not let the google closure compiler try to resolve them at -// compile time. -// import * as grpc from 'grpc'; -// import * as protobufjs from 'protobufjs'; -// import * as util from 'util'; +import * as grpc from 'grpc'; +import * as protobufjs from 'protobufjs'; import firebase from '@firebase/app'; const SDK_VERSION = firebase.SDK_VERSION; -// Temporary type definition until types work again (see above) -export type GrpcMetadataCallback = any; -// Trick the TS compiler & Google closure compiler into executing normal require -// statements, not using goog.require to import modules at compile time -const dynamicRequire = require; -const grpc = dynamicRequire('grpc'); -const grpcVersion = dynamicRequire('grpc/package.json').version; -const util = dynamicRequire('util'); +const grpcVersion = require('grpc/package.json').version; import { Token } from '../api/credentials'; import { DatabaseInfo } from '../core/database_info'; @@ -44,6 +33,7 @@ import * as log from '../util/log'; import { AnyJs } from '../util/misc'; import { NodeCallback, nodePromise } from '../util/node_api'; import { ProtobufProtoBuilder } from './load_protos'; +import { Deferred } from '../util/promise'; const LOG_TAG = 'Connection'; @@ -64,7 +54,7 @@ function createHeaders(databaseInfo: DatabaseInfo, token: Token | null): {} { : grpc.credentials.createInsecure(); const callCredentials = grpc.credentials.createFromMetadataGenerator( - (context: { serviceUrl: string }, cb: GrpcMetadataCallback) => { + (context: { serviceUrl: string }, cb: grpc.MetadataCallback) => { const metadata = new grpc.Metadata(); if (token) { for (const header in token.authHeaders) { @@ -102,7 +92,9 @@ interface CachedStub { /** GRPC errors expose a code property. */ interface GrpcError extends Error { - code: number; + // Errors from GRPC *usually* have a `code`, but in some cases (such as trying + // to send an invalid proto message), they do not. + code?: number; } /** GRPC status information. */ @@ -138,7 +130,7 @@ export class GrpcConnection implements Connection { // tslint:disable-next-line:no-any private getStub(token: Token | null): any { if (!this.cachedStub || !this.sameToken(this.cachedStub.token, token)) { - log.debug(LOG_TAG, 'Creating datastore stubs.'); + log.debug(LOG_TAG, 'Creating Firestore stub.'); const credentials = createHeaders(this.databaseInfo, token); this.cachedStub = { stub: new this.firestore.Firestore(this.databaseInfo.host, credentials), @@ -148,18 +140,25 @@ export class GrpcConnection implements Connection { return this.cachedStub.stub; } - invoke(rpcName: string, request: any, token: Token | null): Promise { + private getRpc(rpcName: string, token: Token | null): any { const stub = this.getStub(token); + + // RPC Methods have the first character lower-cased + // (e.g. Listen => listen(), BatchGetDocuments => batchGetDocuments()). + const rpcMethod = rpcName.charAt(0).toLowerCase() + rpcName.slice(1); + const rpc = stub[rpcMethod]; + assert(rpc != null, 'Unknown RPC: ' + rpcName); + + return rpc.bind(stub); + } + + invokeRPC(rpcName: string, request: any, token: Token | null): Promise { + const rpc = this.getRpc(rpcName, token); return nodePromise((callback: NodeCallback) => { - return stub[rpcName](request, (grpcError?: GrpcError, value?: AnyJs) => { + log.debug(LOG_TAG, `RPC '${rpcName}' invoked with request:`, request); + return rpc(request, (grpcError?: GrpcError, value?: AnyJs) => { if (grpcError) { - log.debug( - LOG_TAG, - 'RPC "' + - rpcName + - '" failed with error ' + - JSON.stringify(grpcError) - ); + log.debug(LOG_TAG, `RPC '${rpcName}' failed with error:`, grpcError); callback( new FirestoreError( mapCodeFromRpcCode(grpcError.code), @@ -167,16 +166,53 @@ export class GrpcConnection implements Connection { ) ); } else { + log.debug( + LOG_TAG, + `RPC '${rpcName}' completed with response:`, + value + ); callback(undefined, value); } }); }); } + invokeStreamingRPC( + rpcName: string, + request: any, + token: Token | null + ): Promise { + const rpc = this.getRpc(rpcName, token); + const results = []; + const responseDeferred = new Deferred(); + + log.debug( + LOG_TAG, + `RPC '${rpcName}' invoked (streaming) with request:`, + request + ); + const stream = rpc(request); + stream.on('data', response => { + log.debug(LOG_TAG, `RPC ${rpcName} received result:`, response); + results.push(response); + }); + stream.on('end', () => { + log.debug(LOG_TAG, `RPC '${rpcName}' completed.`); + responseDeferred.resolve(results); + }); + stream.on('error', grpcError => { + log.debug(LOG_TAG, `RPC '${rpcName}' failed with error:`, grpcError); + const code = mapCodeFromRpcCode(grpcError.code); + responseDeferred.reject(new FirestoreError(code, grpcError.message)); + }); + + return responseDeferred.promise; + } + // TODO(mikelehen): This "method" is a monster. Should be refactored. openStream(rpcName: string, token: Token | null): Stream { - const stub = this.getStub(token); - const grpcStream = stub[rpcName](); + const rpc = this.getRpc(rpcName, token); + const grpcStream = rpc(); let closed = false; let close: (err?: Error) => void; @@ -185,33 +221,22 @@ export class GrpcConnection implements Connection { const stream = new StreamBridge({ sendFn: (msg: any) => { if (!closed) { - log.debug( - LOG_TAG, - 'GRPC stream sending:', - util.inspect(msg, { depth: 100 }) - ); + log.debug(LOG_TAG, 'GRPC stream sending:', msg); try { grpcStream.write(msg); } catch (e) { // This probably means we didn't conform to the proto. Make sure to // log the message we sent. - log.error( - LOG_TAG, - 'Failure sending: ', - util.inspect(msg, { depth: 100 }) - ); - log.error(LOG_TAG, 'Error: ', e); + log.error(LOG_TAG, 'Failure sending:', msg); + log.error(LOG_TAG, 'Error:', e); throw e; } } else { - log.debug( - LOG_TAG, - 'Not sending because gRPC stream is closed:', - util.inspect(msg, { depth: 100 }) - ); + log.debug(LOG_TAG, 'Not sending because gRPC stream is closed:', msg); } }, closeFn: () => { + log.debug(LOG_TAG, 'GRPC stream closed locally via close().'); close(); } }); @@ -226,47 +251,44 @@ export class GrpcConnection implements Connection { grpcStream.on('data', (msg: {}) => { if (!closed) { - log.debug( - LOG_TAG, - 'GRPC stream received: ', - util.inspect(msg, { depth: 100 }) - ); + log.debug(LOG_TAG, 'GRPC stream received:', msg); stream.callOnMessage(msg); } }); grpcStream.on('end', () => { log.debug(LOG_TAG, 'GRPC stream ended.'); - // The server closed the remote end. Close our side too (which will - // trigger the 'finish' event). - remoteEnded = true; - grpcStream.end(); + close(); }); grpcStream.on('finish', () => { - // This means we've closed the write side of the stream. We either did - // this because the StreamBridge was close()ed or because we got an 'end' - // event from the grpcStream. - - // TODO(mikelehen): This is a hack because of weird grpc-node behavior - // (https://github.com/grpc/grpc/issues/7705). The stream may be finished - // because we called end() because we got an 'end' event because there was - // an error. Now that we've called end(), GRPC should deliver the error, - // but it may take some time (e.g. 700ms). So we delay our close handling - // in case we receive such an error. - if (remoteEnded) { - setTimeout(close, 2500); - } else { - close(); - } + // TODO(mikelehen): I *believe* this assert is safe and we can just remove + // the 'finish' event if we don't see the assert getting hit for a while. + assert(closed, 'Received "finish" event without close() being called.'); }); grpcStream.on('error', (grpcError: GrpcError) => { - log.debug(LOG_TAG, 'GRPC stream error:', grpcError); + log.debug( + LOG_TAG, + 'GRPC stream error. Code:', + grpcError.code, + 'Message:', + grpcError.message + ); const code = mapCodeFromRpcCode(grpcError.code); close(new FirestoreError(code, grpcError.message)); }); + grpcStream.on('status', (status: GrpcStatus) => { + // TODO(mikelehen): I *believe* this assert is safe and we can just remove + // the 'status' event if we don't see the assert getting hit for a while. + assert( + closed, + `status event received before "end" or "error". ` + + `code: ${status.code} details: ${status.details}` + ); + }); + grpcStream.on('status', (status: GrpcStatus) => { if (!closed) { log.debug(LOG_TAG, 'GRPC stream received status:', status); diff --git a/packages/firestore/src/platform_node/node_platform.ts b/packages/firestore/src/platform_node/node_platform.ts index ee21577f950..f2de0a58775 100644 --- a/packages/firestore/src/platform_node/node_platform.ts +++ b/packages/firestore/src/platform_node/node_platform.ts @@ -14,6 +14,8 @@ * limitations under the License. */ +import * as util from 'util'; + import { DatabaseId, DatabaseInfo } from '../core/database_info'; import { Platform } from '../platform/platform'; import { Connection } from '../remote/connection'; @@ -22,6 +24,7 @@ import { Code, FirestoreError } from '../util/error'; import { GrpcConnection } from './grpc_connection'; import { loadProtosAsync } from './load_protos'; +import { AnyJs } from '../util/misc'; export class NodePlatform implements Platform { readonly base64Available = true; @@ -38,6 +41,11 @@ export class NodePlatform implements Platform { return new JsonProtoSerializer(partitionId, { useProto3Json: false }); } + formatJSON(value: AnyJs): string { + // util.inspect() results in much more readable output than JSON.stringify() + return util.inspect(value, { depth: 100 }); + } + atob(encoded: string): string { // Node actually doesn't validate base64 strings. // A quick sanity check that is not a fool-proof validation diff --git a/packages/firestore/src/remote/connection.ts b/packages/firestore/src/remote/connection.ts index 20d9f9af958..3088f72734d 100644 --- a/packages/firestore/src/remote/connection.ts +++ b/packages/firestore/src/remote/connection.ts @@ -41,7 +41,24 @@ export interface Connection { * @param token the Token to use for the RPC. * @return a Promise containing the JSON object encoding of the response */ - invoke(rpcName: string, request: any, token: Token | null): Promise; + invokeRPC(rpcName: string, request: any, token: Token | null): Promise; + + /** + * Invokes a streaming RPC by name, given a request message as a JavaScript + * object representing the JSON to send. The responses will be consumed to + * completion and then returned as an array. + * + * @param rpcName the name of the RPC to invoke + * @param request the Raw JSON object encoding of the request message + * @param token the Token to use for the RPC. + * @return a Promise containing an array with the JSON object encodings of the + * responses + */ + invokeStreamingRPC( + rpcName: string, + request: any, + token: Token | null + ): Promise; /** * Opens a stream to the given stream RPC endpoint. Returns a stream which diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 26cb53507bc..baf02e48982 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -82,10 +82,11 @@ export class Datastore { commit(mutations: Mutation[]): Promise { const params: CommitRequest = { + database: this.serializer.encodedDatabaseId, writes: mutations.map(m => this.serializer.toMutation(m)) }; return this.invokeRPC( - 'commit', + 'Commit', params ).then((response: api.CommitResponse) => { return this.serializer.fromWriteResults(response.writeResults); @@ -94,10 +95,11 @@ export class Datastore { lookup(keys: DocumentKey[]): Promise { const params: BatchGetDocumentsRequest = { + database: this.serializer.encodedDatabaseId, documents: keys.map(k => this.serializer.toName(k)) }; - return this.invokeRPC( - 'batchGet', + return this.invokeStreamingRPC( + 'BatchGetDocuments', params ).then((response: api.BatchGetDocumentsResponse[]) => { let docs = maybeDocumentMap(); @@ -119,7 +121,15 @@ export class Datastore { private invokeRPC(rpcName: string, request: any): Promise { // TODO(mikelehen): Retry (with backoff) on token failures? return this.credentials.getToken(/*forceRefresh=*/ false).then(token => { - return this.connection.invoke(rpcName, request, token); + return this.connection.invokeRPC(rpcName, request, token); + }); + } + + /** Gets an auth token and invokes the provided RPC with streamed results. */ + private invokeStreamingRPC(rpcName: string, request: any): Promise { + // TODO(mikelehen): Retry (with backoff) on token failures? + return this.credentials.getToken(/*forceRefresh=*/ false).then(token => { + return this.connection.invokeStreamingRPC(rpcName, request, token); }); } } diff --git a/packages/firestore/src/remote/rpc_error.ts b/packages/firestore/src/remote/rpc_error.ts index 3230769f7fb..1cde2930f0d 100644 --- a/packages/firestore/src/remote/rpc_error.ts +++ b/packages/firestore/src/remote/rpc_error.ts @@ -16,6 +16,7 @@ import { fail } from '../util/assert'; import { Code } from '../util/error'; +import * as log from '../util/log'; /** * Error Codes describing the different ways GRPC can fail. These are copied @@ -103,7 +104,14 @@ export function mapCodeFromRpcStatus(status: string): Code | undefined { * @returns The Code equivalent to the given GRPC status code. Fails if there * is no match. */ -export function mapCodeFromRpcCode(code: number): Code { +export function mapCodeFromRpcCode(code: number | undefined): Code { + if (code === undefined) { + // This shouldn't normally happen, but in certain error cases (like trying + // to send invalid proto messages) we may get an error with no GRPC code. + log.error('GRPC error has no .code'); + return Code.UNKNOWN; + } + switch (code) { case RpcCode.OK: return Code.OK; diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index e2a602e4c6d..d834da5a80f 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -150,6 +150,41 @@ export class JsonProtoSerializer { return new FirestoreError(code, status.message || ''); } + /** + * Returns a value for a number (or undefined) that's appropriate to put into + * a google.protobuf.Int32Value proto. + * DO NOT USE THIS FOR ANYTHING ELSE. + * This method cheats. It's typed as returning "number" because that's what + * our generated proto interfaces say Int32Value must be. But GRPC actually + * expects a { value: } struct. + */ + private toInt32Value(val: number | null): number | undefined { + if (!typeUtils.isNullOrUndefined(val)) { + return { value: val } as any; + } else { + return undefined; + } + } + + /** + * Returns a number (or null) from a google.protobuf.Int32Value proto. + * DO NOT USE THIS FOR ANYTHING ELSE. + * This method cheats. It's typed as accepting "number" because that's what + * our generated proto interfaces say Int32Value must be, but it actually + * accepts { value: number } to match our serialization in toInt32Value(). + */ + private fromInt32Value(val: number | undefined): number | null { + let result; + if (typeof val === 'object') { + result = (val as any).value; + } else { + // We accept raw numbers (without the {value: ... } wrapper) for + // compatibility with legacy persisted data. + result = val; + } + return typeUtils.isNullOrUndefined(result) ? null : result; + } + /** * Returns a value for a Date that's appropriate to put into a proto. * DO NOT USE THIS FOR ANYTHING ELSE. @@ -529,7 +564,7 @@ export class JsonProtoSerializer { fromMaybeDocument(result: api.BatchGetDocumentsResponse): MaybeDocument { // tslint:disable-next-line:no-any - const type = (result as any)['result_type']; + const type = (result as any)['result']; if (hasTag(result, type, 'found')) { return this.fromFound(result); } else if (hasTag(result, type, 'missing')) { @@ -620,7 +655,7 @@ export class JsonProtoSerializer { fromWatchChange(change: api.ListenResponse): WatchChange { // tslint:disable-next-line:no-any - const type = (change as any)['change_type']; + const type = (change as any)['response_type']; let watchChange: WatchChange; if (hasTag(change, type, 'targetChange')) { assertPresent(change.targetChange, 'targetChange'); @@ -831,7 +866,7 @@ export class JsonProtoSerializer { ? this.fromVersion(proto.updateTime) : null; let transformResults: fieldValue.FieldValue[] | null = null; - if (proto.transformResults) { + if (proto.transformResults && proto.transformResults.length > 0) { transformResults = proto.transformResults.map(result => this.fromValue(result) ); @@ -902,7 +937,7 @@ export class JsonProtoSerializer { result.structuredQuery!.orderBy = orderBy; } - const limit = this.toLimit(query.limit); + const limit = this.toInt32Value(query.limit); if (limit !== undefined) { result.structuredQuery!.limit = limit; } @@ -943,7 +978,7 @@ export class JsonProtoSerializer { let limit: number | null = null; if (query.limit) { - limit = query.limit; + limit = this.fromInt32Value(query.limit); } let startAt: Bound | null = null; @@ -1045,13 +1080,6 @@ export class JsonProtoSerializer { return orderBys.map(order => this.fromPropertyOrder(order)); } - private toLimit(limit: number | null): number | undefined { - if (!typeUtils.isNullOrUndefined(limit)) { - return limit!; - } - return; - } - private toCursor(cursor: Bound): api.Cursor { return { before: cursor.before, diff --git a/packages/firestore/src/typings/grpc.d.ts b/packages/firestore/src/typings/grpc.d.ts index 458050b464b..69f3aaf443a 100644 --- a/packages/firestore/src/typings/grpc.d.ts +++ b/packages/firestore/src/typings/grpc.d.ts @@ -18,7 +18,7 @@ * Preliminary and highly incomplete typings for GRPC. */ declare module 'grpc' { - export function loadObject(ns: any): {}; + export function loadObject(ns: any): any; export class Metadata { set(key: string, value: string): void; diff --git a/packages/firestore/src/util/log.ts b/packages/firestore/src/util/log.ts index e47d0dd6549..579d76743c2 100644 --- a/packages/firestore/src/util/log.ts +++ b/packages/firestore/src/util/log.ts @@ -18,6 +18,7 @@ import { SDK_VERSION } from '../core/version'; import { AnyJs } from './misc'; +import { PlatformSupport } from '../platform/platform'; export enum LogLevel { DEBUG, @@ -58,8 +59,9 @@ function argToString(obj: AnyJs): string | AnyJs { if (typeof obj === 'string') { return obj; } else { + const platform = PlatformSupport.getPlatform(); try { - return JSON.stringify(obj); + return platform.formatJSON(obj); } catch (e) { // Converting to JSON failed, just log the object directly return obj; diff --git a/packages/firestore/test/integration/api/batch_writes.test.ts b/packages/firestore/test/integration/api/batch_writes.test.ts index aed112fdb1d..1a389cc5049 100644 --- a/packages/firestore/test/integration/api/batch_writes.test.ts +++ b/packages/firestore/test/integration/api/batch_writes.test.ts @@ -183,6 +183,11 @@ apiDescribe('Database batch writes', persistence => { .update(docB, { b: 2 }) .commit(); + // Node logs warnings if you don't attach an error handler to a + // Promise before it fails, so attach a dummy one here (we handle + // the rejection for real below). + batchCommitPromise.catch(err => {}); + return accumulator.awaitEvent(); }) .then(localSnap => { diff --git a/packages/firestore/test/integration/bootstrap.ts b/packages/firestore/test/integration/bootstrap.ts index 57d83a248a6..4efe658cbf6 100644 --- a/packages/firestore/test/integration/bootstrap.ts +++ b/packages/firestore/test/integration/bootstrap.ts @@ -14,11 +14,11 @@ * limitations under the License. */ -import '../../src/platform_browser/browser_init'; +import '../../index'; /** * This will include all of the test files and compile them as needed - * + * * Taken from karma-webpack source: * https://github.com/webpack-contrib/karma-webpack#alternative-usage */ diff --git a/packages/firestore/test/integration/browser/indexeddb.test.ts b/packages/firestore/test/integration/browser/indexeddb.test.ts index 4ad5c61460f..ff37de62e0d 100644 --- a/packages/firestore/test/integration/browser/indexeddb.test.ts +++ b/packages/firestore/test/integration/browser/indexeddb.test.ts @@ -20,7 +20,8 @@ import * as firestore from 'firestore'; import { isPersistenceAvailable, withTestDb } from '../util/helpers'; describe('where persistence is unsupported, enablePersistence', () => { - // Only test on browsers where persistence is *not* available (e.g. Edge). + // Only test on platforms where persistence is *not* available (e.g. Edge, + // Node.JS). if (isPersistenceAvailable()) { return; } @@ -42,11 +43,15 @@ describe('where persistence is unsupported, enablePersistence', () => { it('falls back without requiring a wait for the promise', () => { return withTestDb(/* persistence= */ false, db => { - // Disregard the promise here intentionally. - db.enablePersistence(); + const persistenceFailedPromise = db + .enablePersistence() + .catch((err: firestore.FirestoreError) => { + expect(err.code).to.equal('unimplemented'); + }); + // Do the set immediately without waiting on the promise. const doc = db.collection('test-collection').doc(); - return doc.set({ foo: 'bar' }); + return doc.set({ foo: 'bar' }).then(() => persistenceFailedPromise); }); }); }); diff --git a/packages/firestore/test/integration/browser/webchannel.test.ts b/packages/firestore/test/integration/browser/webchannel.test.ts index 87d60fc7e6c..6f558a93518 100644 --- a/packages/firestore/test/integration/browser/webchannel.test.ts +++ b/packages/firestore/test/integration/browser/webchannel.test.ts @@ -20,6 +20,11 @@ import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info'; import * as utilHelpers from '../util/helpers'; describe('WebChannel', () => { + if (typeof window === 'undefined') { + console.warn('Skipping WebChannel tests.'); + return; + } + describe('makeUrl', () => { const info = new DatabaseInfo( new DatabaseId('testproject'), @@ -31,7 +36,7 @@ describe('WebChannel', () => { const makeUrl = conn.makeUrl.bind(conn); it('includes project ID and database ID', () => { - const url = makeUrl('commit', {}); + const url = makeUrl('Commit', {}); expect(url).to.equal( 'http://example.com/v1beta1/projects/testproject/' + 'databases/(default)/documents:commit' diff --git a/packages/firestore/test/integration/util/firebase_export.ts b/packages/firestore/test/integration/util/firebase_export.ts index f75cbd1c136..54d05446113 100644 --- a/packages/firestore/test/integration/util/firebase_export.ts +++ b/packages/firestore/test/integration/util/firebase_export.ts @@ -20,7 +20,6 @@ // instead. import firebase from '@firebase/app'; -import '../../../index'; // TODO(b/66917182): This "as any" removes all of our type-checking in tests and // is therefore pretty bad. But I can't figure out how to avoid it right now. diff --git a/packages/firestore/test/integration/util/helpers.ts b/packages/firestore/test/integration/util/helpers.ts index 20eff2e0090..10ae6562971 100644 --- a/packages/firestore/test/integration/util/helpers.ts +++ b/packages/firestore/test/integration/util/helpers.ts @@ -35,7 +35,7 @@ export const ALT_PROJECT_ID = 'test-db2'; const DEFAULT_SETTINGS = getDefaultSettings(); function getDefaultSettings(): firestore.Settings { - const karma = __karma__; //typeof __karma__ !== 'undefined' ? __karma__ : undefined; + const karma = typeof __karma__ !== 'undefined' ? __karma__ : undefined; if (karma && karma.config.firestoreSettings) { return karma.config.firestoreSettings; } else { @@ -46,6 +46,10 @@ function getDefaultSettings(): firestore.Settings { } } +function isBrowser(): boolean { + return typeof window !== 'undefined'; +} + function isIeOrEdge(): boolean { const ua = window.navigator.userAgent; return ( @@ -56,7 +60,7 @@ function isIeOrEdge(): boolean { } export function isPersistenceAvailable(): boolean { - return !isIeOrEdge(); + return isBrowser() && !isIeOrEdge(); } /** diff --git a/packages/firestore/test/unit/remote/node/serializer.test.ts b/packages/firestore/test/unit/remote/node/serializer.test.ts index c24a33538d8..d473db96d12 100644 --- a/packages/firestore/test/unit/remote/node/serializer.test.ts +++ b/packages/firestore/test/unit/remote/node/serializer.test.ts @@ -72,7 +72,7 @@ import { } from '../../../util/helpers'; import { loadProtos } from '../../../../src/platform_node/load_protos'; -describe('Serializer Beta', () => { +describe('Serializer', () => { const partition = new DatabaseId('p', 'd'); const s = new JsonProtoSerializer(partition, { useProto3Json: false }); const emptyResumeToken = new Uint8Array(0); @@ -1056,7 +1056,7 @@ describe('Serializer Beta', () => { direction: 'ASCENDING' } ], - limit: 26 + limit: { value: 26 } } }, targetId: 1 diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 91b842033f9..c7dd4e7d570 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -133,7 +133,11 @@ class MockConnection implements Connection { this.activeTargets = []; } - invoke(rpcName: string, request: any): Promise { + invokeRPC(rpcName: string, request: any): Promise { + throw new Error('Not implemented!'); + } + + invokeStreamingRPC(rpcName: string, request: any): Promise { throw new Error('Not implemented!'); } From 71e547c48ae60e561f7b94cf2e1a43f7964b7999 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Thu, 16 Nov 2017 16:56:25 -0800 Subject: [PATCH 2/5] upgrade grpc --- packages/firestore/package.json | 2 +- .../src/platform_node/grpc_connection.ts | 5 +- packages/firestore/src/typings/grpc.d.ts | 52 ------------------- yarn.lock | 40 ++++++++++---- 4 files changed, 35 insertions(+), 64 deletions(-) delete mode 100644 packages/firestore/src/typings/grpc.d.ts diff --git a/packages/firestore/package.json b/packages/firestore/package.json index 515958a4780..ba7d6f111ff 100644 --- a/packages/firestore/package.json +++ b/packages/firestore/package.json @@ -19,7 +19,7 @@ "license": "Apache-2.0", "dependencies": { "@firebase/webchannel-wrapper": "^0.2.4", - "grpc": "^1.6.6", + "grpc": "^1.7.1", "protobufjs": "^5.0.0" }, "peerDependencies": { diff --git a/packages/firestore/src/platform_node/grpc_connection.ts b/packages/firestore/src/platform_node/grpc_connection.ts index a8724cdb452..4a0d40fa285 100644 --- a/packages/firestore/src/platform_node/grpc_connection.ts +++ b/packages/firestore/src/platform_node/grpc_connection.ts @@ -54,7 +54,10 @@ function createHeaders(databaseInfo: DatabaseInfo, token: Token | null): {} { : grpc.credentials.createInsecure(); const callCredentials = grpc.credentials.createFromMetadataGenerator( - (context: { serviceUrl: string }, cb: grpc.MetadataCallback) => { + ( + context: { service_url: string }, + cb: (error: Error | null, metadata?: grpc.Metadata) => void + ) => { const metadata = new grpc.Metadata(); if (token) { for (const header in token.authHeaders) { diff --git a/packages/firestore/src/typings/grpc.d.ts b/packages/firestore/src/typings/grpc.d.ts deleted file mode 100644 index 69f3aaf443a..00000000000 --- a/packages/firestore/src/typings/grpc.d.ts +++ /dev/null @@ -1,52 +0,0 @@ -/** - * Copyright 2017 Google Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/** - * Preliminary and highly incomplete typings for GRPC. - */ -declare module 'grpc' { - export function loadObject(ns: any): any; - - export class Metadata { - set(key: string, value: string): void; - } - - export type MetadataCallback = (err: Error | null, data: Metadata) => void; - - export type MetadataGenerator = ( - context: { serviceUrl: string }, - cb: MetadataCallback - ) => void; - - // static methods on grpc.credentials - export interface CredentialsStatic { - createInsecure(): ChannelCredentials; - - createFromMetadataGenerator(generator: MetadataGenerator): CallCredentials; - createSsl(): ChannelCredentials; - createFromGoogleCredential(credential: {}): CallCredentials; - combineChannelCredentials( - channelCreds: ChannelCredentials, - callCreds: CallCredentials - ): ChannelCredentials; - } - - export interface CallCredentials {} - - export let credentials: CredentialsStatic; - - export interface ChannelCredentials {} -} diff --git a/yarn.lock b/yarn.lock index 87b1dc4b8ac..32c9a4bda91 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2182,6 +2182,10 @@ detect-indent@^5.0.0: version "5.0.0" resolved "https://registry.yarnpkg.com/detect-indent/-/detect-indent-5.0.0.tgz#3871cc0a6a002e8c3e5b3cf7f336264675f06b9d" +detect-libc@^1.0.2: + version "1.0.2" + resolved "https://registry.yarnpkg.com/detect-libc/-/detect-libc-1.0.2.tgz#71ad5d204bf17a6a6ca8f450c61454066ef461e1" + detect-newline@2.X: version "2.1.0" resolved "https://registry.yarnpkg.com/detect-newline/-/detect-newline-2.1.0.tgz#f41f1c10be4b00e87b5f13da680759f2c5bfd3e2" @@ -3639,15 +3643,15 @@ grpc@1.4.1: node-pre-gyp "^0.6.35" protobufjs "^5.0.0" -grpc@^1.6.6: - version "1.6.6" - resolved "https://registry.yarnpkg.com/grpc/-/grpc-1.6.6.tgz#2051784f6bd6134681fa2c4b5e75dc82c6c23ffa" +grpc@^1.7.1: + version "1.7.1" + resolved "https://registry.yarnpkg.com/grpc/-/grpc-1.7.1.tgz#a1eecd074e78ffe5bb3bb64dcc7417d14fdb5cc4" dependencies: arguejs "^0.2.3" - lodash "^4.17.4" - nan "^2.7.0" - node-pre-gyp "^0.6.38" - protobufjs "^5.0.2" + lodash "^4.15.0" + nan "^2.0.0" + node-pre-gyp "^0.6.39" + protobufjs "^5.0.0" gtoken@^1.2.1: version "1.2.3" @@ -5750,7 +5754,7 @@ mz@^2.7.0: object-assign "^4.0.1" thenify-all "^1.0.0" -nan@^2.0.0, nan@^2.3.0, nan@^2.7.0: +nan@^2.0.0, nan@^2.3.0: version "2.7.0" resolved "https://registry.yarnpkg.com/nan/-/nan-2.7.0.tgz#d95bf721ec877e08db276ed3fc6eb78f9083ad46" @@ -5848,7 +5852,7 @@ node-localstorage@^1.3.0: dependencies: write-file-atomic "^1.1.4" -node-pre-gyp@^0.6.35, node-pre-gyp@^0.6.36, node-pre-gyp@^0.6.38: +node-pre-gyp@^0.6.35, node-pre-gyp@^0.6.36: version "0.6.38" resolved "https://registry.yarnpkg.com/node-pre-gyp/-/node-pre-gyp-0.6.38.tgz#e92a20f83416415bb4086f6d1fb78b3da73d113d" dependencies: @@ -5863,6 +5867,22 @@ node-pre-gyp@^0.6.35, node-pre-gyp@^0.6.36, node-pre-gyp@^0.6.38: tar "^2.2.1" tar-pack "^3.4.0" +node-pre-gyp@^0.6.39: + version "0.6.39" + resolved "https://registry.yarnpkg.com/node-pre-gyp/-/node-pre-gyp-0.6.39.tgz#c00e96860b23c0e1420ac7befc5044e1d78d8649" + dependencies: + detect-libc "^1.0.2" + hawk "3.1.3" + mkdirp "^0.5.1" + nopt "^4.0.1" + npmlog "^4.0.2" + rc "^1.1.7" + request "2.81.0" + rimraf "^2.6.1" + semver "^5.3.0" + tar "^2.2.1" + tar-pack "^3.4.0" + node-status-codes@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/node-status-codes/-/node-status-codes-1.0.0.tgz#5ae5541d024645d32a58fcddc9ceecea7ae3ac2f" @@ -6497,7 +6517,7 @@ prompt@1.0.0: utile "0.3.x" winston "2.1.x" -protobufjs@^5.0.0, protobufjs@^5.0.2: +protobufjs@^5.0.0: version "5.0.2" resolved "https://registry.yarnpkg.com/protobufjs/-/protobufjs-5.0.2.tgz#59748d7dcf03d2db22c13da9feb024e16ab80c91" dependencies: From 5dd5a6f1f371901bd1c6b8173a48851a9f42b5f5 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Fri, 17 Nov 2017 12:11:41 -0800 Subject: [PATCH 3/5] Remove protobufjs dependency (load protos via GRPC) and deal with typings. --- packages/firestore/package.json | 3 +- .../src/platform_node/grpc_connection.ts | 7 +--- .../src/platform_node/load_protos.ts | 39 ++++--------------- .../src/platform_node/node_platform.ts | 7 ++-- .../test/unit/remote/node/serializer.test.ts | 8 ++-- 5 files changed, 17 insertions(+), 47 deletions(-) diff --git a/packages/firestore/package.json b/packages/firestore/package.json index ba7d6f111ff..a32d2d704cf 100644 --- a/packages/firestore/package.json +++ b/packages/firestore/package.json @@ -19,8 +19,7 @@ "license": "Apache-2.0", "dependencies": { "@firebase/webchannel-wrapper": "^0.2.4", - "grpc": "^1.7.1", - "protobufjs": "^5.0.0" + "grpc": "^1.7.1" }, "peerDependencies": { "@firebase/app": "^0.1.0" diff --git a/packages/firestore/src/platform_node/grpc_connection.ts b/packages/firestore/src/platform_node/grpc_connection.ts index 4a0d40fa285..9c11587143e 100644 --- a/packages/firestore/src/platform_node/grpc_connection.ts +++ b/packages/firestore/src/platform_node/grpc_connection.ts @@ -15,7 +15,6 @@ */ import * as grpc from 'grpc'; -import * as protobufjs from 'protobufjs'; import firebase from '@firebase/app'; const SDK_VERSION = firebase.SDK_VERSION; @@ -32,7 +31,6 @@ import { FirestoreError } from '../util/error'; import * as log from '../util/log'; import { AnyJs } from '../util/misc'; import { NodeCallback, nodePromise } from '../util/node_api'; -import { ProtobufProtoBuilder } from './load_protos'; import { Deferred } from '../util/promise'; const LOG_TAG = 'Connection'; @@ -117,11 +115,10 @@ export class GrpcConnection implements Connection { private cachedStub: CachedStub | null = null; constructor( - builder: ProtobufProtoBuilder, + protos: grpc.GrpcObject, private databaseInfo: DatabaseInfo ) { - const protos = grpc.loadObject(builder.ns); - this.firestore = protos.google.firestore.v1beta1; + this.firestore = protos['google']['firestore']['v1beta1']; } private sameToken(tokenA: Token | null, tokenB: Token | null): boolean { diff --git a/packages/firestore/src/platform_node/load_protos.ts b/packages/firestore/src/platform_node/load_protos.ts index b8de02f123b..d56c4cb29a3 100644 --- a/packages/firestore/src/platform_node/load_protos.ts +++ b/packages/firestore/src/platform_node/load_protos.ts @@ -14,48 +14,23 @@ * limitations under the License. */ -const dynamicRequire = require; -const protobufjs = dynamicRequire('protobufjs'); -export type ProtobufProtoBuilder = any; - -import { NodeCallback, nodePromise } from '../util/node_api'; - -export function loadProtosAsync(): Promise { - return nodePromise((callback: NodeCallback) => { - loadProtos(callback); - }); -} +import * as grpc from 'grpc'; /** - * Loads the protocol buffer definitions for the datastore. This is a thin - * wrapper around protobufjs.loadProtoFile which knows the location of the - * proto files. + * Loads the protocol buffer definitions for Firestore. * - * @param callback if specified, the load is performed asynchronously and - * the protos are supplied to the callback. - * @returns the ProtoBuilder if the callback is unspecified. + * @returns The GrpcObject representing our protos. */ -export function loadProtos( - callback?: NodeCallback -): ProtobufProtoBuilder | undefined { - const builder = protobufjs.newBuilder({ +export function loadProtos(): grpc.GrpcObject { + const options = { // Beware that converting fields to camel case does not convert the tag // fields in oneof groups (!!!). convertFieldsToCamelCase: true - }); + }; const root = __dirname + '/../protos'; const firestoreProtoFile = { root: root, file: 'google/firestore/v1beta1/firestore.proto' }; - if (callback === undefined) { - // Synchronous load - return protobufjs.loadProtoFile(firestoreProtoFile, undefined, builder); - } else { - // Load the protos asynchronously - protobufjs.loadProtoFile(firestoreProtoFile, callback, builder); - // We are using the callback so no return value, but we need to explicitly - // return undefined - return undefined; - } + return grpc.load(firestoreProtoFile, /*format=*/'proto', options); } diff --git a/packages/firestore/src/platform_node/node_platform.ts b/packages/firestore/src/platform_node/node_platform.ts index f2de0a58775..8b507ddf6c5 100644 --- a/packages/firestore/src/platform_node/node_platform.ts +++ b/packages/firestore/src/platform_node/node_platform.ts @@ -23,7 +23,7 @@ import { JsonProtoSerializer } from '../remote/serializer'; import { Code, FirestoreError } from '../util/error'; import { GrpcConnection } from './grpc_connection'; -import { loadProtosAsync } from './load_protos'; +import { loadProtos } from './load_protos'; import { AnyJs } from '../util/misc'; export class NodePlatform implements Platform { @@ -32,9 +32,8 @@ export class NodePlatform implements Platform { readonly emptyByteString = new Uint8Array(0); loadConnection(databaseInfo: DatabaseInfo): Promise { - return loadProtosAsync().then(protos => { - return new GrpcConnection(protos, databaseInfo); - }); + const protos = loadProtos(); + return Promise.resolve(new GrpcConnection(protos, databaseInfo)); } newSerializer(partitionId: DatabaseId): JsonProtoSerializer { diff --git a/packages/firestore/test/unit/remote/node/serializer.test.ts b/packages/firestore/test/unit/remote/node/serializer.test.ts index d473db96d12..f063be7651f 100644 --- a/packages/firestore/test/unit/remote/node/serializer.test.ts +++ b/packages/firestore/test/unit/remote/node/serializer.test.ts @@ -76,8 +76,8 @@ describe('Serializer', () => { const partition = new DatabaseId('p', 'd'); const s = new JsonProtoSerializer(partition, { useProto3Json: false }); const emptyResumeToken = new Uint8Array(0); - const protos: any = loadProtos().build(); - const ds = protos.google.firestore.v1beta1; + const protos = loadProtos(); + const ds = protos['google']['firestore']['v1beta1']; /** * Wraps the given query in QueryData. This is useful because the APIs we're @@ -205,7 +205,7 @@ describe('Serializer', () => { const expected = { timestampValue: expectedJson[i] }; expect(obj).to.deep.equal(expected); expectValue(obj, 'timestampValue').to.deep.equal( - new protos.google.protobuf.Timestamp(expectedJson[i]), + new protos['google']['protobuf'].Timestamp(expectedJson[i]), 'for date ' + example ); } @@ -224,7 +224,7 @@ describe('Serializer', () => { const obj = s.toValue(new fieldValue.GeoPointValue(example)); expect(obj).to.deep.equal(expected); expectValue(obj, 'geoPointValue').to.deep.equal( - new protos.google.type.LatLng(1.23, 4.56) + new protos['google']['type'].LatLng(1.23, 4.56) ); }); From 5da1096cc9f40402a7450d28ca18e6d1ebac5f4a Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Fri, 17 Nov 2017 13:56:33 -0800 Subject: [PATCH 4/5] Code review tweaks. --- .../platform_browser/webchannel_connection.ts | 4 ++-- .../src/platform_node/grpc_connection.ts | 16 ++-------------- packages/firestore/src/remote/connection.ts | 2 +- .../test/integration/browser/webchannel.test.ts | 8 ++------ 4 files changed, 7 insertions(+), 23 deletions(-) diff --git a/packages/firestore/src/platform_browser/webchannel_connection.ts b/packages/firestore/src/platform_browser/webchannel_connection.ts index c5a216ef417..637cadf0014 100644 --- a/packages/firestore/src/platform_browser/webchannel_connection.ts +++ b/packages/firestore/src/platform_browser/webchannel_connection.ts @@ -171,7 +171,7 @@ export class WebChannelConnection implements Connection { rpcName: string, request: any, token: Token | null - ): Promise { + ): Promise { // The REST API automatically aggregates all of the streamed results, so we // can just use the normal invoke() method. return this.invokeRPC(rpcName, request, token); @@ -338,7 +338,7 @@ export class WebChannelConnection implements Connection { // visible for testing makeUrl(rpcName: string): string { const urlRpcName = RPC_NAME_REST_MAPPING[rpcName]; - assert(urlRpcName != null, 'Unknown REST mapping for: ' + rpcName); + assert(urlRpcName !== undefined, 'Unknown REST mapping for: ' + rpcName); const url = [this.baseUrl, '/', RPC_URL_VERSION]; url.push('/projects/'); url.push(this.databaseId.projectId); diff --git a/packages/firestore/src/platform_node/grpc_connection.ts b/packages/firestore/src/platform_node/grpc_connection.ts index 03b9ab29477..145b2a31889 100644 --- a/packages/firestore/src/platform_node/grpc_connection.ts +++ b/packages/firestore/src/platform_node/grpc_connection.ts @@ -179,10 +179,10 @@ export class GrpcConnection implements Connection { rpcName: string, request: any, token: Token | null - ): Promise { + ): Promise { const rpc = this.getRpc(rpcName, token); const results = []; - const responseDeferred = new Deferred(); + const responseDeferred = new Deferred(); log.debug( LOG_TAG, @@ -287,18 +287,6 @@ export class GrpcConnection implements Connection { ); }); - grpcStream.on('status', (status: GrpcStatus) => { - if (!closed) { - log.debug(LOG_TAG, 'GRPC stream received status:', status); - if (status.code === 0) { - // all good - } else { - const code = mapCodeFromRpcCode(status.code); - close(new FirestoreError(code, status.details)); - } - } - }); - log.debug(LOG_TAG, 'Opening GRPC stream'); // TODO(dimond): Since grpc has no explicit open status (or does it?) we // simulate an onOpen in the next loop after the stream had it's listeners diff --git a/packages/firestore/src/remote/connection.ts b/packages/firestore/src/remote/connection.ts index 3088f72734d..257d3b17313 100644 --- a/packages/firestore/src/remote/connection.ts +++ b/packages/firestore/src/remote/connection.ts @@ -58,7 +58,7 @@ export interface Connection { rpcName: string, request: any, token: Token | null - ): Promise; + ): Promise; /** * Opens a stream to the given stream RPC endpoint. Returns a stream which diff --git a/packages/firestore/test/integration/browser/webchannel.test.ts b/packages/firestore/test/integration/browser/webchannel.test.ts index 6f558a93518..bc077f687c2 100644 --- a/packages/firestore/test/integration/browser/webchannel.test.ts +++ b/packages/firestore/test/integration/browser/webchannel.test.ts @@ -19,12 +19,8 @@ import { WebChannelConnection } from '../../../src/platform_browser/webchannel_c import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info'; import * as utilHelpers from '../util/helpers'; -describe('WebChannel', () => { - if (typeof window === 'undefined') { - console.warn('Skipping WebChannel tests.'); - return; - } - +const describeFn = typeof window !== 'undefined' ? describe : xdescribe; +describeFn('WebChannel', () => { describe('makeUrl', () => { const info = new DatabaseInfo( new DatabaseId('testproject'), From 32fa31962cadd32a8d4a9d7e93aecc5fd9c5933f Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Mon, 20 Nov 2017 11:12:12 -0800 Subject: [PATCH 5/5] Add note about upgrading to protobufjs 6.x fixing casing in oneof tags. --- packages/firestore/src/platform_node/load_protos.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/firestore/src/platform_node/load_protos.ts b/packages/firestore/src/platform_node/load_protos.ts index 260e01f0504..ef0215221f0 100644 --- a/packages/firestore/src/platform_node/load_protos.ts +++ b/packages/firestore/src/platform_node/load_protos.ts @@ -24,7 +24,8 @@ import * as grpc from 'grpc'; export function loadProtos(): grpc.GrpcObject { const options = { // Beware that converting fields to camel case does not convert the tag - // fields in oneof groups (!!!). + // fields in oneof groups (!!!). This will likely be fixed when we upgrade + // to protobufjs 6.x convertFieldsToCamelCase: true }; const root = __dirname + '/../protos';