Allowing field deletes with merge-enabled sets (#248)

Author: schmidt-sebastian

packages/firestore/src/api/database.ts

@@ -501,11 +501,9 @@ export class Transaction implements firestore.Transaction {
       this._firestore
     );
     options = validateSetOptions('Transaction.set', options);
-    const parsed = this._firestore._dataConverter.parseSetData(
-      'Transaction.set',
-      value,
-      options
-    );
+    const parsed = options.merge
+      ? this._firestore._dataConverter.parseMergeData('Transaction.set', value)
+      : this._firestore._dataConverter.parseSetData('Transaction.set', value);
     this._transaction.set(ref._key, parsed);
     return this;
   }
@@ -593,11 +591,9 @@ export class WriteBatch implements firestore.WriteBatch {
       this._firestore
     );
     options = validateSetOptions('WriteBatch.set', options);
-    const parsed = this._firestore._dataConverter.parseSetData(
-      'WriteBatch.set',
-      value,
-      options
-    );
+    const parsed = options.merge
+      ? this._firestore._dataConverter.parseMergeData('WriteBatch.set', value)
+      : this._firestore._dataConverter.parseSetData('WriteBatch.set', value);
     this._mutations = this._mutations.concat(
       parsed.toMutations(ref._key, Precondition.NONE)
     );
