handle ignoreUndefinedProperties in set(merge: true) (#4347)

Previously any field in the document would be propagated into the
FieldMask regardless of whether or not it had an undefined value, which
led to the client acting as if the user had passed FieldValue.delete(),
which wasn't intended.

Fixes googleapis/nodejs-firestore#1392 for the JS SDK.

Author: wilhuff

.changeset/silent-cars-burn.md

@@ -0,0 +1,5 @@
+---
+'@firebase/firestore': patch
+---
+
+handle `ignoreUndefinedProperties` in `set({ merge: true })`. Previously this would behave as if the undefined value were `FieldValue.delete()`, which wasn't intended.

packages/firestore/src/api/user_data_reader.ts

@@ -738,6 +738,11 @@ export function parseData(
     // context.fieldMask and we return null as our parsing result.
     parseSentinelFieldValue(input, context);
     return null;
+  } else if (input === undefined && context.ignoreUndefinedProperties) {
+    // If the input is undefined it can never participate in the fieldMask, so
+    // don't handle this below. If `ignoreUndefinedProperties` is false,
+    // `parseScalarValue` will reject an undefined value.
+    return null;
   } else {
     // If context.path is null we are inside an array and we don't support
     // field mask paths more granular than the top-level array.
@@ -896,8 +901,6 @@ function parseScalarValue(
         value._key.path
       )
     };
-  } else if (value === undefined && context.ignoreUndefinedProperties) {
-    return null;
   } else {
     throw context.createError(
       `Unsupported field value: ${valueDescription(value)}`

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

@@ -398,6 +398,15 @@ apiDescribe('`undefined` properties', (persistence: boolean) => {
     });
   });
 
+  it('are ignored in set({ merge: true })', () => {
+    return withTestDocAndSettings(persistence, settings, async doc => {
+      await doc.set({ foo: 'foo', bar: 'unchanged' });
+      await doc.set({ foo: 'foo', bar: undefined }, { merge: true });
+      const docSnap = await doc.get();
+      expect(docSnap.data()).to.deep.equal({ foo: 'foo', bar: 'unchanged' });
+    });
+  });
+
   it('are ignored in update()', () => {
     return withTestDocAndSettings(persistence, settings, async doc => {
       await doc.set({});