nodejs-firestore icon indicating copy to clipboard operation
nodejs-firestore copied to clipboard

feat: Support preconditions in `set()`

Open brettwillis opened this issue 1 year ago • 6 comments

There is a bit of a gap in functionality between the update() and set() APIs.

  • With set(), we can control merging or full replacement of the document, but cannot control preconditions.
  • With update() we can control preconditions, but cannot control merging (it always does a shallow merge).

However, what if we want to to a complete replacement of the document (i.e. set() without merge) but enforce a precondition that the document must already exist?

If one makes extensive use of undefined values and wants a exists: true precondition, then one has no other choice but to use update() and then must be very careful to replace all instances of undefined with FieldValue.delete().

I would propose including the lastUpdateTime and exists precondition fields in the SetOptions so that we can apply preconditions on a set operation, e.g.:

export type SetOptions =
    | ({ merge?: boolean } & Precondition)
    | ({ mergeFields?: Array<string | FieldPath> } & Precondition);

Subsequently, a minor win of then using set(..., { exists: true }) instead of update(...) is saving the bandwidth and overhead of calculating and transmitting the field mask with the request to the backend.

brettwillis avatar Jul 24 '24 04:07 brettwillis

I suppose another option: add a merge: false or similar option to update() to disable top-level merging. However update() also has the key-value-pair arguments option so would make it less sensible.

brettwillis avatar Jul 24 '24 07:07 brettwillis

Hi @brettwillis ,

Could you please provide more context of the advantages or the use cases comparing

Use getDoc to check the existence first then calling setDoc

and

setDoc with precondition check?

cherylEnkidu avatar Jul 24 '24 16:07 cherylEnkidu

@cherylEnkidu thanks for your reply! Yes an atomic read-write (get() and then set() within a transaction) can also effectively enforce an existence "precondition" there a couple of fundamental differences:

  • It relies there are no bugs in the application code. It's trivial for a simple operation, but in a complex app with an operation potentially spanning many interacting modules (or just some complex logic), having the precondition also enforced at a database level provides safety nets for overwriting data or whatever if there are bugs in the code. Think: error instead of silently overwriting in case of bug.
  • In other cases one might just desire to do a write without knowing the previous contents and therefore performing the read introduces un-necessary latency and cost.

So yes doing an atomic read-write is an option, just like doing update() and replacing all undefined with FieldValue.delete() is also an option but both are inconvenient workarounds with their own drawbacks and extra complexity due to this missing use case.

brettwillis avatar Jul 24 '24 20:07 brettwillis

@cherylEnkidu another use-case:

Say one wants to do a deep merge, using atomic FieldValue operations (without reading and holding a lock on the document) and to ensure that the document exists rather than creating one if it doesn't exist (i.e. precondition).

That is like so:

// FIXME: we cannot enforce "exists" precondition with this API
ref.set({
  counters: {
    counter1: FieldValue.increment(1),
    counter2: FieldValue.increment(-1),
    // don't touch counter3..N
  }
}, { merge: true });

The only way to do this currently is to awkwardly reshape the data into an array of key-value pairs to pass into the update('counters.counter1', FieldValue.increment(1), 'counters.count2', FieldValue.increment(-1)) API. Gets awkward if it's a dynamic and complex/deep structure. Can be simplified with an exists: true precondition on the set() API.

brettwillis avatar Jul 31 '24 04:07 brettwillis

Hi @brettwillis ,

Thank you for providing the use case context! The team discussed this feature request, we think it is reasonable to support it but it may not get a relative high priority due to 2 reasons:

  1. Firestore has supported workarounds using Update and transaction

  2. Unfortunately the current API difference between create, set, and update is very small, so adding the existence precondition check for set will make it more complicated.

I have log b/357006681 to track the request internally, if any developer has more use cases, please feel free to reply to this github ticket, it will help us better prioritize feature requests. Thank you!

cherylEnkidu avatar Aug 02 '24 07:08 cherylEnkidu

Thanks @cherylEnkidu is this a case where community contribution would be accepted, or does it need to go through the core team due to API design?

brettwillis avatar Aug 03 '24 03:08 brettwillis