Refactor how we pick listings' content type.

There were a few different places where we had some repeated code to
figure out what format an account or container listing response should
be in (text, JSON, or XML). Now that's been pulled into a single
function.

As part of this, you can now raise HTTPException subclasses in proxy
controllers instead of laboriously plumbing error responses all the
way back up to swift.proxy.server.Application.handle_request(). This
lets us avoid certain ugly patterns, like the one where a method
returns a tuple of (x, y, z, error) and the caller has to see if it
got (value, value, value, None) or (None, None, None, errorvalue). Now
we can just raise the error.

Change-Id: I316873df289160d526487ad116f6fbb9a575e3de

swift/account/server.py

@@ -23,21 +23,20 @@
 from eventlet import Timeout
 
 import swift.common.db
-from swift.account.utils import account_listing_response, \
-    account_listing_content_type
+from swift.account.utils import account_listing_response
 from swift.common.db import AccountBroker, DatabaseConnectionError
-from swift.common.request_helpers import get_param
+from swift.common.request_helpers import get_param, get_listing_content_type
 from swift.common.utils import get_logger, hash_path, public, \
     normalize_timestamp, storage_directory, config_true_value, \
     validate_device_partition, json, timing_stats, replication
 from swift.common.constraints import ACCOUNT_LISTING_LIMIT, \
-    check_mount, check_float, check_utf8, FORMAT2CONTENT_TYPE
+    check_mount, check_float, check_utf8
 from swift.common.db_replicator import ReplicatorRpc
 from swift.common.swob import HTTPAccepted, HTTPBadRequest, \
     HTTPCreated, HTTPForbidden, HTTPInternalServerError, \
     HTTPMethodNotAllowed, HTTPNoContent, HTTPNotFound, \
     HTTPPreconditionFailed, HTTPConflict, Request, \
-    HTTPInsufficientStorage, HTTPNotAcceptable, HTTPException
+    HTTPInsufficientStorage, HTTPException
 
 
 DATADIR = 'accounts'
@@ -181,14 +180,7 @@ def HEAD(self, req):
         except ValueError, err:
             return HTTPBadRequest(body=str(err), content_type='text/plain',
                                   request=req)
-        query_format = get_param(req, 'format')
-        if query_format:
-            req.accept = FORMAT2CONTENT_TYPE.get(
-                query_format.lower(), FORMAT2CONTENT_TYPE['plain'])
-        out_content_type = req.accept.best_match(
-            ['text/plain', 'application/json', 'application/xml', 'text/xml'])
-        if not out_content_type:
-            return HTTPNotAcceptable(request=req)
+        out_content_type = get_listing_content_type(req)
         if self.mount_check and not check_mount(self.root, drive):
             return HTTPInsufficientStorage(drive=drive, request=req)
         broker = self._get_account_broker(drive, part, account,
@@ -234,9 +226,7 @@ def GET(self, req):
                                               ACCOUNT_LISTING_LIMIT)
         marker = get_param(req, 'marker', '')
         end_marker = get_param(req, 'end_marker')
-        out_content_type, error = account_listing_content_type(req)
-        if error:
-            return error
+        out_content_type = get_listing_content_type(req)
 
         if self.mount_check and not check_mount(self.root, drive):
             return HTTPInsufficientStorage(drive=drive, request=req)

swift/account/utils.py

@@ -16,9 +16,7 @@
 import time
 from xml.sax import saxutils
 
-from swift.common.constraints import FORMAT2CONTENT_TYPE
-from swift.common.swob import HTTPOk, HTTPNoContent, HTTPNotAcceptable
-from swift.common.request_helpers import get_param
+from swift.common.swob import HTTPOk, HTTPNoContent
 from swift.common.utils import json, normalize_timestamp
 
 
@@ -43,25 +41,6 @@ def metadata(self):
         return {}
 
 
