Apply limit to list versioned containers

Change-Id: I28e062273d673c4f07cd3c5da088aa790b77a599
Closes-Bug: #1863841

swift/account/server.py

@@ -27,7 +27,7 @@ from swift.account.utils import account_listing_response, get_response_headers
 from swift.common.db import DatabaseConnectionError, DatabaseAlreadyExists
 from swift.common.request_helpers import get_param, \
     split_and_validate_path, validate_internal_account, \
-    validate_internal_container
+    validate_internal_container, constrain_req_limit
 from swift.common.utils import get_logger, hash_path, public, \
     Timestamp, storage_directory, config_true_value, \
     timing_stats, replication, get_log_line, \
@@ -247,16 +247,8 @@ class AccountController(BaseStorageServer):
         drive, part, account = get_account_name_and_placement(req)
         prefix = get_param(req, 'prefix')
         delimiter = get_param(req, 'delimiter')
-        limit = constraints.ACCOUNT_LISTING_LIMIT
-        given_limit = get_param(req, 'limit')
         reverse = config_true_value(get_param(req, 'reverse'))
-        if given_limit and given_limit.isdigit():
+        limit = constrain_req_limit(req, constraints.ACCOUNT_LISTING_LIMIT)
-            limit = int(given_limit)
-            if limit > constraints.ACCOUNT_LISTING_LIMIT:
-                return HTTPPreconditionFailed(
-                    request=req,
-                    body='Maximum limit is %d' %
-                    constraints.ACCOUNT_LISTING_LIMIT)
         marker = get_param(req, 'marker', '')
         end_marker = get_param(req, 'end_marker')
         out_content_type = listing_formats.get_listing_content_type(req)

swift/common/middleware/versioned_writes/object_versioning.py

@@ -151,11 +151,13 @@ import time
 from cgi import parse_header
 from six.moves.urllib.parse import unquote
 
-from swift.common.constraints import MAX_FILE_SIZE, valid_api_version
+from swift.common.constraints import MAX_FILE_SIZE, valid_api_version, \
+    ACCOUNT_LISTING_LIMIT
 from swift.common.http import is_success, is_client_error, HTTP_NOT_FOUND, \
     HTTP_CONFLICT
 from swift.common.request_helpers import get_sys_meta_prefix, \
-    copy_header_subset, get_reserved_name, split_reserved_name
+    copy_header_subset, get_reserved_name, split_reserved_name, \
+    constrain_req_limit
 from swift.common.middleware.symlink import TGT_OBJ_SYMLINK_HDR, \
     TGT_ETAG_SYSMETA_SYMLINK_HDR, SYMLOOP_EXTEND, ALLOW_RESERVED_NAMES, \
     TGT_BYTES_SYSMETA_SYMLINK_HDR, TGT_ACCT_SYMLINK_HDR
@@ -183,6 +185,7 @@ SYSMETA_VERSIONS_SYMLINK = get_sys_meta_prefix('object') + 'versions-symlink'
 
 def build_listing(*to_splice, **kwargs):
     reverse = kwargs.pop('reverse')
+    limit = kwargs.pop('limit')
     if kwargs:
         raise TypeError('Invalid keyword arguments received: %r' % kwargs)
 
@@ -195,7 +198,7 @@ def build_listing(*to_splice, **kwargs):
         itertools.chain(*to_splice),
         key=merge_key,
         reverse=reverse,
-    )).encode('ascii')
+    )[:limit]).encode('ascii')
 
 
 def non_expiry_header(header):
@@ -1197,9 +1200,11 @@ class ContainerContext(ObjectVersioningContext):
                     'hash': item['hash'],
                     'last_modified': item['last_modified'],
                 })
+            limit = constrain_req_limit(req, ACCOUNT_LISTING_LIMIT)
             body = build_listing(
                 null_listing, subdir_listing, broken_listing,
-                reverse=config_true_value(params.get('reverse', 'no')))
+                reverse=config_true_value(params.get('reverse', 'no')),
+                limit=limit)
             self.update_content_length(len(body))
             app_resp = [body]
             close_if_possible(versions_resp.app_iter)
