From 6ffc486fd1ebbc2d2d9448f5292770cc2feaf40f Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Mon, 27 Nov 2023 16:30:19 +0100 Subject: [PATCH] rgw/sfs: Fix list-objects check for common-prefixes Fixes a list-objects issue that happened when calculating if a query should be truncated or not when having common prefixes. The proposed code change is using the last valid entry (entry being either the last object name or common prefix) as the next marker for the call that checks if there are more items. That way we are taking into account items that were already added to the results and we're not repeating them. (Also the truncation is calculated fine). Fixes: https://github.com/aquarist-labs/s3gw/issues/838 Signed-off-by: Xavi Garcia --- .../store/sfs/tests/test-sfs-list-objects.py | 24 +++++++++++++++++++ src/rgw/driver/sfs/bucket.cc | 4 ++-- src/rgw/driver/sfs/sqlite/sqlite_list.cc | 9 +++++-- src/rgw/driver/sfs/sqlite/sqlite_list.h | 3 ++- 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/qa/rgw/store/sfs/tests/test-sfs-list-objects.py b/qa/rgw/store/sfs/tests/test-sfs-list-objects.py index 52499c59d4143..6f577b5af6dfa 100644 --- a/qa/rgw/store/sfs/tests/test-sfs-list-objects.py +++ b/qa/rgw/store/sfs/tests/test-sfs-list-objects.py @@ -108,6 +108,7 @@ def check_list_response_prefix(self, list_resp, objects_expected, prefix, common self.assertTrue(common_prefix["Prefix"] in common_prefixes) else: self.assertFalse("CommonPrefixes" in list_resp) + self.assertFalse(list_resp["IsTruncated"]) def test_list_objects_no_prefix(self): @@ -260,6 +261,29 @@ def test_list_objects_with_delimiter(self): ] self.check_list_response_prefix(objs_ret, expected_objects, 'directory/', expected_common_prefixes) + def test_list_objects_with_delimiter_object_name_is_greater(self): + bucket_name = self.get_random_bucket_name() + self.s3_client.create_bucket(Bucket=bucket_name) + self.assert_bucket_exists(bucket_name) + # objects to upload + objects = [ + 'b/test', + 'test', + 'test1', + 'test2' + ] + for obj in objects: + self.upload_file_and_check(bucket_name, obj) + # list all objects with prefix equal to 'prefix' + objs_ret = self.s3_client.list_objects(Bucket=bucket_name, Prefix='', Delimiter = '/') + expected_objects = [ + 'test', 'test1', 'test2' + ] + expected_common_prefixes = [ + 'b/' + ] + self.check_list_response_prefix(objs_ret, expected_objects, '', expected_common_prefixes) + if __name__ == '__main__': if len(sys.argv) == 2: address_port = sys.argv.pop() diff --git a/src/rgw/driver/sfs/bucket.cc b/src/rgw/driver/sfs/bucket.cc index ccf69599c27b1..d073837d875ba 100644 --- a/src/rgw/driver/sfs/bucket.cc +++ b/src/rgw/driver/sfs/bucket.cc @@ -244,7 +244,7 @@ int SFSBucket::list( if (!params.delim.empty()) { std::vector new_results; - list.roll_up_common_prefixes( + auto next_marker = list.roll_up_common_prefixes( params.prefix, params.delim, results.objs, results.common_prefixes, new_results ); @@ -254,7 +254,7 @@ int SFSBucket::list( // anything after the prefix. if (!results.common_prefixes.empty()) { std::vector objects_after; - std::string query = std::prev(results.common_prefixes.end())->first; + std::string query = next_marker; query.append( sfs::S3_MAX_OBJECT_NAME_BYTES - query.size(), std::numeric_limits::max() diff --git a/src/rgw/driver/sfs/sqlite/sqlite_list.cc b/src/rgw/driver/sfs/sqlite/sqlite_list.cc index 8798042f73cdb..e9be683f8d9a9 100644 --- a/src/rgw/driver/sfs/sqlite/sqlite_list.cc +++ b/src/rgw/driver/sfs/sqlite/sqlite_list.cc @@ -155,7 +155,7 @@ bool SQLiteList::versions( return true; } -void SQLiteList::roll_up_common_prefixes( +std::string SQLiteList::roll_up_common_prefixes( const std::string& find_after_prefix, const std::string& delimiter, const std::vector& objects, std::map& out_common_prefixes, @@ -163,9 +163,10 @@ void SQLiteList::roll_up_common_prefixes( ) const { const size_t find_after_pos = find_after_prefix.length(); const size_t delim_len = delimiter.length(); + std::string next_marker = ""; if (delimiter.empty()) { out_objects = objects; - return; + return ""; } const std::string* prefix{nullptr}; // Last added prefix for (size_t i = 0; i < objects.size(); i++) { @@ -181,12 +182,16 @@ void SQLiteList::roll_up_common_prefixes( prefix = &out_common_prefixes .emplace(name.substr(0, delim_pos + delim_len), true) .first->first; + next_marker = *prefix; continue; } } // Not found -> next out_objects.push_back(objects[i]); + next_marker = objects[i].key.name; } + + return next_marker; } } // namespace rgw::sal::sfs::sqlite diff --git a/src/rgw/driver/sfs/sqlite/sqlite_list.h b/src/rgw/driver/sfs/sqlite/sqlite_list.h index f22cb4d0ca06f..bb537a7b6a8fc 100644 --- a/src/rgw/driver/sfs/sqlite/sqlite_list.h +++ b/src/rgw/driver/sfs/sqlite/sqlite_list.h @@ -52,7 +52,8 @@ class SQLiteList { // `delimiter` position. // // Copy all other `objects` to `out_objects`. - void roll_up_common_prefixes( + // Returns the next marker to be used in a subsequent call to list objects + std::string roll_up_common_prefixes( const std::string& find_after_prefix, const std::string& delimiter, const std::vector& objects, std::map& out_common_prefixes,