From 61401907911d09fc1d2232118717b048e77a6649 Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Mon, 6 Jan 2020 16:27:06 -0600 Subject: [PATCH] Make FakeSwift query param order agnostic Apparently FakeSwift has always been persnickety about the order of query params of registered responses and the app making the calls. Since query params can often be converted to dictionaries the order of the encoded params should be able to change without effecting the apps dispatching of registered responses. Change-Id: Ied68c9334201a7663e9c85f3bdaa5b0643d4b6db --- test/unit/common/middleware/helpers.py | 43 ++++++--- .../middleware/test_versioned_writes.py | 95 ++++++++++++------- test/unit/container/test_reconciler.py | 17 ++-- 3 files changed, 100 insertions(+), 55 deletions(-) diff --git a/test/unit/common/middleware/helpers.py b/test/unit/common/middleware/helpers.py index e996a07ee2..2b45055a53 100644 --- a/test/unit/common/middleware/helpers.py +++ b/test/unit/common/middleware/helpers.py @@ -17,6 +17,7 @@ from collections import defaultdict, namedtuple from hashlib import md5 +from six.moves.urllib import parse from swift.common import swob from swift.common.header_key_dict import HeaderKeyDict from swift.common.request_helpers import is_user_meta, \ @@ -28,24 +29,38 @@ from test.unit import FakeLogger, FakeRing class LeakTrackingIter(object): - def __init__(self, inner_iter, mark_closed, path): + def __init__(self, inner_iter, mark_closed, key): if isinstance(inner_iter, bytes): inner_iter = (inner_iter, ) self.inner_iter = inner_iter self.mark_closed = mark_closed - self.path = path + self.key = key def __iter__(self): for x in self.inner_iter: yield x def close(self): - self.mark_closed(self.path) + self.mark_closed(self.key) FakeSwiftCall = namedtuple('FakeSwiftCall', ['method', 'path', 'headers']) +def normalize_query_string(qs): + if qs.startswith('?'): + qs = qs[1:] + if not qs: + return '' + else: + return '?%s' % parse.urlencode(sorted(parse.parse_qsl(qs))) + + +def normalize_path(path): + parsed = parse.urlparse(path) + return parsed.path + normalize_query_string(parsed.query) + + class FakeSwift(object): """ A good-enough fake Swift proxy server to use in testing middleware. @@ -55,7 +70,7 @@ class FakeSwift(object): def __init__(self): self._calls = [] - self._unclosed_req_paths = defaultdict(int) + self._unclosed_req_keys = defaultdict(int) self.req_method_paths = [] self.swift_sources = [] self.txn_ids = [] @@ -68,6 +83,7 @@ class FakeSwift(object): self.get_object_ring = lambda policy_index: FakeRing() def _find_response(self, method, path): + path = normalize_path(path) resp = self._responses[(method, path)] if isinstance(resp, list): try: @@ -88,6 +104,7 @@ class FakeSwift(object): rest_with_last=True) if env.get('QUERY_STRING'): path += '?' + env['QUERY_STRING'] + path = normalize_path(path) if 'swift.authorize' in env: resp = env['swift.authorize'](swob.Request(env)) @@ -171,19 +188,19 @@ class FakeSwift(object): conditional_response=req.method in ('GET', 'HEAD'), conditional_etag=conditional_etag) wsgi_iter = resp(env, start_response) - self.mark_opened(path) - return LeakTrackingIter(wsgi_iter, self.mark_closed, path) + self.mark_opened((method, path)) + return LeakTrackingIter(wsgi_iter, self.mark_closed, (method, path)) - def mark_opened(self, path): - self._unclosed_req_paths[path] += 1 + def mark_opened(self, key): + self._unclosed_req_keys[key] += 1 - def mark_closed(self, path): - self._unclosed_req_paths[path] -= 1 + def mark_closed(self, key): + self._unclosed_req_keys[key] -= 1 @property def unclosed_requests(self): - return {path: count - for path, count in self._unclosed_req_paths.items() + return {key: count + for key, count in self._unclosed_req_keys.items() if count > 0} @property @@ -203,9 +220,11 @@ class FakeSwift(object): return len(self._calls) def register(self, method, path, response_class, headers, body=b''): + path = normalize_path(path) self._responses[(method, path)] = (response_class, headers, body) def register_responses(self, method, path, responses): + path = normalize_path(path) self._responses[(method, path)] = list(responses) diff --git a/test/unit/common/middleware/test_versioned_writes.py b/test/unit/common/middleware/test_versioned_writes.py index 55f54fe982..e4e7d4da6f 100644 --- a/test/unit/common/middleware/test_versioned_writes.py +++ b/test/unit/common/middleware/test_versioned_writes.py @@ -22,7 +22,7 @@ import unittest from swift.common import swob, utils from swift.common.middleware import versioned_writes, copy from swift.common.swob import Request -from test.unit.common.middleware.helpers import FakeSwift +from test.unit.common.middleware import helpers class FakeCache(object): @@ -58,7 +58,7 @@ def local_tz(func): class VersionedWritesBaseTestCase(unittest.TestCase): def setUp(self): - self.app = FakeSwift() + self.app = helpers.FakeSwift() conf = {'allow_versioned_writes': 'true'} self.vw = versioned_writes.filter_factory(conf)(self.app) @@ -590,8 +590,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): self.app.register( 'DELETE', '/v1/a/c/o', swob.HTTPOk, {}, 'passed') self.app.register( - 'GET', - '/v1/a/ver_cont?prefix=001o/&marker=&reverse=on', + 'GET', helpers.normalize_path( + '/v1/a/ver_cont?prefix=001o/&marker=&reverse=on'), swob.HTTPNotFound, {}, None) cache = FakeCache({'sysmeta': {'versions-location': 'ver_cont'}}) @@ -609,7 +609,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ('DELETE', '/v1/a/c/o'), ]) @@ -617,8 +618,7 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): self.app.register( 'DELETE', '/v1/a/c/o', swob.HTTPOk, {}, 'passed') self.app.register( - 'GET', - '/v1/a/ver_cont?prefix=001o/&marker=&reverse=on', + 'GET', '/v1/a/ver_cont?prefix=001o/&marker=&reverse=on', swob.HTTPOk, {}, '[]') cache = FakeCache({'sysmeta': {'versions-location': 'ver_cont'}}) @@ -633,7 +633,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ('DELETE', '/v1/a/c/o'), ]) @@ -681,7 +682,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ('GET', '/v1/a/ver_cont/001o/2?symlink=get'), ('PUT', '/v1/a/c/o'), ('DELETE', '/v1/a/ver_cont/001o/2'), @@ -738,7 +740,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): # in the base versioned container. self.app.register( 'GET', - '/v1/a/ver_cont?prefix=001o/&marker=&reverse=on', + helpers.normalize_path( + '/v1/a/ver_cont?prefix=001o/&marker=&reverse=on'), swob.HTTPOk, {}, '[{"hash": "y", ' '"last_modified": "2014-11-21T14:23:02.206740", ' @@ -775,7 +778,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ('HEAD', '/v1/a/c/o'), ('GET', '/v1/a/ver_cont/001o/1?symlink=get'), ('PUT', '/v1/a/c/o'), @@ -940,7 +944,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ('GET', '/v1/a/ver_cont/001o/1?symlink=get'), ('PUT', '/v1/a/c/o'), ('DELETE', '/v1/a/ver_cont/001o/1'), @@ -988,11 +993,14 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), - ('GET', '/v1/a/ver_cont/001o/2?symlink=get'), - ('GET', '/v1/a/ver_cont/001o/1?symlink=get'), - ('PUT', '/v1/a/c/o'), - ('DELETE', '/v1/a/ver_cont/001o/1'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), + ('GET', helpers.normalize_path( + '/v1/a/ver_cont/001o/2?symlink=get')), + ('GET', helpers.normalize_path( + '/v1/a/ver_cont/001o/1?symlink=get')), + ('PUT', helpers.normalize_path('/v1/a/c/o')), + ('DELETE', helpers.normalize_path('/v1/a/ver_cont/001o/1')), ]) def test_denied_DELETE_of_versioned_object(self): @@ -1030,7 +1038,8 @@ class VersionedWritesTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ]) def test_denied_PUT_of_versioned_object(self): @@ -1112,8 +1121,10 @@ class VersionedWritesOldContainersTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), - ('GET', prefix_listing_prefix + 'marker=001o/2'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/2')), ('GET', '/v1/a/ver_cont/001o/2?symlink=get'), ('PUT', '/v1/a/c/o'), ('DELETE', '/v1/a/ver_cont/001o/2'), @@ -1165,8 +1176,10 @@ class VersionedWritesOldContainersTestCase(VersionedWritesBaseTestCase): prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), - ('GET', prefix_listing_prefix + 'marker=001o/2'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/2')), ('GET', '/v1/a/ver_cont/001o/2?symlink=get'), ('GET', '/v1/a/ver_cont/001o/1?symlink=get'), ('PUT', '/v1/a/c/o'), @@ -1215,8 +1228,10 @@ class VersionedWritesOldContainersTestCase(VersionedWritesBaseTestCase): self.assertRequestEqual(req, authorize_call[0]) prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), - ('GET', prefix_listing_prefix + 'marker=001o/2'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/2')), ]) def test_partially_upgraded_cluster(self): @@ -1281,14 +1296,19 @@ class VersionedWritesOldContainersTestCase(VersionedWritesBaseTestCase): self.assertEqual(status, '204 No Content') prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ('GET', '/v1/a/ver_cont/001o/4?symlink=get'), ('GET', '/v1/a/ver_cont/001o/3?symlink=get'), ('GET', '/v1/a/ver_cont/001o/2?symlink=get'), - ('GET', prefix_listing_prefix + 'marker=001o/2&reverse=on'), - ('GET', prefix_listing_prefix + 'marker=&end_marker=001o/2'), - ('GET', prefix_listing_prefix + 'marker=001o/0&end_marker=001o/2'), - ('GET', prefix_listing_prefix + 'marker=001o/1&end_marker=001o/2'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/2&reverse=on')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&end_marker=001o/2')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/0&end_marker=001o/2')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/1&end_marker=001o/2')), ('GET', '/v1/a/ver_cont/001o/1?symlink=get'), ('PUT', '/v1/a/c/o'), ('DELETE', '/v1/a/ver_cont/001o/1'), @@ -1353,13 +1373,18 @@ class VersionedWritesOldContainersTestCase(VersionedWritesBaseTestCase): self.assertEqual(status, '204 No Content') prefix_listing_prefix = '/v1/a/ver_cont?prefix=001o/&' self.assertEqual(self.app.calls, [ - ('GET', prefix_listing_prefix + 'marker=&reverse=on'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&reverse=on')), ('GET', '/v1/a/ver_cont/001o/4?symlink=get'), ('GET', '/v1/a/ver_cont/001o/3?symlink=get'), - ('GET', prefix_listing_prefix + 'marker=001o/3&reverse=on'), - ('GET', prefix_listing_prefix + 'marker=&end_marker=001o/3'), - ('GET', prefix_listing_prefix + 'marker=001o/1&end_marker=001o/3'), - ('GET', prefix_listing_prefix + 'marker=001o/2&end_marker=001o/3'), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/3&reverse=on')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=&end_marker=001o/3')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/1&end_marker=001o/3')), + ('GET', helpers.normalize_path( + prefix_listing_prefix + 'marker=001o/2&end_marker=001o/3')), ('GET', '/v1/a/ver_cont/001o/2?symlink=get'), ('PUT', '/v1/a/c/o'), ('DELETE', '/v1/a/ver_cont/001o/2'), @@ -1370,7 +1395,7 @@ class VersionedWritesCopyingTestCase(VersionedWritesBaseTestCase): # verify interaction of copy and versioned_writes middlewares def setUp(self): - self.app = FakeSwift() + self.app = helpers.FakeSwift() conf = {'allow_versioned_writes': 'true'} self.vw = versioned_writes.filter_factory(conf)(self.app) self.filter = copy.filter_factory({})(self.vw) diff --git a/test/unit/container/test_reconciler.py b/test/unit/container/test_reconciler.py index 248a383163..fc227b4b87 100644 --- a/test/unit/container/test_reconciler.py +++ b/test/unit/container/test_reconciler.py @@ -36,7 +36,7 @@ from swift.common.header_key_dict import HeaderKeyDict from swift.common.utils import split_path, Timestamp, encode_timestamps from test.unit import debug_logger, FakeRing, fake_http_connect -from test.unit.common.middleware.helpers import FakeSwift +from test.unit.common.middleware import helpers def timestamp_to_last_modified(timestamp): @@ -51,7 +51,7 @@ def container_resp_headers(**kwargs): class FakeStoragePolicySwift(object): def __init__(self): - self.storage_policy = defaultdict(FakeSwift) + self.storage_policy = defaultdict(helpers.FakeSwift) self._mock_oldest_spi_map = {} def __getattribute__(self, name): @@ -158,17 +158,17 @@ class FakeInternalClient(reconciler.InternalClient): container_listing_data.sort(key=operator.itemgetter('name')) # register container listing response container_headers = {} - container_qry_string = \ - '?format=json&marker=&end_marker=&prefix=' + container_qry_string = helpers.normalize_query_string( + '?format=json&marker=&end_marker=&prefix=') self.app.register('GET', container_path + container_qry_string, swob.HTTPOk, container_headers, json.dumps(container_listing_data)) if container_listing_data: obj_name = container_listing_data[-1]['name'] # client should quote and encode marker - end_qry_string = \ + end_qry_string = helpers.normalize_query_string( '?format=json&marker=%s&end_marker=&prefix=' % ( - urllib.parse.quote(obj_name.encode('utf-8'))) + urllib.parse.quote(obj_name.encode('utf-8')))) self.app.register('GET', container_path + end_qry_string, swob.HTTPOk, container_headers, json.dumps([])) @@ -713,8 +713,9 @@ class TestReconcilerUtils(unittest.TestCase): def listing_qs(marker): - return "?format=json&marker=%s&end_marker=&prefix=" % \ - urllib.parse.quote(marker.encode('utf-8')) + return helpers.normalize_query_string( + "?format=json&marker=%s&end_marker=&prefix=" % + urllib.parse.quote(marker.encode('utf-8'))) class TestReconciler(unittest.TestCase):