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

Add missing generation arguments for bucket and blob methods. #7023

Closed
wants to merge 4 commits into from
Closed

Add missing generation arguments for bucket and blob methods. #7023

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81849bd
Add missing generation arguments for bucket and blob methods.
jmcarp Dec 25, 2018
231e8b4
Add generic query parameters to storage property helper.
jmcarp Jan 4, 2019
8bb96d0
Privatize storage query param helper; accept generation in blob const…
jmcarp Jan 13, 2019
31174f6
Add system tests for blob generations.
jmcarp Jan 18, 2019
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
26 changes: 16 additions & 10 deletions storage/google/cloud/storage/_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ class _PropertyMixin(object):
"""Abstract mixin for cloud storage classes with associated properties.

Non-abstract subclasses should implement:
- client
- path
- client
- user_project

:type name: str
:param name: The name of the object. Bucket names must start and end with a
Expand All @@ -71,6 +72,14 @@ def user_project(self):
"""Abstract getter for the object user_project."""
raise NotImplementedError

@property
def _query_params(self):
"""Default query parameters."""
params = {}
if self.user_project is not None:
params["userProject"] = self.user_project
return params
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is shadowed by both Blob and Bucket: let's make it virtual, e.g.:

    @property
    def _query_params(self):
        """Default query parameters."""
        raise NotImplementedError

That way we don't have to add test coverage for it, either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at how it is used in reload, I've changed my mind: lets just get rid of the duplicate version in Bucket.


def _require_client(self, client):
"""Check client or verify over-ride.

Expand All @@ -97,11 +106,10 @@ def reload(self, client=None):
``client`` stored on the current object.
"""
client = self._require_client(client)
query_params = self._query_params
# Pass only '?projection=noAcl' here because 'acl' and related
# are handled via custom endpoints.
query_params = {"projection": "noAcl"}
if self.user_project is not None:
query_params["userProject"] = self.user_project
query_params["projection"] = "noAcl"
api_response = client._connection.api_request(
method="GET", path=self.path, query_params=query_params, _target_object=self
)
Expand Down Expand Up @@ -148,11 +156,10 @@ def patch(self, client=None):
``client`` stored on the current object.
"""
client = self._require_client(client)
query_params = self._query_params
# Pass '?projection=full' here because 'PATCH' documented not
# to work properly w/ 'noAcl'.
query_params = {"projection": "full"}
if self.user_project is not None:
query_params["userProject"] = self.user_project
query_params["projection"] = "full"
update_properties = {key: self._properties[key] for key in self._changes}

# Make the API call.
Expand All @@ -178,9 +185,8 @@ def update(self, client=None):
``client`` stored on the current object.
"""
client = self._require_client(client)
query_params = {"projection": "full"}
if self.user_project is not None:
query_params["userProject"] = self.user_project
query_params = self._query_params
query_params["projection"] = "full"
api_response = client._connection.api_request(
method="PUT",
path=self.path,
Expand Down
33 changes: 30 additions & 3 deletions storage/google/cloud/storage/blob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,13 @@ class Blob(_PropertyMixin):
"""

