Merge "Treat 404s as 204 on object delete in proxy"

swift/proxy/controllers/base.py

@@ -28,6 +28,7 @@ import os
 import time
 import functools
 import inspect
+import operator
 from sys import exc_info
 from swift import gettext_ as _
 from urllib import quote
@@ -1039,7 +1040,7 @@ class Controller(object):
                     {'method': method, 'path': path})
 
     def make_requests(self, req, ring, part, method, path, headers,
-                      query_string=''):
+                      query_string='', overrides=None):
         """
         Sends an HTTP request to multiple nodes and aggregates the results.
         It attempts the primary nodes concurrently, then iterates over the
@@ -1054,6 +1055,8 @@ class Controller(object):
         :param headers: a list of dicts, where each dict represents one
                         backend request that should be made.
         :param query_string: optional query string to send to the backend
+        :param overrides: optional return status override map used to override
+                          the returned status of a request.
         :returns: a swob.Response object
         """
         start_nodes = ring.get_part_nodes(part)
@@ -1083,7 +1086,7 @@ class Controller(object):
         statuses, reasons, resp_headers, bodies = zip(*response)
         return self.best_response(req, statuses, reasons, bodies,
                                   '%s %s' % (self.server_type, req.method),
-                                  headers=resp_headers)
+                                  overrides=overrides, headers=resp_headers)
 
     def have_quorum(self, statuses, node_count):
         """
@@ -1103,7 +1106,7 @@ class Controller(object):
         return False
 
     def best_response(self, req, statuses, reasons, bodies, server_type,
-                      etag=None, headers=None):
+                      etag=None, headers=None, overrides=None):
         """
         Given a list of responses from several servers, choose the best to
         return to the API.
