Review

Author: schmidt-sebastian

config/webpack.test.js

@@ -86,9 +86,10 @@ module.exports = {
     new webpack.NormalModuleReplacementPlugin(
       FIRESTORE_PLATFORM_RE,
       resource => {
+        const targetPlatform = process.env.TEST_PLATFORM || 'browser';
         resource.request = resource.request.replace(
           FIRESTORE_PLATFORM_RE,
-          '$1/platform/browser/$2.ts'
+          `$1/platform/${targetPlatform}/$2.ts`
         );
       }
     ),

packages/firestore/exp/test/bootstrap.ts

@@ -24,8 +24,6 @@ import '../register';
  * https://github.com/webpack-contrib/karma-webpack#alternative-usage
  */
 
-process.env.TEST_PLATFORM = 'browser';
-
 // 'context()' definition requires additional dependency on webpack-env package.
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 const testsContext = (require as any).context(

packages/firestore/karma.conf.js

@@ -55,6 +55,7 @@ function getTestFiles(argv) {
   } else if (argv.integration) {
     return [legcayIntegrationTests];
   } else if (argv.lite) {
+    process.env.TEST_PLATFORM = 'browser_lite';
     return [liteIntegrationTests];
   } else if (argv.exp) {
     return [expIntegrationTests];

packages/firestore/lite/test/bootstrap.ts

@@ -24,8 +24,6 @@ import '../register';
  * https://github.com/webpack-contrib/karma-webpack#alternative-usage
  */
 
-process.env.TEST_PLATFORM = 'browser-lite';
-
 // 'context()' definition requires additional dependency on webpack-env package.
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 const testsContext = (require as any).context('.', true, /^.*\.test.*$/);

packages/firestore/src/platform/browser_lite/connection.ts

@@ -23,5 +23,5 @@ export { newConnectivityMonitor } from '../browser/connection';
 
 /** Initializes the HTTP connection for the REST API. */
 export function newConnection(databaseInfo: DatabaseInfo): Promise<Connection> {
-  return Promise.resolve(new FetchConnection(databaseInfo, fetch));
+  return Promise.resolve(new FetchConnection(databaseInfo, fetch.bind(null)));
 }

packages/firestore/src/platform/node/grpc_connection.ts

@@ -46,10 +46,7 @@ const LOG_TAG = 'Connection';
 // should use the Firebase version instead.
 const X_GOOG_API_CLIENT_VALUE = `gl-node/${process.versions.node} fire/${SDK_VERSION} grpc/${grpcVersion}`;
 
-function createMetadata(
-  databaseInfo: DatabaseInfo,
-  token: Token | null
-): Metadata {
+function createMetadata(databasePath: string, token: Token | null): Metadata {
   hardAssert(
     token === null || token.type === 'OAuth',
     'If provided, token must be OAuth'
@@ -66,11 +63,7 @@ function createMetadata(
   metadata.set('x-goog-api-client', X_GOOG_API_CLIENT_VALUE);
   // This header is used to improve routing and project isolation by the
   // backend.
-  metadata.set(
-    'google-cloud-resource-prefix',
-    `projects/${databaseInfo.databaseId.projectId}/` +
-      `databases/${databaseInfo.databaseId.database}`
-  );
+  metadata.set('google-cloud-resource-prefix', databasePath);
   return metadata;
 }
 
@@ -83,15 +76,17 @@ type GeneratedGrpcStub = any;
  * A Connection implemented by GRPC-Node.
  */
 export class GrpcConnection implements Connection {
+  private readonly databasePath: string;
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  private firestore: any;
+  private readonly firestore: any;
 
   // We cache stubs for the most-recently-used token.
   private cachedStub: GeneratedGrpcStub | null = null;
 
   constructor(protos: GrpcObject, private databaseInfo: DatabaseInfo) {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     this.firestore = (protos as any)['google']['firestore']['v1'];
+    this.databasePath = `projects/${databaseInfo.databaseId.projectId}/databases/${databaseInfo.databaseId.database}`;
   }
 
   private ensureActiveStub(): GeneratedGrpcStub {
@@ -110,16 +105,18 @@ export class GrpcConnection implements Connection {
 
   invokeRPC<Req, Resp>(
     rpcName: string,
+    path: string,
     request: Req,
     token: Token | null
   ): Promise<Resp> {
     const stub = this.ensureActiveStub();
-    const metadata = createMetadata(this.databaseInfo, token);
+    const metadata = createMetadata(this.databasePath, token);
+    const jsonRequest = { database: this.databasePath, ...request };
 
     return nodePromise((callback: NodeCallback<Resp>) => {
       logDebug(LOG_TAG, `RPC '${rpcName}' invoked with request:`, request);
       return stub[rpcName](
-        request,
+        jsonRequest,
         metadata,
         (grpcError?: ServiceError, value?: Resp) => {
           if (grpcError) {
@@ -145,6 +142,7 @@ export class GrpcConnection implements Connection {
 
   invokeStreamingRPC<Req, Resp>(
     rpcName: string,
+    path: string,
     request: Req,
     token: Token | null
   ): Promise<Resp[]> {
@@ -157,8 +155,9 @@ export class GrpcConnection implements Connection {
       request
     );
     const stub = this.ensureActiveStub();
-    const metadata = createMetadata(this.databaseInfo, token);
-    const stream = stub[rpcName](request, metadata);
+    const metadata = createMetadata(this.databasePath, token);
+    const jsonRequest = { ...request, database: this.databasePath };
+    const stream = stub[rpcName](jsonRequest, metadata);
     stream.on('data', (response: Resp) => {
       logDebug(LOG_TAG, `RPC ${rpcName} received result:`, response);
       results.push(response);
@@ -182,7 +181,7 @@ export class GrpcConnection implements Connection {
     token: Token | null
   ): Stream<Req, Resp> {
     const stub = this.ensureActiveStub();
-    const metadata = createMetadata(this.databaseInfo, token);
+    const metadata = createMetadata(this.databasePath, token);
     const grpcStream = stub[rpcName](metadata);
 
     let closed = false;

packages/firestore/src/remote/connection.ts

@@ -38,12 +38,14 @@ export interface Connection {
    * representing the JSON to send.
    *
    * @param rpcName the name of the RPC to invoke
+   * @param path the path to invoke this RPC on
    * @param request the Raw JSON object encoding of the request message
    * @param token the Token to use for the RPC.
    * @return a Promise containing the JSON object encoding of the response
    */
   invokeRPC<Req, Resp>(
     rpcName: string,
+    path: string,
     request: Req,
     token: Token | null
   ): Promise<Resp>;
@@ -54,13 +56,15 @@ export interface Connection {
    * completion and then returned as an array.
    *
    * @param rpcName the name of the RPC to invoke
+   * @param path the path to invoke this RPC on
    * @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<Req, Resp>(
     rpcName: string,
+    path: string,
     request: Req,
     token: Token | null
   ): Promise<Resp[]>;

packages/firestore/src/remote/datastore.ts

@@ -82,12 +82,21 @@ class DatastoreImpl extends Datastore {
   }
 
   /** Gets an auth token and invokes the provided RPC. */
-  invokeRPC<Req, Resp>(rpcName: string, request: Req): Promise<Resp> {
+  invokeRPC<Req, Resp>(
+    rpcName: string,
+    path: string,
+    request: Req
+  ): Promise<Resp> {
     this.verifyInitialized();
     return this.credentials
       .getToken()
       .then(token => {
-        return this.connection.invokeRPC<Req, Resp>(rpcName, request, token);
+        return this.connection.invokeRPC<Req, Resp>(
+          rpcName,
+          path,
+          request,
+          token
+        );
       })
       .catch((error: FirestoreError) => {
         if (error.code === Code.UNAUTHENTICATED) {
@@ -100,6 +109,7 @@ class DatastoreImpl extends Datastore {
   /** Gets an auth token and invokes the provided RPC with streamed results. */
   invokeStreamingRPC<Req, Resp>(
     rpcName: string,
+    path: string,
     request: Req
   ): Promise<Resp[]> {
     this.verifyInitialized();
@@ -108,6 +118,7 @@ class DatastoreImpl extends Datastore {
       .then(token => {
         return this.connection.invokeStreamingRPC<Req, Resp>(
           rpcName,
+          path,
           request,
           token
         );
@@ -139,26 +150,26 @@ export async function invokeCommitRpc(
   mutations: Mutation[]
 ): Promise<void> {
   const datastoreImpl = debugCast(datastore, DatastoreImpl);
-  const params = {
-    database: getEncodedDatabaseId(datastoreImpl.serializer),
+  const path = getEncodedDatabaseId(datastoreImpl.serializer) + '/documents';
+  const request = {
     writes: mutations.map(m => toMutation(datastoreImpl.serializer, m))
   };
-  await datastoreImpl.invokeRPC('Commit', params);
+  await datastoreImpl.invokeRPC('Commit', path, request);
 }
 
 export async function invokeBatchGetDocumentsRpc(
   datastore: Datastore,
   keys: DocumentKey[]
 ): Promise<MaybeDocument[]> {
   const datastoreImpl = debugCast(datastore, DatastoreImpl);
-  const params = {
-    database: getEncodedDatabaseId(datastoreImpl.serializer),
+  const path = getEncodedDatabaseId(datastoreImpl.serializer) + '/documents';
+  const request = {
     documents: keys.map(k => toName(datastoreImpl.serializer, k))
   };
   const response = await datastoreImpl.invokeStreamingRPC<
     api.BatchGetDocumentsRequest,
     api.BatchGetDocumentsResponse
-  >('BatchGetDocuments', params);
+  >('BatchGetDocuments', path, request);
 
   const docs = new Map<string, MaybeDocument>();
   response.forEach(proto => {
@@ -179,21 +190,11 @@ export async function invokeRunQueryRpc(
   query: Query
 ): Promise<Document[]> {
   const datastoreImpl = debugCast(datastore, DatastoreImpl);
-  const { structuredQuery, parent } = toQueryTarget(
-    datastoreImpl.serializer,
-    queryToTarget(query)
-  );
-  const params = {
-    database: getEncodedDatabaseId(datastoreImpl.serializer),
-    parent,
-    structuredQuery
-  };
-
+  const request = toQueryTarget(datastoreImpl.serializer, queryToTarget(query));
   const response = await datastoreImpl.invokeStreamingRPC<
     api.RunQueryRequest,
     api.RunQueryResponse
-  >('RunQuery', params);
-
+  >('RunQuery', request.parent!, { structuredQuery: request.structuredQuery });
   return (
     response
       // Omit RunQueryResponses that only contain readTimes.

packages/firestore/src/remote/rest_connection.ts

@@ -23,7 +23,6 @@ import { logDebug, logWarn } from '../util/log';
 import { FirestoreError } from '../util/error';
 import { StringMap } from '../util/types';
 import { debugAssert } from '../util/assert';
-import { Indexable } from '../util/misc';
 
 const LOG_TAG = 'RestConnection';
 
@@ -64,29 +63,17 @@ export abstract class RestConnection implements Connection {
 
   invokeRPC<Req, Resp>(
     rpcName: string,
+    path: string,
     req: Req,
     token: Token | null
   ): Promise<Resp> {
-    const url = this.makeUrl(rpcName, req);
-
-    // The database and/or parent field is already encoded in URL. Specifying it
-    // again in the body is not necessary in production, and will cause
-    // duplicate field errors in the Firestore Emulator. Let's remove it.
-    const jsonObj = ({ ...req } as unknown) as Indexable;
-    delete jsonObj.parent;
-    delete jsonObj.database;
-
-    logDebug(LOG_TAG, 'Sending: ', url, jsonObj);
+    const url = this.makeUrl(rpcName, path);
+    logDebug(LOG_TAG, 'Sending: ', url, req);
 
     const headers = {};
     this.modifyHeadersForRequest(headers, token);
 
-    return this.performRPCRequest<Indexable, Resp>(
-      rpcName,
-      url,
-      headers,
-      jsonObj
-    ).then(
+    return this.performRPCRequest<Req, Resp>(rpcName, url, headers, req).then(
       response => {
         logDebug(LOG_TAG, 'Received: ', response);
         return response;
@@ -95,7 +82,7 @@ export abstract class RestConnection implements Connection {
         logWarn(
           LOG_TAG,
           `${rpcName} failed with error: `,
-          err.message,
+          err,
           'url: ',
           url,
           'request:',
@@ -108,12 +95,13 @@ export abstract class RestConnection implements Connection {
 
   invokeStreamingRPC<Req, Resp>(
     rpcName: string,
+    path: string,
     request: Req,
     token: Token | null
   ): Promise<Resp[]> {
     // The REST API automatically aggregates all of the streamed results, so we
     // can just use the normal invoke() method.
-    return this.invokeRPC<Req, Resp[]>(rpcName, request, token);
+    return this.invokeRPC<Req, Resp[]>(rpcName, path, request, token);
   }
 
   abstract openStream<Req, Resp>(
@@ -156,13 +144,12 @@ export abstract class RestConnection implements Connection {
     body: Req
   ): Promise<Resp>;
 
-  private makeUrl<Req>(rpcName: string, req: Req): string {
+  private makeUrl<Req>(rpcName: string, path: string): string {
     const urlRpcName = RPC_NAME_URL_MAPPING[rpcName];
     debugAssert(
       urlRpcName !== undefined,
       'Unknown REST mapping for: ' + rpcName
     );
-    const path = ((req as unknown) as Indexable).parent || this.databaseRoot;
     return `${this.baseUrl}/${RPC_URL_VERSION}/${path}:${urlRpcName}`;
   }
 }

packages/firestore/test/integration/api/database.test.ts

@@ -20,7 +20,7 @@ import * as chaiAsPromised from 'chai-as-promised';
 import * as firestore from '@firebase/firestore-types';
 import { expect, use } from 'chai';
 
-import { Deferred } from '../../util/promise';
+import { Deferred } from '@firebase/util';
 import { EventsAccumulator } from '../util/events_accumulator';
 import * as firebaseExport from '../util/firebase_export';
 import {

packages/firestore/test/integration/bootstrap.ts

@@ -24,8 +24,6 @@ import '../../index';
  * https://github.com/webpack-contrib/karma-webpack#alternative-usage
  */
 
-process.env.TEST_PLATFORM = 'browser';
-
 // 'context()' definition requires additional dependency on webpack-env package.
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 const testsContext = (require as any).context(

packages/firestore/test/unit/bootstrap.ts

@@ -22,8 +22,6 @@
  * https://github.com/webpack-contrib/karma-webpack#alternative-usage
  */
 
-process.env.TEST_PLATFORM = 'browser';
-
 // 'context()' definition requires additional dependency on webpack-env package.
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 const testsContext = (require as any).context(