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

fix: handle ignoreUndefinedProperties in set(merge: true) #1396

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

wilhuff
Copy link
Contributor

@wilhuff wilhuff commented Jan 26, 2021

Previously any field in the document would be propagated into the DocumentMask 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 #1392

@wilhuff wilhuff added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Jan 26, 2021
@wilhuff wilhuff requested review from a team as code owners January 26, 2021 20:53
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 26, 2021
@wilhuff wilhuff force-pushed the wilhuff/ignore-undefined-fix branch from 966bf92 to 6e40aee Compare January 26, 2021 20:54
@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #1396 (a72cc82) into master (ea60d71) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1396   +/-   ##
=======================================
  Coverage   98.51%   98.51%           
=======================================
  Files          32       32           
  Lines       19374    19378    +4     
  Branches     1273     1360   +87     
=======================================
+ Hits        19086    19090    +4     
  Misses        284      284           
  Partials        4        4           
Impacted Files Coverage Δ
dev/src/document.ts 98.68% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea60d71...a72cc82. Read the comment docs.

// If the value is undefined it can never participate in the document
// mask. With ignoreUndefinedProperties set to true, this shouldn't
// affect the document mask. With ignoreUndefinedProperties set to
// false, we'll throw an error when trying to create a proto from the
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This error is thrown first by validateDocumentData: "Cannot use "undefined" as a Firestore value (found in field "bar"). If you want to ignore undefined values, enable ignoreUndefinedProperties."

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.

Previously any field in the document would be propagated into the
DocumentMask 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ignoreUndefinedProperties is misleading
2 participants