Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: support autoconversion of Entity to Key for purposes of delete & delete_multi #123

Merged
merged 7 commits into from Dec 4, 2020
Merged

feat: support autoconversion of Entity to Key for purposes of delete & delete_multi #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
066df30
feat: support autoconversion of Entity to Key for purposes of delete,…
crwilcox Nov 26, 2020
d289c9e
Update google/cloud/datastore/client.py
crwilcox Nov 30, 2020
b18ab26
Merge branch 'master' into allow-entity-object-to-delete
crwilcox Dec 4, 2020
394fd32
test: update typing, use entity delete path
crwilcox Dec 4, 2020
2c7d335
fix: remove warning
crwilcox Dec 4, 2020
8ddbfc0
test: add unit test for delete multi with an entity
crwilcox Dec 4, 2020
cba99c5
fix: lint/black
crwilcox Dec 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions google/cloud/datastore/client.py
View file
Open in desktop
Expand Up @@ -622,7 +622,8 @@ def delete(self, key, retry=None, timeout=None):
The backend API does not make a distinction between a single key or
multiple keys in a commit request.

:type key: :class:`google.cloud.datastore.key.Key`
:type key: :class:`google.cloud.datastore.key.Key`, :class:`google.cloud.datastore.entity.Entity`

:param key: The key to be deleted from the datastore.

:type retry: :class:`google.api_core.retry.Retry`
Expand All @@ -643,7 +644,7 @@ def delete(self, key, retry=None, timeout=None):
def delete_multi(self, keys, retry=None, timeout=None):
"""Delete keys from the Cloud Datastore.

:type keys: list of :class:`google.cloud.datastore.key.Key`
:type keys: list of :class:`google.cloud.datastore.key.Key`, :class:`google.cloud.datastore.entity.Entity`
:param keys: The keys to be deleted from the Datastore.

:type retry: :class:`google.api_core.retry.Retry`
Expand Down Expand Up @@ -671,6 +672,9 @@ def delete_multi(self, keys, retry=None, timeout=None):
current.begin()

for key in keys:
if isinstance(key, Entity):
# If the key is in fact an Entity, the key can be extracted.
key = key.key
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • [L683] This works but may result in occasional snarky comments. I would not, however, rewrite it as key = getattr(key, 'key') cuz that's worse.
  • [L646] Would any changes be req'd in this part of the docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may result in occasional snarky comments.

@wescpy I don't quite understand. Could you elaborate.

As for docstr, that is part of the open conversation. We can either

  1. make this a stealth change to help smooth migration. As written with the warning, I wouldn't want to add to docs
  2. say this is actually supported, in which case, no warning should happen, but we should specify in types you can hand an entity as well.

I would be fine to call this supported, but started from the 'smoothing' over as lightest touch.

Copy link

@wescpy wescpy Dec 1, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snarky meaning some people may criticize naming choice in key = key.key while it's perfectly valid IMO. Anyway, it was an aside and a mostly irrelevant comment... pls ignore. For the other one, I'm leaning #2. I'd also like to hear from members of the community as well, esp. those who work w/Datastore regularly. In particular, I'd like to know whether this is something they find useful.

current.delete(key)

if not in_batch:
Expand Down
6 changes: 2 additions & 4 deletions tests/system/test_system.py
View file
Open in desktop
Expand Up @@ -72,9 +72,8 @@ def setUpModule():


def tearDownModule():
keys = [entity.key for entity in Config.TO_DELETE]
with Config.CLIENT.transaction():
Config.CLIENT.delete_multi(keys)
Config.CLIENT.delete_multi(Config.TO_DELETE)


class TestDatastore(unittest.TestCase):
Expand All @@ -83,8 +82,7 @@ def setUp(self):

def tearDown(self):
with Config.CLIENT.transaction():
keys = [entity.key for entity in self.case_entities_to_delete]
Config.CLIENT.delete_multi(keys)
Config.CLIENT.delete_multi(self.case_entities_to_delete)


class TestDatastoreAllocateIDs(TestDatastore):
Expand Down
18 changes: 18 additions & 0 deletions tests/unit/test_client.py
View file
Open in desktop
Expand Up @@ -961,6 +961,24 @@ def test_delete_multi_w_existing_transaction(self):
self.assertEqual(mutated_key, key._key)
client._datastore_api_internal.commit.assert_not_called()

def test_delete_multi_w_existing_transaction_entity(self):
from google.cloud.datastore.entity import Entity

creds = _make_credentials()
client = self._make_one(credentials=creds)
client._datastore_api_internal = _make_datastore_api()

key = _Key()
entity = Entity(key=key)

with _NoCommitTransaction(client) as CURR_XACT:
result = client.delete_multi([entity])

self.assertIsNone(result)
mutated_key = _mutated_pb(self, CURR_XACT.mutations, "delete")
self.assertEqual(mutated_key, key._key)
client._datastore_api_internal.commit.assert_not_called()

def test_allocate_ids_w_partial_key(self):
num_ids = 2

Expand Down