From 5ca7b70612dde4f6f10ff257b67cb149b74dcb27 Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Mon, 10 Feb 2020 13:31:20 +0300 Subject: [PATCH 1/5] refactor(storage): deprecate Bucket.create() and requester_pays --- docs/snippets.py | 3 +- google/cloud/storage/bucket.py | 46 ++----- google/cloud/storage/client.py | 11 +- tests/unit/test_bucket.py | 240 ++++----------------------------- tests/unit/test_client.py | 132 +++++++++++++++++- 5 files changed, 181 insertions(+), 251 deletions(-) diff --git a/docs/snippets.py b/docs/snippets.py index 8171d5cf8..16b324023 100644 --- a/docs/snippets.py +++ b/docs/snippets.py @@ -51,8 +51,7 @@ def storage_get_started(client, to_delete): @snippet def client_bucket_acl(client, to_delete): bucket_name = "system-test-bucket" - bucket = client.bucket(bucket_name) - bucket.create() + client.create_bucket(bucket_name) # [START client_bucket_acl] client = storage.Client() diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index ed275e88f..44143bfc0 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -683,7 +683,7 @@ def create( predefined_acl=None, predefined_default_object_acl=None, ): - """Creates current bucket. + """DEPRECATED. Creates current bucket. If the bucket already exists, will raise :class:`google.cloud.exceptions.Conflict`. @@ -720,43 +720,23 @@ def create( Optional. Name of predefined ACL to apply to bucket's objects. See: https://cloud.google.com/storage/docs/access-control/lists#predefined-acl """ + warnings.warn( + "Bucket.create() is deprecated and will be removed in future." + "Use Client.create_bucket() instead.", + PendingDeprecationWarning, + stacklevel=1, + ) if self.user_project is not None: raise ValueError("Cannot create bucket with 'user_project' set.") client = self._require_client(client) - - if project is None: - project = client.project - - if project is None: - raise ValueError("Client project not set: pass an explicit project.") - - query_params = {"project": project} - - if predefined_acl is not None: - predefined_acl = BucketACL.validate_predefined(predefined_acl) - query_params["predefinedAcl"] = predefined_acl - - if predefined_default_object_acl is not None: - predefined_default_object_acl = DefaultObjectACL.validate_predefined( - predefined_default_object_acl - ) - query_params["predefinedDefaultObjectAcl"] = predefined_default_object_acl - - properties = {key: self._properties[key] for key in self._changes} - properties["name"] = self.name - - if location is not None: - properties["location"] = location - - api_response = client._connection.api_request( - method="POST", - path="/b", - query_params=query_params, - data=properties, - _target_object=self, + client.create_bucket( + bucket_or_name=self, + project=project, + location=location, + predefined_acl=predefined_acl, + predefined_default_object_acl=predefined_default_object_acl, ) - self._set_properties(api_response) def patch(self, client=None): """Sends all changed properties in a PATCH request. diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index 9c89b342d..6ee8f4cdf 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -14,6 +14,8 @@ """Client for interacting with the Google Cloud Storage API.""" +import warnings + import google.api_core.client_options from google.auth.credentials import AnonymousCredentials @@ -348,8 +350,8 @@ def create_bucket( ]): The bucket resource to pass or name to create. requester_pays (bool): - Optional. Whether requester pays for API requests for this - bucket and its blobs. + DEPRECATED. Optional. Whether requester pays for API + requests for this bucket and its blobs. project (str): Optional. The project under which the bucket is to be created. If not passed, uses the project set on the client. @@ -405,6 +407,11 @@ def create_bucket( raise ValueError("Client project not set: pass an explicit project.") if requester_pays is not None: + warnings.warn( + "requester_pays arg is deprecated.", + PendingDeprecationWarning, + stacklevel=1, + ) bucket.requester_pays = requester_pays query_params = {"project": project} diff --git a/tests/unit/test_bucket.py b/tests/unit/test_bucket.py index 68399b3c8..b817582a4 100644 --- a/tests/unit/test_bucket.py +++ b/tests/unit/test_bucket.py @@ -607,217 +607,6 @@ def api_request(cls, *args, **kwargs): expected_cw = [((), expected_called_kwargs)] self.assertEqual(_FakeConnection._called_with, expected_cw) - def test_create_w_user_project(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - USER_PROJECT = "user-project-123" - - client = Client(project=PROJECT) - client._base_connection = _Connection() - - bucket = self._make_one(client, BUCKET_NAME, user_project=USER_PROJECT) - - with self.assertRaises(ValueError): - bucket.create() - - def test_create_w_missing_client_project(self): - from google.cloud.storage.client import Client - - BUCKET_NAME = "bucket-name" - - client = Client(project=None) - bucket = self._make_one(client, BUCKET_NAME) - - with self.assertRaises(ValueError): - bucket.create() - - def test_create_w_explicit_project(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - OTHER_PROJECT = "other-project-123" - DATA = {"name": BUCKET_NAME} - connection = _make_connection(DATA) - - client = Client(project=PROJECT) - client._base_connection = connection - - bucket = self._make_one(client, BUCKET_NAME) - bucket.create(project=OTHER_PROJECT) - connection.api_request.assert_called_once_with( - method="POST", - path="/b", - query_params={"project": OTHER_PROJECT}, - data=DATA, - _target_object=bucket, - ) - - def test_create_w_explicit_location(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - LOCATION = "us-central1" - DATA = {"location": LOCATION, "name": BUCKET_NAME} - - connection = _make_connection( - DATA, "{'location': 'us-central1', 'name': 'bucket-name'}" - ) - - client = Client(project=PROJECT) - client._base_connection = connection - - bucket = self._make_one(client, BUCKET_NAME) - bucket.create(location=LOCATION) - - connection.api_request.assert_called_once_with( - method="POST", - path="/b", - data=DATA, - _target_object=bucket, - query_params={"project": "PROJECT"}, - ) - self.assertEqual(bucket.location, LOCATION) - - def test_create_hit(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - DATA = {"name": BUCKET_NAME} - connection = _make_connection(DATA) - client = Client(project=PROJECT) - client._base_connection = connection - - bucket = self._make_one(client=client, name=BUCKET_NAME) - bucket.create() - - connection.api_request.assert_called_once_with( - method="POST", - path="/b", - query_params={"project": PROJECT}, - data=DATA, - _target_object=bucket, - ) - - def test_create_w_extra_properties(self): - from google.cloud.storage.client import Client - - BUCKET_NAME = "bucket-name" - PROJECT = "PROJECT" - CORS = [ - { - "maxAgeSeconds": 60, - "methods": ["*"], - "origin": ["https://example.com/frontend"], - "responseHeader": ["X-Custom-Header"], - } - ] - LIFECYCLE_RULES = [{"action": {"type": "Delete"}, "condition": {"age": 365}}] - LOCATION = "eu" - LABELS = {"color": "red", "flavor": "cherry"} - STORAGE_CLASS = "NEARLINE" - DATA = { - "name": BUCKET_NAME, - "cors": CORS, - "lifecycle": {"rule": LIFECYCLE_RULES}, - "location": LOCATION, - "storageClass": STORAGE_CLASS, - "versioning": {"enabled": True}, - "billing": {"requesterPays": True}, - "labels": LABELS, - } - - connection = _make_connection(DATA) - client = Client(project=PROJECT) - client._base_connection = connection - - bucket = self._make_one(client=client, name=BUCKET_NAME) - bucket.cors = CORS - bucket.lifecycle_rules = LIFECYCLE_RULES - bucket.storage_class = STORAGE_CLASS - bucket.versioning_enabled = True - bucket.requester_pays = True - bucket.labels = LABELS - bucket.create(location=LOCATION) - - connection.api_request.assert_called_once_with( - method="POST", - path="/b", - query_params={"project": PROJECT}, - data=DATA, - _target_object=bucket, - ) - - def test_create_w_predefined_acl_invalid(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - DATA = {"name": BUCKET_NAME} - connection = _Connection(DATA) - client = Client(project=PROJECT) - client._base_connection = connection - bucket = self._make_one(client=client, name=BUCKET_NAME) - - with self.assertRaises(ValueError): - bucket.create(predefined_acl="bogus") - - def test_create_w_predefined_acl_valid(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - DATA = {"name": BUCKET_NAME} - connection = _Connection(DATA) - client = Client(project=PROJECT) - client._base_connection = connection - bucket = self._make_one(client=client, name=BUCKET_NAME) - bucket.create(predefined_acl="publicRead") - - kw, = connection._requested - self.assertEqual(kw["method"], "POST") - self.assertEqual(kw["path"], "/b") - expected_qp = {"project": PROJECT, "predefinedAcl": "publicRead"} - self.assertEqual(kw["query_params"], expected_qp) - self.assertEqual(kw["data"], DATA) - - def test_create_w_predefined_default_object_acl_invalid(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - DATA = {"name": BUCKET_NAME} - connection = _Connection(DATA) - client = Client(project=PROJECT) - client._base_connection = connection - bucket = self._make_one(client=client, name=BUCKET_NAME) - - with self.assertRaises(ValueError): - bucket.create(predefined_default_object_acl="bogus") - - def test_create_w_predefined_default_object_acl_valid(self): - from google.cloud.storage.client import Client - - PROJECT = "PROJECT" - BUCKET_NAME = "bucket-name" - DATA = {"name": BUCKET_NAME} - connection = _Connection(DATA) - client = Client(project=PROJECT) - client._base_connection = connection - bucket = self._make_one(client=client, name=BUCKET_NAME) - bucket.create(predefined_default_object_acl="publicRead") - - kw, = connection._requested - self.assertEqual(kw["method"], "POST") - self.assertEqual(kw["path"], "/b") - expected_qp = {"project": PROJECT, "predefinedDefaultObjectAcl": "publicRead"} - self.assertEqual(kw["query_params"], expected_qp) - self.assertEqual(kw["data"], DATA) - def test_acl_property(self): from google.cloud.storage.acl import BucketACL @@ -1966,6 +1755,35 @@ def test_versioning_enabled_getter(self): bucket = self._make_one(name=NAME, properties=before) self.assertEqual(bucket.versioning_enabled, True) + @mock.patch("warnings.warn") + def test_create_deprecated(self, mock_warn): + from google.cloud.storage.client import Client + + PROJECT = "PROJECT" + BUCKET_NAME = "bucket-name" + DATA = {"name": BUCKET_NAME} + connection = _make_connection(DATA) + client = Client(project=PROJECT) + client._base_connection = connection + + bucket = self._make_one(client=client, name=BUCKET_NAME) + bucket.create() + + connection.api_request.assert_called_once_with( + method="POST", + path="/b", + query_params={"project": PROJECT}, + data=DATA, + _target_object=bucket, + ) + + mock_warn.assert_called_with( + "Bucket.create() is deprecated and will be removed in future." + "Use Client.create_bucket() instead.", + PendingDeprecationWarning, + stacklevel=1, + ) + def test_versioning_enabled_setter(self): NAME = "name" bucket = self._make_one(name=NAME) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index f3c090ebb..a4856a063 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -552,6 +552,42 @@ def test_create_bucket_w_conflict(self): _target_object=mock.ANY, ) + @mock.patch("warnings.warn") + def test_create_requester_pays_deprecated(self, mock_warn): + from google.cloud.storage.bucket import Bucket + + project = "PROJECT" + credentials = _make_credentials() + client = self._make_one(project=project, credentials=credentials) + bucket_name = "bucket-name" + json_expected = {"name": bucket_name, "billing": {"requesterPays": True}} + http = _make_requests_session([_make_json_response(json_expected)]) + client._http_internal = http + + URI = "/".join( + [ + client._connection.API_BASE_URL, + "storage", + client._connection.API_VERSION, + "b?project=%s" % (project,), + ] + ) + + bucket = client.create_bucket(bucket_name, requester_pays=True) + + self.assertIsInstance(bucket, Bucket) + self.assertEqual(bucket.name, bucket_name) + self.assertTrue(bucket.requester_pays) + http.request.assert_called_once_with( + method="POST", url=URI, data=mock.ANY, headers=mock.ANY, timeout=mock.ANY + ) + json_sent = http.request.call_args_list[0][1]["data"] + self.assertEqual(json_expected, json.loads(json_sent)) + + mock_warn.assert_called_with( + "requester_pays arg is deprecated.", PendingDeprecationWarning, stacklevel=1 + ) + def test_create_bucket_w_predefined_acl_invalid(self): project = "PROJECT" bucket_name = "bucket-name" @@ -639,6 +675,97 @@ def test_create_bucket_w_explicit_location(self): ) self.assertEqual(bucket.location, location) + def test_create_bucket_w_explicit_project(self): + from google.cloud.storage.client import Client + + PROJECT = "PROJECT" + OTHER_PROJECT = "other-project-123" + BUCKET_NAME = "bucket-name" + DATA = {"name": BUCKET_NAME} + connection = _make_connection(DATA) + + client = Client(project=PROJECT) + client._base_connection = connection + + bucket = client.create_bucket(BUCKET_NAME, project=OTHER_PROJECT) + connection.api_request.assert_called_once_with( + method="POST", + path="/b", + query_params={"project": OTHER_PROJECT}, + data=DATA, + _target_object=bucket, + ) + + def test_create_w_extra_properties(self): + from google.cloud.storage.client import Client + from google.cloud.storage.bucket import Bucket + + BUCKET_NAME = "bucket-name" + PROJECT = "PROJECT" + CORS = [ + { + "maxAgeSeconds": 60, + "methods": ["*"], + "origin": ["https://example.com/frontend"], + "responseHeader": ["X-Custom-Header"], + } + ] + LIFECYCLE_RULES = [{"action": {"type": "Delete"}, "condition": {"age": 365}}] + LOCATION = "eu" + LABELS = {"color": "red", "flavor": "cherry"} + STORAGE_CLASS = "NEARLINE" + DATA = { + "name": BUCKET_NAME, + "cors": CORS, + "lifecycle": {"rule": LIFECYCLE_RULES}, + "location": LOCATION, + "storageClass": STORAGE_CLASS, + "versioning": {"enabled": True}, + "billing": {"requesterPays": True}, + "labels": LABELS, + } + + connection = _make_connection(DATA) + client = Client(project=PROJECT) + client._base_connection = connection + + bucket = Bucket(client=client, name=BUCKET_NAME) + bucket.cors = CORS + bucket.lifecycle_rules = LIFECYCLE_RULES + bucket.storage_class = STORAGE_CLASS + bucket.versioning_enabled = True + bucket.requester_pays = True + bucket.labels = LABELS + client.create_bucket(bucket, location=LOCATION) + + connection.api_request.assert_called_once_with( + method="POST", + path="/b", + query_params={"project": PROJECT}, + data=DATA, + _target_object=bucket, + ) + + def test_create_hit(self): + from google.cloud.storage.client import Client + + PROJECT = "PROJECT" + BUCKET_NAME = "bucket-name" + DATA = {"name": BUCKET_NAME} + connection = _make_connection(DATA) + client = Client(project=PROJECT) + client._base_connection = connection + + bucket = client.create_bucket(BUCKET_NAME) + + connection.api_request.assert_called_once_with( + method="POST", + path="/b", + query_params={"project": PROJECT}, + data=DATA, + _target_object=bucket, + ) + def test_create_bucket_w_string_success(self): from google.cloud.storage.bucket import Bucket @@ -655,16 +782,15 @@ def test_create_bucket_w_string_success(self): "b?project=%s" % (project,), ] ) - json_expected = {"name": bucket_name, "billing": {"requesterPays": True}} + json_expected = {"name": bucket_name} data = json_expected http = _make_requests_session([_make_json_response(data)]) client._http_internal = http - bucket = client.create_bucket(bucket_name, requester_pays=True) + bucket = client.create_bucket(bucket_name) self.assertIsInstance(bucket, Bucket) self.assertEqual(bucket.name, bucket_name) - self.assertTrue(bucket.requester_pays) http.request.assert_called_once_with( method="POST", url=URI, data=mock.ANY, headers=mock.ANY, timeout=mock.ANY ) From b8bdc2b0e7da5be1346145585bd0e5dc6376ce85 Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Mon, 10 Feb 2020 13:50:56 +0300 Subject: [PATCH 2/5] add_test_to_increase_cover --- tests/unit/test_bucket.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/unit/test_bucket.py b/tests/unit/test_bucket.py index b817582a4..5f4179216 100644 --- a/tests/unit/test_bucket.py +++ b/tests/unit/test_bucket.py @@ -1784,6 +1784,21 @@ def test_create_deprecated(self, mock_warn): stacklevel=1, ) + def test_create_w_user_project(self): + from google.cloud.storage.client import Client + + PROJECT = "PROJECT" + BUCKET_NAME = "bucket-name" + DATA = {"name": BUCKET_NAME} + connection = _make_connection(DATA) + client = Client(project=PROJECT) + client._base_connection = connection + + bucket = self._make_one(client=client, name=BUCKET_NAME) + bucket._user_project = "USER_PROJECT" + with self.assertRaises(ValueError): + bucket.create() + def test_versioning_enabled_setter(self): NAME = "name" bucket = self._make_one(name=NAME) From 634d8de391703a7c4ec432dbf544727be1ed2cf7 Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Wed, 12 Feb 2020 10:39:07 +0300 Subject: [PATCH 3/5] resolve conflicts --- google/cloud/storage/bucket.py | 3 ++- google/cloud/storage/client.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index d2f1a4438..1511cff37 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -742,7 +742,7 @@ def create( "Use Client.create_bucket() instead.", PendingDeprecationWarning, stacklevel=1, - )https://github.com/googleapis/python-storage/blob/master/google/cloud/storage/bucket.py + ) if self.user_project is not None: raise ValueError("Cannot create bucket with 'user_project' set.") @@ -753,6 +753,7 @@ def create( location=location, predefined_acl=predefined_acl, predefined_default_object_acl=predefined_default_object_acl, + timeout=timeout, ) def patch(self, client=None, timeout=_DEFAULT_TIMEOUT): diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index 350052b8e..ad133e88b 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -15,7 +15,7 @@ """Client for interacting with the Google Cloud Storage API.""" import warnings - +import functools import google.api_core.client_options from google.auth.credentials import AnonymousCredentials From 7e74e7ad94a737407b092954ecba5a251fa5cf16 Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Wed, 12 Feb 2020 10:44:34 +0300 Subject: [PATCH 4/5] resolve conflicts --- tests/unit/test_bucket.py | 1 + tests/unit/test_client.py | 3 +++ 2 files changed, 4 insertions(+) diff --git a/tests/unit/test_bucket.py b/tests/unit/test_bucket.py index ddd719b40..301dc61a1 100644 --- a/tests/unit/test_bucket.py +++ b/tests/unit/test_bucket.py @@ -1816,6 +1816,7 @@ def test_create_deprecated(self, mock_warn): query_params={"project": PROJECT}, data=DATA, _target_object=bucket, + timeout=self._get_default_timeout(), ) mock_warn.assert_called_with( diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 4cc2f7f05..3bb615928 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -726,6 +726,7 @@ def test_create_bucket_w_explicit_project(self): query_params={"project": OTHER_PROJECT}, data=DATA, _target_object=bucket, + timeout=self._get_default_timeout(), ) def test_create_w_extra_properties(self): @@ -776,6 +777,7 @@ def test_create_w_extra_properties(self): query_params={"project": PROJECT}, data=DATA, _target_object=bucket, + timeout=self._get_default_timeout(), ) def test_create_hit(self): @@ -796,6 +798,7 @@ def test_create_hit(self): query_params={"project": PROJECT}, data=DATA, _target_object=bucket, + timeout=self._get_default_timeout(), ) def test_create_bucket_w_string_success(self): From f32c009c4d6945493d2f21f254255544fb9b02e1 Mon Sep 17 00:00:00 2001 From: IlyaFaer Date: Wed, 12 Feb 2020 11:03:23 +0300 Subject: [PATCH 5/5] add new way of setting requester_pays into doclines --- google/cloud/storage/bucket.py | 2 +- google/cloud/storage/client.py | 7 ++++--- tests/unit/test_client.py | 4 +++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index 1511cff37..fae43104f 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -1888,7 +1888,7 @@ def requester_pays(self): def requester_pays(self, value): """Update whether requester pays for API requests for this bucket. - See https://cloud.google.com/storage/docs/ for + See https://cloud.google.com/storage/docs/using-requester-pays for details. :type value: convertible to boolean diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index ad133e88b..ed34ec16f 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -373,8 +373,9 @@ def create_bucket( ]): The bucket resource to pass or name to create. requester_pays (bool): - DEPRECATED. Optional. Whether requester pays for API - requests for this bucket and its blobs. + DEPRECATED. Use Bucket().requester_pays instead. + Optional. Whether requester pays for API requests for + this bucket and its blobs. project (str): Optional. The project under which the bucket is to be created. If not passed, uses the project set on the client. @@ -436,7 +437,7 @@ def create_bucket( if requester_pays is not None: warnings.warn( - "requester_pays arg is deprecated.", + "requester_pays arg is deprecated. Use Bucket().requester_pays instead.", PendingDeprecationWarning, stacklevel=1, ) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 3bb615928..4ba98f82e 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -612,7 +612,9 @@ def test_create_requester_pays_deprecated(self, mock_warn): self.assertEqual(json_expected, json.loads(json_sent)) mock_warn.assert_called_with( - "requester_pays arg is deprecated.", PendingDeprecationWarning, stacklevel=1 + "requester_pays arg is deprecated. Use Bucket().requester_pays instead.", + PendingDeprecationWarning, + stacklevel=1, ) def test_create_bucket_w_predefined_acl_invalid(self):