From 8c1571490015ee698f4c490fc9bd435df3dc7e97 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 9 Mar 2026 09:27:56 -0700 Subject: [PATCH] fix: ListObjectVersions interleave Version and DeleteMarker in sort order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Go's default xml.Marshal serializes struct fields in definition order, causing all elements to appear before all elements. The S3 API contract requires these elements to be interleaved in the correct global sort order (by key ascending, then newest version first within each key). This broke clients that validate version list ordering within a single key — an older Version would appear before a newer DeleteMarker for the same object. Fix: Replace the separate Versions/DeleteMarkers/CommonPrefixes arrays with a single Entries []VersionListEntry slice. Each VersionListEntry uses a per-element MarshalXML that outputs the correct XML tag name (, , or ) based on which field is populated. Since the entries are already in their correct sorted order from buildSortedCombinedList, the XML output is automatically interleaved correctly. Also removes the unused ListObjectVersionsResult struct. Note: The reporter also mentioned a cross-key timestamp ordering issue when paginating with max-keys=1, but that is correct S3 behavior — ListObjectVersions sorts by key name (ascending), not by timestamp. Different keys having non-monotonic timestamps is expected. --- ...api_object_handlers_list_versioned_test.go | 81 +++++++++++++++++++ weed/s3api/s3api_object_versioning.go | 76 ++++++++++------- 2 files changed, 126 insertions(+), 31 deletions(-) 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}) } } }