fix listing object versions #7006
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @chrislusf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses an issue where directory entries were not correctly included when listing object versions via the S3 API. The changes ensure that the ListObjectVersions operation now returns directory objects with properties consistent with AWS S3 and Minio behavior, such as a null VersionId and a size of 0.
Highlights
- S3 API Compatibility: Modified the
ListObjectVersionsimplementation to explicitly include directory entries in the response, aligning with standard S3 and Minio behavior for versioned buckets. - New Test Case: Added a comprehensive Go test (
TestListObjectVersionsIncludesDirectories) to verify that directory entries are correctly returned byListObjectVersionswith the expected metadata (e.g.,VersionId: "null",Size: 0, specific ETag).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to fix the listing of object versions to include directory entries, aligning with S3 behavior. The implementation correctly adds directory entries during the recursive version search. A new integration test is added to verify this behavior.
My review focuses on improving the new test's correctness, robustness, and readability. The most critical issue is that the test is not runnable due to a placeholder S3 client implementation. Additionally, there are opportunities to improve the test's efficiency by optimizing loops and removing redundancy, and to follow best practices by replacing context.TODO. In the application code, a magic string for an ETag could be extracted into a constant for better maintainability.
| func setupS3Client() *s3.Client { | ||
| // This should be implemented based on existing test setup | ||
| // Usually involves setting up endpoint, credentials, etc. | ||
| return nil // Placeholder - implement based on existing pattern | ||
| } |
There was a problem hiding this comment.
The setupS3Client function is a placeholder and returns nil. This will cause a panic (nil pointer dereference) when client.CreateBucket is called on line 23. The test is currently not runnable.
Please implement this function based on the existing test patterns in your project to correctly initialize an S3 client. The same applies to the cleanupBucket function.
| client := setupS3Client() | ||
|
|
||
| // Create bucket | ||
| _, err := client.CreateBucket(context.TODO(), &s3.CreateBucketInput{ |
There was a problem hiding this comment.
context.TODO() is used in multiple places in this test (lines 23, 34, 54, 63). It's generally better practice to use context.Background() in tests when you don't need a specific context with deadlines or cancellation. This makes the intent clearer.
| _, err := client.CreateBucket(context.TODO(), &s3.CreateBucketInput{ | |
| _, err := client.CreateBucket(context.Background(), &s3.CreateBucketInput{ |
| for _, expectedDir := range expectedDirectories { | ||
| found := false | ||
| for _, version := range listResp.Versions { | ||
| if *version.Key == expectedDir { | ||
| found = true | ||
| // Verify directory properties | ||
| assert.Equal(t, "null", *version.VersionId, "Directory %s should have VersionId 'null'", expectedDir) | ||
| assert.Equal(t, int64(0), *version.Size, "Directory %s should have size 0", expectedDir) | ||
| assert.True(t, *version.IsLatest, "Directory %s should be marked as latest", expectedDir) | ||
| assert.Equal(t, "\"d41d8cd98f00b204e9800998ecf8427e\"", *version.ETag, "Directory %s should have MD5 of empty string as ETag", expectedDir) | ||
| assert.Equal(t, types.ObjectStorageClassStandard, version.StorageClass, "Directory %s should have STANDARD storage class", expectedDir) | ||
| break | ||
| } | ||
| } | ||
| assert.True(t, found, "Directory %s should be included in list-object-versions response", expectedDir) | ||
| } |
There was a problem hiding this comment.
This loop iterates over all versions for each expected directory, resulting in O(N*M) complexity where N is the number of expected directories and M is the number of versions. A more efficient and readable approach would be to first convert the list of returned versions into a map for O(1) lookups. This would improve the test's performance, especially if the number of versions grows. The same applies to the file verification loop on lines 106-117.
Example refactoring:
// Convert versions to a map for efficient lookup
versionsMap := make(map[string]types.ObjectVersion)
for _, version := range listResp.Versions {
versionsMap[*version.Key] = version
}
// Verify directories
for _, expectedDir := range expectedDirectories {
version, found := versionsMap[expectedDir]
assert.True(t, found, "Directory %s should be included", expectedDir)
if found {
// ... assert properties on 'version'
}
}
// ... similar logic for files| // Count directories vs files | ||
| directoryCount := 0 | ||
| fileCount := 0 | ||
| for _, version := range listResp.Versions { | ||
| if strings.HasSuffix(*version.Key, "/") && *version.Size == 0 && *version.VersionId == "null" { | ||
| directoryCount++ | ||
| } else { | ||
| fileCount++ | ||
| } | ||
| } | ||
|
|
||
| t.Logf("Found %d directories and %d files", directoryCount, fileCount) | ||
| assert.Equal(t, len(expectedDirectories), directoryCount, "Should find exactly %d directories", len(expectedDirectories)) | ||
| assert.Equal(t, len(testObjects), fileCount, "Should find exactly %d files", len(testObjects)) |
There was a problem hiding this comment.
This block of code iterates through all the versions again just to count directories and files. This is redundant, as this information can be gathered during the verification loops on lines 88-117. You could increment counters within those loops to avoid this extra iteration, making the test cleaner and slightly more efficient.
Example:
directoryCount := 0
for _, expectedDir := range expectedDirectories {
// ... find logic ...
if found {
directoryCount++
// ... assertions ...
}
}
// ... similar for fileCount ...
assert.Equal(t, len(expectedDirectories), directoryCount, "...")
assert.Equal(t, len(testObjects), fileCount, "...")| glog.V(2).Infof("findVersionsRecursively: found directory %s", directoryKey) | ||
|
|
||
| // Calculate ETag for empty directory (same as Minio: MD5 of empty string) | ||
| directoryETag := "\"d41d8cd98f00b204e9800998ecf8427e\"" |
There was a problem hiding this comment.
This hardcoded ETag value is a "magic string". For better maintainability and readability, it's recommended to define this as a package-level constant (e.g., in the s3_constants package) and reference it here. This makes the code's intent clearer and centralizes the value if it ever needs to be changed.
For example, in s3_constants:
const EmptyObjectETag = "\"d41d8cd98f00b204e9800998ecf8427e\""Then you could use s3_constants.EmptyObjectETag here.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes object versioning functionality in the S3 API implementation. The main issue addressed is that the ListObjectVersionsResult struct was incorrectly defining arrays as single elements, causing delete markers to be categorized as versions instead of being properly separated.
- Introduces a new
S3ListObjectVersionsResultstruct with proper array definitions for versions and delete markers - Improves versioning state handling to distinguish between enabled and suspended versioning
- Fixes directory handling in versioned buckets and ACL operations for versioned objects
- Adds comprehensive test coverage for directory versioning scenarios
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| weed/s3api/s3api_object_versioning.go | Replaces ListObjectVersionsResult with S3ListObjectVersionsResult to fix array handling and improves delete marker separation |
| weed/s3api/s3api_object_handlers_put.go | Adds object owner setting for directory objects |
| weed/s3api/s3api_object_handlers_list.go | Enhances directory handling in list operations with versioned object support |
| weed/s3api/s3api_object_handlers_delete.go | Improves versioning state handling for proper suspended vs enabled versioning behavior |
| weed/s3api/s3api_object_handlers_acl.go | Fixes ACL operations to work with versioned objects |
| weed/s3api/s3api_object_handlers.go | Updates owner extraction to use S3 metadata instead of file system attributes |
| test/s3/versioning/s3_directory_versioning_test.go | Adds comprehensive test suite for directory versioning scenarios |
| .github/workflows/s3tests.yml | Updates test configuration to include versioning tests |
Comments suppressed due to low confidence (3)
test/s3/versioning/s3_directory_versioning_test.go:762
- The cleanupBucket function calls deleteAllObjectVersions but this function is not defined in the visible code. This could cause test failures if the function doesn't exist or isn't imported.
func cleanupBucket(t *testing.T, client *s3.Client, bucketName string) {
What problem are we solving?
How are we solving the problem?
How is the PR tested?
Checks