Fix misnamed dictionary key.

The response dictionary in _delete_segment attach's any error
it encounters to the dict key 'exception', all other response
dict's use 'error' to store the exception.

Changed to make it consistent and added tests

Also added a third branch in st_delete as messages
created in _delete_container where being silently dropped.

Change-Id: Ifbc3b1fae78910fbc6acf4a86cfb0f60bb1aa336

4 files changed, 328 insertions, 72 deletions

diff --git a/swiftclient/service.py b/swiftclient/service.py
index 326a041..55e6daa 100644
--- a/swiftclient/service.py
+++ b/swiftclient/service.py
@@ -1530,8 +1530,8 @@ class SwiftService(object):
             old_manifest = None
             old_slo_manifest_paths = []
             new_slo_manifest_paths = set()
-            if options['changed'] or options['skip_identical'] \
-                    or not options['leave_segments']:
+            if (options['changed'] or options['skip_identical']
+                    or not options['leave_segments']):
                 checksum = None
                 if options['skip_identical']:
                     try:
@@ -1556,11 +1556,12 @@ class SwiftService(object):
                                 'status': 'skipped-identical'
                             })
                             return res
+
                     cl = int(headers.get('content-length'))
                     mt = headers.get('x-object-meta-mtime')
-                    if path is not None and options['changed']\
-                            and cl == getsize(path) and \
-                            mt == put_headers['x-object-meta-mtime']:
+                    if (path is not None and options['changed']
+                            and cl == getsize(path)
+                            and mt == put_headers['x-object-meta-mtime']):
                         res.update({
                             'success': True,
                             'status': 'skipped-changed'
@@ -1594,8 +1595,8 @@ class SwiftService(object):
             # a segment job if we're reading from a stream - we may fail if we
             # go over the single object limit, but this gives us a nice way
             # to create objects from memory
-            if path is not None and options['segment_size'] and \
-                    getsize(path) > int(options['segment_size']):
+            if (path is not None and options['segment_size']
+                    and getsize(path) > int(options['segment_size'])):
                 res['large_object'] = True
                 seg_container = container + '_segments'
                 if options['segment_container']:
@@ -1851,9 +1852,8 @@ class SwiftService(object):
 
                             # Cancel the remaining container deletes, but yield
                             # any pending results
-                            if not cancelled and \
-                                    options['fail_fast'] and \
-                                    not res['success']:
+                            if (not cancelled and options['fail_fast']
+                                    and not res['success']):
                                 cancelled = True
 
     @staticmethod
@@ -1861,24 +1861,17 @@ class SwiftService(object):
         results_dict = {}
         try:
             conn.delete_object(container, obj, response_dict=results_dict)
-            res = {
-                'action': 'delete_segment',
-                'container': container,
-                'object': obj,
-                'success': True,
-                'attempts': conn.attempts,
-                'response_dict': results_dict
-            }
+            res = {'success': True}
         except Exception as e:
-            res = {
-                'action': 'delete_segment',
-                'container': container,
-                'object': obj,
-                'success': False,
-                'attempts': conn.attempts,
-                'response_dict': results_dict,
-                'exception': e
-            }
+            res = {'success': False, 'error': e}
+
+        res.update({
+            'action': 'delete_segment',
+            'container': container,
+            'object': obj,
+            'attempts': conn.attempts,
+            'response_dict': results_dict
+        })
 
         if results_queue is not None:
             results_queue.put(res)
@@ -1899,8 +1892,7 @@ class SwiftService(object):
                 try:
                     headers = conn.head_object(container, obj)
                     old_manifest = headers.get('x-object-manifest')
-                    if config_true_value(
-                            headers.get('x-static-large-object')):
+                    if config_true_value(headers.get('x-static-large-object')):
                         query_string = 'multipart-manifest=delete'
                 except ClientException as err:
                     if err.http_status != 404:
@@ -1958,23 +1950,17 @@ class SwiftService(object):
         results_dict = {}
         try:
             conn.delete_container(container, response_dict=results_dict)
-            res = {
-                'action': 'delete_container',
-                'container': container,
-                'object': None,
-                'success': True,
-                'attempts': conn.attempts,
-                'response_dict': results_dict
-            }
+            res = {'success': True}
         except Exception as e:
-            res = {
-                'action': 'delete_container',
-                'container': container,
-                'object': None,
-                'success': False,
-                'response_dict': results_dict,
-                'error': e
-            }
+            res = {'success': False, 'error': e}
+
+        res.update({
+            'action': 'delete_container',
+            'container': container,
+            'object': None,
+            'attempts': conn.attempts,
+            'response_dict': results_dict
+        })
         return res
 
     def _delete_container(self, container, options):
@@ -1982,9 +1968,7 @@ class SwiftService(object):
             objs = []
             for part in self.list(container=container):
                 if part["success"]:
-                    objs.extend([
-                        o['name'] for o in part['listing']
-                    ])
+                    objs.extend([o['name'] for o in part['listing']])
                 else:
                     raise part["error"]
 
diff --git a/swiftclient/shell.py b/swiftclient/shell.py
index 2c627ad..8d4da67 100755
--- a/swiftclient/shell.py
+++ b/swiftclient/shell.py
@@ -118,34 +118,29 @@ def st_delete(parser, args, output_manager):
                     del_iter = swift.delete(container=container)
 
             for r in del_iter:
+                c = r.get('container', '')
+                o = r.get('object', '')
+                a = r.get('attempts')
+
                 if r['success']:
                     if options.verbose:
+                        a = ' [after {0} attempts]'.format(a) if a > 1 else ''
+
                         if r['action'] == 'delete_object':
-                            c = r['container']
-                            o = r['object']
-                            p = '%s/%s' % (c, o) if options.yes_all else o
-                            a = r['attempts']
-                            if a > 1:
-                                output_manager.print_msg(
-                                    '%s [after %d attempts]', p, a)
+                            if options.yes_all:
+                                p = '{0}/{1}'.format(c, o)
                             else:
-                                output_manager.print_msg(p)
-
+                                p = o
                         elif r['action'] == 'delete_segment':
-                            c = r['container']
-                            o = r['object']
-                            p = '%s/%s' % (c, o)
-                            a = r['attempts']
-                            if a > 1:
-                                output_manager.print_msg(
-                                    '%s [after %d attempts]', p, a)
-                            else:
-                                output_manager.print_msg(p)
+                            p = '{0}/{1}'.format(c, o)
+                        elif r['action'] == 'delete_container':
+                            p = c
 
+                        output_manager.print_msg('{0}{1}'.format(p, a))
                 else:
-                    # Special case error prints
-                    output_manager.error("An unexpected error occurred whilst "
-                                         "deleting: %s" % r['error'])
+                    p = '{0}/{1}'.format(c, o) if o else c
+                    output_manager.error('Error Deleting: {0}: {1}'
+                                         .format(p, r['error']))
         except SwiftError as err:
             output_manager.error(err.value)
 
diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py
index 5211456..e10c065 100644
--- a/tests/unit/test_service.py
+++ b/tests/unit/test_service.py
@@ -12,12 +12,15 @@
 # implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-
+import mock
 import testtools
+from mock import Mock, PropertyMock
+from six.moves.queue import Queue, Empty as QueueEmptyError
 from hashlib import md5
 
-from swiftclient.service import SwiftService, SwiftError
 import swiftclient
+from swiftclient.service import SwiftService, SwiftError
+from swiftclient.client import Connection
 
 
 class TestSwiftPostObject(testtools.TestCase):
@@ -59,7 +62,7 @@ class TestSwiftReader(testtools.TestCase):
         self.assertTrue(isinstance(sr._actual_md5, self.md5_type))
 
     def test_create_with_large_object_headers(self):
-        # md5 should not be initalized if large object headers are present
+        # md5 should not be initialized if large object headers are present
         sr = self.sr('path', 'body', {'x-object-manifest': 'test'})
         self.assertEqual(sr._path, 'path')
         self.assertEqual(sr._body, 'body')
@@ -129,6 +132,225 @@ class TestSwiftReader(testtools.TestCase):
                          '97ac82a5b825239e782d0339e2d7b910')
 
 
+class TestServiceDelete(testtools.TestCase):
+    def setUp(self):
+        super(TestServiceDelete, self).setUp()
+        self.opts = {'leave_segments': False, 'yes_all': False}
+        self.exc = Exception('test_exc')
+        # Base response to be copied and updated to matched the expected
+        # response for each test
+        self.expected = {
+            'action': None,   # Should be string in the form delete_XX
+            'container': 'test_c',
+            'object': 'test_o',
+            'attempts': 2,
+            'response_dict': {},
+            'success': None   # Should be a bool
+        }
+
+    def _get_mock_connection(self, attempts=2):
+        m = Mock(spec=Connection)
+        type(m).attempts = PropertyMock(return_value=attempts)
+        return m
+
+    def _get_queue(self, q):
+        # Instead of blocking pull items straight from the queue.
+        # expects at least one item otherwise the test will fail.
+        try:
+            return q.get_nowait()
+        except QueueEmptyError:
+            self.fail('Expected item in queue but found none')
+
+    def _get_expected(self, update=None):
+        expected = self.expected.copy()
+        if update:
+            expected.update(update)
+
+        return expected
+
+    def _assertDictEqual(self, a, b, m=None):
+        # assertDictEqual is not available in py2.6 so use a shallow check
+        # instead
+        if hasattr(self, 'assertDictEqual'):
+            self.assertDictEqual(a, b, m)
+        else:
+            self.assertTrue(isinstance(a, dict))
+            self.assertTrue(isinstance(b, dict))
+            self.assertEqual(len(a), len(b), m)
+            for k, v in a.items():
+                self.assertTrue(k in b, m)
+                self.assertEqual(b[k], v, m)
+
+    def test_delete_segment(self):
+        mock_q = Queue()
+        mock_conn = self._get_mock_connection()
+        expected_r = self._get_expected({
+            'action': 'delete_segment',
+            'object': 'test_s',
+            'success': True,
+        })
+
+        r = SwiftService._delete_segment(mock_conn, 'test_c', 'test_s', mock_q)
+
+        mock_conn.delete_object.assert_called_once_with(
+            'test_c', 'test_s', response_dict={}
+        )
+        self._assertDictEqual(expected_r, r)
+        self._assertDictEqual(expected_r, self._get_queue(mock_q))
+
+    def test_delete_segment_exception(self):
+        mock_q = Queue()
+        mock_conn = self._get_mock_connection()
+        mock_conn.delete_object = Mock(side_effect=self.exc)
+        expected_r = self._get_expected({
+            'action': 'delete_segment',
+            'object': 'test_s',
+            'success': False,
+            'error': self.exc
+        })
+
+        r = SwiftService._delete_segment(mock_conn, 'test_c', 'test_s', mock_q)
+
+        mock_conn.delete_object.assert_called_once_with(
+            'test_c', 'test_s', response_dict={}
+        )
+        self._assertDictEqual(expected_r, r)
+        self._assertDictEqual(expected_r, self._get_queue(mock_q))
+
+    def test_delete_object(self):
+        mock_q = Queue()
+        mock_conn = self._get_mock_connection()
+        mock_conn.head_object = Mock(return_value={})
+        expected_r = self._get_expected({
+            'action': 'delete_object',
+            'success': True
+        })
+
+        s = SwiftService()
+        r = s._delete_object(mock_conn, 'test_c', 'test_o', self.opts, mock_q)
+
+        mock_conn.head_object.assert_called_once_with('test_c', 'test_o')
+        mock_conn.delete_object.assert_called_once_with(
+            'test_c', 'test_o', query_string=None, response_dict={}
+        )
+        self._assertDictEqual(expected_r, r)
+
+    def test_delete_object_exception(self):
+        mock_q = Queue()
+        mock_conn = self._get_mock_connection()
+        mock_conn.delete_object = Mock(side_effect=self.exc)
+        expected_r = self._get_expected({
+            'action': 'delete_object',
+            'success': False,
+            'error': self.exc
+        })
+        # _delete_object doesnt populate attempts or response dict if it hits
+        # an error. This may not be the correct behaviour.
+        del expected_r['response_dict'], expected_r['attempts']
+
+        s = SwiftService()
+        r = s._delete_object(mock_conn, 'test_c', 'test_o', self.opts, mock_q)
+
+        mock_conn.head_object.assert_called_once_with('test_c', 'test_o')
+        mock_conn.delete_object.assert_called_once_with(
+            'test_c', 'test_o', query_string=None, response_dict={}
+        )
+        self._assertDictEqual(expected_r, r)
+
+    def test_delete_object_slo_support(self):
+        # If SLO headers are present the delete call should include an
+        # additional query string to cause the right delete server side
+        mock_q = Queue()
+        mock_conn = self._get_mock_connection()
+        mock_conn.head_object = Mock(
+            return_value={'x-static-large-object': True}
+        )
+        expected_r = self._get_expected({
+            'action': 'delete_object',
+            'success': True
+        })
+
+        s = SwiftService()
+        r = s._delete_object(mock_conn, 'test_c', 'test_o', self.opts, mock_q)
+
+        mock_conn.head_object.assert_called_once_with('test_c', 'test_o')
+        mock_conn.delete_object.assert_called_once_with(
+            'test_c', 'test_o',
+            query_string='multipart-manifest=delete',
+            response_dict={}
+        )
+        self._assertDictEqual(expected_r, r)
+
+    def test_delete_object_dlo_support(self):
+        mock_q = Queue()
+        s = SwiftService()
+        mock_conn = self._get_mock_connection()
+        expected_r = self._get_expected({
+            'action': 'delete_object',
+            'success': True,
+            'dlo_segments_deleted': True
+        })
+        # A DLO object is determined in _delete_object by heading the object
+        # and checking for the existence of a x-object-manifest header.
+        # Mock that here.
+        mock_conn.head_object = Mock(
+            return_value={'x-object-manifest': 'manifest_c/manifest_p'}
+        )
+        mock_conn.get_container = Mock(
+            side_effect=[(None, [{'name': 'test_seg_1'},
+                                 {'name': 'test_seg_2'}]),
+                         (None, {})]
+        )
+
+        def get_mock_list_conn(options):
+            return mock_conn
+
+        with mock.patch('swiftclient.service.get_conn', get_mock_list_conn):
+            r = s._delete_object(
+                mock_conn, 'test_c', 'test_o', self.opts, mock_q
+            )
+
+        self._assertDictEqual(expected_r, r)
+        expected = [
+            mock.call('test_c', 'test_o', query_string=None, response_dict={}),
+            mock.call('manifest_c', 'test_seg_1', response_dict={}),
+            mock.call('manifest_c', 'test_seg_2', response_dict={})]
+        mock_conn.delete_object.assert_has_calls(expected, any_order=True)
+
+    def test_delete_empty_container(self):
+        mock_conn = self._get_mock_connection()
+        expected_r = self._get_expected({
+            'action': 'delete_container',
+            'success': True,
+            'object': None
+        })
+
+        r = SwiftService._delete_empty_container(mock_conn, 'test_c')
+
+        mock_conn.delete_container.assert_called_once_with(
+            'test_c', response_dict={}
+        )
+        self._assertDictEqual(expected_r, r)
+
+    def test_delete_empty_container_excpetion(self):
+        mock_conn = self._get_mock_connection()
+        mock_conn.delete_container = Mock(side_effect=self.exc)
+        expected_r = self._get_expected({
+            'action': 'delete_container',
+            'success': False,
+            'object': None,
+            'error': self.exc
+        })
+
+        s = SwiftService()
+        r = s._delete_empty_container(mock_conn, 'test_c')
+
+        mock_conn.delete_container.assert_called_once_with(
+            'test_c', response_dict={}
+        )
+        self._assertDictEqual(expected_r, r)
+
+
 class TestService(testtools.TestCase):
 
     def test_upload_with_bad_segment_size(self):
diff --git a/tests/unit/test_shell.py b/tests/unit/test_shell.py
index 9d44248..617fba2 100644
--- a/tests/unit/test_shell.py
+++ b/tests/unit/test_shell.py
@@ -387,6 +387,61 @@ class TestShell(unittest.TestCase):
         connection.return_value.delete_object.assert_called_with(
             'container', 'object', query_string=None, response_dict={})
 
+    def test_delete_verbose_output(self):
+        del_obj_res = {'success': True, 'response_dict': {}, 'attempts': 2,
+                       'container': 'test_c', 'action': 'delete_object',
+                       'object': 'test_o'}
+
+        del_seg_res = del_obj_res.copy()
+        del_seg_res.update({'action': 'delete_segment'})
+
+        del_con_res = del_obj_res.copy()
+        del_con_res.update({'action': 'delete_container', 'object': None})
+
+        test_exc = Exception('test_exc')
+        error_res = del_obj_res.copy()
+        error_res.update({'success': False, 'error': test_exc, 'object': None})
+
+        mock_delete = mock.Mock()
+        base_argv = ['', '--verbose', 'delete']
+
+        with mock.patch('swiftclient.shell.SwiftService.delete', mock_delete):
+            with CaptureOutput() as out:
+                mock_delete.return_value = [del_obj_res]
+                swiftclient.shell.main(base_argv + ['test_c', 'test_o'])
+
+                mock_delete.assert_called_once_with(container='test_c',
+                                                    objects=['test_o'])
+                self.assertTrue(out.out.find(
+                    'test_o [after 2 attempts]') >= 0)
+
+            with CaptureOutput() as out:
+                mock_delete.return_value = [del_seg_res]
+                swiftclient.shell.main(base_argv + ['test_c', 'test_o'])
+
+                mock_delete.assert_called_with(container='test_c',
+                                               objects=['test_o'])
+                self.assertTrue(out.out.find(
+                    'test_c/test_o [after 2 attempts]') >= 0)
+
+            with CaptureOutput() as out:
+                mock_delete.return_value = [del_con_res]
+                swiftclient.shell.main(base_argv + ['test_c'])
+
+                mock_delete.assert_called_with(container='test_c')
+                self.assertTrue(out.out.find(
+                    'test_c [after 2 attempts]') >= 0)
+
+            with CaptureOutput() as out:
+                mock_delete.return_value = [error_res]
+                self.assertRaises(SystemExit,
+                                  swiftclient.shell.main,
+                                  base_argv + ['test_c'])
+
+                mock_delete.assert_called_with(container='test_c')
+                self.assertTrue(out.err.find(
+                    'Error Deleting: test_c: test_exc') >= 0)
+
     @mock.patch('swiftclient.service.Connection')
     def test_post_account(self, connection):
         argv = ["", "post"]