From 031ea1cf3820c6d9aa01f9cdfec45a6bd2295a59 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 13 Nov 2014 11:37:44 -0500 Subject: [PATCH 1/2] Drop 'get_acl()' in favor of just using 'acl' property. --- gcloud/storage/_helpers.py | 9 --------- gcloud/storage/acl.py | 6 +++--- gcloud/storage/bucket.py | 6 +++--- gcloud/storage/key.py | 4 ++-- gcloud/storage/test__helpers.py | 19 ------------------- gcloud/storage/test_bucket.py | 3 ++- 6 files changed, 10 insertions(+), 37 deletions(-) diff --git a/gcloud/storage/_helpers.py b/gcloud/storage/_helpers.py index 0224506889bb..303c90e15a40 100644 --- a/gcloud/storage/_helpers.py +++ b/gcloud/storage/_helpers.py @@ -138,15 +138,6 @@ def _get_property(self, field, default=None): return self.properties.get(field, default) - def get_acl(self): - """Get ACL as an object. - - :returns: An ACL object for the current object. - """ - if not self.acl.loaded: - self.acl.reload() - return self.acl - class _PropertyBatch(object): """Context manager: Batch updates to object's ``_patch_properties`` diff --git a/gcloud/storage/acl.py b/gcloud/storage/acl.py index 1bb62b0e6a40..b5d419ac0480 100644 --- a/gcloud/storage/acl.py +++ b/gcloud/storage/acl.py @@ -2,12 +2,12 @@ :class:`gcloud.storage.bucket.Bucket` has a getting method that creates an ACL object under the hood, and you can interact with that using -:func:`gcloud.storage.bucket.Bucket.get_acl`:: +:func:`gcloud.storage.bucket.Bucket.acl`:: >>> from gcloud import storage >>> connection = storage.get_connection(project, email, key_path) >>> bucket = connection.get_bucket(bucket_name) - >>> acl = bucket.get_acl() + >>> acl = bucket.acl Adding and removing permissions can be done with the following methods (in increasing order of granularity): @@ -407,7 +407,7 @@ def save(self, acl=None): >>> bucket1 = connection.get_bucket(bucket1_name) >>> bucket2 = connection.get_bucket(bucket2_name) - >>> bucket2.acl.save(bucket1.get_acl()) + >>> bucket2.acl.save(bucket1.acl) :type acl: :class:`gcloud.storage.acl.ACL`, or a compatible list. :param acl: The ACL object to save. If left blank, this will save diff --git a/gcloud/storage/bucket.py b/gcloud/storage/bucket.py index c1d848bd7f97..38f135e4504a 100644 --- a/gcloud/storage/bucket.py +++ b/gcloud/storage/bucket.py @@ -50,7 +50,7 @@ class Bucket(_PropertyMixin): _iterator_class = _KeyIterator CUSTOM_PROPERTY_ACCESSORS = { - 'acl': 'get_acl()', + 'acl': 'acl', 'cors': 'get_cors()', 'defaultObjectAcl': 'get_default_object_acl()', 'etag': 'etag', @@ -694,7 +694,7 @@ def make_public(self, recursive=False, future=False): :param future: If True, this will make all objects created in the future public as well. """ - self.get_acl().all().grant_read() + self.acl.all().grant_read() self.acl.save() if future: @@ -704,5 +704,5 @@ def make_public(self, recursive=False, future=False): if recursive: for key in self: - key.get_acl().all().grant_read() + key.acl.all().grant_read() key.save_acl() diff --git a/gcloud/storage/key.py b/gcloud/storage/key.py index 523810fe36bd..9e2571e67c97 100644 --- a/gcloud/storage/key.py +++ b/gcloud/storage/key.py @@ -16,7 +16,7 @@ class Key(_PropertyMixin): """A wrapper around Cloud Storage's concept of an ``Object``.""" CUSTOM_PROPERTY_ACCESSORS = { - 'acl': 'get_acl()', + 'acl': 'acl', 'cacheControl': 'cache_control', 'contentDisposition': 'content_disposition', 'contentEncoding': 'content_encoding', @@ -383,7 +383,7 @@ def make_public(self): :returns: The current object. """ - self.get_acl().all().grant_read() + self.acl.all().grant_read() self.acl.save() return self diff --git a/gcloud/storage/test__helpers.py b/gcloud/storage/test__helpers.py index 25f339ef0c9a..7714f118ed71 100644 --- a/gcloud/storage/test__helpers.py +++ b/gcloud/storage/test__helpers.py @@ -117,25 +117,6 @@ def test__patch_properties(self): self.assertEqual(kw[0]['data'], {'foo': 'Foo'}) self.assertEqual(kw[0]['query_params'], {'projection': 'full'}) - def test_get_acl_not_yet_loaded(self): - class ACL(object): - loaded = False - - def reload(self): - self.loaded = True - - mixin = self._makeOne() - acl = mixin.acl = ACL() - self.assertTrue(mixin.get_acl() is acl) - self.assertTrue(acl.loaded) - - def test_get_acl_already_loaded(self): - class ACL(object): - loaded = True - mixin = self._makeOne() - acl = mixin.acl = ACL() - self.assertTrue(mixin.get_acl() is acl) # no 'reload' - class TestPropertyBatch(unittest2.TestCase): diff --git a/gcloud/storage/test_bucket.py b/gcloud/storage/test_bucket.py index 83a88a35d397..46415b74501b 100644 --- a/gcloud/storage/test_bucket.py +++ b/gcloud/storage/test_bucket.py @@ -920,7 +920,8 @@ def __init__(self, bucket, name): self._bucket = bucket self._name = name - def get_acl(self): + @property + def acl(self): return self def all(self): From 7e4acb0f5d62e74008c7a50246019f03eba558e2 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 13 Nov 2014 12:17:47 -0500 Subject: [PATCH 2/2] Make 'ACL' operations trigger a reload, if needed. Fixes #338. --- gcloud/storage/acl.py | 20 ++++-- gcloud/storage/test_acl.py | 126 +++++++++++++++++++++++++++++++--- gcloud/storage/test_bucket.py | 2 + 3 files changed, 130 insertions(+), 18 deletions(-) diff --git a/gcloud/storage/acl.py b/gcloud/storage/acl.py index b5d419ac0480..54174bed5ca1 100644 --- a/gcloud/storage/acl.py +++ b/gcloud/storage/acl.py @@ -172,12 +172,19 @@ class ACL(object): def __init__(self): self.entities = {} + def _ensure_loaded(self): + """Load if not already loaded.""" + if not self.loaded: + self.reload() + def reset(self): """Remove all entities from the ACL, and clear the ``loaded`` flag.""" self.entities.clear() self.loaded = False def __iter__(self): + self._ensure_loaded() + for entity in self.entities.itervalues(): for role in entity.get_roles(): if role: @@ -224,6 +231,7 @@ def has_entity(self, entity): :rtype: bool :returns: True of the entity exists in the ACL. """ + self._ensure_loaded() return str(entity) in self.entities def get_entity(self, entity, default=None): @@ -240,6 +248,7 @@ def get_entity(self, entity, default=None): :returns: The corresponding entity or the value provided to ``default``. """ + self._ensure_loaded() return self.entities.get(str(entity), default) def add_entity(self, entity): @@ -248,8 +257,8 @@ def add_entity(self, entity): :type entity: :class:`_ACLEntity` :param entity: The entity to add to this ACL. """ + self._ensure_loaded() self.entities[str(entity)] = entity - self.loaded = True def entity(self, entity_type, identifier=None): """Factory method for creating an Entity. @@ -332,6 +341,7 @@ def get_entities(self): :rtype: list of :class:`_ACLEntity` objects :returns: A list of all Entity objects. """ + self._ensure_loaded() return self.entities.values() def reload(self): @@ -381,12 +391,10 @@ def reload(self): url_path = '%s/%s' % (self.bucket.path, self._URL_PATH_ELEM) found = self.bucket.connection.api_request(method='GET', path=url_path) + self.loaded = True for entry in found['items']: self.add_entity(self.entity_from_dict(entry)) - # Even if we fetch no entries, the ACL is still loaded. - self.loaded = True - return self def save(self, acl=None): @@ -489,12 +497,10 @@ def reload(self): url_path = '%s/acl' % self.key.path found = self.key.connection.api_request(method='GET', path=url_path) + self.loaded = True for entry in found['items']: self.add_entity(self.entity_from_dict(entry)) - # Even if we fetch no entries, the ACL is still loaded. - self.loaded = True - return self def save(self, acl=None): diff --git a/gcloud/storage/test_acl.py b/gcloud/storage/test_acl.py index 7c771b86d8c2..5cf9d092db70 100644 --- a/gcloud/storage/test_acl.py +++ b/gcloud/storage/test_acl.py @@ -124,27 +124,48 @@ def _makeOne(self, *args, **kw): def test_ctor(self): acl = self._makeOne() self.assertEqual(acl.entities, {}) - self.assertEqual(list(acl.get_entities()), []) self.assertFalse(acl.loaded) + def test__ensure_loaded(self): + acl = self._makeOne() + + def _reload(): + acl._really_loaded = True + + acl.reload = _reload + acl._ensure_loaded() + self.assertTrue(acl._really_loaded) + def test_reset(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True acl.entity(TYPE, ID) acl.reset() - self.assertFalse(acl.loaded) self.assertEqual(acl.entities, {}) - self.assertEqual(list(acl.get_entities()), []) + self.assertFalse(acl.loaded) + + def test___iter___empty_eager(self): + acl = self._makeOne() + acl.loaded = True + self.assertEqual(list(acl), []) - def test___iter___empty(self): + def test___iter___empty_lazy(self): acl = self._makeOne() + + def _reload(): + acl.loaded = True + + acl.reload = _reload self.assertEqual(list(acl), []) + self.assertTrue(acl.loaded) def test___iter___non_empty_no_roles(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True acl.entity(TYPE, ID) self.assertEqual(list(acl), []) @@ -153,6 +174,7 @@ def test___iter___non_empty_w_roles(self): ID = 'id' ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.entity(TYPE, ID) entity.grant(ROLE) self.assertEqual(list(acl), @@ -162,13 +184,15 @@ def test___iter___non_empty_w_empty_role(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True entity = acl.entity(TYPE, ID) entity.grant('') self.assertEqual(list(acl), []) - def test_entity_from_dict_allUsers(self): + def test_entity_from_dict_allUsers_eager(self): ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.entity_from_dict({'entity': 'allUsers', 'role': ROLE}) self.assertEqual(entity.type, 'allUsers') self.assertEqual(entity.identifier, None) @@ -180,6 +204,7 @@ def test_entity_from_dict_allUsers(self): def test_entity_from_dict_allAuthenticatedUsers(self): ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.entity_from_dict({'entity': 'allAuthenticatedUsers', 'role': ROLE}) self.assertEqual(entity.type, 'allAuthenticatedUsers') @@ -192,6 +217,7 @@ def test_entity_from_dict_allAuthenticatedUsers(self): def test_entity_from_dict_string_w_hyphen(self): ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.entity_from_dict({'entity': 'type-id', 'role': ROLE}) self.assertEqual(entity.type, 'type') self.assertEqual(entity.identifier, 'id') @@ -203,27 +229,41 @@ def test_entity_from_dict_string_w_hyphen(self): def test_entity_from_dict_string_wo_hyphen(self): ROLE = 'role' acl = self._makeOne() + acl.loaded = True self.assertRaises(ValueError, acl.entity_from_dict, {'entity': 'bogus', 'role': ROLE}) self.assertEqual(list(acl.get_entities()), []) - def test_has_entity_miss_str(self): + def test_has_entity_miss_str_eager(self): acl = self._makeOne() + acl.loaded = True self.assertFalse(acl.has_entity('nonesuch')) + def test_has_entity_miss_str_lazy(self): + acl = self._makeOne() + + def _reload(): + acl.loaded = True + + acl.reload = _reload + self.assertFalse(acl.has_entity('nonesuch')) + self.assertTrue(acl.loaded) + def test_has_entity_miss_entity(self): from gcloud.storage.acl import _ACLEntity TYPE = 'type' ID = 'id' entity = _ACLEntity(TYPE, ID) acl = self._makeOne() + acl.loaded = True self.assertFalse(acl.has_entity(entity)) def test_has_entity_hit_str(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True acl.entity(TYPE, ID) self.assertTrue(acl.has_entity('%s-%s' % (TYPE, ID))) @@ -231,24 +271,38 @@ def test_has_entity_hit_entity(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True entity = acl.entity(TYPE, ID) self.assertTrue(acl.has_entity(entity)) - def test_get_entity_miss_str_no_default(self): + def test_get_entity_miss_str_no_default_eager(self): acl = self._makeOne() + acl.loaded = True self.assertEqual(acl.get_entity('nonesuch'), None) + def test_get_entity_miss_str_no_default_lazy(self): + acl = self._makeOne() + + def _reload(): + acl.loaded = True + + acl.reload = _reload + self.assertEqual(acl.get_entity('nonesuch'), None) + self.assertTrue(acl.loaded) + def test_get_entity_miss_entity_no_default(self): from gcloud.storage.acl import _ACLEntity TYPE = 'type' ID = 'id' entity = _ACLEntity(TYPE, ID) acl = self._makeOne() + acl.loaded = True self.assertEqual(acl.get_entity(entity), None) def test_get_entity_miss_str_w_default(self): DEFAULT = object() acl = self._makeOne() + acl.loaded = True self.assertTrue(acl.get_entity('nonesuch', DEFAULT) is DEFAULT) def test_get_entity_miss_entity_w_default(self): @@ -258,12 +312,14 @@ def test_get_entity_miss_entity_w_default(self): ID = 'id' entity = _ACLEntity(TYPE, ID) acl = self._makeOne() + acl.loaded = True self.assertTrue(acl.get_entity(entity, DEFAULT) is DEFAULT) def test_get_entity_hit_str(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True acl.entity(TYPE, ID) self.assertTrue(acl.has_entity('%s-%s' % (TYPE, ID))) @@ -271,10 +327,26 @@ def test_get_entity_hit_entity(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True entity = acl.entity(TYPE, ID) self.assertTrue(acl.has_entity(entity)) - def test_add_entity_miss(self): + def test_add_entity_miss_eager(self): + from gcloud.storage.acl import _ACLEntity + TYPE = 'type' + ID = 'id' + ROLE = 'role' + entity = _ACLEntity(TYPE, ID) + entity.grant(ROLE) + acl = self._makeOne() + acl.loaded = True + acl.add_entity(entity) + self.assertTrue(acl.loaded) + self.assertEqual(list(acl), + [{'entity': 'type-id', 'role': ROLE}]) + self.assertEqual(list(acl.get_entities()), [entity]) + + def test_add_entity_miss_lazy(self): from gcloud.storage.acl import _ACLEntity TYPE = 'type' ID = 'id' @@ -282,11 +354,17 @@ def test_add_entity_miss(self): entity = _ACLEntity(TYPE, ID) entity.grant(ROLE) acl = self._makeOne() + + def _reload(): + acl.loaded = True + + acl.reload = _reload acl.add_entity(entity) self.assertTrue(acl.loaded) self.assertEqual(list(acl), [{'entity': 'type-id', 'role': ROLE}]) self.assertEqual(list(acl.get_entities()), [entity]) + self.assertTrue(acl.loaded) def test_add_entity_hit(self): from gcloud.storage.acl import _ACLEntity @@ -297,6 +375,7 @@ def test_add_entity_hit(self): entity = _ACLEntity(TYPE, ID) entity.grant(ROLE) acl = self._makeOne() + acl.loaded = True before = acl.entity(TYPE, ID) acl.add_entity(entity) self.assertTrue(acl.loaded) @@ -311,6 +390,7 @@ def test_entity_miss(self): ID = 'id' ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.entity(TYPE, ID) self.assertTrue(acl.loaded) entity.grant(ROLE) @@ -323,6 +403,7 @@ def test_entity_hit(self): ID = 'id' ROLE = 'role' acl = self._makeOne() + acl.loaded = True before = acl.entity(TYPE, ID) before.grant(ROLE) entity = acl.entity(TYPE, ID) @@ -335,6 +416,7 @@ def test_user(self): ID = 'id' ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.user(ID) entity.grant(ROLE) self.assertEqual(entity.type, 'user') @@ -346,6 +428,7 @@ def test_group(self): ID = 'id' ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.group(ID) entity.grant(ROLE) self.assertEqual(entity.type, 'group') @@ -357,6 +440,7 @@ def test_domain(self): ID = 'id' ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.domain(ID) entity.grant(ROLE) self.assertEqual(entity.type, 'domain') @@ -367,6 +451,7 @@ def test_domain(self): def test_all(self): ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.all() entity.grant(ROLE) self.assertEqual(entity.type, 'allUsers') @@ -377,6 +462,7 @@ def test_all(self): def test_all_authenticated(self): ROLE = 'role' acl = self._makeOne() + acl.loaded = True entity = acl.all_authenticated() entity.grant(ROLE) self.assertEqual(entity.type, 'allAuthenticatedUsers') @@ -384,14 +470,26 @@ def test_all_authenticated(self): self.assertEqual(list(acl), [{'entity': 'allAuthenticatedUsers', 'role': ROLE}]) - def test_get_entities_empty(self): + def test_get_entities_empty_eager(self): acl = self._makeOne() + acl.loaded = True self.assertEqual(acl.get_entities(), []) + def test_get_entities_empty_lazy(self): + acl = self._makeOne() + + def _reload(): + acl.loaded = True + + acl.reload = _reload + self.assertEqual(acl.get_entities(), []) + self.assertTrue(acl.loaded) + def test_get_entities_nonempty(self): TYPE = 'type' ID = 'id' acl = self._makeOne() + acl.loaded = True entity = acl.entity(TYPE, ID) self.assertEqual(acl.get_entities(), [entity]) @@ -421,7 +519,7 @@ def test_ctor(self): bucket = object() acl = self._makeOne(bucket) self.assertEqual(acl.entities, {}) - self.assertEqual(list(acl.get_entities()), []) + self.assertFalse(acl.loaded) self.assertTrue(acl.bucket is bucket) def test_reload_eager_empty(self): @@ -430,6 +528,7 @@ def test_reload_eager_empty(self): connection = _Connection({'items': []}) bucket = _Bucket(connection, NAME) acl = self._makeOne(bucket) + acl.loaded = True acl.entity('allUsers', ROLE) self.assertTrue(acl.reload() is acl) self.assertEqual(list(acl), []) @@ -483,6 +582,7 @@ def test_save_no_arg(self): connection = _Connection({'acl': AFTER}) bucket = _Bucket(connection, NAME) acl = self._makeOne(bucket) + acl.loaded = True acl.entity('allUsers').grant(ROLE) self.assertTrue(acl.save() is acl) self.assertEqual(list(acl), AFTER) @@ -523,6 +623,7 @@ def test_clear(self): connection = _Connection({'acl': [STICKY]}) bucket = _Bucket(connection, NAME) acl = self._makeOne(bucket) + acl.loaded = True acl.entity('allUsers', ROLE1) self.assertTrue(acl.clear() is acl) self.assertEqual(list(acl), [STICKY]) @@ -547,7 +648,7 @@ def test_ctor(self): key = object() acl = self._makeOne(key) self.assertEqual(acl.entities, {}) - self.assertEqual(list(acl.get_entities()), []) + self.assertFalse(acl.loaded) self.assertTrue(acl.key is key) def test_reload_eager_empty(self): @@ -576,6 +677,7 @@ def test_reload_eager_nonempty(self): bucket = _Bucket(connection, NAME) key = _Key(bucket, KEY) acl = self._makeOne(key) + acl.loaded = True acl.entity('allUsers', ROLE) self.assertTrue(acl.reload() is acl) self.assertEqual(list(acl), []) @@ -633,6 +735,7 @@ def test_save_existing_set_new_passed(self): bucket = _Bucket(connection, NAME) key = _Key(bucket, KEY) acl = self._makeOne(key) + acl.loaded = True acl.entity('allUsers', 'other-role') self.assertTrue(acl.save(new_acl) is acl) self.assertEqual(list(acl), new_acl) @@ -651,6 +754,7 @@ def test_clear(self): bucket = _Bucket(connection, NAME) key = _Key(bucket, KEY) acl = self._makeOne(key) + acl.loaded = True acl.entity('allUsers', ROLE) self.assertTrue(acl.clear() is acl) self.assertEqual(list(acl), []) diff --git a/gcloud/storage/test_bucket.py b/gcloud/storage/test_bucket.py index 46415b74501b..54321a186243 100644 --- a/gcloud/storage/test_bucket.py +++ b/gcloud/storage/test_bucket.py @@ -874,6 +874,7 @@ def test_make_public_defaults(self): connection = _Connection(after) bucket = self._makeOne(connection, NAME) bucket.acl.loaded = True + bucket.default_object_acl.loaded = True bucket.make_public() self.assertEqual(list(bucket.acl), permissive) self.assertEqual(list(bucket.default_object_acl), []) @@ -945,6 +946,7 @@ def get_items_from_response(self, response): connection = _Connection(after, {'items': [{'name': KEY}]}) bucket = self._makeOne(connection, NAME) bucket.acl.loaded = True + bucket.default_object_acl.loaded = True bucket._iterator_class = _Iterator bucket.make_public(recursive=True) self.assertEqual(list(bucket.acl), permissive)