preserve Version and DeleteMarker sort order in the list XML response (#15819)

cmd/api-response.go

@@ -34,6 +34,7 @@ import (
 	"github.com/minio/minio/internal/hash"
 	xhttp "github.com/minio/minio/internal/http"
 	"github.com/minio/minio/internal/logger"
+	xxml "github.com/minio/xxml"
 )
 
 const (
@@ -90,8 +91,7 @@ type ListVersionsResponse struct {
 	IsTruncated bool
 
 	CommonPrefixes []CommonPrefix
-	DeleteMarkers  []DeleteMarkerVersion `xml:"DeleteMarker,omitempty"`
-	Versions       []ObjectVersion       `xml:"Version,omitempty"`
+	Versions       []ObjectVersion
 
 	// Encoding type used to encode object keys in the response.
 	EncodingType string `xml:"EncodingType,omitempty"`
@@ -256,6 +256,19 @@ type ObjectVersion struct {
 	Object
 	IsLatest  bool
 	VersionID string `xml:"VersionId"`
+
+	isDeleteMarker bool
+}
+
+// MarshalXML - marshal ObjectVersion
+func (o ObjectVersion) MarshalXML(e *xxml.Encoder, start xxml.StartElement) error {
+	if o.isDeleteMarker {
+		start.Name.Local = "DeleteMarker"
+	} else {
+		start.Name.Local = "Version"
+	}
+	type objectVersionWrapper ObjectVersion
+	return e.EncodeElement(objectVersionWrapper(o), start)
 }
 
 // DeleteMarkerVersion container for delete marker metadata
@@ -482,7 +495,6 @@ func generateListBucketsResponse(buckets []BucketInfo) ListBucketsResponse {
 // generates an ListBucketVersions response for the said bucket with other enumerated options.
 func generateListVersionsResponse(bucket, prefix, marker, versionIDMarker, delimiter, encodingType string, maxKeys int, resp ListObjectVersionsInfo) ListVersionsResponse {
 	versions := make([]ObjectVersion, 0, len(resp.Objects))
-	deleteMarkers := make([]DeleteMarkerVersion, 0, len(resp.Objects))
 
 	owner := Owner{
 		ID:          globalMinioDefaultOwnerID,
@@ -494,21 +506,6 @@ func generateListVersionsResponse(bucket, prefix, marker, versionIDMarker, delim
 		if object.Name == "" {
 			continue
 		}
-
-		if object.DeleteMarker {
-			deleteMarker := DeleteMarkerVersion{
-				Key:          s3EncodeName(object.Name, encodingType),
-				LastModified: object.ModTime.UTC().Format(iso8601TimeFormat),
-				Owner:        owner,
-				VersionID:    object.VersionID,
-			}
-			if deleteMarker.VersionID == "" {
-				deleteMarker.VersionID = nullVersionID
-			}
-			deleteMarker.IsLatest = object.IsLatest
-			deleteMarkers = append(deleteMarkers, deleteMarker)
-			continue
-		}
 		content := ObjectVersion{}
 		content.Key = s3EncodeName(object.Name, encodingType)
 		content.LastModified = object.ModTime.UTC().Format(iso8601TimeFormat)
@@ -527,12 +524,12 @@ func generateListVersionsResponse(bucket, prefix, marker, versionIDMarker, delim
 			content.VersionID = nullVersionID
 		}
 		content.IsLatest = object.IsLatest
+		content.isDeleteMarker = object.DeleteMarker
 		versions = append(versions, content)
 	}
 
 	data.Name = bucket
 	data.Versions = versions
-	data.DeleteMarkers = deleteMarkers
 	data.EncodingType = encodingType
 	data.Prefix = s3EncodeName(prefix, encodingType)
 	data.KeyMarker = s3EncodeName(marker, encodingType)

cmd/server_test.go

@@ -27,6 +27,7 @@ import (
 	"net/http"
 	"net/url"
 	"reflect"
+	"regexp"
 	"runtime"
 	"strings"
 	"sync"
@@ -102,6 +103,7 @@ func runAllTests(suite *TestSuiteCommon, c *check) {
 	suite.TestContentTypePersists(c)
 	suite.TestPartialContent(c)
 	suite.TestListObjectsHandler(c)
+	suite.TestListObjectVersionsOutputOrderHandler(c)
 	suite.TestListObjectsHandlerErrors(c)
 	suite.TestPutBucketErrors(c)
 	suite.TestGetObjectLarge10MiB(c)
@@ -1636,6 +1638,70 @@ func (s *TestSuiteCommon) TestListObjectsHandler(c *check) {
 	}
 }
 
+// TestListObjectVersionsHandler - checks the order of <Version>
+// and <DeleteMarker> XML tags in a version listing
+func (s *TestSuiteCommon) TestListObjectVersionsOutputOrderHandler(c *check) {
+	// generate a random bucket name.
+	bucketName := getRandomBucketName()
+	// HTTP request to create the bucket.
+	makeBucketRequest, err := newTestSignedRequest(http.MethodPut, getMakeBucketURL(s.endPoint, bucketName),
+		0, nil, s.accessKey, s.secretKey, s.signer)
+	c.Assert(err, nil)
+	// execute the HTTP request to create bucket.
+	response, err := s.client.Do(makeBucketRequest)
+	c.Assert(err, nil)
+	c.Assert(response.StatusCode, http.StatusOK)
+
+	// HTTP request to create the bucket.
+	enableVersioningBody := []byte("<VersioningConfiguration><Status>Enabled</Status></VersioningConfiguration>")
+	enableVersioningBucketRequest, err := newTestSignedRequest(http.MethodPut, getBucketVersioningConfigURL(s.endPoint, bucketName),
+		int64(len(enableVersioningBody)), bytes.NewReader(enableVersioningBody), s.accessKey, s.secretKey, s.signer)
+	c.Assert(err, nil)
+	// execute the HTTP request to create bucket.
+	response, err = s.client.Do(enableVersioningBucketRequest)
+	c.Assert(err, nil)
+	c.Assert(response.StatusCode, http.StatusOK)
+
+	for _, objectName := range []string{"file.1", "file.2"} {
+		buffer := bytes.NewReader([]byte("testcontent"))
+		putRequest, err := newTestSignedRequest(http.MethodPut, getPutObjectURL(s.endPoint, bucketName, objectName),
+			int64(buffer.Len()), buffer, s.accessKey, s.secretKey, s.signer)
+		c.Assert(err, nil)
+		response, err = s.client.Do(putRequest)
+		c.Assert(err, nil)
+		c.Assert(response.StatusCode, http.StatusOK)
+
+		delRequest, err := newTestSignedRequest(http.MethodDelete, getDeleteObjectURL(s.endPoint, bucketName, objectName),
+			0, nil, s.accessKey, s.secretKey, s.signer)
+		c.Assert(err, nil)
+		response, err = s.client.Do(delRequest)
+		c.Assert(err, nil)
+		c.Assert(response.StatusCode, http.StatusNoContent)
+	}
+
+	// create listObjectsV1 request with valid parameters
+	request, err := newTestSignedRequest(http.MethodGet, getListObjectVersionsURL(s.endPoint, bucketName, "", "1000", ""),
+		0, nil, s.accessKey, s.secretKey, s.signer)
+	c.Assert(err, nil)
+	// execute the HTTP request.
+	response, err = s.client.Do(request)
+	c.Assert(err, nil)
+	c.Assert(response.StatusCode, http.StatusOK)
+
+	getContent, err := io.ReadAll(response.Body)
+	c.Assert(err, nil)
+
+	r := regexp.MustCompile(
+		`<ListVersionsResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">.*` +
+			`<DeleteMarker><Key>file.1</Key>.*<IsLatest>true</IsLatest>.*</DeleteMarker>` +
+			`<Version><Key>file.1</Key>.*<IsLatest>false</IsLatest>.*</Version>` +
+			`<DeleteMarker><Key>file.2</Key>.*<IsLatest>true</IsLatest>.*</DeleteMarker>` +
+			`<Version><Key>file.2</Key>.*<IsLatest>false</IsLatest>.*</Version>` +
+			`</ListVersionsResult>`)
+
+	c.Assert(r.MatchString(string(getContent)), true)
+}
+
 // TestListObjectsSpecialCharactersHandler - Setting valid parameters to List Objects
 // and then asserting the response with the expected one.
 func (s *TestSuiteCommon) TestListObjectsSpecialCharactersHandler(c *check) {

cmd/test-utils_test.go

@@ -1321,6 +1321,13 @@ func getMakeBucketURL(endPoint, bucketName string) string {
 	return makeTestTargetURL(endPoint, bucketName, "", url.Values{})
 }
 
+// return URL for creating the bucket.
+func getBucketVersioningConfigURL(endPoint, bucketName string) string {
+	vals := make(url.Values)
+	vals.Set("versioning", "")
+	return makeTestTargetURL(endPoint, bucketName, "", vals)
+}
+
 // return URL for listing buckets.
 func getListBucketURL(endPoint string) string {
 	return makeTestTargetURL(endPoint, "", "", url.Values{})
@@ -1369,6 +1376,19 @@ func getListObjectsV1URL(endPoint, bucketName, prefix, maxKeys, encodingType str
 	return makeTestTargetURL(endPoint, bucketName, prefix, queryValue)
 }
 
+// return URL for listing objects in the bucket with V1 legacy API.
+func getListObjectVersionsURL(endPoint, bucketName, prefix, maxKeys, encodingType string) string {
+	queryValue := url.Values{}
+	if maxKeys != "" {
+		queryValue.Set("max-keys", maxKeys)
+	}
+	if encodingType != "" {
+		queryValue.Set("encoding-type", encodingType)
+	}
+	queryValue.Set("versions", "")
+	return makeTestTargetURL(endPoint, bucketName, prefix, queryValue)
+}
+
 // return URL for listing objects in the bucket with V2 API.
 func getListObjectsV2URL(endPoint, bucketName, prefix, maxKeys, fetchOwner, encodingType string) string {
 	queryValue := url.Values{}