Untangle ObjectValue and ObjectValueBuilder (#2970)

firebase · Apr 23, 2020 · 1ae13d1 · 1ae13d1
1 parent 4ad5733
commit 1ae13d1
Show file tree

Hide file tree

Showing 7 changed files with 101 additions and 106 deletions.
diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts
@@ -36,7 +36,7 @@ import { debugAssert, fail } from '../util/assert';
 import { Code, FirestoreError } from '../util/error';
 import { isPlainObject, valueDescription } from '../util/input_validation';
 import { Dict, forEach, isEmpty } from '../util/obj';
-import { ObjectValue } from '../model/field_value';
+import { ObjectValue, ObjectValueBuilder } from '../model/field_value';
 import {
   ArrayRemoveTransformOperation,
   ArrayUnionTransformOperation,
@@ -390,7 +390,7 @@ export class UserDataReader {
     validatePlainObject('Data must be an object, but it was:', context, input);
 
     let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
-    const updateData = ObjectValue.newBuilder();
+    const updateData = new ObjectValueBuilder();
     forEach(input as Dict<unknown>, (key, value) => {
       const path = fieldPathFromDotSeparatedString(methodName, key);
 
@@ -450,7 +450,7 @@ export class UserDataReader {
     }
 
     let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
-    const updateData = ObjectValue.newBuilder();
+    const updateData = new ObjectValueBuilder();
 
     for (let i = 0; i < keys.length; ++i) {
       const path = keys[i];

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
@@ -66,6 +66,7 @@ import { IndexedDbPersistence } from './indexeddb_persistence';
 import { IndexedDbMutationQueue } from './indexeddb_mutation_queue';
 import { IndexedDbRemoteDocumentCache } from './indexeddb_remote_document_cache';
 import { IndexedDbTargetCache } from './indexeddb_target_cache';
+import { extractFieldMask } from '../model/field_value';
 
 const LOG_TAG = 'LocalStore';
 
@@ -339,7 +340,7 @@ export class LocalStore {
                 new PatchMutation(
                   mutation.key,
                   baseValue,
-                  baseValue.fieldMask(),
+                  extractFieldMask(baseValue.proto.mapValue!),
                   Precondition.exists(true)
                 )
               );

diff --git a/packages/firestore/src/model/field_value.ts b/packages/firestore/src/model/field_value.ts
@@ -59,9 +59,8 @@ export class ObjectValue {
     );
   }
 
-  /** Returns a new Builder instance that is based on an empty object. */
-  static newBuilder(): ObjectValueBuilder {
-    return ObjectValue.EMPTY.toBuilder();
+  static empty(): ObjectValue {
+    return new ObjectValue({ mapValue: {} });
   }
 
   /**
@@ -90,48 +89,9 @@ export class ObjectValue {
     }
   }
 
-  /**
-   * Returns a FieldMask built from all FieldPaths starting from this
-   * ObjectValue, including paths from nested objects.
-   */
-  fieldMask(): FieldMask {
-    return this.extractFieldMask(this.proto.mapValue!);
-  }
-
-  private extractFieldMask(value: api.MapValue): FieldMask {
-    let fields = new SortedSet<FieldPath>(FieldPath.comparator);
-    forEach(value.fields || {}, (key, value) => {
-      const currentPath = new FieldPath([key]);
-      if (typeOrder(value) === TypeOrder.ObjectValue) {
-        const nestedMask = this.extractFieldMask(value.mapValue!);
-        const nestedFields = nestedMask.fields;
-        if (nestedFields.isEmpty()) {
-          // Preserve the empty map by adding it to the FieldMask.
-          fields = fields.add(currentPath);
-        } else {
-          // For nested and non-empty ObjectValues, add the FieldPath of the
-          // leaf nodes.
-          nestedFields.forEach(nestedPath => {
-            fields = fields.add(currentPath.child(nestedPath));
-          });
-        }
-      } else {
-        // For nested and non-empty ObjectValues, add the FieldPath of the leaf
-        // nodes.
-        fields = fields.add(currentPath);
-      }
-    });
-    return FieldMask.fromSet(fields);
-  }
-
   isEqual(other: ObjectValue): boolean {
     return valueEquals(this.proto, other.proto);
   }
-
-  /** Creates a ObjectValueBuilder instance that is based on the current value. */
-  toBuilder(): ObjectValueBuilder {
-    return new ObjectValueBuilder(this);
-  }
 }
 
 /**
@@ -152,7 +112,7 @@ export class ObjectValueBuilder {
   /**
    * @param baseObject The object to mutate.
    */
-  constructor(private readonly baseObject: ObjectValue) {}
+  constructor(private readonly baseObject: ObjectValue = ObjectValue.empty()) {}
 
   /**
    * Sets the field to the provided value.
@@ -278,3 +238,32 @@ export class ObjectValueBuilder {
     return modified ? { mapValue: { fields: resultAtPath } } : null;
   }
 }
+
+/**
+ * Returns a FieldMask built from all fields in a MapValue.
+ */
+export function extractFieldMask(value: api.MapValue): FieldMask {
+  let fields = new SortedSet<FieldPath>(FieldPath.comparator);
+  forEach(value!.fields || {}, (key, value) => {
+    const currentPath = new FieldPath([key]);
+    if (isMapValue(value)) {
+      const nestedMask = extractFieldMask(value.mapValue!);
+      const nestedFields = nestedMask.fields;
+      if (nestedFields.isEmpty()) {
+        // Preserve the empty map by adding it to the FieldMask.
+        fields = fields.add(currentPath);
+      } else {
+        // For nested and non-empty ObjectValues, add the FieldPath of the
+        // leaf nodes.
+        nestedFields.forEach(nestedPath => {
+          fields = fields.add(currentPath.child(nestedPath));
+        });
+      }
+    } else {
+      // For nested and non-empty ObjectValues, add the FieldPath of the leaf
+      // nodes.
+      fields = fields.add(currentPath);
+    }
+  });
+  return FieldMask.fromSet(fields);
+}
diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts
@@ -488,13 +488,13 @@ export class PatchMutation extends Mutation {
     if (maybeDoc instanceof Document) {
       data = maybeDoc.data();
     } else {
-      data = ObjectValue.EMPTY;
+      data = ObjectValue.empty();
     }
     return this.patchObject(data);
   }
 
   private patchObject(data: ObjectValue): ObjectValue {
-    const builder = data.toBuilder();
+    const builder = new ObjectValueBuilder(data);
     this.fieldMask.fields.forEach(fieldPath => {
       if (!fieldPath.isEmpty()) {
         const newValue = this.data.field(fieldPath);
@@ -601,7 +601,7 @@ export class TransformMutation extends Mutation {
 
       if (coercedValue != null) {
         if (baseObject == null) {
-          baseObject = ObjectValue.newBuilder().set(
+          baseObject = new ObjectValueBuilder().set(
             fieldTransform.field,
             coercedValue
           );
@@ -729,7 +729,7 @@ export class TransformMutation extends Mutation {
       'TransformResults length mismatch.'
     );
 
-    const builder = data.toBuilder();
+    const builder = new ObjectValueBuilder(data);
     for (let i = 0; i < this.fieldTransforms.length; i++) {
       const fieldTransform = this.fieldTransforms[i];
       const fieldPath = fieldTransform.field;

diff --git a/packages/firestore/test/unit/model/field_value.test.ts b/packages/firestore/test/unit/model/field_value.test.ts
@@ -18,7 +18,12 @@
 import * as api from '../../../src/protos/firestore_proto_api';
 
 import { expect } from 'chai';
-import { ObjectValue, TypeOrder } from '../../../src/model/field_value';
+import {
+  extractFieldMask,
+  ObjectValue,
+  ObjectValueBuilder,
+  TypeOrder
+} from '../../../src/model/field_value';
 import { typeOrder } from '../../../src/model/values';
 import { wrap, wrapObject, field, mask } from '../../util/helpers';
 
@@ -79,13 +84,11 @@ describe('FieldValue', () => {
   });
 
   it('can add multiple new fields', () => {
-    let objValue = ObjectValue.EMPTY;
-    objValue = objValue
-      .toBuilder()
+    let objValue = ObjectValue.empty();
+    objValue = new ObjectValueBuilder(objValue)
       .set(field('a'), wrap('a'))
       .build();
-    objValue = objValue
-      .toBuilder()
+    objValue = new ObjectValueBuilder(objValue)
       .set(field('b'), wrap('b'))
       .set(field('c'), wrap('c'))
       .build();
@@ -154,8 +157,7 @@ describe('FieldValue', () => {
   it('can delete added keys', () => {
     let objValue = wrapObject({});
 
-    objValue = objValue
-      .toBuilder()
+    objValue = new ObjectValueBuilder(objValue)
       .set(field('a'), wrap('a'))
       .delete(field('a'))
       .build();
@@ -189,12 +191,8 @@ describe('FieldValue', () => {
   it('can delete multiple fields', () => {
     let objValue = wrapObject({ a: 'a', b: 'a', c: 'c' });
 
-    objValue = objValue
-      .toBuilder()
-      .delete(field('a'))
-      .build();
-    objValue = objValue
-      .toBuilder()
+    objValue = new ObjectValueBuilder(objValue).delete(field('a')).build();
+    objValue = new ObjectValueBuilder(objValue)
       .delete(field('b'))
       .delete(field('c'))
       .build();
@@ -216,7 +214,7 @@ describe('FieldValue', () => {
       'map.nested.d',
       'emptymap'
     );
-    const actualMask = objValue.fieldMask();
+    const actualMask = extractFieldMask(objValue.proto.mapValue!);
     expect(actualMask.isEqual(expectedMask)).to.be.true;
   });
 
@@ -225,8 +223,7 @@ describe('FieldValue', () => {
     fieldPath: string,
     value: api.Value
   ): ObjectValue {
-    return objectValue
-      .toBuilder()
+    return new ObjectValueBuilder(objectValue)
       .set(field(fieldPath), value)
       .build();
   }
@@ -235,10 +232,7 @@ describe('FieldValue', () => {
     objectValue: ObjectValue,
     fieldPath: string
   ): ObjectValue {
-    return objectValue
-      .toBuilder()
-      .delete(field(fieldPath))
-      .build();
+    return new ObjectValueBuilder(objectValue).delete(field(fieldPath)).build();
   }
 
   function assertObjectEquals(

diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts
@@ -47,6 +47,7 @@ import {
   wrap,
   wrapObject
 } from '../../util/helpers';
+import { ObjectValueBuilder } from '../../../src/model/field_value';
 
 describe('Mutation', () => {
   addEqualityMatcher();
@@ -191,11 +192,12 @@ describe('Mutation', () => {
     );
 
     // Server timestamps aren't parsed, so we manually insert it.
-    const data = wrapObject({
-      foo: { bar: '<server-timestamp>' },
-      baz: 'baz-value'
-    })
-      .toBuilder()
+    const data = new ObjectValueBuilder(
+      wrapObject({
+        foo: { bar: '<server-timestamp>' },
+        baz: 'baz-value'
+      })
+    )
       .set(field('foo.bar'), serverTimestamp(timestamp, null))
       .build();
     const expectedDoc = new Document(key('collection/key'), version(0), data, {