test versioning also #7000
Merged
test versioning also #7000
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
Never-versioned buckets: No VersionId headers, no Status field Pre-versioning objects: Regular files, VersionId="null", included in all operations Post-versioning objects: Stored in .versions directories with real version IDs Suspended versioning: Proper status handling and null version IDs
Bucket Versioning Status Compliance Fixed: New buckets now return no Status field (AWS S3 compliant) Before: Always returned "Suspended" ❌ After: Returns empty VersioningConfiguration for unconfigured buckets ✅ 2. Multi-Object Delete Versioning Support Fixed: DeleteMultipleObjectsHandler now fully versioning-aware Before: Always deleted physical files, breaking versioning ❌ After: Creates delete markers or deletes specific versions properly ✅ Added: DeleteMarker field in response structure for AWS compatibility 3. Copy Operations Versioning Support Fixed: CopyObjectHandler and CopyObjectPartHandler now versioning-aware Before: Only copied regular files, couldn't handle versioned sources ❌ After: Parses version IDs from copy source, creates versions in destination ✅ Added: pathToBucketObjectAndVersion() function for version ID parsing 4. Pre-versioning Object Handling Fixed: getLatestObjectVersion() now has proper fallback logic Before: Failed when .versions directory didn't exist ❌ After: Falls back to regular objects for pre-versioning scenarios ✅ 5. Enhanced Object Version Listings Fixed: listObjectVersions() includes both versioned AND pre-versioning objects Before: Only showed .versions directories, ignored pre-versioning objects ❌ After: Shows complete version history with VersionId="null" for pre-versioning ✅ 6. Null Version ID Handling Fixed: getSpecificObjectVersion() properly handles versionId="null" Before: Couldn't retrieve pre-versioning objects by version ID ❌ After: Returns regular object files for "null" version requests ✅ 7. Version ID Response Headers Fixed: PUT operations only return x-amz-version-id when appropriate Before: Returned version IDs for non-versioned buckets ❌ After: Only returns version IDs for explicitly configured versioning ✅
…ersioning state; a GetBucketVersioning request does not return a versioning state value.
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive S3 object versioning support, transitioning from basic versioning configuration to a full-featured versioning system with proper state handling for enabled, suspended, and never-configured buckets.
Key changes:
- Enhanced version ID generation with chronological ordering using nanosecond timestamps
- Comprehensive versioning state management (enabled/suspended/never-configured) with proper AWS S3 compatibility
- Improved object listing with recursive directory traversal and duplicate detection
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| weed/s3api/s3api_object_versioning.go | Core versioning logic with new version ID generation, recursive listing, and version management |
| weed/s3api/s3api_object_handlers_put.go | PUT operation handling for different versioning states with suspended versioning support |
| weed/s3api/s3api_object_handlers_delete.go | DELETE operation updates for versioned buckets with proper delete marker handling |
| weed/s3api/s3api_object_handlers_copy.go | COPY operation enhancements with version-aware source handling |
| weed/s3api/s3api_object_handlers.go | GET/HEAD operation updates for versioned object retrieval |
| weed/s3api/s3api_bucket_handlers.go | Bucket versioning status handling with AWS-compliant empty responses |
| weed/s3api/s3api_bucket_config.go | Bucket configuration with new versioning state methods and error handling |
| weed/s3api/filer_multipart.go | Multipart upload completion with versioning support |
| test/s3/versioning/s3_versioning_test.go | Test updates for proper versioning status validation |
| Various config files | Volume size limit adjustments for testing |
Comments suppressed due to low confidence (1)
weed/s3api/s3api_object_handlers_delete.go:142
- [nitpick] The field name 'DeleteMarker' could be more descriptive. Consider 'IsDeleteMarker' to better indicate it's a boolean flag.
DeleteMarker bool `xml:"DeleteMarker,omitempty"`
| if _, err := rand.Read(randBytes); err != nil { | ||
| glog.Errorf("Failed to generate random bytes for version ID: %v", err) | ||
| return "" | ||
| // Fallback to timestamp-only if random generation fails | ||
| return timestampHex + "0000000000000000" |
There was a problem hiding this comment.
The hardcoded fallback string "0000000000000000" is a magic number. Consider defining this as a constant to improve maintainability and clarity.
Suggested change
| return timestampHex + "0000000000000000" | |
| return timestampHex + FallbackRandomHex |
Copilot uses AI. Check for mistakes.
|
|
||
| // Then by modification time (newest first) - but use nanosecond precision for ties | ||
| timeDiff := lastModifiedI.Sub(lastModifiedJ) | ||
| if timeDiff.Abs() > time.Millisecond { |
There was a problem hiding this comment.
The hardcoded time.Millisecond threshold should be defined as a constant to make it configurable and improve maintainability.
Copilot uses AI. Check for mistakes.
| if entry.Extended == nil { | ||
| entry.Extended = make(map[string][]byte) | ||
| } | ||
| entry.Extended[s3_constants.ExtVersionIdKey] = []byte("null") |
There was a problem hiding this comment.
The hardcoded string "null" for version ID should be defined as a constant to ensure consistency across the codebase and improve maintainability.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem are we solving?
How are we solving the problem?
How is the PR tested?
Checks