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

feat: Support preconditions in set() #2087

Open
brettwillis opened this issue Jul 24, 2024 · 6 comments
Open

feat: Support preconditions in set() #2087

brettwillis opened this issue Jul 24, 2024 · 6 comments
Assignees
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@brettwillis
Copy link
Contributor

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 brettwillis added priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Jul 24, 2024
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Jul 24, 2024
@brettwillis
Copy link
Contributor Author

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.

@cherylEnkidu cherylEnkidu self-assigned this Jul 24, 2024
@cherylEnkidu
Copy link
Contributor

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?

@brettwillis
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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.

@cherylEnkidu
Copy link
Contributor

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!

@brettwillis
Copy link
Contributor Author

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?

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. priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants