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

feat: allow no project in client methods using storage emulator #703

Merged
merged 4 commits into from Feb 4, 2022
Merged

feat: allow no project in client methods using storage emulator #703

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04aefb4
feat: allow no project in client methods using storage emulator and r…
cojenco Jan 25, 2022
463a17f
Merge branch 'main' into emu699
cojenco Jan 27, 2022
2d7508e
changes from comments
cojenco Jan 27, 2022
728a02b
Merge branch 'main' into emu699
cojenco Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 24 additions & 7 deletions google/cloud/storage/client.py
View file
Open in desktop
Expand Up @@ -932,12 +932,22 @@ def create_bucket(

"""
bucket = self._bucket_arg_to_bucket(bucket_or_name)
query_params = {}

if project is None:
project = self.project

if project is None:
raise ValueError("Client project not set: pass an explicit project.")
# Use no project if STORAGE_EMULATOR_HOST is set
if _get_storage_host().find("storage.googleapis.com") < 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Three points to make here:

I think if "storage.googleapis.com" in _get_storage_host() is a better idiom than using find().

We should avoid hardcoding strings like "storage.googleapis.com" when possible. Perhaps we should have a constant, and/or a helper function that answers the question consistently and globally of whether we are using the emulator?

Is there no way to avoid needing the client to be aware of the emulator at all? Perhaps it's the emulator that needs to change here?

Copy link
Contributor Author

@cojenco cojenco Jan 27, 2022 •
edited

Choose a reason for hiding this comment

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

Thanks for the review! Updated according to your comments.

The user ask is to be able to run the same code against a storage emulator as well as the live server (without writing extra conditions or wrappers), often seen in testing.

client.create_bucket() and client.list_buckets() are the only 2 library methods that explicitly expect a project. I think it makes sense to allow these 2 methods to detect the emulator and use a fake project marker.

if project is None:
project = _get_environ_project()
if project is None:
project = "<none>"

# Only include the project parameter if a project is set.
# If a project is not set, falls back to API validation (BadRequest). Removes client-side validation.
cojenco marked this conversation as resolved.
Show resolved Hide resolved
if project is not None:
query_params = {"project": project}
unforced marked this conversation as resolved.
Show resolved Hide resolved

if requester_pays is not None:
warnings.warn(
Expand All @@ -947,8 +957,6 @@ def create_bucket(
)
bucket.requester_pays = requester_pays

query_params = {"project": project}

if predefined_acl is not None:
predefined_acl = BucketACL.validate_predefined(predefined_acl)
query_params["predefinedAcl"] = predefined_acl
Expand Down Expand Up @@ -1375,13 +1383,22 @@ def list_buckets(
:returns: Iterator of all :class:`~google.cloud.storage.bucket.Bucket`
belonging to this project.
"""
extra_params = {}

if project is None:
project = self.project

if project is None:
raise ValueError("Client project not set: pass an explicit project.")
# Use no project if STORAGE_EMULATOR_HOST is set
if _get_storage_host().find("storage.googleapis.com") < 0:
if project is None:
project = _get_environ_project()
if project is None:
project = "<none>"

extra_params = {"project": project}
# Only include the project parameter if a project is set.
# If a project is not set, falls back to API validation (BadRequest). Removes client-side validation.
if project is not None:
unforced marked this conversation as resolved.
Show resolved Hide resolved
extra_params = {"project": project}

if prefix is not None:
extra_params["prefix"] = prefix
Expand Down
202 changes: 186 additions & 16 deletions tests/unit/test_client.py
View file
Open in desktop
Expand Up @@ -23,15 +23,15 @@
import unittest
import urllib


from google.api_core import exceptions

from google.auth.credentials import AnonymousCredentials
from google.oauth2.service_account import Credentials
from . import _read_local_json

from google.cloud.storage._helpers import STORAGE_EMULATOR_ENV_VAR
from google.cloud.storage.retry import DEFAULT_RETRY
from google.cloud.storage.retry import DEFAULT_RETRY_IF_GENERATION_SPECIFIED

from . import _read_local_json

_SERVICE_ACCOUNT_JSON = _read_local_json("url_signer_v4_test_account.json")
_CONFORMANCE_TESTS = _read_local_json("url_signer_v4_test_data.json")[
Expand Down Expand Up @@ -237,9 +237,6 @@ def test_ctor_mtls(self):
self.assertEqual(client._connection.API_BASE_URL, "http://foo")

def test_ctor_w_emulator_wo_project(self):
from google.auth.credentials import AnonymousCredentials
from google.cloud.storage._helpers import STORAGE_EMULATOR_ENV_VAR

# avoids authentication if STORAGE_EMULATOR_ENV_VAR is set
host = "http://localhost:8080"
environ = {STORAGE_EMULATOR_ENV_VAR: host}
Expand All @@ -259,9 +256,6 @@ def test_ctor_w_emulator_wo_project(self):
self.assertIsInstance(client._connection.credentials, AnonymousCredentials)

def test_ctor_w_emulator_w_environ_project(self):
from google.auth.credentials import AnonymousCredentials
from google.cloud.storage._helpers import STORAGE_EMULATOR_ENV_VAR

# avoids authentication and infers the project from the environment
host = "http://localhost:8080"
environ_project = "environ-project"
Expand All @@ -277,9 +271,6 @@ def test_ctor_w_emulator_w_environ_project(self):
self.assertIsInstance(client._connection.credentials, AnonymousCredentials)

def test_ctor_w_emulator_w_project_arg(self):
from google.auth.credentials import AnonymousCredentials
from google.cloud.storage._helpers import STORAGE_EMULATOR_ENV_VAR

# project argument overrides project set in the enviroment
host = "http://localhost:8080"
environ_project = "environ-project"
Expand All @@ -296,7 +287,6 @@ def test_ctor_w_emulator_w_project_arg(self):
self.assertIsInstance(client._connection.credentials, AnonymousCredentials)

def test_create_anonymous_client(self):
from google.auth.credentials import AnonymousCredentials
from google.cloud.storage._http import Connection

klass = self._get_target_class()
Expand Down Expand Up @@ -1187,11 +1177,91 @@ def test_lookup_bucket_hit_w_retry(self):
)

def test_create_bucket_w_missing_client_project(self):
from google.cloud.exceptions import BadRequest

credentials = _make_credentials()
client = self._make_one(project=None, credentials=credentials)

with self.assertRaises(ValueError):
client.create_bucket("bucket")
client._post_resource = mock.Mock()
client._post_resource.side_effect = BadRequest("Required parameter: project")

bucket_name = "bucket-name"

with self.assertRaises(BadRequest):
client.create_bucket(bucket_name)

expected_path = "/b"
expected_data = {"name": bucket_name}
# no required parameter: project
expected_query_params = {}
client._post_resource.assert_called_once_with(
expected_path,
expected_data,
query_params=expected_query_params,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
_target_object=mock.ANY,
)

def test_create_bucket_w_missing_client_project_w_emulator(self):
# mock STORAGE_EMULATOR_ENV_VAR is set
host = "http://localhost:8080"
environ = {STORAGE_EMULATOR_ENV_VAR: host}
with mock.patch("os.environ", environ):
client = self._make_one()

bucket_name = "bucket-name"
api_response = {"name": bucket_name}
client._post_resource = mock.Mock()
client._post_resource.return_value = api_response

# mock STORAGE_EMULATOR_ENV_VAR is set
with mock.patch("os.environ", environ):
bucket = client.create_bucket(bucket_name)

expected_path = "/b"
expected_data = api_response
expected_query_params = {"project": "<none>"}
client._post_resource.assert_called_once_with(
expected_path,
expected_data,
query_params=expected_query_params,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
_target_object=bucket,
)

def test_create_bucket_w_environ_project_w_emulator(self):
# mock STORAGE_EMULATOR_ENV_VAR is set
host = "http://localhost:8080"
environ_project = "environ-project"
environ = {
STORAGE_EMULATOR_ENV_VAR: host,
"GOOGLE_CLOUD_PROJECT": environ_project,
}
with mock.patch("os.environ", environ):
client = self._make_one()

bucket_name = "bucket-name"
api_response = {"name": bucket_name}
client._post_resource = mock.Mock()
client._post_resource.return_value = api_response

# mock STORAGE_EMULATOR_ENV_VAR is set
with mock.patch("os.environ", environ):
bucket = client.create_bucket(bucket_name)

expected_path = "/b"
expected_data = api_response
expected_query_params = {"project": environ_project}
client._post_resource.assert_called_once_with(
expected_path,
expected_data,
query_params=expected_query_params,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
_target_object=bucket,
)

def test_create_bucket_w_conflict_w_user_project(self):
from google.cloud.exceptions import Conflict
Expand Down Expand Up @@ -1787,12 +1857,112 @@ def test_list_blobs_w_explicit_w_user_project(self):
)