-def account_listing_content_type(req):
-    """
-    Figure out the content type of an account-listing response.
-
-    Returns a 2-tuple: (content_type, error). Only one of them will be set;
-    the other will be None.
-    """
-    query_format = get_param(req, 'format')
-    if query_format:
-        req.accept = FORMAT2CONTENT_TYPE.get(query_format.lower(),
-                                             FORMAT2CONTENT_TYPE['plain'])
-    content_type = req.accept.best_match(
-        ['text/plain', 'application/json', 'application/xml', 'text/xml'])
-    if not content_type:
-        return (None, HTTPNotAcceptable(request=req))
-    else:
-        return (content_type, None)
-
-
 def account_listing_response(account, req, response_content_type, broker=None,
                              limit='', marker='', end_marker='', prefix='',
                              delimiter=''):

swift/common/request_helpers.py

@@ -20,8 +20,8 @@
 from swob in here without creating circular imports.
 """
 
-
-from swift.common.swob import HTTPBadRequest
+from swift.common.constraints import FORMAT2CONTENT_TYPE
+from swift.common.swob import HTTPBadRequest, HTTPNotAcceptable
 
 
 def get_param(req, name, default=None):
@@ -45,3 +45,25 @@ def get_param(req, name, default=None):
                 request=req, content_type='text/plain',
                 body='"%s" parameter not valid UTF-8' % name)
     return value
+
+
+def get_listing_content_type(req):
+    """
+    Determine the content type to use for an account or container listing
+    response.
+
+    :param req: request object
+    :returns: content type as a string (e.g. text/plain, application/json)
+    :raises: HTTPNotAcceptable if the requested content type is not acceptable
+    :raises: HTTPBadRequest if the 'format' query param is provided and
+             not valid UTF-8
+    """
+    query_format = get_param(req, 'format')
+    if query_format:
+        req.accept = FORMAT2CONTENT_TYPE.get(
+            query_format.lower(), FORMAT2CONTENT_TYPE['plain'])
+    out_content_type = req.accept.best_match(
+        ['text/plain', 'application/json', 'application/xml', 'text/xml'])
+    if not out_content_type:
+        raise HTTPNotAcceptable(request=req)
+    return out_content_type

swift/container/server.py

@@ -26,21 +26,21 @@
 
 import swift.common.db
 from swift.common.db import ContainerBroker
-from swift.common.request_helpers import get_param
+from swift.common.request_helpers import get_param, get_listing_content_type
 from swift.common.utils import get_logger, hash_path, public, \
     normalize_timestamp, storage_directory, validate_sync_to, \
     config_true_value, validate_device_partition, json, timing_stats, \
     replication, parse_content_type
 from swift.common.constraints import CONTAINER_LISTING_LIMIT, \
-    check_mount, check_float, check_utf8, FORMAT2CONTENT_TYPE
+    check_mount, check_float, check_utf8
 from swift.common.bufferedhttp import http_connect
 from swift.common.exceptions import ConnectionTimeout
 from swift.common.db_replicator import ReplicatorRpc
 from swift.common.http import HTTP_NOT_FOUND, is_success
 from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPConflict, \
     HTTPCreated, HTTPInternalServerError, HTTPNoContent, HTTPNotFound, \
     HTTPPreconditionFailed, HTTPMethodNotAllowed, Request, Response, \
-    HTTPInsufficientStorage, HTTPNotAcceptable, HTTPException, HeaderKeyDict
+    HTTPInsufficientStorage, HTTPException, HeaderKeyDict
 
 DATADIR = 'containers'
 
@@ -300,14 +300,7 @@ def HEAD(self, req):
         except ValueError, err:
             return HTTPBadRequest(body=str(err), content_type='text/plain',
                                   request=req)
-        query_format = get_param(req, 'format')
-        if query_format:
-            req.accept = FORMAT2CONTENT_TYPE.get(
-                query_format.lower(), FORMAT2CONTENT_TYPE['plain'])
-        out_content_type = req.accept.best_match(
-            ['text/plain', 'application/json', 'application/xml', 'text/xml'])
-        if not out_content_type:
-            return HTTPNotAcceptable(request=req)
+        out_content_type = get_listing_content_type(req)
         if self.mount_check and not check_mount(self.root, drive):
             return HTTPInsufficientStorage(drive=drive, request=req)
         broker = self._get_container_broker(drive, part, account, container,
@@ -388,14 +381,7 @@ def GET(self, req):
                 return HTTPPreconditionFailed(
                     request=req,
                     body='Maximum limit is %d' % CONTAINER_LISTING_LIMIT)
-        query_format = get_param(req, 'format')
-        if query_format:
-            req.accept = FORMAT2CONTENT_TYPE.get(query_format.lower(),
-                                                 FORMAT2CONTENT_TYPE['plain'])
-        out_content_type = req.accept.best_match(
-            ['text/plain', 'application/json', 'application/xml', 'text/xml'])
-        if not out_content_type:
-            return HTTPNotAcceptable(request=req)
+        out_content_type = get_listing_content_type(req)
         if self.mount_check and not check_mount(self.root, drive):
             return HTTPInsufficientStorage(drive=drive, request=req)
         broker = self._get_container_broker(drive, part, account, container,

swift/proxy/controllers/account.py

@@ -27,8 +27,8 @@
 from gettext import gettext as _
 from urllib import unquote
 
-from swift.account.utils import account_listing_response, \
-    account_listing_content_type
+from swift.account.utils import account_listing_response
+from swift.common.request_helpers import get_listing_content_type
 from swift.common.utils import public
 from swift.common.constraints import check_metadata, MAX_ACCOUNT_NAME_LENGTH
 from swift.common.http import HTTP_NOT_FOUND
@@ -60,11 +60,8 @@ def GETorHEAD(self, req):
             req, _('Account'), self.app.account_ring, partition,
             req.path_info.rstrip('/'))
         if resp.status_int == HTTP_NOT_FOUND and self.app.account_autocreate:
-            content_type, error = account_listing_content_type(req)
-            if error:
-                return error
             resp = account_listing_response(self.account_name, req,
-                                            content_type)
+                                            get_listing_content_type(req))
         if not req.environ.get('swift_owner', False):
             for key in self.app.swift_owner_headers:
                 if key in resp.headers:

swift/proxy/server.py

@@ -42,7 +42,7 @@
     ContainerController
 from swift.common.swob import HTTPBadRequest, HTTPForbidden, \
     HTTPMethodNotAllowed, HTTPNotFound, HTTPPreconditionFailed, \
-    HTTPServerError, Request
+    HTTPServerError, HTTPException, Request
 
 
 class Application(object):
@@ -293,6 +293,8 @@ def handle_request(self, req):
             # method the client actually sent.
             req.environ['swift.orig_req_method'] = req.method
             return handler(req)
+        except HTTPException as error_response:
+            return error_response
         except (Exception, Timeout):
             self.logger.exception(_('ERROR Unhandled exception in request'))
             return HTTPServerError(request=req)

test/unit/account/test_server.py

@@ -129,7 +129,7 @@ def test_DELETE_insufficient_storage(self):
     def test_HEAD_not_found(self):
         # Test the case in which account does not exist (can be recreated)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 404)
         self.assertTrue('X-Account-Status' not in resp.headers)
 
@@ -156,7 +156,7 @@ def test_HEAD_empty_account(self):
             'HTTP_X_TIMESTAMP': '0'})
         self.controller.PUT(req)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 204)
         self.assertEquals(resp.headers['x-account-container-count'], '0')
         self.assertEquals(resp.headers['x-account-object-count'], '0')
@@ -181,7 +181,7 @@ def test_HEAD_with_containers(self):
                                      'X-Timestamp': normalize_timestamp(0)})
         self.controller.PUT(req)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 204)
         self.assertEquals(resp.headers['x-account-container-count'], '2')
         self.assertEquals(resp.headers['x-account-object-count'], '0')
@@ -202,7 +202,7 @@ def test_HEAD_with_containers(self):
         self.controller.PUT(req)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD',
             'HTTP_X_TIMESTAMP': '5'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 204)
         self.assertEquals(resp.headers['x-account-container-count'], '2')
         self.assertEquals(resp.headers['x-account-object-count'], '4')
@@ -211,20 +211,20 @@ def test_HEAD_with_containers(self):
     def test_HEAD_invalid_partition(self):
         req = Request.blank('/sda1/./a', environ={'REQUEST_METHOD': 'HEAD',
                                                   'HTTP_X_TIMESTAMP': '1'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 400)
 
     def test_HEAD_invalid_content_type(self):
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'},
                             headers={'Accept': 'application/plain'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 406)
 
     def test_HEAD_insufficient_storage(self):
         self.controller = AccountController({'devices': self.testdir})
         req = Request.blank('/sda-null/p/a', environ={'REQUEST_METHOD': 'HEAD',
                                                       'HTTP_X_TIMESTAMP': '1'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 507)
 
     def test_HEAD_invalid_format(self):
@@ -349,7 +349,7 @@ def test_POST_HEAD_metadata(self):
         resp = self.controller.POST(req)
         self.assertEquals(resp.status_int, 204)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 204)
         self.assertEquals(resp.headers.get('x-account-meta-test'), 'Value')
         # Update metadata header
@@ -359,7 +359,7 @@ def test_POST_HEAD_metadata(self):
         resp = self.controller.POST(req)
         self.assertEquals(resp.status_int, 204)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 204)
         self.assertEquals(resp.headers.get('x-account-meta-test'), 'New Value')
         # Send old update to metadata header
@@ -369,7 +369,7 @@ def test_POST_HEAD_metadata(self):
         resp = self.controller.POST(req)
         self.assertEquals(resp.status_int, 204)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 204)
         self.assertEquals(resp.headers.get('x-account-meta-test'), 'New Value')
         # Remove metadata header (by setting it to empty)
@@ -379,7 +379,7 @@ def test_POST_HEAD_metadata(self):
         resp = self.controller.POST(req)
         self.assertEquals(resp.status_int, 204)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'HEAD'})
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 204)
         self.assert_('x-account-meta-test' not in resp.headers)
 
@@ -974,14 +974,14 @@ def test_GET_accept_not_valid(self):
         self.controller.PUT(req)
         req = Request.blank('/sda1/p/a', environ={'REQUEST_METHOD': 'GET'})
         req.accept = 'application/xml*'
-        resp = self.controller.GET(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 406)
 
     def test_GET_delimiter_too_long(self):
         req = Request.blank('/sda1/p/a?delimiter=xx',
                             environ={'REQUEST_METHOD': 'GET',
                                      'HTTP_X_TIMESTAMP': '0'})
-        resp = self.controller.GET(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.status_int, 412)
 
     def test_GET_prefix_delimiter_plain(self):
@@ -1306,22 +1306,22 @@ def test_content_type_on_HEAD(self):
         env = {'REQUEST_METHOD': 'HEAD'}
 
         req = Request.blank('/sda1/p/a?format=xml', environ=env)
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.content_type, 'application/xml')
 
         req = Request.blank('/sda1/p/a?format=json', environ=env)
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.content_type, 'application/json')
         self.assertEquals(resp.charset, 'utf-8')
 
         req = Request.blank('/sda1/p/a', environ=env)
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.content_type, 'text/plain')
         self.assertEquals(resp.charset, 'utf-8')
 
         req = Request.blank(
             '/sda1/p/a', headers={'Accept': 'application/json'}, environ=env)
-        resp = self.controller.HEAD(req)
+        resp = req.get_response(self.controller)
         self.assertEquals(resp.content_type, 'application/json')
         self.assertEquals(resp.charset, 'utf-8')