Use ACL-specific endpoints where feasible for keys.

- Refactor lazy-handling of 'acl' using a property.

- Fetch 'acl' via GET to a specific endpoint.

- Continue to update 'acl' via PATCH to the object's resource (we can't do
  incremental updates to ACEs).

Fixes #151.

Author: tseaver

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):
@@ -375,11 +381,15 @@ def reload_acl(self):
         :rtype: :class:`Key`
         :returns: The current key.
         """
-        self.acl = ObjectACL(key=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
 
@@ -389,7 +399,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
 
@@ -400,16 +410,19 @@ 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)})
+            self.acl.clear()
+            for entry in result['acl']:
+                self.acl.entity(self.acl.entity_from_dict(entry))
 
-        self.patch_metadata({'acl': list(acl)})
-        self.reload_acl()
         return self
 
     def clear_acl(self):
@@ -420,7 +433,11 @@ 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=[])
+        self.connection.api_request(
+            method='PATCH', path=self.path, data={'acl': []})
+        self.acl.clear()
+        self.acl.loaded = True
+        return self
 
     def make_public(self):
         """Make this key public giving all users read access.

gcloud/storage/test_key.py

@@ -16,7 +16,7 @@ def test_ctor_defaults(self):
         self.assertEqual(key.connection, None)
         self.assertEqual(key.name, None)
         self.assertEqual(key.metadata, {})
-        self.assertEqual(key.acl, None)
+        self.assertTrue(key._acl is None)
 
     def test_ctor_explicit(self):
         KEY = 'key'
@@ -28,7 +28,7 @@ def test_ctor_explicit(self):
         self.assertTrue(key.connection is connection)
         self.assertEqual(key.name, KEY)
         self.assertEqual(key.metadata, metadata)
-        self.assertEqual(key.acl, None)
+        self.assertTrue(key._acl is None)
 
     def test_from_dict_defaults(self):
         KEY = 'key'
@@ -39,7 +39,7 @@ def test_from_dict_defaults(self):
         self.assertEqual(key.connection, None)
         self.assertEqual(key.name, KEY)
         self.assertEqual(key.metadata, metadata)
-        self.assertEqual(key.acl, None)
+        self.assertTrue(key._acl is None)
 
     def test_from_dict_explicit(self):
         KEY = 'key'
@@ -52,7 +52,14 @@ def test_from_dict_explicit(self):
         self.assertTrue(key.connection is connection)
         self.assertEqual(key.name, KEY)
         self.assertEqual(key.metadata, metadata)
-        self.assertEqual(key.acl, None)
+        self.assertTrue(key._acl is None)
+
+    def test_acl_property(self):
+        from gcloud.storage.acl import ObjectACL
+        key = self._makeOne()
+        acl = key.acl
+        self.assertTrue(isinstance(acl, ObjectACL))
+        self.assertTrue(acl is key._acl)
 
     def test_path_no_bucket(self):
         key = self._makeOne()
@@ -461,27 +468,39 @@ def test_patch_metadata(self):
 
     def test_reload_acl_eager_empty(self):
         from gcloud.storage.acl import ObjectACL
-        metadata = {'acl': []}
-        key = self._makeOne(metadata=metadata)
+        KEY = 'key'
+        ROLE = 'role'
+        after = {'items': [{'entity': 'allUsers', 'role': ROLE}]}
+        connection = _Connection(after)
+        bucket = _Bucket(connection)
+        key = self._makeOne(bucket, KEY)
+        key.acl.loaded = True
         self.assertTrue(key.reload_acl() is key)
         self.assertTrue(isinstance(key.acl, ObjectACL))
-        self.assertEqual(list(key.acl), [])
+        self.assertEqual(list(key.acl), after['items'])
+        kw = connection._requested
+        self.assertEqual(len(kw), 1)
+        self.assertEqual(kw[0]['method'], 'GET')
+        self.assertEqual(kw[0]['path'], '/b/name/o/%s/acl' % KEY)
 
     def test_reload_acl_eager_nonempty(self):
         from gcloud.storage.acl import ObjectACL
+        KEY = 'key'
         ROLE = 'role'
-        metadata = {'acl': [{'entity': 'allUsers', 'role': ROLE}]}
-        key = self._makeOne(metadata=metadata)
+        after = {'items': []}
+        connection = _Connection(after)
+        bucket = _Bucket(connection)
+        key = self._makeOne(bucket, KEY)
+        key.acl.entity('allUsers', ROLE)
         self.assertTrue(key.reload_acl() is key)
         self.assertTrue(isinstance(key.acl, ObjectACL))
-        self.assertEqual(list(key.acl),
-                         [{'entity': 'allUsers', 'role': ROLE}])
+        self.assertEqual(list(key.acl), [])
 
     def test_reload_acl_lazy(self):
         from gcloud.storage.acl import ObjectACL
         KEY = 'key'
         ROLE = 'role'
-        after = {'acl': [{'entity': 'allUsers', 'role': ROLE}]}
+        after = {'items': [{'entity': 'allUsers', 'role': ROLE}]}
         connection = _Connection(after)
         bucket = _Bucket(connection)
         key = self._makeOne(bucket, KEY)
@@ -492,22 +511,23 @@ def test_reload_acl_lazy(self):
         kw = connection._requested
         self.assertEqual(len(kw), 1)
         self.assertEqual(kw[0]['method'], 'GET')
-        self.assertEqual(kw[0]['path'], '/b/name/o/%s' % KEY)
-        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
+        self.assertEqual(kw[0]['path'], '/b/name/o/%s/acl' % KEY)
 
     def test_get_acl_lazy(self):
         from gcloud.storage.acl import ObjectACL
-        metadata = {'acl': []}
-        key = self._makeOne(metadata=metadata)
+        KEY = 'key'
+        connection = _Connection({'items': []})
+        bucket = _Bucket(connection)
+        key = self._makeOne(bucket, KEY)
         acl = key.get_acl()
         self.assertTrue(acl is key.acl)
         self.assertTrue(isinstance(acl, ObjectACL))
         self.assertEqual(list(key.acl), [])
 
     def test_get_acl_eager(self):
-        from gcloud.storage.acl import ObjectACL
         key = self._makeOne()
-        preset = key.acl = ObjectACL(key)
+        preset = key.acl
+        preset.loaded = True
         acl = key.get_acl()
         self.assertTrue(acl is preset)
 
@@ -524,71 +544,62 @@ def test_save_acl_existing_set_none_passed(self):
         KEY = 'key'
         connection = _Connection({'foo': 'Foo', 'acl': []})
         bucket = _Bucket(connection)
-        metadata = {'acl': []}
-        key = self._makeOne(bucket, KEY, metadata)
-        key.reload_acl()
+        key = self._makeOne(bucket, KEY)
+        key.acl.loaded = True
         self.assertTrue(key.save_acl() is key)
         kw = connection._requested
         self.assertEqual(len(kw), 1)
         self.assertEqual(kw[0]['method'], 'PATCH')
         self.assertEqual(kw[0]['path'], '/b/name/o/%s' % KEY)
-        self.assertEqual(kw[0]['data'], metadata)
-        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
+        self.assertEqual(kw[0]['data'], {'acl': []})
 
     def test_save_acl_existing_set_new_passed(self):
         KEY = 'key'
         ROLE = 'role'
         new_acl = [{'entity': 'allUsers', 'role': ROLE}]
         connection = _Connection({'foo': 'Foo', 'acl': new_acl})
         bucket = _Bucket(connection)
-        metadata = {'acl': []}
-        key = self._makeOne(bucket, KEY, metadata)
-        key.reload_acl()
+        key = self._makeOne(bucket, KEY)
+        key.acl.entity('allUsers', 'other-role')
         self.assertTrue(key.save_acl(new_acl) is key)
         self.assertEqual(list(key.acl), new_acl)
         kw = connection._requested
         self.assertEqual(len(kw), 1)
         self.assertEqual(kw[0]['method'], 'PATCH')
         self.assertEqual(kw[0]['path'], '/b/name/o/%s' % KEY)
         self.assertEqual(kw[0]['data'], {'acl': new_acl})
-        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
 
     def test_clear_acl(self):
         KEY = 'key'
         ROLE = 'role'
-        old_acl = [{'entity': 'allUsers', 'role': ROLE}]
         connection = _Connection({'foo': 'Foo', 'acl': []})
         bucket = _Bucket(connection)
-        metadata = {'acl': old_acl}
-        key = self._makeOne(bucket, KEY, metadata)
-        key.reload_acl()
+        key = self._makeOne(bucket, KEY)
+        key.acl.entity('allUsers', ROLE)
         self.assertTrue(key.clear_acl() is key)
         self.assertEqual(list(key.acl), [])
         kw = connection._requested
         self.assertEqual(len(kw), 1)
         self.assertEqual(kw[0]['method'], 'PATCH')
         self.assertEqual(kw[0]['path'], '/b/name/o/%s' % KEY)
         self.assertEqual(kw[0]['data'], {'acl': []})
-        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
 
     def test_make_public(self):
         from gcloud.storage.acl import _ACLEntity
         KEY = 'key'
-        before = {'acl': []}
         permissive = [{'entity': 'allUsers', 'role': _ACLEntity.READER_ROLE}]
         after = {'acl': permissive}
         connection = _Connection(after)
         bucket = _Bucket(connection)
-        key = self._makeOne(bucket, KEY, before)
+        key = self._makeOne(bucket, KEY)
+        key.acl.loaded = True
         key.make_public()
-        self.assertEqual(key.metadata, after)
-        self.assertEqual(list(key.acl), after['acl'])
+        self.assertEqual(list(key.acl), permissive)
         kw = connection._requested
         self.assertEqual(len(kw), 1)
         self.assertEqual(kw[0]['method'], 'PATCH')
         self.assertEqual(kw[0]['path'], '/b/name/o/%s' % KEY)
-        self.assertEqual(kw[0]['data'], {'acl': after['acl']})
-        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
+        self.assertEqual(kw[0]['data'], {'acl': permissive})
 
 
 class TestKeyIterator(unittest2.TestCase):