def test_list_buckets_wo_project(self):
from google.cloud.exceptions import BadRequest
from google.cloud.storage.client import _item_to_bucket

credentials = _make_credentials()
client = self._make_one(project=None, credentials=credentials)

with self.assertRaises(ValueError):
client._list_resource = mock.Mock()
client._list_resource.side_effect = BadRequest("Required parameter: project")

with self.assertRaises(BadRequest):
client.list_buckets()

expected_path = "/b"
expected_item_to_value = _item_to_bucket
expected_page_token = None
expected_max_results = None
expected_page_size = None
# no required parameter: project
expected_extra_params = {
"projection": "noAcl",
}
client._list_resource.assert_called_once_with(
expected_path,
expected_item_to_value,
page_token=expected_page_token,
max_results=expected_max_results,
extra_params=expected_extra_params,
page_size=expected_page_size,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
)

def test_list_buckets_wo_project_w_emulator(self):
from google.cloud.storage.client import _item_to_bucket

# mock STORAGE_EMULATOR_ENV_VAR is set
host = "http://localhost:8080"
environ = {STORAGE_EMULATOR_ENV_VAR: host}
with mock.patch("os.environ", environ):
client = self._make_one()

client._list_resource = mock.Mock(spec=[])

# mock STORAGE_EMULATOR_ENV_VAR is set
with mock.patch("os.environ", environ):
client.list_buckets()

