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
fix: repair implementation of Client.reserve_ids
#76
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -816,11 +816,18 @@ def do_something(entity): | |
kwargs["namespace"] = self.namespace | ||
return Query(self, **kwargs) | ||
|
||
def reserve_ids(self, complete_key, num_ids, retry=None, timeout=None): | ||
"""Reserve a list of IDs from a complete key. | ||
def reserve_ids_sequential(self, complete_key, num_ids, retry=None, timeout=None): | ||
"""Reserve a list of IDs sequentially from a complete key. | ||
|
||
This will reserve the key passed as `complete_key` as well as | ||
additional keys derived by incrementing the last ID in the path of | ||
`complete_key` sequentially to obtain the number of keys specified in | ||
`num_ids`. | ||
|
||
:type complete_key: :class:`google.cloud.datastore.key.Key` | ||
:param complete_key: Complete key to use as base for reserved IDs. | ||
:param complete_key: | ||
Complete key to use as base for reserved IDs. Key must use a | ||
numeric ID and not a string name. | ||
|
||
:type num_ids: int | ||
:param num_ids: The number of IDs to reserve. | ||
|
@@ -844,16 +851,70 @@ def reserve_ids(self, complete_key, num_ids, retry=None, timeout=None): | |
if complete_key.is_partial: | ||
raise ValueError(("Key is not Complete.", complete_key)) | ||
|
||
if complete_key.id is None: | ||
raise ValueError(("Key must use numeric id.", complete_key)) | ||
|
||
if not isinstance(num_ids, int): | ||
raise ValueError(("num_ids is not a valid integer.", num_ids)) | ||
|
||
key_class = type(complete_key) | ||
namespace = complete_key._namespace | ||
project = complete_key._project | ||
flat_path = list(complete_key._flat_path[:-1]) | ||
start_id = complete_key._flat_path[-1] | ||
|
||
key_pbs = [] | ||
for id in range(start_id, start_id + num_ids): | ||
path = flat_path + [id] | ||
key = key_class(*path, project=project, namespace=namespace) | ||
key_pbs.append(key.to_protobuf()) | ||
|
||
kwargs = _make_retry_timeout_kwargs(retry, timeout) | ||
self._datastore_api.reserve_ids(complete_key.project, key_pbs, **kwargs) | ||
|
||
return None | ||
|
||
complete_key_pb = complete_key.to_protobuf() | ||
complete_key_pbs = [complete_key_pb] * num_ids | ||
def reserve_ids(self, complete_key, num_ids, retry=None, timeout=None): | ||
"""Reserve a list of IDs sequentially from a complete key. | ||
|
||
self._datastore_api.reserve_ids( | ||
complete_key.project, complete_key_pbs, **kwargs | ||
DEPRECATED. Alias for :meth:`reserve_ids_sequential`. | ||
|
||
Please use either :meth:`reserve_ids_multi` (recommended) or | ||
:meth:`reserve_ids_sequential`. | ||
""" | ||
return self.reserve_ids_sequential( | ||
complete_key, num_ids, retry=retry, timeout=timeout | ||
) | ||
|
||
def reserve_ids_multi(self, complete_keys, retry=None, timeout=None): | ||
"""Reserve IDs from a list of complete keys. | ||
|
||
:type complete_keys: `list` of :class:`google.cloud.datastore.key.Key` | ||
:param complete_keys: | ||
Complete keys for which to reserve IDs. | ||
|
||
:type retry: :class:`google.api_core.retry.Retry` | ||
:param retry: | ||
A retry object used to retry requests. If ``None`` is specified, | ||
requests will be retried using a default configuration. | ||
|
||
:type timeout: float | ||
:param timeout: | ||
Time, in seconds, to wait for the request to complete. | ||
Note that if ``retry`` is specified, the timeout applies | ||
to each individual attempt. | ||
|
||
:rtype: class:`NoneType` | ||
:returns: None | ||
:raises: :class:`ValueError` if any of `complete_keys`` is not a | ||
Complete key. | ||
""" | ||
for complete_key in complete_keys: | ||
if complete_key.is_partial: | ||
raise ValueError(("Key is not Complete.", complete_key)) | ||
|
||
kwargs = _make_retry_timeout_kwargs(retry, timeout) | ||
key_pbs = [key.to_protobuf() for key in complete_keys] | ||
self._datastore_api.reserve_ids(complete_keys[0].project, key_pbs, **kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a concern that we aren't verifying that the keys match on things like project? We assume the first key represents the others right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about that. That is the assumption. Obviously the assumption might be wrong. But I figured the API would probably kick it back if it was. I can verify if that is or isn't the case. Or do the check here, regardless, if that makes us more comfortable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @crwilcox I've verified that the API will kick back an InvalidArgument error in this case. If it's just up to me I'd probably just leave it at that. If you think there should be a check in the client, though, let me know. |
||
|
||
return None |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,6 +102,20 @@ def test_allocate_ids(self): | |
self.assertEqual(len(unique_ids), num_ids) | ||
|
||
|
||
class TestDatastoreReserveIDs(TestDatastore): | ||
def test_reserve_ids_sequential(self): | ||
# Smoke test to make sure it doesn't blow up. No return value or | ||
# verifiable side effect to verify. | ||
num_ids = 10 | ||
Config.CLIENT.reserve_ids_sequential(Config.CLIENT.key("Kind", 1234), num_ids) | ||
|
||
def test_reserve_ids_multi(self): | ||
# Smoke test to make sure it doesn't blow up. No return value or | ||
# verifiable side effect to verify. | ||
keys = [Config.CLIENT.key("KIND", 1234), Config.CLIENT.key("KIND", 1235)] | ||
Config.CLIENT.reserve_ids_multi(keys) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prolly needs a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
|
||
class TestDatastoreSave(TestDatastore): | ||
@classmethod | ||
def setUpClass(cls): | ||
|
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.
Worth a
DeprecationWarningMessage
here?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.
d03607c