From 4f24ef87d6c272babe3f98d3f448a400a9ea8b1c Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 27 Feb 2014 22:38:53 -0800 Subject: [PATCH] Make DLO manifest copying work with ?multipart-manifest=get If a client issues a COPY request for a DLO with the query param multipart-manifest=get, then the expectation is a new manifest. However, this wasn't the case; X-Object-Manifest wasn't being set on the new object, so the result was a normal object with the same contents as the old manifest (typically 0 bytes). There was already a mechanism by which middlewares could modify COPY requests; this commit extends that so they can set headers on the new object. Note that this has nothing to do with a "normal" DLO copy, i.e. one without multipart-manifest=get. That one makes a new object that's the concatenation of the segments, and it was working just fine. Change-Id: I1073af9fee6e34ebdfad7b1a89aeb05e4523a151 --- swift/common/middleware/dlo.py | 26 ++++++++---- swift/common/middleware/slo.py | 21 ++++++---- swift/proxy/controllers/obj.py | 35 +++++++++------- test/functional/tests.py | 19 +++++++++ test/unit/common/middleware/test_dlo.py | 56 +++++++++++++++++++------ test/unit/common/middleware/test_slo.py | 36 ++++++++++------ 6 files changed, 134 insertions(+), 59 deletions(-) diff --git a/swift/common/middleware/dlo.py b/swift/common/middleware/dlo.py index 56673af2bb..a08b818160 100644 --- a/swift/common/middleware/dlo.py +++ b/swift/common/middleware/dlo.py @@ -281,8 +281,9 @@ class DynamicLargeObject(object): return self.app(env, start_response) # install our COPY-callback hook - env['swift.copy_response_hook'] = self.copy_response_hook( - env.get('swift.copy_response_hook', lambda req, resp: resp)) + env['swift.copy_hook'] = self.copy_hook( + env.get('swift.copy_hook', + lambda src_req, src_resp, sink_req: src_resp)) if ((req.method == 'GET' or req.method == 'HEAD') and req.params.get('multipart-manifest') != 'get'): @@ -313,14 +314,21 @@ class DynamicLargeObject(object): body=('X-Object-Manifest must be in the ' 'format container/prefix')) - def copy_response_hook(self, inner_hook): + def copy_hook(self, inner_hook): - def dlo_copy_hook(req, resp): - x_o_m = resp.headers.get('X-Object-Manifest') - if (x_o_m and req.params.get('multipart-manifest') != 'get'): - resp = GetContext(self, self.logger).get_or_head_response( - req, x_o_m, resp.headers.items()) - return inner_hook(req, resp) + def dlo_copy_hook(source_req, source_resp, sink_req): + x_o_m = source_resp.headers.get('X-Object-Manifest') + if x_o_m: + if source_req.params.get('multipart-manifest') == 'get': + # To copy the manifest, we let the copy proceed as normal, + # but ensure that X-Object-Manifest is set on the new + # object. + sink_req.headers['X-Object-Manifest'] = x_o_m + else: + ctx = GetContext(self, self.logger) + source_resp = ctx.get_or_head_response( + source_req, x_o_m, source_resp.headers.items()) + return inner_hook(source_req, source_resp, sink_req) return dlo_copy_hook diff --git a/swift/common/middleware/slo.py b/swift/common/middleware/slo.py index 9c45aa0b4a..cf23a25fee 100644 --- a/swift/common/middleware/slo.py +++ b/swift/common/middleware/slo.py @@ -521,14 +521,16 @@ class StaticLargeObject(object): """ return SloGetContext(self).handle_slo_get_or_head(req, start_response) - def copy_response_hook(self, inner_hook): + def copy_hook(self, inner_hook): - def slo_hook(req, resp): - if (config_true_value(resp.headers.get('X-Static-Large-Object')) - and req.params.get('multipart-manifest') != 'get'): - resp = SloGetContext(self).get_or_head_response( - req, resp.headers.items(), resp.app_iter) - return inner_hook(req, resp) + def slo_hook(source_req, source_resp, sink_req): + x_slo = source_resp.headers.get('X-Static-Large-Object') + if (config_true_value(x_slo) + and source_req.params.get('multipart-manifest') != 'get'): + source_resp = SloGetContext(self).get_or_head_response( + source_req, source_resp.headers.items(), + source_resp.app_iter) + return inner_hook(source_req, source_resp, sink_req) return slo_hook @@ -749,8 +751,9 @@ class StaticLargeObject(object): return self.app(env, start_response) # install our COPY-callback hook - env['swift.copy_response_hook'] = self.copy_response_hook( - env.get('swift.copy_response_hook', lambda req, resp: resp)) + env['swift.copy_hook'] = self.copy_hook( + env.get('swift.copy_hook', + lambda src_req, src_resp, sink_req: src_resp)) try: if req.method == 'PUT' and \ diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index aecc752c9b..a1571cdd46 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -566,46 +566,49 @@ class ObjectController(Controller): orig_container_name = self.container_name self.object_name = src_obj_name self.container_name = src_container_name + sink_req = Request.blank(req.path_info, + environ=req.environ, headers=req.headers) + source_resp = self.GET(source_req) # This gives middlewares a way to change the source; for example, # this lets you COPY a SLO manifest and have the new object be the # concatenation of the segments (like what a GET request gives # the client), not a copy of the manifest file. - source_resp = req.environ.get( - 'swift.copy_response_hook', - lambda req, resp: resp)(source_req, self.GET(source_req)) + hook = req.environ.get( + 'swift.copy_hook', + (lambda source_req, source_resp, sink_req: source_resp)) + source_resp = hook(source_req, source_resp, sink_req) + if source_resp.status_int >= HTTP_MULTIPLE_CHOICES: return source_resp self.object_name = orig_obj_name self.container_name = orig_container_name - new_req = Request.blank(req.path_info, - environ=req.environ, headers=req.headers) data_source = iter(source_resp.app_iter) - new_req.content_length = source_resp.content_length - if new_req.content_length is None: + sink_req.content_length = source_resp.content_length + if sink_req.content_length is None: # This indicates a transfer-encoding: chunked source object, # which currently only happens because there are more than # CONTAINER_LISTING_LIMIT segments in a segmented object. In # this case, we're going to refuse to do the server-side copy. return HTTPRequestEntityTooLarge(request=req) - if new_req.content_length > MAX_FILE_SIZE: + if sink_req.content_length > MAX_FILE_SIZE: return HTTPRequestEntityTooLarge(request=req) - new_req.etag = source_resp.etag + sink_req.etag = source_resp.etag # we no longer need the X-Copy-From header - del new_req.headers['X-Copy-From'] + del sink_req.headers['X-Copy-From'] if not content_type_manually_set: - new_req.headers['Content-Type'] = \ + sink_req.headers['Content-Type'] = \ source_resp.headers['Content-Type'] if not config_true_value( - new_req.headers.get('x-fresh-metadata', 'false')): - copy_headers_into(source_resp, new_req) - copy_headers_into(req, new_req) + sink_req.headers.get('x-fresh-metadata', 'false')): + copy_headers_into(source_resp, sink_req) + copy_headers_into(req, sink_req) # copy over x-static-large-object for POSTs and manifest copies if 'X-Static-Large-Object' in source_resp.headers and \ req.params.get('multipart-manifest') == 'get': - new_req.headers['X-Static-Large-Object'] = \ + sink_req.headers['X-Static-Large-Object'] = \ source_resp.headers['X-Static-Large-Object'] - req = new_req + req = sink_req if 'x-delete-at' in req.headers: try: diff --git a/test/functional/tests.py b/test/functional/tests.py index 6e2900cf47..f222d4e9b8 100644 --- a/test/functional/tests.py +++ b/test/functional/tests.py @@ -1645,6 +1645,25 @@ class TestDlo(Base): file_contents, "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeeeffffffffff") + def test_copy_manifest(self): + # Copying the manifest should result in another manifest + try: + man1_item = self.env.container.file('man1') + man1_item.copy(self.env.container.name, "copied-man1", + parms={'multipart-manifest': 'get'}) + + copied = self.env.container.file("copied-man1") + copied_contents = copied.read(parms={'multipart-manifest': 'get'}) + self.assertEqual(copied_contents, "man1-contents") + + copied_contents = copied.read() + self.assertEqual( + copied_contents, + "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeee") + finally: + # try not to leave this around for other tests to stumble over + self.env.container.file("copied-man1").delete() + def test_dlo_if_match_get(self): manifest = self.env.container.file("man1") etag = manifest.info()['etag'] diff --git a/test/unit/common/middleware/test_dlo.py b/test/unit/common/middleware/test_dlo.py index 08db16ebf3..de495f1bc0 100644 --- a/test/unit/common/middleware/test_dlo.py +++ b/test/unit/common/middleware/test_dlo.py @@ -796,7 +796,7 @@ class TestDloCopyHook(DloTestCase): # slip this guy in there to pull out the hook def extract_copy_hook(env, sr): - copy_hook[0] = env.get('swift.copy_response_hook') + copy_hook[0] = env.get('swift.copy_hook') return self.app(env, sr) self.dlo = dlo.filter_factory({})(extract_copy_hook) @@ -809,23 +809,55 @@ class TestDloCopyHook(DloTestCase): self.assertTrue(self.copy_hook is not None) # sanity check def test_copy_hook_passthrough(self): - req = swob.Request.blank('/v1/AUTH_test/c/man') - # no X-Object-Manifest header, so do nothing - resp = swob.Response(request=req, status=200) + source_req = swob.Request.blank( + '/v1/AUTH_test/c/man', + environ={'REQUEST_METHOD': 'GET'}) + sink_req = swob.Request.blank( + '/v1/AUTH_test/c/man', + environ={'REQUEST_METHOD': 'PUT'}) + source_resp = swob.Response(request=source_req, status=200) - modified_resp = self.copy_hook(req, resp) - self.assertTrue(modified_resp is resp) + # no X-Object-Manifest header, so do nothing + modified_resp = self.copy_hook(source_req, source_resp, sink_req) + self.assertTrue(modified_resp is source_resp) def test_copy_hook_manifest(self): - req = swob.Request.blank('/v1/AUTH_test/c/man') - resp = swob.Response(request=req, status=200, - headers={"X-Object-Manifest": "c/o"}, - app_iter=["manifest"]) + source_req = swob.Request.blank( + '/v1/AUTH_test/c/man', + environ={'REQUEST_METHOD': 'GET'}) + sink_req = swob.Request.blank( + '/v1/AUTH_test/c/man', + environ={'REQUEST_METHOD': 'PUT'}) + source_resp = swob.Response( + request=source_req, status=200, + headers={"X-Object-Manifest": "c/o"}, + app_iter=["manifest"]) - modified_resp = self.copy_hook(req, resp) - self.assertTrue(modified_resp is not resp) + # it's a manifest, so copy the segments to make a normal object + modified_resp = self.copy_hook(source_req, source_resp, sink_req) + self.assertTrue(modified_resp is not source_resp) self.assertEqual(modified_resp.etag, hashlib.md5("o1-etago2-etag").hexdigest()) + self.assertEqual(sink_req.headers.get('X-Object-Manifest'), None) + + def test_copy_hook_manifest_with_multipart_manifest_get(self): + source_req = swob.Request.blank( + '/v1/AUTH_test/c/man', + environ={'REQUEST_METHOD': 'GET', + 'QUERY_STRING': 'multipart-manifest=get'}) + sink_req = swob.Request.blank( + '/v1/AUTH_test/c/man', + environ={'REQUEST_METHOD': 'PUT'}) + source_resp = swob.Response( + request=source_req, status=200, + headers={"X-Object-Manifest": "c/o"}, + app_iter=["manifest"]) + + # make sure the sink request (the backend PUT) gets X-Object-Manifest + # on it, but that's all + modified_resp = self.copy_hook(source_req, source_resp, sink_req) + self.assertTrue(modified_resp is source_resp) + self.assertEqual(sink_req.headers.get('X-Object-Manifest'), 'c/o') class TestDloConfiguration(unittest.TestCase): diff --git a/test/unit/common/middleware/test_slo.py b/test/unit/common/middleware/test_slo.py index 942ef764b6..f58f647555 100644 --- a/test/unit/common/middleware/test_slo.py +++ b/test/unit/common/middleware/test_slo.py @@ -1459,7 +1459,7 @@ class TestSloCopyHook(SloTestCase): # slip this guy in there to pull out the hook def extract_copy_hook(env, sr): - copy_hook[0] = env['swift.copy_response_hook'] + copy_hook[0] = env['swift.copy_hook'] return self.app(env, sr) self.slo = slo.filter_factory({})(extract_copy_hook) @@ -1472,23 +1472,33 @@ class TestSloCopyHook(SloTestCase): self.assertTrue(self.copy_hook is not None) # sanity check def test_copy_hook_passthrough(self): - req = Request.blank('/v1/AUTH_test/c/o') + source_req = Request.blank( + '/v1/AUTH_test/c/o', + environ={'REQUEST_METHOD': 'GET'}) + sink_req = Request.blank( + '/v1/AUTH_test/c/o', + environ={'REQUEST_METHOD': 'PUT'}) # no X-Static-Large-Object header, so do nothing - resp = Response(request=req, status=200) + source_resp = Response(request=source_req, status=200) - modified_resp = self.copy_hook(req, resp) - self.assertTrue(modified_resp is resp) + modified_resp = self.copy_hook(source_req, source_resp, sink_req) + self.assertTrue(modified_resp is source_resp) def test_copy_hook_manifest(self): - req = Request.blank('/v1/AUTH_test/c/o') - resp = Response(request=req, status=200, - headers={"X-Static-Large-Object": "true"}, - app_iter=[json.dumps([{'name': '/c/o', - 'hash': 'obj-etag', - 'bytes': '3'}])]) + source_req = Request.blank( + '/v1/AUTH_test/c/o', + environ={'REQUEST_METHOD': 'GET'}) + sink_req = Request.blank( + '/v1/AUTH_test/c/o', + environ={'REQUEST_METHOD': 'PUT'}) + source_resp = Response(request=source_req, status=200, + headers={"X-Static-Large-Object": "true"}, + app_iter=[json.dumps([{'name': '/c/o', + 'hash': 'obj-etag', + 'bytes': '3'}])]) - modified_resp = self.copy_hook(req, resp) - self.assertTrue(modified_resp is not resp) + modified_resp = self.copy_hook(source_req, source_resp, sink_req) + self.assertTrue(modified_resp is not source_resp) self.assertEqual(modified_resp.etag, md5("obj-etag").hexdigest())