Merge pull request #682 from tseaver/652-handle_empty_acls

#652: Harden ACL 'save'/'reload' against missing element in server response.

Author: tseaver

gcloud/storage/acl.py

@@ -406,7 +406,7 @@ 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']:
+        for entry in found.get('items', ()):
             self.add_entity(self.entity_from_dict(entry))
 
         return self
@@ -450,7 +450,7 @@ def save(self, acl=None):
                 data={self._URL_PATH_ELEM: list(acl)},
                 query_params={'projection': 'full'})
             self.entities.clear()
-            for entry in result[self._URL_PATH_ELEM]:
+            for entry in result.get(self._URL_PATH_ELEM, ()):
                 self.add_entity(self.entity_from_dict(entry))
             self.loaded = True
 
@@ -512,7 +512,7 @@ def reload(self):
         url_path = '%s/acl' % self.blob.path
         found = self.blob.connection.api_request(method='GET', path=url_path)
         self.loaded = True
-        for entry in found['items']:
+        for entry in found.get('items', ()):
             self.add_entity(self.entity_from_dict(entry))
 
         return self
@@ -535,7 +535,7 @@ def save(self, acl=None):
                 method='PATCH', path=self.blob.path, data={'acl': list(acl)},
                 query_params={'projection': 'full'})
             self.entities.clear()
-            for entry in result['acl']:
+            for entry in result.get('acl', ()):
                 self.add_entity(self.entity_from_dict(entry))
             self.loaded = True
 

gcloud/storage/test_acl.py

@@ -536,6 +536,22 @@ def test_ctor(self):
         self.assertFalse(acl.loaded)
         self.assertTrue(acl.bucket is bucket)
 
+    def test_reload_eager_missing(self):
+        # https://github.com/GoogleCloudPlatform/gcloud-python/issues/652
+        NAME = 'name'
+        ROLE = 'role'
+        connection = _Connection({})
+        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), [])
+        kw = connection._requested
+        self.assertEqual(len(kw), 1)
+        self.assertEqual(kw[0]['method'], 'GET')
+        self.assertEqual(kw[0]['path'], '/b/%s/acl' % NAME)
+
     def test_reload_eager_empty(self):
         NAME = 'name'
         ROLE = 'role'
@@ -589,6 +605,21 @@ def test_save_none_set_none_passed(self):
         kw = connection._requested
         self.assertEqual(len(kw), 0)
 
+    def test_save_existing_missing_none_passed(self):
+        NAME = 'name'
+        connection = _Connection({})
+        bucket = _Bucket(connection, NAME)
+        acl = self._makeOne(bucket)
+        acl.loaded = True
+        self.assertTrue(acl.save() is acl)
+        self.assertEqual(list(acl), [])
+        kw = connection._requested
+        self.assertEqual(len(kw), 1)
+        self.assertEqual(kw[0]['method'], 'PATCH')
+        self.assertEqual(kw[0]['path'], '/b/%s' % NAME)
+        self.assertEqual(kw[0]['data'], {'acl': []})
+        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
+
     def test_save_no_arg(self):
         NAME = 'name'
         ROLE = 'role'
@@ -665,24 +696,22 @@ def test_ctor(self):
         self.assertFalse(acl.loaded)
         self.assertTrue(acl.blob is blob)
 
-    def test_reload_eager_empty(self):
+    def test_reload_eager_missing(self):
+        # https://github.com/GoogleCloudPlatform/gcloud-python/issues/652
         NAME = 'name'
         BLOB_NAME = 'blob-name'
         ROLE = 'role'
-        after = {'items': [{'entity': 'allUsers', 'role': ROLE}]}
+        after = {}
         connection = _Connection(after)
         bucket = _Bucket(connection, NAME)
         blob = _Blob(bucket, BLOB_NAME)
         acl = self._makeOne(blob)
         acl.loaded = True
+        acl.entity('allUsers', ROLE)
         self.assertTrue(acl.reload() is acl)
-        self.assertEqual(list(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' % BLOB_NAME)
+        self.assertEqual(list(acl), [])
 
-    def test_reload_eager_nonempty(self):
+    def test_reload_eager_empty(self):
         NAME = 'name'
         BLOB_NAME = 'blob-name'
         ROLE = 'role'
@@ -696,6 +725,23 @@ def test_reload_eager_nonempty(self):
         self.assertTrue(acl.reload() is acl)
         self.assertEqual(list(acl), [])
 
+    def test_reload_eager_nonempty(self):
+        NAME = 'name'
+        BLOB_NAME = 'blob-name'
+        ROLE = 'role'
+        after = {'items': [{'entity': 'allUsers', 'role': ROLE}]}
+        connection = _Connection(after)
+        bucket = _Bucket(connection, NAME)
+        blob = _Blob(bucket, BLOB_NAME)
+        acl = self._makeOne(blob)
+        acl.loaded = True
+        self.assertTrue(acl.reload() is acl)
+        self.assertEqual(list(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' % BLOB_NAME)
+
     def test_reload_lazy(self):
         NAME = 'name'
         BLOB_NAME = 'blob-name'
@@ -724,6 +770,23 @@ def test_save_none_set_none_passed(self):
         kw = connection._requested
         self.assertEqual(len(kw), 0)
 
+    def test_save_existing_missing_none_passed(self):
+        # https://github.com/GoogleCloudPlatform/gcloud-python/issues/652
+        NAME = 'name'
+        BLOB_NAME = 'blob-name'
+        connection = _Connection({'foo': 'Foo'})
+        bucket = _Bucket(connection, NAME)
+        blob = _Blob(bucket, BLOB_NAME)
+        acl = self._makeOne(blob)
+        acl.loaded = True
+        self.assertTrue(acl.save() is acl)
+        kw = connection._requested
+        self.assertEqual(len(kw), 1)
+        self.assertEqual(kw[0]['method'], 'PATCH')
+        self.assertEqual(kw[0]['path'], '/b/name/o/%s' % BLOB_NAME)
+        self.assertEqual(kw[0]['data'], {'acl': []})
+        self.assertEqual(kw[0]['query_params'], {'projection': 'full'})
+
     def test_save_existing_set_none_passed(self):
         NAME = 'name'
         BLOB_NAME = 'blob-name'