From 29e9ae1cc5b74dce205643c101601555c06b67dc Mon Sep 17 00:00:00 2001
From: Tim Burke <tim.burke@gmail.com>
Date: Thu, 19 Oct 2017 18:53:04 +0000
Subject: [PATCH] Make xml responses less insane

Looking at bulk extractions:

   $ tar c *.py | curl http://saio:8090/v1/AUTH_test/c?extract-archive=tar \
      -H accept:application/xml -T -
   <?xml version="1.0" encoding="UTF-8"?>
   <delete>
   <number_files_created>2</number_files_created>
   <response_body></response_body>
   <response_status>201 Created</response_status>
   <errors>
   </errors>
   </delete>

Or SLO upload failures:

   $ curl http://saio:8090/v1/AUTH_test/c/slo?multipart-manifest=put -X PUT \
      -d '[{"path": "not/found"}]' -H accept:application/xml
   <delete>
   <errors>
   <object><name>not/found</name><status>404 Not Found</status></object></errors>
   </delete>

Why <delete>? Makes no sense.

Drive-by: stop being so quadratic.

Change-Id: I46b233864ba2815ac632d856b9f3c40cc9d0001a
---
 swift/common/middleware/bulk.py          | 54 ++++++++++++++----------
 swift/common/middleware/slo.py           |  2 +-
 test/unit/common/middleware/test_bulk.py |  8 +++-
 3 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/swift/common/middleware/bulk.py b/swift/common/middleware/bulk.py
index 2ca73d7be8..d3301c39ea 100644
--- a/swift/common/middleware/bulk.py
+++ b/swift/common/middleware/bulk.py
@@ -218,40 +218,48 @@ ACCEPTABLE_FORMATS = ['text/plain', 'application/json', 'application/xml',
                       'text/xml']
 
 
-def get_response_body(data_format, data_dict, error_list):
+def get_response_body(data_format, data_dict, error_list, root_tag):
     """
-    Returns a properly formatted response body according to format. Handles
-    json and xml, otherwise will return text/plain. Note: xml response does not
-    include xml declaration.
+    Returns a properly formatted response body according to format.
+
+    Handles json and xml, otherwise will return text/plain.
+    Note: xml response does not include xml declaration.
+
     :params data_format: resulting format
     :params data_dict: generated data about results.
     :params error_list: list of quoted filenames that failed
+    :params root_tag: the tag name to use for root elements when returning XML;
+                      e.g. 'extract' or 'delete'
     """
     if data_format == 'application/json':
         data_dict['Errors'] = error_list
         return json.dumps(data_dict)
     if data_format and data_format.endswith('/xml'):
-        output = '<delete>\n'
+        output = ['<', root_tag, '>\n']
         for key in sorted(data_dict):
             xml_key = key.replace(' ', '_').lower()
-            output += '<%s>%s</%s>\n' % (xml_key, data_dict[key], xml_key)
-        output += '<errors>\n'
-        output += '\n'.join(
-            ['<object>'
-             '<name>%s</name><status>%s</status>'
-             '</object>' % (saxutils.escape(name), status) for
-             name, status in error_list])
-        output += '</errors>\n</delete>\n'
-        return output
+            output.extend([
+                '<', xml_key, '>',
+                saxutils.escape(str(data_dict[key])),
+                '</', xml_key, '>\n',
+            ])
+        output.append('<errors>\n')
+        for name, status in error_list:
+            output.extend([
+                '<object><name>', saxutils.escape(name), '</name><status>',
+                saxutils.escape(status), '</status></object>\n',
+            ])
+        output.extend(['</errors>\n</', root_tag, '>\n'])
+        return ''.join(output)
 
-    output = ''
+    output = []
     for key in sorted(data_dict):
-        output += '%s: %s\n' % (key, data_dict[key])
-    output += 'Errors:\n'
-    output += '\n'.join(
-        ['%s, %s' % (name, status)
-         for name, status in error_list])
-    return output
+        output.append('%s: %s\n' % (key, data_dict[key]))
+    output.append('Errors:\n')
+    output.extend(
+        '%s, %s\n' % (name, status)
+        for name, status in error_list)
+    return ''.join(output)
 
 
 def pax_key_to_swift_header(pax_key):
@@ -485,7 +493,7 @@ class Bulk(object):
             resp_dict['Response Status'] = HTTPServerError().status
 
         yield separator + get_response_body(out_content_type,
-                                            resp_dict, failed_files)
+                                            resp_dict, failed_files, 'delete')
 
     def handle_extract_iter(self, req, compress_type,
                             out_content_type='text/plain'):
@@ -639,7 +647,7 @@ class Bulk(object):
             resp_dict['Response Status'] = HTTPServerError().status
 
         yield separator + get_response_body(
-            out_content_type, resp_dict, failed_files)
+            out_content_type, resp_dict, failed_files, 'extract')
 
     def _process_delete(self, resp, pile, obj_name, resp_dict,
                         failed_files, failed_file_response, retry=0):
diff --git a/swift/common/middleware/slo.py b/swift/common/middleware/slo.py
index b27ab60674..acd2735bb0 100644
--- a/swift/common/middleware/slo.py
+++ b/swift/common/middleware/slo.py
@@ -1022,7 +1022,7 @@ class StaticLargeObject(object):
 
         if problem_segments:
             resp_body = get_response_body(
-                out_content_type, {}, problem_segments)
+                out_content_type, {}, problem_segments, 'upload')
             raise HTTPBadRequest(resp_body, content_type=out_content_type)
 
         slo_etag = md5()
diff --git a/test/unit/common/middleware/test_bulk.py b/test/unit/common/middleware/test_bulk.py
index dc9be5647c..feab2dd0c9 100644
--- a/test/unit/common/middleware/test_bulk.py
+++ b/test/unit/common/middleware/test_bulk.py
@@ -604,11 +604,15 @@ class TestUntar(unittest.TestCase):
 
     def test_get_response_body(self):
         txt_body = bulk.get_response_body(
-            'bad_formay', {'hey': 'there'}, [['json > xml', '202 Accepted']])
+            'bad_formay', {'hey': 'there'}, [['json > xml', '202 Accepted']],
+            "doesn't matter for text")
         self.assertTrue('hey: there' in txt_body)
         xml_body = bulk.get_response_body(
-            'text/xml', {'hey': 'there'}, [['json > xml', '202 Accepted']])
+            'text/xml', {'hey': 'there'}, [['json > xml', '202 Accepted']],
+            'root_tag')
         self.assertTrue('&gt' in xml_body)
+        self.assertTrue(xml_body.startswith('<root_tag>\n'))
+        self.assertTrue(xml_body.endswith('\n</root_tag>\n'))
 
 
 class TestDelete(unittest.TestCase):