expected_path = "/b"
expected_item_to_value = _item_to_bucket
expected_page_token = None
expected_max_results = None
expected_page_size = None
expected_extra_params = {
"project": "<none>",
"projection": "noAcl",
}
client._list_resource.assert_called_once_with(
expected_path,
expected_item_to_value,
page_token=expected_page_token,
max_results=expected_max_results,
extra_params=expected_extra_params,
page_size=expected_page_size,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
)

def test_list_buckets_w_environ_project_w_emulator(self):
from google.cloud.storage.client import _item_to_bucket

# mock STORAGE_EMULATOR_ENV_VAR is set
host = "http://localhost:8080"
environ_project = "environ-project"
environ = {
STORAGE_EMULATOR_ENV_VAR: host,
"GOOGLE_CLOUD_PROJECT": environ_project,
}
with mock.patch("os.environ", environ):
client = self._make_one()

client._list_resource = mock.Mock(spec=[])

# mock STORAGE_EMULATOR_ENV_VAR is set
with mock.patch("os.environ", environ):
client.list_buckets()

expected_path = "/b"
expected_item_to_value = _item_to_bucket
expected_page_token = None
expected_max_results = None
expected_page_size = None
expected_extra_params = {
"project": environ_project,
"projection": "noAcl",
}
client._list_resource.assert_called_once_with(
expected_path,
expected_item_to_value,
page_token=expected_page_token,
max_results=expected_max_results,
extra_params=expected_extra_params,
page_size=expected_page_size,
timeout=self._get_default_timeout(),
retry=DEFAULT_RETRY,
)

def test_list_buckets_w_defaults(self):
from google.cloud.storage.client import _item_to_bucket

Expand Down