Add corner case test coverage, and revert unneeded format changes

Issue: CLDSRV-699

Author: williamlardier

lib/api/apiUtils/object/abortMultipartUpload.js

@@ -29,7 +29,6 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
 
     async.waterfall([
         function checkDestBucketVal(next) {
-            // FIX: Call the function from the imported module object.
             metadataUtils.standardMetadataValidateBucketAndObj(metadataValParams, authzIdentityResult, log,
                 (err, destinationBucket, objectMD) => {
                     if (err) {
@@ -67,21 +66,21 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
             // eslint-disable-next-line no-param-reassign
             delete request.actionImplicitDenies;
             return data.abortMPU(objectKey, uploadId, location, bucketName,
-                request, destBucket, locationConstraintCheck, log,
-                (err, skipDataDelete) => {
-                    // eslint-disable-next-line no-param-reassign
-                    request.actionImplicitDenies = originalIdentityAuthzResults;
-                    if (err) {
-                        log.error('error aborting MPU', { error: err });
-                        return next(err, destBucket);
-                    }
-                    // for Azure and GCP we do not need to delete data
-                    // for all other backends, skipDataDelete will be set to false
-                    return next(null, mpuBucket, destBucket, objectMD, skipDataDelete);
-                });
+            request, destBucket, locationConstraintCheck, log,
+            (err, skipDataDelete) => {
+                // eslint-disable-next-line no-param-reassign
+                request.actionImplicitDenies = originalIdentityAuthzResults;
+                if (err) {
+                    log.error('error aborting MPU', { error: err });
+                    return next(err, destBucket);
+                }
+                // for Azure and GCP we do not need to delete data
+                // for all other backends, skipDataDelete will be set to false
+                return next(null, mpuBucket, destBucket, objectMD, skipDataDelete);
+            });
         },
         function getPartLocations(mpuBucket, destBucket, objectMD, skipDataDelete,
-            next) {
+        next) {
             services.getMPUparts(mpuBucket.getName(), uploadId, log,
                 (err, result) => {
                     if (err) {
@@ -90,7 +89,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
                     }
                     const storedParts = result.Contents;
                     return next(null, mpuBucket, storedParts, destBucket, objectMD,
-                        skipDataDelete);
+                    skipDataDelete);
                 });
         },
         // During Abort, we dynamically detect if the previous CompleteMPU call
@@ -123,7 +122,7 @@ function abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log,
             // Otherwise, list all versions to find one with a matching uploadId.
             return services.findObjectVersionByUploadId(bucketName, objectKey, uploadId, log, (err, foundVersion) => {
                 if (err) {
-                    log.error('error finding object version by uploadId, proceeding without cleanup', {
+                    log.warn('error finding object version by uploadId, proceeding without cleanup', {
                         error: err,
                         method: 'abortMultipartUpload.ensureCleanupIsRequired',
                     });

tests/unit/api/apiUtils/object/abortMultipartUpload.spec.js

@@ -124,6 +124,21 @@ describe('abortMultipartUpload', () => {
                 }, abortRequest);
             });
         });
+
+        it('should return error if data backend fails to abort', done => {
+            const testError = new Error('Data backend abort failed');
+            // This stub is now more explicit to avoid side-effects.
+            data.abortMPU.callsFake((objKey, upId, loc, bucket, req, destB, locCheckFn, log, cb) => {
+                cb(testError);
+            });
+            createBucketAndMPU(false, (err, uploadId) => {
+                assert.ifError(err);
+                abortMultipartUpload(authInfo, bucketName, objectKey, uploadId, log, err => {
+                    assert.deepStrictEqual(err, testError);
+                    done();
+                }, { ...abortRequest, query: { uploadId } });
+            });
+        });
     });
 
     describe('with multipart upload parts', () => {
@@ -182,6 +197,45 @@ describe('abortMultipartUpload', () => {
             });
         });
 
+        it('should NOT search for orphans on non-versioned bucket with mismatched master uploadId', done => {
+            const standardMetadataValidateStub = sinon.stub(metadataUtils, 'standardMetadataValidateBucketAndObj');
+            const mockBucket = {
+                isVersioningEnabled: () => false,
+                getOwner: () => 'testCanonicalId',
+                getName: () => bucketName,
+            };
+            const mockMasterMD = { 'uploadId': 'master-id' };
+            standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD);
+
+            abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => {
+                assert.ifError(err);
+                sinon.assert.notCalled(findObjectVersionStub);
+                done();
+            }, { ...abortRequest, query: { uploadId: 'abort-id' } });
+        });
+
+        it('should proceed without cleanup if finding object version fails', done => {
+            const testError = new Error('Find version failed');
+            findObjectVersionStub.yields(testError);
+            const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(null);
+
+            const standardMetadataValidateStub = sinon.stub(metadataUtils, 'standardMetadataValidateBucketAndObj');
+            const mockBucket = {
+                isVersioningEnabled: () => true,
+                getOwner: () => 'testCanonicalId',
+                getName: () => bucketName,
+            };
+            const mockMasterMD = { 'uploadId': 'master-id' };
+            standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD);
+
+            abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => {
+                assert.ifError(err);
+                sinon.assert.calledOnce(findObjectVersionStub);
+                sinon.assert.notCalled(deleteObjectMDStub);
+                done();
+            }, { ...abortRequest, query: { uploadId: 'abort-id' } });
+        });
+
         it('should delete the correct orphaned object version', done => {
             const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(null);
             findObjectVersionStub.yields(null, { uploadId: 'abort-id', versionId: 'orphan-vid' });
@@ -192,9 +246,7 @@ describe('abortMultipartUpload', () => {
                 getOwner: () => 'testCanonicalId',
                 getName: () => bucketName,
             };
-            const mockMasterMD = {
-                'uploadId': 'master-id',
-            };
+            const mockMasterMD = { 'uploadId': 'master-id' };
             standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD);
 
             abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => {
@@ -204,5 +256,25 @@ describe('abortMultipartUpload', () => {
                 done();
             }, { ...abortRequest, query: { uploadId: 'abort-id' } });
         });
+
+        it('should proceed if orphaned object version is already deleted (NoSuchKey)', done => {
+            const deleteObjectMDStub = sinon.stub(metadata, 'deleteObjectMD').yields(errors.NoSuchKey);
+            findObjectVersionStub.yields(null, { uploadId: 'abort-id', versionId: 'orphan-vid' });
+
+            const standardMetadataValidateStub = sinon.stub(metadataUtils, 'standardMetadataValidateBucketAndObj');
+            const mockBucket = {
+                isVersioningEnabled: () => true,
+                getOwner: () => 'testCanonicalId',
+                getName: () => bucketName,
+            };
+            const mockMasterMD = { 'uploadId': 'master-id' };
+            standardMetadataValidateStub.yields(null, mockBucket, mockMasterMD);
+
+            abortMultipartUpload(authInfo, bucketName, objectKey, 'abort-id', log, err => {
+                assert.ifError(err);
+                sinon.assert.calledOnce(deleteObjectMDStub);
+                done();
+            }, { ...abortRequest, query: { uploadId: 'abort-id' } });
+        });
     });
 });

