Merge branch 'master' into mrschmidt/rewritefieldvalue

Author: schmidt-sebastian

README.md

@@ -88,11 +88,12 @@ $ yarn build
 A production project is required to test the Firebase JS SDK. You can create a
 new project by visiting the [Firebase Console](https://console.firebase.google.com/).
 
-#### Firestore Support
+#### Firestore Database Setup
 
-Visit the database section of the console and enable the Cloud Firestore Beta.
-You can select either of the default permissions settings as we will overwrite
-them below.
+Visit the "Database" section of the console and create a Cloud Firestore
+database. When prompted to select the set of initial security rules, select
+any option (e.g. "Start in Production Mode") since these permission settings
+will be overwritten below.
 
 #### Authentication Support
 
@@ -101,17 +102,25 @@ sign-in provider to complete your project config.
 
 #### Automated Setup
 
-The remainder of the test setup requires choosing a test project. You can 
-choose the project manually or specify the project directly at the root of 
-the package.
+The tests need to be configured to use the Firebase production project that you
+created in the "Test Setup" section above. To do this, run the `yarn test:setup`
+command, as follows:
+
 
 ```bash
-# Select a project manually when running setup
+# Select the Firebase project via the text-based UI.
 $ yarn test:setup
 
-# Specify the specific project for setup
-$ yarn test:setup --projectId=<your-test-project>
+# Specify the Firebase project via the command-line arguments.
+$ yarn test:setup --projectId=<test_firebase_project_id>
+```
+
+If you see an error like
+```
+HTTP Error: 404, Project '<test_firebase_project_id>' does not exist.
 ```
+then make sure that you have created the database as specified in the "Firestore
+Database Setup" section above.
 
 ### Running the tests
 

packages/firestore/src/api/user_data_converter.ts

@@ -404,7 +404,7 @@ export class UserDataConverter {
     validatePlainObject('Data must be an object, but it was:', context, input);
 
     let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
-    let updateData = ObjectValue.EMPTY;
+    const updateData = ObjectValue.newBuilder();
     forEach(input as Dict<unknown>, (key, value) => {
       const path = fieldPathFromDotSeparatedString(methodName, key);
 
@@ -417,13 +417,17 @@ export class UserDataConverter {
         const parsedValue = this.parseData(value, childContext);
         if (parsedValue != null) {
           fieldMaskPaths = fieldMaskPaths.add(path);
-          updateData = updateData.set(path, parsedValue);
+          updateData.set(path, parsedValue);
         }
       }
     });
 
     const mask = FieldMask.fromSet(fieldMaskPaths);
-    return new ParsedUpdateData(updateData, mask, context.fieldTransforms);
+    return new ParsedUpdateData(
+      updateData.build(),
+      mask,
+      context.fieldTransforms
+    );
   }
 
   /** Parse update data from a list of field/value arguments. */
@@ -460,7 +464,7 @@ export class UserDataConverter {
     }
 
     let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
-    let updateData = ObjectValue.EMPTY;
+    const updateData = ObjectValue.newBuilder();
 
     for (let i = 0; i < keys.length; ++i) {
       const path = keys[i];
@@ -473,13 +477,17 @@ export class UserDataConverter {
         const parsedValue = this.parseData(value, childContext);
         if (parsedValue != null) {
           fieldMaskPaths = fieldMaskPaths.add(path);
-          updateData = updateData.set(path, parsedValue);
+          updateData.set(path, parsedValue);
         }
       }
     }
 
     const mask = FieldMask.fromSet(fieldMaskPaths);
-    return new ParsedUpdateData(updateData, mask, context.fieldTransforms);
+    return new ParsedUpdateData(
+      updateData.build(),
+      mask,
+      context.fieldTransforms
+    );
   }
 
   /**

packages/firestore/src/model/document.ts

@@ -119,11 +119,11 @@ export class Document extends MaybeDocument {
 
   data(): ObjectValue {
     if (!this.objectValue) {
-      let result = ObjectValue.EMPTY;
+      const result = ObjectValue.newBuilder();
       obj.forEach(this.proto!.fields || {}, (key: string, value: api.Value) => {
-        result = result.set(new FieldPath([key]), this.converter!(value));
+        result.set(new FieldPath([key]), this.converter!(value));
       });
-      this.objectValue = result;
+      this.objectValue = result.build();
 
       // Once objectValue is computed, values inside the fieldValueCache are no
       // longer accessed.

packages/firestore/src/model/field_value.ts

@@ -203,7 +203,7 @@ export class BooleanValue extends FieldValue {
 
   compareTo(other: FieldValue): number {
     if (other instanceof BooleanValue) {
-      return primitiveComparator(this, other);
+      return primitiveComparator(this.internalValue, other.internalValue);
     }
     return this.defaultCompareTo(other);
   }
@@ -558,6 +558,11 @@ export class ObjectValue extends FieldValue {
     super();
   }
 
+  /** Returns a new ObjectValueBuilder instance that is based on an empty object. */
+  static newBuilder(): ObjectValueBuilder {
+    return new ObjectValueBuilder(ObjectValue.EMPTY.internalValue);
+  }
+
   value(options?: FieldValueOptions): JsonObject<FieldType> {
     const result: JsonObject<FieldType> = {};
     this.internalValue.inorderTraversal((key, val) => {
@@ -610,42 +615,6 @@ export class ObjectValue extends FieldValue {
     }
   }
 
-  set(path: FieldPath, to: FieldValue): ObjectValue {
-    assert(!path.isEmpty(), 'Cannot set field for empty path on ObjectValue');
-    if (path.length === 1) {
-      return this.setChild(path.firstSegment(), to);
-    } else {
-      let child = this.child(path.firstSegment());
-      if (!(child instanceof ObjectValue)) {
-        child = ObjectValue.EMPTY;
-      }
-      const newChild = (child as ObjectValue).set(path.popFirst(), to);
-      return this.setChild(path.firstSegment(), newChild);
-    }
-  }
-
-  delete(path: FieldPath): ObjectValue {
-    assert(
-      !path.isEmpty(),
-      'Cannot delete field for empty path on ObjectValue'
-    );
-    if (path.length === 1) {
-      return new ObjectValue(this.internalValue.remove(path.firstSegment()));
-    } else {
-      // nested field
-      const child = this.child(path.firstSegment());
-      if (child instanceof ObjectValue) {
-        const newChild = child.delete(path.popFirst());
-        return new ObjectValue(
-          this.internalValue.insert(path.firstSegment(), newChild)
-        );
-      } else {
-        // Don't actually change a primitive value to an object for a delete
-        return this;
-      }
-    }
-  }
-
   contains(path: FieldPath): boolean {
     return this.field(path) !== null;
   }
@@ -703,17 +672,90 @@ export class ObjectValue extends FieldValue {
     return this.internalValue.toString();
   }
 
-  private child(childName: string): FieldValue | undefined {
-    return this.internalValue.get(childName) || undefined;
+  static EMPTY = new ObjectValue(
+    new SortedMap<string, FieldValue>(primitiveComparator)
+  );
+
+  /** Creates a ObjectValueBuilder instance that is based on the current value. */
+  toBuilder(): ObjectValueBuilder {
+    return new ObjectValueBuilder(this.internalValue);
+  }
+}
+
+/**
+ * An ObjectValueBuilder provides APIs to set and delete fields from an
+ * ObjectValue. All operations mutate the existing instance.
+ */
+export class ObjectValueBuilder {
+  constructor(private internalValue: SortedMap<string, FieldValue>) {}
+
+  /**
+   * Sets the field to the provided value.
+   *
+   * @param path The field path to set.
+   * @param value The value to set.
+   * @return The current Builder instance.
+   */
+  set(path: FieldPath, value: FieldValue): ObjectValueBuilder {
+    assert(!path.isEmpty(), 'Cannot set field for empty path on ObjectValue');
+    const childName = path.firstSegment();
+    if (path.length === 1) {
+      this.internalValue = this.internalValue.insert(childName, value);
+    } else {
+      // nested field
+      const child = this.internalValue.get(childName);
+      let obj: ObjectValue;
+      if (child instanceof ObjectValue) {
+        obj = child;
+      } else {
+        obj = ObjectValue.EMPTY;
+      }
+      const newChild = obj
+        .toBuilder()
+        .set(path.popFirst(), value)
+        .build();
+      this.internalValue = this.internalValue.insert(childName, newChild);
+    }
+    return this;
   }
 
-  private setChild(childName: string, value: FieldValue): ObjectValue {
-    return new ObjectValue(this.internalValue.insert(childName, value));
+  /**
+   * Removes the field at the current path. If there is no field at the
+   * specified path, nothing is changed.
+   *
+   * @param path The field path to remove
+   * @return The current Builder instance.
+   */
+  delete(path: FieldPath): ObjectValueBuilder {
+    assert(
+      !path.isEmpty(),
+      'Cannot delete field for empty path on ObjectValue'
+    );
+    const childName = path.firstSegment();
+    if (path.length === 1) {
+      this.internalValue = this.internalValue.remove(childName);
+    } else {
+      // nested field
+      const child = this.internalValue.get(childName);
+      if (child instanceof ObjectValue) {
+        const newChild = child
+          .toBuilder()
+          .delete(path.popFirst())
+          .build();
+        this.internalValue = this.internalValue.insert(
+          path.firstSegment(),
+          newChild
+        );
+      } else {
+        // Don't actually change a primitive value to an object for a delete
+      }
+    }
+    return this;
   }
 
-  static EMPTY = new ObjectValue(
-    new SortedMap<string, FieldValue>(primitiveComparator)
-  );
+  build(): ObjectValue {
+    return new ObjectValue(this.internalValue);
+  }
 }
 
 export class ArrayValue extends FieldValue {

packages/firestore/src/model/mutation.ts

@@ -28,7 +28,7 @@ import {
   UnknownDocument
 } from './document';
 import { DocumentKey } from './document_key';
-import { FieldValue, ObjectValue } from './field_value';
+import { FieldValue, ObjectValue, ObjectValueBuilder } from './field_value';
 import { FieldPath } from './path';
 import { TransformOperation } from './transform_operation';
 
@@ -507,17 +507,18 @@ export class PatchMutation extends Mutation {
   }
 
   private patchObject(data: ObjectValue): ObjectValue {
+    const builder = data.toBuilder();
     this.fieldMask.fields.forEach(fieldPath => {
       if (!fieldPath.isEmpty()) {
         const newValue = this.data.field(fieldPath);
         if (newValue !== null) {
-          data = data.set(fieldPath, newValue);
+          builder.set(fieldPath, newValue);
         } else {
-          data = data.delete(fieldPath);
+          builder.delete(fieldPath);
         }
       }
     });
-    return data;
+    return builder.build();
   }
 }
 
@@ -611,7 +612,7 @@ export class TransformMutation extends Mutation {
   }
 
   extractBaseValue(maybeDoc: MaybeDocument | null): ObjectValue | null {
-    let baseObject: ObjectValue | null = null;
+    let baseObject: ObjectValueBuilder | null = null;
     for (const fieldTransform of this.fieldTransforms) {
       const existingValue =
         maybeDoc instanceof Document
@@ -623,7 +624,7 @@ export class TransformMutation extends Mutation {
 
       if (coercedValue != null) {
         if (baseObject == null) {
-          baseObject = ObjectValue.EMPTY.set(
+          baseObject = ObjectValue.newBuilder().set(
             fieldTransform.field,
             coercedValue
           );
@@ -632,7 +633,7 @@ export class TransformMutation extends Mutation {
         }
       }
     }
-    return baseObject;
+    return baseObject ? baseObject.build() : null;
   }
 
   isEqual(other: Mutation): boolean {
@@ -749,12 +750,13 @@ export class TransformMutation extends Mutation {
       'TransformResults length mismatch.'
     );
 
+    const builder = data.toBuilder();
     for (let i = 0; i < this.fieldTransforms.length; i++) {
       const fieldTransform = this.fieldTransforms[i];
       const fieldPath = fieldTransform.field;
-      data = data.set(fieldPath, transformResults[i]);
+      builder.set(fieldPath, transformResults[i]);
     }
-    return data;
+    return builder.build();
   }
 }
 

packages/firestore/src/platform_browser/webchannel_connection.ts

@@ -228,11 +228,6 @@ export class WebChannelConnection implements Connection {
     ];
     const webchannelTransport = createWebChannelTransport();
     const request: WebChannelOptions = {
-      // Background channel test avoids the initial two test calls and decreases
-      // initial cold start time.
-      // TODO(dimond): wenboz@ mentioned this might affect use with proxies and
-      // we should monitor closely for any reports.
-      backgroundChannelTest: true,
       // Required for backend stickiness, routing behavior is based on this
       // parameter.
       httpSessionIdParam: 'gsessionid',

packages/firestore/src/remote/serializer.ts

@@ -577,11 +577,11 @@ export class JsonProtoSerializer {
   fromFields(object: {}): fieldValue.ObjectValue {
     // Proto map<string, Value> gets mapped to Object, so cast it.
     const map = object as { [key: string]: api.Value };
-    let result = fieldValue.ObjectValue.EMPTY;
+    const result = fieldValue.ObjectValue.newBuilder();
     obj.forEach(map, (key, value) => {
-      result = result.set(new FieldPath([key]), this.fromValue(value));
+      result.set(new FieldPath([key]), this.fromValue(value));
     });
-    return result;
+    return result.build();
   }
 
   toMapValue(map: fieldValue.ObjectValue): api.MapValue {