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.

Sign up for GitHub

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

Jump to bottom

Inconsistent transaction API between admin and js #2112

Closed
lookfirst opened this issue Aug 25, 2019 · 2 comments
Closed

Inconsistent transaction API between admin and js #2112

lookfirst opened this issue Aug 25, 2019 · 2 comments
Labels
api: firestore

Comments

@lookfirst
Copy link

While porting some code from admin to client so that I can take advantage of firestore rules from firestore functions (because admin ignores rules, sigh) and re-use some common code between client and server, I discovered that the admin sdk allows me to pass in a refOrQuery and the client sdk only allows a ref.

https://firebase.google.com/docs/reference/js/firebase.firestore.Transaction.html#get
get(documentRef: DocumentReference): Promise

https://googleapis.dev/nodejs/firestore/latest/Transaction.html#get
get(refOrQuery) → {Promise}

The client sdk is also missing transaction.getAll() and transaction.create():

https://googleapis.dev/nodejs/firestore/latest/Transaction.html#getAll
https://googleapis.dev/nodejs/firestore/latest/Transaction.html#create

It would be nice if the two apis were more consistent or heck... merged into one. Is there a reason they need to be separate?
@google-oss-bot
Copy link
Contributor

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

@lookfirst lookfirst mentioned this issue Aug 25, 2019
Open
@var-const
Copy link
Contributor

Thank you for your question. Unfortunately, it isn't really feasible merge the two implementations because they are quite different, despite the superficial similarities in the API. More specifically:

  • mobile SDK uses optimistic locking which is incompatible with queries; thus, refOrQuery is infeasible to implement in the client SDK;
  • the usual workaround for the lack of create in the client SDK is to read the document first, then use set if it doesn't exist. Having said that, implementing create is possible and it's something that's on our radar, but it's relatively low-priority because this functionality can be achieved using the existing API. If it's something you can contribute on, please feel free to send a PR;
  • getAll could potentially be implemented in the client SDK, and is something we might consider looking into in the future if there's more demand for this feature. Once again, if this is something you're interested in contributing to, you're very welcome to send a PR -- let us know if you need pointers on how to get started. The batch lookup RPC already exists, but this would need internal plumbing and exposure in the public API.

Note that the feature request to add transaction.getAll() is already tracked in #1176. transaction.create() is already tracked in #1324.

@firebase firebase locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@lookfirst @var-const @google-oss-bot