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
Firestore #1865
Conversation
0492a0e
to
cf4a6f5
Compare
Coverage increased (+0.05%) to 93.677% when pulling cf4a6f57e7fbe9500d3e7a2ef001b3107bace96e on firestore into 8b6dc68 on master. |
Coverage increased (+0.05%) to 93.677% when pulling f38d92dd369e7252788cd339cb04ddc147a50f46 on firestore into 8b6dc68 on master. |
Coverage increased (+0.05%) to 93.677% when pulling fd67403f85012678a137c284f95b0b10cb28df28 on firestore into 8b6dc68 on master. |
Coverage increased (+0.07%) to 93.692% when pulling ca7b8574e99b0143e9079dfcc8e07631468151aa on firestore into 8b6dc68 on master. |
@sebastian-schmidt @frankyn Can you please take a look at this now? |
|
||
Gem::Specification.new do |gem| | ||
gem.name = "google-cloud-firestore" | ||
gem.version = "0.6.8" |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
google-cloud-firestore/docs/toc.json
Outdated
{ | ||
"title": "V1beta1", | ||
"type": "google/cloud/firestore/v1", | ||
"patterns": ["\/firestore\/v1$", "\/firestore\/v1\/"], |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Will Review today. |
@blowmage I'm seeing a
Steps to reproduce:
Run IRB
Ruby version: |
@blowmage I'm still reviewing, and should have comments tomorrow. I'm running through samples written for other langues presented on https://cloud.google.com/firestore/docs/ to review the client library. I do have an open question, where are Firestore samples/documentation for the client library similar to other clients? |
Update to match the current gem dependencies.
Remove host, channel, and chan_creds as we are using the credentials object now.
The main object is going to be Database, so return it from the constructor. At some point in the future users will be able to create databases.
Call list_collection_ids and create Collection objects.
Add Database#doc to create a new Document::Reference object.
@blowmage follow-up on the gem install question. I tried requiring the gem again and then I was able to use the gem as expected. I'm trying to understand the |
There was an issue where an empty response with the transaction_id would be treated as a Document result. Fix implementation and add test coverage.
@frankyn The |
The documentation was showing the wrong return type. It returns Document::Snapshot objects, not Document::Refernce objects. Update the code examples to show accessing the snapshot data.
@frankyn fwiw, I just use rbenv gemset. |
@blowmage At which level (alpha, beta) is this library being released? (I noticed the generated |
I honestly don’t know what level it will be released. I was expecting another PR doing a documentation pass and polish before it is released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't know (or forgot) that there would be a subsequent PR for docs. I'll just submit these comments now, I think they are all for documentation.
end | ||
alias_method :document, :doc | ||
|
||
def add data = nil |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
end | ||
|
||
## | ||
# The project resource the Cloud Firestore batch belongs to. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# | ||
# All changes are accumulated in memory until the block passed to | ||
# {Database#batch} completes. Unlike transactions, batches are not | ||
# automatically retried. See {Database#batch}. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# path of the document, or a document reference object. | ||
# @param [Hash] data The document's fields and values. | ||
# @param [true, String|Symbol, Array<String|Symbol>] merge When provided | ||
# and `true` all data is merged with the existing docuemnt data. When |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# Write to document with the provided object values. If the document | ||
# does not exist, it will be created. By default, the provided data | ||
# overwrites existing data, but the provided data can be merged into the | ||
# existing document using the `merge` argument. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
## | ||
# Create a document with the provided object values. | ||
# | ||
# The batch will fail if the document already exists. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
It seems potentially misleading for # firestore.batch do |b|
# # Get a document reference
# nyc_ref = b.doc "cities/NYC"
#
# # Create a document
# b.create(nyc_ref, { name: "New York City" })
# end The semantics here are identical to that of
A batch can be understood as the complement of a read-only transaction. |
Make changes requested by the review process.
I do not. I think it would be confusing to add to the top-level README until there is a gem that users can install. |
I understand why you say that, but the ruby library is doing something extra that I think is more in-line with Rubyist expectations. But yes, the requirements document does indicate that the write batch operations should be specific to write operations: # Get a document reference
nyc_ref = firestore.doc "cities/NYC"
# Create a document in a batch
firestore.batch do |b|
b.create(nyc_ref, { name: "New York City" })
end This PR delivers that usage. However, I believe a more idiomatic approach is to make all writes that occur within a closure use the batch. Similar to the approach taken by ActiveRecord's transactions. To deliver something like this, objects created or retrieved within the batch will make writes on the batch as long as they are made within the closure: # Functionally equivalent to the previous example
firestore.batch do |b|
b.doc("cities/NYC").create({ name: "New York City" })
end |
It is the similarity to transactions that concerns me: Batches are not read-write transactions, but if the semantics are identical, with reads performed on the object yielded to the block, people unfamiliar with the difference may be misled. That's why I recommend removing the read methods from # Read existing documents
nyc_ref = firestore.doc "cities/NYC"
sf_ref = firestore.doc "cities/SF"
# Update multiple document in a batch rather than in a
# transaction to avoid contention
firestore.batch do |b|
b.update(nyc_ref, { name: "Big Apple" })
b.update(sf_ref, { name: "San Fran" })
end |
Your code comments are misleading. That code does not read the documents, it creates document reference objects. No reads are happening there. |
That's correct. |
@@ -0,0 +1,230 @@ | |||
# Copyright 2017, Google Inc. All rights reserved. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one open question about custom objects with te Ruby client library. It's not possible. Is this an expected state?
Reference: https://cloud.google.com/firestore/docs/manage-data/add-data#custom_objects
I also commented on the documentation and samples.
# | ||
# firestore.batch do |b| | ||
# # Create a query | ||
# query = b.where(:population, :>=, 1000000). |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# @param [String, Symbol] direction The direction to order the results | ||
# by. Optional. Default is ascending. | ||
# | ||
# @return [Query] A query with `order` called on it. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# | ||
# firestore.batch do |b| | ||
# # Create a document | ||
# b.update("cities/NYC", { name: "New York City" }, |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
## | ||
# Create a document with random document identifier. | ||
# | ||
# The batch will fail if the document already exists. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# Write to document with the provided object values. If the document | ||
# does not exist, it will be created. By default, the provided data | ||
# overwrites existing data, but the provided data can be merged into | ||
# the existing document using the `merge` argument. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# | ||
# All changes are accumulated in memory until the block passed to | ||
# {Database#batch} completes. Unlike transactions, batches are not | ||
# automatically retried. See {Database#batch}. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
I didn't see that in the original Firestore requirements. Is there any guidance on how this is supposed to work? Does the library automatically map a Document::Snapshot's data to the class? Is this something that is needed to release? Or can it be added afterwards? Here are some quick thoughts on how to possibly implement this: Probably the best is to include a module in your class. Another option is to see of the object can be represented as a Hash to use it as a Hash. The simplest way is to check if the object responds to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First couple of comments. Note that I only looked at the API surface and don't understand Ruby. More comments to follow on the rest of the files.
# | ||
# Batched writes have fewer failure cases than transactions and use | ||
# simpler code. They are not affected by contention issues, because they | ||
# don't depend on consistently reading any documents. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# The database resource the Cloud Firestore batch belongs to. | ||
# | ||
# @return [Database] database resource. | ||
def database |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# end | ||
# end | ||
# | ||
def cols |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# b.set(nyc_ref, { name: "New York City" }) | ||
# end | ||
# | ||
def col collection_path |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@closed = true | ||
return nil if @writes.empty? | ||
resp = service.commit @writes | ||
Convert.timestamp_to_time resp.commit_time |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# end | ||
# | ||
def docs &block | ||
query.run(&block) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# puts "#{city.document_id} has #{city[:population]} residents." | ||
# end | ||
# | ||
def get_all *docs, mask: nil, &block |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
def query | ||
ensure_context! | ||
|
||
Query.start(parent_path, context).from(collection_id) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# @example | ||
# require "google/cloud/firestore" | ||
# | ||
# firestore = Google::Cloud::Firestore.new |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
end | ||
end | ||
|
||
def create_writes doc_path, data |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Whether this is a requirement really depends on how we expect this client to be used. Is it easy to create Firestore objects without it? Or do users commonly have their own types that they want to serialize? We added in Java, since POJOs are very common, but left it out of most other clients. |
So I looked at the Python example again, and they don't actually accept the custom object, they call City = Struct.new(:name, :state, :country, :capital, :population) city = City.new("Los Angeles", "CA", "USA", false, 3900000)
db.col("cities").doc("LA").set(city.to_h) Would this approach be acceptable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the Python implementation a user should only need to implement a to_h
method for a custom class
and possibly a Hash to custom class
method. I think documented examples showing how to do this using a Struct with to_h
and Hash to a Struct is acceptable.
# Write to document with the provided object values. If the document | ||
# does not exist, it will be created. By default, the provided data | ||
# overwrites existing data, but the provided data can be merged into | ||
# the existing document using the `merge` argument. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
The documentation was a duplicate of the set method documentation.
# firestore.set(nyc_ref, { name: "New York City" }) | ||
# | ||
def col collection_path | ||
if collection_path.to_s.split("/").count.even? |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# puts "#{city.document_id} has #{city[:population]} residents." | ||
# end | ||
# | ||
def docs collection_path, &block |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# @param [String, Document::Reference] docs One or more strings | ||
# representing the path of the document, or document reference | ||
# objects. | ||
# @param [Array<String|Symbol>, String|Symbol] mask A list of field |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# # Create a document | ||
# firestore.create(nyc_ref, { name: "New York City" }) | ||
# | ||
def create doc, data |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# firestore.set("cities/NYC", { name: "New York City" }, | ||
# merge: [:name]) | ||
# | ||
def set doc, data, merge: nil |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
retry | ||
rescue Google::Cloud::InvalidArgumentError => err | ||
# Return if a previous call has succeeded | ||
return nil if retries > 0 |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# end | ||
# end | ||
# | ||
def read_only_transaction read_time: nil |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# puts "#{city.document_id} has #{city[:population]} residents." | ||
# end | ||
# | ||
def query |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# puts "#{city.document_id} has #{city[:population]} residents." | ||
# end | ||
# | ||
def select *fields |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
# puts "#{city.document_id} has #{city[:population]} residents." | ||
# end | ||
# | ||
def get obj |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This PR adds the Firestore Ruby client library.
The library also implements writing multiple changes in a batch:
It also implements support for transactions
And implements read-only transactions: