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

fix(storage): anonymous credentials for private bucket #107

Merged
merged 12 commits into from
Apr 25, 2020
Merged

fix(storage): anonymous credentials for private bucket #107

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3204edc
fix(storage): anonymous credentials for private bucket
HemangChothani Apr 15, 2020
134066e
fix(storage): use API_BASE_URL for hostname
HemangChothani Apr 16, 2020
85729b7
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Apr 16, 2020
dbd74fa
fix(storage): nit
HemangChothani Apr 17, 2020
1d1ae94
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Apr 17, 2020
ec45809
fix(storage): fix system test
HemangChothani Apr 20, 2020
95f12c4
Merge branch 'master' into storage_issue_102
frankyn Apr 20, 2020
6cc573b
Merge branch 'master' into storage_issue_102
frankyn Apr 22, 2020
c293e2f
Merge branch 'master' of https://github.com/googleapis/python-storage…
HemangChothani Apr 23, 2020
0588395
Merge branch 'storage_issue_102' of https://github.com/q-logic/python…
HemangChothani Apr 23, 2020
6e8213b
fix(storage): revert changes
HemangChothani Apr 24, 2020
dc32e92
Merge branch 'master' into storage_issue_102
frankyn Apr 24, 2020
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
20 changes: 12 additions & 8 deletions google/cloud/storage/blob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
from google.cloud._helpers import _rfc3339_to_datetime
from google.cloud._helpers import _to_bytes
from google.cloud.exceptions import NotFound
from google.cloud.storage._helpers import _get_storage_host
from google.cloud.storage._helpers import _PropertyMixin
from google.cloud.storage._helpers import _scalar_property
from google.cloud.storage._helpers import _convert_to_timestamp
Expand All @@ -70,12 +69,11 @@
from google.cloud.storage.constants import REGIONAL_LEGACY_STORAGE_CLASS
from google.cloud.storage.constants import STANDARD_STORAGE_CLASS

_STORAGE_HOST = _get_storage_host()

_API_ACCESS_ENDPOINT = "https://storage.googleapis.com"
_DEFAULT_CONTENT_TYPE = u"application/octet-stream"
_DOWNLOAD_URL_TEMPLATE = _STORAGE_HOST + u"/download/storage/v1{path}?alt=media"
_BASE_UPLOAD_TEMPLATE = _STORAGE_HOST + u"/upload/storage/v1{bucket_path}/o?uploadType="
_DOWNLOAD_URL_TEMPLATE = u"{hostname}/download/storage/v1{path}?alt=media"
_BASE_UPLOAD_TEMPLATE = u"{hostname}/upload/storage/v1{bucket_path}/o?uploadType="
_MULTIPART_URL_TEMPLATE = _BASE_UPLOAD_TEMPLATE + u"multipart"
_RESUMABLE_URL_TEMPLATE = _BASE_UPLOAD_TEMPLATE + u"resumable"
# NOTE: "acl" is also writeable but we defer ACL management to
Expand Down Expand Up @@ -662,7 +660,9 @@ def _get_download_url(self):
"""
name_value_pairs = []
if self.media_link is None:
base_url = _DOWNLOAD_URL_TEMPLATE.format(path=self.path)
base_url = _DOWNLOAD_URL_TEMPLATE.format(
hostname=self.client._connection.API_BASE_URL, path=self.path
)
if self.generation is not None:
name_value_pairs.append(("generation", "{:d}".format(self.generation)))
else:
Expand Down Expand Up @@ -790,11 +790,11 @@ def download_to_file(

:raises: :class:`google.cloud.exceptions.NotFound`
"""
download_url = self._get_download_url()
headers = _get_encryption_headers(self._encryption_key)
headers["accept-encoding"] = "gzip"

transport = self._get_transport(client)
download_url = self._get_download_url()
try:
self._do_download(
transport, file_obj, download_url, headers, start, end, raw_download
Expand Down Expand Up @@ -1055,7 +1055,9 @@ def _do_multipart_upload(
info = self._get_upload_arguments(content_type)
headers, object_metadata, content_type = info

base_url = _MULTIPART_URL_TEMPLATE.format(bucket_path=self.bucket.path)
base_url = _MULTIPART_URL_TEMPLATE.format(
hostname=self.client._connection.API_BASE_URL, bucket_path=self.bucket.path
)
name_value_pairs = []

if self.user_project is not None:
Expand Down Expand Up @@ -1190,7 +1192,9 @@ def _initiate_resumable_upload(
if extra_headers is not None:
headers.update(extra_headers)

base_url = _RESUMABLE_URL_TEMPLATE.format(bucket_path=self.bucket.path)
base_url = _RESUMABLE_URL_TEMPLATE.format(
hostname=self.client._connection.API_BASE_URL, bucket_path=self.bucket.path
)
name_value_pairs = []

if self.user_project is not None:
Expand Down
2 changes: 1 addition & 1 deletion google/cloud/storage/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,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, client=self)
Copy link
Member

Choose a reason for hiding this comment

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

was this an accidental miss in implementation? Seems like it but want to confirm.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at it again, client is passed below to download_to_file(). The change doesn't look necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the problem arise here , if i didn't pass the client so bucket class has received None

python-storage/google/cloud/storage/blob.py

Line 351 in 18b1e07

bucket = Bucket(client, name=netloc)

so when call _require_client() method it find the client object which passed in download_to_file() so hostname=self.client._connection.API_BASE_URL line hit the error as self.client found None

Copy link
Member

@frankyn frankyn Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Gotcha, when I raised that you should revert my last comment, I was referring to updating the method _get_download_url() to accept client parameter because even though _get_transport calls _require_client() it doesn't update self.client with a value if it's None.

I think you may want to pass client returned from _require_client() to _get_download_url().

Thank you for your patience.

blob.download_to_file(file_obj, client=self, start=start, end=end)

def list_blobs(
Expand Down
15 changes: 10 additions & 5 deletions tests/unit/test_blob.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1003,7 +1003,8 @@ def test_download_to_file_with_failure(self):

def test_download_to_file_wo_media_link(self):
blob_name = "blob-name"
client = mock.Mock(spec=[u"_http"])
client = mock.Mock(_connection=_Connection, spec=[u"_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
bucket = _Bucket(client)
blob = self._make_one(blob_name, bucket=bucket)
blob._do_download = mock.Mock()
Expand Down Expand Up @@ -1319,7 +1320,8 @@ def _do_multipart_success(
transport = self._mock_transport(http_client.OK, {})

# Create some mock arguments.
client = mock.Mock(_http=transport, spec=["_http"])
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"
Expand Down Expand Up @@ -1485,7 +1487,8 @@ def _initiate_resumable_helper(
transport = self._mock_transport(http_client.OK, response_headers)

# Create some mock arguments and call the method under test.
client = mock.Mock(_http=transport, spec=[u"_http"])
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"
Expand Down Expand Up @@ -1772,7 +1775,8 @@ def _do_resumable_helper(
)

# Create some mock arguments and call the method under test.
client = mock.Mock(_http=transport, spec=["_http"])
client = mock.Mock(_http=transport, _connection=_Connection, spec=["_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
stream = io.BytesIO(data)
content_type = u"text/html"
response = blob._do_resumable_upload(
Expand Down Expand Up @@ -2114,7 +2118,8 @@ def _create_resumable_upload_session_helper(self, origin=None, side_effect=None)
# Create some mock arguments and call the method under test.
content_type = u"text/plain"
size = 10000
client = mock.Mock(_http=transport, spec=[u"_http"])
client = mock.Mock(_http=transport, _connection=_Connection, spec=[u"_http"])
client._connection.API_BASE_URL = "https://storage.googleapis.com"
new_url = blob.create_resumable_upload_session(
content_type=content_type, size=size, origin=origin, client=client
)
Expand Down
8 changes: 3 additions & 5 deletions tests/unit/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,18 +137,16 @@ def test_ctor_w_empty_client_options(self):
)

def test_ctor_w_client_options_dict(self):

PROJECT = "PROJECT"
credentials = _make_credentials()
client_options = {"api_endpoint": "https://www.foo-googleapis.com"}
api_endpoint = "https://www.foo-googleapis.com"
client_options = {"api_endpoint": api_endpoint}

client = self._make_one(
project=PROJECT, credentials=credentials, client_options=client_options
)

self.assertEqual(
client._connection.API_BASE_URL, "https://www.foo-googleapis.com"
)
self.assertEqual(client._connection.API_BASE_URL, api_endpoint)

def test_ctor_w_client_options_object(self):
from google.api_core.client_options import ClientOptions
Expand Down