From 14a227f305a9e498fab761672bd8652bba8de5d2 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 29 Jun 2023 15:56:54 -0700 Subject: [PATCH] versioning: Only list versions container if it seems to exist It costs us an extra HEAD sometimes, but those at least get cached some. Seems better than doing real GETs and going out to handoffs every time when versioning isn't enabled. Change-Id: Ic1b3a8c6c9b1aaead25070e49f514785c2d52c98 --- .../versioned_writes/object_versioning.py | 20 ++++-- .../middleware/test_object_versioning.py | 72 +++++++++++++++++++ 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/swift/common/middleware/versioned_writes/object_versioning.py b/swift/common/middleware/versioned_writes/object_versioning.py index 26dcb5c546..34d716964c 100644 --- a/swift/common/middleware/versioned_writes/object_versioning.py +++ b/swift/common/middleware/versioned_writes/object_versioning.py @@ -1165,12 +1165,22 @@ class ContainerContext(ObjectVersioningContext): params['prefix'] = get_reserved_name(params['prefix']) # NB: no end_marker support (yet) - versions_req.params = { - k: params.get(k, '') - for k in ('prefix', 'marker', 'limit', 'delimiter', 'reverse')} - versions_resp = versions_req.get_response(self.app) + if get_container_info(versions_req.environ, self.app, + swift_source='OV')['status'] == 404: + # we don't usually like to LBYL like this, but 404s tend to be + # expensive (since we check all primaries and a bunch of handoffs) + # and we expect this to be a reasonably common way to listing + # objects since it's more complete from the user's perspective + # (see also: s3api and that client ecosystem) + versions_resp = None + else: + versions_req.params = { + k: params.get(k, '') for k in ( + 'prefix', 'marker', 'limit', 'delimiter', 'reverse')} + versions_resp = versions_req.get_response(self.app) - if versions_resp.status_int == HTTP_NOT_FOUND: + if versions_resp is None \ + or versions_resp.status_int == HTTP_NOT_FOUND: subdir_listing = [{'subdir': s} for s in subdir_set] broken_listing = [] for item in current_versions.values(): diff --git a/test/unit/common/middleware/test_object_versioning.py b/test/unit/common/middleware/test_object_versioning.py index 691d93fbce..cd2d27bf04 100644 --- a/test/unit/common/middleware/test_object_versioning.py +++ b/test/unit/common/middleware/test_object_versioning.py @@ -101,6 +101,11 @@ class ObjectVersioningBaseTestCase(unittest.TestCase): self.cache_version_off.set( get_cache_key('a', self.build_container_name('c')), {'status': 200}) + + self.cache_version_never_on = FakeMemcache() + self.cache_version_never_on.set(get_cache_key('a'), {'status': 200}) + self.cache_version_never_on.set(get_cache_key('a', 'c'), + {'status': 200}) self.expected_unread_requests = {} def tearDown(self): @@ -2999,6 +3004,73 @@ class ObjectVersioningTestContainerOperations(ObjectVersioningBaseTestCase): }] self.assertEqual(expected, json.loads(body)) + def test_list_versions_never_enabled(self): + listing_body = [{ + 'bytes': 8, + 'name': 'my-other-object', + 'hash': 'ebdd8d46ecb4a07f6c433d67eb05d5f3', + 'last_modified': '1970-01-01T00:00:05.000000', + 'content_type': 'application/bar', + }, { + 'bytes': 9, + 'name': 'obj', + 'hash': 'e55cedc11adb39c404b7365f7d6291fa', + 'last_modified': '1970-01-01T00:00:20.000000', + 'content_type': 'text/plain', + }] + + self.app.register( + 'GET', self.build_versions_path(), swob.HTTPNotFound, {}, '') + self.app.register( + 'GET', '/v1/a/c', swob.HTTPOk, {}, + json.dumps(listing_body).encode('utf8')) + req = Request.blank( + '/v1/a/c?versions', + environ={'REQUEST_METHOD': 'GET', + 'swift.cache': self.cache_version_never_on}) + status, headers, body = self.call_ov(req) + self.assertEqual(status, '200 OK') + self.assertNotIn('x-versions-enabled', [h.lower() for h, _ in headers]) + self.assertEqual(len(self.authorized), 1) + self.assertRequestEqual(req, self.authorized[0]) + expected = [{ + 'bytes': 8, + 'name': 'my-other-object', + 'version_id': 'null', + 'hash': 'ebdd8d46ecb4a07f6c433d67eb05d5f3', + 'last_modified': '1970-01-01T00:00:05.000000', + 'content_type': 'application/bar', + 'is_latest': True, + }, { + 'bytes': 9, + 'name': 'obj', + 'version_id': 'null', + 'hash': 'e55cedc11adb39c404b7365f7d6291fa', + 'last_modified': '1970-01-01T00:00:20.000000', + 'content_type': 'text/plain', + 'is_latest': True, + }] + self.assertEqual(expected, json.loads(body)) + self.assertEqual(self.app.calls, [ + ('GET', '/v1/a/c?format=json'), + ('HEAD', self.build_versions_path()), + ]) + + # if it's in cache, we won't even get the HEAD + self.app._calls = [] + self.cache_version_never_on.set( + get_cache_key('a', self.build_container_name('c')), + {'status': 404}) + req = Request.blank( + '/v1/a/c?versions', + environ={'REQUEST_METHOD': 'GET', + 'swift.cache': self.cache_version_never_on}) + status, headers, body = self.call_ov(req) + self.assertEqual(status, '200 OK') + self.assertNotIn('x-versions-enabled', [h.lower() for h, _ in headers]) + self.assertEqual(expected, json.loads(body)) + self.assertEqual(self.app.calls, [('GET', '/v1/a/c?format=json')]) + def test_bytes_count(self): self.app.register( 'HEAD', self.build_versions_path(), swob.HTTPOk,