Fix #151/#163: use ACL-specific endpoints where feasible for buckets and keys

gcloud/storage/acl.py

@@ -167,9 +167,20 @@ def revoke_owner(self):
 class ACL(object):
     """Container class representing a list of access controls."""
 
+    loaded = False
+
     def __init__(self):
         self.entities = {}
 
+    def clear(self):
+        """Remove all entities from the ACL."""
+        self.entities.clear()
+
+    def reset(self):
+        """Remove all entities from the ACL, and clear the ``loaded`` flag."""
+        self.entities.clear()
+        self.loaded = False
+
     def __iter__(self):
         for entity in self.entities.itervalues():
             for role in entity.get_roles():
@@ -242,6 +253,7 @@ def add_entity(self, entity):
         :param entity: The entity to add to this ACL.
         """
         self.entities[str(entity)] = entity
+        self.loaded = True
 
     def entity(self, entity_type, identifier=None):
         """Factory method for creating an Entity.

gcloud/storage/bucket.py

@@ -19,15 +19,27 @@ class Bucket(object):
     :type name: string
     :param name: The name of the bucket.
     """
+    # ACL rules are lazily retrieved.
+    _acl = _default_object_acl = None
 
     def __init__(self, connection=None, name=None, metadata=None):
         self.connection = connection
         self.name = name
         self.metadata = metadata
 
-        # ACL rules are lazily retrieved.
-        self.acl = None
-        self.default_object_acl = None
+    @property
+    def acl(self):
+        """Create our ACL on demand."""
+        if self._acl is None:
+            self._acl = BucketACL(self)
+        return self._acl
+
+    @property
+    def default_object_acl(self):
+        """Create our defaultObjectACL on demand."""
+        if self._default_object_acl is None:
+            self._default_object_acl = DefaultObjectACL(self)
+        return self._default_object_acl
 
     @classmethod
     def from_dict(cls, bucket_dict, connection=None):
@@ -313,17 +325,15 @@ def has_metadata(self, field=None):
         else:
             return True
 
-    def reload_metadata(self, full=False):
+    def reload_metadata(self):
         """Reload metadata from Cloud Storage.
 
-        :type full: bool
-        :param full: If True, loads all data (include ACL data).
-
         :rtype: :class:`Bucket`
         :returns: The bucket you just reloaded data for.
         """
-        projection = 'full' if full else 'noAcl'
-        query_params = {'projection': projection}
+        # Pass only '?projection=noAcl' here because 'acl'/'defaultObjectAcl'
+        # are handled via 'get_acl()'/'get_default_object_acl()'
+        query_params = {'projection': 'noAcl'}
         self.metadata = self.connection.api_request(
             method='GET', path=self.path, query_params=query_params)
         return self
@@ -344,9 +354,14 @@ def get_metadata(self, field=None, default=None):
         :rtype: dict or anything
         :returns: All metadata or the value of the specific field.
         """
+        if field == 'acl':
+            raise KeyError("Use 'get_acl()'")
+
+        if field == 'defaultObjectAcl':
+            raise KeyError("Use 'get_default_object_acl()'")
+
         if not self.has_metadata(field=field):
-            full = (field and field in ('acl', 'defaultObjectAcl'))
-            self.reload_metadata(full=full)
+            self.reload_metadata()
 
         if field:
             return self.metadata.get(field, default)
@@ -431,11 +446,15 @@ def reload_acl(self):
         :rtype: :class:`Bucket`
         :returns: The current bucket.
         """
-        self.acl = BucketACL(bucket=self)
+        self.acl.clear()
+
+        url_path = '%s/acl' % self.path
+        found = self.connection.api_request(method='GET', path=url_path)
+        for entry in found['items']:
+            self.acl.add_entity(self.acl.entity_from_dict(entry))
 
-        for entry in self.get_metadata('acl', []):
-            entity = self.acl.entity_from_dict(entry)
-            self.acl.add_entity(entity)
+        # Even if we fetch no entries, the ACL is still loaded.
+        self.acl.loaded = True
 
         return self
 
@@ -445,7 +464,7 @@ def get_acl(self):
         :rtype: :class:`gcloud.storage.acl.BucketACL`
         :returns: An ACL object for the current bucket.
         """
-        if not self.acl:
+        if not self.acl.loaded:
             self.reload_acl()
         return self.acl
 
@@ -487,12 +506,19 @@ def save_acl(self, acl=None):
         # both evaluate to False, but mean very different things.
         if acl is None:
             acl = self.acl
+            dirty = acl.loaded
+        else:
+            dirty = True
 
-        if acl is None:
-            return self
+        if dirty:
+            result = self.connection.api_request(
+                method='PATCH', path=self.path, data={'acl': list(acl)},
+                query_params={'projection': 'full'})
+            self.acl.clear()
+            for entry in result['acl']:
+                self.acl.entity(self.acl.entity_from_dict(entry))
+            self.acl.loaded = True
 
-        self.patch_metadata({'acl': list(acl)})
-        self.reload_acl()
         return self
 
     def clear_acl(self):
@@ -522,19 +548,26 @@ def clear_acl(self):
 
         At this point all the custom rules you created have been removed.
         """
-        return self.save_acl(acl=[])
+        # NOTE:  back-end makes some ACL entries sticky (they remain even
+        #        after the PATCH succeeds.
+        return self.save_acl([])
 
     def reload_default_object_acl(self):
         """Reload the Default Object ACL rules for this bucket.
 
         :rtype: :class:`Bucket`
         :returns: The current bucket.
         """
-        self.default_object_acl = DefaultObjectACL(bucket=self)
+        doa = self.default_object_acl
+        doa.clear()
 
-        for entry in self.get_metadata('defaultObjectAcl', []):
-            entity = self.default_object_acl.entity_from_dict(entry)
-            self.default_object_acl.add_entity(entity)
+        url_path = '%s/defaultObjectAcl' % self.path
+        found = self.connection.api_request(method='GET', path=url_path)
+        for entry in found['items']:
+            doa.add_entity(doa.entity_from_dict(entry))
+
+        # Even if we fetch no entries, the ACL is still loaded.
+        doa.loaded = True
 
         return self
 
@@ -547,7 +580,7 @@ def get_default_object_acl(self):
         :rtype: :class:`gcloud.storage.acl.DefaultObjectACL`
         :returns: A DefaultObjectACL object for this bucket.
         """
-        if not self.default_object_acl:
+        if not self.default_object_acl.loaded:
             self.reload_default_object_acl()
         return self.default_object_acl
 
@@ -562,18 +595,26 @@ def save_default_object_acl(self, acl=None):
         """
         if acl is None:
             acl = self.default_object_acl
+            dirty = acl.loaded
+        else:
+            dirty = True
+
+        if dirty:
+            result = self.connection.api_request(
+                method='PATCH', path=self.path,
+                data={'defaultObjectAcl': list(acl)},
+                query_params={'projection': 'full'})
+            doa = self.default_object_acl
+            doa.clear()
+            for entry in result['defaultObjectAcl']:
+                doa.entity(doa.entity_from_dict(entry))
+            doa.loaded = True
 
-        if acl is None:
-            return self
-
-        self.patch_metadata({'defaultObjectAcl': list(acl)})
-        self.reload_default_object_acl()
         return self
 
     def clear_default_object_acl(self):
         """Remove the Default Object ACL from this bucket."""
-
-        return self.save_default_object_acl(acl=[])
+        return self.save_default_object_acl([])
 
     def make_public(self, recursive=False, future=False):
         """Make a bucket public.

gcloud/storage/key.py

@@ -17,6 +17,8 @@ class Key(object):
 
     This must be a multiple of 256 KB per the API specification.
     """
+    # ACL rules are lazily retrieved.
+    _acl = None
 
     def __init__(self, bucket=None, name=None, metadata=None):
         """Key constructor.
