Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Empty delimiter should be treated as null to match AWS, delimiter at start of key is ignored #306

Closed
alwaysmpe opened this issue Nov 22, 2021 · 0 comments · Fixed by #312
Assignees
Labels

Comments

@alwaysmpe
Copy link

alwaysmpe commented Nov 22, 2021

There's an inconsistency between this implementation's interpretation of delimiters and the AWS S3 api, as well as what seems to be a bug or two.

    • (AWS) When making a list_objects_v2 query, if there's an empty delimiter specified, AWS ignores it, returning the same as were there no delimiter specified (I've tested this against AWS).
    • (S3 Mock) This mock has inconsistent behaviour. If the prefix is empty, this mock returns all keys separately within the contents. If the prefix is not empty, this mock treats all keys matching that prefix as containing the delimiter, and combines them into a single CommonPrefixes (that is the Prefix within the user request).
    • (AWS) When making a list_objects_v2 query, if the delimiter is non-empty and occurs at index zero of the key, AWS splits this into a separate CommonPrefixes entry. Likewise, if it occurs at an index greater than zero, this is split into another separate CommonPrefixes entry (I've not tested this against AWS, but guessing).
    • (S3 Mock) When the delimiter is non-empty and occurs at index zero of the key, this mock doesn't separate this into a common prefix, instead outputting the full key (even if the delimiter occurs multiple times within the key). If the delimiter occurs at an index greater than zero, this case is handled correctly.

This implementation only checks for the delimiter being null:


Where I assume it should use
StringUtils.isEmpty
from docs which treats an empty string and a null string the same.

The above then calls the collapseCommonPrefixes function

private void collapseCommonPrefixes(final String queryPrefix, final String delimiter,
final List<BucketContents> contents, final Set<String> commonPrefixes) {
final String normalizedQueryPrefix = queryPrefix == null ? "" : queryPrefix;
for (final Iterator<BucketContents> i = contents.iterator(); i.hasNext(); ) {
final BucketContents c = i.next();
final String key = c.getKey();
if (key.startsWith(normalizedQueryPrefix)) {
final int delimiterIndex = key.indexOf(delimiter, normalizedQueryPrefix.length());
if (delimiterIndex > 0) {
commonPrefixes.add(key.substring(0, delimiterIndex + delimiter.length()));
i.remove();
}
}
}
}

This function normalizes the prefix then tests for the index of the above delimiter in each key, after the prefix.
For an empty delimiter, as the empty string is present in any string this will return a non-negative number for any key containing the prefix (where -1 indicates the string isn't present). Hence it's necessary to change the above conditional to use StringUtils.isEmpty. The index returned will be the index after the prefix, and the same for every key containing the prefix meaning they're all combined into the same CommonPrefixes.

The above conditional


Also causes another problem. In the case of a zero-length QueryPrefix, if the delimiter is at index zero, eg (delimiter '/'):

/some/object/key
/some/other/key

These keys contain the delimiter but are ignored by the above conditional, resulting in the full keys being listed. By changing the conditional to
if (delimiterIndex >= 0) {
This should work correctly as indexOf returns -1 when the key is not present.

Don't mind creating a pull request, but it's been a while since I've coded in java.

@afranken afranken self-assigned this Nov 23, 2021
@afranken afranken added the bug label Nov 23, 2021
afranken added a commit that referenced this issue Nov 23, 2021
afranken added a commit that referenced this issue Dec 22, 2021
Replacing the "modify incoming iterator" pattern in

Fixes #306
afranken added a commit that referenced this issue Dec 22, 2021
Also replacing the "modify incoming iterator" pattern in
FileStoreController#collapseCommonPrefixes which is an anti-pattern
and should be avoided at all costs.

Fixes #306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants