feat: Added read_time as a parameter to various calls (synchronous/base classes) #1013
Conversation
product-auto-label
bot
added
size: l
between sync/async
| @@ -20,6 +20,7 @@ | |||
| """ | |||
| from __future__ import annotations | |||
|
|
|||
| from datetime import datetime | |||
There was a problem hiding this comment.
nit: just importing datetime as a module is probably better? This import overrides the "datetime" module name with the class name, which would get a bit weird if we have to use other datetime imports in the future. And it makes some of the type annotations a bit ambiguous
But let me know what you think
| read_time (Optional[datetime]): If set, reads documents as they were at the given | ||
| time. This must be a microsecond precision timestamp within the past one hour, | ||
| or if Point-in-Time Recovery is enabled, can additionally be a whole minute | ||
| timestamp within the past 7 days. |
There was a problem hiding this comment.
A couple questions:
- how are timezones handled? Do you expect "aware" or "naive" datetimes?
- I just did a quick search, but it seems like datetime.datetime also has microsecond precision. Do we need to mention the precision here?
| field_paths: Iterable[str] | None = None, | ||
| transaction: Transaction | None = None, | ||
| field_paths: Iterable[str] = None, | ||
| transaction: Transaction = None, |
There was a problem hiding this comment.
why remove None from these? Doesn't that cause a mypy error?
| retry: retries.Retry | object | None = gapic_v1.method.DEFAULT, | ||
| timeout: float | None = None, | ||
| timeout: float = None, |
There was a problem hiding this comment.
same comment: isn't this a type violation?
| read_time (Optional[datetime.datetime]): If set, reads documents as they were at the given | ||
| time. This must be a microsecond precision timestamp within the past one hour, or | ||
| if Point-in-Time Recovery is enabled, can additionally be a whole minute timestamp | ||
| within the past 7 days. For the most accurate results, use UTC timezone. |
There was a problem hiding this comment.
- I'm confused by this comment. Why "most accurate results". Wouldn't the wrong timezone give wrong results? How are timezones meant to be used?
- Shouldn't this docstring be consistent with others? I don't see UTC note this in the previous files
| # make sure we didn't skip assertions in inner function | ||
| assert inner_fn_ran is True | ||
|
|
||
|
|
There was a problem hiding this comment.
it would be good to test a read_time that is far in the past, or in the future, to make sure the exception is raised as you expect it to be
| def _collections_helper(page_size=None, retry=None, timeout=None, database=None): | ||
| @pytest.mark.parametrize("database", [None, "somedb"]) | ||
| def test_documentreference_get_with_read_time(database): | ||
| _get_helper(read_time=DatetimeWithNanoseconds.now(), database=database) |
There was a problem hiding this comment.
Why use from DatetimeWithNanoseconds here instead of the regular datetime?
This PR adds
read_timeto API calls that map to various Firestore gRPC calls whose request objects containread_timeas a field.Calls with
read_timeadded:AggregationQuery.get/streamClient.get_allClient.collectionsCollection.list_documentsCollection.get/streamDocumentReference.getDocumentReference.collectionsQuery.get/streamQuery.list_partitionsTransaction.get_allTransaction.getPartially fixes #775 for synchronous/base classes, async classes will be in a separate PR.