From 2f0aaf43c54a5b60a03fb614fa2b07fa0799034a Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Tue, 26 Jan 2021 14:34:44 -0800 Subject: [PATCH 1/4] handle ignoreUndefinedProperties in set(merge: true) 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. --- packages/firestore/src/api/user_data_reader.ts | 6 +++++- packages/firestore/test/integration/api/fields.test.ts | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index c58da2e4209..af319042fba 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -741,7 +741,11 @@ export function parseData( } 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. - if (context.path) { + // + // If the input is undefined it can never participate in the fieldMask. With + // `ignoreUndefinedProperties` set to false, `parseScalarValue` will reject + // an undefined value. + if (context.path && input !== undefined) { context.fieldMask.push(context.path); } diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index a1f46f4d0c7..e51e474f1bb 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/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({}); From 3dca7caae8cfe2d90343da4b082c6657b51047e1 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Tue, 26 Jan 2021 14:43:44 -0800 Subject: [PATCH 2/4] Add changeset --- .changeset/silent-cars-burn.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/silent-cars-burn.md diff --git a/.changeset/silent-cars-burn.md b/.changeset/silent-cars-burn.md new file mode 100644 index 00000000000..df9e0a74d51 --- /dev/null +++ b/.changeset/silent-cars-burn.md @@ -0,0 +1,7 @@ +--- +'@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. From c5df0fda0c2a191cc5e19f298c27c17bd23eec56 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Tue, 26 Jan 2021 15:44:53 -0800 Subject: [PATCH 3/4] Single line changeset. --- .changeset/silent-cars-burn.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.changeset/silent-cars-burn.md b/.changeset/silent-cars-burn.md index df9e0a74d51..a526ac37e8f 100644 --- a/.changeset/silent-cars-burn.md +++ b/.changeset/silent-cars-burn.md @@ -2,6 +2,4 @@ '@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. +handle `ignoreUndefinedProperties` in `set({ merge: true })`. Previously this would behave as if the undefined value were `FieldValue.delete()`, which wasn't intended. From f211ccfe25e653a59a5610fc90144f45480df3c4 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Tue, 26 Jan 2021 15:52:43 -0800 Subject: [PATCH 4/4] Review feedback --- packages/firestore/src/api/user_data_reader.ts | 13 ++++++------- .../firestore/test/integration/api/fields.test.ts | 4 ++-- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index af319042fba..ddb41efb927 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -738,14 +738,15 @@ 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. - // - // If the input is undefined it can never participate in the fieldMask. With - // `ignoreUndefinedProperties` set to false, `parseScalarValue` will reject - // an undefined value. - if (context.path && input !== undefined) { + if (context.path) { context.fieldMask.push(context.path); } @@ -900,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)}` diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index e51e474f1bb..6d92def2493 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -400,8 +400,8 @@ 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 }); + 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' }); });