Skip to content

Commit fdca4e6

Browse files
Add test coverage and make code simpler to understand
Issue: CLDSRV-669
1 parent c859be7 commit fdca4e6

File tree

3 files changed

+95
-33
lines changed

3 files changed

+95
-33
lines changed

lib/api/apiUtils/object/abortMultipartUpload.js

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
101101
// to honor the uploadId filter in standardMetadataValidateBucketAndObj
102102
// ensuring the objMD returned has the right uploadId. But this is not
103103
// supported by Metadata.
104-
function ensureCleanupIsRequired(mpuBucket, storedParts, destBucket,
104+
function findObjectToCleanup(mpuBucket, storedParts, destBucket,
105105
objectMD, skipDataDelete, next) {
106106
if (!objectMD) {
107107
return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete);
@@ -124,29 +124,29 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
124124
if (err) {
125125
log.warn('error finding object version by uploadId, proceeding without cleanup', {
126126
error: err,
127-
method: 'abortMultipartUpload.ensureCleanupIsRequired',
127+
method: 'abortMultipartUpload.findObjectToCleanup',
128128
});
129129
// On error, continue the abort without an objectMD to clean up.
130130
return next(null, mpuBucket, storedParts, destBucket, null, skipDataDelete);
131131
}
132132
return next(null, mpuBucket, storedParts, destBucket, foundVersion, skipDataDelete);
133133
});
134134
},
135-
function deleteObjectMetadata(mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID,
135+
function deleteObjectMetadata(mpuBucket, storedParts, destBucket, objectMD,
136136
skipDataDelete, next) {
137-
if (!objMDWithMatchingUploadID) {
138-
return next(null, mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID, skipDataDelete);
137+
if (!objectMD) {
138+
return next(null, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete);
139139
}
140140

141-
log.debug('Object has existing metadata with matching uploadId, deleting them', {
141+
log.debug('Object has existing metadata, deleting them', {
142142
method: 'abortMultipartUpload',
143143
bucketName,
144144
objectKey,
145145
uploadId,
146-
versionId: objMDWithMatchingUploadID.versionId,
146+
versionId: objectMD.versionId,
147147
});
148148
return metadata.deleteObjectMD(bucketName, objectKey, {
149-
versionId: objMDWithMatchingUploadID.versionId,
149+
versionId: objectMD.versionId,
150150
}, log, err => {
151151
if (err) {
152152
// Handle concurrent deletion of this object metadata
@@ -155,18 +155,18 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
155155
method: 'abortMultipartUpload',
156156
bucketName,
157157
objectKey,
158-
versionId: objMDWithMatchingUploadID.versionId,
158+
versionId: objectMD.versionId,
159159
});
160160
} else {
161161
log.error('error deleting object metadata', { error: err });
162162
}
163163
}
164164
// Continue with the operation regardless of deletion success/failure
165165
// The important part is that we tried to clean up
166-
return next(null, mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID, skipDataDelete);
166+
return next(null, mpuBucket, storedParts, destBucket, objectMD, skipDataDelete);
167167
});
168168
},
169-
function deleteData(mpuBucket, storedParts, destBucket, objMDWithMatchingUploadID,
169+
function deleteData(mpuBucket, storedParts, destBucket, objectMD,
170170
skipDataDelete, next) {
171171
if (skipDataDelete) {
172172
return next(null, mpuBucket, storedParts, destBucket);
@@ -178,10 +178,10 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
178178
return next(null, mpuBucket, storedParts, destBucket);
179179
}
180180

181-
// Add object data locations if they exist and we can trust uploadId matches
182-
if (objMDWithMatchingUploadID?.location) {
181+
// Add object data locations if they exist
182+
if (objectMD?.location) {
183183
const existingLocations = new Set(locations.map(loc => loc.key));
184-
const remainingObjectLocations = objMDWithMatchingUploadID.
184+
const remainingObjectLocations = objectMD.
185185
location.filter(loc => !existingLocations.has(loc.key));
186186
locations.push(...remainingObjectLocations);
187187
}

tests/functional/aws-node-sdk/test/object/abortMPU.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ describe('Abort MPU - No Such Upload', () => {
293293
});
294294

295295
describe('Abort MPU - Versioned Bucket Cleanup', function testSuite() {
296-
this.timeout(120000);
296+
this.timeout(30000);
297297

298298
withV4(sigCfg => {
299299
let bucketUtil;

tests/unit/lib/services.spec.js

Lines changed: 80 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,54 +38,116 @@ describe('services', () => {
3838
});
3939
});
4040

41-
it('should correctly handle pagination', done => {
41+
it('should handle an error from getObjectListing', done => {
42+
const testError = new Error('listing failed');
43+
getObjectListingStub.yields(testError);
44+
45+
services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => {
46+
assert.deepStrictEqual(err, testError);
47+
sinon.assert.calledOnce(getObjectListingStub);
48+
done();
49+
});
50+
});
51+
52+
it('should return null if no matching version is found', done => {
53+
getObjectListingStub.yields(null, { Versions: [], IsTruncated: false });
54+
55+
services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, (err, foundVersion) => {
56+
assert.ifError(err);
57+
sinon.assert.calledOnce(getObjectListingStub);
58+
assert.strictEqual(foundVersion, null);
59+
done();
60+
});
61+
});
62+
63+
it('should find a version on the only page of results', done => {
64+
const uploadIdToFind = 'the-correct-upload-id';
65+
const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' };
66+
const versions = [
67+
{ key: objectKey, value: { uploadId: 'some-other-id' } },
68+
// Version with a different key but same uploadId to test the key check
69+
{ key: 'another-object-key', value: { uploadId: uploadIdToFind } },
70+
{ key: objectKey, value: correctVersionValue },
71+
];
72+
getObjectListingStub.yields(null, { Versions: versions, IsTruncated: false });
73+
74+
services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => {
75+
assert.ifError(err);
76+
sinon.assert.calledOnce(getObjectListingStub);
77+
assert.deepStrictEqual(foundVersion, correctVersionValue);
78+
done();
79+
});
80+
});
81+
82+
it('should read all pages if a matching version is not found', done => {
4283
getObjectListingStub.onFirstCall().yields(null, {
43-
Versions: [],
84+
Versions: [{ key: objectKey, value: { uploadId: 'id-page-1' } }],
4485
IsTruncated: true,
4586
NextKeyMarker: 'key-marker',
4687
NextVersionIdMarker: 'version-marker',
4788
});
4889
getObjectListingStub.onSecondCall().yields(null, {
49-
Versions: [],
90+
Versions: [{ key: objectKey, value: { uploadId: 'id-page-2' } }],
5091
IsTruncated: false,
5192
});
5293

53-
services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => {
94+
services.findObjectVersionByUploadId(bucketName, objectKey, 'non-existent-upload-id', log, (err, foundVersion) => {
5495
assert.ifError(err);
5596
sinon.assert.calledTwice(getObjectListingStub);
5697

5798
const secondCallParams = getObjectListingStub.getCall(1).args[1];
5899
assert.strictEqual(secondCallParams.keyMarker, 'key-marker');
59100
assert.strictEqual(secondCallParams.versionIdMarker, 'version-marker');
101+
assert.strictEqual(foundVersion, null);
60102
done();
61103
});
62104
});
63105

64-
it('should handle error from getObjectListing', done => {
65-
const testError = new Error('listing failed');
66-
getObjectListingStub.yields(testError);
106+
it('should find a version on the first page of many and stop listing', done => {
107+
const uploadIdToFind = 'the-correct-upload-id';
108+
const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' };
109+
const versions = [{ key: objectKey, value: correctVersionValue }];
110+
111+
getObjectListingStub.onFirstCall().yields(null, {
112+
Versions: versions,
113+
IsTruncated: true,
114+
NextKeyMarker: 'key-marker',
115+
NextVersionIdMarker: 'version-marker',
116+
});
117+
getObjectListingStub.onSecondCall().yields(new Error('should not have been called'));
67118

68-
services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => {
69-
assert.deepStrictEqual(err, testError);
119+
services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => {
120+
assert.ifError(err);
70121
sinon.assert.calledOnce(getObjectListingStub);
122+
assert.deepStrictEqual(foundVersion, correctVersionValue);
71123
done();
72124
});
73125
});
74126

75-
it('should find a version with the matching uploadId', done => {
127+
it('should find a version on a subsequent page', done => {
76128
const uploadIdToFind = 'the-correct-upload-id';
77129
const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' };
78-
const versions = [
79-
{ key: objectKey, value: { uploadId: 'some-other-id' } },
80-
// Version with a different key but same uploadId to test the key check
81-
{ key: 'another-object-key', value: { uploadId: uploadIdToFind } },
82-
{ key: objectKey, value: correctVersionValue },
83-
];
84-
getObjectListingStub.yields(null, { Versions: versions, IsTruncated: false });
130+
const secondPageVersions = [{ key: objectKey, value: correctVersionValue }];
131+
132+
getObjectListingStub.onFirstCall().yields(null, {
133+
Versions: [{ key: objectKey, value: { uploadId: 'some-other-id' } }],
134+
IsTruncated: true,
135+
NextKeyMarker: 'key-marker',
136+
NextVersionIdMarker: 'version-marker',
137+
});
138+
getObjectListingStub.onSecondCall().yields(null, {
139+
Versions: secondPageVersions,
140+
IsTruncated: false,
141+
});
85142

86143
services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => {
87144
assert.ifError(err);
88-
sinon.assert.calledOnce(getObjectListingStub);
145+
sinon.assert.calledTwice(getObjectListingStub);
146+
147+
const secondCallParams = getObjectListingStub.getCall(1).args[1];
148+
assert.strictEqual(secondCallParams.keyMarker, 'key-marker');
149+
assert.strictEqual(secondCallParams.versionIdMarker, 'version-marker');
150+
89151
assert.deepStrictEqual(foundVersion, correctVersionValue);
90152
done();
91153
});

0 commit comments

Comments
 (0)