tests/unit/lib/services.spec.js

@@ -60,5 +60,35 @@ describe('services', () => {
                 done();
             });
         });
+
+        it('should handle error from getObjectListing', done => {
+            const testError = new Error('listing failed');
+            getObjectListingStub.yields(testError);
+
+            services.findObjectVersionByUploadId(bucketName, objectKey, 'any-upload-id', log, err => {
+                assert.deepStrictEqual(err, testError);
+                sinon.assert.calledOnce(getObjectListingStub);
+                done();
+            });
+        });
+
+        it('should find a version with the matching uploadId', done => {
+            const uploadIdToFind = 'the-correct-upload-id';
+            const correctVersionValue = { uploadId: uploadIdToFind, data: 'this is it' };
+            const versions = [
+                { key: objectKey, value: { uploadId: 'some-other-id' } },
+                // Version with a different key but same uploadId to test the key check
+                { key: 'another-object-key', value: { uploadId: uploadIdToFind } },
+                { key: objectKey, value: correctVersionValue },
+            ];
+            getObjectListingStub.yields(null, { Versions: versions, IsTruncated: false });
+
+            services.findObjectVersionByUploadId(bucketName, objectKey, uploadIdToFind, log, (err, foundVersion) => {
+                assert.ifError(err);
+                sinon.assert.calledOnce(getObjectListingStub);
+                assert.deepStrictEqual(foundVersion, correctVersionValue);
+                done();
+            });
+        });
     });
 });