@@ -1260,10 +1265,13 @@ class ContainerContext(ObjectVersioningContext):
                         'last_modified': item['last_modified'],
                     })
 
+                limit = constrain_req_limit(req, ACCOUNT_LISTING_LIMIT)
                 body = build_listing(
                     null_listing, versions_listing,
                     subdir_listing, broken_listing,
-                    reverse=config_true_value(params.get('reverse', 'no')))
+                    reverse=config_true_value(params.get('reverse', 'no')),
+                    limit=limit,
+                )
                 self.update_content_length(len(body))
                 app_resp = [body]
         else:
@@ -1333,9 +1341,8 @@ class AccountContext(ObjectVersioningContext):
                 # look-up by name. Ignore 'subdir' items
                 for item in [item for item in versions_listing
                              if 'name' in item]:
-                    name = self._split_versions_container_name(
+                    container_name = self._split_versions_container_name(
                         item['name'])
-                    container_name = bytes_to_wsgi(name.encode('utf-8'))
                     versions_dict[container_name] = item
 
                 # update bytes from original listing with bytes from
@@ -1343,10 +1350,8 @@ class AccountContext(ObjectVersioningContext):
                 if len(versions_dict) > 0:
                     # ignore 'subdir' items
                     for item in [item for item in listing if 'name' in item]:
-                        container_name = bytes_to_wsgi(
+                        if item['name'] in versions_dict:
-                            item['name'].encode('utf-8'))
+                            v_info = versions_dict.pop(item['name'])
-                        if container_name in versions_dict:
-                            v_info = versions_dict.pop(container_name)
                             item['bytes'] = item['bytes'] + v_info['bytes']
 
                 # if there are items left in versions_dict, it indicates an
@@ -1360,9 +1365,12 @@ class AccountContext(ObjectVersioningContext):
                     item['count'] = 0  # None of these are current
                     listing.append(item)
 
+                limit = constrain_req_limit(req, ACCOUNT_LISTING_LIMIT)
                 body = build_listing(
                     listing,
-                    reverse=config_true_value(params.get('reverse', 'no')))
+                    reverse=config_true_value(params.get('reverse', 'no')),
+                    limit=limit,
+                )
                 self.update_content_length(len(body))
                 app_resp = [body]
 

swift/common/request_helpers.py

@@ -94,6 +94,17 @@ def get_param(req, name, default=None):
     return value
 
 
+def constrain_req_limit(req, constrained_limit):
+    given_limit = get_param(req, 'limit')
+    limit = constrained_limit
+    if given_limit and given_limit.isdigit():
+        limit = int(given_limit)
+        if limit > constrained_limit:
+            raise HTTPPreconditionFailed(
+                request=req, body='Maximum limit is %d' % constrained_limit)
+    return limit
+
+
 def _validate_internal_name(name, type_='name'):
     if RESERVED in name and not name.startswith(RESERVED):
         raise HTTPBadRequest(body='Invalid reserved-namespace %s' % (type_))

swift/container/server.py

@@ -34,7 +34,7 @@ from swift.common.db import DatabaseAlreadyExists
 from swift.common.container_sync_realms import ContainerSyncRealms
 from swift.common.request_helpers import get_param, \
     split_and_validate_path, is_sys_or_user_meta, \
-    validate_internal_container, validate_internal_obj
+    validate_internal_container, validate_internal_obj, constrain_req_limit
 from swift.common.utils import get_logger, hash_path, public, \
     Timestamp, storage_directory, validate_sync_to, \
     config_true_value, timing_stats, replication, \
@@ -689,16 +689,8 @@ class ContainerController(BaseStorageServer):
         delimiter = get_param(req, 'delimiter')
         marker = get_param(req, 'marker', '')
         end_marker = get_param(req, 'end_marker')
-        limit = constraints.CONTAINER_LISTING_LIMIT
+        limit = constrain_req_limit(req, constraints.CONTAINER_LISTING_LIMIT)
-        given_limit = get_param(req, 'limit')
         reverse = config_true_value(get_param(req, 'reverse'))
-        if given_limit and given_limit.isdigit():
-            limit = int(given_limit)
-            if limit > constraints.CONTAINER_LISTING_LIMIT:
-                return HTTPPreconditionFailed(
-                    request=req,
-                    body='Maximum limit is %d'
-                    % constraints.CONTAINER_LISTING_LIMIT)
         out_content_type = listing_formats.get_listing_content_type(req)
         try:
             check_drive(self.root, drive, self.mount_check)

test/functional/test_object_versioning.py

@@ -209,6 +209,40 @@ class TestObjectVersioning(TestObjectVersioningBase):
         self.assertTrue(
             config_true_value(self.env.container.info()['versions_enabled']))
 
+    def test_account_list_containers(self):
+        cont_listing = self.env.account.containers()
+        self.assertEqual(cont_listing, [self.env.container.name,
+                                        self.env.unversioned_container.name])
+        self.env.account.delete_containers()
+        prefix = Utils.create_name()
+
+        def get_name(i):
+            return prefix + '-%02d' % i
+
+        num_container = [15, 20]
+        for i in range(num_container[1]):
+            name = get_name(i)
+            container = self.env.account.container(name)
+            container.create()
+
+        limit = 5
+        cont_listing = self.env.account.containers(parms={'limit': limit})
+        self.assertEqual(cont_listing, [get_name(i) for i in range(limit)])
+
+        for i in range(num_container[0], num_container[1]):
+            name = get_name(i)
+            container = self.env.account.container(name)
+            container.update_metadata(
+                hdrs={self.env.versions_header_key: 'True'})
+
+        cont_listing = self.env.account.containers(parms={'limit': limit})
+        self.assertEqual(cont_listing, [get_name(i) for i in range(limit)])
+
+        # we're in charge of getting everything back to normal
+        self.env.account.delete_containers()
+        self.env.container.create()
+        self.env.unversioned_container.create()
+
     def assert_previous_version(self, object_name, version_id, content,
                                 content_type, expected_headers={},
                                 not_expected_header_keys=[],

test/unit/common/middleware/test_object_versioning.py

@@ -3067,6 +3067,20 @@ class ObjectVersioningTestAccountOperations(ObjectVersioningBaseTestCase):
         }]
         self.assertEqual(expected, json.loads(body))
 
+        req.query_string = 'limit=1'
+        status, headers, body = self.call_ov(req)
+        self.assertEqual(status, '200 OK')
+        self.assertEqual(1, len(json.loads(body)))
+
+        req.query_string = 'limit=foo'
+        status, headers, body = self.call_ov(req)
+        self.assertEqual(status, '200 OK')
+        self.assertEqual(2, len(json.loads(body)))
+
+        req.query_string = 'limit=100000000000000000000000'
+        status, headers, body = self.call_ov(req)
+        self.assertEqual(status, '412 Precondition Failed')
+
     def test_list_containers_prefix(self):
         listing_body = [{
             'bytes': 0,

test/unit/common/test_request_helpers.py

@@ -29,6 +29,19 @@ server_types = ['account', 'container', 'object']
 
 
 class TestRequestHelpers(unittest.TestCase):
+
+    def test_constrain_req_limit(self):
+        req = Request.blank('')
+        self.assertEqual(10, rh.constrain_req_limit(req, 10))
+        req = Request.blank('', query_string='limit=1')
+        self.assertEqual(1, rh.constrain_req_limit(req, 10))
+        req = Request.blank('', query_string='limit=1.0')
+        self.assertEqual(10, rh.constrain_req_limit(req, 10))
+        req = Request.blank('', query_string='limit=11')
+        with self.assertRaises(HTTPException) as raised:
+            rh.constrain_req_limit(req, 10)
+        self.assertEqual(raised.exception.status_int, 412)
+
     def test_is_user_meta(self):
         m_type = 'meta'
         for st in server_types: