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(firestore): adds snapshot reads impl. #6718
Conversation
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.
Some general design questions first.
}, | ||
}) | ||
|
||
_, err := c.WithReadOptions(ReadTime(tm)).GetAll(ctx, []*DocumentRef{ |
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.
Does this call fail if it doesn't match what was added with srv.addRPC
above?
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.
Yes
firestore/transaction_test.go
Outdated
Database: db, | ||
Transaction: tid, | ||
}, | ||
&pb.CommitResponse{CommitTime: aTimestamp3}, |
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.
Could we declare a timestamp within this test? Otherwise it's hard to know how it relates to tm
.
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.
We could, but that wouldn't really exercise anything meaningful. The mock server simply returns the response object we define here--we would be testing that we set up the response in the mock server correctly, not the read time behavior.
This PR adds the
ReadOption
struct and allows callers to specify a read time for documents from the database.