@@ -35,8 +37,12 @@ def __init__(self, bucket=None, name=None, metadata=None):
         self.name = name
         self.metadata = metadata or {}
 
-        # Lazily get the ACL information.
-        self.acl = None
+    @property
+    def acl(self):
+        """Create our ACL on demand."""
+        if self._acl is None:
+            self._acl = ObjectACL(self)
+        return self._acl
 
     @classmethod
     def from_dict(cls, key_dict, bucket=None):
@@ -316,17 +322,15 @@ def has_metadata(self, field=None):
         else:
             return True
 
-    def reload_metadata(self, full=False):
+    def reload_metadata(self):
         """Reload metadata from Cloud Storage.
 
-        :type full: bool
-        :param full: If True, loads all data (include ACL data).
-
         :rtype: :class:`Key`
         :returns: The key you just reloaded data for.
         """
-        projection = 'full' if full else 'noAcl'
-        query_params = {'projection': projection}
+        # Pass only '?projection=noAcl' here because 'acl' is handled via
+        # 'get_acl().
+        query_params = {'projection': 'noAcl'}
         self.metadata = self.connection.api_request(
             method='GET', path=self.path, query_params=query_params)
         return self
@@ -347,9 +351,12 @@ def get_metadata(self, field=None, default=None):
         :rtype: dict or anything
         :returns: All metadata or the value of the specific field.
         """
+        # We ignore 'acl' because it is meant to be handled via 'get_acl()'.
+        if field == 'acl':
+            raise KeyError("Use 'get_acl()'")
+
         if not self.has_metadata(field=field):
-            full = (field and field == 'acl')
-            self.reload_metadata(full=full)
+            self.reload_metadata()
 
         if field:
             return self.metadata.get(field, default)
@@ -382,11 +389,15 @@ def reload_acl(self):
         :rtype: :class:`Key`
         :returns: The current key.
         """
-        self.acl = ObjectACL(key=self)
+        self.acl.clear()
 
-        for entry in self.get_metadata('acl', []):
-            entity = self.acl.entity_from_dict(entry)
-            self.acl.add_entity(entity)
+        url_path = '%s/acl' % self.path
+        found = self.connection.api_request(method='GET', path=url_path)
+        for entry in found['items']:
+            self.acl.add_entity(self.acl.entity_from_dict(entry))
+
+        # Even if we fetch no entries, the ACL is still loaded.
+        self.acl.loaded = True
 
         return self
 
@@ -396,7 +407,7 @@ def get_acl(self):
         :rtype: :class:`gcloud.storage.acl.ObjectACL`
         :returns: An ACL object for the current key.
         """
-        if not self.acl:
+        if not self.acl.loaded:
             self.reload_acl()
         return self.acl
 
@@ -407,16 +418,21 @@ def save_acl(self, acl=None):
         :param acl: The ACL object to save.  If left blank, this will
                     save the ACL set locally on the key.
         """
-        # We do things in this weird way because [] and None
-        # both evaluate to False, but mean very different things.
         if acl is None:
             acl = self.acl
+            dirty = acl.loaded
+        else:
+            dirty = True
 
-        if acl is None:
-            return self
+        if dirty:
+            result = self.connection.api_request(
+                method='PATCH', path=self.path, data={'acl': list(acl)},
+                query_params={'projection': 'full'})
+            self.acl.clear()
+            for entry in result['acl']:
+                self.acl.entity(self.acl.entity_from_dict(entry))
+            self.acl.loaded = True
 
-        self.patch_metadata({'acl': list(acl)})
-        self.reload_acl()
         return self
 
     def clear_acl(self):
@@ -427,7 +443,7 @@ def clear_acl(self):
         have access to a key that you created even after you clear ACL
         rules with this method.
         """
-        return self.save_acl(acl=[])
+        return self.save_acl([])
 
     def make_public(self):
         """Make this key public giving all users read access.