Merge 8c15714900 into 78a3441b30

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, "<DeleteMarker>")
+	vIdx := strings.Index(xmlStr, "<Version>")
+	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, "<DeleteMarker>")
+	v1Idx := strings.Index(xmlStr, "<Version><Key>aaa</Key>")
+	v2Idx := strings.Index(xmlStr, "<Version><Key>bbb</Key>")
+
+	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) {

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 (<Version>, <DeleteMarker>, or <CommonPrefixes>).
+	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 <Version>, <DeleteMarker>, or <CommonPrefixes>
+// 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})
 			}
 		}
 	}