From 0908ee566e6bc5f2ca31d367d104951ffde30674 Mon Sep 17 00:00:00 2001 From: Stanislav B Date: Sun, 8 Sep 2019 19:49:18 +0200 Subject: [PATCH] Google: Only verify bucket if auto_create is True (#718) Motivation for this change is to send fewer requests to Google Storage API. We do not check existence of bucket unless auto_create_bucket is configured or exists('') is called. When creating bucket, instead of making two requests to get and create, we just create and look for conflict. Based on the work in #575 and supersedes #412. --- storages/backends/gcloud.py | 33 ++++++++++++++++----------------- tests/test_gcloud.py | 28 +++++++++++++++++++--------- 2 files changed, 35 insertions(+), 26 deletions(-) diff --git a/storages/backends/gcloud.py b/storages/backends/gcloud.py index 86c5b2571..2aa0e0003 100644 --- a/storages/backends/gcloud.py +++ b/storages/backends/gcloud.py @@ -15,9 +15,8 @@ ) try: - from google.cloud.storage.client import Client - from google.cloud.storage.blob import Blob - from google.cloud.exceptions import NotFound + from google.cloud.storage import Blob, Client + from google.cloud.exceptions import Conflict, NotFound except ImportError: raise ImproperlyConfigured("Could not load Google Cloud Storage bindings.\n" "See https://github.com/GoogleCloudPlatform/gcloud-python") @@ -129,19 +128,19 @@ def bucket(self): def _get_or_create_bucket(self, name): """ - Retrieves a bucket if it exists, otherwise creates it. + Returns bucket. If auto_create_bucket is True, creates bucket if it + doesn't exist. """ - try: - return self.client.get_bucket(name) - except NotFound: - if self.auto_create_bucket: - bucket = self.client.create_bucket(name) - bucket.acl.save_predefined(self.auto_create_acl) - return bucket - raise ImproperlyConfigured("Bucket %s does not exist. Buckets " - "can be automatically created by " - "setting GS_AUTO_CREATE_BUCKET to " - "``True``." % name) + bucket = self.client.bucket(name) + if self.auto_create_bucket: + try: + new_bucket = self.client.create_bucket(name) + new_bucket.acl.save_predefined(self.auto_create_acl) + return new_bucket + except Conflict: + # Bucket already exists + pass + return bucket def _normalize_name(self, name): """ @@ -191,9 +190,9 @@ def delete(self, name): def exists(self, name): if not name: # root element aka the bucket try: - self.bucket + self.client.get_bucket(self.bucket) return True - except ImproperlyConfigured: + except NotFound: return False name = self._normalize_name(clean_name(name)) diff --git a/tests/test_gcloud.py b/tests/test_gcloud.py index 773250aa7..746222ca1 100644 --- a/tests/test_gcloud.py +++ b/tests/test_gcloud.py @@ -12,7 +12,7 @@ from django.core.files.base import ContentFile from django.test import TestCase from django.utils import timezone -from google.cloud.exceptions import NotFound +from google.cloud.exceptions import Conflict, NotFound from google.cloud.storage.blob import Blob from storages.backends import gcloud @@ -41,7 +41,7 @@ def test_open_read(self): data = b'This is some test read data.' f = self.storage.open(self.filename) - self.storage._client.get_bucket.assert_called_with(self.bucket_name) + self.storage._client.bucket.assert_called_with(self.bucket_name) self.storage._bucket.get_blob.assert_called_with(self.filename) f.blob.download_to_file = lambda tmpfile: tmpfile.write(data) @@ -52,7 +52,7 @@ def test_open_read_num_bytes(self): num_bytes = 10 f = self.storage.open(self.filename) - self.storage._client.get_bucket.assert_called_with(self.bucket_name) + self.storage._client.bucket.assert_called_with(self.bucket_name) self.storage._bucket.get_blob.assert_called_with(self.filename) f.blob.download_to_file = lambda tmpfile: tmpfile.write(data) @@ -102,7 +102,7 @@ def test_save(self): self.storage.save(self.filename, content) - self.storage._client.get_bucket.assert_called_with(self.bucket_name) + self.storage._client.bucket.assert_called_with(self.bucket_name) self.storage._bucket.get_blob().upload_from_file.assert_called_with( content, rewind=True, size=len(data), content_type=mimetypes.guess_type(self.filename)[0]) @@ -113,7 +113,7 @@ def test_save2(self): self.storage.save(filename, content) - self.storage._client.get_bucket.assert_called_with(self.bucket_name) + self.storage._client.bucket.assert_called_with(self.bucket_name) self.storage._bucket.get_blob().upload_from_file.assert_called_with( content, rewind=True, size=len(data), content_type=mimetypes.guess_type(filename)[0]) @@ -129,7 +129,7 @@ def test_save_with_default_acl(self): self.storage.save(filename, content) - self.storage._client.get_bucket.assert_called_with(self.bucket_name) + self.storage._client.bucket.assert_called_with(self.bucket_name) self.storage._bucket.get_blob().upload_from_file.assert_called_with( content, rewind=True, size=len(data), content_type=mimetypes.guess_type(filename)[0], predefined_acl='publicRead') @@ -137,7 +137,7 @@ def test_save_with_default_acl(self): def test_delete(self): self.storage.delete(self.filename) - self.storage._client.get_bucket.assert_called_with(self.bucket_name) + self.storage._client.bucket.assert_called_with(self.bucket_name) self.storage._bucket.delete_blob.assert_called_with(self.filename) def test_exists(self): @@ -160,12 +160,22 @@ def test_exists_bucket(self): # exists('') should return True if the bucket exists self.assertTrue(self.storage.exists('')) + def test_exists_no_bucket_auto_create(self): + # exists('') should return true when auto_create_bucket is configured + # and bucket already exists + # exists('') should automatically create the bucket if + # auto_create_bucket is configured + self.storage.auto_create_bucket = True + self.storage._client = mock.MagicMock() + self.storage._client.create_bucket.side_effect = Conflict('dang') + + self.assertTrue(self.storage.exists('')) + def test_exists_bucket_auto_create(self): # exists('') should automatically create the bucket if # auto_create_bucket is configured self.storage.auto_create_bucket = True self.storage._client = mock.MagicMock() - self.storage._client.get_bucket.side_effect = NotFound('dang') self.assertTrue(self.storage.exists('')) self.storage._client.create_bucket.assert_called_with(self.bucket_name) @@ -383,7 +393,7 @@ def test_cache_control(self): self.storage.cache_control = cache_control self.storage.save(filename, content) - bucket = self.storage.client.get_bucket(self.bucket_name) + bucket = self.storage.client.bucket(self.bucket_name) blob = bucket.get_blob(filename) self.assertEqual(blob.cache_control, cache_control)