diff --git a/weed/s3api/s3api_object_handlers_list_versioned_test.go b/weed/s3api/s3api_object_handlers_list_versioned_test.go index fdadcfe57..443ae5d3e 100644 --- a/weed/s3api/s3api_object_handlers_list_versioned_test.go +++ b/weed/s3api/s3api_object_handlers_list_versioned_test.go @@ -3,6 +3,7 @@ package s3api import ( "context" "encoding/hex" + "encoding/xml" "fmt" "strconv" "strings" @@ -432,6 +433,86 @@ func (c *customTestFilerClient) ListEntries(ctx context.Context, in *filer_pb.Li return c.testFilerClient.ListEntries(ctx, in, opts...) } +// TestListObjectVersionsResult_XMLInterleaving validates that Version and DeleteMarker elements +// are interleaved in the correct sort order in the XML output, matching the S3 API contract. +func TestListObjectVersionsResult_XMLInterleaving(t *testing.T) { + now := time.Now() + earlier := now.Add(-24 * time.Hour) + + dm := &DeleteMarkerEntry{ + Key: "object1.txt", + VersionId: "newer-dm", + IsLatest: true, + LastModified: now, + } + ver := &VersionEntry{ + Key: "object1.txt", + VersionId: "older-version", + IsLatest: false, + LastModified: earlier, + ETag: "\"abc\"", + Size: 100, + StorageClass: "STANDARD", + } + + // Entries are in correct sort order: newest first for same key + result := &S3ListObjectVersionsResult{ + Name: "test-bucket", + MaxKeys: 1000, + Entries: []VersionListEntry{ + {DeleteMarker: dm}, + {Version: ver}, + }, + } + + xmlBytes, err := xml.Marshal(result) + assert.NoError(t, err) + xmlStr := string(xmlBytes) + + // Verify DeleteMarker appears BEFORE Version in the XML output + dmIdx := strings.Index(xmlStr, "") + vIdx := strings.Index(xmlStr, "") + assert.True(t, dmIdx >= 0, "DeleteMarker element should be present") + assert.True(t, vIdx >= 0, "Version element should be present") + assert.True(t, dmIdx < vIdx, "DeleteMarker should appear before Version for same key (newest first), got DM at %d, V at %d", dmIdx, vIdx) +} + +// TestListObjectVersionsResult_XMLInterleavingMultipleKeys validates interleaving across multiple keys +func TestListObjectVersionsResult_XMLInterleavingMultipleKeys(t *testing.T) { + now := time.Now() + earlier := now.Add(-24 * time.Hour) + + dm := &DeleteMarkerEntry{Key: "aaa", VersionId: "dm1", IsLatest: true, LastModified: now} + v1 := &VersionEntry{Key: "aaa", VersionId: "v1", IsLatest: false, LastModified: earlier, StorageClass: "STANDARD"} + v2 := &VersionEntry{Key: "bbb", VersionId: "v2", IsLatest: true, LastModified: now, StorageClass: "STANDARD"} + + // Expected XML order: aaa/DM, aaa/V, bbb/V + result := &S3ListObjectVersionsResult{ + Name: "test-bucket", + MaxKeys: 1000, + Entries: []VersionListEntry{ + {DeleteMarker: dm}, + {Version: v1}, + {Version: v2}, + }, + } + + xmlBytes, err := xml.Marshal(result) + assert.NoError(t, err) + xmlStr := string(xmlBytes) + + // Find positions of each element + dmIdx := strings.Index(xmlStr, "") + v1Idx := strings.Index(xmlStr, "aaa") + v2Idx := strings.Index(xmlStr, "bbb") + + assert.True(t, dmIdx >= 0, "DeleteMarker should be present") + assert.True(t, v1Idx >= 0, "Version for aaa should be present") + assert.True(t, v2Idx >= 0, "Version for bbb should be present") + assert.True(t, dmIdx < v1Idx, "aaa DeleteMarker should appear before aaa Version") + assert.True(t, v1Idx < v2Idx, "aaa Version should appear before bbb Version") +} + // TestListObjectVersions_PrefixWithLeadingSlash tests that prefixes with leading slashes work correctly // This validates the fix for the bug where "/Veeam/Archive/" would fail to match relative paths func TestListObjectVersions_PrefixWithLeadingSlash(t *testing.T) { diff --git a/weed/s3api/s3api_object_versioning.go b/weed/s3api/s3api_object_versioning.go index 10b909442..1cfd9d2f0 100644 --- a/weed/s3api/s3api_object_versioning.go +++ b/weed/s3api/s3api_object_versioning.go @@ -82,9 +82,14 @@ func clearCachedListMetadata(extended map[string][]byte) { clearCachedVersionMetadata(extended) } -// S3ListObjectVersionsResult - Custom struct for S3 list-object-versions response -// This avoids conflicts with the XSD generated ListVersionsResult struct -// and ensures proper separation of versions and delete markers into arrays +// S3ListObjectVersionsResult - Custom struct for S3 list-object-versions response. +// This avoids conflicts with the XSD generated ListVersionsResult struct. +// +// The Entries slice holds Version, DeleteMarker, and CommonPrefix items in their +// correct interleaved sort order (by key ascending, then newest version first). +// Each entry uses a per-element MarshalXML to output the correct XML element name. +// This ensures the XML output matches the S3 API contract where these elements +// are interleaved in sort order, not grouped by type. type S3ListObjectVersionsResult struct { XMLName xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ ListVersionsResult"` @@ -98,29 +103,37 @@ type S3ListObjectVersionsResult struct { Delimiter string `xml:"Delimiter,omitempty"` IsTruncated bool `xml:"IsTruncated"` - // These are the critical fields - arrays instead of single elements - Versions []VersionEntry `xml:"Version,omitempty"` // Array for versions - DeleteMarkers []DeleteMarkerEntry `xml:"DeleteMarker,omitempty"` // Array for delete markers + // Entries holds all versions, delete markers, and common prefixes in their + // correct interleaved sort order. Each entry's MarshalXML outputs the correct + // XML element name (, , or ). + Entries []VersionListEntry `xml:"Version"` + + EncodingType string `xml:"EncodingType,omitempty"` +} - CommonPrefixes []PrefixEntry `xml:"CommonPrefixes,omitempty"` - EncodingType string `xml:"EncodingType,omitempty"` +// VersionListEntry represents a single item in the ListObjectVersions response. +// It wraps either a VersionEntry, DeleteMarkerEntry, or PrefixEntry and outputs +// the correct XML element name via custom MarshalXML. +type VersionListEntry struct { + Version *VersionEntry + DeleteMarker *DeleteMarkerEntry + Prefix *PrefixEntry } -// Original struct - keeping for compatibility but will use S3ListObjectVersionsResult for XML response -type ListObjectVersionsResult struct { - XMLName xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ ListVersionsResult"` - Name string `xml:"Name"` - Prefix string `xml:"Prefix"` - KeyMarker string `xml:"KeyMarker,omitempty"` - VersionIdMarker string `xml:"VersionIdMarker,omitempty"` - NextKeyMarker string `xml:"NextKeyMarker,omitempty"` - NextVersionIdMarker string `xml:"NextVersionIdMarker,omitempty"` - MaxKeys int `xml:"MaxKeys"` - Delimiter string `xml:"Delimiter,omitempty"` - IsTruncated bool `xml:"IsTruncated"` - Versions []VersionEntry `xml:"Version,omitempty"` - DeleteMarkers []DeleteMarkerEntry `xml:"DeleteMarker,omitempty"` - CommonPrefixes []PrefixEntry `xml:"CommonPrefixes,omitempty"` +// MarshalXML outputs the entry as , , or +// depending on which field is populated. This ensures elements are interleaved in +// their correct sort order in the XML response. +func (e VersionListEntry) MarshalXML(enc *xml.Encoder, start xml.StartElement) error { + if e.DeleteMarker != nil { + start.Name.Local = "DeleteMarker" + return enc.EncodeElement(e.DeleteMarker, start) + } + if e.Prefix != nil { + start.Name.Local = "CommonPrefixes" + return enc.EncodeElement(e.Prefix, start) + } + start.Name.Local = "Version" + return enc.EncodeElement(e.Version, start) } // ObjectVersion represents a version of an S3 object @@ -260,7 +273,7 @@ func (s3a *S3ApiServer) listObjectVersions(bucket, prefix, keyMarker, versionIdM // Build the final response by splitting items back into their respective fields result := s3a.splitIntoResult(truncatedList, bucket, prefix, keyMarker, versionIdMarker, delimiter, maxKeys, isTruncated, nextKeyMarker, nextVersionIdMarker) - glog.V(1).Infof("listObjectVersions: final result - %d versions, %d delete markers, %d common prefixes", len(result.Versions), len(result.DeleteMarkers), len(result.CommonPrefixes)) + glog.V(1).Infof("listObjectVersions: final result - %d entries", len(result.Entries)) return result, nil } @@ -329,7 +342,8 @@ func (s3a *S3ApiServer) truncateAndSetMarkers(combinedList []versionListItem, ma return combinedList, nextKeyMarker, nextVersionIdMarker, isTruncated } -// splitIntoResult builds the final S3ListObjectVersionsResult from the combined list +// splitIntoResult builds the final S3ListObjectVersionsResult from the combined list. +// It populates a single Entries slice that preserves the interleaved sort order. func (s3a *S3ApiServer) splitIntoResult(combinedList []versionListItem, bucket, prefix, keyMarker, versionIdMarker, delimiter string, maxKeys int, isTruncated bool, nextKeyMarker, nextVersionIdMarker string) *S3ListObjectVersionsResult { result := &S3ListObjectVersionsResult{ Name: bucket, @@ -341,20 +355,20 @@ func (s3a *S3ApiServer) splitIntoResult(combinedList []versionListItem, bucket, IsTruncated: isTruncated, NextKeyMarker: nextKeyMarker, NextVersionIdMarker: nextVersionIdMarker, - Versions: make([]VersionEntry, 0), - DeleteMarkers: make([]DeleteMarkerEntry, 0), - CommonPrefixes: make([]PrefixEntry, 0), + Entries: make([]VersionListEntry, 0, len(combinedList)), } for _, item := range combinedList { if item.isPrefix { - result.CommonPrefixes = append(result.CommonPrefixes, PrefixEntry{Prefix: item.key}) + result.Entries = append(result.Entries, VersionListEntry{ + Prefix: &PrefixEntry{Prefix: item.key}, + }) } else { switch v := item.versionData.(type) { case *VersionEntry: - result.Versions = append(result.Versions, *v) + result.Entries = append(result.Entries, VersionListEntry{Version: v}) case *DeleteMarkerEntry: - result.DeleteMarkers = append(result.DeleteMarkers, *v) + result.Entries = append(result.Entries, VersionListEntry{DeleteMarker: v}) } } }