From a7211d48e8eb49cd5d2a54806b5766bfe6499522 Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Thu, 8 Oct 2020 18:11:43 +0530 Subject: [PATCH 1/3] fix: from_string method of blob and bucket class --- google/cloud/storage/blob.py | 4 ++-- google/cloud/storage/bucket.py | 4 ++-- google/cloud/storage/client.py | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index b1e13788d..fa13a713b 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -330,7 +330,7 @@ def public_url(self): ) @classmethod - def from_string(cls, uri, client=None): + def from_string(cls, uri, client): """Get a constructor for blob object by URI. :type uri: str @@ -338,7 +338,7 @@ def from_string(cls, uri, client=None): :type client: :class:`~google.cloud.storage.client.Client` or ``NoneType`` - :param client: (Optional) The client to use. + :param client: The client to use. :rtype: :class:`google.cloud.storage.blob.Blob` :returns: The blob object created. diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index adf37d398..dd6017aab 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -612,7 +612,7 @@ def user_project(self): return self._user_project @classmethod - def from_string(cls, uri, client=None): + def from_string(cls, uri, client): """Get a constructor for bucket object by URI. :type uri: str @@ -620,7 +620,7 @@ def from_string(cls, uri, client=None): :type client: :class:`~google.cloud.storage.client.Client` or ``NoneType`` - :param client: (Optional) The client to use. + :param client: The client to use. :rtype: :class:`google.cloud.storage.bucket.Bucket` :returns: The bucket object created. diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index e522052da..f7691e7df 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -579,7 +579,7 @@ def download_blob_to_file(self, blob_or_uri, file_obj, start=None, end=None): try: blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end) except AttributeError: - blob = Blob.from_string(blob_or_uri) + blob = Blob.from_string(blob_or_uri, self) blob.download_to_file(file_obj, client=self, start=start, end=end) def list_blobs( From a83bbdf6ffd7a66b02e82848425bb1a4a9971bc5 Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Fri, 9 Oct 2020 17:35:56 +0530 Subject: [PATCH 2/3] fix: revert changes and apply another approach --- google/cloud/storage/blob.py | 10 ++++++---- google/cloud/storage/bucket.py | 4 ++-- google/cloud/storage/client.py | 2 +- tests/unit/test_blob.py | 3 ++- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index fa13a713b..7f5ef1faa 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -330,7 +330,7 @@ def public_url(self): ) @classmethod - def from_string(cls, uri, client): + def from_string(cls, uri, client=None): """Get a constructor for blob object by URI. :type uri: str @@ -338,7 +338,7 @@ def from_string(cls, uri, client): :type client: :class:`~google.cloud.storage.client.Client` or ``NoneType`` - :param client: The client to use. + :param client: (Optional) The client to use. :rtype: :class:`google.cloud.storage.blob.Blob` :returns: The blob object created. @@ -1598,6 +1598,7 @@ def _do_multipart_upload( :raises: :exc:`ValueError` if ``size`` is not :data:`None` but the ``stream`` has fewer than ``size`` bytes remaining. """ + client = self._require_client(client) if size is None: data = stream.read() else: @@ -1611,7 +1612,7 @@ def _do_multipart_upload( headers, object_metadata, content_type = info base_url = _MULTIPART_URL_TEMPLATE.format( - hostname=self.client._connection.API_BASE_URL, bucket_path=self.bucket.path + hostname=client._connection.API_BASE_URL, bucket_path=self.bucket.path ) name_value_pairs = [] @@ -1768,6 +1769,7 @@ def _initiate_resumable_upload( that was created * The ``transport`` used to initiate the upload. """ + client = self._require_client(client) if chunk_size is None: chunk_size = self.chunk_size if chunk_size is None: @@ -1780,7 +1782,7 @@ def _initiate_resumable_upload( headers.update(extra_headers) base_url = _RESUMABLE_URL_TEMPLATE.format( - hostname=self.client._connection.API_BASE_URL, bucket_path=self.bucket.path + hostname=client._connection.API_BASE_URL, bucket_path=self.bucket.path ) name_value_pairs = [] diff --git a/google/cloud/storage/bucket.py b/google/cloud/storage/bucket.py index dd6017aab..adf37d398 100644 --- a/google/cloud/storage/bucket.py +++ b/google/cloud/storage/bucket.py @@ -612,7 +612,7 @@ def user_project(self): return self._user_project @classmethod - def from_string(cls, uri, client): + def from_string(cls, uri, client=None): """Get a constructor for bucket object by URI. :type uri: str @@ -620,7 +620,7 @@ def from_string(cls, uri, client): :type client: :class:`~google.cloud.storage.client.Client` or ``NoneType`` - :param client: The client to use. + :param client: (Optional) The client to use. :rtype: :class:`google.cloud.storage.bucket.Bucket` :returns: The bucket object created. diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index f7691e7df..e522052da 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -579,7 +579,7 @@ def download_blob_to_file(self, blob_or_uri, file_obj, start=None, end=None): try: blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end) except AttributeError: - blob = Blob.from_string(blob_or_uri, self) + blob = Blob.from_string(blob_or_uri) blob.download_to_file(file_obj, client=self, start=start, end=end) def list_blobs( diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index f67b6501e..b3157a6d2 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -1974,6 +1974,7 @@ def test__do_multipart_upload_with_generation_not_match(self, mock_get_boundary) def test__do_multipart_upload_bad_size(self): blob = self._make_one(u"blob-name", bucket=None) + client = mock.Mock() data = b"data here hear hier" stream = io.BytesIO(data) @@ -1982,7 +1983,7 @@ def test__do_multipart_upload_bad_size(self): with self.assertRaises(ValueError) as exc_info: blob._do_multipart_upload( - None, stream, None, size, None, None, None, None, None, None + client, stream, None, size, None, None, None, None, None, None ) exc_contents = str(exc_info.exception) From 054c633bd7047363599991acf0671789d284c3e2 Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Mon, 12 Oct 2020 15:34:55 +0530 Subject: [PATCH 3/3] fix: add extra unit tests --- google/cloud/storage/blob.py | 2 +- google/cloud/storage/client.py | 2 +- tests/unit/test_blob.py | 55 +++++++++++++++++++++++----------- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 7f5ef1faa..ec428dd6a 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -1598,7 +1598,6 @@ def _do_multipart_upload( :raises: :exc:`ValueError` if ``size`` is not :data:`None` but the ``stream`` has fewer than ``size`` bytes remaining. """ - client = self._require_client(client) if size is None: data = stream.read() else: @@ -1607,6 +1606,7 @@ def _do_multipart_upload( msg = _READ_LESS_THAN_SIZE.format(size, len(data)) raise ValueError(msg) + client = self._require_client(client) transport = self._get_transport(client) info = self._get_upload_arguments(content_type) headers, object_metadata, content_type = info diff --git a/google/cloud/storage/client.py b/google/cloud/storage/client.py index e522052da..f7691e7df 100644 --- a/google/cloud/storage/client.py +++ b/google/cloud/storage/client.py @@ -579,7 +579,7 @@ def download_blob_to_file(self, blob_or_uri, file_obj, start=None, end=None): try: blob_or_uri.download_to_file(file_obj, client=self, start=start, end=end) except AttributeError: - blob = Blob.from_string(blob_or_uri) + blob = Blob.from_string(blob_or_uri, self) blob.download_to_file(file_obj, client=self, start=start, end=end) def list_blobs( diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index b3157a6d2..17eb9f41b 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -1816,6 +1816,7 @@ def _mock_transport(self, status_code, headers, content=b""): def _do_multipart_success( self, mock_get_boundary, + client=None, size=None, num_retries=None, user_project=None, @@ -1833,12 +1834,13 @@ def _do_multipart_success( blob = self._make_one(u"blob-name", bucket=bucket, kms_key_name=kms_key_name) self.assertIsNone(blob.chunk_size) - # Create mocks to be checked for doing transport. - transport = self._mock_transport(http_client.OK, {}) - # Create some mock arguments. - client = mock.Mock(_http=transport, _connection=_Connection, spec=["_http"]) - client._connection.API_BASE_URL = "https://storage.googleapis.com" + if not client: + # Create mocks to be checked for doing transport. + transport = self._mock_transport(http_client.OK, {}) + + client = mock.Mock(_http=transport, _connection=_Connection, spec=["_http"]) + client._connection.API_BASE_URL = "https://storage.googleapis.com" data = b"data here hear hier" stream = io.BytesIO(data) content_type = u"application/xml" @@ -1865,7 +1867,7 @@ def _do_multipart_success( ) # Check the mocks and the returned value. - self.assertIs(response, transport.request.return_value) + self.assertIs(response, client._http.request.return_value) if size is None: data_read = data self.assertEqual(stream.tell(), len(data)) @@ -1914,7 +1916,7 @@ def _do_multipart_success( + b"\r\n--==0==--" ) headers = {"content-type": b'multipart/related; boundary="==0=="'} - transport.request.assert_called_once_with( + client._http.request.assert_called_once_with( "POST", upload_url, data=payload, headers=headers, timeout=expected_timeout ) @@ -1972,9 +1974,15 @@ def test__do_multipart_upload_with_generation_not_match(self, mock_get_boundary) mock_get_boundary, if_generation_not_match=4, if_metageneration_not_match=4 ) + @mock.patch(u"google.resumable_media._upload.get_boundary", return_value=b"==0==") + def test__do_multipart_upload_with_client(self, mock_get_boundary): + transport = self._mock_transport(http_client.OK, {}) + client = mock.Mock(_http=transport, _connection=_Connection, spec=["_http"]) + client._connection.API_BASE_URL = "https://storage.googleapis.com" + self._do_multipart_success(mock_get_boundary, client=client) + def test__do_multipart_upload_bad_size(self): blob = self._make_one(u"blob-name", bucket=None) - client = mock.Mock() data = b"data here hear hier" stream = io.BytesIO(data) @@ -1983,7 +1991,7 @@ def test__do_multipart_upload_bad_size(self): with self.assertRaises(ValueError) as exc_info: blob._do_multipart_upload( - client, stream, None, size, None, None, None, None, None, None + None, stream, None, size, None, None, None, None, None, None ) exc_contents = str(exc_info.exception) @@ -1992,6 +2000,7 @@ def test__do_multipart_upload_bad_size(self): def _initiate_resumable_helper( self, + client=None, size=None, extra_headers=None, chunk_size=None, @@ -2024,14 +2033,17 @@ def _initiate_resumable_helper( object_metadata = blob._get_writable_metadata() blob._get_writable_metadata = mock.Mock(return_value=object_metadata, spec=[]) - # Create mocks to be checked for doing transport. resumable_url = "http://test.invalid?upload_id=hey-you" - response_headers = {"location": resumable_url} - transport = self._mock_transport(http_client.OK, response_headers) - - # Create some mock arguments and call the method under test. - client = mock.Mock(_http=transport, _connection=_Connection, spec=[u"_http"]) - client._connection.API_BASE_URL = "https://storage.googleapis.com" + if not client: + # Create mocks to be checked for doing transport. + response_headers = {"location": resumable_url} + transport = self._mock_transport(http_client.OK, response_headers) + + # Create some mock arguments and call the method under test. + client = mock.Mock( + _http=transport, _connection=_Connection, spec=[u"_http"] + ) + client._connection.API_BASE_URL = "https://storage.googleapis.com" data = b"hello hallo halo hi-low" stream = io.BytesIO(data) content_type = u"text/plain" @@ -2120,7 +2132,7 @@ def _initiate_resumable_helper( else: self.assertIsNone(retry_strategy.max_cumulative_retry) self.assertEqual(retry_strategy.max_retries, num_retries) - self.assertIs(transport, transport) + self.assertIs(client._http, transport) # Make sure we never read from the stream. self.assertEqual(stream.tell(), 0) @@ -2202,6 +2214,15 @@ def test__initiate_resumable_upload_with_generation_not_match(self): def test__initiate_resumable_upload_with_predefined_acl(self): self._initiate_resumable_helper(predefined_acl="private") + def test__initiate_resumable_upload_with_client(self): + resumable_url = "http://test.invalid?upload_id=hey-you" + response_headers = {"location": resumable_url} + transport = self._mock_transport(http_client.OK, response_headers) + + client = mock.Mock(_http=transport, _connection=_Connection, spec=[u"_http"]) + client._connection.API_BASE_URL = "https://storage.googleapis.com" + self._initiate_resumable_helper(client=client) + def _make_resumable_transport( self, headers1, headers2, headers3, total_bytes, data_corruption=False ):