From 917f27c7b3800ae8e5e8ace7b06ae6160e95b560 Mon Sep 17 00:00:00 2001 From: David Steele Date: Tue, 15 Sep 2026 10:33:08 +0300 Subject: [PATCH] Fix pagination of S3 version listing. Listing with a repository target time uses the ListObjectVersions API but pagination read NextContinuationToken from the response, which is only returned by ListObjectsV2. A version listing truncated at the maximum key count failed with "unable to find child 'NextContinuationToken':0 in node 'ListVersionsResult'", so a repository with more than a thousand versions under a prefix could not be read at a target time. Paginate the version listing with NextKeyMarker and NextVersionIdMarker instead. NextVersionIdMarker is missing or empty when the listing is truncated at a common prefix, in which case the next page starts at the key after the key marker, which is what is needed. Build the query for each page from the base query so a marker set for one page is not reused for the next. The mock response used to test version listing pagination returned a continuation token rather than markers, so the error was not caught. --- doc/xml/release/2020s/2026/2.60.0.xml | 14 ++++++++ src/storage/s3/storage.c | 41 ++++++++++++++++++--- test/src/module/storage/s3Test.c | 52 +++++++++++++++++++++------ 3 files changed, 91 insertions(+), 16 deletions(-) diff --git a/doc/xml/release/2020s/2026/2.60.0.xml b/doc/xml/release/2020s/2026/2.60.0.xml index 22510234c..9d44d4cc6 100644 --- a/doc/xml/release/2020s/2026/2.60.0.xml +++ b/doc/xml/release/2020s/2026/2.60.0.xml @@ -12,6 +12,20 @@

Fix annotate command on a remote encrypted repository.

+ + + + + + + + + + + +

Fix pagination of S3 version listing.

+ + diff --git a/src/storage/s3/storage.c b/src/storage/s3/storage.c index ef7737f43..039492b79 100644 --- a/src/storage/s3/storage.c +++ b/src/storage/s3/storage.c @@ -49,8 +49,10 @@ S3 query tokens STRING_STATIC(S3_QUERY_CONTINUATION_TOKEN_STR, "continuation-token"); STRING_STATIC(S3_QUERY_DELETE_STR, "delete"); STRING_STATIC(S3_QUERY_DELIMITER_STR, "delimiter"); +STRING_STATIC(S3_QUERY_KEY_MARKER_STR, "key-marker"); STRING_STATIC(S3_QUERY_LIST_TYPE_STR, "list-type"); STRING_STATIC(S3_QUERY_PREFIX_STR, "prefix"); +STRING_STATIC(S3_QUERY_VERSION_ID_MARKER_STR, "version-id-marker"); STRING_STATIC(S3_QUERY_VERSIONS_STR, "versions"); STRING_STATIC(S3_QUERY_VALUE_LIST_TYPE_2_STR, "2"); @@ -68,6 +70,9 @@ STRING_STATIC(S3_XML_TAG_KEY_STR, "Key"); STRING_STATIC(S3_XML_TAG_LAST_MODIFIED_STR, "LastModified"); #define S3_XML_TAG_NEXT_CONTINUATION_TOKEN "NextContinuationToken" STRING_STATIC(S3_XML_TAG_NEXT_CONTINUATION_TOKEN_STR, S3_XML_TAG_NEXT_CONTINUATION_TOKEN); +#define S3_XML_TAG_NEXT_KEY_MARKER "NextKeyMarker" +STRING_STATIC(S3_XML_TAG_NEXT_KEY_MARKER_STR, S3_XML_TAG_NEXT_KEY_MARKER); +STRING_STATIC(S3_XML_TAG_NEXT_VERSION_ID_MARKER_STR, "NextVersionIdMarker"); STRING_STATIC(S3_XML_TAG_OBJECT_STR, "Object"); STRING_STATIC(S3_XML_TAG_PREFIX_STR, "Prefix"); STRING_STATIC(S3_XML_TAG_QUIET_STR, "Quiet"); @@ -839,16 +844,42 @@ storageS3ListInternal( // If list is truncated then send an async request to get more data if (strEq(xmlNodeContent(xmlNodeChild(xmlRoot, S3_XML_TAG_IS_TRUNCATED_STR, true)), TRUE_STR)) { - const String *const nextContinuationToken = xmlNodeContent( - xmlNodeChild(xmlRoot, S3_XML_TAG_NEXT_CONTINUATION_TOKEN_STR, true)); - CHECK(FormatError, !strEmpty(nextContinuationToken), S3_XML_TAG_NEXT_CONTINUATION_TOKEN " may not be empty"); + // Build the query for the next page from the base query since the markers are set per page + HttpQuery *const queryNext = httpQueryDupP(query); - httpQueryPut(query, S3_QUERY_CONTINUATION_TOKEN_STR, nextContinuationToken); + // Version listing is paginated with key and version id markers + if (targetTime != 0) + { + const String *const nextKeyMarker = xmlNodeContent( + xmlNodeChild(xmlRoot, S3_XML_TAG_NEXT_KEY_MARKER_STR, true)); + CHECK(FormatError, !strEmpty(nextKeyMarker), S3_XML_TAG_NEXT_KEY_MARKER " may not be empty"); + + httpQueryAdd(queryNext, S3_QUERY_KEY_MARKER_STR, nextKeyMarker); + + // The version id marker may be missing or empty when the next result is a common prefix. In that case the + // next page starts at the key after the key marker, which is what is needed. + const String *const nextVersionIdMarker = xmlNodeContent( + xmlNodeChild(xmlRoot, S3_XML_TAG_NEXT_VERSION_ID_MARKER_STR, false)); + + if (nextVersionIdMarker != NULL && !strEmpty(nextVersionIdMarker)) + httpQueryAdd(queryNext, S3_QUERY_VERSION_ID_MARKER_STR, nextVersionIdMarker); + } + // Else object listing is paginated with a continuation token + else + { + const String *const nextContinuationToken = xmlNodeContent( + xmlNodeChild(xmlRoot, S3_XML_TAG_NEXT_CONTINUATION_TOKEN_STR, true)); + CHECK( + FormatError, !strEmpty(nextContinuationToken), + S3_XML_TAG_NEXT_CONTINUATION_TOKEN " may not be empty"); + + httpQueryAdd(queryNext, S3_QUERY_CONTINUATION_TOKEN_STR, nextContinuationToken); + } // Store request in the outer temp context MEM_CONTEXT_PRIOR_BEGIN() { - request = storageS3RequestAsyncP(this, HTTP_VERB_GET_STR, FSLASH_STR, .query = query); + request = storageS3RequestAsyncP(this, HTTP_VERB_GET_STR, FSLASH_STR, .query = queryNext); } MEM_CONTEXT_PRIOR_END(); } diff --git a/test/src/module/storage/s3Test.c b/test/src/module/storage/s3Test.c index ed2b1b503..9cb8de6d0 100644 --- a/test/src/module/storage/s3Test.c +++ b/test/src/module/storage/s3Test.c @@ -1900,14 +1900,16 @@ testRun(void) // ----------------------------------------------------------------------------------------------------------------- TEST_TITLE("list with time limit"); + // Truncated in the middle of the versions for a key so both markers are returned testRequestP(service, s3, HTTP_VERB_GET, "/?delimiter=%2F&prefix=path%2Fto%2F&versions=", .requesterPays = true); testResponseP( service, .content = "" - "" + "" " true" - " 1ueG" + " path/to/test_file" + " aaaa" " " " path/to/test_file" " 2024-08-04T02:54:10.000Z" @@ -1916,17 +1918,21 @@ testRun(void) " path/to/test_file" " 2024-08-04T02:54:10.000Z" " " - ""); + ""); + // Truncated at a key boundary with an empty version id marker testRequestP( - service, s3, HTTP_VERB_GET, "/?continuation-token=1ueG&delimiter=%2F&prefix=path%2Fto%2F&versions=", + service, s3, HTTP_VERB_GET, + "/?delimiter=%2F&key-marker=path%2Fto%2Ftest_file&prefix=path%2Fto%2F&version-id-marker=aaaa&versions=", .requesterPays = true); testResponseP( service, .content = "" - "" - " false" + "" + " true" + " path/to/test_file2" + " " " " " path/to/test_file" " 2024-08-04T02:54:09.000Z" @@ -1937,14 +1943,38 @@ testRun(void) " path/to/test_file" " 2024-08-04T02:54:09.000Z" " " + ""); + + // Truncated at a common prefix so no version id marker is returned + testRequestP( + service, s3, HTTP_VERB_GET, + "/?delimiter=%2F&key-marker=path%2Fto%2Ftest_file2&prefix=path%2Fto%2F&versions=", .requesterPays = true); + testResponseP( + service, + .content = + "" + "" + " true" + " path/to/test_path/" " " " path/to/test_file2" " 2024-08-04T02:54:10.000Z" " " + ""); + + testRequestP( + service, s3, HTTP_VERB_GET, + "/?delimiter=%2F&key-marker=path%2Fto%2Ftest_path%2F&prefix=path%2Fto%2F&versions=", .requesterPays = true); + testResponseP( + service, + .content = + "" + "" + " false" " " " path/to/test_path/" " " - ""); + ""); TEST_STORAGE_LIST( s3, "/path/to", @@ -1960,7 +1990,7 @@ testRun(void) service, .content = "" - "" + "" " false" " " " path/3/test_file" @@ -1975,7 +2005,7 @@ testRun(void) " " " path/3/test_path/" " " - ""); + ""); testRequestP(service, s3, HTTP_VERB_GET, "/path/3/test_file?versionId=bbbb", .requesterPays = true); testResponseP(service, .content = "123456"); @@ -1991,7 +2021,7 @@ testRun(void) service, .content = "" - "" + "" " false" " " " missing_file" @@ -2004,7 +2034,7 @@ testRun(void) " " " test_path/" " " - ""); + ""); TEST_RESULT_PTR( storageGetP(storageNewReadP(s3, STRDEF("missing_file"), .ignoreMissing = true)), NULL, "missing file");