Rejecting invalid nested map updates

Author: schmidt-sebastian

src/document.js

@@ -680,7 +680,7 @@ class DocumentSnapshot {
    * This functions turns { foo.bar : foobar } into { foo { bar : foobar }}
    *
    * @private
-   * @param {Map.<string|FieldPath, *>} data - The field/value map to expand.
+   * @param {Map.<FieldPath, *>} data - The field/value map to expand.
    * @returns {DocumentData} The expanded JavaScript object.
    */
   static expandMap(data) {
@@ -701,30 +701,17 @@ class DocumentSnapshot {
           target[key] = {};
           merge(target[key], value, path, pos + 1);
         }
-      } else if (isPlainObject(target[key])) {
-        if (isLast) {
-          // The existing object has deeper nesting that the value we are trying
-          // to merge.
-          throw new Error(
-            `Field "${new FieldPath(path)}" has conflicting definitions.`
-          );
-        } else {
-          merge(target[key], value, path, pos + 1);
-        }
       } else {
-        // We are trying to merge an object with a primitive.
-        throw new Error(
-          `Field "${new FieldPath(
-            path.slice(0, pos + 1)
-          )}" has conflicting definitions.`
-        );
+        validate.isPlainObject(target[key]);
+        assert(!isLast, "Can't merge current value into a nested map");
+        merge(target[key], value, path, pos + 1);
       }
     }
 
     let res = {};
 
     data.forEach((value, key) => {
-      let components = FieldPath.fromArgument(key).toArray();
+      let components = key.toArray();
       merge(res, value, components, 0);
     });
 

src/write-batch.js

@@ -370,6 +370,8 @@ class WriteBatch {
       }
     }
 
+    validate.isUpdateMap('dataOrField', updateMap);
+
     let documentMask = DocumentMask.fromMap(updateMap);
     let expandedObject = DocumentSnapshot.expandMap(updateMap);
     let document = new DocumentSnapshot(
@@ -523,6 +525,34 @@ class WriteBatch {
   }
 }
 
+/*!
+ * Validates that the update data does not contain any ambiguous field
+ * definitions (such as 'a.b' and 'a').
+ *
+ * @param {Map.<FieldPath, *>} data - An update map with field/value pairs.
+ * @returns {boolean} 'true' if the input is a valid update map.
+ */
+function validateUpdateMap(data) {
+  if (!is.instance(data, Map)) {
+    throw new Error('Input is not a map.');
+  }
+
+  const fields = [];
+  data.forEach((value, key) => {
+    fields.push(key);
+  });
+
+  fields.sort((left, right) => left.compareTo(right));
+
+  for (let i = 1; i < fields.length; ++i) {
+    if (fields[i - 1].isPrefixOf(fields[i])) {
+      throw new Error(`Field "${fields[i - 1]}" has conflicting definitions.`);
+    }
+  }
+
+  return true;
+}
+
 module.exports = (
   FirestoreType,
   DocumentReferenceType,
@@ -540,6 +570,7 @@ module.exports = (
     FieldPath: FieldPath.validateFieldPath,
     Precondition: document.validatePrecondition,
     SetOptions: document.validateSetOptions,
+    UpdateMap: validateUpdateMap,
   });
   return {
     WriteBatch,

test/document.js

@@ -1305,47 +1305,53 @@ describe('update document', function() {
         foo: 'foobar',
         'foo.bar': 'foobar',
       });
-    }, /Field "foo" has conflicting definitions\./);
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo" has conflicting definitions\./);
 
     assert.throws(() => {
       firestore.doc('collectionId/documentId').update({
         foo: 'foobar',
         'foo.bar.foobar': 'foobar',
       });
-    }, /Field "foo" has conflicting definitions\./);
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo" has conflicting definitions\./);
 
     assert.throws(() => {
       firestore.doc('collectionId/documentId').update({
         'foo.bar': 'foobar',
         foo: 'foobar',
       });
-    }, /Field "foo" has conflicting definitions\./);
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo" has conflicting definitions\./);
 
     assert.throws(() => {
       firestore.doc('collectionId/documentId').update({
         'foo.bar': 'foobar',
         'foo.bar.foo': 'foobar',
       });
-    }, /Field "foo.bar" has conflicting definitions\./);
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo.bar" has conflicting definitions\./);
 
     assert.throws(() => {
       firestore.doc('collectionId/documentId').update({
         'foo.bar': {foo: 'foobar'},
         'foo.bar.foo': 'foobar',
       });
-    }, /Field "foo.bar.foo" has conflicting definitions\./);
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo.bar" has conflicting definitions\./);
 
     assert.throws(() => {
       firestore
         .doc('collectionId/documentId')
         .update('foo.bar', 'foobar', 'foo', 'foobar');
-    }, /Field "foo" has conflicting definitions\./);
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo" has conflicting definitions\./);
 
     assert.throws(() => {
       firestore
         .doc('collectionId/documentId')
-        .update('foo', 'foobar', 'foo.bar', 'foobar');
-    }, /Field "foo" has conflicting definitions\./);
+        .update('foo', {foobar: 'foobar'}, 'foo.bar', {foobar: 'foobar'});
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo" has conflicting definitions\./);
+
+    assert.throws(() => {
+      firestore
+        .doc('collectionId/documentId')
+        .update('foo', {foobar: 'foobar'}, 'foo.bar', {foobar: 'foobar'});
+    }, /Argument "dataOrField" is not a valid UpdateMap. Field "foo" has conflicting definitions\./);
   });
 
   it('with valid field paths', function() {