@@ -1117,14 +1120,49 @@ class Controller(object):
         :param headers: headers of each response
         :returns: swob.Response object with the correct status, body, etc. set
         """
+        resp = self._compute_quorum_response(
+            req, statuses, reasons, bodies, etag, headers)
+        if overrides and not resp:
+            faked_up_status_indices = set()
+            transformed = []
+            for (i, (status, reason, hdrs, body)) in enumerate(zip(
+                    statuses, reasons, headers, bodies)):
+                if status in overrides:
+                    faked_up_status_indices.add(i)
+                    transformed.append((overrides[status], '', '', ''))
+                else:
+                    transformed.append((status, reason, hdrs, body))
+            statuses, reasons, headers, bodies = zip(*transformed)
+            resp = self._compute_quorum_response(
+                req, statuses, reasons, bodies, etag, headers,
+                indices_to_avoid=faked_up_status_indices)
+
+        if not resp:
             resp = Response(request=req)
-        if len(statuses):
+            self.app.logger.error(_('%(type)s returning 503 for %(statuses)s'),
+                                  {'type': server_type, 'statuses': statuses})
+            resp.status = '503 Internal Server Error'
+
+        return resp
+
+    def _compute_quorum_response(self, req, statuses, reasons, bodies, etag,
+                                 headers, indices_to_avoid=()):
+        if not statuses:
+            return None
         for hundred in (HTTP_OK, HTTP_MULTIPLE_CHOICES, HTTP_BAD_REQUEST):
             hstatuses = \
-                    [s for s in statuses if hundred <= s < hundred + 100]
+                [(i, s) for i, s in enumerate(statuses)
+                 if hundred <= s < hundred + 100]
             if len(hstatuses) >= quorum_size(len(statuses)):
-                    status = max(hstatuses)
+                resp = Response(request=req)
-                    status_index = statuses.index(status)
+                try:
+                    status_index, status = max(
+                        ((i, stat) for i, stat in hstatuses
+                            if i not in indices_to_avoid),
+                        key=operator.itemgetter(1))
+                except ValueError:
+                    # All statuses were indices to avoid
+                    continue
                 resp.status = '%s %s' % (status, reasons[status_index])
                 resp.body = bodies[status_index]
                 if headers:
@@ -1132,10 +1170,7 @@ class Controller(object):
                 if etag:
                     resp.headers['etag'] = etag.strip('"')
                 return resp
-        self.app.logger.error(_('%(type)s returning 503 for %(statuses)s'),
+        return None
-                              {'type': server_type, 'statuses': statuses})
-        resp.status = '503 Internal Server Error'
-        return resp
 
     @public
     def GET(self, req):

swift/proxy/controllers/obj.py

@@ -867,9 +867,11 @@ class ObjectController(Controller):
 
         headers = self._backend_requests(
             req, len(nodes), container_partition, containers)
+        # When deleting objects treat a 404 status as 204.
+        status_overrides = {404: 204}
         resp = self.make_requests(req, obj_ring,
                                   partition, 'DELETE', req.swift_entity_path,
-                                  headers)
+                                  headers, overrides=status_overrides)
         return resp
 
     @public

test/unit/proxy/controllers/test_base.py

@@ -550,6 +550,32 @@ class TestFuncs(unittest.TestCase):
         self.assertEqual(base.have_quorum([404, 404], 2), True)
         self.assertEqual(base.have_quorum([201, 404, 201, 201], 4), True)
 
+    def test_best_response_overrides(self):
+        base = Controller(self.app)
+        responses = [
+            (302, 'Found', '', 'The resource has moved temporarily.'),
+            (100, 'Continue', '', ''),
+            (404, 'Not Found', '', 'Custom body'),
+        ]
+        server_type = "Base DELETE"
+        req = Request.blank('/v1/a/c/o', method='DELETE')
+        statuses, reasons, headers, bodies = zip(*responses)
+
+        # First test that you can't make a quorum with only overridden
+        # responses
+        overrides = {302: 204, 100: 204}
+        resp = base.best_response(req, statuses, reasons, bodies, server_type,
+                                  headers=headers, overrides=overrides)
+        self.assertEqual(resp.status, '503 Internal Server Error')
+
+        # next make a 404 quorum and make sure the last delete (real) 404
+        # status is the one returned.
+        overrides = {100: 404}
+        resp = base.best_response(req, statuses, reasons, bodies, server_type,
+                                  headers=headers, overrides=overrides)
+        self.assertEqual(resp.status, '404 Not Found')
+        self.assertEqual(resp.body, 'Custom body')
+
     def test_range_fast_forward(self):
         req = Request.blank('/')
         handler = GetOrHeadHandler(None, req, None, None, None, None, {})

test/unit/proxy/controllers/test_obj.py

@@ -226,6 +226,31 @@ class TestObjController(unittest.TestCase):
             resp = req.get_response(self.app)
         self.assertEquals(resp.status_int, 204)
 
+    def test_DELETE_half_not_found_statuses(self):
+        self.obj_ring.set_replicas(4)
+
+        req = swift.common.swob.Request.blank('/v1/a/c/o', method='DELETE')
+        with set_http_connect(404, 204, 404, 204):
+            resp = req.get_response(self.app)
+        self.assertEquals(resp.status_int, 204)
+
+    def test_DELETE_half_not_found_headers_and_body(self):
+        # Transformed responses have bogus bodies and headers, so make sure we
+        # send the client headers and body from a real node's response.
+        self.obj_ring.set_replicas(4)
+
+        status_codes = (404, 404, 204, 204)
+        bodies = ('not found', 'not found', '', '')
+        headers = [{}, {}, {'Pick-Me': 'yes'}, {'Pick-Me': 'yes'}]
+
+        req = swift.common.swob.Request.blank('/v1/a/c/o', method='DELETE')
+        with set_http_connect(*status_codes, body_iter=bodies,
+                              headers=headers):
+            resp = req.get_response(self.app)
+        self.assertEquals(resp.status_int, 204)
+        self.assertEquals(resp.headers.get('Pick-Me'), 'yes')
+        self.assertEquals(resp.body, '')
+
     def test_DELETE_not_found(self):
         req = swift.common.swob.Request.blank('/v1/a/c/o', method='DELETE')
         with set_http_connect(404, 404, 204):

test/unit/proxy/test_server.py

@@ -1899,9 +1899,9 @@ class TestObjectController(unittest.TestCase):
             test_status_map((200, 200, 204, 204, 204), 204)
             test_status_map((200, 200, 204, 204, 500), 204)
             test_status_map((200, 200, 204, 404, 404), 404)
-            test_status_map((200, 200, 204, 500, 404), 503)
+            test_status_map((200, 204, 500, 500, 404), 503)
             test_status_map((200, 200, 404, 404, 404), 404)
-            test_status_map((200, 200, 404, 404, 500), 404)
+            test_status_map((200, 200, 400, 400, 400), 400)
 
     def test_HEAD(self):
         with save_globals():