fix copying for paused versioning buckets (#7548)
* fix copying for paused versioning buckets * copy for non versioned files * add tests * better tests * Update weed/s3api/s3api_object_handlers_copy.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * remove etag * update * Update s3api_object_handlers_copy_test.go * Update weed/s3api/s3api_object_handlers_copy_test.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update weed/s3api/s3api_object_handlers_copy_test.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * revert --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This commit is contained in:
Chris Lu
@@ -230,10 +230,11 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
|
||||
}
|
||||
}
|
||||
|
||||
// Check if destination bucket has versioning configured
|
||||
dstVersioningConfigured, err := s3a.isVersioningConfigured(dstBucket)
|
||||
// Check if destination bucket has versioning enabled
|
||||
// Only create versions if versioning is explicitly "Enabled", not "Suspended" or unconfigured
|
||||
dstVersioningState, err := s3a.getVersioningState(dstBucket)
|
||||
if err != nil {
|
||||
glog.Errorf("Error checking versioning status for destination bucket %s: %v", dstBucket, err)
|
||||
glog.Errorf("Error checking versioning state for destination bucket %s: %v", dstBucket, err)
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
|
||||
return
|
||||
}
|
||||
@@ -241,7 +242,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
|
||||
var dstVersionId string
|
||||
var etag string
|
||||
|
||||
if dstVersioningConfigured {
|
||||
if shouldCreateVersionForCopy(dstVersioningState) {
|
||||
// For versioned destination, create a new version
|
||||
dstVersionId = generateVersionId()
|
||||
glog.V(2).Infof("CopyObjectHandler: creating version %s for destination %s/%s", dstVersionId, dstBucket, dstObject)
|
||||
@@ -294,6 +295,9 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
|
||||
w.Header().Set("x-amz-version-id", dstVersionId)
|
||||
} else {
|
||||
// For non-versioned destination, use regular copy
|
||||
// Remove any versioning-related metadata from source that shouldn't carry over
|
||||
cleanupVersioningMetadata(dstEntry.Extended)
|
||||
|
||||
dstPath := util.FullPath(fmt.Sprintf("%s/%s%s", s3a.option.BucketsPath, dstBucket, dstObject))
|
||||
dstDir, dstName := dstPath.DirAndName()
|
||||
|
||||
@@ -2327,6 +2331,25 @@ func (ctx *EncryptionHeaderContext) shouldSkipEncryptedToUnencryptedHeader() boo
|
||||
return hasSourceEncryption && !hasDestinationEncryption && isAnyEncryptionHeader
|
||||
}
|
||||
|
||||
// cleanupVersioningMetadata removes versioning-related metadata from Extended attributes
|
||||
// when copying to non-versioned or suspended-versioning buckets.
|
||||
// This prevents objects in non-versioned buckets from carrying invalid versioning metadata.
|
||||
// It also removes the source ETag to prevent metadata inconsistency, as a new ETag will be
|
||||
// calculated for the destination object.
|
||||
func cleanupVersioningMetadata(metadata map[string][]byte) {
|
||||
delete(metadata, s3_constants.ExtVersionIdKey)
|
||||
delete(metadata, s3_constants.ExtDeleteMarkerKey)
|
||||
delete(metadata, s3_constants.ExtIsLatestKey)
|
||||
delete(metadata, s3_constants.ExtETagKey)
|
||||
}
|
||||
|
||||
// shouldCreateVersionForCopy determines whether a version should be created during a copy operation
|
||||
// based on the destination bucket's versioning state.
|
||||
// Returns true only if versioning is explicitly "Enabled", not "Suspended" or unconfigured.
|
||||
func shouldCreateVersionForCopy(versioningState string) bool {
|
||||
return versioningState == s3_constants.VersioningEnabled
|
||||
}
|
||||
|
||||
// shouldSkipEncryptionHeader determines if a header should be skipped when copying extended attributes
|
||||
// based on the source and destination encryption types. This consolidates the repetitive logic for
|
||||
// filtering encryption-related headers during copy operations.
|
||||
|
||||
@@ -436,3 +436,206 @@ func transferHeaderToH(data map[string][]string) H {
|
||||
}
|
||||
return m
|
||||
}
|
||||
|
||||
// TestShouldCreateVersionForCopy tests the production function that determines
|
||||
// whether a version should be created during a copy operation.
|
||||
// This addresses issue #7505 where copies were incorrectly creating versions for non-versioned buckets.
|
||||
func TestShouldCreateVersionForCopy(t *testing.T) {
|
||||
testCases := []struct {
|
||||
name string
|
||||
versioningState string
|
||||
expectedResult bool
|
||||
description string
|
||||
}{
|
||||
{
|
||||
name: "VersioningEnabled",
|
||||
versioningState: s3_constants.VersioningEnabled,
|
||||
expectedResult: true,
|
||||
description: "Should create versions in .versions/ directory when versioning is Enabled",
|
||||
},
|
||||
{
|
||||
name: "VersioningSuspended",
|
||||
versioningState: s3_constants.VersioningSuspended,
|
||||
expectedResult: false,
|
||||
description: "Should NOT create versions when versioning is Suspended",
|
||||
},
|
||||
{
|
||||
name: "VersioningNotConfigured",
|
||||
versioningState: "",
|
||||
expectedResult: false,
|
||||
description: "Should NOT create versions when versioning is not configured",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
// Call the actual production function
|
||||
result := shouldCreateVersionForCopy(tc.versioningState)
|
||||
|
||||
if result != tc.expectedResult {
|
||||
t.Errorf("Test case %s failed: %s\nExpected shouldCreateVersionForCopy(%q)=%v, got %v",
|
||||
tc.name, tc.description, tc.versioningState, tc.expectedResult, result)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestCleanupVersioningMetadata tests the production function that removes versioning metadata.
|
||||
// This ensures objects copied to non-versioned buckets don't carry invalid versioning metadata
|
||||
// or stale ETag values from the source.
|
||||
func TestCleanupVersioningMetadata(t *testing.T) {
|
||||
testCases := []struct {
|
||||
name string
|
||||
sourceMetadata map[string][]byte
|
||||
expectedKeys []string // Keys that should be present after cleanup
|
||||
removedKeys []string // Keys that should be removed
|
||||
}{
|
||||
{
|
||||
name: "RemovesAllVersioningMetadata",
|
||||
sourceMetadata: map[string][]byte{
|
||||
s3_constants.ExtVersionIdKey: []byte("version-123"),
|
||||
s3_constants.ExtDeleteMarkerKey: []byte("false"),
|
||||
s3_constants.ExtIsLatestKey: []byte("true"),
|
||||
s3_constants.ExtETagKey: []byte("\"abc123\""),
|
||||
"X-Amz-Meta-Custom": []byte("value"),
|
||||
},
|
||||
expectedKeys: []string{"X-Amz-Meta-Custom"},
|
||||
removedKeys: []string{s3_constants.ExtVersionIdKey, s3_constants.ExtDeleteMarkerKey, s3_constants.ExtIsLatestKey, s3_constants.ExtETagKey},
|
||||
},
|
||||
{
|
||||
name: "HandlesEmptyMetadata",
|
||||
sourceMetadata: map[string][]byte{},
|
||||
expectedKeys: []string{},
|
||||
removedKeys: []string{s3_constants.ExtVersionIdKey, s3_constants.ExtDeleteMarkerKey, s3_constants.ExtIsLatestKey, s3_constants.ExtETagKey},
|
||||
},
|
||||
{
|
||||
name: "PreservesNonVersioningMetadata",
|
||||
sourceMetadata: map[string][]byte{
|
||||
s3_constants.ExtVersionIdKey: []byte("version-456"),
|
||||
s3_constants.ExtETagKey: []byte("\"def456\""),
|
||||
"X-Amz-Meta-Custom": []byte("value1"),
|
||||
"X-Amz-Meta-Another": []byte("value2"),
|
||||
s3_constants.ExtIsLatestKey: []byte("true"),
|
||||
},
|
||||
expectedKeys: []string{"X-Amz-Meta-Custom", "X-Amz-Meta-Another"},
|
||||
removedKeys: []string{s3_constants.ExtVersionIdKey, s3_constants.ExtETagKey, s3_constants.ExtIsLatestKey},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
// Create a copy of the source metadata
|
||||
dstMetadata := make(map[string][]byte)
|
||||
for k, v := range tc.sourceMetadata {
|
||||
dstMetadata[k] = v
|
||||
}
|
||||
|
||||
// Call the actual production function
|
||||
cleanupVersioningMetadata(dstMetadata)
|
||||
|
||||
// Verify expected keys are present
|
||||
for _, key := range tc.expectedKeys {
|
||||
if _, exists := dstMetadata[key]; !exists {
|
||||
t.Errorf("Expected key %s to be present in destination metadata", key)
|
||||
}
|
||||
}
|
||||
|
||||
// Verify removed keys are absent
|
||||
for _, key := range tc.removedKeys {
|
||||
if _, exists := dstMetadata[key]; exists {
|
||||
t.Errorf("Expected key %s to be removed from destination metadata, but it's still present", key)
|
||||
}
|
||||
}
|
||||
|
||||
// Verify the count matches to ensure no extra keys are present
|
||||
if len(dstMetadata) != len(tc.expectedKeys) {
|
||||
t.Errorf("Expected %d metadata keys, but got %d. Extra keys might be present.", len(tc.expectedKeys), len(dstMetadata))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestCopyVersioningIntegration validates the interaction between
|
||||
// shouldCreateVersionForCopy and cleanupVersioningMetadata functions.
|
||||
// This integration test ensures the complete fix for issue #7505.
|
||||
func TestCopyVersioningIntegration(t *testing.T) {
|
||||
testCases := []struct {
|
||||
name string
|
||||
versioningState string
|
||||
sourceMetadata map[string][]byte
|
||||
expectVersionPath bool
|
||||
expectMetadataKeys []string
|
||||
}{
|
||||
{
|
||||
name: "EnabledPreservesMetadata",
|
||||
versioningState: s3_constants.VersioningEnabled,
|
||||
sourceMetadata: map[string][]byte{
|
||||
s3_constants.ExtVersionIdKey: []byte("v123"),
|
||||
"X-Amz-Meta-Custom": []byte("value"),
|
||||
},
|
||||
expectVersionPath: true,
|
||||
expectMetadataKeys: []string{
|
||||
s3_constants.ExtVersionIdKey,
|
||||
"X-Amz-Meta-Custom",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "SuspendedCleansMetadata",
|
||||
versioningState: s3_constants.VersioningSuspended,
|
||||
sourceMetadata: map[string][]byte{
|
||||
s3_constants.ExtVersionIdKey: []byte("v123"),
|
||||
"X-Amz-Meta-Custom": []byte("value"),
|
||||
},
|
||||
expectVersionPath: false,
|
||||
expectMetadataKeys: []string{
|
||||
"X-Amz-Meta-Custom",
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "NotConfiguredCleansMetadata",
|
||||
versioningState: "",
|
||||
sourceMetadata: map[string][]byte{
|
||||
s3_constants.ExtVersionIdKey: []byte("v123"),
|
||||
s3_constants.ExtDeleteMarkerKey: []byte("false"),
|
||||
"X-Amz-Meta-Custom": []byte("value"),
|
||||
},
|
||||
expectVersionPath: false,
|
||||
expectMetadataKeys: []string{
|
||||
"X-Amz-Meta-Custom",
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
// Test version creation decision using production function
|
||||
shouldCreateVersion := shouldCreateVersionForCopy(tc.versioningState)
|
||||
if shouldCreateVersion != tc.expectVersionPath {
|
||||
t.Errorf("shouldCreateVersionForCopy(%q) = %v, expected %v",
|
||||
tc.versioningState, shouldCreateVersion, tc.expectVersionPath)
|
||||
}
|
||||
|
||||
// Test metadata cleanup using production function
|
||||
metadata := make(map[string][]byte)
|
||||
for k, v := range tc.sourceMetadata {
|
||||
metadata[k] = v
|
||||
}
|
||||
|
||||
if !shouldCreateVersion {
|
||||
cleanupVersioningMetadata(metadata)
|
||||
}
|
||||
|
||||
// Verify only expected keys remain
|
||||
for _, expectedKey := range tc.expectMetadataKeys {
|
||||
if _, exists := metadata[expectedKey]; !exists {
|
||||
t.Errorf("Expected key %q to be present in metadata", expectedKey)
|
||||
}
|
||||
}
|
||||
|
||||
// Verify the count matches (no extra keys)
|
||||
if len(metadata) != len(tc.expectMetadataKeys) {
|
||||
t.Errorf("Expected %d metadata keys, got %d", len(tc.expectMetadataKeys), len(metadata))
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user