def __init__(
self, name, bucket, chunk_size=None, encryption_key=None, kms_key_name=None
self,
name,
bucket,
chunk_size=None,
encryption_key=None,
kms_key_name=None,
generation=None,
):
name = _bytes_to_unicode(name)
super(Blob, self).__init__(name=name)
Expand All @@ -177,6 +183,9 @@ def __init__(
if kms_key_name is not None:
self._properties["kmsKeyName"] = kms_key_name

if generation is not None:
self._properties["generation"] = generation

@property
def chunk_size(self):
"""Get the blob's default chunk size.
Expand Down Expand Up @@ -257,6 +266,16 @@ def user_project(self):
"""
return self.bucket.user_project

@property
def _query_params(self):
"""Default query parameters."""
params = {}
if self.generation is not None:
params["generation"] = self.generation
if self.user_project is not None:
params["userProject"] = self.user_project
return params
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add explicit test coverage for the branches in this property.


@property
def public_url(self):
"""The public URL for this blob.
Expand Down Expand Up @@ -387,6 +406,9 @@ def exists(self, client=None):
# minimize the returned payload.
query_params = {"fields": "name"}

if self.generation:
query_params["generation"] = self.generation

if self.user_project is not None:
query_params["userProject"] = self.user_project
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this block should use the _query_params property, e.g.:

        # minimize the returned payload.
        query_params = self._query_params
        query_params["fields"] = "name"


Expand Down Expand Up @@ -423,7 +445,9 @@ def delete(self, client=None):
(propagated from
:meth:`google.cloud.storage.bucket.Bucket.delete_blob`).
"""
return self.bucket.delete_blob(self.name, client=client)
return self.bucket.delete_blob(
self.name, client=client, generation=self.generation
)

def _get_transport(self, client):
"""Return the client's transport.
Expand Down Expand Up @@ -1485,6 +1509,9 @@ def rewrite(self, source, token=None, client=None):
if token:
query_params["rewriteToken"] = token

if source.generation:
query_params["sourceGeneration"] = source.generation

if self.user_project is not None:
query_params["userProject"] = self.user_project

Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise here:

        query_params = self._query_params

        if token:
            query_params["rewriteToken"] = token

        if source.generation:
            query_params["sourceGeneration"] = source.generation

        if self.kms_key_name is not None:
            query_params["destinationKmsKeyName"] = self.kms_key_name

Expand Down Expand Up @@ -1533,7 +1560,7 @@ def update_storage_class(self, new_class, client=None):
raise ValueError("Invalid storage class: %s" % (new_class,))

# Update current blob's storage class prior to rewrite
self._patch_property('storageClass', new_class)
self._patch_property("storageClass", new_class)

# Execute consecutive rewrite operations until operation is done
token, _, _ = self.rewrite(self)
Expand Down
43 changes: 40 additions & 3 deletions storage/google/cloud/storage/bucket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,22 @@ def user_project(self):
"""
return self._user_project

def blob(self, blob_name, chunk_size=None, encryption_key=None, kms_key_name=None):
@property
def _query_params(self):
"""Default query parameters."""
params = {}
if self.user_project is not None:
params["userProject"] = self.user_project
return params
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add explicit test coverage for the branches in this property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per above, let's just delete this version of the property, which duplicates the one in the base class.


def blob(
self,
blob_name,
chunk_size=None,
encryption_key=None,
kms_key_name=None,
generation=None,
):
"""Factory constructor for blob object.

This comment was marked as spam.

Sign in to view


.. note::
Expand All @@ -368,6 +383,10 @@ def blob(self, blob_name, chunk_size=None, encryption_key=None, kms_key_name=Non
:param kms_key_name:
Optional resource name of KMS key used to encrypt blob's content.

:type generation: long
:param generation: Optional. If present, selects a specific revision of
this object.

:rtype: :class:`google.cloud.storage.blob.Blob`
:returns: The blob object created.
"""
Expand All @@ -377,6 +396,7 @@ def blob(self, blob_name, chunk_size=None, encryption_key=None, kms_key_name=Non
chunk_size=chunk_size,
encryption_key=encryption_key,
kms_key_name=kms_key_name,
generation=generation,
)

def notification(
Expand Down Expand Up @@ -549,7 +569,9 @@ def path(self):

return self.path_helper(self.name)

def get_blob(self, blob_name, client=None, encryption_key=None, **kwargs):
def get_blob(
self, blob_name, client=None, encryption_key=None, generation=None, **kwargs
):
"""Get a blob object by name.

This will return None if the blob doesn't exist:
Expand All @@ -574,6 +596,10 @@ def get_blob(self, blob_name, client=None, encryption_key=None, **kwargs):
See
https://cloud.google.com/storage/docs/encryption#customer-supplied.

:type generation: long
:param generation: Optional. If present, selects a specific revision of
this object.

:type kwargs: dict
:param kwargs: Keyword arguments to pass to the
:class:`~google.cloud.storage.blob.Blob` constructor.
Expand All @@ -586,6 +612,10 @@ def get_blob(self, blob_name, client=None, encryption_key=None, **kwargs):

if self.user_project is not None:
query_params["userProject"] = self.user_project

if generation is not None:
query_params["generation"] = generation

blob = Blob(
bucket=self, name=blob_name, encryption_key=encryption_key, **kwargs
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather create the blob with the expected generation here, and then use its _query_params property, e.g.:

        client = self._require_client(client)
        blob = Blob(
            bucket=self, name=blob_name, encryption_key=encryption_key, generation=generation, **kwargs
        )
        query_params = blob._query_params

        try:
            headers = _get_encryption_headers(encryption_key)
            response = client._connection.api_request(
                method="GET",
                path=blob.path,
                query_params=query_params,
                headers=headers,
                _target_object=blob,
            )

Now that I look at it more closely, I'm not sure why this method doesn't just call blob.reload().

Expand Down Expand Up @@ -791,7 +821,7 @@ def delete(self, force=False, client=None):
_target_object=None,
)

def delete_blob(self, blob_name, client=None):
def delete_blob(self, blob_name, client=None, generation=None):
"""Deletes a blob from the current bucket.

If the blob isn't found (backend 404), raises a
Expand All @@ -813,6 +843,10 @@ def delete_blob(self, blob_name, client=None):
:param client: Optional. The client to use. If not passed, falls back
to the ``client`` stored on the current bucket.

:type generation: long
:param generation: Optional. If present, permanently deletes a specific
revision of this object.

:raises: :class:`google.cloud.exceptions.NotFound` (to suppress
the exception, call ``delete_blobs``, passing a no-op
``on_error`` callback, e.g.:
Expand All @@ -825,6 +859,9 @@ def delete_blob(self, blob_name, client=None):
client = self._require_client(client)
query_params = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use self._query_params here as well, e.g.:

        query_params = self._query_params

        if generation is not None:
            query_params["generation"] = generation

        blob_path = Blob.path_helper(self.path, blob_name)


if generation is not None:
query_params["generation"] = generation

if self.user_project is not None:
query_params["userProject"] = self.user_project

Expand Down
59 changes: 52 additions & 7 deletions storage/tests/unit/test_blob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ def test_ctor_w_kms_key_name(self):
self.assertEqual(blob._encryption_key, None)
self.assertEqual(blob.kms_key_name, KMS_RESOURCE)

def test_ctor_with_generation(self):
BLOB_NAME = "blob-name"
GENERATION = 12345
bucket = _Bucket()
blob = self._make_one(BLOB_NAME, bucket=bucket, generation=GENERATION)
self.assertEqual(blob.generation, GENERATION)

def _set_properties_helper(self, kms_key_name=None):
import datetime
from google.cloud._helpers import UTC
Expand Down Expand Up @@ -576,7 +583,7 @@ def test_delete(self):
bucket._blobs[BLOB_NAME] = 1
blob.delete()
self.assertFalse(blob.exists())
self.assertEqual(bucket._deleted, [(BLOB_NAME, None)])
self.assertEqual(bucket._deleted, [(BLOB_NAME, None, blob.generation)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please duplicate this test, once with an explicit generation, and make this assertion be clear for both cases (None for the case where the blob is created without a generation).


def test__get_transport(self):
client = mock.Mock(spec=[u"_credentials", "_http"])
Expand Down Expand Up @@ -2295,6 +2302,44 @@ def test_rewrite_response_without_resource(self):
self.assertEqual(rewritten, 33)
self.assertEqual(size, 42)

def test_rewrite_generation(self):
SOURCE_BLOB = "source"
SOURCE_GENERATION = 42
DEST_BLOB = "dest"
DEST_BUCKET = "other-bucket"
TOKEN = "TOKEN"
RESPONSE = {
"totalBytesRewritten": 33,
"objectSize": 42,
"done": False,
"rewriteToken": TOKEN,
}
response = ({"status": http_client.OK}, RESPONSE)
connection = _Connection(response)
client = _Client(connection)
source_bucket = _Bucket(client=client)
source_blob = self._make_one(SOURCE_BLOB, bucket=source_bucket)
source_blob._set_properties({"generation": SOURCE_GENERATION})
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pass the generation to _make_one, e.g.:

        source_blob = self._make_one(SOURCE_BLOB, bucket=source_bucket, generation=GENERATION)

dest_bucket = _Bucket(client=client, name=DEST_BUCKET)
dest_blob = self._make_one(DEST_BLOB, bucket=dest_bucket)

token, rewritten, size = dest_blob.rewrite(source_blob)

self.assertEqual(token, TOKEN)
self.assertEqual(rewritten, 33)
self.assertEqual(size, 42)

kw, = connection._requested
self.assertEqual(kw["method"], "POST")
self.assertEqual(
kw["path"],
"/b/%s/o/%s/rewriteTo/b/%s/o/%s"
% (
(source_bucket.name, source_blob.name, dest_bucket.name, dest_blob.name)
),
)
self.assertEqual(kw["query_params"], {"sourceGeneration": SOURCE_GENERATION})

def test_rewrite_other_bucket_other_name_no_encryption_partial(self):
SOURCE_BLOB = "source"
DEST_BLOB = "dest"
Expand Down Expand Up @@ -2508,13 +2553,13 @@ def test_update_storage_class_large_file(self):
"objectSize": 84,
"done": False,
"rewriteToken": TOKEN,
"resource": {"storageClass": STORAGE_CLASS}
"resource": {"storageClass": STORAGE_CLASS},
}
COMPLETE_RESPONSE = {
"totalBytesRewritten": 84,
"objectSize": 84,
"done": True,
"resource": {"storageClass": STORAGE_CLASS}
"resource": {"storageClass": STORAGE_CLASS},
}
response_1 = ({"status": http_client.OK}, INCOMPLETE_RESPONSE)
response_2 = ({"status": http_client.OK}, COMPLETE_RESPONSE)
Expand All @@ -2534,7 +2579,7 @@ def test_update_storage_class_wo_encryption_key(self):
"totalBytesRewritten": 42,
"objectSize": 42,
"done": True,
"resource": {"storageClass": STORAGE_CLASS}
"resource": {"storageClass": STORAGE_CLASS},
}
response = ({"status": http_client.OK}, RESPONSE)
connection = _Connection(response)
Expand Down Expand Up @@ -2579,7 +2624,7 @@ def test_update_storage_class_w_encryption_key_w_user_project(self):
"totalBytesRewritten": 42,
"objectSize": 42,
"done": True,
"resource": {"storageClass": STORAGE_CLASS}
"resource": {"storageClass": STORAGE_CLASS},
}
response = ({"status": http_client.OK}, RESPONSE)
connection = _Connection(response)
Expand Down Expand Up @@ -3175,9 +3220,9 @@ def __init__(self, client=None, name="name", user_project=None):
self.path = "/b/" + name
self.user_project = user_project

def delete_blob(self, blob_name, client=None):
def delete_blob(self, blob_name, client=None, generation=None):
del self._blobs[blob_name]
self._deleted.append((blob_name, client))
self._deleted.append((blob_name, client, generation))


class _Signer(object):
Expand Down
Loading