Skip to content

fix: propagate blob.generation in Bucket.delete_blobs #817

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

Closed
cojenco wants to merge 3 commits into from
Closed

fix: propagate blob.generation in Bucket.delete_blobs #817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f8275ad
fix: propagate blob.generation in Bucket.delete_blobs
cojenco Jun 15, 2022
d0a013c
update test and add comments
cojenco Jun 15, 2022
b7d646d
handle bucket.delete with force true
cojenco Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions google/cloud/storage/bucket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1578,6 +1578,9 @@ def delete(
) % (self._MAX_OBJECTS_FOR_ITERATION,)
raise ValueError(message)

# Convert to a list of blob names as we want to delete the latest versions of each blob.
# Blob generations propagated from the list op may no longer represent the latest versions.
blobs = [blob.name for blob in blobs]
# Ignore 404 errors on delete.
self.delete_blobs(
blobs,
Expand Down Expand Up @@ -1787,11 +1790,16 @@ def delete_blobs(
for blob in blobs:
try:
blob_name = blob
generation = None
if not isinstance(blob_name, str):
blob_name = blob.name
if blob.generation:
generation = blob.generation
Copy link
Contributor

@andrewsg andrewsg Jun 23, 2022

Choose a reason for hiding this comment

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

I'm still uncertain whether we should change behavior in this way, though I do agree the current behavior is bad. If we do make this change, can we skip the "take the name from the blob and recreate it" runaround and just run delete on the blob objects directly? This seems unnecessarily convoluted.

Copy link
Contributor Author

@cojenco cojenco Jun 23, 2022

Choose a reason for hiding this comment

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

I understand your concerns. Unfortunately, with the current setup, it will be difficult to bypass "take the name from the blob...". Reasons being (1) params blobs can be a list of str blob_name OR blob instances, (2) bucket.delete_blob() only takes str blob_name whereas blob.delete() requires a blob instance.

Copy link
Contributor Author

@cojenco cojenco Jun 23, 2022

Choose a reason for hiding this comment

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

As discussed, I'm wondering if adding a flag to the method will better ensure backwards compatibility, but I'm also concerned that it will make this even more convoluted >"<


self.delete_blob(
blob_name,
client=client,
generation=generation,
if_generation_match=next(if_generation_match, None),
if_generation_not_match=next(if_generation_not_match, None),
if_metageneration_match=next(if_metageneration_match, None),
Expand Down
3 changes: 2 additions & 1 deletion tests/conformance/test_conformance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,8 @@ def bucket_delete_blob(client, _preconditions, **resources):
def bucket_delete_blobs(client, _preconditions, **resources):
object = resources.get("object")
bucket = client.bucket(resources.get("bucket").name)
sources = [object]
# Generation can be propagated from blob instances
sources = [object.name]
source_generations = [object.generation]
if _preconditions:
bucket.delete_blobs(sources, if_generation_match=source_generations)
Expand Down
99 changes: 96 additions & 3 deletions tests/unit/test_bucket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1437,7 +1437,11 @@ def test_delete_hit_w_force_delete_blobs(self):
client = mock.Mock(spec=["_delete_resource"])
client._delete_resource.return_value = None
bucket = self._make_one(client=client, name=name)
blobs = [mock.Mock(spec=[]), mock.Mock(spec=[])]
blob = mock.Mock(spec=["name", "generation"])
blob.name = "blob-name"
blob.generation = 1512565576797178
blobs = [blob]
blob_names = [blob.name]
bucket.list_blobs = mock.Mock(return_value=iter(blobs))
bucket.delete_blobs = mock.Mock(return_value=None)

Expand All @@ -1453,7 +1457,44 @@ def test_delete_hit_w_force_delete_blobs(self):
)

bucket.delete_blobs.assert_called_once_with(
blobs,
blob_names,
on_error=mock.ANY,
client=client,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
)

expected_query_params = {}
client._delete_resource.assert_called_once_with(
bucket.path,
query_params=expected_query_params,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
_target_object=None,
)

def test_delete_hit_w_force_no_blobs(self):
name = "name"
client = mock.Mock(spec=["_delete_resource"])
client._delete_resource.return_value = None
bucket = self._make_one(client=client, name=name)
no_blobs = []
bucket.list_blobs = mock.Mock(return_value=iter(no_blobs))
bucket.delete_blobs = mock.Mock(return_value=None)

result = bucket.delete(force=True)

self.assertIsNone(result)

bucket.list_blobs.assert_called_once_with(
max_results=bucket._MAX_OBJECTS_FOR_ITERATION + 1,
client=client,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
)

bucket.delete_blobs.assert_called_once_with(
no_blobs,
on_error=mock.ANY,
client=client,
timeout=self._get_default_timeout(),
Expand All @@ -1474,11 +1515,13 @@ def test_delete_w_force_w_user_project_w_miss_on_blob(self):

name = "name"
blob_name = "blob-name"
generation = 1512565576797178
client = mock.Mock(spec=["_delete_resource"])
client._delete_resource.return_value = None
bucket = self._make_one(client=client, name=name)
blob = mock.Mock(spec=["name"])
blob = mock.Mock(spec=["name", "generation"])
blob.name = blob_name
blob.generation = generation
blobs = [blob]
bucket.list_blobs = mock.Mock(return_value=iter(blobs))
bucket.delete_blob = mock.Mock(side_effect=NotFound("testing"))
Expand All @@ -1490,6 +1533,7 @@ def test_delete_w_force_w_user_project_w_miss_on_blob(self):
bucket.delete_blob.assert_called_once_with(
blob_name,
client=client,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
Expand Down Expand Up @@ -1649,6 +1693,7 @@ def test_delete_blobs_hit_w_explicit_client_w_timeout(self):
bucket.delete_blob.assert_called_once_with(
blob_name,
client=client,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
Expand Down Expand Up @@ -1693,6 +1738,7 @@ def test_delete_blobs_w_generation_match_w_retry(self):
call_1 = mock.call(
blob_name,
client=None,
generation=None,
if_generation_match=generation_number,
if_generation_not_match=None,
if_metageneration_match=None,
Expand All @@ -1703,6 +1749,7 @@ def test_delete_blobs_w_generation_match_w_retry(self):
call_2 = mock.call(
blob_name2,
client=None,
generation=None,
if_generation_match=generation_number2,
if_generation_not_match=None,
if_metageneration_match=None,
Expand Down Expand Up @@ -1730,6 +1777,7 @@ def test_delete_blobs_w_generation_match_none(self):
call_1 = mock.call(
blob_name,
client=None,
generation=None,
if_generation_match=generation_number,
if_generation_not_match=None,
if_metageneration_match=None,
Expand All @@ -1740,6 +1788,47 @@ def test_delete_blobs_w_generation_match_none(self):
call_2 = mock.call(
blob_name2,
client=None,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
if_metageneration_not_match=None,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
)
bucket.delete_blob.assert_has_calls([call_1, call_2])

def test_delete_blobs_w_generation(self):
name = "name"
blob_name = "blob-name"
blob_name2 = "blob-name2"
generation_number = 1512565576797178
client = mock.Mock(spec=[])
bucket = self._make_one(client=client, name=name)
bucket.delete_blob = mock.Mock()
blob = mock.Mock(spec=["name", "generation"])
blob.name = blob_name
blob.generation = generation_number
blob2 = mock.Mock(spec=["name", "generation"])
blob2.name = blob_name2
blob2.generation = None
bucket.delete_blobs([blob, blob2])

call_1 = mock.call(
blob_name,
client=None,
generation=generation_number,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
if_metageneration_not_match=None,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
)
call_2 = mock.call(
blob_name2,
client=None,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
Expand All @@ -1766,6 +1855,7 @@ def test_delete_blobs_miss_wo_on_error(self):
call_1 = mock.call(
blob_name,
client=None,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
Expand All @@ -1776,6 +1866,7 @@ def test_delete_blobs_miss_wo_on_error(self):
call_2 = mock.call(
blob_name2,
client=None,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
Expand Down Expand Up @@ -1804,6 +1895,7 @@ def test_delete_blobs_miss_w_on_error(self):
call_1 = mock.call(
blob_name,
client=None,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
Expand All @@ -1814,6 +1906,7 @@ def test_delete_blobs_miss_w_on_error(self):
call_2 = mock.call(
blob_name2,
client=None,
generation=None,
if_generation_match=None,
if_generation_not_match=None,
if_metageneration_match=None,
Expand Down