[Firestore] API Feedback – Empty Object Merge Behavior

[amiller-gh]

Environment

• Operating System version: macOS Catalina 10.15.2
• Firebase SDK version: 7.17.2
• Firebase Product: Firestore
• Node.js version: 12.14.1
• NPM version: 6.14.7

Problem

It's been discussed in more than a few other places, but the current behavior of firestore.set('any/document'. { property: {} }, { merge: true }) is unintuitive and dangerous!

ref: firebase/firebase-js-sdk#1371 (comment)
ref: #365
ref: mesqueeb/vuex-easy-firestore#73
ref: firebase/firebase-js-sdk#1371
ref: firebase/firebase-js-sdk#1168

We nearly had a production error because of this database API quirk – we caught it through sheer luck. I wanted to flag it again and make the case for changing the behavior in a future major release.

Steps to reproduce:

The Good

firestore.set('any/document'. { property: { foo: 'bar' } }, { merge: true });
firestore.set('any/document'. { property: { biz: 'baz' } }, { merge: true });
console.log(await firestore.doc('any/document').get()).data());
// Expected Log: { foo: 'bar', biz: 'baz' }  👍 
// Actual Log: { foo: 'bar', biz: 'baz' }    😎 

The Bad

firestore.set('any/document'. { property: { foo: 'bar' } }, { merge: true });
firestore.set('any/document'. { property: {} }, { merge: true });
console.log(await firestore.doc('any/document').get()).data());
// Expected Log: { foo: 'bar' } 👍 
// Actual Log: { } 😱 

This is a very easy mistake to make, especially since it it not clearly called out in the docs, and depending on the setup a system may remove keys to patch update based on user input. This behavior forces extra (expensive!) input validation on deep objects that should not be necessary.

I would expect that any destructive operations with { merge: true } must be explicitly set using FieldValue.delete().

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[schmidt-sebastian]

@amiller-gh Thanks for filing this. We have had some lengthy discussions on how to best implement this. While we know that the current "drop on empty" can be surprising, we found it less surprising that simply ignoring empty objects. Since changing the behavior here would be a major breaking change, I don't think this is realistically going to happen.

We can certainly improve our documentation here. I will use this issue to do so.

[Zloka]

I would like to add that I also think this is strange, particularly for apps with offline support where writes are combined at a later time.

My use case, from the point of view of an offline device, is essentially "Create an empty placeholder object if it does not already exist. If it exists, do nothing".

Disclaimer: I could change my data schema so that an "empty placeholder object" would be replaced with no object and handle it accordingly when reading data, but that is besides the point that I also think this behavior is rather unintuitive.

[hiranya911]

@schmidt-sebastian can this be closed now?

[schmidt-sebastian]

We still need to improve the API documentation.

[JarnoRFB]

New firebase user chiming in: I came here because I just tripped over this behavior when updating a document with data from another query that could potentially be empty. I think it definitely makes sense to document the behavior more explicitly. However, I would agree with @amiller-gh that is behavior is really not intuitive and I think it is preferable to provide an intuitive API than to document an unintuitive one.

I would also like to bring algebraic argument: I think most people (including me) think of merging as a kind of addition operation and expect {} to behave as a neutral element. This also makes sense, since merging objects is basically a union of sets, which can be seen as equivalent to addition. In the current API {} is treated as an absorbing element which apparently trips many people off.

[schmidt-sebastian]

Documentation improvement coming.

We won't be able to adjust the behavior as this would be a breaking change across all of our APIs. While I agree that there are some downsides to the current behavior, we decided early on that the downsides are potentially less than just ignoring empty maps.