@@ -756,11 +752,15 @@ export class DocumentReference implements firestore.DocumentReference {
     validateBetweenNumberOfArgs('DocumentReference.set', arguments, 1, 2);
     options = validateSetOptions('DocumentReference.set', options);
 
-    const parsed = this.firestore._dataConverter.parseSetData(
-      'DocumentReference.set',
-      value,
-      options
-    );
+    const parsed = options.merge
+      ? this.firestore._dataConverter.parseMergeData(
+          'DocumentReference.set',
+          value
+        )
+      : this.firestore._dataConverter.parseSetData(
+          'DocumentReference.set',
+          value
+        );
     return this._firestoreClient.write(
       parsed.toMutations(this._key, Precondition.NONE)
     );

packages/firestore/src/api/user_data_converter.ts

@@ -117,9 +117,23 @@ export class ParsedUpdateData {
 enum UserDataSource {
   Set,
   Update,
+  MergeSet,
   QueryValue // from a where clause or cursor bound
 }
 
+function isWrite(dataSource: UserDataSource) {
+  switch (dataSource) {
+    case UserDataSource.Set: // fall through
+    case UserDataSource.MergeSet: // fall through
+    case UserDataSource.Update:
+      return true;
+    case UserDataSource.QueryValue:
+      return false;
+    default:
+      throw fail(`Unexpected case for UserDataSource: ${dataSource}`);
+  }
+}
+
 /** A "context" object passed around while parsing user data. */
 class ParseContext {
   readonly fieldTransforms: FieldTransform[];
@@ -230,7 +244,7 @@ class ParseContext {
   }
 
   private validatePathSegment(segment: string) {
-    if (this.isWrite() && RESERVED_FIELD_REGEX.test(segment)) {
+    if (isWrite(this.dataSource) && RESERVED_FIELD_REGEX.test(segment)) {
       throw this.createError('Document fields cannot begin and end with __');
     }
   }
@@ -273,40 +287,43 @@ export class DocumentKeyReference {
 export class UserDataConverter {
   constructor(private preConverter: DataPreConverter) {}
 
-  /** Parse document data (e.g. from a set() call). */
-  parseSetData(
-    methodName: string,
-    input: AnyJs,
-    options: firestore.SetOptions
-  ): ParsedSetData {
+  /** Parse document data from a non-merge set() call.*/
+  parseSetData(methodName: string, input: AnyJs): ParsedSetData {
     const context = new ParseContext(
       UserDataSource.Set,
       methodName,
       FieldPath.EMPTY_PATH
     );
     validatePlainObject('Data must be an object, but it was:', context, input);
 
-    const merge = options.merge !== undefined ? options.merge : false;
+    let updateData = this.parseData(input, context);
 
-    let updateData = ObjectValue.EMPTY;
-
-    objUtils.forEach(input as Dict<AnyJs>, (key, value) => {
-      const path = new ExternalFieldPath(key)._internalPath;
-
-      const childContext = context.childContextForFieldPath(path);
-      value = this.runPreConverter(value, childContext);
+    return new ParsedSetData(
+      updateData as ObjectValue,
+      /* fieldMask= */ null,
+      context.fieldTransforms
+    );
+  }
 
-      const parsedValue = this.parseData(value, childContext);
-      if (parsedValue) {
-        updateData = updateData.set(path, parsedValue);
-      }
-    });
+  /** Parse document data from a set() call with '{merge:true}'. */
+  parseMergeData(methodName: string, input: AnyJs): ParsedSetData {
+    const context = new ParseContext(
+      UserDataSource.MergeSet,
+      methodName,
+      FieldPath.EMPTY_PATH
+    );
+    validatePlainObject('Data must be an object, but it was:', context, input);
 
-    const fieldMask = merge ? new FieldMask(context.fieldMask) : null;
-    return new ParsedSetData(updateData, fieldMask, context.fieldTransforms);
+    let updateData = this.parseData(input, context);
+    const fieldMask = new FieldMask(context.fieldMask);
+    return new ParsedSetData(
+      updateData as ObjectValue,
+      fieldMask,
+      context.fieldTransforms
+    );
   }
 
-  /** Parse update data (e.g. from an update() call). */
+  /** Parse update data from an update() call. */
   parseUpdateData(methodName: string, input: AnyJs): ParsedUpdateData {
     const context = new ParseContext(
       UserDataSource.Update,
@@ -523,12 +540,9 @@ export class UserDataConverter {
       return new RefValue(value.databaseId, value.key);
     } else if (value instanceof FieldValueImpl) {
       if (value instanceof DeleteFieldValueImpl) {
-        // We shouldn't encounter delete sentinels here. Provide a good error.
-        if (context.dataSource !== UserDataSource.Update) {
-          throw context.createError(
-            'FieldValue.delete() can only be used with update()'
-          );
-        } else {
+        if (context.dataSource == UserDataSource.MergeSet) {
+          return null;
+        } else if (context.dataSource === UserDataSource.Update) {
           assert(
             context.path == null || context.path.length > 0,
             'FieldValue.delete() at the top level should have already' +
@@ -538,12 +552,14 @@ export class UserDataConverter {
             'FieldValue.delete() can only appear at the top level ' +
               'of your update data'
           );
+        } else {
+          // We shouldn't encounter delete sentinels for queries or non-merge set() calls.
+          throw context.createError(
+            'FieldValue.delete() can only be used with update() and set() with {merge:true}'
+          );
         }
       } else if (value instanceof ServerTimestampFieldValueImpl) {
-        if (
-          context.dataSource !== UserDataSource.Set &&
-          context.dataSource !== UserDataSource.Update
-        ) {
+        if (!isWrite(context.dataSource)) {
           throw context.createError(
             'FieldValue.serverTimestamp() can only be used with set()' +
               ' and update()'

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

@@ -26,7 +26,6 @@ import {
   withTestDb,
   withTestDoc
 } from '../util/helpers';
-import { Firestore } from '../../../src/api/database';
 
 apiDescribe('Database', persistence => {
   asyncIt('can set a document', () => {
@@ -141,6 +140,32 @@ apiDescribe('Database', persistence => {
     });
   });
 
+  asyncIt('can delete field using merge', () => {
+    return withTestDoc(persistence, doc => {
+      const initialData = {
+        untouched: true,
+        foo: 'bar',
+        nested: { untouched: true, foo: 'bar' }
+      };
+      const mergeData = {
+        foo: firebase.firestore.FieldValue.delete(),
+        nested: { foo: firebase.firestore.FieldValue.delete() }
+      };
+      const finalData = {
+        untouched: true,
+        nested: { untouched: true }
+      };
+      return doc
+        .set(initialData)
+        .then(() => doc.set(mergeData, { merge: true }))
+        .then(() => doc.get())
+        .then(docSnapshot => {
+          expect(docSnapshot.exists).to.be.ok;
+          expect(docSnapshot.data()).to.deep.equal(finalData);
+        });
+    });
+  });
+
   asyncIt('can replace an array by merging using set', () => {
     return withTestDoc(persistence, doc => {
       const initialData = {

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

@@ -419,8 +419,7 @@ apiDescribe('Validation:', persistence => {
         return expectSetToFail(
           db,
           { foo: firebase.firestore.FieldValue.delete() },
-          'FieldValue.delete() can only be used with update() (found in ' +
-            'field foo)'
+          'FieldValue.delete() can only be used with update() and set() with {merge:true} (found in field foo)'
         );
       }
     );