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

do not remove md in DelimeterFilter then add it by commonPrefixes; just keep it #188

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

jixinchi
Copy link
Contributor

If the metadata ends with delimiter, it is removed in DelimterFilter.apply, then add it back by loop commonPrefixes. But many information in the metadata lost, such as lastModified time. My solution is keeping them in the DelimeterFilter.

@jixinchi
Copy link
Contributor Author

This PR is related to gaul/s3proxy#574

@nacx nacx requested a review from gaul November 14, 2023 03:19
Copy link
Member

@gaul gaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you exercise this change with some test?

@@ -364,6 +364,9 @@ private SortedSet<StorageMetadata> extractCommonPrefixes(SortedSet<StorageMetada
o = prefix + o;
}
md.setName(o + delimiter);
if (!contents.contains(md)) {
contents.add(md);
}
contents.add(md);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this add md twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a typo and I removed it. Thanks!

@jixinchi
Copy link
Contributor Author

Can you exercise this change with some test?

I add a test testListBlobEndsWithDelimiterAndDelimiterFilter

contents.add(md);
if (!contents.contains(md)) {
contents.add(md);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contents is a Set so this is the same as before. Could you revert it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is reverted now. I thought that item will be replaced if I add an existing item in Set, but actually it will be ignored.

@gaul gaul merged commit 9b01dbc into apache:master Nov 14, 2023
1 check passed
@gaul
Copy link
Member

gaul commented Nov 14, 2023

Thank you for your contribution @jixinchi!

@jixinchi jixinchi deleted the fix/delimeter branch November 14, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants