Add delimiter support for S3 List API #2996
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2996 +/- ##
=============================================
- Coverage 64.24% 39.62% -24.62%
+ Complexity 10398 6281 -4117
=============================================
Files 840 884 +44
Lines 71755 74829 +3074
Branches 8611 8976 +365
=============================================
- Hits 46099 29651 -16448
- Misses 23004 42850 +19846
+ Partials 2652 2328 -324 ☔ View full report in Codecov by Sentry. |
| @@ -706,10 +709,27 @@ private Page<NamedBlobRecord> run_list_v2(String accountName, String containerNa | |||
| int resultIndex = 0; | |||
| while (resultSet.next()) { | |||
| String blobName = resultSet.getString(1); | |||
| if (resultIndex++ == maxKeysValue) { | |||
| if (resultIndex == maxKeysValue) { | |||
There was a problem hiding this comment.
we only set nextContinuationToken to the blobName when resultIndex == maxKeysValue. The idea is to only do that when it's the last key, but now that we change the logic when incrementing resultIndex. resultIndex would not equal to maxKeysValue in the last key, so we probably have to use an other index, or other way to set next continuation token here.
| // Extract the portion after the prefix and before the next '/' | ||
| String remainingPath = blobName.substring(blobNamePrefix == null ? 0 : blobNamePrefix.length()); | ||
| remainingPath = remainingPath.startsWith("/") ? remainingPath.substring(1) : remainingPath; | ||
| int delimiterIndex = remainingPath.indexOf("/"); |
There was a problem hiding this comment.
let's use a constant for "/".
| if (groupDirectories) { | ||
| // Extract the portion after the prefix and before the next '/' | ||
| String remainingPath = blobName.substring(blobNamePrefix == null ? 0 : blobNamePrefix.length()); | ||
| remainingPath = remainingPath.startsWith("/") ? remainingPath.substring(1) : remainingPath; |
There was a problem hiding this comment.
this is not right, if the prefix is "abc" and this blobname is "abc/efg/hij", and the delimiter is "/", then the we should "abc/" as common prefix, not "abc/efg/".
| if (groupDirectories) { | ||
| // Add the directories to the result | ||
| entries.addAll(directories.stream() | ||
| .map(directory -> new NamedBlobRecord(accountName, containerName, directory, null, Utils.Infinite_Time, 0, |
There was a problem hiding this comment.
This is not right, when we are returning a common prefix, it should include the prefix as well. if we have "abc/def/ghi", and the prefix is "abc/", and the common prefix should be "abc/def/", not jsut "def/".
| remainingPath = remainingPath.startsWith("/") ? remainingPath.substring(1) : remainingPath; | ||
| int delimiterIndex = remainingPath.indexOf("/"); | ||
| if (delimiterIndex != -1) { | ||
| boolean validEntry = directories.add(remainingPath.substring(0, delimiterIndex) + "/"); |
There was a problem hiding this comment.
Nit: it could be remainingPath.substring(0, delimiterIndex +DELIMITER_STRING.length());
This adds support for treating "/" in blob names as delimiter. It allows to treat prefixes as "subdirectories" and group them under "CommonPrefixes" response in the LIST API. This also enables us to list and delete directories using AWS S3 CLIs
More details can be found in https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html under "CommonPrefixes" section.