firestore.CreateIfMissingOption should be replaced

[dhermes]

Paraphrased from @samtstern:

It's still possible to do

    city_ref.update({
        u'capital': True
    }, firestore.CreateIfMissingOption(True))

The other Firestore SDKs have removed this option and moved to an option on the set() operation called, merge(). For example, in the Android SDK:

db.collection("cities").document("BJ")
        .set(data, SetOptions.merge());

The new semantics make it so that the set() operation (and create() which some SDKs offer) is the only operation which can make a new document. The merge() option means that the data will be deep merged into any existing document, rather than just obliterating and replacing.

[samtstern]

A list of things that need to change:

• Total removal of firestore.CreateIfMissingOption(), which will require changing the signature of the update() methods for document references, batched writes, and transactions.
• Addition of a new option for set operations called MergeOption or similar, which will require changing the signature of the set() methods for document references, batched writes and transactions.

In spirit, users will be able to use the "set with merge" operation wherever they previously used the "update and create if missing", however there are a few key differences.

Update operations on key:value data where the key is a fieldPath. Set operates on object-like data. So take the following document:

{
  name: Sam
  address: {
    city: 'SF',
    state: 'CA'
  }
}

If I want to update only my city to LA and create the document if it does not exist, I would previously have done:

    ref.update({
        u'address.city': u'LA'
    }, firestore.CreateIfMissingOption(True))

Now I would do:

    ref.set({
        u'address': {
            u'city': u'LA'
        }
    }, firestore.MergeOption())

Notice that this is a deep merge meaning that if the address.state field was previously set it is left intact. The incorrect behavior would be replacing the entire address field with { city: 'LA' }.

[dhermes]

@jba FWIW @samtstern and I had previously discussed a way to map out what features are missing before starting the work on updating the API surface. As of now, this is the only "known" work to be done.

[schmidt-sebastian]

@dhermes Can you give us an estimate for the completion of this change?

[jba]

Does Python handle set/merge with deletes?

There are also the conformance tests. I started work on this (#4359) but can't get anyone to look at it.

[schmidt-sebastian]

Does Python handle set/merge with deletes?

There are also the conformance tests. I started work on this (#4359) but can't get anyone to look at it.

The Python API was not updated after the last API change. It would only handle them if the client never added validation to reject these sets in the first place, but I would have to dig through the code to find out.

[schmidt-sebastian]

@chemelnucfin Can you provide an estimate for this issue?

[chemelnucfin]

@schmidt-sebastian Sorry I didn't realize I should be looking at this. I'll take a look right away.

[chemelnucfin]

@samtstern Hi, I'm relatively new here so I'm not too familiar with protobuf, but it doesn't seem like there is a good option within protobuf in python to do what you want. MergeFrom creates 2 duplicate keys for the example you gave. I can try to fix it locally, but would it be better to work on the problem on the protobuf side?

[chemelnucfin]

This is basically what it looks like:
https://gist.github.com/chemelnucfin/71649e34f6b731c7a866124b3cde37d7

[schmidt-sebastian]

The Protobuf representation of the set() and the update() example that Sam provided is the same (and the one you provided in the update.pb example). The main difference is that set() takes a literal object value and that we extract the field paths from it, whereas update takes a list of field path/value pairs.

When a user calls set(..., merge), we only transmit the data that is being merged and perform an update with an automatically obtained field mask.

[chemelnucfin]

@schmidt-sebastian @samtstern
I think I miscommunicated my thoughts. In my example, original.pb is the 1st block in here.
The update.pb is the protobuf that should be in the 2nd block, and which should be the same as the getset() and update() example. The merged.pb is what happened when I tried to merge the two protobufs to update.

It doesn't seem like the current code also merge properly doing the update and create_if_missing, but I may have the code wrong.
create_and_update

[schmidt-sebastian]

In the create_and_update example you linked, you are updating the field address and replacing its entire contents with {city: LA}. If you only want to change the city itself and keep the state, you need to use the dotted field path notation: address.city:LA.

For set(..., merge) you would automatically infer the field mask address.city and do a deep merge.

[chemelnucfin]

Thanks, that made it a lot clearer.

[chemelnucfin]

@schmidt-sebastian After working on the PR for this, I realized there is a design question needed to be discussed. Are there likely going to be more write options in the future?

Currently, the way it is implemented is using a kwargs:
write option

However, MergeOption doesn't take an argument. What I was thinking is changing it to defaulting to MergeOption if the other two aren't provided. Would that be a good idea? I thought about having merge=None or merge=True being the only acceptable value, but that doesn't seem to be the best idea either.

[schmidt-sebastian]

We do something similar with the Mobile SDKs. Take a look here: https://github.com/firebase/firebase-js-sdk/blob/453677f005d04e47be2324b9c1577051139652c0/packages/firestore/src/api/user_data_converter.ts#L115

We have four different 'parse modes': Set, Update, MergeSet, and QueryValue. Looks like you could do something similar and do Set, Update, MergeSet and Delete.

For now, we don't have any plans on adding more options to SetOptions. But no promises :)

[chemelnucfin]

See #4851