Drop SortedSet from FieldMask (#2999)

Author: schmidt-sebastian

packages/firestore/src/api/user_data_reader.ts

@@ -44,7 +44,6 @@ import {
   ServerTimestampTransform
 } from '../model/transform_operation';
 import { JsonProtoSerializer } from '../remote/serializer';
-import { SortedSet } from '../util/sorted_set';
 import { Blob } from './blob';
 import {
   FieldPath as ExternalFieldPath,
@@ -337,10 +336,10 @@ export class UserDataReader {
     let fieldTransforms: FieldTransform[];
 
     if (!fieldPaths) {
-      fieldMask = FieldMask.fromArray(context.fieldMask);
+      fieldMask = new FieldMask(context.fieldMask);
       fieldTransforms = context.fieldTransforms;
     } else {
-      let validatedFieldPaths = new SortedSet<FieldPath>(FieldPath.comparator);
+      const validatedFieldPaths: FieldPath[] = [];
 
       for (const stringOrFieldPath of fieldPaths) {
         let fieldPath: FieldPath;
@@ -365,10 +364,12 @@ export class UserDataReader {
           );
         }
 
-        validatedFieldPaths = validatedFieldPaths.add(fieldPath);
+        if (!fieldMaskContains(validatedFieldPaths, fieldPath)) {
+          validatedFieldPaths.push(fieldPath);
+        }
       }
 
-      fieldMask = FieldMask.fromSet(validatedFieldPaths);
+      fieldMask = new FieldMask(validatedFieldPaths);
       fieldTransforms = context.fieldTransforms.filter(transform =>
         fieldMask.covers(transform.field)
       );
@@ -389,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 fieldMaskPaths: FieldPath[] = [];
     const updateData = new ObjectValueBuilder();
     forEach(input as Dict<unknown>, (key, value) => {
       const path = fieldPathFromDotSeparatedString(methodName, key);
@@ -398,17 +399,17 @@ export class UserDataReader {
       value = this.runPreConverter(value, childContext);
       if (value instanceof DeleteFieldValueImpl) {
         // Add it to the field mask, but don't add anything to updateData.
-        fieldMaskPaths = fieldMaskPaths.add(path);
+        fieldMaskPaths.push(path);
       } else {
         const parsedValue = this.parseData(value, childContext);
         if (parsedValue != null) {
-          fieldMaskPaths = fieldMaskPaths.add(path);
+          fieldMaskPaths.push(path);
           updateData.set(path, parsedValue);
         }
       }
     });
 
-    const mask = FieldMask.fromSet(fieldMaskPaths);
+    const mask = new FieldMask(fieldMaskPaths);
     return new ParsedUpdateData(
       updateData.build(),
       mask,
@@ -449,26 +450,30 @@ export class UserDataReader {
       values.push(moreFieldsAndValues[i + 1]);
     }
 
-    let fieldMaskPaths = new SortedSet<FieldPath>(FieldPath.comparator);
+    const fieldMaskPaths: FieldPath[] = [];
     const updateData = new ObjectValueBuilder();
 
-    for (let i = 0; i < keys.length; ++i) {
-      const path = keys[i];
-      const childContext = context.childContextForFieldPath(path);
-      const value = this.runPreConverter(values[i], childContext);
-      if (value instanceof DeleteFieldValueImpl) {
-        // Add it to the field mask, but don't add anything to updateData.
-        fieldMaskPaths = fieldMaskPaths.add(path);
-      } else {
-        const parsedValue = this.parseData(value, childContext);
-        if (parsedValue != null) {
-          fieldMaskPaths = fieldMaskPaths.add(path);
-          updateData.set(path, parsedValue);
+    // We iterate in reverse order to pick the last value for a field if the
+    // user specified the field multiple times.
+    for (let i = keys.length - 1; i >= 0; --i) {
+      if (!fieldMaskContains(fieldMaskPaths, keys[i])) {
+        const path = keys[i];
+        const childContext = context.childContextForFieldPath(path);
+        const value = this.runPreConverter(values[i], childContext);
+        if (value instanceof DeleteFieldValueImpl) {
+          // Add it to the field mask, but don't add anything to updateData.
+          fieldMaskPaths.push(path);
+        } else {
+          const parsedValue = this.parseData(value, childContext);
+          if (parsedValue != null) {
+            fieldMaskPaths.push(path);
+            updateData.set(path, parsedValue);
+          }
         }
       }
     }
 
-    const mask = FieldMask.fromSet(fieldMaskPaths);
+    const mask = new FieldMask(fieldMaskPaths);
     return new ParsedUpdateData(
       updateData.build(),
       mask,
@@ -841,3 +846,8 @@ function fieldPathFromDotSeparatedString(
 function errorMessage(error: Error | object): string {
   return error instanceof Error ? error.message : error.toString();
 }
+
+/** Checks `haystack` if FieldPath `needle` is present. Runs in O(n). */
+function fieldMaskContains(haystack: FieldPath[], needle: FieldPath): boolean {
+  return haystack.some(v => v.isEqual(needle));
+}

packages/firestore/src/model/mutation.ts

@@ -20,7 +20,6 @@ import * as api from '../protos/firestore_proto_api';
 import { Timestamp } from '../api/timestamp';
 import { SnapshotVersion } from '../core/snapshot_version';
 import { debugAssert, fail, hardAssert } from '../util/assert';
-import { SortedSet } from '../util/sorted_set';
 
 import {
   Document,
@@ -45,18 +44,15 @@ import { arrayEquals } from '../util/misc';
  *             containing foo
  */
 export class FieldMask {
-  constructor(readonly fields: SortedSet<FieldPath>) {
+  constructor(readonly fields: FieldPath[]) {
     // TODO(dimond): validation of FieldMask
-  }
-
-  static fromSet(fields: SortedSet<FieldPath>): FieldMask {
-    return new FieldMask(fields);
-  }
-
-  static fromArray(fields: FieldPath[]): FieldMask {
-    let fieldsAsSet = new SortedSet<FieldPath>(FieldPath.comparator);
-    fields.forEach(fieldPath => (fieldsAsSet = fieldsAsSet.add(fieldPath)));
-    return new FieldMask(fieldsAsSet);
+    // Sort the field mask to support `FieldMask.isEqual()` and assert below.
+    fields.sort(FieldPath.comparator);
+    debugAssert(
+      !fields.some((v, i) => i !== 0 && v.isEqual(fields[i - 1])),
+      'FieldMask contains field that is not unique: ' +
+        fields.find((v, i) => i !== 0 && v.isEqual(fields[i - 1]))!
+    );
   }
 
   /**
@@ -66,17 +62,16 @@ export class FieldMask {
    * This is an O(n) operation, where `n` is the size of the field mask.
    */
   covers(fieldPath: FieldPath): boolean {
-    let found = false;
-    this.fields.forEach(fieldMaskPath => {
+    for (const fieldMaskPath of this.fields) {
       if (fieldMaskPath.isPrefixOf(fieldPath)) {
-        found = true;
+        return true;
       }
-    });
-    return found;
+    }
+    return false;
   }
 
   isEqual(other: FieldMask): boolean {
-    return this.fields.isEqual(other.fields);
+    return arrayEquals(this.fields, other.fields, (l, r) => l.isEqual(r));
   }
 }
 

packages/firestore/src/model/object_value.ts

@@ -23,7 +23,6 @@ import { FieldPath } from './path';
 import { isServerTimestamp } from './server_timestamps';
 import { valueEquals, isMapValue, typeOrder } from './values';
 import { forEach } from '../util/obj';
-import { SortedSet } from '../util/sorted_set';
 
 export interface JsonObject<T> {
   [name: string]: T;
@@ -243,27 +242,27 @@ export class ObjectValueBuilder {
  * Returns a FieldMask built from all fields in a MapValue.
  */
 export function extractFieldMask(value: api.MapValue): FieldMask {
-  let fields = new SortedSet<FieldPath>(FieldPath.comparator);
+  const fields: FieldPath[] = [];
   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()) {
+      if (nestedFields.length === 0) {
         // Preserve the empty map by adding it to the FieldMask.
-        fields = fields.add(currentPath);
+        fields.push(currentPath);
       } else {
         // For nested and non-empty ObjectValues, add the FieldPath of the
         // leaf nodes.
-        nestedFields.forEach(nestedPath => {
-          fields = fields.add(currentPath.child(nestedPath));
-        });
+        for (const nestedPath of nestedFields) {
+          fields.push(currentPath.child(nestedPath));
+        }
       }
     } else {
       // For nested and non-empty ObjectValues, add the FieldPath of the leaf
       // nodes.
-      fields = fields.add(currentPath);
+      fields.push(currentPath);
     }
   });
-  return FieldMask.fromSet(fields);
+  return new FieldMask(fields);
 }

packages/firestore/src/remote/serializer.ts

@@ -1086,8 +1086,7 @@ export class JsonProtoSerializer {
 
   fromDocumentMask(proto: api.DocumentMask): FieldMask {
     const paths = proto.fieldPaths || [];
-    const fields = paths.map(path => FieldPath.fromServerFormat(path));
-    return FieldMask.fromArray(fields);
+    return new FieldMask(paths.map(path => FieldPath.fromServerFormat(path)));
   }
 }
 

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

@@ -531,6 +531,18 @@ apiDescribe('Database', (persistence: boolean) => {
     });
   });
 
+  it('can specify updated field multiple times', () => {
+    return withTestDoc(persistence, doc => {
+      return doc
+        .set({})
+        .then(() => doc.update('field', 100, new FieldPath('field'), 200))
+        .then(() => doc.get())
+        .then(docSnap => {
+          expect(docSnap.data()).to.deep.equal({ field: 200 });
+        });
+    });
+  });
+
   describe('documents: ', () => {
     const invalidDocValues = [undefined, null, 0, 'foo', ['a'], new Date()];
     for (const val of invalidDocValues) {

packages/firestore/test/unit/remote/serializer.helper.ts

@@ -540,7 +540,7 @@ export function serializerTest(
         const expected: api.DocumentMask = {
           fieldPaths: ['foo.`bar.baz\\qux`']
         };
-        const mask = FieldMask.fromArray([
+        const mask = new FieldMask([
           FieldPath.fromServerFormat('foo.bar\\.baz\\\\qux')
         ]);
         const actual = s.toDocumentMask(mask);
@@ -554,7 +554,7 @@ export function serializerTest(
       // TODO(b/34988481): Implement correct escaping
       // eslint-disable-next-line no-restricted-properties
       it.skip('converts a weird path', () => {
-        const expected = FieldMask.fromArray([
+        const expected = new FieldMask([
           FieldPath.fromServerFormat('foo.bar\\.baz\\\\qux')
         ]);
         const proto: api.DocumentMask = { fieldPaths: ['foo.`bar.baz\\qux`'] };

packages/firestore/test/unit/util/misc.test.ts

@@ -18,6 +18,7 @@
 import { expect } from 'chai';
 import { immediateSuccessor } from '../../../src/util/misc';
 import { debugCast } from '../../../src/util/assert';
+import { mask } from '../../util/helpers';
 
 describe('immediateSuccessor', () => {
   it('generates the correct immediate successors', () => {
@@ -43,3 +44,11 @@ describe('typeCast', () => {
     );
   });
 });
+
+describe('FieldMask', () => {
+  it('cannot contain duplicate fields', () => {
+    expect(() => mask('a', 'b', 'a')).to.throw(
+      'FieldMask contains field that is not unique: a'
+    );
+  });
+});

packages/firestore/test/util/helpers.ts

@@ -212,11 +212,7 @@ export function field(path: string): FieldPath {
 }
 
 export function mask(...paths: string[]): FieldMask {
-  let fieldPaths = new SortedSet<FieldPath>(FieldPath.comparator);
-  for (const path of paths) {
-    fieldPaths = fieldPaths.add(field(path));
-  }
-  return FieldMask.fromSet(fieldPaths);
+  return new FieldMask(paths.map(v => field(v)));
 }
 
 export function blob(...bytes: number[]): Blob {