Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle ignoreUndefinedProperties in set(merge: true) #4347

Merged
merged 4 commits into from
Jan 27, 2021

Conversation

wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jan 26, 2021

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.

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.
@changeset-bot
Copy link

changeset-bot bot commented Jan 26, 2021

🦋 Changeset detected

Latest commit: f211ccf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@firebase/firestore Patch
firebase Patch
@firebase/rules-unit-testing Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jan 26, 2021

Changeset File Check ✅

  • No modified packages are missing from the changeset file.
  • No changeset formatting errors detected.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 26, 2021

Size Analysis Report

Affected Products

No changes between base commit (749c7f3) and head commit (e8092b7).

@@ -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' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised the pre-commit hook didn't drop the quotes around bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be cleaner to pull the undefined check from parseScalarValue into this function. Something like:

if (lookslikeJson) { ... }
else if (input instanceof FieldValue) { ... }
else if (input == undefined && context.ignoreUndefinedProperties) {
    return null;
}  else { ... }

Optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Done.

@wilhuff wilhuff merged commit 9533688 into master Jan 27, 2021
@wilhuff wilhuff deleted the wilhuff/ignore-undefined-fix branch January 27, 2021 18:01
@google-oss-bot google-oss-bot mentioned this pull request Feb 2, 2021
@brcarey
Copy link

brcarey commented Feb 17, 2021

Hi folks, should this work in firebase-admin@9.5.0?

@schmidt-sebastian
Copy link
Contributor

It should, but you may need to delete your package lock file before re-installing your dependencies. The fix for @google-cloud/firestore (which, by the way, was googleapis/nodejs-firestore#1396) was released with version 4.9.1

@brcarey
Copy link

brcarey commented Feb 17, 2021

Awesome, works - much appreciated

@firebase firebase locked and limited conversation to collaborators Feb 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ignoreUndefinedProperties is misleading
4 participants