Merge "Put correct Etag and Accept-Ranges in EC 304 and 416 responses" into stable/libertyliberty-eolstable/liberty

5 files changed, 126 insertions, 39 deletions

diff --git a/swift/common/swob.py b/swift/common/swob.py
index fdcbaf3e9..e6b3d4294 100644
--- a/swift/common/swob.py
+++ b/swift/common/swob.py
@@ -1331,7 +1331,7 @@ class Response(object):
         object length and body or app_iter to reset the content_length
         properties on the request.
 
-        It is ok to not call this method, the conditional resposne will be
+        It is ok to not call this method, the conditional response will be
         maintained for you when you __call__ the response.
         """
         self.response_iter = self._response_iter(self.app_iter, self._body)
diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py
index bdb2028f3..e6e3982f8 100644
--- a/swift/proxy/controllers/obj.py
+++ b/swift/proxy/controllers/obj.py
@@ -56,9 +56,10 @@ from swift.common.exceptions import ChunkReadTimeout, \
     PutterConnectError, ChunkReadError
 from swift.common.http import (
     is_informational, is_success, is_client_error, is_server_error,
-    HTTP_CONTINUE, HTTP_CREATED, HTTP_MULTIPLE_CHOICES,
+    is_redirection, HTTP_CONTINUE, HTTP_CREATED, HTTP_MULTIPLE_CHOICES,
     HTTP_INTERNAL_SERVER_ERROR, HTTP_SERVICE_UNAVAILABLE,
-    HTTP_INSUFFICIENT_STORAGE, HTTP_PRECONDITION_FAILED, HTTP_CONFLICT)
+    HTTP_INSUFFICIENT_STORAGE, HTTP_PRECONDITION_FAILED, HTTP_CONFLICT,
+    HTTP_REQUESTED_RANGE_NOT_SATISFIABLE)
 from swift.common.storage_policy import (POLICIES, REPL_POLICY, EC_POLICY,
                                          ECDriverError, PolicyError)
 from swift.proxy.controllers.base import Controller, delay_denial, \
@@ -2040,7 +2041,12 @@ class ECObjectController(BaseObjectController):
                     headers=resp_headers,
                     conditional_response=True,
                     app_iter=app_iter)
-                app_iter.kickoff(req, resp)
+                try:
+                    app_iter.kickoff(req, resp)
+                except HTTPException as err_resp:
+                    # catch any HTTPException response here so that we can
+                    # process response headers uniformly in _fix_response
+                    resp = err_resp
             else:
                 statuses = []
                 reasons = []
@@ -2060,10 +2066,12 @@ class ECObjectController(BaseObjectController):
     def _fix_response(self, resp):
         # EC fragment archives each have different bytes, hence different
         # etags. However, they all have the original object's etag stored in
-        # sysmeta, so we copy that here so the client gets it.
+        # sysmeta, so we copy that here (if it exists) so the client gets it.
+        resp.headers['Etag'] = resp.headers.get('X-Object-Sysmeta-Ec-Etag')
+        if (is_success(resp.status_int) or is_redirection(resp.status_int) or
+                resp.status_int == HTTP_REQUESTED_RANGE_NOT_SATISFIABLE):
+            resp.accept_ranges = 'bytes'
         if is_success(resp.status_int):
-            resp.headers['Etag'] = resp.headers.get(
-                'X-Object-Sysmeta-Ec-Etag')
             resp.headers['Content-Length'] = resp.headers.get(
                 'X-Object-Sysmeta-Ec-Content-Length')
             resp.fix_conditional_response()
diff --git a/test/functional/tests.py b/test/functional/tests.py
index 09b2068ca..9e284a337 100644
--- a/test/functional/tests.py
+++ b/test/functional/tests.py
@@ -81,6 +81,14 @@ class Base(unittest.TestCase):
             'Status returned: %d Expected: %s' %
             (self.env.conn.response.status, status_or_statuses))
 
+    def assert_header(self, header_name, expected_value):
+        try:
+            actual_value = self.env.conn.response.getheader(header_name)
+        except KeyError:
+            self.fail(
+                'Expected header name %r not found in response.' % header_name)
+        self.assertEqual(expected_value, actual_value)
+
 
 class Base2(object):
     def setUp(self):
@@ -1377,32 +1385,35 @@ class TestFile(Base):
                 self.assert_status(416)
             else:
                 self.assertEqual(file_item.read(hdrs=hdrs), data[-i:])
+            self.assert_header('etag', file_item.md5)
+            self.assert_header('accept-ranges', 'bytes')
 
             range_string = 'bytes=%d-' % (i)
             hdrs = {'Range': range_string}
-            self.assertTrue(
-                file_item.read(hdrs=hdrs) == data[i - file_length:],
+            self.assertEqual(
+                file_item.read(hdrs=hdrs), data[i - file_length:],
                 range_string)
 
         range_string = 'bytes=%d-%d' % (file_length + 1000, file_length + 2000)
         hdrs = {'Range': range_string}
         self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
         self.assert_status(416)
+        self.assert_header('etag', file_item.md5)
+        self.assert_header('accept-ranges', 'bytes')
 
         range_string = 'bytes=%d-%d' % (file_length - 1000, file_length + 2000)
         hdrs = {'Range': range_string}
-        self.assertTrue(
-            file_item.read(hdrs=hdrs) == data[-1000:], range_string)
+        self.assertEqual(file_item.read(hdrs=hdrs), data[-1000:], range_string)
 
         hdrs = {'Range': '0-4'}
-        self.assertTrue(file_item.read(hdrs=hdrs) == data, range_string)
+        self.assertEqual(file_item.read(hdrs=hdrs), data, '0-4')
 
         # RFC 2616 14.35.1
         # "If the entity is shorter than the specified suffix-length, the
         # entire entity-body is used."
         range_string = 'bytes=-%d' % (file_length + 10)
         hdrs = {'Range': range_string}
-        self.assertTrue(file_item.read(hdrs=hdrs) == data, range_string)
+        self.assertEqual(file_item.read(hdrs=hdrs), data, range_string)
 
     def testRangedGetsWithLWSinHeader(self):
         # Skip this test until webob 1.2 can tolerate LWS in Range header.
@@ -2031,6 +2042,17 @@ class TestFileComparison(Base):
             hdrs = {'If-Match': 'bogus'}
             self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
             self.assert_status(412)
+            self.assert_header('etag', file_item.md5)
+
+    def testIfMatchMultipleEtags(self):
+        for file_item in self.env.files:
+            hdrs = {'If-Match': '"bogus1", "%s", "bogus2"' % file_item.md5}
+            self.assertTrue(file_item.read(hdrs=hdrs))
+
+            hdrs = {'If-Match': '"bogus1", "bogus2", "bogus3"'}
+            self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
+            self.assert_status(412)
+            self.assert_header('etag', file_item.md5)
 
     def testIfNoneMatch(self):
         for file_item in self.env.files:
@@ -2040,6 +2062,20 @@ class TestFileComparison(Base):
             hdrs = {'If-None-Match': file_item.md5}
             self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
             self.assert_status(304)
+            self.assert_header('etag', file_item.md5)
+            self.assert_header('accept-ranges', 'bytes')
+
+    def testIfNoneMatchMultipleEtags(self):
+        for file_item in self.env.files:
+            hdrs = {'If-None-Match': '"bogus1", "bogus2", "bogus3"'}
+            self.assertTrue(file_item.read(hdrs=hdrs))
+
+            hdrs = {'If-None-Match':
+                    '"bogus1", "bogus2", "%s"' % file_item.md5}
+            self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
+            self.assert_status(304)
+            self.assert_header('etag', file_item.md5)
+            self.assert_header('accept-ranges', 'bytes')
 
     def testIfModifiedSince(self):
         for file_item in self.env.files:
@@ -2050,8 +2086,12 @@ class TestFileComparison(Base):
             hdrs = {'If-Modified-Since': self.env.time_new}
             self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
             self.assert_status(304)
+            self.assert_header('etag', file_item.md5)
+            self.assert_header('accept-ranges', 'bytes')
             self.assertRaises(ResponseError, file_item.info, hdrs=hdrs)
             self.assert_status(304)
+            self.assert_header('etag', file_item.md5)
+            self.assert_header('accept-ranges', 'bytes')
 
     def testIfUnmodifiedSince(self):
         for file_item in self.env.files:
@@ -2062,8 +2102,10 @@ class TestFileComparison(Base):
             hdrs = {'If-Unmodified-Since': self.env.time_old_f2}
             self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
             self.assert_status(412)
+            self.assert_header('etag', file_item.md5)
             self.assertRaises(ResponseError, file_item.info, hdrs=hdrs)
             self.assert_status(412)
+            self.assert_header('etag', file_item.md5)
 
     def testIfMatchAndUnmodified(self):
         for file_item in self.env.files:
@@ -2075,33 +2117,38 @@ class TestFileComparison(Base):
                     'If-Unmodified-Since': self.env.time_new}
             self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
             self.assert_status(412)
+            self.assert_header('etag', file_item.md5)
 
             hdrs = {'If-Match': file_item.md5,
                     'If-Unmodified-Since': self.env.time_old_f3}
             self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
             self.assert_status(412)
+            self.assert_header('etag', file_item.md5)
 
     def testLastModified(self):
         file_name = Utils.create_name()
         content_type = Utils.create_name()
 
-        file = self.env.container.file(file_name)
-        file.content_type = content_type
-        resp = file.write_random_return_resp(self.env.file_size)
+        file_item = self.env.container.file(file_name)
+        file_item.content_type = content_type
+        resp = file_item.write_random_return_resp(self.env.file_size)
         put_last_modified = resp.getheader('last-modified')
+        etag = file_item.md5
 
-        file = self.env.container.file(file_name)
-        info = file.info()
+        file_item = self.env.container.file(file_name)
+        info = file_item.info()
         self.assertIn('last_modified', info)
         last_modified = info['last_modified']
         self.assertEqual(put_last_modified, info['last_modified'])
 
         hdrs = {'If-Modified-Since': last_modified}
-        self.assertRaises(ResponseError, file.read, hdrs=hdrs)
+        self.assertRaises(ResponseError, file_item.read, hdrs=hdrs)
         self.assert_status(304)
+        self.assert_header('etag', etag)
+        self.assert_header('accept-ranges', 'bytes')
 
         hdrs = {'If-Unmodified-Since': last_modified}
-        self.assertTrue(file.read(hdrs=hdrs))
+        self.assertTrue(file_item.read(hdrs=hdrs))
 
 
 class TestFileComparisonUTF8(Base2, TestFileComparison):
diff --git a/test/unit/proxy/controllers/test_obj.py b/test/unit/proxy/controllers/test_obj.py
index da3abd0c8..8141e4634 100755
--- a/test/unit/proxy/controllers/test_obj.py
+++ b/test/unit/proxy/controllers/test_obj.py
@@ -2193,7 +2193,7 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase):
         self.assertEqual(resp.status_int, 201)
 
     def test_GET_with_invalid_ranges(self):
-        # reall body size is segment_size - 10 (just 1 segment)
+        # real body size is segment_size - 10 (just 1 segment)
         segment_size = self.policy.ec_segment_size
         real_body = ('a' * segment_size)[:-10]
 
@@ -2205,7 +2205,7 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase):
                                   segment_size, '%s-' % (segment_size + 10))
 
     def test_COPY_with_invalid_ranges(self):
-        # reall body size is segment_size - 10 (just 1 segment)
+        # real body size is segment_size - 10 (just 1 segment)
         segment_size = self.policy.ec_segment_size
         real_body = ('a' * segment_size)[:-10]
 
@@ -2218,6 +2218,7 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase):
 
     def _test_invalid_ranges(self, method, real_body, segment_size, req_range):
         # make a request with range starts from more than real size.
+        body_etag = md5(real_body).hexdigest()
         req = swift.common.swob.Request.blank(
             '/v1/a/c/o', method=method,
             headers={'Destination': 'c1/o',
@@ -2228,7 +2229,8 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase):
 
         node_fragments = zip(*fragment_payloads)
         self.assertEqual(len(node_fragments), self.replicas())  # sanity
-        headers = {'X-Object-Sysmeta-Ec-Content-Length': str(len(real_body))}
+        headers = {'X-Object-Sysmeta-Ec-Content-Length': str(len(real_body)),
+                   'X-Object-Sysmeta-Ec-Etag': body_etag}
         start = int(req_range.split('-')[0])
         self.assertTrue(start >= 0)  # sanity
         title, exp = swob.RESPONSE_REASONS[416]
@@ -2248,9 +2250,11 @@ class TestECObjController(BaseObjectControllerMixin, unittest.TestCase):
         with set_http_connect(*status_codes, body_iter=body_iter,
                               headers=headers, expect_headers=expect_headers):
             resp = req.get_response(self.app)
-        self.assertEquals(resp.status_int, 416)
-        self.assertEquals(resp.content_length, len(range_not_satisfiable_body))
-        self.assertEquals(resp.body, range_not_satisfiable_body)
+        self.assertEqual(resp.status_int, 416)
+        self.assertEqual(resp.content_length, len(range_not_satisfiable_body))
+        self.assertEqual(resp.body, range_not_satisfiable_body)
+        self.assertEqual(resp.etag, body_etag)
+        self.assertEqual(resp.headers['Accept-Ranges'], 'bytes')
 
 
 if __name__ == '__main__':
diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py
index 94613f6ea..6909660ea 100644
--- a/test/unit/proxy/test_server.py
+++ b/test/unit/proxy/test_server.py
@@ -40,6 +40,7 @@ from swift.obj import diskfile
 import re
 import random
 from collections import defaultdict
+import uuid
 
 import mock
 from eventlet import sleep, spawn, wsgi, listen, Timeout, debug
@@ -2220,9 +2221,10 @@ class TestObjectController(unittest.TestCase):
         self.assertEquals(len(error_lines), 0)  # sanity
         self.assertEquals(len(warn_lines), 0)  # sanity
 
-    @unpatch_policies
-    def test_conditional_GET_ec(self):
-        self.put_container("ec", "ec-con")
+    def _test_conditional_GET(self, policy):
+        container_name = uuid.uuid4().hex
+        object_path = '/v1/a/%s/conditionals' % container_name
+        self.put_container(policy.name, container_name)
 
         obj = 'this object has an etag and is otherwise unimportant'
         etag = md5(obj).hexdigest()
@@ -2232,13 +2234,13 @@ class TestObjectController(unittest.TestCase):
         prosrv = _test_servers[0]
         sock = connect_tcp(('localhost', prolis.getsockname()[1]))
         fd = sock.makefile()
-        fd.write('PUT /v1/a/ec-con/conditionals HTTP/1.1\r\n'
+        fd.write('PUT %s HTTP/1.1\r\n'
                  'Host: localhost\r\n'
                  'Connection: close\r\n'
                  'Content-Length: %d\r\n'
                  'X-Storage-Token: t\r\n'
                  'Content-Type: application/octet-stream\r\n'
-                 '\r\n%s' % (len(obj), obj))
+                 '\r\n%s' % (object_path, len(obj), obj))
         fd.flush()
         headers = readuntil2crlfs(fd)
         exp = 'HTTP/1.1 201'
@@ -2247,56 +2249,80 @@ class TestObjectController(unittest.TestCase):
         for verb, body in (('GET', obj), ('HEAD', '')):
             # If-Match
             req = Request.blank(
-                '/v1/a/ec-con/conditionals',
+                object_path,
                 environ={'REQUEST_METHOD': verb},
                 headers={'If-Match': etag})
             resp = req.get_response(prosrv)
             self.assertEqual(resp.status_int, 200)
             self.assertEqual(resp.body, body)
+            self.assertEqual(etag, resp.headers.get('etag'))
+            self.assertEqual('bytes', resp.headers.get('accept-ranges'))
 
             req = Request.blank(
-                '/v1/a/ec-con/conditionals',
+                object_path,
                 environ={'REQUEST_METHOD': verb},
                 headers={'If-Match': not_etag})
             resp = req.get_response(prosrv)
             self.assertEqual(resp.status_int, 412)
+            self.assertEqual(etag, resp.headers.get('etag'))
 
             req = Request.blank(
-                '/v1/a/ec-con/conditionals',
+                object_path,
                 environ={'REQUEST_METHOD': verb},
                 headers={'If-Match': "*"})
             resp = req.get_response(prosrv)
             self.assertEqual(resp.status_int, 200)
             self.assertEqual(resp.body, body)
+            self.assertEqual(etag, resp.headers.get('etag'))
+            self.assertEqual('bytes', resp.headers.get('accept-ranges'))
 
             # If-None-Match
             req = Request.blank(
-                '/v1/a/ec-con/conditionals',
+                object_path,
                 environ={'REQUEST_METHOD': verb},
                 headers={'If-None-Match': etag})
             resp = req.get_response(prosrv)
             self.assertEqual(resp.status_int, 304)
+            self.assertEqual(etag, resp.headers.get('etag'))
+            self.assertEqual('bytes', resp.headers.get('accept-ranges'))
 
             req = Request.blank(
-                '/v1/a/ec-con/conditionals',
+                object_path,
                 environ={'REQUEST_METHOD': verb},
                 headers={'If-None-Match': not_etag})
             resp = req.get_response(prosrv)
             self.assertEqual(resp.status_int, 200)
             self.assertEqual(resp.body, body)
+            self.assertEqual(etag, resp.headers.get('etag'))
+            self.assertEqual('bytes', resp.headers.get('accept-ranges'))
 
             req = Request.blank(
-                '/v1/a/ec-con/conditionals',
+                object_path,
                 environ={'REQUEST_METHOD': verb},
                 headers={'If-None-Match': "*"})
             resp = req.get_response(prosrv)
             self.assertEqual(resp.status_int, 304)
+            self.assertEqual(etag, resp.headers.get('etag'))
+            self.assertEqual('bytes', resp.headers.get('accept-ranges'))
+
         error_lines = prosrv.logger.get_lines_for_level('error')
         warn_lines = prosrv.logger.get_lines_for_level('warning')
         self.assertEquals(len(error_lines), 0)  # sanity
         self.assertEquals(len(warn_lines), 0)  # sanity
 
     @unpatch_policies
+    def test_conditional_GET_ec(self):
+        policy = POLICIES[3]
+        self.assertEqual('erasure_coding', policy.policy_type)  # sanity
+        self._test_conditional_GET(policy)
+
+    @unpatch_policies
+    def test_conditional_GET_replication(self):
+        policy = POLICIES[0]
+        self.assertEqual('replication', policy.policy_type)  # sanity
+        self._test_conditional_GET(policy)
+
+    @unpatch_policies
     def test_GET_ec_big(self):
         self.put_container("ec", "ec-con")
 
@@ -6347,7 +6373,7 @@ class TestObjectECRangedGET(unittest.TestCase):
             str(s) for s in range(431))
         assert seg_size * 4 > len(cls.obj) > seg_size * 3, \
             "object is wrong number of segments"
-
+        cls.obj_etag = md5(cls.obj).hexdigest()
         cls.tiny_obj = 'tiny, tiny object'
         assert len(cls.tiny_obj) < seg_size, "tiny_obj too large"
 
@@ -6495,9 +6521,11 @@ class TestObjectECRangedGET(unittest.TestCase):
     def test_unsatisfiable(self):
         # Goes just one byte too far off the end of the object, so it's
         # unsatisfiable
-        status, _junk, _junk = self._get_obj(
+        status, headers, _junk = self._get_obj(
             "bytes=%d-%d" % (len(self.obj), len(self.obj) + 100))
         self.assertEqual(status, 416)
+        self.assertEqual(self.obj_etag, headers.get('Etag'))
+        self.assertEqual('bytes', headers.get('Accept-Ranges'))
 
     def test_off_end(self):
         # Ranged GET that's mostly off the end of the object, but overlaps