diff --git a/CHANGELOG.md b/CHANGELOG.md index a5bbcdeca..7da941290 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ This document outlines major changes between releases. - Fix parsing `key-marker` for object list versions (#243) - Fix marshaling errors in `DeleteObjects` method (#222) - Fix status code in GET/HEAD delete marker (#226) +- Fix `NextVersionIDMarker` in `list-object-versions` (#248) ### Added - Add `trace_id` value into log record when tracing is enabled (#142) diff --git a/api/errors/errors.go b/api/errors/errors.go index 33ec96e3b..d9c2bfaa6 100644 --- a/api/errors/errors.go +++ b/api/errors/errors.go @@ -183,6 +183,7 @@ const ( ErrInvalidRequest ErrInvalidRequestLargeCopy ErrInvalidStorageClass + VersionIDMarkerWithoutKeyMarker ErrMalformedJSON ErrInsecureClientRequest @@ -314,6 +315,12 @@ var errorCodes = errorCodeMap{ Description: "Invalid storage class.", HTTPStatusCode: http.StatusBadRequest, }, + VersionIDMarkerWithoutKeyMarker: { + ErrCode: VersionIDMarkerWithoutKeyMarker, + Code: "VersionIDMarkerWithoutKeyMarker", + Description: "A version-id marker cannot be specified without a key marker.", + HTTPStatusCode: http.StatusBadRequest, + }, ErrInvalidRequestBody: { ErrCode: ErrInvalidRequestBody, Code: "InvalidArgument", diff --git a/api/handler/object_list.go b/api/handler/object_list.go index 5144b5c9e..88b269842 100644 --- a/api/handler/object_list.go +++ b/api/handler/object_list.go @@ -260,6 +260,10 @@ func parseListObjectVersionsRequest(reqInfo *middleware.ReqInfo) (*layer.ListObj res.Encode = queryValues.Get("encoding-type") res.VersionIDMarker = queryValues.Get("version-id-marker") + if res.VersionIDMarker != "" && res.KeyMarker == "" { + return nil, errors.GetAPIError(errors.VersionIDMarkerWithoutKeyMarker) + } + return &res, nil } diff --git a/api/handler/object_list_test.go b/api/handler/object_list_test.go index 983a281a2..c899511a1 100644 --- a/api/handler/object_list_test.go +++ b/api/handler/object_list_test.go @@ -210,6 +210,41 @@ func TestS3BucketListV2DelimiterPrefix(t *testing.T) { validateListV2(t, tc, bktName, prefix, delim, "", 2, false, true, []string{"boo/bar"}, []string{"boo/baz/"}) } +func TestMintVersioningListObjectVersionsVersionIDContinuation(t *testing.T) { + hc := prepareHandlerContext(t) + + bktName, objName := "mint-bucket-for-listing-versions", "objName" + createTestBucket(hc, bktName) + putBucketVersioning(t, hc, bktName, true) + + length := 10 + objects := make([]string, length) + for i := 0; i < length; i++ { + objects[i] = objName + putObject(hc, bktName, objName) + } + + maxKeys := 5 + + page1 := listObjectsVersions(hc, bktName, "", "", "", "", maxKeys) + require.Len(t, page1.Version, maxKeys) + checkVersionsNames(t, page1, objects) + require.Equal(t, page1.Version[maxKeys-1].VersionID, page1.NextVersionIDMarker) + require.True(t, page1.IsTruncated) + + page2 := listObjectsVersions(hc, bktName, "", "", page1.NextKeyMarker, page1.NextVersionIDMarker, maxKeys) + require.Len(t, page2.Version, maxKeys) + checkVersionsNames(t, page1, objects) + require.Empty(t, page2.NextVersionIDMarker) + require.False(t, page2.IsTruncated) +} + +func checkVersionsNames(t *testing.T, versions *ListObjectsVersionsResponse, names []string) { + for i, v := range versions.Version { + require.Equal(t, names[i], v.Key) + } +} + func listObjectsV2(hc *handlerContext, bktName, prefix, delimiter, startAfter, continuationToken string, maxKeys int) *ListObjectsV2Response { query := prepareCommonListObjectsQuery(prefix, delimiter, maxKeys) if len(startAfter) != 0 { diff --git a/api/layer/versioning.go b/api/layer/versioning.go index 9a4d0da54..2bf9b2bc6 100644 --- a/api/layer/versioning.go +++ b/api/layer/versioning.go @@ -5,6 +5,7 @@ import ( "sort" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data" + s3errors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" ) func (n *layer) ListObjectVersions(ctx context.Context, p *ListObjectVersionsParams) (*ListObjectVersionsInfo, error) { @@ -36,29 +37,58 @@ func (n *layer) ListObjectVersions(ctx context.Context, p *ListObjectVersionsPar } } - for i, obj := range allObjects { - if obj.ObjectInfo.Name >= p.KeyMarker && obj.ObjectInfo.VersionID() >= p.VersionIDMarker { - allObjects = allObjects[i:] - break - } + if allObjects, err = filterVersionsByMarker(allObjects, p); err != nil { + return nil, err } res.CommonPrefixes, allObjects = triageExtendedObjects(allObjects) if len(allObjects) > p.MaxKeys { res.IsTruncated = true - res.NextKeyMarker = allObjects[p.MaxKeys].ObjectInfo.Name - res.NextVersionIDMarker = allObjects[p.MaxKeys].ObjectInfo.VersionID() + res.NextKeyMarker = allObjects[p.MaxKeys-1].ObjectInfo.Name + res.NextVersionIDMarker = allObjects[p.MaxKeys-1].ObjectInfo.VersionID() allObjects = allObjects[:p.MaxKeys] - res.KeyMarker = allObjects[p.MaxKeys-1].ObjectInfo.Name - res.VersionIDMarker = allObjects[p.MaxKeys-1].ObjectInfo.VersionID() + res.KeyMarker = p.KeyMarker + res.VersionIDMarker = p.VersionIDMarker } res.Version, res.DeleteMarker = triageVersions(allObjects) return res, nil } +func filterVersionsByMarker(objects []*data.ExtendedObjectInfo, p *ListObjectVersionsParams) ([]*data.ExtendedObjectInfo, error) { + if p.KeyMarker == "" { + return objects, nil + } + + for i, obj := range objects { + if obj.ObjectInfo.Name == p.KeyMarker { + for j := i; j < len(objects); j++ { + if objects[j].ObjectInfo.Name != obj.ObjectInfo.Name { + if p.VersionIDMarker == "" { + return objects[j:], nil + } + break + } + if objects[j].ObjectInfo.VersionID() == p.VersionIDMarker { + return objects[j+1:], nil + } + } + return nil, s3errors.GetAPIError(s3errors.ErrInvalidVersion) + } else if obj.ObjectInfo.Name > p.KeyMarker { + if p.VersionIDMarker != "" { + return nil, s3errors.GetAPIError(s3errors.ErrInvalidVersion) + } + return objects[i:], nil + } + } + + // don't use nil as empty slice to be consistent with `return objects[j+1:], nil` above + // that can be empty + return []*data.ExtendedObjectInfo{}, nil +} + func triageVersions(objVersions []*data.ExtendedObjectInfo) ([]*data.ExtendedObjectInfo, []*data.ExtendedObjectInfo) { if len(objVersions) == 0 { return nil, nil diff --git a/api/layer/versioning_test.go b/api/layer/versioning_test.go index d59a3b995..471de3a38 100644 --- a/api/layer/versioning_test.go +++ b/api/layer/versioning_test.go @@ -12,6 +12,7 @@ import ( bearertest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer/test" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object" oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" + oidtest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id/test" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user" "github.com/nspcc-dev/neo-go/pkg/crypto/keys" "github.com/stretchr/testify/require" @@ -311,3 +312,133 @@ func TestNoVersioningDeleteObject(t *testing.T) { tc.getObject(tc.obj, "", true) tc.checkListObjects() } + +func TestFilterVersionsByMarker(t *testing.T) { + n := 10 + testOIDs := make([]oid.ID, n) + for i := 0; i < n; i++ { + testOIDs[i] = oidtest.ID() + } + + for _, tc := range []struct { + name string + objects []*data.ExtendedObjectInfo + params *ListObjectVersionsParams + expected []*data.ExtendedObjectInfo + error bool + }{ + { + name: "missed key marker", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "", VersionIDMarker: "dummy"}, + expected: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + }, + }, + { + name: "last version id", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj0", VersionIDMarker: testOIDs[1].EncodeToString()}, + expected: []*data.ExtendedObjectInfo{}, + }, + { + name: "same name, different versions", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj0", VersionIDMarker: testOIDs[0].EncodeToString()}, + expected: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + }, + }, + { + name: "different name, different versions", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[1]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj0", VersionIDMarker: testOIDs[0].EncodeToString()}, + expected: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[1]}}, + }, + }, + { + name: "not matched name alphabetically less", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[1]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj", VersionIDMarker: ""}, + expected: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[1]}}, + }, + }, + { + name: "not matched name alphabetically less with dummy version id", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj", VersionIDMarker: "dummy"}, + error: true, + }, + { + name: "not matched name alphabetically greater", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[1]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj2", VersionIDMarker: testOIDs[2].EncodeToString()}, + expected: []*data.ExtendedObjectInfo{}, + }, + { + name: "not found version id", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[2]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj0", VersionIDMarker: "dummy"}, + error: true, + }, + { + name: "not found version id, obj last", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj0", VersionIDMarker: "dummy"}, + error: true, + }, + { + name: "not found version id, obj last", + objects: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[0]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj0", ID: testOIDs[1]}}, + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[2]}}, + }, + params: &ListObjectVersionsParams{KeyMarker: "obj0", VersionIDMarker: ""}, + expected: []*data.ExtendedObjectInfo{ + {ObjectInfo: &data.ObjectInfo{Name: "obj1", ID: testOIDs[2]}}, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + actual, err := filterVersionsByMarker(tc.objects, tc.params) + if tc.error { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tc.expected, actual) + } + }) + } +}