Firebase-js-sdk: Multi-Location .update() behaves like .set()

[crrobinson14]

Just found a gotcha in the JS SDK - haven't tested this anywhere else. The documentation for .set() vs .update() states that Set will overwrite an entire record if it's an object. Update will merge in just the fields being updated, leaving the others alone. This works as documented when called with a single-location update. However, it behaves differently when called with a multi-location update. In my app I have found that this code:

firebase.database.ref()
    .update({
        '/path/to/revisionA': {
            id: 'revisionA',
            name: 'Revision A',
            status: 'draft',
        },
        '/path/to/documentX': {
            draftRevisionId: 'A',
        }
    }).then(...)...;

will actually overwrite all of documentX such that it contains only field draftRevisionId. All other fields will be removed, as if .set() was called. A workaround is to directly reference fields to be updated:

firebase.database.ref()
    .update({
        '/path/to/revisionA': {
            id: 'revisionA',
            name: 'Revision A',
            status: 'draft',
        },
        '/path/to/documentX/draftRevisionId': 'A',
    }).then(...)...;

although obviously it would be more convenient (and less surprising!) if multi-location updates worked the same way in terms of merging fields as single-location updates.

[puf]

This is the expected behavior: when you specify paths as the keys, the database performs a set-operation on each specified path.

If you want to patch only part of a object, you will have to update each specific property with a separate path as you second snippet shows.

Changing the behavior to merge the value with the existing object at the path would break backwards compatibility. It would also require a syntax for indicating whether the value should replace the existing object at the location or whether it should be merged.

[crrobinson14]

Could I at least suggest the documentation be updated? This was totally not clear to me and I've been using Firebase for what, two years now? The documentation for single-location mutations clearly differentiates between set and update. Set overwrites. Update merges.

I'm SUPER unclear on why a multi-location update that overwrites shouldn't be called a multi-location set? Why wouldn't that be the syntax indicator you suggested?

[crrobinson14]

Here's the relevant doc section for update(), from https://firebase.google.com/docs/reference/js/firebase.database.Reference#update but also mirrored in other spots:

Writes multiple values to the Database at once.

The values argument contains multiple property-value pairs that will be written to the Database together. Each child property can either be a simple property (for example, "name") or a relative path (for example, "name/first") from the current location to the data to update.

As opposed to the set() method, update() can be use to selectively update only the referenced properties at the current location (instead of replacing all the child properties at the current location).

No part of this is clear to me that it is expected behavior for a multi-location update to overwrite entire records.

[puf]

Agreed that this is confusing.

I often summarize it as "An update() loops over the keys of the object you pass in and calls set() for each". That type of statement that works for developers who run into the problem that you have. But it's a vast oversimplification of the system behavior and thus ill suited for the actual docs.

I don't think we ever found a simple way to document it correctly, without it becoming even more confusing for the developers who never use this API for multi-location, deep updates.

[crrobinson14]

To not boil the ocean, I personally think simply updating this text:

As opposed to the set() method, update() can be use to selectively update only the referenced properties at the current location (instead of replacing all the child properties at the current location).

to something like:

As opposed to the set() method, update() can be use to selectively update only the referenced properties at the current location (instead of replacing all the child properties at the current location). Note that for multi-location updates this behavior works more like set(). A workaround for multi-location updates is to deeply reference each individual field to update in the call.

in each of the SDK docs. Even a simple comment would be enough to avoid the issue.

[mikelehen]

FWIW, I think perhaps the way to look at it is just that update() performs a compound set (a bunch of set()s as a single atomic write). Each entry in the update() map represents a single set() call. The path on the left will be completely replaced with the value on the right (like a set() would).

[crrobinson14]

Perhaps another way to address this in the docs is to say something exactly like that. You might hint that the operation being performed is "we will take the base ref and append to it each ref in the keys supplied, then set that new ref's value to what you supply". Because from that perspective, it does make sense.

My original point is less that something is broken than working unexpectedly, and "unexpectedly" is because the docs are unclear... Remember that until multi-location updates were announced, there was no conflict here. Thinking that "set is set, and update is merge in these fields" made perfect sense in the context of past developer experience with other services and the terminology used.

I think you can close this ticket, although I would dearly love to see this doc change, speaking on defense of other new devs...

[mikelehen]

Yeah. To be clear, I think this is great feedback and agree that we should try to improve the docs. :-)

[OlafvdSpek]

Why is it called update if it behaves more like set?

[mikelehen]

Well, I would say it behaves like multiple sets at different locations. So we'd need a name other than set(). To be honest, I'm sure there are better names than update(), and perhaps you could even suggest one, but unfortunately we are unlikely to be able to change it at this point given how many people are using the SDK and are accustomed to the current naming. But I agree that some clearer documentation is in order!

[crrobinson14]

I don't understand: why was it OK to add multi-path to .update() using the same name, with an array param indicating the behavior should be triggered, but .set() would need to be renamed?

Is there a reason it would not be acceptable to:

1. Document the side effect, which I think we're all on the same page with doing,
2. Keep .update() as it but deprecate the behavior.
3. Add array param handling to .set() and have it call exactly the code .update() already is.

[mikelehen]

Could you clarify the array param you're describing? The original change to update() was backwards-compatible and just an extension of the current functionality (instead of throwing an exception if you included '/' in your keys, we started interpreting the keys as /-separated paths).

[OlafvdSpek]

I'm sure there are better names than update(), and perhaps you could even suggest one,

.set()
.multiset()

but unfortunately we are unlikely to be able to change it at this point given how many people are using the SDK and are accustomed to the current naming.

Introducing the new name is backwards compatible.
Removing the old behavior is not, of course, so you'd have to deprecate that after a long while.

[richarddavenport]

This is interesting. I was unaware that .update() was doing this. I agree with @OlafvdSpek, having something like .multiset() is much more clear and could be documented better. I'm glad I randomly read over issues, otherwise I would have run into this, guaranteed! Thanks @crrobinson14

[mikelehen]

@slattouf95 To be clear, there is no bug that was discussed in issue. It was talking about naming confusion and potential docs improvements. So if you think you are encountering a bug, you should open a new issue and be sure to include repro steps. If you are confused about the behavior, perhaps you can ask a more specific question... and github may not be the best place for that. You could ask on stack overflow or the google group (https://groups.google.com/d/forum/google-cloud-firestore-discuss) or something.