feat(firestore): Added support for read time #19851
Conversation
product-auto-label
bot
added
the
api: firestore
diptanshumittal
added
the
do not merge
diptanshumittal
removed
the
do not merge
|
|
||
| collection_group = firestore.collection_group(rand_col.collection_id) | ||
|
|
||
| partitions = collection_group.partitions 6, read_time: read_time |
There was a problem hiding this comment.
What if there's some time skew between the client and server? Will this test work or will it flake?
There was a problem hiding this comment.
To accomodate that we are pausing 1 sec after a DML operation or after setting the read_time. But if skew becomes larger than that, tests will start failing.
There was a problem hiding this comment.
Okay, we'll keep an eye on it. I'm just not sure I trust the clock on the Kokoro instances.
| return nil if time.nil? | ||
|
|
||
| # Force the object to be a Time object. | ||
| time = time.to_time.utc |
There was a problem hiding this comment.
What is this doing? What type could time be, where this kind of conversion would be needed?
I'm asking because all the read_time parameters above are documented to require type Time. This could be problematic by itself because users might want to pass a DateTime, or maybe even a Numeric (representing seconds since the posix Epoch). However, if you require Time, then it seems like this line wouldn't be needed. If this line (or something like it) is needed, and you're supporting some other classes, then those should be called out in the @param documentation.
There was a problem hiding this comment.
This was done keeping the user perspective in mind. If by mistake wrong data type is given as input, user gets a error like - undefined method 'usec' for String without the above conversion. But with above check , user gets - undefined method 'to_time' for String, which is easy to debug.
There was a problem hiding this comment.
If you want to enforce a type, just check the type using is_a? or respond_to? and raise TypeError. That would be more clear.
In this case, I don't think it would work at all, and in fact I'm a bit puzzled why the tests are passing. Because to_time isn't a method of Time, so if you pass Time.now as the read time, I think you'll get a NoMethodError.
There was a problem hiding this comment.
Then i will update the type check and use is_a? to raise the error.
Checked the ruby docs, they haven't mentioned about to_time method but it is there in the Time class and returns self.
There was a problem hiding this comment.
Then i will update the type check and use
is_a?to raise the error. Checked the ruby docs, they haven't mentioned aboutto_timemethod but it is there in theTimeclass and returnsself.
Really?
▶ irb
irb(main):001:0> RUBY_VERSION
=> "3.1.3"
irb(main):002:0> Time.now.to_time
(irb):2:in `<main>': undefined method `to_time' for 2023-01-13 12:37:57.34993 -0800:Time (NoMethodError)
Did you mean? to_i
from /Users/dazuma/.asdf/installs/ruby/3.1.3/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
from /Users/dazuma/.asdf/installs/ruby/3.1.3/bin/irb:25:in `load'
from /Users/dazuma/.asdf/installs/ruby/3.1.3/bin/irb:25:in `<main>'
irb(main):003:0>
There was a problem hiding this comment.
Oh, it looks like it's present if the date library has been loaded.
▶ irb
irb(main):001:0> RUBY_VERSION
=> "3.1.3"
irb(main):002:0> Time.now.to_time
(irb):2:in `<main>': undefined method `to_time' for 2023-01-13 12:37:57.34993 -0800:Time (NoMethodError)
Did you mean? to_i
from /Users/dazuma/.asdf/installs/ruby/3.1.3/lib/ruby/gems/3.1.0/gems/irb-1.4.1/exe/irb:11:in `<top (required)>'
from /Users/dazuma/.asdf/installs/ruby/3.1.3/bin/irb:25:in `load'
from /Users/dazuma/.asdf/installs/ruby/3.1.3/bin/irb:25:in `<main>'
irb(main):003:0> require "date"
=> true
irb(main):004:0> Time.now.to_time
=> 2023-01-13 12:39:20.768914 -0800
irb(main):005:0>
which is an annoying quirk in Ruby's standard library. I don't think we should be depending on it.
google-cloud-firestore/test/google/cloud/firestore/client/transaction_test.rb
Show resolved
Hide resolved
|
|
||
| collection_group = firestore.collection_group(rand_col.collection_id) | ||
|
|
||
| partitions = collection_group.partitions 6, read_time: read_time |
There was a problem hiding this comment.
Okay, we'll keep an eye on it. I'm just not sure I trust the clock on the Kokoro instances.
| return nil if time.nil? | ||
|
|
||
| # Force the object to be a Time object. | ||
| time = time.to_time.utc |
There was a problem hiding this comment.
If you want to enforce a type, just check the type using is_a? or respond_to? and raise TypeError. That would be more clear.
In this case, I don't think it would work at all, and in fact I'm a bit puzzled why the tests are passing. Because to_time isn't a method of Time, so if you pass Time.now as the read time, I think you'll get a NoMethodError.
| from: [Google::Cloud::Firestore::V1::StructuredQuery::CollectionSelector.new(collection_id: "users", all_descendants: false)] | ||
| ) | ||
| firestore_mock.expect :run_query, query_results_enum, run_query_args(expected_query, new_transaction: read_only_transaction_opt) | ||
| # firestore_mock.expect :commit, empty_commit_resp, commit_args(transaction: transaction_id) |
There was a problem hiding this comment.
Please don't leave commented out code. Either uncomment or delete the line. If the line is meant to be disabled temporarily, include a clear comment detailing why it's disabled and when it should be re-enabled.
There was a problem hiding this comment.
My bad.. Forgot to remove the line. Will fix this.
| def read_time_to_timestamp read_time | ||
| return nil if read_time.nil? | ||
|
|
||
| raise TypeError, "read_time is expected to be a Time object" unless read_time.is_a? Time |
There was a problem hiding this comment.
Thinking about this some more, I'm now actually inclined not even to have this current check here, but just let it say NoMethodError: usec if users pass a non-Time-compatible object. Ruby types are duck-types, and it's not common to check for specific classes. But we can just leave it in for now; removing it later is not a breaking change.
No description provided.